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

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

pon., 21 paź 2019 o 09:00 Vaittinen, Matti
<Matti.Vaittinen@xxxxxxxxxxxxxxxxx> 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@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?
>

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@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.
>

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
>