Date:   Thu, 07 Oct 2021 11:26:42 +0300
From:   Kalle Valo <kvalo@...eaurora.org>
To:     Jérôme Pouiller <jerome.pouiller@...abs.com>
Cc:     Ulf Hansson <ulf.hansson@...aro.org>,
        Pali Rohár <pali@...nel.org>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        driverdevel <devel@...verdev.osuosl.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "David S . Miller" <davem@...emloft.net>,
        DTML <devicetree@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        linux-mmc <linux-mmc@...r.kernel.org>
Subject: Re: [PATCH v7 08/24] wfx: add bus_sdio.c

Jérôme Pouiller <jerome.pouiller@...abs.com> 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 <jerome.pouiller@...abs.com> wrote:
>> > On Friday 1 October 2021 17:23:16 CEST Ulf Hansson wrote:
>> > > On Thu, 30 Sept 2021 at 19:06, Pali Rohár <pali@...nel.org> 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
>> > > > > > <Jerome.Pouiller@...abs.com> wrote:
>> > > > > > >
>> > > > > > > From: Jérôme Pouiller <jerome.pouiller@...abs.com>
>> > > > > > >
>> > > > > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@...abs.com>
>> > > > > > > ---
>> > > > > > >  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!

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Powered by blists - more mailing lists