lkml.org 
[lkml]   [2022]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v9 08/24] wfx: add bus_sdio.c
    On Wed, 12 Jan 2022 at 12:43, Pali Rohár <pali@kernel.org> wrote:
    >
    > On Wednesday 12 January 2022 12:18:58 Jérôme Pouiller wrote:
    > > On Wednesday 12 January 2022 11:58:59 CET Pali Rohár wrote:
    > > > On Tuesday 11 January 2022 18:14:08 Jerome Pouiller wrote:
    > > > > +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.
    >
    > Ulf, any opinion?

    The sdio_match_device() is what is being used to match the device to
    its sdio_driver, which is being called from the sdio_bus_type's
    ->match() callback.

    In regards to the DT compatible strings from a drivers'
    .of_match_table, that is currently left to be matched by the sdio
    driver's ->probe() function internally, by calling
    of_driver_match_device().

    In other words, I think what Jerome has suggested here seems
    reasonable to me. Matching on "SDIO_ANY_ID" would work too, but I
    think it's better with a poor filter like SDIO_VENDOR_ID_SILABS*,
    rather than none.

    An entirely different and new approach would be to extend
    sdio_match_device() to call of_driver_match_device() too. However, in
    that case we would also need to add a new corresponding ->probe()
    callback for the sdio_driver, as the current one takes a const struct
    sdio_device_id, which doesn't work when matching on DT compatibles.

    >
    > Btw, is there any project which maintains SDIO ids, like there is
    > pci-ids.ucw.cz for PCI or www.linux-usb.org/usb-ids.html for USB?
    >
    > > > > +MODULE_DEVICE_TABLE(sdio, wfx_sdio_ids);
    > > > > +
    > > > > +struct sdio_driver wfx_sdio_driver = {
    > > > > + .name = "wfx-sdio",
    > > > > + .id_table = wfx_sdio_ids,
    > > > > + .probe = wfx_sdio_probe,
    > > > > + .remove = wfx_sdio_remove,
    > > > > + .drv = {
    > > > > + .owner = THIS_MODULE,
    > > > > + .of_match_table = wfx_sdio_of_match,
    > > > > + }
    > > > > +};
    > > > > --
    > > > > 2.34.1
    > > > >
    > > >
    > >
    > >
    > > --
    > > Jérôme Pouiller

    Kind regards
    Uffe

    \
     
     \ /
      Last update: 2022-01-12 16:05    [W:4.059 / U:0.116 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site