[v2,3/3] pinctrl: da9062: add driver support
diff mbox series

Message ID 20191127115619.20278-4-m.felsch@pengutronix.de
State New
Headers show
Series
  • Add DA9062 GPIO support
Related show

Commit Message

Marco Felsch Nov. 27, 2019, 11:56 a.m. UTC
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

Comments

Linus Walleij Nov. 27, 2019, 1:49 p.m. UTC | #1
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
Marco Felsch Nov. 27, 2019, 3:01 p.m. UTC | #2
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
>
Bartosz Golaszewski Nov. 28, 2019, 10:47 a.m. UTC | #3
ś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 |
Marco Felsch Nov. 29, 2019, 9:07 a.m. UTC | #4
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 |
>
Bartosz Golaszewski Nov. 29, 2019, 1:30 p.m. UTC | #5
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 |

Patch
diff mbox series

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");