| Message ID | 1494948714-15203-2-git-send-email-ckeepax@opensource.wolfsonmicro.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 16, 2017 at 5:31 PM, Charles Keepax <ckeepax@opensource.wolfsonmicro.com> 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@opensource.wolfsonmicro.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 23, 2017 at 10:46:29AM +0200, Linus Walleij wrote: > On Tue, May 16, 2017 at 5:31 PM, Charles Keepax > <ckeepax@opensource.wolfsonmicro.com> 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@opensource.wolfsonmicro.com> > > 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. > Apologies hadn't realised you wanted me to put so much of the functionality into the core, will respin again. Thanks, Charles -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpio-arizona.c b/drivers/gpio/gpio-arizona.c index cd23fd7..2d86776 100644 --- a/drivers/gpio/gpio-arizona.c +++ b/drivers/gpio/gpio-arizona.c @@ -12,10 +12,12 @@ * */ +#include <dt-bindings/gpio/gpio.h> #include <linux/kernel.h> #include <linux/slab.h> #include <linux/module.h> #include <linux/gpio.h> +#include <linux/of_gpio.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/seq_file.h> @@ -27,15 +29,30 @@ struct arizona_gpio { struct arizona *arizona; struct gpio_chip gpio_chip; + int status[ARIZONA_MAX_GPIO]; }; 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]; + bool change; + int ret; - return regmap_update_bits(arizona->regmap, ARIZONA_GPIO1_CTRL + offset, - ARIZONA_GPN_DIR, ARIZONA_GPN_DIR); + ret = regmap_update_bits_check(arizona->regmap, + ARIZONA_GPIO1_CTRL + offset, + ARIZONA_GPN_DIR, ARIZONA_GPN_DIR, + &change); + if (ret < 0) + return ret; + + if (change && !(status & GPIO_SLEEP_MAY_LOOSE_VALUE)) { + pm_runtime_mark_last_busy(chip->parent); + pm_runtime_put_autosuspend(chip->parent); + } + + return 0; } static int arizona_gpio_get(struct gpio_chip *chip, unsigned offset) @@ -85,6 +102,21 @@ static int arizona_gpio_direction_out(struct gpio_chip *chip, { struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip); struct arizona *arizona = arizona_gpio->arizona; + int status = arizona_gpio->status[offset]; + unsigned int val; + int ret; + + ret = regmap_read(arizona->regmap, ARIZONA_GPIO1_CTRL + offset, &val); + if (ret < 0) + return ret; + + if ((val & ARIZONA_GPN_DIR) && !(status & GPIO_SLEEP_MAY_LOOSE_VALUE)) { + ret = pm_runtime_get_sync(chip->parent); + if (ret < 0) { + dev_err(chip->parent, "Failed to resume: %d\n", ret); + return ret; + } + } if (value) value = ARIZONA_GPN_LVL; @@ -105,6 +137,25 @@ static void arizona_gpio_set(struct gpio_chip *chip, unsigned offset, int value) ARIZONA_GPN_LVL, value); } +#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 + static const struct gpio_chip template_chip = { .label = "arizona", .owner = THIS_MODULE, @@ -132,6 +183,8 @@ static int arizona_gpio_probe(struct platform_device *pdev) arizona_gpio->gpio_chip.parent = &pdev->dev; #ifdef CONFIG_OF_GPIO arizona_gpio->gpio_chip.of_node = arizona->dev->of_node; + arizona_gpio->gpio_chip.of_xlate = arizona_gpio_of_xlate; + arizona_gpio->gpio_chip.of_gpio_n_cells = 2; #endif switch (arizona->type) { @@ -158,6 +211,8 @@ static int arizona_gpio_probe(struct platform_device *pdev) else arizona_gpio->gpio_chip.base = -1; + pm_runtime_enable(&pdev->dev); + ret = devm_gpiochip_add_data(&pdev->dev, &arizona_gpio->gpio_chip, arizona_gpio); if (ret < 0) {
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@opensource.wolfsonmicro.com> --- drivers/gpio/gpio-arizona.c | 59 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 2 deletions(-)