Message ID | 4b29b869b8910f82e4dabe96975619df23c95eae.1633529948.git.michal.simek@xilinx.com |
---|---|
State | Changes Requested |
Delegated to: | Simon Glass |
Headers | show |
Series | [v3,1/3] dm: core: Bind another driver with the same compatible string | expand |
Hi Michal, On Wed, 6 Oct 2021 at 08:19, 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 v3: > - New patch in series > > drivers/core/lists.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) Can you refactor this to avoid the 'goto'? E.g. put the loop in a new function? Also, if pre_reloc_only, it still quits. Don't you want it to continue in that case too? Finally, can you add such a driver to sandbox test.dts and add a check in test/dm/core.c or similar so that this behaviour is tested? > > diff --git a/drivers/core/lists.c b/drivers/core/lists.c > index 350b9d32687c..199ee3a8e0fc 100644 > --- a/drivers/core/lists.c > +++ b/drivers/core/lists.c > @@ -221,12 +221,14 @@ 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++) { > + entry = driver; > +next: > + for (; entry != driver + n_ents; entry++) { > ret = driver_check_compatible(entry->of_match, &id, > compat); > if (!ret) > break; > + > } > if (entry == driver + n_ents) > continue; > @@ -246,7 +248,8 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp, > id->data, node, &dev); > if (ret == -ENODEV) { > log_debug("Driver '%s' refuses to bind\n", entry->name); > - continue; > + entry++; > + goto next; > } > if (ret) { > dm_warn("Error binding driver '%s': %d\n", entry->name, > -- > 2.33.0 > Regards, Simon
On 10/14/21 17:09, Simon Glass wrote: > Hi Michal, > > On Wed, 6 Oct 2021 at 08:19, 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 v3: >> - New patch in series >> >> drivers/core/lists.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) > > Can you refactor this to avoid the 'goto'? E.g. put the loop in a new function? Sent v4 with adding one more loop. There are so many parameters use that new function won't really help. > > Also, if pre_reloc_only, it still quits. Don't you want it to continue > in that case too? Also fixed in v4. > > Finally, can you add such a driver to sandbox test.dts and add a check > in test/dm/core.c or similar so that this behaviour is tested? When the patch is acked I will take a look at writing test case for it. I have tested it with pwm driver to see proper behavior. Thanks, Michal
diff --git a/drivers/core/lists.c b/drivers/core/lists.c index 350b9d32687c..199ee3a8e0fc 100644 --- a/drivers/core/lists.c +++ b/drivers/core/lists.c @@ -221,12 +221,14 @@ 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++) { + entry = driver; +next: + for (; entry != driver + n_ents; entry++) { ret = driver_check_compatible(entry->of_match, &id, compat); if (!ret) break; + } if (entry == driver + n_ents) continue; @@ -246,7 +248,8 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp, id->data, node, &dev); if (ret == -ENODEV) { log_debug("Driver '%s' refuses to bind\n", entry->name); - continue; + entry++; + goto next; } if (ret) { dm_warn("Error binding driver '%s': %d\n", entry->name,
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 v3: - New patch in series drivers/core/lists.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)