[v7,08/24] wfx: add bus_sdio.c

Commit Message

Jérôme Pouiller Sept. 20, 2021, 4:11 p.m. UTC

From: Jérôme Pouiller <[e-mail]>

Signed-off-by: Jérôme Pouiller <[e-mail]>
---
 drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +++++++++++++++++++++
 1 file changed, 261 insertions(+)
 create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c

Comments

Ulf Hansson Sept. 30, 2021, 10:07 a.m. UTC | #1

On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
<[e-mail]> wrote:
>
> From: Jérôme Pouiller <[e-mail]>
>
> Signed-off-by: Jérôme Pouiller <[e-mail]>
> ---
>  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +++++++++++++++++++++
>  1 file changed, 261 insertions(+)
>  create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
>
> diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c b/drivers/net/wireless/silabs/wfx/bus_sdio.c

[...]

> +
> +static int wfx_sdio_probe(struct sdio_func *func,
> +                         const struct sdio_device_id *id)
> +{
> +       struct device_node *np = func->dev.of_node;
> +       struct wfx_sdio_priv *bus;
> +       int ret;
> +
> +       if (func->num != 1) {
> +               dev_err(&func->dev, "SDIO function number is %d while it should always be 1 (unsupported chip?)\n",
> +                       func->num);
> +               return -ENODEV;
> +       }
> +
> +       bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL);
> +       if (!bus)
> +               return -ENOMEM;
> +
> +       if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> +               dev_warn(&func->dev, "no compatible device found in DT\n");
> +               return -ENODEV;
> +       }
> +
> +       bus->func = func;
> +       bus->of_irq = irq_of_parse_and_map(np, 0);
> +       sdio_set_drvdata(func, bus);
> +       func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> +                             MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> +                             MMC_QUIRK_BROKEN_BYTE_MODE_512;

I would rather see that you add an SDIO_FIXUP for the SDIO card, to
the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
this.

> +
> +       sdio_claim_host(func);
> +       ret = sdio_enable_func(func);
> +       /* Block of 64 bytes is more efficient than 512B for frame sizes < 4k */
> +       sdio_set_block_size(func, 64);
> +       sdio_release_host(func);
> +       if (ret)
> +               return ret;
> +
> +       bus->core = wfx_init_common(&func->dev, &wfx_sdio_pdata,
> +                                   &wfx_sdio_hwbus_ops, bus);
> +       if (!bus->core) {
> +               ret = -EIO;
> +               goto sdio_release;
> +       }
> +
> +       ret = wfx_probe(bus->core);
> +       if (ret)
> +               goto sdio_release;
> +
> +       return 0;
> +
> +sdio_release:
> +       sdio_claim_host(func);
> +       sdio_disable_func(func);
> +       sdio_release_host(func);
> +       return ret;
> +}
> +
> +static void wfx_sdio_remove(struct sdio_func *func)
> +{
> +       struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
> +
> +       wfx_release(bus->core);
> +       sdio_claim_host(func);
> +       sdio_disable_func(func);
> +       sdio_release_host(func);
> +}
> +
> +static const struct sdio_device_id wfx_sdio_ids[] = {
> +       { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
> +       { },
> +};
> +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,

Is there no power management? Or do you intend to add that on top?

> +       }
> +};
> --
> 2.33.0
>

Kind regards
Uffe

Jérôme Pouiller Sept. 30, 2021, 4:51 p.m. UTC | #2

Hello Ulf,

