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

[Stephen Warren]

On 05/04/2016 12:42 PM, Stephen Warren wrote:
> 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.

Simon, any more thoughts on how the Tegra186 GPIO should handle the need 
for HW knowledge in bind()? I'd like to repost that driver soon, and 
doing so requires resolving this discussion. Thanks.