[U-Boot] [PATCH] dm: allow setting driver_data before/during bind

[Stephen Warren]

On 05/01/2016 01:27 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 28 April 2016 at 17:08, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren at 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 <swarren at 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.