[RFC,10/13] gpio: bd71828: Initial support for ROHM BD71828 PMIC GPIOs
diff mbox series

Message ID f8f8c323d378244afe4e94f48c0a94bb296cbbe0.1571302099.git.matti.vaittinen@fi.rohmeurope.com
State New
Headers show
Series
  • Support ROHM BD71828 PMIC
Related show

Commit Message

Vaittinen, Matti Oct. 17, 2019, 9:53 a.m. UTC
ROHM BD71828 PMIC contains 4 pins which can be configured by OTP
to be used for general purposes. First 3 can be used as outputs
and 4.th pin can be used as input. Allow them to be controlled
via GPIO framework.

The driver assumes all of the pins are configured as GPIOs and
rusts that the reserved pins in other OTP configurations are
excluded from control using "gpio-reserved-ranges" device tree
property (or left untouched by GPIO users).

Typical use for 4.th pin (input) is to use it as HALL sensor
input so that this pin state is toggled when HALL sensor detects
LID position change (from close to open or open to close). PMIC
HW implements some extra logic which allows PMIC to power-up the
system when this pin is toggled. Please see the data sheet for
details of GPIO options which can be selcted by OTP settings.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/gpio/Kconfig        |  12 +++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-bd71828.c | 161 ++++++++++++++++++++++++++++++++++++
 3 files changed, 174 insertions(+)
 create mode 100644 drivers/gpio/gpio-bd71828.c

Comments

Bartosz Golaszewski Oct. 17, 2019, 12:45 p.m. UTC | #1
czw., 17 paź 2019 o 11:53 Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> napisał(a):
>
> ROHM BD71828 PMIC contains 4 pins which can be configured by OTP
> to be used for general purposes. First 3 can be used as outputs
> and 4.th pin can be used as input. Allow them to be controlled
> via GPIO framework.
>
> The driver assumes all of the pins are configured as GPIOs and
> rusts that the reserved pins in other OTP configurations are
> excluded from control using "gpio-reserved-ranges" device tree
> property (or left untouched by GPIO users).
>
> Typical use for 4.th pin (input) is to use it as HALL sensor
> input so that this pin state is toggled when HALL sensor detects
> LID position change (from close to open or open to close). PMIC
> HW implements some extra logic which allows PMIC to power-up the
> system when this pin is toggled. Please see the data sheet for
> details of GPIO options which can be selcted by OTP settings.
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>  drivers/gpio/Kconfig        |  12 +++
>  drivers/gpio/Makefile       |   1 +
>  drivers/gpio/gpio-bd71828.c | 161 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 174 insertions(+)
>  create mode 100644 drivers/gpio/gpio-bd71828.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index bb13c266c329..fb0a099de961 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -986,6 +986,18 @@ config GPIO_BD70528
>           This driver can also be built as a module. If so, the module
>           will be called gpio-bd70528.
>
> +config GPIO_BD71828
> +       tristate "ROHM BD71828 GPIO support"
> +       depends on MFD_ROHM_BD71828
> +       help
> +         Support for GPIOs on ROHM BD71828 PMIC. There are three GPIOs
> +         available on the ROHM PMIC in total. The GPIOs are limited to
> +         outputs only and pins must be configured to GPIO outputs by
> +         OTP. Enable this only if you want to use these pins as outputs.
> +
> +         This driver can also be built as a module. If so, the module
> +         will be called gpio-bd71828.
> +
>  config GPIO_BD9571MWV
>         tristate "ROHM BD9571 GPIO support"
>         depends on MFD_BD9571MWV
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index a4e91175c708..b11932844768 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_GPIO_ASPEED)             += gpio-aspeed.o
>  obj-$(CONFIG_GPIO_ATH79)               += gpio-ath79.o
>  obj-$(CONFIG_GPIO_BCM_KONA)            += gpio-bcm-kona.o
>  obj-$(CONFIG_GPIO_BD70528)             += gpio-bd70528.o
> +obj-$(CONFIG_GPIO_BD71828)             += gpio-bd71828.o
>  obj-$(CONFIG_GPIO_BD9571MWV)           += gpio-bd9571mwv.o
>  obj-$(CONFIG_GPIO_BRCMSTB)             += gpio-brcmstb.o
>  obj-$(CONFIG_GPIO_BT8XX)               += gpio-bt8xx.o
> diff --git a/drivers/gpio/gpio-bd71828.c b/drivers/gpio/gpio-bd71828.c
> new file mode 100644
> index 000000000000..3cf3890a24c4
> --- /dev/null
> +++ b/drivers/gpio/gpio-bd71828.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 ROHM Semiconductors
> +// gpio-bd71828.c ROHM BD71828 gpio driver

I don't think the name of the source file is needed here.

> +
> +#include <linux/gpio/driver.h>
> +#include <linux/mfd/rohm-bd71828.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define OUT 0
> +#define IN 1

If you really want to define those, please use a common prefix for all
symbols in the driver.

