Message ID | 20191127115619.20278-1-m.felsch@pengutronix.de |
---|---|
Headers | show |
Series | Add DA9062 GPIO support | expand |
On Wed, Nov 27, 2019 at 12:56 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > Currently the da9062 GPIO's aren't available. The patch adds the support > to make these available by adding a gpio device with the corresponding > irq resources. Furthermore the patch fixes a minor style issue for the > onkey device. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> This is a regmap irqchip so I guess not much to say about it. Acked-by: Linus Walleij <linus.walleij@linaro.org> HOWEVER: this looks very much hierarchical does it not? I can clearly see that regmap's irqchip does not support hierarchical interrupt domains, so we should just make a mental note somewhere that "oh yeah and then one day we should make regmap irqchips play well with hierarchical interrupts". Yours, Linus Walleij
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, thanks for your feedback. On 19-11-27 14:35, Linus Walleij wrote: > On Wed, Nov 27, 2019 at 12:56 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > > > Currently the da9062 GPIO's aren't available. The patch adds the support > > to make these available by adding a gpio device with the corresponding > > irq resources. Furthermore the patch fixes a minor style issue for the > > onkey device. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > This is a regmap irqchip so I guess not much to say about it. > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > HOWEVER: this looks very much hierarchical does it not? Yes it that's right and I converted it upon Bartosz comments. > I can clearly see that regmap's irqchip does not support > hierarchical interrupt domains, so we should just make a > mental note somewhere that "oh yeah and then one day > we should make regmap irqchips play well with hierarchical > interrupts". That's right, should I add this somewhere and if the answer is yes then where? Regards, Marco > > 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 >
On Wed, Nov 27, 2019 at 02:35:33PM +0100, Linus Walleij wrote: > I can clearly see that regmap's irqchip does not support > hierarchical interrupt domains, so we should just make a > mental note somewhere that "oh yeah and then one day > we should make regmap irqchips play well with hierarchical > interrupts". Why, what do we need to do? We're just doing all the default stuff, it's not something we've opted out of, and the whole point with using the frameworks should be that we should get software features like this for free :(
ś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 Wed, Nov 27, 2019 at 4:19 PM Mark Brown <broonie@kernel.org> wrote: > On Wed, Nov 27, 2019 at 02:35:33PM +0100, Linus Walleij wrote: > > > I can clearly see that regmap's irqchip does not support > > hierarchical interrupt domains, so we should just make a > > mental note somewhere that "oh yeah and then one day > > we should make regmap irqchips play well with hierarchical > > interrupts". > > Why, what do we need to do? We're just doing all the default stuff, > it's not something we've opted out of, and the whole point with using > the frameworks should be that we should get software features like this > for free :( I guess it is a bit about moving targets. The regmap irq thing was covering all reasonable cases until the hierarchical interrupts were introduced some years ago. The hallmark of these are that the irq_domain_ops implement .translate() .alloc() and .free() rather than .map() and .xlate() as the irqdomain in reqmap-irq currently does. The problem with hierarchical domains is that the system using them need to be hierarchical "all the way up" to the overarching top-level irqchip. Currently only the ARM GIC and the IXP4xx irq top-level irq controllers supports this, ruling out some obvious users like all non-ARM systems (for now). I think the assumption in hierarchical irq is that you have a few hardware-specific irchips and you know exactly which irqchip that goes on top of another one, as well as which hardware irq line is connected to which hardware irq line on the parent. Since we know the specific hardware (from a compatible string or so) we can hardcode the parent-to-child mappings of interrupt lines in the driver. These mappings are not in the device tree for example. Therefore supporting hierarchical and nonhierarchical alike in a very generic plug-in irqchip like the regmap-irq is a bit of a challenge as it has to support both hierarchical and non-hierarchical, because it is not possible to just convert this to hierarchical callbacks: it has to check what its parent is doing and adapt, essentially implement both hierarchical and non-hierarchical at this time. Also we need to pass mappings between parent and child somehow. In the gpiolib we did this with a callback to the GPIO-chip-specific driver. How to do it for something generic like regmap-irq is an open question. So it is a bit complex. (Marc may correct me here.) Yours, Linus Walleij
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 |
On Fri, Nov 29, 2019 at 09:49:56AM +0100, Linus Walleij wrote: > On Wed, Nov 27, 2019 at 4:19 PM Mark Brown <broonie@kernel.org> wrote: > > Why, what do we need to do? We're just doing all the default stuff, > > it's not something we've opted out of, and the whole point with using > > the frameworks should be that we should get software features like this > > for free :( > I guess it is a bit about moving targets. > The regmap irq thing was covering all reasonable cases until > the hierarchical interrupts were introduced some years ago. > The hallmark of these are that the irq_domain_ops implement > .translate() .alloc() and .free() rather than .map() and .xlate() > as the irqdomain in reqmap-irq currently does. So there's two completely different APIs. Are all the drivers supposed to be being updated to implement both? It looks like the second API is ifdefed out when not in use so I guess so but... > The problem with hierarchical domains is that the system using > them need to be hierarchical "all the way up" to the overarching > top-level irqchip. Currently only the ARM GIC and the IXP4xx > irq top-level irq controllers supports this, ruling out some > obvious users like all non-ARM systems (for now). Are you sure? It looks like the API was introduced by Intel and io_apic appears to be using the new interfaces. > I think the assumption in hierarchical irq is that you have > a few hardware-specific irchips and you know exactly which > irqchip that goes on top of another one, as well as which > hardware irq line is connected to which hardware irq line > on the parent. The documentation says that this is for systems where "there may be multiple interrupt controllers involved in delivering an interrupt from the device to the target CPU" which describes more or less every single regmap-irq user, though the threading might mean it doesn't quite map onto what was being thought of there. But basically everything I can find suggests that regmap-irq should be a hierarchical controller apart from how we figure out the primary IRQ. > Since we know the specific hardware (from a compatible > string or so) we can hardcode the parent-to-child mappings > of interrupt lines in the driver. These mappings are not > in the device tree for example. That seems to be part of it, yes, which seems unfortunate. > Therefore supporting hierarchical and nonhierarchical alike > in a very generic plug-in irqchip like the regmap-irq is a bit > of a challenge as it has to support both hierarchical and > non-hierarchical, because it is not possible to just > convert this to hierarchical callbacks: it has to check what > its parent is doing and adapt, essentially implement both > hierarchical and non-hierarchical at this time. It looks that way, yes. > Also we need to pass mappings between parent and child > somehow. In the gpiolib we did this with a callback to the > GPIO-chip-specific driver. How to do it for something > generic like regmap-irq is an open question. Currently regmap-irq just gets a parent interrupt from whatever is using it so yeah, this doesn't map on at all well especially since the user is likely to be using just a normal interrupt binding for this which is apparently explicitly not allowed[1]. I'm not sure what to do here. [1] https://elinux.org/images/8/8c/Zyngier.pdf