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

[<Balamanikandan.Gunasundar@microchip.com>]

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@microchip.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@microchip.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@f0000000 {
	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