Re: [RFC PATCH 10/13] gpio: bd71828: Initial support for ROHM BD71828 PMIC GPIOs

[Vaittinen, Matti]

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@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
> > 
> > +++ 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@xxxxxxxxxxxxxxxxx>
> > ");
> > +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