| Message ID | f8f8c323d378244afe4e94f48c0a94bb296cbbe0.1571302099.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support ROHM BD71828 PMIC | expand |
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 =]
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
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 >
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
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
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");
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