On Thursday 30 September 2021 12:07:55 CEST Ulf Hansson wrote:
> On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
> <[e-mail]> wrote:
> >
> > From: Jérôme Pouiller <[e-mail]>
> >
> > Signed-off-by: Jérôme Pouiller <[e-mail]>
> > ---
> >  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +++++++++++++++++++++
> >  1 file changed, 261 insertions(+)
> >  create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> >
> > diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c b/drivers/net/wireless/silabs/wfx/bus_sdio.c
> 
> [...]
> 
> > +
> > +static int wfx_sdio_probe(struct sdio_func *func,
> > +                         const struct sdio_device_id *id)
> > +{
> > +       struct device_node *np = func->dev.of_node;
> > +       struct wfx_sdio_priv *bus;
> > +       int ret;
> > +
> > +       if (func->num != 1) {
> > +               dev_err(&func->dev, "SDIO function number is %d while it should always be 1 (unsupported chip?)\n",
> > +                       func->num);
> > +               return -ENODEV;
> > +       }
> > +
> > +       bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL);
> > +       if (!bus)
> > +               return -ENOMEM;
> > +
> > +       if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> > +               dev_warn(&func->dev, "no compatible device found in DT\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       bus->func = func;
> > +       bus->of_irq = irq_of_parse_and_map(np, 0);
> > +       sdio_set_drvdata(func, bus);
> > +       func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> > +                             MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> > +                             MMC_QUIRK_BROKEN_BYTE_MODE_512;
> 
> I would rather see that you add an SDIO_FIXUP for the SDIO card, to
> the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
> this.

In the current patch, these quirks are applied only if the device appears
in the device tree (see the condition above). If I implement them in
drivers/mmc/core/quirks.h they will be applied as soon as the device is
detected. Is it what we want?

Note: we already have had a discussion about the strange VID/PID declared
by this device:
  https://www.spinics.net/lists/netdev/msg692577.html


[...]
> > +
> > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > +       { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
> > +       { },
> > +};
> > +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,
> 
> Is there no power management? Or do you intend to add that on top?

It seems we already have had this discussion:

  https://lore.kernel.org/netdev/CAPDyKFqJf=vUqpQg3suDCadKrFTkQWFTY_qp=+yDK=[e-mail]/#r

In this thread, Kalle said:
> Many mac80211 drivers do so that the device is powered off during
> interface down (ifconfig wlan0 down), and as mac80211 does interface
> down automatically during suspend, suspend then works without extra
> handlers.

Pali Rohár Sept. 30, 2021, 5:06 p.m. UTC | #3

On Thursday 30 September 2021 18:51:09 Jérôme Pouiller wrote:
> Hello Ulf,
> 
> On Thursday 30 September 2021 12:07:55 CEST Ulf Hansson wrote:
> > On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
> > <[e-mail]> wrote:
> > >
> > > From: Jérôme Pouiller <[e-mail]>
> > >
> > > Signed-off-by: Jérôme Pouiller <[e-mail]>
> > > ---
> > >  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +++++++++++++++++++++
> > >  1 file changed, 261 insertions(+)
> > >  create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> > >
> > > diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c b/drivers/net/wireless/silabs/wfx/bus_sdio.c
> > 
> > [...]
> > 
> > > +
> > > +static int wfx_sdio_probe(struct sdio_func *func,
> > > +                         const struct sdio_device_id *id)
> > > +{
> > > +       struct device_node *np = func->dev.of_node;
> > > +       struct wfx_sdio_priv *bus;
> > > +       int ret;
> > > +
> > > +       if (func->num != 1) {
> > > +               dev_err(&func->dev, "SDIO function number is %d while it should always be 1 (unsupported chip?)\n",
> > > +                       func->num);
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL);
> > > +       if (!bus)
> > > +               return -ENOMEM;
> > > +
> > > +       if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> > > +               dev_warn(&func->dev, "no compatible device found in DT\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       bus->func = func;
> > > +       bus->of_irq = irq_of_parse_and_map(np, 0);
> > > +       sdio_set_drvdata(func, bus);
> > > +       func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> > > +                             MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> > > +                             MMC_QUIRK_BROKEN_BYTE_MODE_512;
> > 
> > I would rather see that you add an SDIO_FIXUP for the SDIO card, to
> > the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
> > this.
> 
> In the current patch, these quirks are applied only if the device appears
> in the device tree (see the condition above). If I implement them in
> drivers/mmc/core/quirks.h they will be applied as soon as the device is
> detected. Is it what we want?
> 
> Note: we already have had a discussion about the strange VID/PID declared
> by this device:
>   https://www.spinics.net/lists/netdev/msg692577.html

Yes, vendor id 0x0000 is invalid per SDIO spec. So based on this vendor
id, it is not possible to write any quirk in mmc/sdio generic code.

Ulf, but maybe it could be possible to write quirk based on OF
compatible string?

Jérôme, could you please notify your hw departement that this sdio card
does not comply with SDIO spec due to incorrect vendor id stored in hw,
so they could fix this issue in next product, by proper allocation of
vendor id number from USB-IF (*)? I know that for existing products it
is not possible to fix, but it can be fixed in next generation of
products based on used SDIO IP.

(*) - USB-IF really allocates SDIO vendor ids, see:
https://lore.kernel.org/linux-mmc/20210607140216.64iuprp3siggslrk@pali/

> 
> [...]
> > > +
> > > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > > +       { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
> > > +       { },
> > > +};
> > > +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,
> > 
> > Is there no power management? Or do you intend to add that on top?
> 
> It seems we already have had this discussion:
> 
>   https://lore.kernel.org/netdev/CAPDyKFqJf=vUqpQg3suDCadKrFTkQWFTY_qp=+yDK=[e-mail]/#r
> 
> In this thread, Kalle said:
> > Many mac80211 drivers do so that the device is powered off during
> > interface down (ifconfig wlan0 down), and as mac80211 does interface
> > down automatically during suspend, suspend then works without extra
> > handlers.
> 
> 
> -- 
> Jérôme Pouiller
> 
>

Ulf Hansson Oct. 1, 2021, 3:23 p.m. UTC | #4

On Thu, 30 Sept 2021 at 19:06, Pali Rohár <[e-mail]> wrote:
>
> On Thursday 30 September 2021 18:51:09 Jérôme Pouiller wrote:
> > Hello Ulf,
> >
> > On Thursday 30 September 2021 12:07:55 CEST Ulf Hansson wrote:
> > > On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
> > > <[e-mail]> wrote:
> > > >
> > > > From: Jérôme Pouiller <[e-mail]>
> > > >
> > > > Signed-off-by: Jérôme Pouiller <[e-mail]>
> > > > ---
> > > >  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +++++++++++++++++++++
> > > >  1 file changed, 261 insertions(+)
> > > >  create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> > > >
> > > > diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c b/drivers/net/wireless/silabs/wfx/bus_sdio.c
> > >
> > > [...]
> > >
> > > > +
> > > > +static int wfx_sdio_probe(struct sdio_func *func,
> > > > +                         const struct sdio_device_id *id)
> > > > +{
> > > > +       struct device_node *np = func->dev.of_node;
> > > > +       struct wfx_sdio_priv *bus;
> > > > +       int ret;
> > > > +
> > > > +       if (func->num != 1) {
> > > > +               dev_err(&func->dev, "SDIO function number is %d while it should always be 1 (unsupported chip?)\n",
> > > > +                       func->num);
> > > > +               return -ENODEV;
> > > > +       }
> > > > +
> > > > +       bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL);
> > > > +       if (!bus)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> > > > +               dev_warn(&func->dev, "no compatible device found in DT\n");
> > > > +               return -ENODEV;
> > > > +       }
> > > > +
> > > > +       bus->func = func;
> > > > +       bus->of_irq = irq_of_parse_and_map(np, 0);
> > > > +       sdio_set_drvdata(func, bus);
> > > > +       func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> > > > +                             MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> > > > +                             MMC_QUIRK_BROKEN_BYTE_MODE_512;
> > >
> > > I would rather see that you add an SDIO_FIXUP for the SDIO card, to
> > > the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
> > > this.
> >
> > In the current patch, these quirks are applied only if the device appears
> > in the device tree (see the condition above). If I implement them in
> > drivers/mmc/core/quirks.h they will be applied as soon as the device is
> > detected. Is it what we want?
> >
> > Note: we already have had a discussion about the strange VID/PID declared
> > by this device:
> >   https://www.spinics.net/lists/netdev/msg692577.html
>
> Yes, vendor id 0x0000 is invalid per SDIO spec. So based on this vendor
> id, it is not possible to write any quirk in mmc/sdio generic code.
>
> Ulf, but maybe it could be possible to write quirk based on OF
> compatible string?

Yes, that would be better in my opinion.

We already have DT bindings to describe embedded SDIO cards (a subnode
to the mmc controller node), so we should be able to extend that I
think.

The main reason why I think it's a good idea, is that we may need to
know (future wise) about quirks from the mmc core point of view,
before the SDIO func driver gets probed.

[...]

Kind regards
Uffe

Ulf Hansson Oct. 1, 2021, 3:37 p.m. UTC | #5

On Thu, 30 Sept 2021 at 18:51, Jérôme Pouiller
<[e-mail]> wrote:
>
> Hello Ulf,
>
> On Thursday 30 September 2021 12:07:55 CEST Ulf Hansson wrote:
> > On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
> > <[e-mail]> wrote:
> > >
> > > From: Jérôme Pouiller <[e-mail]>
> > >
> > > Signed-off-by: Jérôme Pouiller <[e-mail]>
> > > ---
> > >  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +++++++++++++++++++++
> > >  1 file changed, 261 insertions(+)
> > >  create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> > >
> > > diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c b/drivers/net/wireless/silabs/wfx/bus_sdio.c
> >
> > [...]
> >
> > > +
> > > +static int wfx_sdio_probe(struct sdio_func *func,
> > > +                         const struct sdio_device_id *id)
> > > +{
> > > +       struct device_node *np = func->dev.of_node;
> > > +       struct wfx_sdio_priv *bus;
> > > +       int ret;
> > > +
> > > +       if (func->num != 1) {
> > > +               dev_err(&func->dev, "SDIO function number is %d while it should always be 1 (unsupported chip?)\n",
> > > +                       func->num);
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL);
> > > +       if (!bus)
> > > +               return -ENOMEM;
> > > +
> > > +       if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> > > +               dev_warn(&func->dev, "no compatible device found in DT\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       bus->func = func;
> > > +       bus->of_irq = irq_of_parse_and_map(np, 0);
> > > +       sdio_set_drvdata(func, bus);
> > > +       func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> > > +                             MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> > > +                             MMC_QUIRK_BROKEN_BYTE_MODE_512;
> >
> > I would rather see that you add an SDIO_FIXUP for the SDIO card, to
> > the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
> > this.
>
> In the current patch, these quirks are applied only if the device appears
> in the device tree (see the condition above). If I implement them in
> drivers/mmc/core/quirks.h they will be applied as soon as the device is
> detected. Is it what we want?
>
> Note: we already have had a discussion about the strange VID/PID declared
> by this device:
>   https://www.spinics.net/lists/netdev/msg692577.html

Please, see my other reply to Pali.

>
>
> [...]
> > > +
> > > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > > +       { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
> > > +       { },
> > > +};
> > > +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,
> >
> > Is there no power management? Or do you intend to add that on top?
>
> It seems we already have had this discussion:
>
>   https://lore.kernel.org/netdev/CAPDyKFqJf=vUqpQg3suDCadKrFTkQWFTY_qp=+yDK=[e-mail]/#r
>
> In this thread, Kalle said:
> > Many mac80211 drivers do so that the device is powered off during
> > interface down (ifconfig wlan0 down), and as mac80211 does interface
> > down automatically during suspend, suspend then works without extra
> > handlers.

Yeah, it's been a while since I looked at this, thanks for the pointer.

[...]

Kind regards
Uffe

Kalle Valo Oct. 5, 2021, 5:59 a.m. UTC | #6

Ulf Hansson <[e-mail]> writes:

>> > > +static const struct sdio_device_id wfx_sdio_ids[] = {
>> > > +       { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
>> > > +       { },
>> > > +};
>> > > +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,
>> >
>> > Is there no power management? Or do you intend to add that on top?
>>
>> It seems we already have had this discussion:
>>
>>   https://lore.kernel.org/netdev/CAPDyKFqJf=vUqpQg3suDCadKrFTkQWFTY_qp=+yDK=[e-mail]/#r
>>
>> In this thread, Kalle said:
>> > Many mac80211 drivers do so that the device is powered off during
>> > interface down (ifconfig wlan0 down), and as mac80211 does interface
>> > down automatically during suspend, suspend then works without extra
>> > handlers.
>
> Yeah, it's been a while since I looked at this, thanks for the pointer.

I want to emphasize that what I said above was just a generic comment
about mac80211 drivers and just trying to give some ideas how to solve
this, I did not check how wfx driver behaves in this regard.

Jérôme Pouiller Oct. 5, 2021, 8:14 a.m. UTC | #7

On Friday 1 October 2021 17:23:16 CEST Ulf Hansson wrote:
> On Thu, 30 Sept 2021 at 19:06, Pali Rohár <[e-mail]> wrote:
> > On Thursday 30 September 2021 18:51:09 Jérôme Pouiller wrote:
> > > On Thursday 30 September 2021 12:07:55 CEST Ulf Hansson wrote:
> > > > On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
> > > > <[e-mail]> wrote:
> > > > >
> > > > > From: Jérôme Pouiller <[e-mail]>
> > > > >
> > > > > Signed-off-by: Jérôme Pouiller <[e-mail]>
> > > > > ---
> > > > >  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +++++++++++++++++++++
> > > > >  1 file changed, 261 insertions(+)
> > > > >  create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> > > > >
> > > > > diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c b/drivers/net/wireless/silabs/wfx/bus_sdio.c
> > > >
> > > > [...]
> > > >
> > > > > +
> > > > > +static int wfx_sdio_probe(struct sdio_func *func,
> > > > > +                         const struct sdio_device_id *id)
> > > > > +{
> > > > > +       struct device_node *np = func->dev.of_node;
> > > > > +       struct wfx_sdio_priv *bus;
> > > > > +       int ret;
> > > > > +
> > > > > +       if (func->num != 1) {
> > > > > +               dev_err(&func->dev, "SDIO function number is %d while it should always be 1 (unsupported chip?)\n",
> > > > > +                       func->num);
> > > > > +               return -ENODEV;
> > > > > +       }
> > > > > +
> > > > > +       bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL);
> > > > > +       if (!bus)
> > > > > +               return -ENOMEM;
> > > > > +
> > > > > +       if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> > > > > +               dev_warn(&func->dev, "no compatible device found in DT\n");
> > > > > +               return -ENODEV;
> > > > > +       }
> > > > > +
> > > > > +       bus->func = func;
> > > > > +       bus->of_irq = irq_of_parse_and_map(np, 0);
> > > > > +       sdio_set_drvdata(func, bus);
> > > > > +       func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> > > > > +                             MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> > > > > +                             MMC_QUIRK_BROKEN_BYTE_MODE_512;
> > > >
> > > > I would rather see that you add an SDIO_FIXUP for the SDIO card, to
> > > > the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
> > > > this.
> > >
> > > In the current patch, these quirks are applied only if the device appears
> > > in the device tree (see the condition above). If I implement them in
> > > drivers/mmc/core/quirks.h they will be applied as soon as the device is
> > > detected. Is it what we want?
> > >
> > > Note: we already have had a discussion about the strange VID/PID declared
> > > by this device:
> > >   https://www.spinics.net/lists/netdev/msg692577.html
> >
> > Yes, vendor id 0x0000 is invalid per SDIO spec. So based on this vendor
> > id, it is not possible to write any quirk in mmc/sdio generic code.
> >
> > Ulf, but maybe it could be possible to write quirk based on OF
> > compatible string?
> 
> Yes, that would be better in my opinion.
> 
> We already have DT bindings to describe embedded SDIO cards (a subnode
> to the mmc controller node), so we should be able to extend that I
> think.

So, this feature does not yet exist? Do you consider it is a blocker for
the current patch?

To be honest, I don't really want to take over this change in mmc/core.

Ulf Hansson Oct. 6, 2021, 3:02 p.m. UTC | #8

On Tue, 5 Oct 2021 at 10:14, Jérôme Pouiller <[e-mail]> wrote:
>
> On Friday 1 October 2021 17:23:16 CEST Ulf Hansson wrote:
> > On Thu, 30 Sept 2021 at 19:06, Pali Rohár <[e-mail]> wrote:
> > > On Thursday 30 September 2021 18:51:09 Jérôme Pouiller wrote:
> > > > On Thursday 30 September 2021 12:07:55 CEST Ulf Hansson wrote:
> > > > > On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > From: Jérôme Pouiller <[e-mail]>
> > > > > >
> > > > > > Signed-off-by: Jérôme Pouiller <[e-mail]>
> > > > > > ---
> > > > > >  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +++++++++++++++++++++
> > > > > >  1 file changed, 261 insertions(+)
> > > > > >  create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> > > > > >
> > > > > > diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c b/drivers/net/wireless/silabs/wfx/bus_sdio.c
> > > > >
> > > > > [...]
> > > > >
> > > > > > +
> > > > > > +static int wfx_sdio_probe(struct sdio_func *func,
> > > > > > +                         const struct sdio_device_id *id)
> > > > > > +{
> > > > > > +       struct device_node *np = func->dev.of_node;
> > > > > > +       struct wfx_sdio_priv *bus;
> > > > > > +       int ret;
> > > > > > +
> > > > > > +       if (func->num != 1) {
> > > > > > +               dev_err(&func->dev, "SDIO function number is %d while it should always be 1 (unsupported chip?)\n",
> > > > > > +                       func->num);
> > > > > > +               return -ENODEV;
> > > > > > +       }
> > > > > > +
> > > > > > +       bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL);
> > > > > > +       if (!bus)
> > > > > > +               return -ENOMEM;
> > > > > > +
> > > > > > +       if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> > > > > > +               dev_warn(&func->dev, "no compatible device found in DT\n");
> > > > > > +               return -ENODEV;
> > > > > > +       }
> > > > > > +
> > > > > > +       bus->func = func;
> > > > > > +       bus->of_irq = irq_of_parse_and_map(np, 0);
> > > > > > +       sdio_set_drvdata(func, bus);
> > > > > > +       func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> > > > > > +                             MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> > > > > > +                             MMC_QUIRK_BROKEN_BYTE_MODE_512;
> > > > >
> > > > > I would rather see that you add an SDIO_FIXUP for the SDIO card, to
> > > > > the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
> > > > > this.
> > > >
> > > > In the current patch, these quirks are applied only if the device appears
> > > > in the device tree (see the condition above). If I implement them in
> > > > drivers/mmc/core/quirks.h they will be applied as soon as the device is
> > > > detected. Is it what we want?
> > > >
> > > > Note: we already have had a discussion about the strange VID/PID declared
> > > > by this device:
> > > >   https://www.spinics.net/lists/netdev/msg692577.html
> > >
> > > Yes, vendor id 0x0000 is invalid per SDIO spec. So based on this vendor
> > > id, it is not possible to write any quirk in mmc/sdio generic code.
> > >
> > > Ulf, but maybe it could be possible to write quirk based on OF
> > > compatible string?
> >
> > Yes, that would be better in my opinion.
> >
> > We already have DT bindings to describe embedded SDIO cards (a subnode
> > to the mmc controller node), so we should be able to extend that I
> > think.
>
> So, this feature does not yet exist? Do you consider it is a blocker for
> the current patch?

Yes, sorry. I think we should avoid unnecessary hacks in SDIO func
drivers, especially those that deserve to be fixed in the mmc core.

Moreover, we already support the similar thing for eMMC cards, plus
that most parts are already done for SDIO too.

>
> To be honest, I don't really want to take over this change in mmc/core.

I understand. Allow me a couple of days, then I can post a patch to
help you out.

>
> --
> Jérôme Pouiller

Kind regards
Uffe

Jérôme Pouiller Oct. 6, 2021, 3:42 p.m. UTC | #9

On Wednesday 6 October 2021 17:02:07 CEST Ulf Hansson wrote:
> On Tue, 5 Oct 2021 at 10:14, Jérôme Pouiller <[e-mail]> wrote:
> > On Friday 1 October 2021 17:23:16 CEST Ulf Hansson wrote:
> > > On Thu, 30 Sept 2021 at 19:06, Pali Rohár <[e-mail]> wrote:
> > > > On Thursday 30 September 2021 18:51:09 Jérôme Pouiller wrote:
> > > > > On Thursday 30 September 2021 12:07:55 CEST Ulf Hansson wrote:
> > > > > > On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
> > > > > > <[e-mail]> wrote:
> > > > > > >
> > > > > > > From: Jérôme Pouiller <[e-mail]>
> > > > > > >
> > > > > > > Signed-off-by: Jérôme Pouiller <[e-mail]>
> > > > > > > ---
> > > > > > >  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +++++++++++++++++++++
> > > > > > >  1 file changed, 261 insertions(+)
> > > > > > >  create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> > > > > > >
> > > > > > > diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c b/drivers/net/wireless/silabs/wfx/bus_sdio.c
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > +
> > > > > > > +static int wfx_sdio_probe(struct sdio_func *func,
> > > > > > > +                         const struct sdio_device_id *id)
> > > > > > > +{
> > > > > > > +       struct device_node *np = func->dev.of_node;
> > > > > > > +       struct wfx_sdio_priv *bus;
> > > > > > > +       int ret;
> > > > > > > +
> > > > > > > +       if (func->num != 1) {
> > > > > > > +               dev_err(&func->dev, "SDIO function number is %d while it should always be 1 (unsupported chip?)\n",
> > > > > > > +                       func->num);
> > > > > > > +               return -ENODEV;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL);
> > > > > > > +       if (!bus)
> > > > > > > +               return -ENOMEM;
> > > > > > > +
> > > > > > > +       if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> > > > > > > +               dev_warn(&func->dev, "no compatible device found in DT\n");
> > > > > > > +               return -ENODEV;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       bus->func = func;
> > > > > > > +       bus->of_irq = irq_of_parse_and_map(np, 0);
> > > > > > > +       sdio_set_drvdata(func, bus);
> > > > > > > +       func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> > > > > > > +                             MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> > > > > > > +                             MMC_QUIRK_BROKEN_BYTE_MODE_512;
> > > > > >
> > > > > > I would rather see that you add an SDIO_FIXUP for the SDIO card, to
> > > > > > the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
> > > > > > this.
> > > > >
> > > > > In the current patch, these quirks are applied only if the device appears
> > > > > in the device tree (see the condition above). If I implement them in
> > > > > drivers/mmc/core/quirks.h they will be applied as soon as the device is
> > > > > detected. Is it what we want?
> > > > >
> > > > > Note: we already have had a discussion about the strange VID/PID declared
> > > > > by this device:
> > > > >   https://www.spinics.net/lists/netdev/msg692577.html
> > > >
> > > > Yes, vendor id 0x0000 is invalid per SDIO spec. So based on this vendor
> > > > id, it is not possible to write any quirk in mmc/sdio generic code.
> > > >
> > > > Ulf, but maybe it could be possible to write quirk based on OF
> > > > compatible string?
> > >
> > > Yes, that would be better in my opinion.
> > >
> > > We already have DT bindings to describe embedded SDIO cards (a subnode
> > > to the mmc controller node), so we should be able to extend that I
> > > think.
> >
> > So, this feature does not yet exist? Do you consider it is a blocker for
> > the current patch?
> 
> Yes, sorry. I think we should avoid unnecessary hacks in SDIO func
> drivers, especially those that deserve to be fixed in the mmc core.
> 
> Moreover, we already support the similar thing for eMMC cards, plus
> that most parts are already done for SDIO too.
> 
> >
> > To be honest, I don't really want to take over this change in mmc/core.
> 
> I understand. Allow me a couple of days, then I can post a patch to
> help you out.

Great! Thank you. I apologize for the extra work due to this invalid
vendor id.

Kalle Valo Oct. 7, 2021, 8:26 a.m. UTC | #10

Jérôme Pouiller <[e-mail]> writes:

> On Wednesday 6 October 2021 17:02:07 CEST Ulf Hansson wrote:
>> On Tue, 5 Oct 2021 at 10:14, Jérôme Pouiller <[e-mail]> wrote:
>> > On Friday 1 October 2021 17:23:16 CEST Ulf Hansson wrote:
>> > > On Thu, 30 Sept 2021 at 19:06, Pali Rohár <[e-mail]> wrote:
>> > > > On Thursday 30 September 2021 18:51:09 Jérôme Pouiller wrote:
>> > > > > On Thursday 30 September 2021 12:07:55 CEST Ulf Hansson wrote:
>> > > > > > On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
>> > > > > > <[e-mail]> wrote:
>> > > > > > >
>> > > > > > > From: Jérôme Pouiller <[e-mail]>
>> > > > > > >
>> > > > > > > Signed-off-by: Jérôme Pouiller <[e-mail]>
>> > > > > > > ---
>> > > > > > >  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +++++++++++++++++++++
>> > > > > > >  1 file changed, 261 insertions(+)
>> > > > > > >  create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
>> > > > > > >
>> > > > > > > diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c
>> > > > > > > b/drivers/net/wireless/silabs/wfx/bus_sdio.c
>> > > > > >
>> > > > > > [...]
>> > > > > >
>> > > > > > > +
>> > > > > > > +static int wfx_sdio_probe(struct sdio_func *func,
>> > > > > > > +                         const struct sdio_device_id *id)
>> > > > > > > +{
>> > > > > > > +       struct device_node *np = func->dev.of_node;
>> > > > > > > +       struct wfx_sdio_priv *bus;
>> > > > > > > +       int ret;
>> > > > > > > +
>> > > > > > > +       if (func->num != 1) {
>> > > > > > > + dev_err(&func->dev, "SDIO function number is %d while
>> > > > > > > it should always be 1 (unsupported chip?)\n",
>> > > > > > > +                       func->num);
>> > > > > > > +               return -ENODEV;
>> > > > > > > +       }
>> > > > > > > +
>> > > > > > > +       bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL);
>> > > > > > > +       if (!bus)
>> > > > > > > +               return -ENOMEM;
>> > > > > > > +
>> > > > > > > +       if (!np || !of_match_node(wfx_sdio_of_match, np)) {
>> > > > > > > + dev_warn(&func->dev, "no compatible device found in
>> > > > > > > DT\n");
>> > > > > > > +               return -ENODEV;
>> > > > > > > +       }
>> > > > > > > +
>> > > > > > > +       bus->func = func;
>> > > > > > > +       bus->of_irq = irq_of_parse_and_map(np, 0);
>> > > > > > > +       sdio_set_drvdata(func, bus);
>> > > > > > > +       func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
>> > > > > > > +                             MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
>> > > > > > > +                             MMC_QUIRK_BROKEN_BYTE_MODE_512;
>> > > > > >
>> > > > > > I would rather see that you add an SDIO_FIXUP for the SDIO card, to
>> > > > > > the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
>> > > > > > this.
>> > > > >
>> > > > > In the current patch, these quirks are applied only if the device appears
>> > > > > in the device tree (see the condition above). If I implement them in
>> > > > > drivers/mmc/core/quirks.h they will be applied as soon as the device is
>> > > > > detected. Is it what we want?
>> > > > >
>> > > > > Note: we already have had a discussion about the strange VID/PID declared
>> > > > > by this device:
>> > > > >   https://www.spinics.net/lists/netdev/msg692577.html
>> > > >
>> > > > Yes, vendor id 0x0000 is invalid per SDIO spec. So based on this vendor
>> > > > id, it is not possible to write any quirk in mmc/sdio generic code.
>> > > >
>> > > > Ulf, but maybe it could be possible to write quirk based on OF
>> > > > compatible string?
>> > >
>> > > Yes, that would be better in my opinion.
>> > >
>> > > We already have DT bindings to describe embedded SDIO cards (a subnode
>> > > to the mmc controller node), so we should be able to extend that I
>> > > think.
>> >
>> > So, this feature does not yet exist? Do you consider it is a blocker for
>> > the current patch?
>> 
>> Yes, sorry. I think we should avoid unnecessary hacks in SDIO func
>> drivers, especially those that deserve to be fixed in the mmc core.
>> 
>> Moreover, we already support the similar thing for eMMC cards, plus
>> that most parts are already done for SDIO too.
>> 
>> >
>> > To be honest, I don't really want to take over this change in mmc/core.
>> 
>> I understand. Allow me a couple of days, then I can post a patch to
>> help you out.
>
> Great! Thank you. I apologize for the extra work due to this invalid
> vendor id.

BTW please escalate in your company how HORRIBLE it is that you
manufacture SDIO devices without proper device ids, and make sure that
all your future devices have officially assigned ids. I cannot stress
enough how important that is for the Linux community!

Pali Rohár Oct. 7, 2021, 10:24 a.m. UTC | #11

On Thursday 07 October 2021 11:26:42 Kalle Valo wrote:
> Jérôme Pouiller <[e-mail]> writes:
> > On Wednesday 6 October 2021 17:02:07 CEST Ulf Hansson wrote:
> >> On Tue, 5 Oct 2021 at 10:14, Jérôme Pouiller <[e-mail]> wrote:
> >> > On Friday 1 October 2021 17:23:16 CEST Ulf Hansson wrote:
> >> > > On Thu, 30 Sept 2021 at 19:06, Pali Rohár <[e-mail]> wrote:
> >> > > > On Thursday 30 September 2021 18:51:09 Jérôme Pouiller wrote:
> >> > > > > On Thursday 30 September 2021 12:07:55 CEST Ulf Hansson wrote:
> >> > > > > > On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
> >> > > > > > <[e-mail]> wrote:
> >> > > > > > >
> >> > > > > > > From: Jérôme Pouiller <[e-mail]>
> >> > > > > > >
> >> > > > > > > Signed-off-by: Jérôme Pouiller <[e-mail]>
> >> > > > > > > ---
> >> > > > > > >  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +++++++++++++++++++++
> >> > > > > > >  1 file changed, 261 insertions(+)
> >> > > > > > >  create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> >> > > > > > >
> >> > > > > > > diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c
> >> > > > > > > b/drivers/net/wireless/silabs/wfx/bus_sdio.c
> >> > > > > >
> >> > > > > > [...]
> >> > > > > >
> >> > > > > > > +
> >> > > > > > > +static int wfx_sdio_probe(struct sdio_func *func,
> >> > > > > > > +                         const struct sdio_device_id *id)
> >> > > > > > > +{
> >> > > > > > > +       struct device_node *np = func->dev.of_node;
> >> > > > > > > +       struct wfx_sdio_priv *bus;
> >> > > > > > > +       int ret;
> >> > > > > > > +
> >> > > > > > > +       if (func->num != 1) {
> >> > > > > > > + dev_err(&func->dev, "SDIO function number is %d while
> >> > > > > > > it should always be 1 (unsupported chip?)\n",
> >> > > > > > > +                       func->num);
> >> > > > > > > +               return -ENODEV;
> >> > > > > > > +       }
> >> > > > > > > +
> >> > > > > > > +       bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL);
> >> > > > > > > +       if (!bus)
> >> > > > > > > +               return -ENOMEM;
> >> > > > > > > +
> >> > > > > > > +       if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> >> > > > > > > + dev_warn(&func->dev, "no compatible device found in
> >> > > > > > > DT\n");
> >> > > > > > > +               return -ENODEV;
> >> > > > > > > +       }
> >> > > > > > > +
> >> > > > > > > +       bus->func = func;
> >> > > > > > > +       bus->of_irq = irq_of_parse_and_map(np, 0);
> >> > > > > > > +       sdio_set_drvdata(func, bus);
> >> > > > > > > +       func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> >> > > > > > > +                             MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> >> > > > > > > +                             MMC_QUIRK_BROKEN_BYTE_MODE_512;
> >> > > > > >
> >> > > > > > I would rather see that you add an SDIO_FIXUP for the SDIO card, to
> >> > > > > > the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
> >> > > > > > this.
> >> > > > >
> >> > > > > In the current patch, these quirks are applied only if the device appears
> >> > > > > in the device tree (see the condition above). If I implement them in
> >> > > > > drivers/mmc/core/quirks.h they will be applied as soon as the device is
> >> > > > > detected. Is it what we want?
> >> > > > >
> >> > > > > Note: we already have had a discussion about the strange VID/PID declared
> >> > > > > by this device:
> >> > > > >   https://www.spinics.net/lists/netdev/msg692577.html
> >> > > >
> >> > > > Yes, vendor id 0x0000 is invalid per SDIO spec. So based on this vendor
> >> > > > id, it is not possible to write any quirk in mmc/sdio generic code.
> >> > > >
> >> > > > Ulf, but maybe it could be possible to write quirk based on OF
> >> > > > compatible string?
> >> > >
> >> > > Yes, that would be better in my opinion.
> >> > >
> >> > > We already have DT bindings to describe embedded SDIO cards (a subnode
> >> > > to the mmc controller node), so we should be able to extend that I
> >> > > think.
> >> >
> >> > So, this feature does not yet exist? Do you consider it is a blocker for
> >> > the current patch?
> >> 
> >> Yes, sorry. I think we should avoid unnecessary hacks in SDIO func
> >> drivers, especially those that deserve to be fixed in the mmc core.
> >> 
> >> Moreover, we already support the similar thing for eMMC cards, plus
> >> that most parts are already done for SDIO too.
> >> 
> >> >
> >> > To be honest, I don't really want to take over this change in mmc/core.
> >> 
> >> I understand. Allow me a couple of days, then I can post a patch to
> >> help you out.
> >
> > Great! Thank you. I apologize for the extra work due to this invalid
> > vendor id.
> 
> BTW please escalate in your company how HORRIBLE it is that you
> manufacture SDIO devices without proper device ids, and make sure that
> all your future devices have officially assigned ids. I cannot stress
> enough how important that is for the Linux community!

Absolutely! Please really escalate this problem in your company and
properly ask USB-IF for assigning PCMCIA vendor ID as USB-IF maintains
PCMCIA vendor database and PCMCIA ids are used in SDIO devices:
https://lore.kernel.org/linux-mmc/20210607140216.64iuprp3siggslrk@pali/

Patch

diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c b/drivers/net/wireless/silabs/wfx/bus_sdio.c
new file mode 100644
index 000000000000..869ecb7d99db
--- /dev/null
+++ b/drivers/net/wireless/silabs/wfx/bus_sdio.c
@@ -0,0 +1,261 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SDIO interface.
+ *
+ * Copyright (c) 2017-2020, Silicon Laboratories, Inc.
+ * Copyright (c) 2010, ST-Ericsson
+ */
+#include <linux/module.h>
+#include <linux/mmc/sdio.h>
+#include <linux/mmc/sdio_func.h>
+#include <linux/mmc/sdio_ids.h>
+#include <linux/mmc/card.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
+#include <linux/irq.h>
+
+#include "bus.h"
+#include "wfx.h"
+#include "hwio.h"
+#include "main.h"
+#include "bh.h"
+
+static const struct wfx_platform_data wfx_sdio_pdata = {
+	.file_fw = "wfm_wf200",
+	.file_pds = "wf200.pds",
+};
+
+struct wfx_sdio_priv {
+	struct sdio_func *func;
+	struct wfx_dev *core;
+	u8 buf_id_tx;
+	u8 buf_id_rx;
+	int of_irq;
+};
+
+static int wfx_sdio_copy_from_io(void *priv, unsigned int reg_id,
+				 void *dst, size_t count)
+{
+	struct wfx_sdio_priv *bus = priv;
+	unsigned int sdio_addr = reg_id << 2;
+	int ret;
+
+	WARN(reg_id > 7, "chip only has 7 registers");
+	WARN(((uintptr_t)dst) & 3, "unaligned buffer size");
+	WARN(count & 3, "unaligned buffer address");
+
+	/* Use queue mode buffers */
+	if (reg_id == WFX_REG_IN_OUT_QUEUE)
+		sdio_addr |= (bus->buf_id_rx + 1) << 7;
+	ret = sdio_memcpy_fromio(bus->func, dst, sdio_addr, count);
+	if (!ret && reg_id == WFX_REG_IN_OUT_QUEUE)
+		bus->buf_id_rx = (bus->buf_id_rx + 1) % 4;
+
+	return ret;
+}
+
+static int wfx_sdio_copy_to_io(void *priv, unsigned int reg_id,
+			       const void *src, size_t count)
+{
+	struct wfx_sdio_priv *bus = priv;
+	unsigned int sdio_addr = reg_id << 2;
+	int ret;
+
+	WARN(reg_id > 7, "chip only has 7 registers");
+	WARN(((uintptr_t)src) & 3, "unaligned buffer size");
+	WARN(count & 3, "unaligned buffer address");
+
+	/* Use queue mode buffers */
+	if (reg_id == WFX_REG_IN_OUT_QUEUE)
+		sdio_addr |= bus->buf_id_tx << 7;
+	/* FIXME: discards 'const' qualifier for src */
+	ret = sdio_memcpy_toio(bus->func, sdio_addr, (void *)src, count);
+	if (!ret && reg_id == WFX_REG_IN_OUT_QUEUE)
+		bus->buf_id_tx = (bus->buf_id_tx + 1) % 32;
+
+	return ret;
+}
+
+static void wfx_sdio_lock(void *priv)
+{
+	struct wfx_sdio_priv *bus = priv;
+
+	sdio_claim_host(bus->func);
+}
+
+static void wfx_sdio_unlock(void *priv)
+{
+	struct wfx_sdio_priv *bus = priv;
+
+	sdio_release_host(bus->func);
+}
+
+static void wfx_sdio_irq_handler(struct sdio_func *func)
+{
+	struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
+
+	wfx_bh_request_rx(bus->core);
+}
+
+static irqreturn_t wfx_sdio_irq_handler_ext(int irq, void *priv)
+{
+	struct wfx_sdio_priv *bus = priv;
+
+	sdio_claim_host(bus->func);
+	wfx_bh_request_rx(bus->core);
+	sdio_release_host(bus->func);
+	return IRQ_HANDLED;
+}
+
+static int wfx_sdio_irq_subscribe(void *priv)
+{
+	struct wfx_sdio_priv *bus = priv;
+	u32 flags;
+	int ret;
+	u8 cccr;
+
+	if (!bus->of_irq) {
+		sdio_claim_host(bus->func);
+		ret = sdio_claim_irq(bus->func, wfx_sdio_irq_handler);
+		sdio_release_host(bus->func);
+		return ret;
+	}
+
+	flags = irq_get_trigger_type(bus->of_irq);
+	if (!flags)
+		flags = IRQF_TRIGGER_HIGH;
+	flags |= IRQF_ONESHOT;
+	ret = devm_request_threaded_irq(&bus->func->dev, bus->of_irq, NULL,
+					wfx_sdio_irq_handler_ext, flags,
+					"wfx", bus);
+	if (ret)
+		return ret;
+	sdio_claim_host(bus->func);
+	cccr = sdio_f0_readb(bus->func, SDIO_CCCR_IENx, NULL);
+	cccr |= BIT(0);
+	cccr |= BIT(bus->func->num);
+	sdio_f0_writeb(bus->func, cccr, SDIO_CCCR_IENx, NULL);
+	sdio_release_host(bus->func);
+	return 0;
+}
+
+static int wfx_sdio_irq_unsubscribe(void *priv)
+{
+	struct wfx_sdio_priv *bus = priv;
+	int ret;
+
+	if (bus->of_irq)
+		devm_free_irq(&bus->func->dev, bus->of_irq, bus);
+	sdio_claim_host(bus->func);
+	ret = sdio_release_irq(bus->func);
+	sdio_release_host(bus->func);
+	return ret;
+}
+
+static size_t wfx_sdio_align_size(void *priv, size_t size)
+{
+	struct wfx_sdio_priv *bus = priv;
+
+	return sdio_align_size(bus->func, size);
+}
+
+static const struct hwbus_ops wfx_sdio_hwbus_ops = {
+	.copy_from_io = wfx_sdio_copy_from_io,
+	.copy_to_io = wfx_sdio_copy_to_io,
+	.irq_subscribe = wfx_sdio_irq_subscribe,
+	.irq_unsubscribe = wfx_sdio_irq_unsubscribe,
+	.lock			= wfx_sdio_lock,
+	.unlock			= wfx_sdio_unlock,
+	.align_size		= wfx_sdio_align_size,
+};
+
+static const struct of_device_id wfx_sdio_of_match[] = {
+	{ .compatible = "silabs,wfx-sdio" },
+	{ .compatible = "silabs,wf200" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, wfx_sdio_of_match);
+
+static int wfx_sdio_probe(struct sdio_func *func,
+			  const struct sdio_device_id *id)
+{
+	struct device_node *np = func->dev.of_node;
+	struct wfx_sdio_priv *bus;
+	int ret;
+
+	if (func->num != 1) {
+		dev_err(&func->dev, "SDIO function number is %d while it should always be 1 (unsupported chip?)\n",
+			func->num);
+		return -ENODEV;
+	}
+
+	bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL);
+	if (!bus)
+		return -ENOMEM;
+
+	if (!np || !of_match_node(wfx_sdio_of_match, np)) {
+		dev_warn(&func->dev, "no compatible device found in DT\n");
+		return -ENODEV;
+	}
+
+	bus->func = func;
+	bus->of_irq = irq_of_parse_and_map(np, 0);
+	sdio_set_drvdata(func, bus);
+	func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
+			      MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
+			      MMC_QUIRK_BROKEN_BYTE_MODE_512;
+
+	sdio_claim_host(func);
+	ret = sdio_enable_func(func);
+	/* Block of 64 bytes is more efficient than 512B for frame sizes < 4k */
+	sdio_set_block_size(func, 64);
+	sdio_release_host(func);
+	if (ret)
+		return ret;
+
+	bus->core = wfx_init_common(&func->dev, &wfx_sdio_pdata,
+				    &wfx_sdio_hwbus_ops, bus);
+	if (!bus->core) {
+		ret = -EIO;
+		goto sdio_release;
+	}
+
+	ret = wfx_probe(bus->core);
+	if (ret)
+		goto sdio_release;
+
+	return 0;
+
+sdio_release:
+	sdio_claim_host(func);
+	sdio_disable_func(func);
+	sdio_release_host(func);
+	return ret;
+}
+
+static void wfx_sdio_remove(struct sdio_func *func)
+{
+	struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
+
+	wfx_release(bus->core);
+	sdio_claim_host(func);
+	sdio_disable_func(func);
+	sdio_release_host(func);
+}
+
+static const struct sdio_device_id wfx_sdio_ids[] = {
+	{ SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
+	{ },
+};
+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,
+	}
+};