lkml.org 
[lkml]   [2019]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC PATCH 10/13] gpio: bd71828: Initial support for ROHM BD71828 PMIC GPIOs
    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 =]

    \
     
     \ /
      Last update: 2019-10-17 14:45    [W:4.597 / U:0.144 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site