> +#define GPIO_OUT_REG(off) (BD71828_REG_GPIO_CTRL1 + (off))
> +#define HALL_GPIO_OFFSET 3
> +
> +struct bd71828_gpio {
> +       struct rohm_regmap_dev chip;
> +       struct gpio_chip gpio;
> +};
> +
> +static void bd71828_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +                            int value)
> +{
> +       int ret;
> +       struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
> +       u8 val = (value) ? BD71828_GPIO_OUT_HI : BD71828_GPIO_OUT_LO;
> +
> +       if (offset == HALL_GPIO_OFFSET)
> +               return;

Can you add a comment here saying that this pin can only be used as
input? Otherwise this information is only available in the commit
message.

> +
> +       ret = regmap_update_bits(bdgpio->chip.regmap, GPIO_OUT_REG(offset),
> +                                BD71828_GPIO_OUT_MASK, val);
> +       if (ret)
> +               dev_err(bdgpio->chip.dev, "Could not set gpio to %d\n", value);
> +}
> +
> +static int bd71828_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       int ret;
> +       unsigned int val;
> +       struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
> +
> +       if (offset == HALL_GPIO_OFFSET)
> +               ret = regmap_read(bdgpio->chip.regmap, BD71828_REG_IO_STAT,
> +                                 &val);
> +       else
> +               ret = regmap_read(bdgpio->chip.regmap, GPIO_OUT_REG(offset),
> +                                 &val);
> +       if (!ret)
> +               ret = (val & BD71828_GPIO_OUT_MASK);
> +
> +       return ret;
> +}
> +
> +static int bd71828_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
> +                                  unsigned long config)
> +{
> +       struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
> +
> +       if (offset == HALL_GPIO_OFFSET)
> +               return -ENOTSUPP;
> +
> +       switch (pinconf_to_config_param(config)) {
> +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +               return regmap_update_bits(bdgpio->chip.regmap,
> +                                         GPIO_OUT_REG(offset),
> +                                         BD71828_GPIO_DRIVE_MASK,
> +                                         BD71828_GPIO_OPEN_DRAIN);
> +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> +               return regmap_update_bits(bdgpio->chip.regmap,
> +                                         GPIO_OUT_REG(offset),
> +                                         BD71828_GPIO_DRIVE_MASK,
> +                                         BD71828_GPIO_PUSH_PULL);
> +       default:
> +               break;
> +       }
> +       return -ENOTSUPP;
> +}
> +
> +static int bd71828_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +       /*
> +        * Pin usage is selected by OTP data. We can't read it runtime. Hence
> +        * we trust that if the pin is not excluded by "gpio-reserved-ranges"
> +        * the OTP configuration is set to OUT. (Other pins but HALL input pin
> +        * on BD71828 can't really be used for general purpose input - input
> +        * states are used for specific cases like regulator control or
> +        * PMIC_ON_REQ.
> +        */
> +       if (offset == HALL_GPIO_OFFSET)
> +               return IN;
> +
> +       return OUT;
> +}
> +
> +static int bd71828_gpio_parse_dt(struct device *dev,
> +                                struct bd71828_gpio *bdgpio)
> +{
> +       /*
> +        * TBD: See if we need some implementation to mark some PINs as
> +        * not controllable based on DT info or if core can handle
> +        * "gpio-reserved-ranges" and exclude them from control
> +        */
> +       return 0;
> +}

Please don't implement empty functions. Just add this comment next to
gpiochip's initialization...

> +
> +static int bd71828_probe(struct platform_device *pdev)
> +{
> +       struct bd71828_gpio *bdgpio;
> +       struct rohm_regmap_dev *bd71828;
> +       int ret;
> +
> +       bd71828 = dev_get_drvdata(pdev->dev.parent);
> +       if (!bd71828) {
> +               dev_err(&pdev->dev, "No MFD driver data\n");
> +               return -EINVAL;
> +       }
> +
> +       bdgpio = devm_kzalloc(&pdev->dev, sizeof(*bdgpio),
> +                             GFP_KERNEL);
> +       if (!bdgpio)
> +               return -ENOMEM;
> +
> +       ret = bd71828_gpio_parse_dt(pdev->dev.parent, bdgpio);

... and remove this call.

> +
> +       bdgpio->chip.dev = &pdev->dev;
> +       bdgpio->gpio.parent = pdev->dev.parent;
> +       bdgpio->gpio.label = "bd71828-gpio";
> +       bdgpio->gpio.owner = THIS_MODULE;
> +       bdgpio->gpio.get_direction = bd71828_get_direction;
> +       bdgpio->gpio.set_config = bd71828_gpio_set_config;
> +       bdgpio->gpio.can_sleep = true;
> +       bdgpio->gpio.get = bd71828_gpio_get;
> +       bdgpio->gpio.set = bd71828_gpio_set;

Not implementing direction_output() and direction_input() here will
results in warnings from the GPIO framework: for instance you
implement set() but not direction_output(). I'd say: just add those
callbacks and return an error if they're called for invalid lines (for
instance: direction_output() being called for line 3).

> +       bdgpio->gpio.base = -1;
> +       bdgpio->gpio.ngpio = 4;
> +#ifdef CONFIG_OF_GPIO

This is not needed - for CONFIG_OF_GPIO disabled the parent of_node
will be NULL.

> +       bdgpio->gpio.of_node = pdev->dev.parent->of_node;
> +#endif
> +       bdgpio->chip.regmap = bd71828->regmap;
> +
> +       ret = devm_gpiochip_add_data(&pdev->dev, &bdgpio->gpio,
> +                                    bdgpio);
> +       if (ret)
> +               dev_err(&pdev->dev, "gpio_init: Failed to add bd71828-gpio\n");

Since there aren't many places where this function can fail, you can
directly return devm_gpiochip_add_data() here.

> +
> +       return ret;
> +}
> +
> +static struct platform_driver bd71828_gpio = {
> +       .driver = {
> +               .name = "bd71828-gpio"
> +       },
> +       .probe = bd71828_probe,
> +};
> +
> +module_platform_driver(bd71828_gpio);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD71828 voltage regulator driver");
> +MODULE_LICENSE("GPL");

Don't you need a MODULE_ALIAS() here since this is an MFD sub-module?

Bart

> --
> 2.21.0
>
>
> --
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
>
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> Simon says - in Latin please.
> ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> Thanks to Simon Glass for the translation =]
Vaittinen, Matti Oct. 21, 2019, 7 a.m. UTC | #2
Hello Bartosz,

