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

[Marek Vasut <marex@denx.de>]

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.

[...]