[RFC,v4,2/6] mmc: core: allow to match the device tree to apply quirks

Commit Message

H. Nikolaus Schaller Nov. 5, 2021, 9:05 a.m. UTC

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

MMC subsystem provides a way to apply quirks when a device match some
properties (VID, PID, etc...) Unfortunately, some SDIO devices does not
comply with the SDIO specification and does not provide reliable VID/PID
(eg. Silabs WF200).

So, the drivers for these devices rely on device tree to identify the
device.

This patch allows the MMC to also rely on the device tree to apply a
quirk.

Signed-off-by: Jérôme Pouiller <[e-mail]>
---
 drivers/mmc/core/card.h   |  3 +++
 drivers/mmc/core/quirks.h | 17 +++++++++++++++++
 2 files changed, 20 insertions(+)

Comments

Jérôme Pouiller Nov. 5, 2021, 2:27 p.m. UTC | #1

On Friday 5 November 2021 10:05:47 CET H. Nikolaus Schaller wrote:
> From: Jérôme Pouiller <[e-mail]>
> 
> MMC subsystem provides a way to apply quirks when a device match some
> properties (VID, PID, etc...) Unfortunately, some SDIO devices does not
> comply with the SDIO specification and does not provide reliable VID/PID
> (eg. Silabs WF200).
> 
> So, the drivers for these devices rely on device tree to identify the
> device.
> 
> This patch allows the MMC to also rely on the device tree to apply a
> quirk.
> 
> Signed-off-by: Jérôme Pouiller <[e-mail]>

Thank you for to have taken care of that (Maybe, you would like to add a
"Co-developed-by:" tag).