Thanks for reading this through! I'll rework this patch during this
week :)

On Thu, 2019-10-17 at 14:45 +0200, Bartosz Golaszewski wrote:
> czw., 17 paź 2019 o 11:53 Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> napisał(a):
> > ROHM BD71828 PMIC contains 4 pins which can be configured by OTP
> > to be used for general purposes. First 3 can be used as outputs
> > and 4.th pin can be used as input. Allow them to be controlled
> > via GPIO framework.
> > 
> > The driver assumes all of the pins are configured as GPIOs and
> > rusts that the reserved pins in other OTP configurations are
> > excluded from control using "gpio-reserved-ranges" device tree
> > property (or left untouched by GPIO users).
> > 
> > Typical use for 4.th pin (input) is to use it as HALL sensor
> > input so that this pin state is toggled when HALL sensor detects
> > LID position change (from close to open or open to close). PMIC
> > HW implements some extra logic which allows PMIC to power-up the
> > system when this pin is toggled. Please see the data sheet for
> > details of GPIO options which can be selcted by OTP settings.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > 
> > +++ b/drivers/gpio/gpio-bd71828.c
> > @@ -0,0 +1,161 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2018 ROHM Semiconductors
> > +// gpio-bd71828.c ROHM BD71828 gpio driver
> 
> I don't think the name of the source file is needed here.

I Agree.

> 
> > +
> > +#include <linux/gpio/driver.h>
> > +#include <linux/mfd/rohm-bd71828.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +#define OUT 0
> > +#define IN 1
> 
> If you really want to define those, please use a common prefix for
> all
> symbols in the driver.

I prefer defining them because I always need to check the meaning of
these values. My brains just refuse from remembering which value is
used for in and which for out. I will add the prefix even though the
scope of these defines is limited to this file :)

