Re: [PATCH v9 08/24] wfx: add bus_sdio.c

On Wednesday 12 January 2022 18:48:48 CET Pali Rohár wrote:

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.


On Wednesday 12 January 2022 17:45:45 Jérôme Pouiller wrote:

quoted

On Wednesday 12 January 2022 12:43:32 CET Pali Rohár wrote:

quoted

On Wednesday 12 January 2022 12:18:58 Jérôme Pouiller wrote:

quoted

On Wednesday 12 January 2022 11:58:59 CET Pali Rohár wrote:

quoted

On Tuesday 11 January 2022 18:14:08 Jerome Pouiller wrote:

quoted

+static const struct sdio_device_id wfx_sdio_ids[] = {
+     { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
+     { },
+};

Hello! Is this table still required?

As far as I understand, if the driver does not provide an id_table, the
probe function won't be never called (see sdio_match_device()).

Since, we rely on the device tree, we could replace SDIO_VENDOR_ID_SILABS
and SDIO_DEVICE_ID_SILABS_WF200 by SDIO_ANY_ID. However, it does not hurt
to add an extra filter here.

Now when this particular id is not required, I'm thinking if it is still
required and it is a good idea to define these SDIO_VENDOR_ID_SILABS
macros into kernel include files. As it would mean that other broken
SDIO devices could define these bogus numbers too... And having them in
common kernel includes files can cause issues... e.g. other developers
could think that it is correct to use them as they are defined in common
header files. But as these numbers are not reliable (other broken cards
may have same ids as wf200) and their usage may cause issues in future.

In order to make SDIO_VENDOR_ID_SILABS less official, do you prefer to
define it in wfx/bus_sdio.c instead of mmc/sdio_ids.h?

Or even not defined at all like:

    static const struct sdio_device_id wfx_sdio_ids[] = {
         /* WF200 does not have official VID/PID */
         { SDIO_DEVICE(0x0000, 0x1000) },
         { },
    };

This has advantage that it is explicitly visible that this device does
not use any officially assigned ids.

Ulf, are you also agree?


-- 
Jérôme Pouiller