| Message ID | 1093778300bca9eaf811908feafc45ee2e792dac.1634303847.git.michal.simek@xilinx.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [v4,1/4] dm: core: Bind another driver with the same compatible string | expand |
Hi Michal, On Fri, 15 Oct 2021 at 07:17, Michal Simek <michal.simek@xilinx.com> wrote: > > When one IP can have multiple configurations (like timer and PWM) covered > by multiple drivers. Because it is the same IP it should also have the same > compatible string. > Current code look for the first driver which matches compatible string and > call bind function. If this is not the right driver which is express by > returning -ENODEV ("Driver XYZ refuses to bind") there is no reason not to > continue over compatible list to try to find out different driver with the > same compatible string which match it. > > When the first compatible string is found core looks for driver bind() > function. If if assigned driver refuse to bind, core continue in a loop to > check if there is another driver which match compatible string. > > This driver is going to be used with cdnc,ttc driver which has driver as > timer and also can be used as PWM. The difference is done via #pwm-cells > property in DT. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > Changes in v4: > - Remove goto from code > - Also continue on prereloc case > > Changes in v3: > - New patch in series > > When this is acked I will spent my time on writing test for it. > > --- > drivers/core/lists.c | 84 +++++++++++++++++++++++++++----------------- > 1 file changed, 52 insertions(+), 32 deletions(-) > > diff --git a/drivers/core/lists.c b/drivers/core/lists.c > index 5d4f2ea0e3ad..8b655db68edb 100644 > --- a/drivers/core/lists.c > +++ b/drivers/core/lists.c > @@ -221,45 +221,65 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp, > compat = compat_list + i; > log_debug(" - attempt to match compatible string '%s'\n", > compat); > - > - for (entry = driver; entry != driver + n_ents; entry++) { > - ret = driver_check_compatible(entry->of_match, &id, > - compat); > - if ((drv) && (drv == entry)) > - break; > - if (!ret) > + entry = driver; > + > + while (1) { > + for (; entry != driver + n_ents; entry++) { > + ret = driver_check_compatible(entry->of_match, > + &id, compat); > + if (drv && drv == entry) > + break; > + if (!ret) > + break; > + } > + /* > + * Reach the last entry - time to look at next > + * compatible string. > + */ > + if (entry == driver + n_ents) > break; > - } > - if (entry == driver + n_ents) > - continue; > - > - if (pre_reloc_only) { > - if (!ofnode_pre_reloc(node) && > - !(entry->flags & DM_FLAG_PRE_RELOC)) { > - log_debug("Skipping device pre-relocation\n"); > - return 0; > + > + if (pre_reloc_only) { > + if (!ofnode_pre_reloc(node) && > + !(entry->flags & DM_FLAG_PRE_RELOC)) { > + log_debug("Skipping device pre-relocation\n"); > + entry++; > + continue; > + } > } > - } > > - log_debug(" - found match at '%s': '%s' matches '%s'\n", > - entry->name, entry->of_match->compatible, > - id->compatible); > - ret = device_bind_with_driver_data(parent, entry, name, > - id->data, node, &dev); > - if (ret == -ENODEV) { > - log_debug("Driver '%s' refuses to bind\n", entry->name); > - continue; > - } > - if (ret) { > - dm_warn("Error binding driver '%s': %d\n", entry->name, > - ret); > - return log_msg_ret("bind", ret); > - } else { > + log_debug(" - found match at '%s': '%s' matches '%s'\n", > + entry->name, entry->of_match->compatible, > + id->compatible); > + ret = device_bind_with_driver_data(parent, entry, name, > + id->data, node, > + &dev); > + /* > + * Driver found but didn't bind properly try another one > + */ > + if (ret == -ENODEV) { > + log_debug("Driver '%s' refuses to bind\n", > + entry->name); > + entry++; > + continue; > + } > + > + /* Error in binding driver */ > + if (ret) { > + dm_warn("Error binding driver '%s': %d\n", > + entry->name, ret); > + return log_msg_ret("bind", ret); > + } > + > + /* Properly binded driver */ bound > found = true; > if (devp) > *devp = dev; > + break; > } > - break; > + > + if (found) > + break; > } > > if (!found && !result && ret != -ENODEV) > -- > 2.33.1 > The impl looks OK to me. I still feel a separate function would help readability...we don't need to worry about the parameters in general, since the compiler will inline the code and LTO is even more agressive. But I'm OK with this as it is if you prefer it, as it isn't too horrendous. Should we put this behind a Kconfig given the run-time penalty? Also, please add a test. Regards, Simon
ne 24. 10. 2021 v 22:01 odesÃlatel Simon Glass <sjg@chromium.org> napsal: > > Hi Michal, > > On Fri, 15 Oct 2021 at 07:17, Michal Simek <michal.simek@xilinx.com> wrote: > > > > When one IP can have multiple configurations (like timer and PWM) covered > > by multiple drivers. Because it is the same IP it should also have the same > > compatible string. > > Current code look for the first driver which matches compatible string and > > call bind function. If this is not the right driver which is express by > > returning -ENODEV ("Driver XYZ refuses to bind") there is no reason not to > > continue over compatible list to try to find out different driver with the > > same compatible string which match it. > > > > When the first compatible string is found core looks for driver bind() > > function. If if assigned driver refuse to bind, core continue in a loop to > > check if there is another driver which match compatible string. > > > > This driver is going to be used with cdnc,ttc driver which has driver as > > timer and also can be used as PWM. The difference is done via #pwm-cells > > property in DT. > > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > --- > > > > Changes in v4: > > - Remove goto from code > > - Also continue on prereloc case > > > > Changes in v3: > > - New patch in series > > > > When this is acked I will spent my time on writing test for it. > > > > --- > > drivers/core/lists.c | 84 +++++++++++++++++++++++++++----------------- > > 1 file changed, 52 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/core/lists.c b/drivers/core/lists.c > > index 5d4f2ea0e3ad..8b655db68edb 100644 > > --- a/drivers/core/lists.c > > +++ b/drivers/core/lists.c > > @@ -221,45 +221,65 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp, > > compat = compat_list + i; > > log_debug(" - attempt to match compatible string '%s'\n", > > compat); > > - > > - for (entry = driver; entry != driver + n_ents; entry++) { > > - ret = driver_check_compatible(entry->of_match, &id, > > - compat); > > - if ((drv) && (drv == entry)) > > - break; > > - if (!ret) > > + entry = driver; > > + > > + while (1) { > > + for (; entry != driver + n_ents; entry++) { > > + ret = driver_check_compatible(entry->of_match, > > + &id, compat); > > + if (drv && drv == entry) > > + break; > > + if (!ret) > > + break; > > + } > > + /* > > + * Reach the last entry - time to look at next > > + * compatible string. > > + */ > > + if (entry == driver + n_ents) > > break; > > - } > > - if (entry == driver + n_ents) > > - continue; > > - > > - if (pre_reloc_only) { > > - if (!ofnode_pre_reloc(node) && > > - !(entry->flags & DM_FLAG_PRE_RELOC)) { > > - log_debug("Skipping device pre-relocation\n"); > > - return 0; > > + > > + if (pre_reloc_only) { > > + if (!ofnode_pre_reloc(node) && > > + !(entry->flags & DM_FLAG_PRE_RELOC)) { > > + log_debug("Skipping device pre-relocation\n"); > > + entry++; > > + continue; > > + } > > } > > - } > > > > - log_debug(" - found match at '%s': '%s' matches '%s'\n", > > - entry->name, entry->of_match->compatible, > > - id->compatible); > > - ret = device_bind_with_driver_data(parent, entry, name, > > - id->data, node, &dev); > > - if (ret == -ENODEV) { > > - log_debug("Driver '%s' refuses to bind\n", entry->name); > > - continue; > > - } > > - if (ret) { > > - dm_warn("Error binding driver '%s': %d\n", entry->name, > > - ret); > > - return log_msg_ret("bind", ret); > > - } else { > > + log_debug(" - found match at '%s': '%s' matches '%s'\n", > > + entry->name, entry->of_match->compatible, > > + id->compatible); > > + ret = device_bind_with_driver_data(parent, entry, name, > > + id->data, node, > > + &dev); > > + /* > > + * Driver found but didn't bind properly try another one > > + */ > > + if (ret == -ENODEV) { > > + log_debug("Driver '%s' refuses to bind\n", > > + entry->name); > > + entry++; > > + continue; > > + } > > + > > + /* Error in binding driver */ > > + if (ret) { > > + dm_warn("Error binding driver '%s': %d\n", > > + entry->name, ret); > > + return log_msg_ret("bind", ret); > > + } > > + > > + /* Properly binded driver */ > > bound > > > found = true; > > if (devp) > > *devp = dev; > > + break; > > } > > - break; > > + > > + if (found) > > + break; > > } > > > > if (!found && !result && ret != -ENODEV) > > -- > > 2.33.1 > > > > The impl looks OK to me. I still feel a separate function would help > readability...we don't need to worry about the parameters in general, > since the compiler will inline the code and LTO is even more > agressive. > > But I'm OK with this as it is if you prefer it, as it isn't too horrendous. > > Should we put this behind a Kconfig given the run-time penalty? > > Also, please add a test. I will take a look at adding a test. But in standard configuration PMW and TTC timers are not enabled both that's why the patch can come later. Thanks, Michal
diff --git a/drivers/core/lists.c b/drivers/core/lists.c index 5d4f2ea0e3ad..8b655db68edb 100644 --- a/drivers/core/lists.c +++ b/drivers/core/lists.c @@ -221,45 +221,65 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp, compat = compat_list + i; log_debug(" - attempt to match compatible string '%s'\n", compat); - - for (entry = driver; entry != driver + n_ents; entry++) { - ret = driver_check_compatible(entry->of_match, &id, - compat); - if ((drv) && (drv == entry)) - break; - if (!ret) + entry = driver; + + while (1) { + for (; entry != driver + n_ents; entry++) { + ret = driver_check_compatible(entry->of_match, + &id, compat); + if (drv && drv == entry) + break; + if (!ret) + break; + } + /* + * Reach the last entry - time to look at next + * compatible string. + */ + if (entry == driver + n_ents) break; - } - if (entry == driver + n_ents) - continue; - - if (pre_reloc_only) { - if (!ofnode_pre_reloc(node) && - !(entry->flags & DM_FLAG_PRE_RELOC)) { - log_debug("Skipping device pre-relocation\n"); - return 0; + + if (pre_reloc_only) { + if (!ofnode_pre_reloc(node) && + !(entry->flags & DM_FLAG_PRE_RELOC)) { + log_debug("Skipping device pre-relocation\n"); + entry++; + continue; + } } - } - log_debug(" - found match at '%s': '%s' matches '%s'\n", - entry->name, entry->of_match->compatible, - id->compatible); - ret = device_bind_with_driver_data(parent, entry, name, - id->data, node, &dev); - if (ret == -ENODEV) { - log_debug("Driver '%s' refuses to bind\n", entry->name); - continue; - } - if (ret) { - dm_warn("Error binding driver '%s': %d\n", entry->name, - ret); - return log_msg_ret("bind", ret); - } else { + log_debug(" - found match at '%s': '%s' matches '%s'\n", + entry->name, entry->of_match->compatible, + id->compatible); + ret = device_bind_with_driver_data(parent, entry, name, + id->data, node, + &dev); + /* + * Driver found but didn't bind properly try another one + */ + if (ret == -ENODEV) { + log_debug("Driver '%s' refuses to bind\n", + entry->name); + entry++; + continue; + } + + /* Error in binding driver */ + if (ret) { + dm_warn("Error binding driver '%s': %d\n", + entry->name, ret); + return log_msg_ret("bind", ret); + } + + /* Properly binded driver */ found = true; if (devp) *devp = dev; + break; } - break; + + if (found) + break; } if (!found && !result && ret != -ENODEV)
When one IP can have multiple configurations (like timer and PWM) covered by multiple drivers. Because it is the same IP it should also have the same compatible string. Current code look for the first driver which matches compatible string and call bind function. If this is not the right driver which is express by returning -ENODEV ("Driver XYZ refuses to bind") there is no reason not to continue over compatible list to try to find out different driver with the same compatible string which match it. When the first compatible string is found core looks for driver bind() function. If if assigned driver refuse to bind, core continue in a loop to check if there is another driver which match compatible string. This driver is going to be used with cdnc,ttc driver which has driver as timer and also can be used as PWM. The difference is done via #pwm-cells property in DT. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- Changes in v4: - Remove goto from code - Also continue on prereloc case Changes in v3: - New patch in series When this is acked I will spent my time on writing test for it. --- drivers/core/lists.c | 84 +++++++++++++++++++++++++++----------------- 1 file changed, 52 insertions(+), 32 deletions(-)