| Message ID | 20191127115619.20278-4-m.felsch@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | Add DA9062 GPIO support | expand |
Hi Marco, thanks for your patch! On Wed, Nov 27, 2019 at 12:56 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > be used as input, output or have a special use-case. > > The patch adds the support for the normal input/output use-case. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> (...) > +config PINCTRL_DA9062 > + tristate "Dialog Semiconductor DA9062 PMIC pinctrl and GPIO Support" > + depends on MFD_DA9062 > + select GPIOLIB Hm this would be one of those that could use GENERIC_REGMAP_GPIO the day we invent it but we haven't invented it yet. > +#include <../gpio/gpiolib.h> Put a comment above this telling us why you do this thing. > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > +{ (...) > + return val & BIT(offset); You should #include <linux/bits.h> since you use BIT() Usually I clamp it like this: return !!(val & BIT(offset)); > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > +{ > + struct da9062_pctl *pctl = gpiochip_get_data(gc); > + struct regmap *regmap = pctl->da9062->regmap; > + int gpio_mode; > + > + gpio_mode = da9062_pctl_get_pin_mode(regmap, offset); > + if (gpio_mode < 0) > + return gpio_mode; > + > + switch (gpio_mode) { > + case DA9062_PIN_ALTERNATE: > + return -ENOTSUPP; > + case DA9062_PIN_GPI: > + return 1; > + case DA9062_PIN_GPO_OD: > + case DA9062_PIN_GPO_PP: > + return 0; We recently added defines for these directions in <linux/gpio/driver.h>: #define GPIO_LINE_DIRECTION_IN 1 #define GPIO_LINE_DIRECTION_OUT 0 Please use these. (Soon in Torvald's tree, else in my "devel" branch.) > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > + unsigned int offset) > +{ > + struct da9062_pctl *pctl = gpiochip_get_data(gc); > + struct regmap *regmap = pctl->da9062->regmap; > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > + unsigned int gpi_type; > + int ret; > + > + ret = da9062_pctl_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > + if (ret) > + return ret; > + > + /* > + * If the gpio is active low we should set it in hw too. No worries > + * about gpio_get() because we read and return the gpio-level. So the > + * gpiolib active_low handling is still correct. > + * > + * 0 - active low, 1 - active high > + */ > + gpi_type = !gpiod_is_active_low(desc); That's interesting. Correct too, I guess. > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > + unsigned int offset, int value) > +{ > + /* Push-Pull / Open-Drain options are configured during set_config */ > + da9062_gpio_set(gc, offset, value); That looks dangerous. There is no guarantee that .set_config() always gets called. Please create a local state container for the mode of each pin in struct da9062_pctl and set it to push-pull by default at probe, then set this to whatever the state is here and let the .set_config() alter it later if need be. If we don't do that you will get boot-time defaults I think and that might create bugs. > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > + unsigned long config) > +{ (...) > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > + return da9062_pctl_set_pin_mode(regmap, offset, > + DA9062_PIN_GPO_OD); > + case PIN_CONFIG_DRIVE_PUSH_PULL: > + return da9062_pctl_set_pin_mode(regmap, offset, > + DA9062_PIN_GPO_PP); So also store this in the per-pin state. Yours, Linus Walleij
Hi Linus, On 19-11-27 14:49, Linus Walleij wrote: > Hi Marco, > > thanks for your patch! thanks for your fast response. > On Wed, Nov 27, 2019 at 12:56 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > > > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > > be used as input, output or have a special use-case. > > > > The patch adds the support for the normal input/output use-case. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > (...) > > > +config PINCTRL_DA9062 > > + tristate "Dialog Semiconductor DA9062 PMIC pinctrl and GPIO Support" > > + depends on MFD_DA9062 > > + select GPIOLIB > > Hm this would be one of those that could use GENERIC_REGMAP_GPIO > the day we invent it but we haven't invented it yet. Yes it is. Is there a plan for GENERIC_REGMAP_GPIO? > > +#include <../gpio/gpiolib.h> > > Put a comment above this telling us why you do this thing. Okay. > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > > +{ > (...) > > + return val & BIT(offset); > > You should #include <linux/bits.h> since you use BIT() Argh.. of course I will add the include. > Usually I clamp it like this: > return !!(val & BIT(offset)); Okay, I can change that too. > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > > +{ > > + struct da9062_pctl *pctl = gpiochip_get_data(gc); > > + struct regmap *regmap = pctl->da9062->regmap; > > + int gpio_mode; > > + > > + gpio_mode = da9062_pctl_get_pin_mode(regmap, offset); > > + if (gpio_mode < 0) > > + return gpio_mode; > > + > > + switch (gpio_mode) { > > + case DA9062_PIN_ALTERNATE: > > + return -ENOTSUPP; > > + case DA9062_PIN_GPI: > > + return 1; > > + case DA9062_PIN_GPO_OD: > > + case DA9062_PIN_GPO_PP: > > + return 0; > > We recently added defines for these directions in > <linux/gpio/driver.h>: > > #define GPIO_LINE_DIRECTION_IN 1 > #define GPIO_LINE_DIRECTION_OUT 0 > > Please use these. (Soon in Torvald's tree, else > in my "devel" branch.) I rebased it onto your devel, thanks for the hint. > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > + unsigned int offset) > > +{ > > + struct da9062_pctl *pctl = gpiochip_get_data(gc); > > + struct regmap *regmap = pctl->da9062->regmap; > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > + unsigned int gpi_type; > > + int ret; > > + > > + ret = da9062_pctl_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > + if (ret) > > + return ret; > > + > > + /* > > + * If the gpio is active low we should set it in hw too. No worries > > + * about gpio_get() because we read and return the gpio-level. So the > > + * gpiolib active_low handling is still correct. > > + * > > + * 0 - active low, 1 - active high > > + */ > > + gpi_type = !gpiod_is_active_low(desc); > > That's interesting. Correct too, I guess. Double checked it again and the datasheet calls it gpio-level so I assume that this is correct. > > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > > + unsigned int offset, int value) > > +{ > > + /* Push-Pull / Open-Drain options are configured during set_config */ > > + da9062_gpio_set(gc, offset, value); > > That looks dangerous. There is no guarantee that .set_config() > always gets called. Hm.. it seems that other drivers using this assumption too: - gpio-lp87565.c - gpio-tps65218.c - ... But you're right I missed the possible users of gpiod_direction_output_raw(). > Please create a local state container for the mode of each pin in > struct da9062_pctl and set it to push-pull by default at probe, then > set this to whatever the state is here and let the .set_config() > alter it later if need be. > > If we don't do that you will get boot-time defaults I think and that > might create bugs. I will add a container for each pin, thanks for covering that. > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > > + unsigned long config) > > +{ > (...) > > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > + return da9062_pctl_set_pin_mode(regmap, offset, > > + DA9062_PIN_GPO_OD); > > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > + return da9062_pctl_set_pin_mode(regmap, offset, > > + DA9062_PIN_GPO_PP); > > So also store this in the per-pin state. Of course. Regards, Marco > > Yours, > Linus Walleij >
śr., 27 lis 2019 o 16:01 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > Hi Linus, > > On 19-11-27 14:49, Linus Walleij wrote: > > Hi Marco, > > > > thanks for your patch! > > thanks for your fast response. > > > On Wed, Nov 27, 2019 at 12:56 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > > > > > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > > > be used as input, output or have a special use-case. > > > > > > The patch adds the support for the normal input/output use-case. > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > (...) > > > > > +config PINCTRL_DA9062 > > > + tristate "Dialog Semiconductor DA9062 PMIC pinctrl and GPIO Support" > > > + depends on MFD_DA9062 > > > + select GPIOLIB > > > > Hm this would be one of those that could use GENERIC_REGMAP_GPIO > > the day we invent it but we haven't invented it yet. > > Yes it is. Is there a plan for GENERIC_REGMAP_GPIO? > Yes, it's the second item on my TODO after the LINEINFO_FD series. I just got a board I can use for developing this so I should have something shortly. Bart > > > +#include <../gpio/gpiolib.h> > > > > Put a comment above this telling us why you do this thing. > > Okay. > > > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > > > +{ > > (...) > > > + return val & BIT(offset); > > > > You should #include <linux/bits.h> since you use BIT() > > Argh.. of course I will add the include. > > > Usually I clamp it like this: > > return !!(val & BIT(offset)); > > Okay, I can change that too. > > > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > > > +{ > > > + struct da9062_pctl *pctl = gpiochip_get_data(gc); > > > + struct regmap *regmap = pctl->da9062->regmap; > > > + int gpio_mode; > > > + > > > + gpio_mode = da9062_pctl_get_pin_mode(regmap, offset); > > > + if (gpio_mode < 0) > > > + return gpio_mode; > > > + > > > + switch (gpio_mode) { > > > + case DA9062_PIN_ALTERNATE: > > > + return -ENOTSUPP; > > > + case DA9062_PIN_GPI: > > > + return 1; > > > + case DA9062_PIN_GPO_OD: > > > + case DA9062_PIN_GPO_PP: > > > + return 0; > > > > We recently added defines for these directions in > > <linux/gpio/driver.h>: > > > > #define GPIO_LINE_DIRECTION_IN 1 > > #define GPIO_LINE_DIRECTION_OUT 0 > > > > Please use these. (Soon in Torvald's tree, else > > in my "devel" branch.) > > I rebased it onto your devel, thanks for the hint. > > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > > + unsigned int offset) > > > +{ > > > + struct da9062_pctl *pctl = gpiochip_get_data(gc); > > > + struct regmap *regmap = pctl->da9062->regmap; > > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > > + unsigned int gpi_type; > > > + int ret; > > > + > > > + ret = da9062_pctl_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > > + if (ret) > > > + return ret; > > > + > > > + /* > > > + * If the gpio is active low we should set it in hw too. No worries > > > + * about gpio_get() because we read and return the gpio-level. So the > > > + * gpiolib active_low handling is still correct. > > > + * > > > + * 0 - active low, 1 - active high > > > + */ > > > + gpi_type = !gpiod_is_active_low(desc); > > > > That's interesting. Correct too, I guess. > > Double checked it again and the datasheet calls it gpio-level so I > assume that this is correct. > > > > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > > > + unsigned int offset, int value) > > > +{ > > > + /* Push-Pull / Open-Drain options are configured during set_config */ > > > + da9062_gpio_set(gc, offset, value); > > > > That looks dangerous. There is no guarantee that .set_config() > > always gets called. > > Hm.. it seems that other drivers using this assumption too: > - gpio-lp87565.c > - gpio-tps65218.c > - ... > > But you're right I missed the possible users of > gpiod_direction_output_raw(). > > > Please create a local state container for the mode of each pin in > > struct da9062_pctl and set it to push-pull by default at probe, then > > set this to whatever the state is here and let the .set_config() > > alter it later if need be. > > > > If we don't do that you will get boot-time defaults I think and that > > might create bugs. > > I will add a container for each pin, thanks for covering that. > > > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > > > + unsigned long config) > > > +{ > > (...) > > > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > > + return da9062_pctl_set_pin_mode(regmap, offset, > > > + DA9062_PIN_GPO_OD); > > > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > > + return da9062_pctl_set_pin_mode(regmap, offset, > > > + DA9062_PIN_GPO_PP); > > > > So also store this in the per-pin state. > > Of course. > > Regards, > Marco > > > > > Yours, > > Linus Walleij > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 19-11-28 11:47, Bartosz Golaszewski wrote: > śr., 27 lis 2019 o 16:01 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > Hi Linus, > > > > On 19-11-27 14:49, Linus Walleij wrote: > > > Hi Marco, > > > > > > thanks for your patch! > > > > thanks for your fast response. > > > > > On Wed, Nov 27, 2019 at 12:56 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > > > > > > > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > > > > be used as input, output or have a special use-case. > > > > > > > > The patch adds the support for the normal input/output use-case. > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > (...) > > > > > > > +config PINCTRL_DA9062 > > > > + tristate "Dialog Semiconductor DA9062 PMIC pinctrl and GPIO Support" > > > > + depends on MFD_DA9062 > > > > + select GPIOLIB > > > > > > Hm this would be one of those that could use GENERIC_REGMAP_GPIO > > > the day we invent it but we haven't invented it yet. > > > > Yes it is. Is there a plan for GENERIC_REGMAP_GPIO? > > > > Yes, it's the second item on my TODO after the LINEINFO_FD series. I > just got a board I can use for developing this so I should have > something shortly. So it is okay to keep the above select and change it later? Regards, Marco > Bart > > > > > +#include <../gpio/gpiolib.h> > > > > > > Put a comment above this telling us why you do this thing. > > > > Okay. > > > > > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > > > > +{ > > > (...) > > > > + return val & BIT(offset); > > > > > > You should #include <linux/bits.h> since you use BIT() > > > > Argh.. of course I will add the include. > > > > > Usually I clamp it like this: > > > return !!(val & BIT(offset)); > > > > Okay, I can change that too. > > > > > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > > > > +{ > > > > + struct da9062_pctl *pctl = gpiochip_get_data(gc); > > > > + struct regmap *regmap = pctl->da9062->regmap; > > > > + int gpio_mode; > > > > + > > > > + gpio_mode = da9062_pctl_get_pin_mode(regmap, offset); > > > > + if (gpio_mode < 0) > > > > + return gpio_mode; > > > > + > > > > + switch (gpio_mode) { > > > > + case DA9062_PIN_ALTERNATE: > > > > + return -ENOTSUPP; > > > > + case DA9062_PIN_GPI: > > > > + return 1; > > > > + case DA9062_PIN_GPO_OD: > > > > + case DA9062_PIN_GPO_PP: > > > > + return 0; > > > > > > We recently added defines for these directions in > > > <linux/gpio/driver.h>: > > > > > > #define GPIO_LINE_DIRECTION_IN 1 > > > #define GPIO_LINE_DIRECTION_OUT 0 > > > > > > Please use these. (Soon in Torvald's tree, else > > > in my "devel" branch.) > > > > I rebased it onto your devel, thanks for the hint. > > > > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > > > + unsigned int offset) > > > > +{ > > > > + struct da9062_pctl *pctl = gpiochip_get_data(gc); > > > > + struct regmap *regmap = pctl->da9062->regmap; > > > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > > > + unsigned int gpi_type; > > > > + int ret; > > > > + > > > > + ret = da9062_pctl_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* > > > > + * If the gpio is active low we should set it in hw too. No worries > > > > + * about gpio_get() because we read and return the gpio-level. So the > > > > + * gpiolib active_low handling is still correct. > > > > + * > > > > + * 0 - active low, 1 - active high > > > > + */ > > > > + gpi_type = !gpiod_is_active_low(desc); > > > > > > That's interesting. Correct too, I guess. > > > > Double checked it again and the datasheet calls it gpio-level so I > > assume that this is correct. > > > > > > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > > > > + unsigned int offset, int value) > > > > +{ > > > > + /* Push-Pull / Open-Drain options are configured during set_config */ > > > > + da9062_gpio_set(gc, offset, value); > > > > > > That looks dangerous. There is no guarantee that .set_config() > > > always gets called. > > > > Hm.. it seems that other drivers using this assumption too: > > - gpio-lp87565.c > > - gpio-tps65218.c > > - ... > > > > But you're right I missed the possible users of > > gpiod_direction_output_raw(). > > > > > Please create a local state container for the mode of each pin in > > > struct da9062_pctl and set it to push-pull by default at probe, then > > > set this to whatever the state is here and let the .set_config() > > > alter it later if need be. > > > > > > If we don't do that you will get boot-time defaults I think and that > > > might create bugs. > > > > I will add a container for each pin, thanks for covering that. > > > > > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > > > > + unsigned long config) > > > > +{ > > > (...) > > > > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > > > + return da9062_pctl_set_pin_mode(regmap, offset, > > > > + DA9062_PIN_GPO_OD); > > > > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > > > + return da9062_pctl_set_pin_mode(regmap, offset, > > > > + DA9062_PIN_GPO_PP); > > > > > > So also store this in the per-pin state. > > > > Of course. > > > > Regards, > > Marco > > > > > > > > Yours, > > > Linus Walleij > > > > > > > -- > > Pengutronix e.K. | | > > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >
pt., 29 lis 2019 o 10:07 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > On 19-11-28 11:47, Bartosz Golaszewski wrote: > > śr., 27 lis 2019 o 16:01 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > > > Hi Linus, > > > > > > On 19-11-27 14:49, Linus Walleij wrote: > > > > Hi Marco, > > > > > > > > thanks for your patch! > > > > > > thanks for your fast response. > > > > > > > On Wed, Nov 27, 2019 at 12:56 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > > > > > > > > > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > > > > > be used as input, output or have a special use-case. > > > > > > > > > > The patch adds the support for the normal input/output use-case. > > > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > (...) > > > > > > > > > +config PINCTRL_DA9062 > > > > > + tristate "Dialog Semiconductor DA9062 PMIC pinctrl and GPIO Support" > > > > > + depends on MFD_DA9062 > > > > > + select GPIOLIB > > > > > > > > Hm this would be one of those that could use GENERIC_REGMAP_GPIO > > > > the day we invent it but we haven't invented it yet. > > > > > > Yes it is. Is there a plan for GENERIC_REGMAP_GPIO? > > > > > > > Yes, it's the second item on my TODO after the LINEINFO_FD series. I > > just got a board I can use for developing this so I should have > > something shortly. > > So it is okay to keep the above select and change it later? > Yes, we don't want to stop you from getting this upstream. We'll convert it once the new module is ready. Bart > Regards, > Marco > > > Bart > > > > > > > +#include <../gpio/gpiolib.h> > > > > > > > > Put a comment above this telling us why you do this thing. > > > > > > Okay. > > > > > > > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > > > > > +{ > > > > (...) > > > > > + return val & BIT(offset); > > > > > > > > You should #include <linux/bits.h> since you use BIT() > > > > > > Argh.. of course I will add the include. > > > > > > > Usually I clamp it like this: > > > > return !!(val & BIT(offset)); > > > > > > Okay, I can change that too. > > > > > > > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > > > > > +{ > > > > > + struct da9062_pctl *pctl = gpiochip_get_data(gc); > > > > > + struct regmap *regmap = pctl->da9062->regmap; > > > > > + int gpio_mode; > > > > > + > > > > > + gpio_mode = da9062_pctl_get_pin_mode(regmap, offset); > > > > > + if (gpio_mode < 0) > > > > > + return gpio_mode; > > > > > + > > > > > + switch (gpio_mode) { > > > > > + case DA9062_PIN_ALTERNATE: > > > > > + return -ENOTSUPP; > > > > > + case DA9062_PIN_GPI: > > > > > + return 1; > > > > > + case DA9062_PIN_GPO_OD: > > > > > + case DA9062_PIN_GPO_PP: > > > > > + return 0; > > > > > > > > We recently added defines for these directions in > > > > <linux/gpio/driver.h>: > > > > > > > > #define GPIO_LINE_DIRECTION_IN 1 > > > > #define GPIO_LINE_DIRECTION_OUT 0 > > > > > > > > Please use these. (Soon in Torvald's tree, else > > > > in my "devel" branch.) > > > > > > I rebased it onto your devel, thanks for the hint. > > > > > > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > > > > + unsigned int offset) > > > > > +{ > > > > > + struct da9062_pctl *pctl = gpiochip_get_data(gc); > > > > > + struct regmap *regmap = pctl->da9062->regmap; > > > > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > > > > + unsigned int gpi_type; > > > > > + int ret; > > > > > + > > > > > + ret = da9062_pctl_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + /* > > > > > + * If the gpio is active low we should set it in hw too. No worries > > > > > + * about gpio_get() because we read and return the gpio-level. So the > > > > > + * gpiolib active_low handling is still correct. > > > > > + * > > > > > + * 0 - active low, 1 - active high > > > > > + */ > > > > > + gpi_type = !gpiod_is_active_low(desc); > > > > > > > > That's interesting. Correct too, I guess. > > > > > > Double checked it again and the datasheet calls it gpio-level so I > > > assume that this is correct. > > > > > > > > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > > > > > + unsigned int offset, int value) > > > > > +{ > > > > > + /* Push-Pull / Open-Drain options are configured during set_config */ > > > > > + da9062_gpio_set(gc, offset, value); > > > > > > > > That looks dangerous. There is no guarantee that .set_config() > > > > always gets called. > > > > > > Hm.. it seems that other drivers using this assumption too: > > > - gpio-lp87565.c > > > - gpio-tps65218.c > > > - ... > > > > > > But you're right I missed the possible users of > > > gpiod_direction_output_raw(). > > > > > > > Please create a local state container for the mode of each pin in > > > > struct da9062_pctl and set it to push-pull by default at probe, then > > > > set this to whatever the state is here and let the .set_config() > > > > alter it later if need be. > > > > > > > > If we don't do that you will get boot-time defaults I think and that > > > > might create bugs. > > > > > > I will add a container for each pin, thanks for covering that. > > > > > > > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > > > > > + unsigned long config) > > > > > +{ > > > > (...) > > > > > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > > > > + return da9062_pctl_set_pin_mode(regmap, offset, > > > > > + DA9062_PIN_GPO_OD); > > > > > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > > > > + return da9062_pctl_set_pin_mode(regmap, offset, > > > > > + DA9062_PIN_GPO_PP); > > > > > > > > So also store this in the per-pin state. > > > > > > Of course. > > > > > > Regards, > > > Marco > > > > > > > > > > > Yours, > > > > Linus Walleij > > > > > > > > > > -- > > > Pengutronix e.K. | | > > > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > > > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index b372419d61f2..977787c158cc 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -126,6 +126,18 @@ config PINCTRL_DA850_PUPD Driver for TI DA850/OMAP-L138/AM18XX pinconf. Used to control pullup/pulldown pin groups. +config PINCTRL_DA9062 + tristate "Dialog Semiconductor DA9062 PMIC pinctrl and GPIO Support" + depends on MFD_DA9062 + select GPIOLIB + help + The Dialog DA9062 PMIC provides multiple GPIOs that can be muxed for + different functions. This driver bundles a pinctrl driver to select the + function muxing and a GPIO driver to handle the GPIO when the GPIO + function is selected. + + Say yes to enable pinctrl and GPIO support for the DA9062 PMIC. + config PINCTRL_DIGICOLOR bool depends on OF && (ARCH_DIGICOLOR || COMPILE_TEST) diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index ac537fdbc998..2397684cbe11 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_PINCTRL_AT91PIO4) += pinctrl-at91-pio4.o obj-$(CONFIG_PINCTRL_AMD) += pinctrl-amd.o obj-$(CONFIG_PINCTRL_BM1880) += pinctrl-bm1880.o obj-$(CONFIG_PINCTRL_DA850_PUPD) += pinctrl-da850-pupd.o +obj-$(CONFIG_PINCTRL_DA9062) += pinctrl-da9062.o obj-$(CONFIG_PINCTRL_DIGICOLOR) += pinctrl-digicolor.o obj-$(CONFIG_PINCTRL_FALCON) += pinctrl-falcon.o obj-$(CONFIG_PINCTRL_GEMINI) += pinctrl-gemini.o diff --git a/drivers/pinctrl/pinctrl-da9062.c b/drivers/pinctrl/pinctrl-da9062.c new file mode 100644 index 000000000000..35ea8b488162 --- /dev/null +++ b/drivers/pinctrl/pinctrl-da9062.c @@ -0,0 +1,272 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Dialog DA9062 pinctrl and GPIO driver. + * Based on DA9055 GPIO driver. + * + * TODO: + * - add pinmux and pinctrl support (gpio alternate mode) + * + * Documents: + * [1] https://www.dialog-semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf + * + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> + */ +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#include <linux/gpio/driver.h> + +#include <linux/mfd/da9062/core.h> +#include <linux/mfd/da9062/registers.h> + +#include <../gpio/gpiolib.h> + +#define DA9062_TYPE(offset) (4 * (offset % 2)) +#define DA9062_PIN_SHIFT(offset) (4 * (offset % 2)) +#define DA9062_PIN_ALTERNATE 0x00 /* gpio alternate mode */ +#define DA9062_PIN_GPI 0x01 /* gpio in */ +#define DA9062_PIN_GPO_OD 0x02 /* gpio out open-drain */ +#define DA9062_PIN_GPO_PP 0x03 /* gpio out push-pull */ +#define DA9062_GPIO_NUM 5 + +struct da9062_pctl { + struct da9062 *da9062; + struct gpio_chip gc; +}; + +static int da9062_pctl_get_pin_mode(struct regmap *regmap, unsigned int offset) +{ + int ret, val; + + ret = regmap_read(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), &val); + if (ret < 0) + return ret; + + val >>= DA9062_PIN_SHIFT(offset); + val &= DA9062AA_GPIO0_PIN_MASK; + + return val; +} + +static int da9062_pctl_set_pin_mode(struct regmap *regmap, unsigned int offset, + unsigned int mode) +{ + unsigned int mask; + + mode &= DA9062AA_GPIO0_PIN_MASK; + mode <<= DA9062_PIN_SHIFT(offset); + mask = DA9062AA_GPIO0_PIN_MASK << DA9062_PIN_SHIFT(offset); + + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), + mask, mode); +} + +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) +{ + struct da9062_pctl *pctl = gpiochip_get_data(gc); + struct regmap *regmap = pctl->da9062->regmap; + int gpio_mode, val; + int ret; + + gpio_mode = da9062_pctl_get_pin_mode(regmap, offset); + if (gpio_mode < 0) + return gpio_mode; + + switch (gpio_mode) { + case DA9062_PIN_ALTERNATE: + return -ENOTSUPP; + case DA9062_PIN_GPI: + ret = regmap_read(regmap, DA9062AA_STATUS_B, &val); + if (ret < 0) + return ret; + break; + case DA9062_PIN_GPO_OD: + case DA9062_PIN_GPO_PP: + ret = regmap_read(regmap, DA9062AA_GPIO_MODE0_4, &val); + if (ret < 0) + return ret; + } + + return val & BIT(offset); +} + +static void da9062_gpio_set(struct gpio_chip *gc, unsigned int offset, + int value) +{ + struct da9062_pctl *pctl = gpiochip_get_data(gc); + struct regmap *regmap = pctl->da9062->regmap; + + regmap_update_bits(regmap, DA9062AA_GPIO_MODE0_4, BIT(offset), + value << offset); +} + +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) +{ + struct da9062_pctl *pctl = gpiochip_get_data(gc); + struct regmap *regmap = pctl->da9062->regmap; + int gpio_mode; + + gpio_mode = da9062_pctl_get_pin_mode(regmap, offset); + if (gpio_mode < 0) + return gpio_mode; + + switch (gpio_mode) { + case DA9062_PIN_ALTERNATE: + return -ENOTSUPP; + case DA9062_PIN_GPI: + return 1; + case DA9062_PIN_GPO_OD: + case DA9062_PIN_GPO_PP: + return 0; + } + + return -EINVAL; +} + +static int da9062_gpio_direction_input(struct gpio_chip *gc, + unsigned int offset) +{ + struct da9062_pctl *pctl = gpiochip_get_data(gc); + struct regmap *regmap = pctl->da9062->regmap; + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); + unsigned int gpi_type; + int ret; + + ret = da9062_pctl_set_pin_mode(regmap, offset, DA9062_PIN_GPI); + if (ret) + return ret; + + /* + * If the gpio is active low we should set it in hw too. No worries + * about gpio_get() because we read and return the gpio-level. So the + * gpiolib active_low handling is still correct. + * + * 0 - active low, 1 - active high + */ + gpi_type = !gpiod_is_active_low(desc); + + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), + gpi_type << DA9062_TYPE(offset)); +} + +static int da9062_gpio_direction_output(struct gpio_chip *gc, + unsigned int offset, int value) +{ + /* Push-Pull / Open-Drain options are configured during set_config */ + da9062_gpio_set(gc, offset, value); + + return 0; +} + +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, + unsigned long config) +{ + struct da9062_pctl *pctl = gpiochip_get_data(gc); + struct regmap *regmap = pctl->da9062->regmap; + int gpio_mode; + + /* + * We need to meet the following restrictions [1, Figure 18]: + * - PIN_CONFIG_BIAS_PULL_DOWN -> only allowed of the pin is used as + * gpio input + * - PIN_CONFIG_BIAS_PULL_UP -> only allowed of the pin is used as + * gpio output open-drain. + */ + + switch (pinconf_to_config_param(config)) { + case PIN_CONFIG_BIAS_PULL_DOWN: + gpio_mode = da9062_pctl_get_pin_mode(regmap, offset); + if (gpio_mode < 0) + return -EINVAL; + else if (gpio_mode != DA9062_PIN_GPI) + return -ENOTSUPP; + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, + BIT(offset), BIT(offset)); + case PIN_CONFIG_BIAS_PULL_UP: + gpio_mode = da9062_pctl_get_pin_mode(regmap, offset); + if (gpio_mode < 0) + return -EINVAL; + else if (gpio_mode != DA9062_PIN_GPO_OD) + return -ENOTSUPP; + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, + BIT(offset), BIT(offset)); + case PIN_CONFIG_DRIVE_OPEN_DRAIN: + return da9062_pctl_set_pin_mode(regmap, offset, + DA9062_PIN_GPO_OD); + case PIN_CONFIG_DRIVE_PUSH_PULL: + return da9062_pctl_set_pin_mode(regmap, offset, + DA9062_PIN_GPO_PP); + default: + return -ENOTSUPP; + } +} + +static int da9062_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) +{ + struct da9062_pctl *pctl = gpiochip_get_data(gc); + struct da9062 *da9062 = pctl->da9062; + + return regmap_irq_get_virq(da9062->regmap_irq, + DA9062_IRQ_GPI0 + offset); +} + +static const struct gpio_chip reference_gc = { + .owner = THIS_MODULE, + .get = da9062_gpio_get, + .set = da9062_gpio_set, + .get_direction = da9062_gpio_get_direction, + .direction_input = da9062_gpio_direction_input, + .direction_output = da9062_gpio_direction_output, + .set_config = da9062_gpio_set_config, + .to_irq = da9062_gpio_to_irq, + .can_sleep = true, + .ngpio = DA9062_GPIO_NUM, + .base = -1, +}; + +static int da9062_pctl_probe(struct platform_device *pdev) +{ + struct device *parent = pdev->dev.parent; + struct da9062_pctl *pctl; + + pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL); + if (!pctl) + return -ENOMEM; + + pctl->da9062 = dev_get_drvdata(parent); + if (!pctl->da9062) + return -EINVAL; + + if (!device_property_present(parent, "gpio-controller")) + return 0; + + /* + * Currently the driver handles only the GPIO support. The + * pinctrl/pinmux support can be added later if needed. + */ + pctl->gc = reference_gc; + pctl->gc.label = dev_name(&pdev->dev); + pctl->gc.parent = &pdev->dev; +#ifdef CONFIG_OF_GPIO + pctl->gc.of_node = parent->of_node; +#endif + + platform_set_drvdata(pdev, pctl); + + return devm_gpiochip_add_data(&pdev->dev, &pctl->gc, pctl); +} + +static struct platform_driver da9062_pctl_driver = { + .probe = da9062_pctl_probe, + .driver = { + .name = "da9062-gpio", + }, +}; +module_platform_driver(da9062_pctl_driver); + +MODULE_AUTHOR("Marco Felsch <kernel@pengutronix.de>"); +MODULE_DESCRIPTION("DA9062 PMIC pinctrl and GPIO Driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:da9062-gpio");
The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can be used as input, output or have a special use-case. The patch adds the support for the normal input/output use-case. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- Changelog: v2: - fix minor style issue - move from drivers/gpio to drivers/pinctrl - Fix spelling issue - rename local gpio_dir to gpio_mode - Add datasheet reference and TODO notes - move gpio to mfd-root node to avoid hierarchical interrupt chips - Add gpio-controller property check - remove of_device_id since we drop the gpio of-subnode - Drop da9062_gpio_get_hwgpio --- drivers/pinctrl/Kconfig | 12 ++ drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-da9062.c | 272 +++++++++++++++++++++++++++++++ 3 files changed, 285 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-da9062.c