Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220111171424.862764-1-Jerome.Pouiller@silabs.com>
 <20220111171424.862764-9-Jerome.Pouiller@silabs.com> <20220112105859.u4j76o7cpsr4znmb@pali>
 <42104281.b1Mx7tgHyx@pc-42> <20220112114332.jadw527pe7r2j4vv@pali>
In-Reply-To: <20220112114332.jadw527pe7r2j4vv@pali>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Wed, 12 Jan 2022 16:03:56 +0100
Message-ID: <CAPDyKFoMj1r+bEh-MqOdTVzs0C=LCFPPbXj3jHwB4Yty=bA03Q@mail.gmail.com>
Subject: Re: [PATCH v9 08/24] wfx: add bus_sdio.c
To:     =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>,
        =?UTF-8?B?SsOpcsO0bWUgUG91aWxsZXI=?= <Jerome.Pouiller@silabs.com>
Cc:     linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        Kalle Valo <kvalo@codeaurora.org>, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "David S . Miller" <davem@davemloft.net>,
        devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
        linux-mmc@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, 12 Jan 2022 at 12:43, Pali Roh=C3=A1r <pali@kernel.org> wrote:
>
> On Wednesday 12 January 2022 12:18:58 J=C3=A9r=C3=B4me Pouiller wrote:
> > On Wednesday 12 January 2022 11:58:59 CET Pali Roh=C3=A1r wrote:
> > > On Tuesday 11 January 2022 18:14:08 Jerome Pouiller wrote:
> > > > +static const struct sdio_device_id wfx_sdio_ids[] =3D {
> > > > +     { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF=
200) },
> > > > +     { },
> > > > +};
> > >
> > > 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_SILA=
BS
> > and SDIO_DEVICE_ID_SILABS_WF200 by SDIO_ANY_ID. However, it does not hu=
rt
> > 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 =3D {
> > > > +     .name =3D "wfx-sdio",
> > > > +     .id_table =3D wfx_sdio_ids,
> > > > +     .probe =3D wfx_sdio_probe,
> > > > +     .remove =3D wfx_sdio_remove,
> > > > +     .drv =3D {
> > > > +             .owner =3D THIS_MODULE,
> > > > +             .of_match_table =3D wfx_sdio_of_match,
> > > > +     }
> > > > +};
> > > > --
> > > > 2.34.1
> > > >
> > >
> >
> >
> > --
> > J=C3=A9r=C3=B4me Pouiller

Kind regards
Uffe