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