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