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

[Simon Glass]

Hi Stephen,

On 4 May 2016 at 12:42, Stephen Warren <swarren at wwwdotorg.org> 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.

Yes, but platdata is defined to be needed by bind(), whereas
driver_data is supposed to be used in probe()  to find out which
device tree compatible string matched. Remember that the device tree
properties are not looked at during bind(), only later. So it makes
sense to include platdata in the device_bind() call, but not
driver_data.

>
>> - 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.

I don't think so. That value is set in lists_bind_fdt().

I wonder if you could set it yourself after calling device_bind()?

>
>> - 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:

That's right. For DT the paltform data is set up in the
ofdata_to_platdata() method. Since you are using DT, you should follow
that convention.

>
> 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.

OK I see. This feels like something that your device tree should
describe. Failing that, how about a hard-coded table of information in
the source code? You can look through the table and create the
appropriate child devices.

>
> 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.

Yes, but you should use different compatible strings for the nodes. As
I understand it, you only have a single node, so re-purposing this
does not seem right to me.

>
> 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.

Best not - it is good to have the devices known on start-up. Let me
know if the above solution doesn't work.

Regards,
Simon