linux-kernel - Re: [PATCH v5 3/3] mmc: atmel-mci: Move card detect gpio polarity quirk to gpiolib

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <63156971-6f3d-4023-90d9-98adc000a32e@microchip.com>
Date:   Thu, 24 Aug 2023 06:39:06 +0000
From:   <Balamanikandan.Gunasundar@...rochip.com>
To:     <linus.walleij@...aro.org>
CC:     <dmitry.torokhov@...il.com>, <ulf.hansson@...aro.org>,
        <linux-kernel@...r.kernel.org>, <linux-mmc@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <Ludovic.Desroches@...rochip.com>, <Nicolas.Ferre@...rochip.com>,
        <alexandre.belloni@...tlin.com>, <Hari.PrasathGE@...rochip.com>,
        <Balamanikandan.Gunasundar@...rochip.com>
Subject: Re: [PATCH v5 3/3] mmc: atmel-mci: Move card detect gpio polarity
 quirk to gpiolib

Hi Linus,

Thanks for reviewing the patch.

On 23/08/23 6:11 pm, Linus Walleij wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Balamanikandan,
> 
> thanks for your patch!
> 
> On Wed, Aug 23, 2023 at 12:40 PM Balamanikandan Gunasundar
> <balamanikandan.gunasundar@...rochip.com> wrote:
> 
>> The polarity of the card detection gpio is handled by the "cd-inverted"
>> property in the device tree. Move this inversion logic to gpiolib to avoid
>> reading the gpio raw value.
>>
>> Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@...rochip.com>
> (...)
>>   drivers/gpio/gpiolib-of.c    |  7 +++++++
> 
> Patching here is the right approach!
> 
>> +#if IS_ENABLED(CONFIG_MMC_ATMELMCI)
>> +       if (of_device_is_compatible(np->parent, "atmel,hsmci") &&

The only difference above is "np->parent" while the existing code uses 
"np". This is because the compatible string is defined in the parent 
node here while the others have it in the same node.

mmc0: mmc@...00000 {
	compatible = "atmel,hsmci";
	...
	
	slot@0 {
		cd-gpios = <&pioE 0 GPIO_ACTIVE_LOW>;
		cd-inverted;
	}
}

>> +           !strcmp(propname, "cd-gpios")) {
>> +               active_high = of_property_read_bool(np, "cd-inverted");
>> +               of_gpio_quirk_polarity(np, active_high, flags);
>> +       }
>> +#endif
>>          for (i = 0; i < ARRAY_SIZE(gpios); i++) {
>>                  if (of_device_is_compatible(np, gpios[i].compatible) &&
>>                      !strcmp(propname, gpios[i].gpio_propname)) {
> 
> This duplicates the code right below. Can't you just add an entry to the
> existing gpios[] array?

Yes! I attempted to do so. But as explained above in this particular 
case, of_device_is_compatible(np->parent, ..) should be called. Adding a 
condition check here looked clumsy.

> 
> I would imagine:
> 
>      static const struct {
>          const char *compatible;
>          const char *gpio_propname;
>          const char *polarity_propname;
>      } gpios[] = {
> #if IS_ENABLED(CONFIG_MMC_ATMELMCI)
>          { "atmel,hsmci", "cd-gpios", "cd-inverted" },
> #endif
> (...)
> 

I think with the above entry I can also add a check like ...

if (np->parent) {
	if (of_device_is_compatible(np->parent, "atmel,hsmci") &&
		!strcmp(propname, gpios[i].gpio_propname))
		active_high = of_property_read_bool();
}

Please let me know your thoughts.

Thanks,
Balamanikandan

> 
> Yours,
> Linus Walleij