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?
> +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
>
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.
> > +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
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?
Btw, is there any project which maintains SDIO ids, like there is
pci-ids.ucw.cz for PCI or http://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
>
>
>
On Wed, Jan 12, 2022 at 12:43:32PM +0100, Pali Roh?r wrote:
> Btw, is there any project which maintains SDIO ids, like there is
> pci-ids.ucw.cz for PCI or http://www.linux-usb.org/usb-ids.html for USB?
Both of those projects have nothing to do with the kernel drivers or
values at all, they are only for userspace tools to use.
So even if there was such a thing for SDIO ids, I doubt it would help
here.
thanks,
greg k-h
On Wednesday 12 January 2022 13:06:17 Greg Kroah-Hartman wrote:
> On Wed, Jan 12, 2022 at 12:43:32PM +0100, Pali Rohár wrote:
> > Btw, is there any project which maintains SDIO ids, like there is
> > pci-ids.ucw.cz for PCI or http://www.linux-usb.org/usb-ids.html for USB?
>
> Both of those projects have nothing to do with the kernel drivers or
> values at all, they are only for userspace tools to use.
>
> So even if there was such a thing for SDIO ids, I doubt it would help
> here.
Why do you doubt? For sure if would help! Just checking comments if some
user reported different card with this id would tell us how broken it is
and how sane it is to define macro for particular id.
On Wed, 12 Jan 2022 at 12:43, Pali Rohár <[email protected]> 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 http://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
On Wednesday 12 January 2022 12:43:32 CET Pali Roh?r 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.
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) },
{ },
};
--
J?r?me Pouiller
On Wednesday 12 January 2022 17:45:45 Jérôme Pouiller wrote:
> On Wednesday 12 January 2022 12:43:32 CET Pali Rohár 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.
>
> 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.
>
>
> --
> Jérôme Pouiller
>
>
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:
> > On Wednesday 12 January 2022 12:43:32 CET Pali Roh?r 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.
> >
> > 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
On Wed, 12 Jan 2022 at 19:24, Jérôme Pouiller
<[email protected]> wrote:
>
> 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:
> > > On Wednesday 12 January 2022 12:43:32 CET Pali Rohár 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.
> > >
> > > 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?
Sure, that works for me too.
Kind regards
Uffe