> ---
>  drivers/mmc/core/card.h   |  3 +++
>  drivers/mmc/core/quirks.h | 17 +++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> index 7bd392d55cfa5..a3204c19861a4 100644
> --- a/drivers/mmc/core/card.h
> +++ b/drivers/mmc/core/card.h
> @@ -59,6 +59,9 @@ struct mmc_fixup {
>         /* for MMC cards */
>         unsigned int ext_csd_rev;
> 
> +       /* Match against functions declared in device tree */
> +       const char **of_compatible;
> +
>         void (*vendor_fixup)(struct mmc_card *card, int data);
>         int data;
>  };
> diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> index c7ef2d14b359f..ee591fd8aeca2 100644
> --- a/drivers/mmc/core/quirks.h
> +++ b/drivers/mmc/core/quirks.h
> @@ -10,6 +10,7 @@
>   *
>   */
> 
> +#include <linux/of.h>
>  #include <linux/mmc/sdio_ids.h>
> 
>  #include "card.h"
> @@ -145,6 +146,19 @@ static const struct mmc_fixup __maybe_unused sdio_fixup_methods[] = {
>         END_FIXUP
>  };
> 
> +static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
> +                                                const char *const *compat_list)
> +{
> +       struct device_node *np;
> +
> +       for_each_child_of_node(mmc_dev(card->host)->of_node, np) {
> +               if (of_device_compatible_match(np, compat_list))
> +                       return true;

Intel robot complains about of_device_compatible_match():

    ERROR: modpost: "of_device_compatible_match" [drivers/mmc/core/mmc_core.ko] undefined!

I think we have to add this line:

    EXPORT_SYMBOL(of_device_compatible_match);

in drivers/of/base.c

> +       }
> +
> +       return false;
> +}
> +
>  static inline void mmc_fixup_device(struct mmc_card *card,
>                                     const struct mmc_fixup *table)
>  {
> @@ -173,6 +187,9 @@ static inline void mmc_fixup_device(struct mmc_card *card,
>                         continue;
>                 if (rev < f->rev_start || rev > f->rev_end)
>                         continue;
> +               if (f->of_compatible &&
> +                   !mmc_fixup_of_compatible_match(card, f->of_compatible))
> +                       continue;
> 
>                 dev_dbg(&card->dev, "calling %ps\n", f->vendor_fixup);
>                 f->vendor_fixup(card, f->data);
> --
> 2.33.0
> 
>

H. Nikolaus Schaller Nov. 6, 2021, 2:31 p.m. UTC | #2

Hi Jérôme,

> Am 05.11.2021 um 15:27 schrieb Jérôme Pouiller <[e-mail]>:
> 
> On Friday 5 November 2021 10:05:47 CET H. Nikolaus Schaller wrote:
>> From: Jérôme Pouiller <[e-mail]>
>> 
>> MMC subsystem provides a way to apply quirks when a device match some
>> properties (VID, PID, etc...) Unfortunately, some SDIO devices does not
>> comply with the SDIO specification and does not provide reliable VID/PID
>> (eg. Silabs WF200).
>> 
>> So, the drivers for these devices rely on device tree to identify the
>> device.
>> 
>> This patch allows the MMC to also rely on the device tree to apply a
>> quirk.
>> 
>> Signed-off-by: Jérôme Pouiller <[e-mail]>
> 
> Thank you for to have taken care of that (Maybe, you would like to add a
> "Co-developed-by:" tag).

Well, I just have taken your and Ulf's proposal and done 90% copy&paste.
So there wasn't much development, just editing...

> 
> 
>> ---
>> drivers/mmc/core/card.h   |  3 +++
>> drivers/mmc/core/quirks.h | 17 +++++++++++++++++
>> 2 files changed, 20 insertions(+)
>> 
>> +static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
>> +                                                const char *const *compat_list)
>> +{
>> +       struct device_node *np;
>> +
>> +       for_each_child_of_node(mmc_dev(card->host)->of_node, np) {
>> +               if (of_device_compatible_match(np, compat_list))
>> +                       return true;
> 
> Intel robot complains about of_device_compatible_match():
> 
>    ERROR: modpost: "of_device_compatible_match" [drivers/mmc/core/mmc_core.ko] undefined!
> 
> I think we have to add this line:
> 
>    EXPORT_SYMBOL(of_device_compatible_match);
> 
> in drivers/of/base.c

I had seen the krobot message as well but could not figure out
what it meant...

But with your hint it indeed looks like an omission in drivers/of/base.c
Having something exported in include/linux/of.h but code not
marked EXPORT_SYMBOL...

That needs a separate patch. I'll add one with a

Reported-by: kernel test robot <[e-mail]>
Reported-by: Jérôme Pouiller <[e-mail]>

and some Fixes: tag. Since it has a different audience I think
I should post it separately.

BTW: krobot noted the same issue for mmc_of_find_child_device()
in drivers/mmc/core/core.c (which we do not touch in this series).
But maybe it should be fixed as well.

So let's wait for more comments and then I may distribute a [PATCH v1].
Or should I do a [PATCH v5] to continue version counting?

BR,
Nikolaus

Ulf Hansson Nov. 8, 2021, 3 p.m. UTC | #3

On Sat, 6 Nov 2021 at 15:31, H. Nikolaus Schaller <[e-mail]> wrote:
>
> Hi Jérôme,
>
> > Am 05.11.2021 um 15:27 schrieb Jérôme Pouiller <[e-mail]>:
> >
> > On Friday 5 November 2021 10:05:47 CET H. Nikolaus Schaller wrote:
> >> From: Jérôme Pouiller <[e-mail]>
> >>
> >> MMC subsystem provides a way to apply quirks when a device match some
> >> properties (VID, PID, etc...) Unfortunately, some SDIO devices does not
> >> comply with the SDIO specification and does not provide reliable VID/PID
> >> (eg. Silabs WF200).
> >>
> >> So, the drivers for these devices rely on device tree to identify the
> >> device.
> >>
> >> This patch allows the MMC to also rely on the device tree to apply a
> >> quirk.
> >>
> >> Signed-off-by: Jérôme Pouiller <[e-mail]>
> >
> > Thank you for to have taken care of that (Maybe, you would like to add a
> > "Co-developed-by:" tag).
>
> Well, I just have taken your and Ulf's proposal and done 90% copy&paste.
> So there wasn't much development, just editing...
>
> >
> >
> >> ---
> >> drivers/mmc/core/card.h   |  3 +++
> >> drivers/mmc/core/quirks.h | 17 +++++++++++++++++
> >> 2 files changed, 20 insertions(+)
> >>
> >> +static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
> >> +                                                const char *const *compat_list)

After a second thought, I am not sure we really need a list of
compatibles here. The quirks we may want to apply should be specific
per device and most likely not shared among a family of devices, don't
you think?

> >> +{
> >> +       struct device_node *np;
> >> +
> >> +       for_each_child_of_node(mmc_dev(card->host)->of_node, np) {
> >> +               if (of_device_compatible_match(np, compat_list))
> >> +                       return true;
> >
> > Intel robot complains about of_device_compatible_match():
> >
> >    ERROR: modpost: "of_device_compatible_match" [drivers/mmc/core/mmc_core.ko] undefined!
> >
> > I think we have to add this line:
> >
> >    EXPORT_SYMBOL(of_device_compatible_match);
> >
> > in drivers/of/base.c

If we change to use one compatible string, rather than a list - then
we can use of_device_is_compatible() instead, which is already
properly exported with EXPORT_SYMBOL().

>
> I had seen the krobot message as well but could not figure out
> what it meant...
>
> But with your hint it indeed looks like an omission in drivers/of/base.c
> Having something exported in include/linux/of.h but code not
> marked EXPORT_SYMBOL...
>
> That needs a separate patch. I'll add one with a
>
> Reported-by: kernel test robot <[e-mail]>
> Reported-by: Jérôme Pouiller <[e-mail]>
>
> and some Fixes: tag. Since it has a different audience I think
> I should post it separately.
>
> BTW: krobot noted the same issue for mmc_of_find_child_device()
> in drivers/mmc/core/core.c (which we do not touch in this series).
> But maybe it should be fixed as well.

If there is an existing problem, please send a separate fix/patch for that.

>
> So let's wait for more comments and then I may distribute a [PATCH v1].
> Or should I do a [PATCH v5] to continue version counting?

I suggest moving from RFC into using PATCH v1, as this isn't really an
RFC any more.

>
> BR,
> Nikolaus
>

Kind regards
Uffe

Jérôme Pouiller Nov. 8, 2021, 3:34 p.m. UTC | #4

On Monday 8 November 2021 16:00:02 CET Ulf Hansson wrote:
> On Sat, 6 Nov 2021 at 15:31, H. Nikolaus Schaller <[e-mail]> wrote:
> >
> > Hi Jérôme,
> >
> > > Am 05.11.2021 um 15:27 schrieb Jérôme Pouiller <[e-mail]>:
> > >
> > > On Friday 5 November 2021 10:05:47 CET H. Nikolaus Schaller wrote:
> > >> From: Jérôme Pouiller <[e-mail]>
> > >>
> > >> MMC subsystem provides a way to apply quirks when a device match some
> > >> properties (VID, PID, etc...) Unfortunately, some SDIO devices does not
> > >> comply with the SDIO specification and does not provide reliable VID/PID
> > >> (eg. Silabs WF200).
> > >>
> > >> So, the drivers for these devices rely on device tree to identify the
> > >> device.
> > >>
> > >> This patch allows the MMC to also rely on the device tree to apply a
> > >> quirk.
> > >>
> > >> Signed-off-by: Jérôme Pouiller <[e-mail]>

[...]

> > >> ---
> > >> drivers/mmc/core/card.h   |  3 +++
> > >> drivers/mmc/core/quirks.h | 17 +++++++++++++++++
> > >> 2 files changed, 20 insertions(+)
> > >>
> > >> +static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
> > >> +                                                const char *const *compat_list)
> 
> After a second thought, I am not sure we really need a list of
> compatibles here. The quirks we may want to apply should be specific
> per device and most likely not shared among a family of devices, don't
> you think?

Indeed. I dislike to have to declare a list of compatible device (see 
wl1251_compatible_list in patch 5) outside of the fixup list.

If I have several devices, I prefer to copy-paste a few lines in the 
mmc_fixup list (for the WFX driver, I have 4 devices to declare).

H. Nikolaus Schaller Nov. 8, 2021, 4:01 p.m. UTC | #5

> Am 08.11.2021 um 16:34 schrieb Jérôme Pouiller <[e-mail]>:
> 
> On Monday 8 November 2021 16:00:02 CET Ulf Hansson wrote:
>> On Sat, 6 Nov 2021 at 15:31, H. Nikolaus Schaller <[e-mail]> wrote:
>>> 
>>> Hi Jérôme,
>>> 
>>>> Am 05.11.2021 um 15:27 schrieb Jérôme Pouiller <[e-mail]>:
>>>> 
>>>> On Friday 5 November 2021 10:05:47 CET H. Nikolaus Schaller wrote:
>>>>> From: Jérôme Pouiller <[e-mail]>
>>>>> 
>>>>> MMC subsystem provides a way to apply quirks when a device match some
>>>>> properties (VID, PID, etc...) Unfortunately, some SDIO devices does not
>>>>> comply with the SDIO specification and does not provide reliable VID/PID
>>>>> (eg. Silabs WF200).
>>>>> 
>>>>> So, the drivers for these devices rely on device tree to identify the
>>>>> device.
>>>>> 
>>>>> This patch allows the MMC to also rely on the device tree to apply a
>>>>> quirk.
>>>>> 
>>>>> Signed-off-by: Jérôme Pouiller <[e-mail]>
> 
> [...]
> 
>>>>> ---
>>>>> drivers/mmc/core/card.h   |  3 +++
>>>>> drivers/mmc/core/quirks.h | 17 +++++++++++++++++
>>>>> 2 files changed, 20 insertions(+)
>>>>> 
>>>>> +static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
>>>>> +                                                const char *const *compat_list)
>> 
>> After a second thought, I am not sure we really need a list of
>> compatibles here. The quirks we may want to apply should be specific
>> per device and most likely not shared among a family of devices, don't
>> you think?
> 
> Indeed. I dislike to have to declare a list of compatible device (see 
> wl1251_compatible_list in patch 5) outside of the fixup list.
> 
> If I have several devices, I prefer to copy-paste a few lines in the 
> mmc_fixup list (for the WFX driver, I have 4 devices to declare).

Agreed. It makes the macro easier to use.

BR and thanks,
Nikolaus

Patch

diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
index 7bd392d55cfa5..a3204c19861a4 100644
--- a/drivers/mmc/core/card.h
+++ b/drivers/mmc/core/card.h
@@ -59,6 +59,9 @@  struct mmc_fixup {
 	/* for MMC cards */
 	unsigned int ext_csd_rev;
 
+	/* Match against functions declared in device tree */
+	const char **of_compatible;
+
 	void (*vendor_fixup)(struct mmc_card *card, int data);
 	int data;
 };
diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
index c7ef2d14b359f..ee591fd8aeca2 100644
--- a/drivers/mmc/core/quirks.h
+++ b/drivers/mmc/core/quirks.h
@@ -10,6 +10,7 @@ 
  *
  */
 
+#include <linux/of.h>
 #include <linux/mmc/sdio_ids.h>
 
 #include "card.h"
@@ -145,6 +146,19 @@  static const struct mmc_fixup __maybe_unused sdio_fixup_methods[] = {
 	END_FIXUP
 };
 
+static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
+						 const char *const *compat_list)
+{
+	struct device_node *np;
+
+	for_each_child_of_node(mmc_dev(card->host)->of_node, np) {
+		if (of_device_compatible_match(np, compat_list))
+			return true;
+	}
+
+	return false;
+}
+
 static inline void mmc_fixup_device(struct mmc_card *card,
 				    const struct mmc_fixup *table)
 {
@@ -173,6 +187,9 @@  static inline void mmc_fixup_device(struct mmc_card *card,
 			continue;
 		if (rev < f->rev_start || rev > f->rev_end)
 			continue;
+		if (f->of_compatible &&
+		    !mmc_fixup_of_compatible_match(card, f->of_compatible))
+			continue;
 
 		dev_dbg(&card->dev, "calling %ps\n", f->vendor_fixup);
 		f->vendor_fixup(card, f->data);