Re: [PATCH 2/3] gpio: arizona: Add support for GPIOs that need to be maintained

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Tue, May 16, 2017 at 5:31 PM, Charles Keepax
<ckeepax@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:

> The Arizona devices only maintain the state of output GPIOs whilst the
> CODEC is active, this can cause issues if the CODEC suspends whilst
> something is relying on the state of one of its GPIOs. However, in
> many systems the CODEC GPIOs are used for audio related features
> and thus the state of the GPIOs is unimportant whilst the CODEC is
> suspended. Often keeping the CODEC resumed in such a system would
> incur a power impact that is unacceptable.
>
> Allow the user to select whether a GPIO output should keep the
> CODEC resumed, by adding a flag through the second cell of the GPIO
> specifier in device tree.
>
> Signed-off-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxxxxxxxx>

This adds the handling locally in the driver for a certain Arizona chip.

And then the next time you have to duplicate the code again and again.

That's not working, put this in the core.

>  static int arizona_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
>  {
>         struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
>         struct arizona *arizona = arizona_gpio->arizona;
> +       int status = arizona_gpio->status[offset];

Don't use a local array for this.

This should be something like gpiochip_get_sleep_persistance(chip, offset)

> +       if (change && !(status & GPIO_SLEEP_MAY_LOOSE_VALUE)) {

That us a dt-binfings header define, that is not a Linux constant.

Add FLAG_MAY_LOOSE_VALUE_DURING_SLEEP or something in
drivers/gpio/gpiolib.h, store this inside the struct gpio_desc like all
other flags and retrieve it from there.

Augment the code in gpiolib.c and gpiolib-of.c to parse this and
store it properly in the gpio_desc and use it as described above.

Then it is reusable for others.

> +#ifdef CONFIG_OF_GPIO
> +static int arizona_gpio_of_xlate(struct gpio_chip *chip,
> +                                const struct of_phandle_args *gpiospec,
> +                                u32 *flags)
> +{
> +       struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
> +       int offset;
> +
> +       offset = of_gpio_simple_xlate(chip, gpiospec, flags);
> +       if (offset < 0)
> +               return offset;
> +
> +       if (flags)
> +               arizona_gpio->status[offset] = *flags;
> +
> +       return offset;
> +}
> +#endif

No need for this hackery if you move the parsing to the core, so do that.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html