On 05/01/2016 01:27 PM, Simon Glass wrote:
Hi Stephen,
On 28 April 2016 at 17:08, Stephen Warren <swar...@wwwdotorg.org> wrote:
From: Stephen Warren <swar...@nvidia.com>
This will allow a driver's bind function to use the driver data. One
example is the Tegra186 GPIO driver, which instantiates child devices
for each of its GPIO ports, yet supports two different HW instances each
with a different set of ports, and identified by the udevice_id .data
field.
Signed-off-by: Stephen Warren <swar...@nvidia.com>
---
drivers/core/device.c | 7 ++++---
drivers/core/lists.c | 6 +++---
drivers/gpio/dwapb_gpio.c | 2 +-
drivers/gpio/s5p_gpio.c | 2 +-
drivers/gpio/sunxi_gpio.c | 2 +-
drivers/gpio/tegra_gpio.c | 2 +-
drivers/mtd/spi/sandbox.c | 2 +-
drivers/net/mvpp2.c | 3 ++-
drivers/pci/pci-uclass.c | 5 ++---
drivers/power/pmic/pmic-uclass.c | 2 +-
drivers/usb/host/usb-uclass.c | 5 ++---
include/dm/device-internal.h | 5 +++--
12 files changed, 22 insertions(+), 21 deletions(-)
I'm not sure this extra parameter carries its weight:
- most callers just pass 0
The same is true of the existing platdata field in many cases.
- the field is supposed to be set up by device tree and probing
tables, not code
While the existence of this new parameter does allow arbitrary code to
set the parameter, this patch only actually sets the parameter in the
case where DT and probing tables have determined that value.
- bind() methods should not care about the driver data (they are not
allowed to touch hardware), so setting it later is fine
Not touching HW is fine, but the driver data can still feed into purely
SW decisions that bind makes. More details below.
- you can already pass platform data to the driver which is the
preferred communication method from a parent to its children
I don't believe this is possible for devices instantiated from DT is it?
In that case, platform data is always NULL:
int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
struct udevice **devp)
...
ret = device_bind(parent, entry, name, NULL, id->data,
offset, &dev);
(That quoted code is with this patch applied, and the NULL value is the
platform data parameter.)
Also it's not clear from your Tegra 186 GPIO patch where you are using
this.
Here's the relevant part from the Tegra186 GPIO driver patch I posted:
+static int tegra186_gpio_bind(struct udevice *parent)
+{
+ struct tegra186_gpio_platdata *parent_plat = parent->platdata;
+ struct tegra186_gpio_ctlr_data *ctlr_data =
+ (struct tegra186_gpio_ctlr_data *)parent->driver_data;
...
+ /* If this is a child device, there is nothing to do here */
+ if (parent_plat)
+ return 0;
...
+ for (port = 0; port < ctlr_data->port_count; port++) {
...
+ plat->name = ctlr_data->ports[port].name;
+ plat->regs = &(regs[ctlr_data->ports[port].offset / 4]);
The data is used to determine how many child devices (one per port) to
create, and the name and register offset of each one. This is modelled
after the logic in the previous Tegra GPIO driver that you wrote, with
the unfortunate modification that the register layout is more
"interesting" on Tegra186, and so we can't determine the number of and
parameters for the child devices purely algorithmically, since the
register layout is decidedly non-linear.
I suppose an alternative would be to create separate U_BOOT_DRIVER()s
for each compatible value with different register layout, and then have
the bind() for each of those call into some common implementation with a
hard-coded parameter. Still, it seems like the usage in the current code
is exactly what udevice_id.data is for; to avoid having to implement
separate functions that do that.
Perhaps the creation of the child devices could happen in probe() rather
than bind()? I imagine there's some reason this wouldn't work (such as
this causing the devices to be created too late to be referenced by
other drivers?) or you would have done this in the existing Tegra GPIO
driver.