> 
> > +#define GPIO_OUT_REG(off) (BD71828_REG_GPIO_CTRL1 + (off))
> > +#define HALL_GPIO_OFFSET 3
> > +
> > +struct bd71828_gpio {
> > +       struct rohm_regmap_dev chip;
> > +       struct gpio_chip gpio;
> > +};
> > +
> > +static void bd71828_gpio_set(struct gpio_chip *chip, unsigned int
> > offset,
> > +                            int value)
> > +{
> > +       int ret;
> > +       struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
> > +       u8 val = (value) ? BD71828_GPIO_OUT_HI :
> > BD71828_GPIO_OUT_LO;
> > +
> > +       if (offset == HALL_GPIO_OFFSET)
> > +               return;
> 
> Can you add a comment here saying that this pin can only be used as
> input? Otherwise this information is only available in the commit
> message.

Sure thing. I thought the comment in get_direction was sufficient but
you are correct - it's nice to see this limitation also here.

> > +
> > +       ret = regmap_update_bits(bdgpio->chip.regmap,
> > GPIO_OUT_REG(offset),
> > +                                BD71828_GPIO_OUT_MASK, val);
> > +       if (ret)
> > +               dev_err(bdgpio->chip.dev, "Could not set gpio to
> > %d\n", value);
> > +}
> > +
> > +static int bd71828_gpio_get(struct gpio_chip *chip, unsigned int
> > offset)
> > +{
> > +       int ret;
> > +       unsigned int val;
> > +       struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
> > +
> > +       if (offset == HALL_GPIO_OFFSET)
> > +               ret = regmap_read(bdgpio->chip.regmap,
> > BD71828_REG_IO_STAT,
> > +                                 &val);
> > +       else
> > +               ret = regmap_read(bdgpio->chip.regmap,
> > GPIO_OUT_REG(offset),
> > +                                 &val);
> > +       if (!ret)
> > +               ret = (val & BD71828_GPIO_OUT_MASK);
> > +
> > +       return ret;
> > +}
> > +
> > +static int bd71828_gpio_set_config(struct gpio_chip *chip,
> > unsigned int offset,
> > +                                  unsigned long config)
> > +{
> > +       struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
> > +
> > +       if (offset == HALL_GPIO_OFFSET)
> > +               return -ENOTSUPP;
> > +
> > +       switch (pinconf_to_config_param(config)) {
> > +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > +               return regmap_update_bits(bdgpio->chip.regmap,
> > +                                         GPIO_OUT_REG(offset),
> > +                                         BD71828_GPIO_DRIVE_MASK,
> > +                                         BD71828_GPIO_OPEN_DRAIN);
> > +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> > +               return regmap_update_bits(bdgpio->chip.regmap,
> > +                                         GPIO_OUT_REG(offset),
> > +                                         BD71828_GPIO_DRIVE_MASK,
> > +                                         BD71828_GPIO_PUSH_PULL);
> > +       default:
> > +               break;
> > +       }
> > +       return -ENOTSUPP;
> +}+static int bd71828_get_direction(struct gpio_chip *chip, unsigned
> int offset)
> > +{
> > +       /*
> > +        * Pin usage is selected by OTP data. We can't read it
> > runtime. Hence
> > +        * we trust that if the pin is not excluded by "gpio-
> > reserved-ranges"
> > +        * the OTP configuration is set to OUT. (Other pins but
> > HALL input pin
> > +        * on BD71828 can't really be used for general purpose
> > input - input
> > +        * states are used for specific cases like regulator
> > control or
> > +        * PMIC_ON_REQ.
> > +        */
> > +       if (offset == HALL_GPIO_OFFSET)
> > +               return IN;
> > +
> > +       return OUT;
> > +}
> > +
> > +static int bd71828_gpio_parse_dt(struct device *dev,
> > +                                struct bd71828_gpio *bdgpio)
> > +{
> > +       /*
> > +        * TBD: See if we need some implementation to mark some
> > PINs as
> > +        * not controllable based on DT info or if core can handle
> > +        * "gpio-reserved-ranges" and exclude them from control
> > +        */
> > +       return 0;
> > +}
> 
> Please don't implement empty functions. Just add this comment next to
> gpiochip's initialization...

Yep. I should have cleaned this before sending even this. Thanks for
pointing it out!

> 
> 
> > +
> > +       bdgpio->chip.dev = &pdev->dev;
> > +       bdgpio->gpio.parent = pdev->dev.parent;
> > +       bdgpio->gpio.label = "bd71828-gpio";
> > +       bdgpio->gpio.owner = THIS_MODULE;
> > +       bdgpio->gpio.get_direction = bd71828_get_direction;
> > +       bdgpio->gpio.set_config = bd71828_gpio_set_config;
> > +       bdgpio->gpio.can_sleep = true;
> > +       bdgpio->gpio.get = bd71828_gpio_get;
> > +       bdgpio->gpio.set = bd71828_gpio_set;
> 
> Not implementing direction_output() and direction_input() here will
> results in warnings from the GPIO framework: for instance you
> implement set() but not direction_output(). I'd say: just add those
> callbacks and return an error if they're called for invalid lines
> (for
> instance: direction_output() being called for line 3).

Ok. I will implement dummy functions.

But out of the curiosity - why the GPIO core emits the warnings if
these are not implemented? I think the core should not require "no-
operation" functions to be implemented for pins which don't support
both of the directions. GPIO core could only emit warning if it needs
to set direction to something the HW does not support. That would avoid
adding the dummy functions to all of the drivers, right?

> 
> > +       bdgpio->gpio.base = -1;
> > +       bdgpio->gpio.ngpio = 4;
> > +#ifdef CONFIG_OF_GPIO
> 
> This is not needed - for CONFIG_OF_GPIO disabled the parent of_node
> will be NULL.

Right. Thanks.

> > +       bdgpio->gpio.of_node = pdev->dev.parent->of_node;
> > +#endif
> > +       bdgpio->chip.regmap = bd71828->regmap;
> > +
> > +       ret = devm_gpiochip_add_data(&pdev->dev, &bdgpio->gpio,
> > +                                    bdgpio);
> > +       if (ret)
> > +               dev_err(&pdev->dev, "gpio_init: Failed to add
> > bd71828-gpio\n");
> 
> Since there aren't many places where this function can fail, you can
> directly return devm_gpiochip_add_data() here.

Ok.

> > +
> > +       return ret;
> > +}
> > +
> > +static struct platform_driver bd71828_gpio = {
> > +       .driver = {
> > +               .name = "bd71828-gpio"
> > +       },
> > +       .probe = bd71828_probe,
> > +};
> > +
> > +module_platform_driver(bd71828_gpio);
> > +
> > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ");
> > +MODULE_DESCRIPTION("BD71828 voltage regulator driver");
> > +MODULE_LICENSE("GPL");
> 
> Don't you need a MODULE_ALIAS() here since this is an MFD sub-module?

I must admit I don't know the details of how module loading is done. I
used system where modules are load by scripts. (I guess the module
alias could be used to allow automatic module loading [by udev?])

Can you please educate me - If I add module aliases matching the sub-
device name given in in MFD cell - should the sub module loading be
automatic when MFD driver gets probed? For some reason I didn't get
that working on my test bed. Or maybe I misunderstood something.

Eg, this should be enough for GPIO sub-module to be also load:

MFD:
static struct mfd_cell bd71828_mfd_cells[] = {
        { .name = "bd71828-pmic", },
        { .name = "bd71828-gpio", },
...
ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
                           bd71828_mfd_cells,
                           ARRAY_SIZE(bd71828_mfd_cells), NULL, 0,
                           regmap_irq_get_domain(irq_data)); 

GPIO driver:
MODULE_ALIAS("platform:bd71828-gpio");

I had the sub-devices probed even without the MODULE_ALIAS - but manual
loading is required. I will gladly add the alias if it should enable
the automatic module loading.

Br,
	Matti Vaittinen
Bartosz Golaszewski Oct. 21, 2019, 2:36 p.m. UTC | #3
pon., 21 paź 2019 o 09:00 Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> napisał(a):
>
> Hello Bartosz,
>
> Thanks for reading this through! I'll rework this patch during this
> week :)
>
> On Thu, 2019-10-17 at 14:45 +0200, Bartosz Golaszewski wrote:
> > czw., 17 paź 2019 o 11:53 Matti Vaittinen
> > <matti.vaittinen@fi.rohmeurope.com> napisał(a):
> > > ROHM BD71828 PMIC contains 4 pins which can be configured by OTP
> > > to be used for general purposes. First 3 can be used as outputs
> > > and 4.th pin can be used as input. Allow them to be controlled
> > > via GPIO framework.
> > >
> > > The driver assumes all of the pins are configured as GPIOs and
> > > rusts that the reserved pins in other OTP configurations are
> > > excluded from control using "gpio-reserved-ranges" device tree
> > > property (or left untouched by GPIO users).
> > >
> > > Typical use for 4.th pin (input) is to use it as HALL sensor
> > > input so that this pin state is toggled when HALL sensor detects
> > > LID position change (from close to open or open to close). PMIC
> > > HW implements some extra logic which allows PMIC to power-up the
> > > system when this pin is toggled. Please see the data sheet for
> > > details of GPIO options which can be selcted by OTP settings.
> > >
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > >
> > > +++ b/drivers/gpio/gpio-bd71828.c
> > > @@ -0,0 +1,161 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Copyright (C) 2018 ROHM Semiconductors
> > > +// gpio-bd71828.c ROHM BD71828 gpio driver
> >
> > I don't think the name of the source file is needed here.
>
> I Agree.
>
> >
> > > +
> > > +#include <linux/gpio/driver.h>
> > > +#include <linux/mfd/rohm-bd71828.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +#define OUT 0
> > > +#define IN 1
> >
> > If you really want to define those, please use a common prefix for
> > all
> > symbols in the driver.
>
> I prefer defining them because I always need to check the meaning of
> these values. My brains just refuse from remembering which value is
> used for in and which for out. I will add the prefix even though the
> scope of these defines is limited to this file :)
>
> >
> > > +#define GPIO_OUT_REG(off) (BD71828_REG_GPIO_CTRL1 + (off))
> > > +#define HALL_GPIO_OFFSET 3
> > > +
> > > +struct bd71828_gpio {
> > > +       struct rohm_regmap_dev chip;
> > > +       struct gpio_chip gpio;
> > > +};
> > > +
> > > +static void bd71828_gpio_set(struct gpio_chip *chip, unsigned int
> > > offset,
> > > +                            int value)
> > > +{
> > > +       int ret;
> > > +       struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
> > > +       u8 val = (value) ? BD71828_GPIO_OUT_HI :
> > > BD71828_GPIO_OUT_LO;
> > > +
> > > +       if (offset == HALL_GPIO_OFFSET)
> > > +               return;
> >
> > Can you add a comment here saying that this pin can only be used as
> > input? Otherwise this information is only available in the commit
> > message.
>
> Sure thing. I thought the comment in get_direction was sufficient but
> you are correct - it's nice to see this limitation also here.
>
> > > +
> > > +       ret = regmap_update_bits(bdgpio->chip.regmap,
> > > GPIO_OUT_REG(offset),
> > > +                                BD71828_GPIO_OUT_MASK, val);
> > > +       if (ret)
> > > +               dev_err(bdgpio->chip.dev, "Could not set gpio to
> > > %d\n", value);
> > > +}
> > > +
> > > +static int bd71828_gpio_get(struct gpio_chip *chip, unsigned int
> > > offset)
> > > +{
> > > +       int ret;
> > > +       unsigned int val;
> > > +       struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
> > > +
> > > +       if (offset == HALL_GPIO_OFFSET)
> > > +               ret = regmap_read(bdgpio->chip.regmap,
> > > BD71828_REG_IO_STAT,
> > > +                                 &val);
> > > +       else
> > > +               ret = regmap_read(bdgpio->chip.regmap,
> > > GPIO_OUT_REG(offset),
> > > +                                 &val);
> > > +       if (!ret)
> > > +               ret = (val & BD71828_GPIO_OUT_MASK);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static int bd71828_gpio_set_config(struct gpio_chip *chip,
> > > unsigned int offset,
> > > +                                  unsigned long config)
> > > +{
> > > +       struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
> > > +
> > > +       if (offset == HALL_GPIO_OFFSET)
> > > +               return -ENOTSUPP;
> > > +
> > > +       switch (pinconf_to_config_param(config)) {
> > > +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > > +               return regmap_update_bits(bdgpio->chip.regmap,
> > > +                                         GPIO_OUT_REG(offset),
> > > +                                         BD71828_GPIO_DRIVE_MASK,
> > > +                                         BD71828_GPIO_OPEN_DRAIN);
> > > +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> > > +               return regmap_update_bits(bdgpio->chip.regmap,
> > > +                                         GPIO_OUT_REG(offset),
> > > +                                         BD71828_GPIO_DRIVE_MASK,
> > > +                                         BD71828_GPIO_PUSH_PULL);
> > > +       default:
> > > +               break;
> > > +       }
> > > +       return -ENOTSUPP;
> > +}+static int bd71828_get_direction(struct gpio_chip *chip, unsigned
> > int offset)
> > > +{
> > > +       /*
> > > +        * Pin usage is selected by OTP data. We can't read it
> > > runtime. Hence
> > > +        * we trust that if the pin is not excluded by "gpio-
> > > reserved-ranges"
> > > +        * the OTP configuration is set to OUT. (Other pins but
> > > HALL input pin
> > > +        * on BD71828 can't really be used for general purpose
> > > input - input
> > > +        * states are used for specific cases like regulator
> > > control or
> > > +        * PMIC_ON_REQ.
> > > +        */
> > > +       if (offset == HALL_GPIO_OFFSET)
> > > +               return IN;
> > > +
> > > +       return OUT;
> > > +}
> > > +
> > > +static int bd71828_gpio_parse_dt(struct device *dev,
> > > +                                struct bd71828_gpio *bdgpio)
> > > +{
> > > +       /*
> > > +        * TBD: See if we need some implementation to mark some
> > > PINs as
> > > +        * not controllable based on DT info or if core can handle
> > > +        * "gpio-reserved-ranges" and exclude them from control
> > > +        */
> > > +       return 0;
> > > +}
> >
> > Please don't implement empty functions. Just add this comment next to
> > gpiochip's initialization...
>
> Yep. I should have cleaned this before sending even this. Thanks for
> pointing it out!
>
> >
> >
> > > +
> > > +       bdgpio->chip.dev = &pdev->dev;
> > > +       bdgpio->gpio.parent = pdev->dev.parent;
> > > +       bdgpio->gpio.label = "bd71828-gpio";
> > > +       bdgpio->gpio.owner = THIS_MODULE;
> > > +       bdgpio->gpio.get_direction = bd71828_get_direction;
> > > +       bdgpio->gpio.set_config = bd71828_gpio_set_config;
> > > +       bdgpio->gpio.can_sleep = true;
> > > +       bdgpio->gpio.get = bd71828_gpio_get;
> > > +       bdgpio->gpio.set = bd71828_gpio_set;
> >
> > Not implementing direction_output() and direction_input() here will
> > results in warnings from the GPIO framework: for instance you
> > implement set() but not direction_output(). I'd say: just add those
> > callbacks and return an error if they're called for invalid lines
> > (for
> > instance: direction_output() being called for line 3).
>
> Ok. I will implement dummy functions.
>
> But out of the curiosity - why the GPIO core emits the warnings if
> these are not implemented? I think the core should not require "no-
> operation" functions to be implemented for pins which don't support
> both of the directions. GPIO core could only emit warning if it needs
> to set direction to something the HW does not support. That would avoid
> adding the dummy functions to all of the drivers, right?
>

I looked at the code again and it seems I was wrong. If we don't have
direction_input() or direction_output() we check the actual direction
with get_direction() before emitting any warnings and if there's no
direction_output(), but line is in input mode then all's fine. In
other words: false alarm, and you can keep it this way.

> >
> > > +       bdgpio->gpio.base = -1;
> > > +       bdgpio->gpio.ngpio = 4;
> > > +#ifdef CONFIG_OF_GPIO
> >
> > This is not needed - for CONFIG_OF_GPIO disabled the parent of_node
> > will be NULL.
>
> Right. Thanks.
>
> > > +       bdgpio->gpio.of_node = pdev->dev.parent->of_node;
> > > +#endif
> > > +       bdgpio->chip.regmap = bd71828->regmap;
> > > +
> > > +       ret = devm_gpiochip_add_data(&pdev->dev, &bdgpio->gpio,
> > > +                                    bdgpio);
> > > +       if (ret)
> > > +               dev_err(&pdev->dev, "gpio_init: Failed to add
> > > bd71828-gpio\n");
> >
> > Since there aren't many places where this function can fail, you can
> > directly return devm_gpiochip_add_data() here.
>
> Ok.
>
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static struct platform_driver bd71828_gpio = {
> > > +       .driver = {
> > > +               .name = "bd71828-gpio"
> > > +       },
> > > +       .probe = bd71828_probe,
> > > +};
> > > +
> > > +module_platform_driver(bd71828_gpio);
> > > +
> > > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > ");
> > > +MODULE_DESCRIPTION("BD71828 voltage regulator driver");
> > > +MODULE_LICENSE("GPL");
> >
> > Don't you need a MODULE_ALIAS() here since this is an MFD sub-module?
>
> I must admit I don't know the details of how module loading is done. I
> used system where modules are load by scripts. (I guess the module
> alias could be used to allow automatic module loading [by udev?])
>
> Can you please educate me - If I add module aliases matching the sub-
> device name given in in MFD cell - should the sub module loading be
> automatic when MFD driver gets probed? For some reason I didn't get
> that working on my test bed. Or maybe I misunderstood something.
>

If the gpio module is a sub-node on the device tree than you may need
to use a sub-compatible to get the module loaded by udev.

Bart

> Eg, this should be enough for GPIO sub-module to be also load:
>
> MFD:
> static struct mfd_cell bd71828_mfd_cells[] = {
>         { .name = "bd71828-pmic", },
>         { .name = "bd71828-gpio", },
> ...
> ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
>                            bd71828_mfd_cells,
>                            ARRAY_SIZE(bd71828_mfd_cells), NULL, 0,
>                            regmap_irq_get_domain(irq_data));
>
> GPIO driver:
> MODULE_ALIAS("platform:bd71828-gpio");
>
> I had the sub-devices probed even without the MODULE_ALIAS - but manual
> loading is required. I will gladly add the alias if it should enable
> the automatic module loading.
>
> Br,
>         Matti Vaittinen
>
Vaittinen, Matti Oct. 21, 2019, 2:56 p.m. UTC | #4
Thanks again Bart =)


On Mon, 2019-10-21 at 16:36 +0200, Bartosz Golaszewski wrote:
> pon., 21 paź 2019 o 09:00 Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> napisał(a):
> > Hello Bartosz,
> > 
> > +
> > > > +       bdgpio->chip.dev = &pdev->dev;
> > > > +       bdgpio->gpio.parent = pdev->dev.parent;
> > > > +       bdgpio->gpio.label = "bd71828-gpio";
> > > > +       bdgpio->gpio.owner = THIS_MODULE;
> > > > +       bdgpio->gpio.get_direction = bd71828_get_direction;
> > > > +       bdgpio->gpio.set_config = bd71828_gpio_set_config;
> > > > +       bdgpio->gpio.can_sleep = true;
> > > > +       bdgpio->gpio.get = bd71828_gpio_get;
> > > > +       bdgpio->gpio.set = bd71828_gpio_set;
> > > 
> > > Not implementing direction_output() and direction_input() here
> > > will
> > > results in warnings from the GPIO framework: for instance you
> > > implement set() but not direction_output(). I'd say: just add
> > > those
> > > callbacks and return an error if they're called for invalid lines
> > > (for
> > > instance: direction_output() being called for line 3).
> > 
> > Ok. I will implement dummy functions.
> > 
> > But out of the curiosity - why the GPIO core emits the warnings if
> > these are not implemented? I think the core should not require "no-
> > operation" functions to be implemented for pins which don't support
> > both of the directions. GPIO core could only emit warning if it
> > needs
> > to set direction to something the HW does not support. That would
> > avoid
> > adding the dummy functions to all of the drivers, right?
> > 
> 
> I looked at the code again and it seems I was wrong. If we don't have
> direction_input() or direction_output() we check the actual direction
> with get_direction() before emitting any warnings and if there's no
> direction_output(), but line is in input mode then all's fine. In
> other words: false alarm, and you can keep it this way.

Thanks for clarifying this - it makes sense :) I wont change this then.

> > > Don't you need a MODULE_ALIAS() here since this is an MFD sub-
> > > module?
> > 
> > I must admit I don't know the details of how module loading is
> > done. I
> > used system where modules are load by scripts. (I guess the module
> > alias could be used to allow automatic module loading [by udev?])
> > 
> > Can you please educate me - If I add module aliases matching the
> > sub-
> > device name given in in MFD cell - should the sub module loading be
> > automatic when MFD driver gets probed? For some reason I didn't get
> > that working on my test bed. Or maybe I misunderstood something.
> > 
> 
> If the gpio module is a sub-node on the device tree than you may need
> to use a sub-compatible to get the module loaded by udev.

Just found out that the last update broke my test bed I2C completely.
I'll experiment with the MODULE_ALIAS when I get my board running...

I don't want to add own DT node for gpio (and all other sub devices).
That shouldn't be needed. It really should be enough to kick the MFD
part from DT using the compatible - subdevices should be load by MFD
without having own DT compatibles for them. But I'll see how this works
out - thanks!

Br,
	Matti Vaittinen
Vaittinen, Matti Oct. 22, 2019, 1:19 p.m. UTC | #5
Hello Bartosz,

Just a short note at this point.

On Mon, 2019-10-21 at 16:36 +0200, Bartosz Golaszewski wrote:
> pon., 21 paź 2019 o 09:00 Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> napisał(a):
> > On Thu, 2019-10-17 at 14:45 +0200, Bartosz Golaszewski wrote:
> > > czw., 17 paź 2019 o 11:53 Matti Vaittinen
> > > 
> > > > +static struct platform_driver bd71828_gpio = {
> > > > +       .driver = {
> > > > +               .name = "bd71828-gpio"
> > > > +       },
> > > > +       .probe = bd71828_probe,
> > > > +};
> > > > +
> > > > +module_platform_driver(bd71828_gpio);
> > > > +
> > > > +MODULE_AUTHOR("Matti Vaittinen <
> > > > matti.vaittinen@fi.rohmeurope.com>
> > > > ");
> > > > +MODULE_DESCRIPTION("BD71828 voltage regulator driver");
> > > > +MODULE_LICENSE("GPL");
> > > 
> > > Don't you need a MODULE_ALIAS() here since this is an MFD sub-
> > > module?
> > 
> > I must admit I don't know the details of how module loading is
> > done. I
> > used system where modules are load by scripts. (I guess the module
> > alias could be used to allow automatic module loading [by udev?])
> > 
> > Can you please educate me - If I add module aliases matching the
> > sub-
> > device name given in in MFD cell - should the sub module loading be
> > automatic when MFD driver gets probed? For some reason I didn't get
> > that working on my test bed. Or maybe I misunderstood something.
> > 
> 
> If the gpio module is a sub-node on the device tree than you may need
> to use a sub-compatible to get the module loaded by udev.
> 
> Bart
> 
> > Eg, this should be enough for GPIO sub-module to be also load:
> > 
> > MFD:
> > static struct mfd_cell bd71828_mfd_cells[] = {
> >         { .name = "bd71828-pmic", },
> >         { .name = "bd71828-gpio", },
> > ...
> > ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> >                            bd71828_mfd_cells,
> >                            ARRAY_SIZE(bd71828_mfd_cells), NULL, 0,
> >                            regmap_irq_get_domain(irq_data));
> > 
> > GPIO driver:
> > MODULE_ALIAS("platform:bd71828-gpio");
> > 
> > I had the sub-devices probed even without the MODULE_ALIAS - but
> > manual
> > loading is required. I will gladly add the alias if it should
> > enable
> > the automatic module loading.

I had some problems with my test setup. After I fixed those I was able
to test the automatic module loading. The sub-devices do not need own
compatible to be load - they only need the correct module_alias:
"platform:sub-device-name-used-in-mfd-cell". If this is added, the
module for sub-device is load when MFD cell with name matching the
alias is instantiated.

So to your original question - yes, MODULE_ALIAS() is needed for
automated loading. No compatible is required then :)

Thanks for pointing me to correct direction here! It's always nice to
learn something new!

Br,
	Matti Vaittinen

Patch
diff mbox series

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index bb13c266c329..fb0a099de961 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -986,6 +986,18 @@  config GPIO_BD70528
 	  This driver can also be built as a module. If so, the module
 	  will be called gpio-bd70528.
 
+config GPIO_BD71828
+	tristate "ROHM BD71828 GPIO support"
+	depends on MFD_ROHM_BD71828
+	help
+	  Support for GPIOs on ROHM BD71828 PMIC. There are three GPIOs
+	  available on the ROHM PMIC in total. The GPIOs are limited to
+	  outputs only and pins must be configured to GPIO outputs by
+	  OTP. Enable this only if you want to use these pins as outputs.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called gpio-bd71828.
+
 config GPIO_BD9571MWV
 	tristate "ROHM BD9571 GPIO support"
 	depends on MFD_BD9571MWV
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a4e91175c708..b11932844768 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -35,6 +35,7 @@  obj-$(CONFIG_GPIO_ASPEED)		+= gpio-aspeed.o
 obj-$(CONFIG_GPIO_ATH79)		+= gpio-ath79.o
 obj-$(CONFIG_GPIO_BCM_KONA)		+= gpio-bcm-kona.o
 obj-$(CONFIG_GPIO_BD70528)		+= gpio-bd70528.o
+obj-$(CONFIG_GPIO_BD71828)		+= gpio-bd71828.o
 obj-$(CONFIG_GPIO_BD9571MWV)		+= gpio-bd9571mwv.o
 obj-$(CONFIG_GPIO_BRCMSTB)		+= gpio-brcmstb.o
 obj-$(CONFIG_GPIO_BT8XX)		+= gpio-bt8xx.o
diff --git a/drivers/gpio/gpio-bd71828.c b/drivers/gpio/gpio-bd71828.c
new file mode 100644
index 000000000000..3cf3890a24c4
--- /dev/null
+++ b/drivers/gpio/gpio-bd71828.c
@@ -0,0 +1,161 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// gpio-bd71828.c ROHM BD71828 gpio driver
+
+#include <linux/gpio/driver.h>
+#include <linux/mfd/rohm-bd71828.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define OUT 0
+#define IN 1
+#define GPIO_OUT_REG(off) (BD71828_REG_GPIO_CTRL1 + (off))
+#define HALL_GPIO_OFFSET 3
+
+struct bd71828_gpio {
+	struct rohm_regmap_dev chip;
+	struct gpio_chip gpio;
+};
+
+static void bd71828_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			     int value)
+{
+	int ret;
+	struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
+	u8 val = (value) ? BD71828_GPIO_OUT_HI : BD71828_GPIO_OUT_LO;
+
+	if (offset == HALL_GPIO_OFFSET)
+		return;
+
+	ret = regmap_update_bits(bdgpio->chip.regmap, GPIO_OUT_REG(offset),
+				 BD71828_GPIO_OUT_MASK, val);
+	if (ret)
+		dev_err(bdgpio->chip.dev, "Could not set gpio to %d\n", value);
+}
+
+static int bd71828_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	int ret;
+	unsigned int val;
+	struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
+
+	if (offset == HALL_GPIO_OFFSET)
+		ret = regmap_read(bdgpio->chip.regmap, BD71828_REG_IO_STAT,
+				  &val);
+	else
+		ret = regmap_read(bdgpio->chip.regmap, GPIO_OUT_REG(offset),
+				  &val);
+	if (!ret)
+		ret = (val & BD71828_GPIO_OUT_MASK);
+
+	return ret;
+}
+
+static int bd71828_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
+				   unsigned long config)
+{
+	struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
+
+	if (offset == HALL_GPIO_OFFSET)
+		return -ENOTSUPP;
+
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		return regmap_update_bits(bdgpio->chip.regmap,
+					  GPIO_OUT_REG(offset),
+					  BD71828_GPIO_DRIVE_MASK,
+					  BD71828_GPIO_OPEN_DRAIN);
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		return regmap_update_bits(bdgpio->chip.regmap,
+					  GPIO_OUT_REG(offset),
+					  BD71828_GPIO_DRIVE_MASK,
+					  BD71828_GPIO_PUSH_PULL);
+	default:
+		break;
+	}
+	return -ENOTSUPP;
+}
+
+static int bd71828_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	/*
+	 * Pin usage is selected by OTP data. We can't read it runtime. Hence
+	 * we trust that if the pin is not excluded by "gpio-reserved-ranges"
+	 * the OTP configuration is set to OUT. (Other pins but HALL input pin
+	 * on BD71828 can't really be used for general purpose input - input
+	 * states are used for specific cases like regulator control or
+	 * PMIC_ON_REQ.
+	 */
+	if (offset == HALL_GPIO_OFFSET)
+		return IN;
+
+	return OUT;
+}
+
+static int bd71828_gpio_parse_dt(struct device *dev,
+				 struct bd71828_gpio *bdgpio)
+{
+	/*
+	 * TBD: See if we need some implementation to mark some PINs as
+	 * not controllable based on DT info or if core can handle
+	 * "gpio-reserved-ranges" and exclude them from control
+	 */
+	return 0;
+}
+
+static int bd71828_probe(struct platform_device *pdev)
+{
+	struct bd71828_gpio *bdgpio;
+	struct rohm_regmap_dev *bd71828;
+	int ret;
+
+	bd71828 = dev_get_drvdata(pdev->dev.parent);
+	if (!bd71828) {
+		dev_err(&pdev->dev, "No MFD driver data\n");
+		return -EINVAL;
+	}
+
+	bdgpio = devm_kzalloc(&pdev->dev, sizeof(*bdgpio),
+			      GFP_KERNEL);
+	if (!bdgpio)
+		return -ENOMEM;
+
+	ret = bd71828_gpio_parse_dt(pdev->dev.parent, bdgpio);
+
+	bdgpio->chip.dev = &pdev->dev;
+	bdgpio->gpio.parent = pdev->dev.parent;
+	bdgpio->gpio.label = "bd71828-gpio";
+	bdgpio->gpio.owner = THIS_MODULE;
+	bdgpio->gpio.get_direction = bd71828_get_direction;
+	bdgpio->gpio.set_config = bd71828_gpio_set_config;
+	bdgpio->gpio.can_sleep = true;
+	bdgpio->gpio.get = bd71828_gpio_get;
+	bdgpio->gpio.set = bd71828_gpio_set;
+	bdgpio->gpio.base = -1;
+	bdgpio->gpio.ngpio = 4;
+#ifdef CONFIG_OF_GPIO
+	bdgpio->gpio.of_node = pdev->dev.parent->of_node;
+#endif
+	bdgpio->chip.regmap = bd71828->regmap;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &bdgpio->gpio,
+				     bdgpio);
+	if (ret)
+		dev_err(&pdev->dev, "gpio_init: Failed to add bd71828-gpio\n");
+
+	return ret;
+}
+
+static struct platform_driver bd71828_gpio = {
+	.driver = {
+		.name = "bd71828-gpio"
+	},
+	.probe = bd71828_probe,
+};
+
+module_platform_driver(bd71828_gpio);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD71828 voltage regulator driver");
+MODULE_LICENSE("GPL");