[PATCH v4 1/4] dm: core: Bind another driver with the same compatible string

[Michal Simek]

ne 24. 10. 2021 v 22:01 odesílatel Simon Glass <sjg at chromium.org> napsal:
>
> Hi Michal,
>
> On Fri, 15 Oct 2021 at 07:17, Michal Simek <michal.simek at 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 at 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



-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs