Re: [PATCH 07/23] wfx: add bus_sdio.c

[Jérôme Pouiller]

Hello Pali,

On Tuesday 13 October 2020 22:11:56 CEST Pali Rohár wrote:
> Hello!
> 
> On Monday 12 October 2020 12:46:32 Jerome Pouiller wrote:
> > +#define SDIO_VENDOR_ID_SILABS        0x0000
> > +#define SDIO_DEVICE_ID_SILABS_WF200  0x1000
> > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > +     { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
> 
> Please move ids into common include file include/linux/mmc/sdio_ids.h
> where are all SDIO ids. Now all drivers have ids defined in that file.
> 
> > +     // FIXME: ignore VID/PID and only rely on device tree
> > +     // { SDIO_DEVICE(SDIO_ANY_ID, SDIO_ANY_ID) },
> 
> What is the reason for ignoring vendor and device ids?

The device has a particularity, its VID/PID is 0000:1000 (as you can see
above). This value is weird. The risk of collision with another device is
high.

So, maybe the device should be probed only if it appears in the DT. Since
WF200 targets embedded platforms, I don't think it is a problem to rely on
DT. You will find another FIXME further in the code about that:

+               dev_warn(&func->dev,
+                        "device is not declared in DT, features will be limited\n");
+               // FIXME: ignore VID/PID and only rely on device tree
+               // return -ENODEV;

However, it wouldn't be usual way to manage SDIO devices (and it is the
reason why the code is commented out).

Anyway, if we choose to rely on the DT, should we also check the VID/PID?

Personally, I am in favor to probe the device only if VID/PID match and if
a DT node is found, even if it is not the usual way.

-- 
Jérôme Pouiller