Re: [PATCH v3 1/2] soc: imx8m: Probe the SoC driver as platform driver

[Marek Vasut]

On 9/27/24 1:39 AM, Saravana Kannan wrote:

[...]

> > +static int imx8mq_soc_revision(u32 *socrev)
> >   {
> >          struct device_node *np;
> >          void __iomem *ocotp_base;
> >          u32 magic;
> >          u32 rev;
> >          struct clk *clk;
> > +       int ret;
> >
> >          np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp");
> >          if (!np)
> > -               return 0;
> > +               return -EINVAL;
> >
> >          ocotp_base = of_iomap(np, 0);
>
> Using devm_of_iomap() and scoped "whatever it's called" might help
> simplify the error handling.
>
> So something like this for np:
> struct device_node *np __free(device_node) = np =
> of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp");
>
> And this for ocotp_base:
> ocotp_base = devm_of_iomap(dev, np, 0);

This would fail if OCOTP driver probes first and claims the memory area 
with request_mem_region() (or devm_request_mem_region(), used in 
__devm_ioremap_resource() which is called from devm_of_iomap()). I ran 
into this with ANATOP, which is the other iomap()d device here. The 
of_iomap() does not use request_mem_region(), so it can map the area.

> Would mean you can delete all the error handling parts

All right, let's do this in separate 3/3 patch , because the amount of 
changes in this one patch are growing to be too much and difficult to 
review.

[...]

> > @@ -212,8 +240,11 @@ static int __init imx8_soc_init(void)
> >          data = id->data;
> >          if (data) {
> >                  soc_dev_attr->soc_id = data->name;
> > -               if (data->soc_revision)
> > -                       soc_rev = data->soc_revision();
> > +               if (data->soc_revision) {
> > +                       ret = data->soc_revision(&soc_rev);
> > +                       if (ret)
> > +                               goto free_soc;
> > +               }
> >          }
>
> I'm glad it's working for you, but I think there might still be a race
> that you are just lucky enough to not hit. I think you still need to
> fix up drivers/base/soc.c to return -EPROBE_DEFER when
> soc_device_match() is called but soc_bus_type has no devices
> registered. That way any drivers that try to use that API will defer
> probe until this device gets to probe.

soc_device_match() returns a pointer to soc_device_attribute or NULL, do 
you have some other function in mind ?

> And then you'll have to look at all the callers of that API for the
> boards this driver is meant for and make sure they don't ignore the
> error return value. Just add a WARN() on the API to figure out all the
> callers in your board.
>
> Also, you might want to check that your list of probed devices doesn't
> change without any async probing or this patch vs with async probing
> and this patch. Quick way to get list of successfully probed devices
> is:
> # find /sys/devices -name driver

It seems OK.

[...]