| Message ID | 20190917105902.445-4-m.felsch@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | Add DA9062 GPIO support | expand |
wt., 17 wrz 2019 o 12:59 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > be used as input, output or have a special use-case. > > The patch adds the support for the normal input/output use-case. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/gpio/Kconfig | 11 ++ > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-da9062.c | 265 ++++++++++++++++++++++++++++++++ > include/linux/mfd/da9062/gpio.h | 13 ++ > 4 files changed, 290 insertions(+) > create mode 100644 drivers/gpio/gpio-da9062.c > create mode 100644 include/linux/mfd/da9062/gpio.h > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index bb13c266c329..b308ea549aaa 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -1038,6 +1038,17 @@ config GPIO_DA9055 > > If driver is built as a module it will be called gpio-da9055. > > +config GPIO_DA9062 > + tristate "Dialog Semiconductor DA9062 GPIO" > + depends on MFD_DA9062 > + help > + Say yes here to enable the GPIO driver for the DA9062 chip. > + > + The Dialog DA9062 PMIC chip has 5 GPIO pins that can be > + be controller by this driver. > + > + If driver is built as a module it will be called gpio-da9062. > + > config GPIO_DLN2 > tristate "Diolan DLN2 GPIO support" > depends on MFD_DLN2 > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index a4e91175c708..f29c8af2d096 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -45,6 +45,7 @@ obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o > obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o > obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o > obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o > +obj-$(CONFIG_GPIO_DA9062) += gpio-da9062.o > obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o > obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o > obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o > diff --git a/drivers/gpio/gpio-da9062.c b/drivers/gpio/gpio-da9062.c > new file mode 100644 > index 000000000000..6035963929a2 > --- /dev/null > +++ b/drivers/gpio/gpio-da9062.c > @@ -0,0 +1,265 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * GPIO Driver for Dialog DA9062 PMICs. > + * Based on DA9055 GPIO driver. > + * > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > + */ > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include <linux/gpio/driver.h> > +#include <linux/gpio/consumer.h> > + > +#include <linux/mfd/da9062/core.h> > +#include <linux/mfd/da9062/registers.h> > + > +#include "gpiolib.h" > + > +#define DA9062_TYPE(offset) (4 * (offset % 2)) > +#define DA9062_PIN_SHIFT(offset) (4 * (offset % 2)) > +#define DA9062_PIN_ALTERNATE 0x00 /* gpio alternate mode */ > +#define DA9062_PIN_GPI 0x01 /* gpio in */ > +#define DA9062_PIN_GPO_OD 0x02 /* gpio out open-drain */ > +#define DA9062_PIN_GPO_PP 0x03 /* gpio out push-pull */ > +#define DA9062_GPIO_NUM 5 > + > +struct da9062_gpio { > + struct da9062 *da9062; > + struct gpio_chip gp; > +}; > + > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc) > +{ > + return gpio_chip_hwgpio(desc); > +} > +EXPORT_SYMBOL_GPL(da9062_gpio_get_hwgpio); > + Is this going to be used anywhere? I'm not really a fan of adding new APIs without users. > +static int da9062_gpio_get_pin_mode(struct regmap *regmap, unsigned int offset) > +{ > + int ret; > + int val; Nit: maybe put these two in a single line? > + > + ret = regmap_read(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), &val); > + if (ret < 0) > + return ret; > + > + val >>= DA9062_PIN_SHIFT(offset); > + val &= DA9062AA_GPIO0_PIN_MASK; > + > + return val; > +} > + > +static int da9062_gpio_set_pin_mode(struct regmap *regmap, unsigned int offset, > + unsigned int mode) > +{ > + unsigned int mask; > + > + mode &= DA9062AA_GPIO0_PIN_MASK; > + mode <<= DA9062_PIN_SHIFT(offset); > + mask = DA9062AA_GPIO0_PIN_MASK << DA9062_PIN_SHIFT(offset); > + > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > + mask, mode); > +} > + > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > +{ > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > + struct regmap *regmap = gpio->da9062->regmap; > + int gpio_dir, val; > + int ret; > + > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > + if (gpio_dir < 0) > + return gpio_dir; > + > + switch (gpio_dir) { > + case DA9062_PIN_ALTERNATE: > + return -ENOTSUPP; > + case DA9062_PIN_GPI: > + ret = regmap_read(regmap, DA9062AA_STATUS_B, &val); > + if (ret < 0) > + return ret; > + break; > + case DA9062_PIN_GPO_OD: > + /* falltrough */ > + case DA9062_PIN_GPO_PP: > + ret = regmap_read(regmap, DA9062AA_GPIO_MODE0_4, &val); > + if (ret < 0) > + return ret; > + } > + > + return val & BIT(offset); > +} > + > +static void da9062_gpio_set(struct gpio_chip *gc, unsigned int offset, > + int value) > +{ > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > + struct regmap *regmap = gpio->da9062->regmap; > + > + regmap_update_bits(regmap, DA9062AA_GPIO_MODE0_4, BIT(offset), > + value << offset); > +} > + > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > +{ > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > + struct regmap *regmap = gpio->da9062->regmap; > + int gpio_dir; > + > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > + if (gpio_dir < 0) > + return gpio_dir; > + > + switch (gpio_dir) { > + case DA9062_PIN_ALTERNATE: > + return -ENOTSUPP; > + case DA9062_PIN_GPI: > + return 1; > + case DA9062_PIN_GPO_OD: > + /* falltrough */ > + case DA9062_PIN_GPO_PP: > + return 0; > + } > + > + return -EINVAL; > +} > + > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > + unsigned int offset) > +{ > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > + struct regmap *regmap = gpio->da9062->regmap; > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > + unsigned int gpi_type; > + int ret; > + > + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > + if (ret) > + return ret; > + > + /* > + * If the gpio is active low we should set it in hw too. No worries > + * about gpio_get() because we read and return the gpio-level. So the > + * gpiolob active_low handling is still correct. > + * > + * 0 - active low, 1 - active high > + */ > + gpi_type = !gpiod_is_active_low(desc); > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), > + gpi_type << DA9062_TYPE(offset)); > +} > + > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > + unsigned int offset, int value) > +{ > + /* Push-Pull / Open-Drain options are configured during set_config */ > + da9062_gpio_set(gc, offset, value); > + > + return 0; > +} > + > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > + unsigned long config) > +{ > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > + struct regmap *regmap = gpio->da9062->regmap; > + int gpio_dir; > + > + switch (pinconf_to_config_param(config)) { > + case PIN_CONFIG_BIAS_PULL_DOWN: > + /* PD only if pin is input */ > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > + if (gpio_dir < 0) > + return -EINVAL; > + else if (gpio_dir != DA9062_PIN_GPI) > + return -ENOTSUPP; > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > + BIT(offset), BIT(offset)); > + case PIN_CONFIG_BIAS_PULL_UP: > + /* PU only if pin is output open-drain */ > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > + if (gpio_dir < 0) > + return -EINVAL; > + else if (gpio_dir != DA9062_PIN_GPO_OD) > + return -ENOTSUPP; > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > + BIT(offset), BIT(offset)); > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > + return da9062_gpio_set_pin_mode(regmap, offset, > + DA9062_PIN_GPO_OD); > + case PIN_CONFIG_DRIVE_PUSH_PULL: > + return da9062_gpio_set_pin_mode(regmap, offset, > + DA9062_PIN_GPO_PP); > + default: > + return -ENOTSUPP; > + } > +} > + > +static int da9062_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > +{ > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > + struct da9062 *da9062 = gpio->da9062; > + > + return regmap_irq_get_virq(da9062->regmap_irq, > + DA9062_IRQ_GPI0 + offset); > +} > + I'm afraid this won't fly anymore. We now have support for hierarchical GPIO irqchips (take a look at Documentation/driver-api/gpio/driver.rst) and Linus is quite strict on enforcing its usage. What I did with a very simple mfd device with GPIOs (where using hierarchical irqchips was clearly overkill) was to put the gpio-controller on the same DT node as the core mfd device - that way there's no need for a hierarchy. Bart > +static const struct gpio_chip reference_gp = { > + .label = "da9062-gpio", > + .owner = THIS_MODULE, > + .get = da9062_gpio_get, > + .set = da9062_gpio_set, > + .get_direction = da9062_gpio_get_direction, > + .direction_input = da9062_gpio_direction_input, > + .direction_output = da9062_gpio_direction_output, > + .set_config = da9062_gpio_set_config, > + .to_irq = da9062_gpio_to_irq, > + .can_sleep = true, > + .ngpio = DA9062_GPIO_NUM, > + .base = -1, > +}; > + > +static int da9062_gpio_probe(struct platform_device *pdev) > +{ > + struct da9062_gpio *gpio; > + > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); > + if (!gpio) > + return -ENOMEM; > + > + gpio->da9062 = dev_get_drvdata(pdev->dev.parent); > + if (!gpio->da9062) > + return -EINVAL; > + > + gpio->gp = reference_gp; > + gpio->gp.parent = &pdev->dev; > + > + platform_set_drvdata(pdev, gpio); > + > + return devm_gpiochip_add_data(&pdev->dev, &gpio->gp, gpio); > +} > + > +static const struct of_device_id da9062_compatible_id_table[] = { > + { .compatible = "dlg,da9062-gpio" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, da9062_compatible_id_table); > + > +static struct platform_driver da9062_gpio_driver = { > + .probe = da9062_gpio_probe, > + .driver = { > + .name = "da9062-gpio", > + .of_match_table = da9062_compatible_id_table, > + }, > +}; > +module_platform_driver(da9062_gpio_driver); > + > +MODULE_AUTHOR("Marco Felsch <kernel@pengutronix.de>"); > +MODULE_DESCRIPTION("DA9062 GPIO Device Driver"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:da9062-gpio"); > diff --git a/include/linux/mfd/da9062/gpio.h b/include/linux/mfd/da9062/gpio.h > new file mode 100644 > index 000000000000..67627ada1ad4 > --- /dev/null > +++ b/include/linux/mfd/da9062/gpio.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > + */ > + > +#ifndef __MFD_DA9062_GPIO_H__ > +#define __MFD_DA9062_GPIO_H__ > + > +struct gpio_desc; > + > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc); > + > +#endif /* __MFD_DA9062_GPIO_H__ */ > -- > 2.20.1 >
On 19-09-18 09:04, Bartosz Golaszewski wrote: > wt., 17 wrz 2019 o 12:59 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > > be used as input, output or have a special use-case. > > > > The patch adds the support for the normal input/output use-case. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/gpio/Kconfig | 11 ++ > > drivers/gpio/Makefile | 1 + > > drivers/gpio/gpio-da9062.c | 265 ++++++++++++++++++++++++++++++++ > > include/linux/mfd/da9062/gpio.h | 13 ++ > > 4 files changed, 290 insertions(+) > > create mode 100644 drivers/gpio/gpio-da9062.c > > create mode 100644 include/linux/mfd/da9062/gpio.h > > > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > index bb13c266c329..b308ea549aaa 100644 > > --- a/drivers/gpio/Kconfig > > +++ b/drivers/gpio/Kconfig > > @@ -1038,6 +1038,17 @@ config GPIO_DA9055 > > > > If driver is built as a module it will be called gpio-da9055. > > > > +config GPIO_DA9062 > > + tristate "Dialog Semiconductor DA9062 GPIO" > > + depends on MFD_DA9062 > > + help > > + Say yes here to enable the GPIO driver for the DA9062 chip. > > + > > + The Dialog DA9062 PMIC chip has 5 GPIO pins that can be > > + be controller by this driver. > > + > > + If driver is built as a module it will be called gpio-da9062. > > + > > config GPIO_DLN2 > > tristate "Diolan DLN2 GPIO support" > > depends on MFD_DLN2 > > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > > index a4e91175c708..f29c8af2d096 100644 > > --- a/drivers/gpio/Makefile > > +++ b/drivers/gpio/Makefile > > @@ -45,6 +45,7 @@ obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o > > obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o > > obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o > > obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o > > +obj-$(CONFIG_GPIO_DA9062) += gpio-da9062.o > > obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o > > obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o > > obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o > > diff --git a/drivers/gpio/gpio-da9062.c b/drivers/gpio/gpio-da9062.c > > new file mode 100644 > > index 000000000000..6035963929a2 > > --- /dev/null > > +++ b/drivers/gpio/gpio-da9062.c > > @@ -0,0 +1,265 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * GPIO Driver for Dialog DA9062 PMICs. > > + * Based on DA9055 GPIO driver. > > + * > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > + */ > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > + > > +#include <linux/gpio/driver.h> > > +#include <linux/gpio/consumer.h> > > + > > +#include <linux/mfd/da9062/core.h> > > +#include <linux/mfd/da9062/registers.h> > > + > > +#include "gpiolib.h" > > + > > +#define DA9062_TYPE(offset) (4 * (offset % 2)) > > +#define DA9062_PIN_SHIFT(offset) (4 * (offset % 2)) > > +#define DA9062_PIN_ALTERNATE 0x00 /* gpio alternate mode */ > > +#define DA9062_PIN_GPI 0x01 /* gpio in */ > > +#define DA9062_PIN_GPO_OD 0x02 /* gpio out open-drain */ > > +#define DA9062_PIN_GPO_PP 0x03 /* gpio out push-pull */ > > +#define DA9062_GPIO_NUM 5 > > + > > +struct da9062_gpio { > > + struct da9062 *da9062; > > + struct gpio_chip gp; > > +}; > > + > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc) > > +{ > > + return gpio_chip_hwgpio(desc); > > +} > > +EXPORT_SYMBOL_GPL(da9062_gpio_get_hwgpio); > > + > > Is this going to be used anywhere? I'm not really a fan of adding new > APIs without users. Yes, it is used here: https://lkml.org/lkml/2019/9/17/411 I don't know if I should add the gpio here or as seperate patch within the above series. > > +static int da9062_gpio_get_pin_mode(struct regmap *regmap, unsigned int offset) > > +{ > > + int ret; > > + int val; > > Nit: maybe put these two in a single line? Yes. > > + > > + ret = regmap_read(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), &val); > > + if (ret < 0) > > + return ret; > > + > > + val >>= DA9062_PIN_SHIFT(offset); > > + val &= DA9062AA_GPIO0_PIN_MASK; > > + > > + return val; > > +} > > + > > +static int da9062_gpio_set_pin_mode(struct regmap *regmap, unsigned int offset, > > + unsigned int mode) > > +{ > > + unsigned int mask; > > + > > + mode &= DA9062AA_GPIO0_PIN_MASK; > > + mode <<= DA9062_PIN_SHIFT(offset); > > + mask = DA9062AA_GPIO0_PIN_MASK << DA9062_PIN_SHIFT(offset); > > + > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > + mask, mode); > > +} > > + > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > > +{ > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > + struct regmap *regmap = gpio->da9062->regmap; > > + int gpio_dir, val; > > + int ret; > > + > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > + if (gpio_dir < 0) > > + return gpio_dir; > > + > > + switch (gpio_dir) { > > + case DA9062_PIN_ALTERNATE: > > + return -ENOTSUPP; > > + case DA9062_PIN_GPI: > > + ret = regmap_read(regmap, DA9062AA_STATUS_B, &val); > > + if (ret < 0) > > + return ret; > > + break; > > + case DA9062_PIN_GPO_OD: > > + /* falltrough */ > > + case DA9062_PIN_GPO_PP: > > + ret = regmap_read(regmap, DA9062AA_GPIO_MODE0_4, &val); > > + if (ret < 0) > > + return ret; > > + } > > + > > + return val & BIT(offset); > > +} > > + > > +static void da9062_gpio_set(struct gpio_chip *gc, unsigned int offset, > > + int value) > > +{ > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > + struct regmap *regmap = gpio->da9062->regmap; > > + > > + regmap_update_bits(regmap, DA9062AA_GPIO_MODE0_4, BIT(offset), > > + value << offset); > > +} > > + > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > > +{ > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > + struct regmap *regmap = gpio->da9062->regmap; > > + int gpio_dir; > > + > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > + if (gpio_dir < 0) > > + return gpio_dir; > > + > > + switch (gpio_dir) { > > + case DA9062_PIN_ALTERNATE: > > + return -ENOTSUPP; > > + case DA9062_PIN_GPI: > > + return 1; > > + case DA9062_PIN_GPO_OD: > > + /* falltrough */ > > + case DA9062_PIN_GPO_PP: > > + return 0; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > + unsigned int offset) > > +{ > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > + struct regmap *regmap = gpio->da9062->regmap; > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > + unsigned int gpi_type; > > + int ret; > > + > > + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > + if (ret) > > + return ret; > > + > > + /* > > + * If the gpio is active low we should set it in hw too. No worries > > + * about gpio_get() because we read and return the gpio-level. So the > > + * gpiolob active_low handling is still correct. > > + * > > + * 0 - active low, 1 - active high > > + */ > > + gpi_type = !gpiod_is_active_low(desc); > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), > > + gpi_type << DA9062_TYPE(offset)); > > +} > > + > > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > > + unsigned int offset, int value) > > +{ > > + /* Push-Pull / Open-Drain options are configured during set_config */ > > + da9062_gpio_set(gc, offset, value); > > + > > + return 0; > > +} > > + > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > > + unsigned long config) > > +{ > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > + struct regmap *regmap = gpio->da9062->regmap; > > + int gpio_dir; > > + > > + switch (pinconf_to_config_param(config)) { > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > + /* PD only if pin is input */ > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > + if (gpio_dir < 0) > > + return -EINVAL; > > + else if (gpio_dir != DA9062_PIN_GPI) > > + return -ENOTSUPP; > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > + BIT(offset), BIT(offset)); > > + case PIN_CONFIG_BIAS_PULL_UP: > > + /* PU only if pin is output open-drain */ > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > + if (gpio_dir < 0) > > + return -EINVAL; > > + else if (gpio_dir != DA9062_PIN_GPO_OD) > > + return -ENOTSUPP; > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > + BIT(offset), BIT(offset)); > > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > + return da9062_gpio_set_pin_mode(regmap, offset, > > + DA9062_PIN_GPO_OD); > > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > + return da9062_gpio_set_pin_mode(regmap, offset, > > + DA9062_PIN_GPO_PP); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int da9062_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > > +{ > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > + struct da9062 *da9062 = gpio->da9062; > > + > > + return regmap_irq_get_virq(da9062->regmap_irq, > > + DA9062_IRQ_GPI0 + offset); > > +} > > + > > I'm afraid this won't fly anymore. We now have support for > hierarchical GPIO irqchips (take a look at > Documentation/driver-api/gpio/driver.rst) and Linus is quite strict on > enforcing its usage. What I did with a very simple mfd device with > GPIOs (where using hierarchical irqchips was clearly overkill) was to > put the gpio-controller on the same DT node as the core mfd device - > that way there's no need for a hierarchy. Okay, I've checked the documentation and the code. If I understood it right I should request each irq using platform_get_irq_byname() as you did for the max77650? Regards, Marco > Bart > > > +static const struct gpio_chip reference_gp = { > > + .label = "da9062-gpio", > > + .owner = THIS_MODULE, > > + .get = da9062_gpio_get, > > + .set = da9062_gpio_set, > > + .get_direction = da9062_gpio_get_direction, > > + .direction_input = da9062_gpio_direction_input, > > + .direction_output = da9062_gpio_direction_output, > > + .set_config = da9062_gpio_set_config, > > + .to_irq = da9062_gpio_to_irq, > > + .can_sleep = true, > > + .ngpio = DA9062_GPIO_NUM, > > + .base = -1, > > +}; > > + > > +static int da9062_gpio_probe(struct platform_device *pdev) > > +{ > > + struct da9062_gpio *gpio; > > + > > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); > > + if (!gpio) > > + return -ENOMEM; > > + > > + gpio->da9062 = dev_get_drvdata(pdev->dev.parent); > > + if (!gpio->da9062) > > + return -EINVAL; > > + > > + gpio->gp = reference_gp; > > + gpio->gp.parent = &pdev->dev; > > + > > + platform_set_drvdata(pdev, gpio); > > + > > + return devm_gpiochip_add_data(&pdev->dev, &gpio->gp, gpio); > > +} > > + > > +static const struct of_device_id da9062_compatible_id_table[] = { > > + { .compatible = "dlg,da9062-gpio" }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(of, da9062_compatible_id_table); > > + > > +static struct platform_driver da9062_gpio_driver = { > > + .probe = da9062_gpio_probe, > > + .driver = { > > + .name = "da9062-gpio", > > + .of_match_table = da9062_compatible_id_table, > > + }, > > +}; > > +module_platform_driver(da9062_gpio_driver); > > + > > +MODULE_AUTHOR("Marco Felsch <kernel@pengutronix.de>"); > > +MODULE_DESCRIPTION("DA9062 GPIO Device Driver"); > > +MODULE_LICENSE("GPL v2"); > > +MODULE_ALIAS("platform:da9062-gpio"); > > diff --git a/include/linux/mfd/da9062/gpio.h b/include/linux/mfd/da9062/gpio.h > > new file mode 100644 > > index 000000000000..67627ada1ad4 > > --- /dev/null > > +++ b/include/linux/mfd/da9062/gpio.h > > @@ -0,0 +1,13 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > + */ > > + > > +#ifndef __MFD_DA9062_GPIO_H__ > > +#define __MFD_DA9062_GPIO_H__ > > + > > +struct gpio_desc; > > + > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc); > > + > > +#endif /* __MFD_DA9062_GPIO_H__ */ > > -- > > 2.20.1 > > >
śr., 18 wrz 2019 o 14:06 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > On 19-09-18 09:04, Bartosz Golaszewski wrote: > > wt., 17 wrz 2019 o 12:59 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > > > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > > > be used as input, output or have a special use-case. > > > > > > The patch adds the support for the normal input/output use-case. > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > --- > > > drivers/gpio/Kconfig | 11 ++ > > > drivers/gpio/Makefile | 1 + > > > drivers/gpio/gpio-da9062.c | 265 ++++++++++++++++++++++++++++++++ > > > include/linux/mfd/da9062/gpio.h | 13 ++ > > > 4 files changed, 290 insertions(+) > > > create mode 100644 drivers/gpio/gpio-da9062.c > > > create mode 100644 include/linux/mfd/da9062/gpio.h > > > > > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > > index bb13c266c329..b308ea549aaa 100644 > > > --- a/drivers/gpio/Kconfig > > > +++ b/drivers/gpio/Kconfig > > > @@ -1038,6 +1038,17 @@ config GPIO_DA9055 > > > > > > If driver is built as a module it will be called gpio-da9055. > > > > > > +config GPIO_DA9062 > > > + tristate "Dialog Semiconductor DA9062 GPIO" > > > + depends on MFD_DA9062 > > > + help > > > + Say yes here to enable the GPIO driver for the DA9062 chip. > > > + > > > + The Dialog DA9062 PMIC chip has 5 GPIO pins that can be > > > + be controller by this driver. > > > + > > > + If driver is built as a module it will be called gpio-da9062. > > > + > > > config GPIO_DLN2 > > > tristate "Diolan DLN2 GPIO support" > > > depends on MFD_DLN2 > > > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > > > index a4e91175c708..f29c8af2d096 100644 > > > --- a/drivers/gpio/Makefile > > > +++ b/drivers/gpio/Makefile > > > @@ -45,6 +45,7 @@ obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o > > > obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o > > > obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o > > > obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o > > > +obj-$(CONFIG_GPIO_DA9062) += gpio-da9062.o > > > obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o > > > obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o > > > obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o > > > diff --git a/drivers/gpio/gpio-da9062.c b/drivers/gpio/gpio-da9062.c > > > new file mode 100644 > > > index 000000000000..6035963929a2 > > > --- /dev/null > > > +++ b/drivers/gpio/gpio-da9062.c > > > @@ -0,0 +1,265 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * GPIO Driver for Dialog DA9062 PMICs. > > > + * Based on DA9055 GPIO driver. > > > + * > > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > > + */ > > > +#include <linux/module.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/regmap.h> > > > + > > > +#include <linux/gpio/driver.h> > > > +#include <linux/gpio/consumer.h> > > > + > > > +#include <linux/mfd/da9062/core.h> > > > +#include <linux/mfd/da9062/registers.h> > > > + > > > +#include "gpiolib.h" > > > + > > > +#define DA9062_TYPE(offset) (4 * (offset % 2)) > > > +#define DA9062_PIN_SHIFT(offset) (4 * (offset % 2)) > > > +#define DA9062_PIN_ALTERNATE 0x00 /* gpio alternate mode */ > > > +#define DA9062_PIN_GPI 0x01 /* gpio in */ > > > +#define DA9062_PIN_GPO_OD 0x02 /* gpio out open-drain */ > > > +#define DA9062_PIN_GPO_PP 0x03 /* gpio out push-pull */ > > > +#define DA9062_GPIO_NUM 5 > > > + > > > +struct da9062_gpio { > > > + struct da9062 *da9062; > > > + struct gpio_chip gp; > > > +}; > > > + > > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc) > > > +{ > > > + return gpio_chip_hwgpio(desc); > > > +} > > > +EXPORT_SYMBOL_GPL(da9062_gpio_get_hwgpio); > > > + > > > > Is this going to be used anywhere? I'm not really a fan of adding new > > APIs without users. > > Yes, it is used here: https://lkml.org/lkml/2019/9/17/411 > > I don't know if I should add the gpio here or as seperate patch within > the above series. > > > > +static int da9062_gpio_get_pin_mode(struct regmap *regmap, unsigned int offset) > > > +{ > > > + int ret; > > > + int val; > > > > Nit: maybe put these two in a single line? > > Yes. > > > > + > > > + ret = regmap_read(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), &val); > > > + if (ret < 0) > > > + return ret; > > > + > > > + val >>= DA9062_PIN_SHIFT(offset); > > > + val &= DA9062AA_GPIO0_PIN_MASK; > > > + > > > + return val; > > > +} > > > + > > > +static int da9062_gpio_set_pin_mode(struct regmap *regmap, unsigned int offset, > > > + unsigned int mode) > > > +{ > > > + unsigned int mask; > > > + > > > + mode &= DA9062AA_GPIO0_PIN_MASK; > > > + mode <<= DA9062_PIN_SHIFT(offset); > > > + mask = DA9062AA_GPIO0_PIN_MASK << DA9062_PIN_SHIFT(offset); > > > + > > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > > + mask, mode); > > > +} > > > + > > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > > > +{ > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > + struct regmap *regmap = gpio->da9062->regmap; > > > + int gpio_dir, val; > > > + int ret; > > > + > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > + if (gpio_dir < 0) > > > + return gpio_dir; > > > + > > > + switch (gpio_dir) { > > > + case DA9062_PIN_ALTERNATE: > > > + return -ENOTSUPP; > > > + case DA9062_PIN_GPI: > > > + ret = regmap_read(regmap, DA9062AA_STATUS_B, &val); > > > + if (ret < 0) > > > + return ret; > > > + break; > > > + case DA9062_PIN_GPO_OD: > > > + /* falltrough */ > > > + case DA9062_PIN_GPO_PP: > > > + ret = regmap_read(regmap, DA9062AA_GPIO_MODE0_4, &val); > > > + if (ret < 0) > > > + return ret; > > > + } > > > + > > > + return val & BIT(offset); > > > +} > > > + > > > +static void da9062_gpio_set(struct gpio_chip *gc, unsigned int offset, > > > + int value) > > > +{ > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > + struct regmap *regmap = gpio->da9062->regmap; > > > + > > > + regmap_update_bits(regmap, DA9062AA_GPIO_MODE0_4, BIT(offset), > > > + value << offset); > > > +} > > > + > > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > > > +{ > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > + struct regmap *regmap = gpio->da9062->regmap; > > > + int gpio_dir; > > > + > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > + if (gpio_dir < 0) > > > + return gpio_dir; > > > + > > > + switch (gpio_dir) { > > > + case DA9062_PIN_ALTERNATE: > > > + return -ENOTSUPP; > > > + case DA9062_PIN_GPI: > > > + return 1; > > > + case DA9062_PIN_GPO_OD: > > > + /* falltrough */ > > > + case DA9062_PIN_GPO_PP: > > > + return 0; > > > + } > > > + > > > + return -EINVAL; > > > +} > > > + > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > > + unsigned int offset) > > > +{ > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > + struct regmap *regmap = gpio->da9062->regmap; > > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > > + unsigned int gpi_type; > > > + int ret; > > > + > > > + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > > + if (ret) > > > + return ret; > > > + > > > + /* > > > + * If the gpio is active low we should set it in hw too. No worries > > > + * about gpio_get() because we read and return the gpio-level. So the > > > + * gpiolob active_low handling is still correct. > > > + * > > > + * 0 - active low, 1 - active high > > > + */ > > > + gpi_type = !gpiod_is_active_low(desc); > > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > > + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), > > > + gpi_type << DA9062_TYPE(offset)); > > > +} > > > + > > > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > > > + unsigned int offset, int value) > > > +{ > > > + /* Push-Pull / Open-Drain options are configured during set_config */ > > > + da9062_gpio_set(gc, offset, value); > > > + > > > + return 0; > > > +} > > > + > > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > > > + unsigned long config) > > > +{ > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > + struct regmap *regmap = gpio->da9062->regmap; > > > + int gpio_dir; > > > + > > > + switch (pinconf_to_config_param(config)) { > > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > > + /* PD only if pin is input */ > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > + if (gpio_dir < 0) > > > + return -EINVAL; > > > + else if (gpio_dir != DA9062_PIN_GPI) > > > + return -ENOTSUPP; > > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > > + BIT(offset), BIT(offset)); > > > + case PIN_CONFIG_BIAS_PULL_UP: > > > + /* PU only if pin is output open-drain */ > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > + if (gpio_dir < 0) > > > + return -EINVAL; > > > + else if (gpio_dir != DA9062_PIN_GPO_OD) > > > + return -ENOTSUPP; > > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > > + BIT(offset), BIT(offset)); > > > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > > + return da9062_gpio_set_pin_mode(regmap, offset, > > > + DA9062_PIN_GPO_OD); > > > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > > + return da9062_gpio_set_pin_mode(regmap, offset, > > > + DA9062_PIN_GPO_PP); > > > + default: > > > + return -ENOTSUPP; > > > + } > > > +} > > > + > > > +static int da9062_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > > > +{ > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > + struct da9062 *da9062 = gpio->da9062; > > > + > > > + return regmap_irq_get_virq(da9062->regmap_irq, > > > + DA9062_IRQ_GPI0 + offset); > > > +} > > > + > > > > I'm afraid this won't fly anymore. We now have support for > > hierarchical GPIO irqchips (take a look at > > Documentation/driver-api/gpio/driver.rst) and Linus is quite strict on > > enforcing its usage. What I did with a very simple mfd device with > > GPIOs (where using hierarchical irqchips was clearly overkill) was to > > put the gpio-controller on the same DT node as the core mfd device - > > that way there's no need for a hierarchy. > > Okay, I've checked the documentation and the code. If I understood it > right I should request each irq using platform_get_irq_byname() as you > did for the max77650? > No, regmap irq domain is fine, as long as you modify the DT bindings to not use a sub-node for the gpio-controller. > Regards, > Marco > > > Bart > > > > > +static const struct gpio_chip reference_gp = { > > > + .label = "da9062-gpio", > > > + .owner = THIS_MODULE, > > > + .get = da9062_gpio_get, > > > + .set = da9062_gpio_set, > > > + .get_direction = da9062_gpio_get_direction, > > > + .direction_input = da9062_gpio_direction_input, > > > + .direction_output = da9062_gpio_direction_output, > > > + .set_config = da9062_gpio_set_config, > > > + .to_irq = da9062_gpio_to_irq, > > > + .can_sleep = true, > > > + .ngpio = DA9062_GPIO_NUM, > > > + .base = -1, > > > +}; > > > + > > > +static int da9062_gpio_probe(struct platform_device *pdev) > > > +{ > > > + struct da9062_gpio *gpio; > > > + > > > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); > > > + if (!gpio) > > > + return -ENOMEM; > > > + > > > + gpio->da9062 = dev_get_drvdata(pdev->dev.parent); > > > + if (!gpio->da9062) > > > + return -EINVAL; > > > + > > > + gpio->gp = reference_gp; > > > + gpio->gp.parent = &pdev->dev; > > > + > > > + platform_set_drvdata(pdev, gpio); > > > + > > > + return devm_gpiochip_add_data(&pdev->dev, &gpio->gp, gpio); > > > +} > > > + > > > +static const struct of_device_id da9062_compatible_id_table[] = { > > > + { .compatible = "dlg,da9062-gpio" }, > > > + { }, > > > +}; > > > +MODULE_DEVICE_TABLE(of, da9062_compatible_id_table); > > > + > > > +static struct platform_driver da9062_gpio_driver = { > > > + .probe = da9062_gpio_probe, > > > + .driver = { > > > + .name = "da9062-gpio", > > > + .of_match_table = da9062_compatible_id_table, > > > + }, > > > +}; > > > +module_platform_driver(da9062_gpio_driver); > > > + > > > +MODULE_AUTHOR("Marco Felsch <kernel@pengutronix.de>"); > > > +MODULE_DESCRIPTION("DA9062 GPIO Device Driver"); > > > +MODULE_LICENSE("GPL v2"); > > > +MODULE_ALIAS("platform:da9062-gpio"); > > > diff --git a/include/linux/mfd/da9062/gpio.h b/include/linux/mfd/da9062/gpio.h > > > new file mode 100644 > > > index 000000000000..67627ada1ad4 > > > --- /dev/null > > > +++ b/include/linux/mfd/da9062/gpio.h > > > @@ -0,0 +1,13 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +/* > > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > > + */ > > > + > > > +#ifndef __MFD_DA9062_GPIO_H__ > > > +#define __MFD_DA9062_GPIO_H__ > > > + > > > +struct gpio_desc; > > > + > > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc); > > > + > > > +#endif /* __MFD_DA9062_GPIO_H__ */ > > > -- > > > 2.20.1 > > > > > > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 19-09-19 10:24, Bartosz Golaszewski wrote: > śr., 18 wrz 2019 o 14:06 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > On 19-09-18 09:04, Bartosz Golaszewski wrote: > > > wt., 17 wrz 2019 o 12:59 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > > > > > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > > > > be used as input, output or have a special use-case. > > > > > > > > The patch adds the support for the normal input/output use-case. > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > --- > > > > drivers/gpio/Kconfig | 11 ++ > > > > drivers/gpio/Makefile | 1 + > > > > drivers/gpio/gpio-da9062.c | 265 ++++++++++++++++++++++++++++++++ > > > > include/linux/mfd/da9062/gpio.h | 13 ++ > > > > 4 files changed, 290 insertions(+) > > > > create mode 100644 drivers/gpio/gpio-da9062.c > > > > create mode 100644 include/linux/mfd/da9062/gpio.h > > > > > > > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > > > index bb13c266c329..b308ea549aaa 100644 > > > > --- a/drivers/gpio/Kconfig > > > > +++ b/drivers/gpio/Kconfig > > > > @@ -1038,6 +1038,17 @@ config GPIO_DA9055 > > > > > > > > If driver is built as a module it will be called gpio-da9055. > > > > > > > > +config GPIO_DA9062 > > > > + tristate "Dialog Semiconductor DA9062 GPIO" > > > > + depends on MFD_DA9062 > > > > + help > > > > + Say yes here to enable the GPIO driver for the DA9062 chip. > > > > + > > > > + The Dialog DA9062 PMIC chip has 5 GPIO pins that can be > > > > + be controller by this driver. > > > > + > > > > + If driver is built as a module it will be called gpio-da9062. > > > > + > > > > config GPIO_DLN2 > > > > tristate "Diolan DLN2 GPIO support" > > > > depends on MFD_DLN2 > > > > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > > > > index a4e91175c708..f29c8af2d096 100644 > > > > --- a/drivers/gpio/Makefile > > > > +++ b/drivers/gpio/Makefile > > > > @@ -45,6 +45,7 @@ obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o > > > > obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o > > > > obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o > > > > obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o > > > > +obj-$(CONFIG_GPIO_DA9062) += gpio-da9062.o > > > > obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o > > > > obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o > > > > obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o > > > > diff --git a/drivers/gpio/gpio-da9062.c b/drivers/gpio/gpio-da9062.c > > > > new file mode 100644 > > > > index 000000000000..6035963929a2 > > > > --- /dev/null > > > > +++ b/drivers/gpio/gpio-da9062.c > > > > @@ -0,0 +1,265 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * GPIO Driver for Dialog DA9062 PMICs. > > > > + * Based on DA9055 GPIO driver. > > > > + * > > > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > > > + */ > > > > +#include <linux/module.h> > > > > +#include <linux/platform_device.h> > > > > +#include <linux/regmap.h> > > > > + > > > > +#include <linux/gpio/driver.h> > > > > +#include <linux/gpio/consumer.h> > > > > + > > > > +#include <linux/mfd/da9062/core.h> > > > > +#include <linux/mfd/da9062/registers.h> > > > > + > > > > +#include "gpiolib.h" > > > > + > > > > +#define DA9062_TYPE(offset) (4 * (offset % 2)) > > > > +#define DA9062_PIN_SHIFT(offset) (4 * (offset % 2)) > > > > +#define DA9062_PIN_ALTERNATE 0x00 /* gpio alternate mode */ > > > > +#define DA9062_PIN_GPI 0x01 /* gpio in */ > > > > +#define DA9062_PIN_GPO_OD 0x02 /* gpio out open-drain */ > > > > +#define DA9062_PIN_GPO_PP 0x03 /* gpio out push-pull */ > > > > +#define DA9062_GPIO_NUM 5 > > > > + > > > > +struct da9062_gpio { > > > > + struct da9062 *da9062; > > > > + struct gpio_chip gp; > > > > +}; > > > > + > > > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc) > > > > +{ > > > > + return gpio_chip_hwgpio(desc); > > > > +} > > > > +EXPORT_SYMBOL_GPL(da9062_gpio_get_hwgpio); > > > > + > > > > > > Is this going to be used anywhere? I'm not really a fan of adding new > > > APIs without users. > > > > Yes, it is used here: https://lkml.org/lkml/2019/9/17/411 > > > > I don't know if I should add the gpio here or as seperate patch within > > the above series. > > > > > > +static int da9062_gpio_get_pin_mode(struct regmap *regmap, unsigned int offset) > > > > +{ > > > > + int ret; > > > > + int val; > > > > > > Nit: maybe put these two in a single line? > > > > Yes. > > > > > > + > > > > + ret = regmap_read(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), &val); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + val >>= DA9062_PIN_SHIFT(offset); > > > > + val &= DA9062AA_GPIO0_PIN_MASK; > > > > + > > > > + return val; > > > > +} > > > > + > > > > +static int da9062_gpio_set_pin_mode(struct regmap *regmap, unsigned int offset, > > > > + unsigned int mode) > > > > +{ > > > > + unsigned int mask; > > > > + > > > > + mode &= DA9062AA_GPIO0_PIN_MASK; > > > > + mode <<= DA9062_PIN_SHIFT(offset); > > > > + mask = DA9062AA_GPIO0_PIN_MASK << DA9062_PIN_SHIFT(offset); > > > > + > > > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > > > + mask, mode); > > > > +} > > > > + > > > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > > > > +{ > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > + int gpio_dir, val; > > > > + int ret; > > > > + > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > + if (gpio_dir < 0) > > > > + return gpio_dir; > > > > + > > > > + switch (gpio_dir) { > > > > + case DA9062_PIN_ALTERNATE: > > > > + return -ENOTSUPP; > > > > + case DA9062_PIN_GPI: > > > > + ret = regmap_read(regmap, DA9062AA_STATUS_B, &val); > > > > + if (ret < 0) > > > > + return ret; > > > > + break; > > > > + case DA9062_PIN_GPO_OD: > > > > + /* falltrough */ > > > > + case DA9062_PIN_GPO_PP: > > > > + ret = regmap_read(regmap, DA9062AA_GPIO_MODE0_4, &val); > > > > + if (ret < 0) > > > > + return ret; > > > > + } > > > > + > > > > + return val & BIT(offset); > > > > +} > > > > + > > > > +static void da9062_gpio_set(struct gpio_chip *gc, unsigned int offset, > > > > + int value) > > > > +{ > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > + > > > > + regmap_update_bits(regmap, DA9062AA_GPIO_MODE0_4, BIT(offset), > > > > + value << offset); > > > > +} > > > > + > > > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > > > > +{ > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > + int gpio_dir; > > > > + > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > + if (gpio_dir < 0) > > > > + return gpio_dir; > > > > + > > > > + switch (gpio_dir) { > > > > + case DA9062_PIN_ALTERNATE: > > > > + return -ENOTSUPP; > > > > + case DA9062_PIN_GPI: > > > > + return 1; > > > > + case DA9062_PIN_GPO_OD: > > > > + /* falltrough */ > > > > + case DA9062_PIN_GPO_PP: > > > > + return 0; > > > > + } > > > > + > > > > + return -EINVAL; > > > > +} > > > > + > > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > > > + unsigned int offset) > > > > +{ > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > > > + unsigned int gpi_type; > > > > + int ret; > > > > + > > > > + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* > > > > + * If the gpio is active low we should set it in hw too. No worries > > > > + * about gpio_get() because we read and return the gpio-level. So the > > > > + * gpiolob active_low handling is still correct. > > > > + * > > > > + * 0 - active low, 1 - active high > > > > + */ > > > > + gpi_type = !gpiod_is_active_low(desc); > > > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > > > + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), > > > > + gpi_type << DA9062_TYPE(offset)); > > > > +} > > > > + > > > > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > > > > + unsigned int offset, int value) > > > > +{ > > > > + /* Push-Pull / Open-Drain options are configured during set_config */ > > > > + da9062_gpio_set(gc, offset, value); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > > > > + unsigned long config) > > > > +{ > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > + int gpio_dir; > > > > + > > > > + switch (pinconf_to_config_param(config)) { > > > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > > > + /* PD only if pin is input */ > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > + if (gpio_dir < 0) > > > > + return -EINVAL; > > > > + else if (gpio_dir != DA9062_PIN_GPI) > > > > + return -ENOTSUPP; > > > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > > > + BIT(offset), BIT(offset)); > > > > + case PIN_CONFIG_BIAS_PULL_UP: > > > > + /* PU only if pin is output open-drain */ > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > + if (gpio_dir < 0) > > > > + return -EINVAL; > > > > + else if (gpio_dir != DA9062_PIN_GPO_OD) > > > > + return -ENOTSUPP; > > > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > > > + BIT(offset), BIT(offset)); > > > > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > > > + return da9062_gpio_set_pin_mode(regmap, offset, > > > > + DA9062_PIN_GPO_OD); > > > > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > > > + return da9062_gpio_set_pin_mode(regmap, offset, > > > > + DA9062_PIN_GPO_PP); > > > > + default: > > > > + return -ENOTSUPP; > > > > + } > > > > +} > > > > + > > > > +static int da9062_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > > > > +{ > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > + struct da9062 *da9062 = gpio->da9062; > > > > + > > > > + return regmap_irq_get_virq(da9062->regmap_irq, > > > > + DA9062_IRQ_GPI0 + offset); > > > > +} > > > > + > > > > > > I'm afraid this won't fly anymore. We now have support for > > > hierarchical GPIO irqchips (take a look at > > > Documentation/driver-api/gpio/driver.rst) and Linus is quite strict on > > > enforcing its usage. What I did with a very simple mfd device with > > > GPIOs (where using hierarchical irqchips was clearly overkill) was to > > > put the gpio-controller on the same DT node as the core mfd device - > > > that way there's no need for a hierarchy. > > > > Okay, I've checked the documentation and the code. If I understood it > > right I should request each irq using platform_get_irq_byname() as you > > did for the max77650? > > > > No, regmap irq domain is fine, as long as you modify the DT bindings > to not use a sub-node for the gpio-controller. Ah okay.. thanks for clearing that. Regards, Marco > > Regards, > > Marco > > > > > Bart > > > > > > > +static const struct gpio_chip reference_gp = { > > > > + .label = "da9062-gpio", > > > > + .owner = THIS_MODULE, > > > > + .get = da9062_gpio_get, > > > > + .set = da9062_gpio_set, > > > > + .get_direction = da9062_gpio_get_direction, > > > > + .direction_input = da9062_gpio_direction_input, > > > > + .direction_output = da9062_gpio_direction_output, > > > > + .set_config = da9062_gpio_set_config, > > > > + .to_irq = da9062_gpio_to_irq, > > > > + .can_sleep = true, > > > > + .ngpio = DA9062_GPIO_NUM, > > > > + .base = -1, > > > > +}; > > > > + > > > > +static int da9062_gpio_probe(struct platform_device *pdev) > > > > +{ > > > > + struct da9062_gpio *gpio; > > > > + > > > > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); > > > > + if (!gpio) > > > > + return -ENOMEM; > > > > + > > > > + gpio->da9062 = dev_get_drvdata(pdev->dev.parent); > > > > + if (!gpio->da9062) > > > > + return -EINVAL; > > > > + > > > > + gpio->gp = reference_gp; > > > > + gpio->gp.parent = &pdev->dev; > > > > + > > > > + platform_set_drvdata(pdev, gpio); > > > > + > > > > + return devm_gpiochip_add_data(&pdev->dev, &gpio->gp, gpio); > > > > +} > > > > + > > > > +static const struct of_device_id da9062_compatible_id_table[] = { > > > > + { .compatible = "dlg,da9062-gpio" }, > > > > + { }, > > > > +}; > > > > +MODULE_DEVICE_TABLE(of, da9062_compatible_id_table); > > > > + > > > > +static struct platform_driver da9062_gpio_driver = { > > > > + .probe = da9062_gpio_probe, > > > > + .driver = { > > > > + .name = "da9062-gpio", > > > > + .of_match_table = da9062_compatible_id_table, > > > > + }, > > > > +}; > > > > +module_platform_driver(da9062_gpio_driver); > > > > + > > > > +MODULE_AUTHOR("Marco Felsch <kernel@pengutronix.de>"); > > > > +MODULE_DESCRIPTION("DA9062 GPIO Device Driver"); > > > > +MODULE_LICENSE("GPL v2"); > > > > +MODULE_ALIAS("platform:da9062-gpio"); > > > > diff --git a/include/linux/mfd/da9062/gpio.h b/include/linux/mfd/da9062/gpio.h > > > > new file mode 100644 > > > > index 000000000000..67627ada1ad4 > > > > --- /dev/null > > > > +++ b/include/linux/mfd/da9062/gpio.h > > > > @@ -0,0 +1,13 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > +/* > > > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > > > + */ > > > > + > > > > +#ifndef __MFD_DA9062_GPIO_H__ > > > > +#define __MFD_DA9062_GPIO_H__ > > > > + > > > > +struct gpio_desc; > > > > + > > > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc); > > > > + > > > > +#endif /* __MFD_DA9062_GPIO_H__ */ > > > > -- > > > > 2.20.1 > > > > > > > > > > > -- > > Pengutronix e.K. | | > > Industrial Linux Solutions | http://www.pengutronix.de/ | > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >
Hi Bartosz, On 19-09-19 10:38, Marco Felsch wrote: > On 19-09-19 10:24, Bartosz Golaszewski wrote: > > śr., 18 wrz 2019 o 14:06 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > > > On 19-09-18 09:04, Bartosz Golaszewski wrote: > > > > wt., 17 wrz 2019 o 12:59 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > > > > > > > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > > > > > be used as input, output or have a special use-case. > > > > > > > > > > The patch adds the support for the normal input/output use-case. > > > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > > --- > > > > > drivers/gpio/Kconfig | 11 ++ > > > > > drivers/gpio/Makefile | 1 + > > > > > drivers/gpio/gpio-da9062.c | 265 ++++++++++++++++++++++++++++++++ > > > > > include/linux/mfd/da9062/gpio.h | 13 ++ > > > > > 4 files changed, 290 insertions(+) > > > > > create mode 100644 drivers/gpio/gpio-da9062.c > > > > > create mode 100644 include/linux/mfd/da9062/gpio.h > > > > > > > > > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > > > > index bb13c266c329..b308ea549aaa 100644 > > > > > --- a/drivers/gpio/Kconfig > > > > > +++ b/drivers/gpio/Kconfig > > > > > @@ -1038,6 +1038,17 @@ config GPIO_DA9055 > > > > > > > > > > If driver is built as a module it will be called gpio-da9055. > > > > > > > > > > +config GPIO_DA9062 > > > > > + tristate "Dialog Semiconductor DA9062 GPIO" > > > > > + depends on MFD_DA9062 > > > > > + help > > > > > + Say yes here to enable the GPIO driver for the DA9062 chip. > > > > > + > > > > > + The Dialog DA9062 PMIC chip has 5 GPIO pins that can be > > > > > + be controller by this driver. > > > > > + > > > > > + If driver is built as a module it will be called gpio-da9062. > > > > > + > > > > > config GPIO_DLN2 > > > > > tristate "Diolan DLN2 GPIO support" > > > > > depends on MFD_DLN2 > > > > > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > > > > > index a4e91175c708..f29c8af2d096 100644 > > > > > --- a/drivers/gpio/Makefile > > > > > +++ b/drivers/gpio/Makefile > > > > > @@ -45,6 +45,7 @@ obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o > > > > > obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o > > > > > obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o > > > > > obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o > > > > > +obj-$(CONFIG_GPIO_DA9062) += gpio-da9062.o > > > > > obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o > > > > > obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o > > > > > obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o > > > > > diff --git a/drivers/gpio/gpio-da9062.c b/drivers/gpio/gpio-da9062.c > > > > > new file mode 100644 > > > > > index 000000000000..6035963929a2 > > > > > --- /dev/null > > > > > +++ b/drivers/gpio/gpio-da9062.c > > > > > @@ -0,0 +1,265 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > +/* > > > > > + * GPIO Driver for Dialog DA9062 PMICs. > > > > > + * Based on DA9055 GPIO driver. > > > > > + * > > > > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > > > > + */ > > > > > +#include <linux/module.h> > > > > > +#include <linux/platform_device.h> > > > > > +#include <linux/regmap.h> > > > > > + > > > > > +#include <linux/gpio/driver.h> > > > > > +#include <linux/gpio/consumer.h> > > > > > + > > > > > +#include <linux/mfd/da9062/core.h> > > > > > +#include <linux/mfd/da9062/registers.h> > > > > > + > > > > > +#include "gpiolib.h" > > > > > + > > > > > +#define DA9062_TYPE(offset) (4 * (offset % 2)) > > > > > +#define DA9062_PIN_SHIFT(offset) (4 * (offset % 2)) > > > > > +#define DA9062_PIN_ALTERNATE 0x00 /* gpio alternate mode */ > > > > > +#define DA9062_PIN_GPI 0x01 /* gpio in */ > > > > > +#define DA9062_PIN_GPO_OD 0x02 /* gpio out open-drain */ > > > > > +#define DA9062_PIN_GPO_PP 0x03 /* gpio out push-pull */ > > > > > +#define DA9062_GPIO_NUM 5 > > > > > + > > > > > +struct da9062_gpio { > > > > > + struct da9062 *da9062; > > > > > + struct gpio_chip gp; > > > > > +}; > > > > > + > > > > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc) > > > > > +{ > > > > > + return gpio_chip_hwgpio(desc); > > > > > +} > > > > > +EXPORT_SYMBOL_GPL(da9062_gpio_get_hwgpio); > > > > > + > > > > > > > > Is this going to be used anywhere? I'm not really a fan of adding new > > > > APIs without users. > > > > > > Yes, it is used here: https://lkml.org/lkml/2019/9/17/411 > > > > > > I don't know if I should add the gpio here or as seperate patch within > > > the above series. > > > > > > > > +static int da9062_gpio_get_pin_mode(struct regmap *regmap, unsigned int offset) > > > > > +{ > > > > > + int ret; > > > > > + int val; > > > > > > > > Nit: maybe put these two in a single line? > > > > > > Yes. > > > > > > > > + > > > > > + ret = regmap_read(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), &val); > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + > > > > > + val >>= DA9062_PIN_SHIFT(offset); > > > > > + val &= DA9062AA_GPIO0_PIN_MASK; > > > > > + > > > > > + return val; > > > > > +} > > > > > + > > > > > +static int da9062_gpio_set_pin_mode(struct regmap *regmap, unsigned int offset, > > > > > + unsigned int mode) > > > > > +{ > > > > > + unsigned int mask; > > > > > + > > > > > + mode &= DA9062AA_GPIO0_PIN_MASK; > > > > > + mode <<= DA9062_PIN_SHIFT(offset); > > > > > + mask = DA9062AA_GPIO0_PIN_MASK << DA9062_PIN_SHIFT(offset); > > > > > + > > > > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > > > > + mask, mode); > > > > > +} > > > > > + > > > > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > > > > > +{ > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > + int gpio_dir, val; > > > > > + int ret; > > > > > + > > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > > + if (gpio_dir < 0) > > > > > + return gpio_dir; > > > > > + > > > > > + switch (gpio_dir) { > > > > > + case DA9062_PIN_ALTERNATE: > > > > > + return -ENOTSUPP; > > > > > + case DA9062_PIN_GPI: > > > > > + ret = regmap_read(regmap, DA9062AA_STATUS_B, &val); > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + break; > > > > > + case DA9062_PIN_GPO_OD: > > > > > + /* falltrough */ > > > > > + case DA9062_PIN_GPO_PP: > > > > > + ret = regmap_read(regmap, DA9062AA_GPIO_MODE0_4, &val); > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + } > > > > > + > > > > > + return val & BIT(offset); > > > > > +} > > > > > + > > > > > +static void da9062_gpio_set(struct gpio_chip *gc, unsigned int offset, > > > > > + int value) > > > > > +{ > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > + > > > > > + regmap_update_bits(regmap, DA9062AA_GPIO_MODE0_4, BIT(offset), > > > > > + value << offset); > > > > > +} > > > > > + > > > > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > > > > > +{ > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > + int gpio_dir; > > > > > + > > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > > + if (gpio_dir < 0) > > > > > + return gpio_dir; > > > > > + > > > > > + switch (gpio_dir) { > > > > > + case DA9062_PIN_ALTERNATE: > > > > > + return -ENOTSUPP; > > > > > + case DA9062_PIN_GPI: > > > > > + return 1; > > > > > + case DA9062_PIN_GPO_OD: > > > > > + /* falltrough */ > > > > > + case DA9062_PIN_GPO_PP: > > > > > + return 0; > > > > > + } > > > > > + > > > > > + return -EINVAL; > > > > > +} > > > > > + > > > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > > > > + unsigned int offset) > > > > > +{ > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > > > > + unsigned int gpi_type; > > > > > + int ret; > > > > > + > > > > > + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + /* > > > > > + * If the gpio is active low we should set it in hw too. No worries > > > > > + * about gpio_get() because we read and return the gpio-level. So the > > > > > + * gpiolob active_low handling is still correct. > > > > > + * > > > > > + * 0 - active low, 1 - active high > > > > > + */ > > > > > + gpi_type = !gpiod_is_active_low(desc); > > > > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > > > > + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), > > > > > + gpi_type << DA9062_TYPE(offset)); > > > > > +} > > > > > + > > > > > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > > > > > + unsigned int offset, int value) > > > > > +{ > > > > > + /* Push-Pull / Open-Drain options are configured during set_config */ > > > > > + da9062_gpio_set(gc, offset, value); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > > > > > + unsigned long config) > > > > > +{ > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > + int gpio_dir; > > > > > + > > > > > + switch (pinconf_to_config_param(config)) { > > > > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > > > > + /* PD only if pin is input */ > > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > > + if (gpio_dir < 0) > > > > > + return -EINVAL; > > > > > + else if (gpio_dir != DA9062_PIN_GPI) > > > > > + return -ENOTSUPP; > > > > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > > > > + BIT(offset), BIT(offset)); > > > > > + case PIN_CONFIG_BIAS_PULL_UP: > > > > > + /* PU only if pin is output open-drain */ > > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > > + if (gpio_dir < 0) > > > > > + return -EINVAL; > > > > > + else if (gpio_dir != DA9062_PIN_GPO_OD) > > > > > + return -ENOTSUPP; > > > > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > > > > + BIT(offset), BIT(offset)); > > > > > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > > > > + return da9062_gpio_set_pin_mode(regmap, offset, > > > > > + DA9062_PIN_GPO_OD); > > > > > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > > > > + return da9062_gpio_set_pin_mode(regmap, offset, > > > > > + DA9062_PIN_GPO_PP); > > > > > + default: > > > > > + return -ENOTSUPP; > > > > > + } > > > > > +} > > > > > + > > > > > +static int da9062_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > > > > > +{ > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > + struct da9062 *da9062 = gpio->da9062; > > > > > + > > > > > + return regmap_irq_get_virq(da9062->regmap_irq, > > > > > + DA9062_IRQ_GPI0 + offset); > > > > > +} > > > > > + > > > > > > > > I'm afraid this won't fly anymore. We now have support for > > > > hierarchical GPIO irqchips (take a look at > > > > Documentation/driver-api/gpio/driver.rst) and Linus is quite strict on > > > > enforcing its usage. What I did with a very simple mfd device with > > > > GPIOs (where using hierarchical irqchips was clearly overkill) was to > > > > put the gpio-controller on the same DT node as the core mfd device - > > > > that way there's no need for a hierarchy. > > > > > > Okay, I've checked the documentation and the code. If I understood it > > > right I should request each irq using platform_get_irq_byname() as you > > > did for the max77650? > > > > > > > No, regmap irq domain is fine, as long as you modify the DT bindings > > to not use a sub-node for the gpio-controller. > > Ah okay.. thanks for clearing that. While implementing your suggestion I found a possible bug within the max77650-gpio driver: 8<----------------------------------------------------- diff --git a/drivers/gpio/gpio-max77650.c b/drivers/gpio/gpio-max77650.c index 3f03f4e8956c..1c25c55e7818 100644 --- a/drivers/gpio/gpio-max77650.c +++ b/drivers/gpio/gpio-max77650.c @@ -174,6 +174,10 @@ static int max77650_gpio_probe(struct platform_device *pdev) chip->gc.set_config = max77650_gpio_set_config; chip->gc.to_irq = max77650_gpio_to_irq; +#ifdef CONFIG_OF_GPIO + chip->gc.of_node = parent->of_node; +#endif + return devm_gpiochip_add_data(dev, &chip->gc, chip); } 8<----------------------------------------------------- If I understood it right the max77650-gpio won't work without this fix. Regards, Marco > Regards, > Marco > > > > Regards, > > > Marco > > > > > > > Bart > > > > > > > > > +static const struct gpio_chip reference_gp = { > > > > > + .label = "da9062-gpio", > > > > > + .owner = THIS_MODULE, > > > > > + .get = da9062_gpio_get, > > > > > + .set = da9062_gpio_set, > > > > > + .get_direction = da9062_gpio_get_direction, > > > > > + .direction_input = da9062_gpio_direction_input, > > > > > + .direction_output = da9062_gpio_direction_output, > > > > > + .set_config = da9062_gpio_set_config, > > > > > + .to_irq = da9062_gpio_to_irq, > > > > > + .can_sleep = true, > > > > > + .ngpio = DA9062_GPIO_NUM, > > > > > + .base = -1, > > > > > +}; > > > > > + > > > > > +static int da9062_gpio_probe(struct platform_device *pdev) > > > > > +{ > > > > > + struct da9062_gpio *gpio; > > > > > + > > > > > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); > > > > > + if (!gpio) > > > > > + return -ENOMEM; > > > > > + > > > > > + gpio->da9062 = dev_get_drvdata(pdev->dev.parent); > > > > > + if (!gpio->da9062) > > > > > + return -EINVAL; > > > > > + > > > > > + gpio->gp = reference_gp; > > > > > + gpio->gp.parent = &pdev->dev; > > > > > + > > > > > + platform_set_drvdata(pdev, gpio); > > > > > + > > > > > + return devm_gpiochip_add_data(&pdev->dev, &gpio->gp, gpio); > > > > > +} > > > > > + > > > > > +static const struct of_device_id da9062_compatible_id_table[] = { > > > > > + { .compatible = "dlg,da9062-gpio" }, > > > > > + { }, > > > > > +}; > > > > > +MODULE_DEVICE_TABLE(of, da9062_compatible_id_table); > > > > > + > > > > > +static struct platform_driver da9062_gpio_driver = { > > > > > + .probe = da9062_gpio_probe, > > > > > + .driver = { > > > > > + .name = "da9062-gpio", > > > > > + .of_match_table = da9062_compatible_id_table, > > > > > + }, > > > > > +}; > > > > > +module_platform_driver(da9062_gpio_driver); > > > > > + > > > > > +MODULE_AUTHOR("Marco Felsch <kernel@pengutronix.de>"); > > > > > +MODULE_DESCRIPTION("DA9062 GPIO Device Driver"); > > > > > +MODULE_LICENSE("GPL v2"); > > > > > +MODULE_ALIAS("platform:da9062-gpio"); > > > > > diff --git a/include/linux/mfd/da9062/gpio.h b/include/linux/mfd/da9062/gpio.h > > > > > new file mode 100644 > > > > > index 000000000000..67627ada1ad4 > > > > > --- /dev/null > > > > > +++ b/include/linux/mfd/da9062/gpio.h > > > > > @@ -0,0 +1,13 @@ > > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > > +/* > > > > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > > > > + */ > > > > > + > > > > > +#ifndef __MFD_DA9062_GPIO_H__ > > > > > +#define __MFD_DA9062_GPIO_H__ > > > > > + > > > > > +struct gpio_desc; > > > > > + > > > > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc); > > > > > + > > > > > +#endif /* __MFD_DA9062_GPIO_H__ */ > > > > > -- > > > > > 2.20.1 > > > > > > > > > > > > > > > -- > > > Pengutronix e.K. | | > > > Industrial Linux Solutions | http://www.pengutronix.de/ | > > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > >
pon., 30 wrz 2019 o 20:42 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > Hi Bartosz, > > On 19-09-19 10:38, Marco Felsch wrote: > > On 19-09-19 10:24, Bartosz Golaszewski wrote: > > > śr., 18 wrz 2019 o 14:06 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > > > > > On 19-09-18 09:04, Bartosz Golaszewski wrote: > > > > > wt., 17 wrz 2019 o 12:59 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > > > > > > > > > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > > > > > > be used as input, output or have a special use-case. > > > > > > > > > > > > The patch adds the support for the normal input/output use-case. > > > > > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > > > --- > > > > > > drivers/gpio/Kconfig | 11 ++ > > > > > > drivers/gpio/Makefile | 1 + > > > > > > drivers/gpio/gpio-da9062.c | 265 ++++++++++++++++++++++++++++++++ > > > > > > include/linux/mfd/da9062/gpio.h | 13 ++ > > > > > > 4 files changed, 290 insertions(+) > > > > > > create mode 100644 drivers/gpio/gpio-da9062.c > > > > > > create mode 100644 include/linux/mfd/da9062/gpio.h > > > > > > > > > > > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > > > > > index bb13c266c329..b308ea549aaa 100644 > > > > > > --- a/drivers/gpio/Kconfig > > > > > > +++ b/drivers/gpio/Kconfig > > > > > > @@ -1038,6 +1038,17 @@ config GPIO_DA9055 > > > > > > > > > > > > If driver is built as a module it will be called gpio-da9055. > > > > > > > > > > > > +config GPIO_DA9062 > > > > > > + tristate "Dialog Semiconductor DA9062 GPIO" > > > > > > + depends on MFD_DA9062 > > > > > > + help > > > > > > + Say yes here to enable the GPIO driver for the DA9062 chip. > > > > > > + > > > > > > + The Dialog DA9062 PMIC chip has 5 GPIO pins that can be > > > > > > + be controller by this driver. > > > > > > + > > > > > > + If driver is built as a module it will be called gpio-da9062. > > > > > > + > > > > > > config GPIO_DLN2 > > > > > > tristate "Diolan DLN2 GPIO support" > > > > > > depends on MFD_DLN2 > > > > > > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > > > > > > index a4e91175c708..f29c8af2d096 100644 > > > > > > --- a/drivers/gpio/Makefile > > > > > > +++ b/drivers/gpio/Makefile > > > > > > @@ -45,6 +45,7 @@ obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o > > > > > > obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o > > > > > > obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o > > > > > > obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o > > > > > > +obj-$(CONFIG_GPIO_DA9062) += gpio-da9062.o > > > > > > obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o > > > > > > obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o > > > > > > obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o > > > > > > diff --git a/drivers/gpio/gpio-da9062.c b/drivers/gpio/gpio-da9062.c > > > > > > new file mode 100644 > > > > > > index 000000000000..6035963929a2 > > > > > > --- /dev/null > > > > > > +++ b/drivers/gpio/gpio-da9062.c > > > > > > @@ -0,0 +1,265 @@ > > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > > +/* > > > > > > + * GPIO Driver for Dialog DA9062 PMICs. > > > > > > + * Based on DA9055 GPIO driver. > > > > > > + * > > > > > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > > > > > + */ > > > > > > +#include <linux/module.h> > > > > > > +#include <linux/platform_device.h> > > > > > > +#include <linux/regmap.h> > > > > > > + > > > > > > +#include <linux/gpio/driver.h> > > > > > > +#include <linux/gpio/consumer.h> > > > > > > + > > > > > > +#include <linux/mfd/da9062/core.h> > > > > > > +#include <linux/mfd/da9062/registers.h> > > > > > > + > > > > > > +#include "gpiolib.h" > > > > > > + > > > > > > +#define DA9062_TYPE(offset) (4 * (offset % 2)) > > > > > > +#define DA9062_PIN_SHIFT(offset) (4 * (offset % 2)) > > > > > > +#define DA9062_PIN_ALTERNATE 0x00 /* gpio alternate mode */ > > > > > > +#define DA9062_PIN_GPI 0x01 /* gpio in */ > > > > > > +#define DA9062_PIN_GPO_OD 0x02 /* gpio out open-drain */ > > > > > > +#define DA9062_PIN_GPO_PP 0x03 /* gpio out push-pull */ > > > > > > +#define DA9062_GPIO_NUM 5 > > > > > > + > > > > > > +struct da9062_gpio { > > > > > > + struct da9062 *da9062; > > > > > > + struct gpio_chip gp; > > > > > > +}; > > > > > > + > > > > > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc) > > > > > > +{ > > > > > > + return gpio_chip_hwgpio(desc); > > > > > > +} > > > > > > +EXPORT_SYMBOL_GPL(da9062_gpio_get_hwgpio); > > > > > > + > > > > > > > > > > Is this going to be used anywhere? I'm not really a fan of adding new > > > > > APIs without users. > > > > > > > > Yes, it is used here: https://lkml.org/lkml/2019/9/17/411 > > > > > > > > I don't know if I should add the gpio here or as seperate patch within > > > > the above series. > > > > > > > > > > +static int da9062_gpio_get_pin_mode(struct regmap *regmap, unsigned int offset) > > > > > > +{ > > > > > > + int ret; > > > > > > + int val; > > > > > > > > > > Nit: maybe put these two in a single line? > > > > > > > > Yes. > > > > > > > > > > + > > > > > > + ret = regmap_read(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), &val); > > > > > > + if (ret < 0) > > > > > > + return ret; > > > > > > + > > > > > > + val >>= DA9062_PIN_SHIFT(offset); > > > > > > + val &= DA9062AA_GPIO0_PIN_MASK; > > > > > > + > > > > > > + return val; > > > > > > +} > > > > > > + > > > > > > +static int da9062_gpio_set_pin_mode(struct regmap *regmap, unsigned int offset, > > > > > > + unsigned int mode) > > > > > > +{ > > > > > > + unsigned int mask; > > > > > > + > > > > > > + mode &= DA9062AA_GPIO0_PIN_MASK; > > > > > > + mode <<= DA9062_PIN_SHIFT(offset); > > > > > > + mask = DA9062AA_GPIO0_PIN_MASK << DA9062_PIN_SHIFT(offset); > > > > > > + > > > > > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > > > > > + mask, mode); > > > > > > +} > > > > > > + > > > > > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > > > > > > +{ > > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > > + int gpio_dir, val; > > > > > > + int ret; > > > > > > + > > > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > > > + if (gpio_dir < 0) > > > > > > + return gpio_dir; > > > > > > + > > > > > > + switch (gpio_dir) { > > > > > > + case DA9062_PIN_ALTERNATE: > > > > > > + return -ENOTSUPP; > > > > > > + case DA9062_PIN_GPI: > > > > > > + ret = regmap_read(regmap, DA9062AA_STATUS_B, &val); > > > > > > + if (ret < 0) > > > > > > + return ret; > > > > > > + break; > > > > > > + case DA9062_PIN_GPO_OD: > > > > > > + /* falltrough */ > > > > > > + case DA9062_PIN_GPO_PP: > > > > > > + ret = regmap_read(regmap, DA9062AA_GPIO_MODE0_4, &val); > > > > > > + if (ret < 0) > > > > > > + return ret; > > > > > > + } > > > > > > + > > > > > > + return val & BIT(offset); > > > > > > +} > > > > > > + > > > > > > +static void da9062_gpio_set(struct gpio_chip *gc, unsigned int offset, > > > > > > + int value) > > > > > > +{ > > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > > + > > > > > > + regmap_update_bits(regmap, DA9062AA_GPIO_MODE0_4, BIT(offset), > > > > > > + value << offset); > > > > > > +} > > > > > > + > > > > > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > > > > > > +{ > > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > > + int gpio_dir; > > > > > > + > > > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > > > + if (gpio_dir < 0) > > > > > > + return gpio_dir; > > > > > > + > > > > > > + switch (gpio_dir) { > > > > > > + case DA9062_PIN_ALTERNATE: > > > > > > + return -ENOTSUPP; > > > > > > + case DA9062_PIN_GPI: > > > > > > + return 1; > > > > > > + case DA9062_PIN_GPO_OD: > > > > > > + /* falltrough */ > > > > > > + case DA9062_PIN_GPO_PP: > > > > > > + return 0; > > > > > > + } > > > > > > + > > > > > > + return -EINVAL; > > > > > > +} > > > > > > + > > > > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > > > > > + unsigned int offset) > > > > > > +{ > > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > > > > > + unsigned int gpi_type; > > > > > > + int ret; > > > > > > + > > > > > > + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + /* > > > > > > + * If the gpio is active low we should set it in hw too. No worries > > > > > > + * about gpio_get() because we read and return the gpio-level. So the > > > > > > + * gpiolob active_low handling is still correct. > > > > > > + * > > > > > > + * 0 - active low, 1 - active high > > > > > > + */ > > > > > > + gpi_type = !gpiod_is_active_low(desc); > > > > > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > > > > > + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), > > > > > > + gpi_type << DA9062_TYPE(offset)); > > > > > > +} > > > > > > + > > > > > > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > > > > > > + unsigned int offset, int value) > > > > > > +{ > > > > > > + /* Push-Pull / Open-Drain options are configured during set_config */ > > > > > > + da9062_gpio_set(gc, offset, value); > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > > > > > > + unsigned long config) > > > > > > +{ > > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > > + int gpio_dir; > > > > > > + > > > > > > + switch (pinconf_to_config_param(config)) { > > > > > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > > > > > + /* PD only if pin is input */ > > > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > > > + if (gpio_dir < 0) > > > > > > + return -EINVAL; > > > > > > + else if (gpio_dir != DA9062_PIN_GPI) > > > > > > + return -ENOTSUPP; > > > > > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > > > > > + BIT(offset), BIT(offset)); > > > > > > + case PIN_CONFIG_BIAS_PULL_UP: > > > > > > + /* PU only if pin is output open-drain */ > > > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > > > + if (gpio_dir < 0) > > > > > > + return -EINVAL; > > > > > > + else if (gpio_dir != DA9062_PIN_GPO_OD) > > > > > > + return -ENOTSUPP; > > > > > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > > > > > + BIT(offset), BIT(offset)); > > > > > > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > > > > > + return da9062_gpio_set_pin_mode(regmap, offset, > > > > > > + DA9062_PIN_GPO_OD); > > > > > > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > > > > > + return da9062_gpio_set_pin_mode(regmap, offset, > > > > > > + DA9062_PIN_GPO_PP); > > > > > > + default: > > > > > > + return -ENOTSUPP; > > > > > > + } > > > > > > +} > > > > > > + > > > > > > +static int da9062_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > > > > > > +{ > > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > > + struct da9062 *da9062 = gpio->da9062; > > > > > > + > > > > > > + return regmap_irq_get_virq(da9062->regmap_irq, > > > > > > + DA9062_IRQ_GPI0 + offset); > > > > > > +} > > > > > > + > > > > > > > > > > I'm afraid this won't fly anymore. We now have support for > > > > > hierarchical GPIO irqchips (take a look at > > > > > Documentation/driver-api/gpio/driver.rst) and Linus is quite strict on > > > > > enforcing its usage. What I did with a very simple mfd device with > > > > > GPIOs (where using hierarchical irqchips was clearly overkill) was to > > > > > put the gpio-controller on the same DT node as the core mfd device - > > > > > that way there's no need for a hierarchy. > > > > > > > > Okay, I've checked the documentation and the code. If I understood it > > > > right I should request each irq using platform_get_irq_byname() as you > > > > did for the max77650? > > > > > > > > > > No, regmap irq domain is fine, as long as you modify the DT bindings > > > to not use a sub-node for the gpio-controller. > > > > Ah okay.. thanks for clearing that. > > While implementing your suggestion I found a possible bug within the > max77650-gpio driver: > > 8<----------------------------------------------------- > > diff --git a/drivers/gpio/gpio-max77650.c b/drivers/gpio/gpio-max77650.c > index 3f03f4e8956c..1c25c55e7818 100644 > --- a/drivers/gpio/gpio-max77650.c > +++ b/drivers/gpio/gpio-max77650.c > @@ -174,6 +174,10 @@ static int max77650_gpio_probe(struct platform_device *pdev) > chip->gc.set_config = max77650_gpio_set_config; > chip->gc.to_irq = max77650_gpio_to_irq; > > +#ifdef CONFIG_OF_GPIO > + chip->gc.of_node = parent->of_node; > +#endif > + > return devm_gpiochip_add_data(dev, &chip->gc, chip); > } This does make sense (except that you don't need the ifdef guard), but the driver works, I have tested it on real HW. I'll take a look tomorrow, I don't have the board with me. Bart > > 8<----------------------------------------------------- > > If I understood it right the max77650-gpio won't work without this fix. > > Regards, > Marco > > > Regards, > > Marco > > > > > > Regards, > > > > Marco > > > > > > > > > Bart > > > > > > > > > > > +static const struct gpio_chip reference_gp = { > > > > > > + .label = "da9062-gpio", > > > > > > + .owner = THIS_MODULE, > > > > > > + .get = da9062_gpio_get, > > > > > > + .set = da9062_gpio_set, > > > > > > + .get_direction = da9062_gpio_get_direction, > > > > > > + .direction_input = da9062_gpio_direction_input, > > > > > > + .direction_output = da9062_gpio_direction_output, > > > > > > + .set_config = da9062_gpio_set_config, > > > > > > + .to_irq = da9062_gpio_to_irq, > > > > > > + .can_sleep = true, > > > > > > + .ngpio = DA9062_GPIO_NUM, > > > > > > + .base = -1, > > > > > > +}; > > > > > > + > > > > > > +static int da9062_gpio_probe(struct platform_device *pdev) > > > > > > +{ > > > > > > + struct da9062_gpio *gpio; > > > > > > + > > > > > > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); > > > > > > + if (!gpio) > > > > > > + return -ENOMEM; > > > > > > + > > > > > > + gpio->da9062 = dev_get_drvdata(pdev->dev.parent); > > > > > > + if (!gpio->da9062) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + gpio->gp = reference_gp; > > > > > > + gpio->gp.parent = &pdev->dev; > > > > > > + > > > > > > + platform_set_drvdata(pdev, gpio); > > > > > > + > > > > > > + return devm_gpiochip_add_data(&pdev->dev, &gpio->gp, gpio); > > > > > > +} > > > > > > + > > > > > > +static const struct of_device_id da9062_compatible_id_table[] = { > > > > > > + { .compatible = "dlg,da9062-gpio" }, > > > > > > + { }, > > > > > > +}; > > > > > > +MODULE_DEVICE_TABLE(of, da9062_compatible_id_table); > > > > > > + > > > > > > +static struct platform_driver da9062_gpio_driver = { > > > > > > + .probe = da9062_gpio_probe, > > > > > > + .driver = { > > > > > > + .name = "da9062-gpio", > > > > > > + .of_match_table = da9062_compatible_id_table, > > > > > > + }, > > > > > > +}; > > > > > > +module_platform_driver(da9062_gpio_driver); > > > > > > + > > > > > > +MODULE_AUTHOR("Marco Felsch <kernel@pengutronix.de>"); > > > > > > +MODULE_DESCRIPTION("DA9062 GPIO Device Driver"); > > > > > > +MODULE_LICENSE("GPL v2"); > > > > > > +MODULE_ALIAS("platform:da9062-gpio"); > > > > > > diff --git a/include/linux/mfd/da9062/gpio.h b/include/linux/mfd/da9062/gpio.h > > > > > > new file mode 100644 > > > > > > index 000000000000..67627ada1ad4 > > > > > > --- /dev/null > > > > > > +++ b/include/linux/mfd/da9062/gpio.h > > > > > > @@ -0,0 +1,13 @@ > > > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > > > +/* > > > > > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > > > > > + */ > > > > > > + > > > > > > +#ifndef __MFD_DA9062_GPIO_H__ > > > > > > +#define __MFD_DA9062_GPIO_H__ > > > > > > + > > > > > > +struct gpio_desc; > > > > > > + > > > > > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc); > > > > > > + > > > > > > +#endif /* __MFD_DA9062_GPIO_H__ */ > > > > > > -- > > > > > > 2.20.1 > > > > > > > > > > > > > > > > > > > -- > > > > Pengutronix e.K. | | > > > > Industrial Linux Solutions | http://www.pengutronix.de/ | > > > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > > > > > > -- > > Pengutronix e.K. | | > > Industrial Linux Solutions | http://www.pengutronix.de/ | > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > > > > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
czw., 3 paź 2019 o 10:12 Bartosz Golaszewski <bgolaszewski@baylibre.com> napisał(a): > > pon., 30 wrz 2019 o 20:42 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > Hi Bartosz, > > > > On 19-09-19 10:38, Marco Felsch wrote: > > > On 19-09-19 10:24, Bartosz Golaszewski wrote: > > > > śr., 18 wrz 2019 o 14:06 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > > > > > > > On 19-09-18 09:04, Bartosz Golaszewski wrote: > > > > > > wt., 17 wrz 2019 o 12:59 Marco Felsch <m.felsch@pengutronix.de> napisał(a): > > > > > > > > > > > > > > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > > > > > > > be used as input, output or have a special use-case. > > > > > > > > > > > > > > The patch adds the support for the normal input/output use-case. > > > > > > > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > > > > --- > > > > > > > drivers/gpio/Kconfig | 11 ++ > > > > > > > drivers/gpio/Makefile | 1 + > > > > > > > drivers/gpio/gpio-da9062.c | 265 ++++++++++++++++++++++++++++++++ > > > > > > > include/linux/mfd/da9062/gpio.h | 13 ++ > > > > > > > 4 files changed, 290 insertions(+) > > > > > > > create mode 100644 drivers/gpio/gpio-da9062.c > > > > > > > create mode 100644 include/linux/mfd/da9062/gpio.h > > > > > > > > > > > > > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > > > > > > index bb13c266c329..b308ea549aaa 100644 > > > > > > > --- a/drivers/gpio/Kconfig > > > > > > > +++ b/drivers/gpio/Kconfig > > > > > > > @@ -1038,6 +1038,17 @@ config GPIO_DA9055 > > > > > > > > > > > > > > If driver is built as a module it will be called gpio-da9055. > > > > > > > > > > > > > > +config GPIO_DA9062 > > > > > > > + tristate "Dialog Semiconductor DA9062 GPIO" > > > > > > > + depends on MFD_DA9062 > > > > > > > + help > > > > > > > + Say yes here to enable the GPIO driver for the DA9062 chip. > > > > > > > + > > > > > > > + The Dialog DA9062 PMIC chip has 5 GPIO pins that can be > > > > > > > + be controller by this driver. > > > > > > > + > > > > > > > + If driver is built as a module it will be called gpio-da9062. > > > > > > > + > > > > > > > config GPIO_DLN2 > > > > > > > tristate "Diolan DLN2 GPIO support" > > > > > > > depends on MFD_DLN2 > > > > > > > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > > > > > > > index a4e91175c708..f29c8af2d096 100644 > > > > > > > --- a/drivers/gpio/Makefile > > > > > > > +++ b/drivers/gpio/Makefile > > > > > > > @@ -45,6 +45,7 @@ obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o > > > > > > > obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o > > > > > > > obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o > > > > > > > obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o > > > > > > > +obj-$(CONFIG_GPIO_DA9062) += gpio-da9062.o > > > > > > > obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o > > > > > > > obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o > > > > > > > obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o > > > > > > > diff --git a/drivers/gpio/gpio-da9062.c b/drivers/gpio/gpio-da9062.c > > > > > > > new file mode 100644 > > > > > > > index 000000000000..6035963929a2 > > > > > > > --- /dev/null > > > > > > > +++ b/drivers/gpio/gpio-da9062.c > > > > > > > @@ -0,0 +1,265 @@ > > > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > > > +/* > > > > > > > + * GPIO Driver for Dialog DA9062 PMICs. > > > > > > > + * Based on DA9055 GPIO driver. > > > > > > > + * > > > > > > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > > > > > > + */ > > > > > > > +#include <linux/module.h> > > > > > > > +#include <linux/platform_device.h> > > > > > > > +#include <linux/regmap.h> > > > > > > > + > > > > > > > +#include <linux/gpio/driver.h> > > > > > > > +#include <linux/gpio/consumer.h> > > > > > > > + > > > > > > > +#include <linux/mfd/da9062/core.h> > > > > > > > +#include <linux/mfd/da9062/registers.h> > > > > > > > + > > > > > > > +#include "gpiolib.h" > > > > > > > + > > > > > > > +#define DA9062_TYPE(offset) (4 * (offset % 2)) > > > > > > > +#define DA9062_PIN_SHIFT(offset) (4 * (offset % 2)) > > > > > > > +#define DA9062_PIN_ALTERNATE 0x00 /* gpio alternate mode */ > > > > > > > +#define DA9062_PIN_GPI 0x01 /* gpio in */ > > > > > > > +#define DA9062_PIN_GPO_OD 0x02 /* gpio out open-drain */ > > > > > > > +#define DA9062_PIN_GPO_PP 0x03 /* gpio out push-pull */ > > > > > > > +#define DA9062_GPIO_NUM 5 > > > > > > > + > > > > > > > +struct da9062_gpio { > > > > > > > + struct da9062 *da9062; > > > > > > > + struct gpio_chip gp; > > > > > > > +}; > > > > > > > + > > > > > > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc) > > > > > > > +{ > > > > > > > + return gpio_chip_hwgpio(desc); > > > > > > > +} > > > > > > > +EXPORT_SYMBOL_GPL(da9062_gpio_get_hwgpio); > > > > > > > + > > > > > > > > > > > > Is this going to be used anywhere? I'm not really a fan of adding new > > > > > > APIs without users. > > > > > > > > > > Yes, it is used here: https://lkml.org/lkml/2019/9/17/411 > > > > > > > > > > I don't know if I should add the gpio here or as seperate patch within > > > > > the above series. > > > > > > > > > > > > +static int da9062_gpio_get_pin_mode(struct regmap *regmap, unsigned int offset) > > > > > > > +{ > > > > > > > + int ret; > > > > > > > + int val; > > > > > > > > > > > > Nit: maybe put these two in a single line? > > > > > > > > > > Yes. > > > > > > > > > > > > + > > > > > > > + ret = regmap_read(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), &val); > > > > > > > + if (ret < 0) > > > > > > > + return ret; > > > > > > > + > > > > > > > + val >>= DA9062_PIN_SHIFT(offset); > > > > > > > + val &= DA9062AA_GPIO0_PIN_MASK; > > > > > > > + > > > > > > > + return val; > > > > > > > +} > > > > > > > + > > > > > > > +static int da9062_gpio_set_pin_mode(struct regmap *regmap, unsigned int offset, > > > > > > > + unsigned int mode) > > > > > > > +{ > > > > > > > + unsigned int mask; > > > > > > > + > > > > > > > + mode &= DA9062AA_GPIO0_PIN_MASK; > > > > > > > + mode <<= DA9062_PIN_SHIFT(offset); > > > > > > > + mask = DA9062AA_GPIO0_PIN_MASK << DA9062_PIN_SHIFT(offset); > > > > > > > + > > > > > > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > > > > > > + mask, mode); > > > > > > > +} > > > > > > > + > > > > > > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > > > > > > > +{ > > > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > > > + int gpio_dir, val; > > > > > > > + int ret; > > > > > > > + > > > > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > > > > + if (gpio_dir < 0) > > > > > > > + return gpio_dir; > > > > > > > + > > > > > > > + switch (gpio_dir) { > > > > > > > + case DA9062_PIN_ALTERNATE: > > > > > > > + return -ENOTSUPP; > > > > > > > + case DA9062_PIN_GPI: > > > > > > > + ret = regmap_read(regmap, DA9062AA_STATUS_B, &val); > > > > > > > + if (ret < 0) > > > > > > > + return ret; > > > > > > > + break; > > > > > > > + case DA9062_PIN_GPO_OD: > > > > > > > + /* falltrough */ > > > > > > > + case DA9062_PIN_GPO_PP: > > > > > > > + ret = regmap_read(regmap, DA9062AA_GPIO_MODE0_4, &val); > > > > > > > + if (ret < 0) > > > > > > > + return ret; > > > > > > > + } > > > > > > > + > > > > > > > + return val & BIT(offset); > > > > > > > +} > > > > > > > + > > > > > > > +static void da9062_gpio_set(struct gpio_chip *gc, unsigned int offset, > > > > > > > + int value) > > > > > > > +{ > > > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > > > + > > > > > > > + regmap_update_bits(regmap, DA9062AA_GPIO_MODE0_4, BIT(offset), > > > > > > > + value << offset); > > > > > > > +} > > > > > > > + > > > > > > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > > > > > > > +{ > > > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > > > + int gpio_dir; > > > > > > > + > > > > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > > > > + if (gpio_dir < 0) > > > > > > > + return gpio_dir; > > > > > > > + > > > > > > > + switch (gpio_dir) { > > > > > > > + case DA9062_PIN_ALTERNATE: > > > > > > > + return -ENOTSUPP; > > > > > > > + case DA9062_PIN_GPI: > > > > > > > + return 1; > > > > > > > + case DA9062_PIN_GPO_OD: > > > > > > > + /* falltrough */ > > > > > > > + case DA9062_PIN_GPO_PP: > > > > > > > + return 0; > > > > > > > + } > > > > > > > + > > > > > > > + return -EINVAL; > > > > > > > +} > > > > > > > + > > > > > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > > > > > > + unsigned int offset) > > > > > > > +{ > > > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > > > > > > + unsigned int gpi_type; > > > > > > > + int ret; > > > > > > > + > > > > > > > + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > + > > > > > > > + /* > > > > > > > + * If the gpio is active low we should set it in hw too. No worries > > > > > > > + * about gpio_get() because we read and return the gpio-level. So the > > > > > > > + * gpiolob active_low handling is still correct. > > > > > > > + * > > > > > > > + * 0 - active low, 1 - active high > > > > > > > + */ > > > > > > > + gpi_type = !gpiod_is_active_low(desc); > > > > > > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > > > > > > + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), > > > > > > > + gpi_type << DA9062_TYPE(offset)); > > > > > > > +} > > > > > > > + > > > > > > > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > > > > > > > + unsigned int offset, int value) > > > > > > > +{ > > > > > > > + /* Push-Pull / Open-Drain options are configured during set_config */ > > > > > > > + da9062_gpio_set(gc, offset, value); > > > > > > > + > > > > > > > + return 0; > > > > > > > +} > > > > > > > + > > > > > > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > > > > > > > + unsigned long config) > > > > > > > +{ > > > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > > > + int gpio_dir; > > > > > > > + > > > > > > > + switch (pinconf_to_config_param(config)) { > > > > > > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > > > > > > + /* PD only if pin is input */ > > > > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > > > > + if (gpio_dir < 0) > > > > > > > + return -EINVAL; > > > > > > > + else if (gpio_dir != DA9062_PIN_GPI) > > > > > > > + return -ENOTSUPP; > > > > > > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > > > > > > + BIT(offset), BIT(offset)); > > > > > > > + case PIN_CONFIG_BIAS_PULL_UP: > > > > > > > + /* PU only if pin is output open-drain */ > > > > > > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > > > > > > + if (gpio_dir < 0) > > > > > > > + return -EINVAL; > > > > > > > + else if (gpio_dir != DA9062_PIN_GPO_OD) > > > > > > > + return -ENOTSUPP; > > > > > > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > > > > > > + BIT(offset), BIT(offset)); > > > > > > > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > > > > > > + return da9062_gpio_set_pin_mode(regmap, offset, > > > > > > > + DA9062_PIN_GPO_OD); > > > > > > > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > > > > > > + return da9062_gpio_set_pin_mode(regmap, offset, > > > > > > > + DA9062_PIN_GPO_PP); > > > > > > > + default: > > > > > > > + return -ENOTSUPP; > > > > > > > + } > > > > > > > +} > > > > > > > + > > > > > > > +static int da9062_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > > > > > > > +{ > > > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > > > + struct da9062 *da9062 = gpio->da9062; > > > > > > > + > > > > > > > + return regmap_irq_get_virq(da9062->regmap_irq, > > > > > > > + DA9062_IRQ_GPI0 + offset); > > > > > > > +} > > > > > > > + > > > > > > > > > > > > I'm afraid this won't fly anymore. We now have support for > > > > > > hierarchical GPIO irqchips (take a look at > > > > > > Documentation/driver-api/gpio/driver.rst) and Linus is quite strict on > > > > > > enforcing its usage. What I did with a very simple mfd device with > > > > > > GPIOs (where using hierarchical irqchips was clearly overkill) was to > > > > > > put the gpio-controller on the same DT node as the core mfd device - > > > > > > that way there's no need for a hierarchy. > > > > > > > > > > Okay, I've checked the documentation and the code. If I understood it > > > > > right I should request each irq using platform_get_irq_byname() as you > > > > > did for the max77650? > > > > > > > > > > > > > No, regmap irq domain is fine, as long as you modify the DT bindings > > > > to not use a sub-node for the gpio-controller. > > > > > > Ah okay.. thanks for clearing that. > > > > While implementing your suggestion I found a possible bug within the > > max77650-gpio driver: > > > > 8<----------------------------------------------------- > > > > diff --git a/drivers/gpio/gpio-max77650.c b/drivers/gpio/gpio-max77650.c > > index 3f03f4e8956c..1c25c55e7818 100644 > > --- a/drivers/gpio/gpio-max77650.c > > +++ b/drivers/gpio/gpio-max77650.c > > @@ -174,6 +174,10 @@ static int max77650_gpio_probe(struct platform_device *pdev) > > chip->gc.set_config = max77650_gpio_set_config; > > chip->gc.to_irq = max77650_gpio_to_irq; > > > > +#ifdef CONFIG_OF_GPIO > > + chip->gc.of_node = parent->of_node; > > +#endif > > + > > return devm_gpiochip_add_data(dev, &chip->gc, chip); > > } > > This does make sense (except that you don't need the ifdef guard), but > the driver works, I have tested it on real HW. I'll take a look > tomorrow, I don't have the board with me. > Ok, so I checked it and if the chip has a parent assigned, the gpiolib core will take its of_node and assign it to gdev, so all looks fine to me. Bart > Bart > > > > > 8<----------------------------------------------------- > > > > If I understood it right the max77650-gpio won't work without this fix. > > > > Regards, > > Marco > > > > > Regards, > > > Marco > > > > > > > > Regards, > > > > > Marco > > > > > > > > > > > Bart > > > > > > > > > > > > > +static const struct gpio_chip reference_gp = { > > > > > > > + .label = "da9062-gpio", > > > > > > > + .owner = THIS_MODULE, > > > > > > > + .get = da9062_gpio_get, > > > > > > > + .set = da9062_gpio_set, > > > > > > > + .get_direction = da9062_gpio_get_direction, > > > > > > > + .direction_input = da9062_gpio_direction_input, > > > > > > > + .direction_output = da9062_gpio_direction_output, > > > > > > > + .set_config = da9062_gpio_set_config, > > > > > > > + .to_irq = da9062_gpio_to_irq, > > > > > > > + .can_sleep = true, > > > > > > > + .ngpio = DA9062_GPIO_NUM, > > > > > > > + .base = -1, > > > > > > > +}; > > > > > > > + > > > > > > > +static int da9062_gpio_probe(struct platform_device *pdev) > > > > > > > +{ > > > > > > > + struct da9062_gpio *gpio; > > > > > > > + > > > > > > > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); > > > > > > > + if (!gpio) > > > > > > > + return -ENOMEM; > > > > > > > + > > > > > > > + gpio->da9062 = dev_get_drvdata(pdev->dev.parent); > > > > > > > + if (!gpio->da9062) > > > > > > > + return -EINVAL; > > > > > > > + > > > > > > > + gpio->gp = reference_gp; > > > > > > > + gpio->gp.parent = &pdev->dev; > > > > > > > + > > > > > > > + platform_set_drvdata(pdev, gpio); > > > > > > > + > > > > > > > + return devm_gpiochip_add_data(&pdev->dev, &gpio->gp, gpio); > > > > > > > +} > > > > > > > + > > > > > > > +static const struct of_device_id da9062_compatible_id_table[] = { > > > > > > > + { .compatible = "dlg,da9062-gpio" }, > > > > > > > + { }, > > > > > > > +}; > > > > > > > +MODULE_DEVICE_TABLE(of, da9062_compatible_id_table); > > > > > > > + > > > > > > > +static struct platform_driver da9062_gpio_driver = { > > > > > > > + .probe = da9062_gpio_probe, > > > > > > > + .driver = { > > > > > > > + .name = "da9062-gpio", > > > > > > > + .of_match_table = da9062_compatible_id_table, > > > > > > > + }, > > > > > > > +}; > > > > > > > +module_platform_driver(da9062_gpio_driver); > > > > > > > + > > > > > > > +MODULE_AUTHOR("Marco Felsch <kernel@pengutronix.de>"); > > > > > > > +MODULE_DESCRIPTION("DA9062 GPIO Device Driver"); > > > > > > > +MODULE_LICENSE("GPL v2"); > > > > > > > +MODULE_ALIAS("platform:da9062-gpio"); > > > > > > > diff --git a/include/linux/mfd/da9062/gpio.h b/include/linux/mfd/da9062/gpio.h > > > > > > > new file mode 100644 > > > > > > > index 000000000000..67627ada1ad4 > > > > > > > --- /dev/null > > > > > > > +++ b/include/linux/mfd/da9062/gpio.h > > > > > > > @@ -0,0 +1,13 @@ > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > > > > +/* > > > > > > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > > > > > > + */ > > > > > > > + > > > > > > > +#ifndef __MFD_DA9062_GPIO_H__ > > > > > > > +#define __MFD_DA9062_GPIO_H__ > > > > > > > + > > > > > > > +struct gpio_desc; > > > > > > > + > > > > > > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc); > > > > > > > + > > > > > > > +#endif /* __MFD_DA9062_GPIO_H__ */ > > > > > > > -- > > > > > > > 2.20.1 > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Pengutronix e.K. | | > > > > > Industrial Linux Solutions | http://www.pengutronix.de/ | > > > > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > > > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > > > > > > > > > -- > > > Pengutronix e.K. | | > > > Industrial Linux Solutions | http://www.pengutronix.de/ | > > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > > > > > > > > > -- > > Pengutronix e.K. | | > > Industrial Linux Solutions | http://www.pengutronix.de/ | > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, Sep 17, 2019 at 12:59 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > be used as input, output or have a special use-case. > > The patch adds the support for the normal input/output use-case. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> Nice to get this covered! > +#define DA9062_TYPE(offset) (4 * (offset % 2)) > +#define DA9062_PIN_SHIFT(offset) (4 * (offset % 2)) > +#define DA9062_PIN_ALTERNATE 0x00 /* gpio alternate mode */ The there is pin control related to the DA9062, we should put the driver in drivers/pinctrl/pinctrl-da9062.c from day one. I am not saying you have to implement pin control from scratch, just have it there and look at sibling drivers to make sure we don't screw something up for also adding pin control later on. > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc) > +{ > + return gpio_chip_hwgpio(desc); > +} > +EXPORT_SYMBOL_GPL(da9062_gpio_get_hwgpio); I would have to look at the other patch to see why this is needed. Normally other drivers should just have their GPIOs assigned like anyone else, no shortcuts into the hardware offsets like this. > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > +{ > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > + struct regmap *regmap = gpio->da9062->regmap; > + int gpio_dir, val; > + int ret; > + > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); This includes other things than the direction of the pin so call the variable gpio_mode or something rather than gpio_dir. > + if (gpio_dir < 0) > + return gpio_dir; > + > + switch (gpio_dir) { > + case DA9062_PIN_ALTERNATE: > + return -ENOTSUPP; This is fine for now. Toss in a comment that we may add pin muxing later. > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) Same comments as above. > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > + unsigned int offset) > +{ > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > + struct regmap *regmap = gpio->da9062->regmap; > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > + unsigned int gpi_type; > + int ret; > + > + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > + if (ret) > + return ret; Fair enough. > + /* > + * If the gpio is active low we should set it in hw too. No worries > + * about gpio_get() because we read and return the gpio-level. So the > + * gpiolob active_low handling is still correct. gpiolib? > + * > + * 0 - active low, 1 - active high > + */ > + gpi_type = !gpiod_is_active_low(desc); > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), > + gpi_type << DA9062_TYPE(offset)); > +} So this does not affect the value out set by da9062_gpio_set()? What is the electrical effect of this then, really? To me that seems like something that is mostly going to be related to how interrupts trigger (like whether to trig on rising or falling edge) and then it should really be in the .set_type() callback, should it not? > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > + unsigned int offset, int value) > +{ > + /* Push-Pull / Open-Drain options are configured during set_config */ > + da9062_gpio_set(gc, offset, value); > + > + return 0; > +} You probably want to make sure to clear the active low bit in this function, right? > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > + unsigned long config) > +{ > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > + struct regmap *regmap = gpio->da9062->regmap; > + int gpio_dir; Name this gpio_mode > + switch (pinconf_to_config_param(config)) { > + case PIN_CONFIG_BIAS_PULL_DOWN: > + /* PD only if pin is input */ Info from datasheet? Electrical limitation? Add to comment! > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > + if (gpio_dir < 0) > + return -EINVAL; > + else if (gpio_dir != DA9062_PIN_GPI) > + return -ENOTSUPP; > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > + BIT(offset), BIT(offset)); > + case PIN_CONFIG_BIAS_PULL_UP: > + /* PU only if pin is output open-drain */ Dito. > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > + if (gpio_dir < 0) > + return -EINVAL; > + else if (gpio_dir != DA9062_PIN_GPO_OD) > + return -ENOTSUPP; > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > + BIT(offset), BIT(offset)); > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > + return da9062_gpio_set_pin_mode(regmap, offset, > + DA9062_PIN_GPO_OD); > + case PIN_CONFIG_DRIVE_PUSH_PULL: > + return da9062_gpio_set_pin_mode(regmap, offset, > + DA9062_PIN_GPO_PP); > + default: > + return -ENOTSUPP; > + } Overall very nice use of this callback. > +static int da9062_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > +{ > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > + struct da9062 *da9062 = gpio->da9062; > + > + return regmap_irq_get_virq(da9062->regmap_irq, > + DA9062_IRQ_GPI0 + offset); > +} That's interesting. I never saw that before. It's fine, I guess, I can't figure out whether these slow expanders could be set as hierarchical though, what do you think? Saw a comment on this from Bartosz as well. But I generally trust his comment on that it should be fine unless you have subnodes (i.e. hierarchy). > +++ b/include/linux/mfd/da9062/gpio.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > + */ > + > +#ifndef __MFD_DA9062_GPIO_H__ > +#define __MFD_DA9062_GPIO_H__ > + > +struct gpio_desc; > + > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc); > + > +#endif /* __MFD_DA9062_GPIO_H__ */ So I'm sceptical about this and it needs a very good motivation. Yours, Linus Walleij
Hi Linus, thanks for you feedback. On 19-10-04 21:27, Linus Walleij wrote: > On Tue, Sep 17, 2019 at 12:59 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > > > The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can > > be used as input, output or have a special use-case. > > > > The patch adds the support for the normal input/output use-case. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > Nice to get this covered! > > > +#define DA9062_TYPE(offset) (4 * (offset % 2)) > > +#define DA9062_PIN_SHIFT(offset) (4 * (offset % 2)) > > +#define DA9062_PIN_ALTERNATE 0x00 /* gpio alternate mode */ > > The there is pin control related to the DA9062, we should put the driver > in drivers/pinctrl/pinctrl-da9062.c from day one. Mh.. didn't checked the pinctrl framework and assumed that this place is just fine due to the existing da9052/5 drivers which have an alternate mode too. > I am not saying you have to implement pin control from scratch, just > have it there and look at sibling drivers to make sure we don't > screw something up for also adding pin control later on. I will check the pinctrl framework. Maybe I will have upcoming question. > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc) > > +{ > > + return gpio_chip_hwgpio(desc); > > +} > > +EXPORT_SYMBOL_GPL(da9062_gpio_get_hwgpio); > > I would have to look at the other patch to see why this is needed. > Normally other drivers should just have their GPIOs assigned > like anyone else, no shortcuts into the hardware offsets > like this. I saw your comments on the other patch. As you mentioned in the other patche it is better to have a api within the gpiolib. I will add this api within the other patch series. > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) > > +{ > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > + struct regmap *regmap = gpio->da9062->regmap; > > + int gpio_dir, val; > > + int ret; > > + > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > This includes other things than the direction of the pin so call > the variable gpio_mode or something rather than gpio_dir. Okay, I will change that. > > + if (gpio_dir < 0) > > + return gpio_dir; > > + > > + switch (gpio_dir) { > > + case DA9062_PIN_ALTERNATE: > > + return -ENOTSUPP; > > This is fine for now. Toss in a comment that we may add pin muxing > later. > > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > > Same comments as above. Okay. > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > + unsigned int offset) > > +{ > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > + struct regmap *regmap = gpio->da9062->regmap; > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > + unsigned int gpi_type; > > + int ret; > > + > > + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > + if (ret) > > + return ret; > > Fair enough. > > > + /* > > + * If the gpio is active low we should set it in hw too. No worries > > + * about gpio_get() because we read and return the gpio-level. So the > > + * gpiolob active_low handling is still correct. > > gpiolib? Thanks for covering that. > > + * > > + * 0 - active low, 1 - active high > > + */ > > + gpi_type = !gpiod_is_active_low(desc); > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), > > + gpi_type << DA9062_TYPE(offset)); > > +} > > So this does not affect the value out set by da9062_gpio_set()? Please check [1] table 54, the datasheet says it is only gpi (gpio-input). So I assume it doesn't affect out values. [1] https://www.dialog-semiconductor.com/sites/default/files/da9062-a_datasheet_2v3.pdf Unfortunately the other gpio-da90* drivers sets this as active low hard within the driver. I wanted to avoid this here since it isn't always true. > What is the electrical effect of this then, really? To me that seems like > something that is mostly going to be related to how interrupts > trigger (like whether to trig on rising or falling edge) and then it > should really be in the .set_type() callback, should it not? Not only interrupts.. The dialog pmics has a lot of options to use this pins e.g. you can set it as voltage-selection input. You saw the patches I made for the regulator :) > > +static int da9062_gpio_direction_output(struct gpio_chip *gc, > > + unsigned int offset, int value) > > +{ > > + /* Push-Pull / Open-Drain options are configured during set_config */ > > + da9062_gpio_set(gc, offset, value); > > + > > + return 0; > > +} > > You probably want to make sure to clear the active low bit in this function, > right? Nope, as I mentioned above this is only input related. If we can trust the datasheet. > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > > + unsigned long config) > > +{ > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > + struct regmap *regmap = gpio->da9062->regmap; > > + int gpio_dir; > > Name this gpio_mode okay. > > + switch (pinconf_to_config_param(config)) { > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > + /* PD only if pin is input */ > > Info from datasheet? Electrical limitation? Add to comment! Okay, I will add more comments. > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > + if (gpio_dir < 0) > > + return -EINVAL; > > + else if (gpio_dir != DA9062_PIN_GPI) > > + return -ENOTSUPP; > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > + BIT(offset), BIT(offset)); > > + case PIN_CONFIG_BIAS_PULL_UP: > > + /* PU only if pin is output open-drain */ > > Dito. Here too. > > + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); > > + if (gpio_dir < 0) > > + return -EINVAL; > > + else if (gpio_dir != DA9062_PIN_GPO_OD) > > + return -ENOTSUPP; > > + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, > > + BIT(offset), BIT(offset)); > > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > + return da9062_gpio_set_pin_mode(regmap, offset, > > + DA9062_PIN_GPO_OD); > > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > + return da9062_gpio_set_pin_mode(regmap, offset, > > + DA9062_PIN_GPO_PP); > > + default: > > + return -ENOTSUPP; > > + } > > Overall very nice use of this callback. > > > +static int da9062_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > > +{ > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > + struct da9062 *da9062 = gpio->da9062; > > + > > + return regmap_irq_get_virq(da9062->regmap_irq, > > + DA9062_IRQ_GPI0 + offset); > > +} > > That's interesting. I never saw that before. It's fine, I guess, > I can't figure out whether these slow expanders could be set as > hierarchical though, what do you think? Saw a comment on this > from Bartosz as well. But I generally trust his comment on that > it should be fine unless you have subnodes (i.e. hierarchy). This v1 had a hierarchy.. but I changed that upon Bartosz comments. So there is no hierarchy in the next version. > > +++ b/include/linux/mfd/da9062/gpio.h > > @@ -0,0 +1,13 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> > > + */ > > + > > +#ifndef __MFD_DA9062_GPIO_H__ > > +#define __MFD_DA9062_GPIO_H__ > > + > > +struct gpio_desc; > > + > > +int da9062_gpio_get_hwgpio(struct gpio_desc *desc); > > + > > +#endif /* __MFD_DA9062_GPIO_H__ */ > > So I'm sceptical about this and it needs a very good motivation. I will change it to have a common api call as you mentioned it in the regulator-patch series. Regards, Marco > Yours, > Linus Walleij >
Hi Linus, On 19-10-07 10:51, Marco Felsch wrote: > Hi Linus, > > thanks for you feedback. > > On 19-10-04 21:27, Linus Walleij wrote: > > On Tue, Sep 17, 2019 at 12:59 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > > > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > > + unsigned int offset) > > > +{ > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > + struct regmap *regmap = gpio->da9062->regmap; > > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); This won't work anymore since I moved the driver to pinctrl and can't include the drivers/gpio/gpiolib.h anymore. What is the right way to get the same result within the pinctrl space? There are three possible ways: 1) Revert commit 1bd6b601fe196b6fbce2c93536ce0f3f53577cec which isn't the best due to safeness. 2) Set the gpio as active low hard as the other da90*-gpio drivers did 3) Introduce a dt-binding (seems wrong because the information is already there). 4) "Re-implement" the gpiochip_get_desc() functionality driver internally. Thanks for your advice. Regards, Marco > > > + unsigned int gpi_type; > > > + int ret; > > > + > > > + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > > + if (ret) > > > + return ret; > > > > Fair enough. > > > > > + /* > > > + * If the gpio is active low we should set it in hw too. No worries > > > + * about gpio_get() because we read and return the gpio-level. So the > > > + * gpiolob active_low handling is still correct. > > > > gpiolib? > > Thanks for covering that. > > > > + * > > > + * 0 - active low, 1 - active high > > > + */ > > > + gpi_type = !gpiod_is_active_low(desc); > > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > > + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), > > > + gpi_type << DA9062_TYPE(offset)); > > > +} > > > > So this does not affect the value out set by da9062_gpio_set()? > > Please check [1] table 54, the datasheet says it is only gpi > (gpio-input). So I assume it doesn't affect out values. > > [1] https://www.dialog-semiconductor.com/sites/default/files/da9062-a_datasheet_2v3.pdf > > Unfortunately the other gpio-da90* drivers sets this as active low hard > within the driver. I wanted to avoid this here since it isn't always > true. > > > What is the electrical effect of this then, really? To me that seems like > > something that is mostly going to be related to how interrupts > > trigger (like whether to trig on rising or falling edge) and then it > > should really be in the .set_type() callback, should it not? > > Not only interrupts.. The dialog pmics has a lot of options to use this > pins e.g. you can set it as voltage-selection input. You saw the patches > I made for the regulator :)
Hi Linus, sorry for the noise.. On 19-10-09 11:56, Marco Felsch wrote: > Hi Linus, > > On 19-10-07 10:51, Marco Felsch wrote: > > Hi Linus, > > > > thanks for you feedback. > > > > On 19-10-04 21:27, Linus Walleij wrote: > > > On Tue, Sep 17, 2019 at 12:59 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > > > > > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > > > + unsigned int offset) > > > > +{ > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > This won't work anymore since I moved the driver to pinctrl and can't > include the drivers/gpio/gpiolib.h anymore. What is the right way to > get the same result within the pinctrl space? There are three possible > ways: > 1) Revert commit 1bd6b601fe196b6fbce2c93536ce0f3f53577cec which isn't > the best due to safeness. > 2) Set the gpio as active low hard as the other da90*-gpio drivers did > 3) Introduce a dt-binding (seems wrong because the information is > already there). > 4) "Re-implement" the gpiochip_get_desc() functionality driver > internally. 4) won't work didn't recognize that 'struct gpio_device' is an internal struct. Regards, Marco > Thanks for your advice. > > Regards, > Marco > > > > > + unsigned int gpi_type; > > > > + int ret; > > > > + > > > > + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI); > > > > + if (ret) > > > > + return ret; > > > > > > Fair enough. > > > > > > > + /* > > > > + * If the gpio is active low we should set it in hw too. No worries > > > > + * about gpio_get() because we read and return the gpio-level. So the > > > > + * gpiolob active_low handling is still correct. > > > > > > gpiolib? > > > > Thanks for covering that. > > > > > > + * > > > > + * 0 - active low, 1 - active high > > > > + */ > > > > + gpi_type = !gpiod_is_active_low(desc); > > > > + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), > > > > + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), > > > > + gpi_type << DA9062_TYPE(offset)); > > > > +} > > > > > > So this does not affect the value out set by da9062_gpio_set()? > > > > Please check [1] table 54, the datasheet says it is only gpi > > (gpio-input). So I assume it doesn't affect out values. > > > > [1] https://www.dialog-semiconductor.com/sites/default/files/da9062-a_datasheet_2v3.pdf > > > > Unfortunately the other gpio-da90* drivers sets this as active low hard > > within the driver. I wanted to avoid this here since it isn't always > > true. > > > > > What is the electrical effect of this then, really? To me that seems like > > > something that is mostly going to be related to how interrupts > > > trigger (like whether to trig on rising or falling edge) and then it > > > should really be in the .set_type() callback, should it not? > > > > Not only interrupts.. The dialog pmics has a lot of options to use this > > pins e.g. you can set it as voltage-selection input. You saw the patches > > I made for the regulator :) > >
On Wed, Oct 9, 2019 at 12:17 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > > > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc, > > > > > + unsigned int offset) > > > > > +{ > > > > > + struct da9062_gpio *gpio = gpiochip_get_data(gc); > > > > > + struct regmap *regmap = gpio->da9062->regmap; > > > > > + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); > > > > This won't work anymore since I moved the driver to pinctrl and can't > > include the drivers/gpio/gpiolib.h anymore. What is the right way to > > get the same result within the pinctrl space? There are three possible > > ways: > > 1) Revert commit 1bd6b601fe196b6fbce2c93536ce0f3f53577cec which isn't > > the best due to safeness. > > 2) Set the gpio as active low hard as the other da90*-gpio drivers did > > 3) Introduce a dt-binding (seems wrong because the information is > > already there). > > 4) "Re-implement" the gpiochip_get_desc() functionality driver > > internally. > > 4) won't work didn't recognize that 'struct gpio_device' is an internal > struct. This is after all a gpiochip so it can use the internal structures. Go with #include <../gpio/gpiolib.h> Even if it is a bit ugly. One day I want to join the subsystems into one, and then this would go away, but until then we have to do this messy thing. Yours, Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index bb13c266c329..b308ea549aaa 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1038,6 +1038,17 @@ config GPIO_DA9055 If driver is built as a module it will be called gpio-da9055. +config GPIO_DA9062 + tristate "Dialog Semiconductor DA9062 GPIO" + depends on MFD_DA9062 + help + Say yes here to enable the GPIO driver for the DA9062 chip. + + The Dialog DA9062 PMIC chip has 5 GPIO pins that can be + be controller by this driver. + + If driver is built as a module it will be called gpio-da9062. + config GPIO_DLN2 tristate "Diolan DLN2 GPIO support" depends on MFD_DLN2 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index a4e91175c708..f29c8af2d096 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -45,6 +45,7 @@ obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o +obj-$(CONFIG_GPIO_DA9062) += gpio-da9062.o obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o diff --git a/drivers/gpio/gpio-da9062.c b/drivers/gpio/gpio-da9062.c new file mode 100644 index 000000000000..6035963929a2 --- /dev/null +++ b/drivers/gpio/gpio-da9062.c @@ -0,0 +1,265 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * GPIO Driver for Dialog DA9062 PMICs. + * Based on DA9055 GPIO driver. + * + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> + */ +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#include <linux/gpio/driver.h> +#include <linux/gpio/consumer.h> + +#include <linux/mfd/da9062/core.h> +#include <linux/mfd/da9062/registers.h> + +#include "gpiolib.h" + +#define DA9062_TYPE(offset) (4 * (offset % 2)) +#define DA9062_PIN_SHIFT(offset) (4 * (offset % 2)) +#define DA9062_PIN_ALTERNATE 0x00 /* gpio alternate mode */ +#define DA9062_PIN_GPI 0x01 /* gpio in */ +#define DA9062_PIN_GPO_OD 0x02 /* gpio out open-drain */ +#define DA9062_PIN_GPO_PP 0x03 /* gpio out push-pull */ +#define DA9062_GPIO_NUM 5 + +struct da9062_gpio { + struct da9062 *da9062; + struct gpio_chip gp; +}; + +int da9062_gpio_get_hwgpio(struct gpio_desc *desc) +{ + return gpio_chip_hwgpio(desc); +} +EXPORT_SYMBOL_GPL(da9062_gpio_get_hwgpio); + +static int da9062_gpio_get_pin_mode(struct regmap *regmap, unsigned int offset) +{ + int ret; + int val; + + ret = regmap_read(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), &val); + if (ret < 0) + return ret; + + val >>= DA9062_PIN_SHIFT(offset); + val &= DA9062AA_GPIO0_PIN_MASK; + + return val; +} + +static int da9062_gpio_set_pin_mode(struct regmap *regmap, unsigned int offset, + unsigned int mode) +{ + unsigned int mask; + + mode &= DA9062AA_GPIO0_PIN_MASK; + mode <<= DA9062_PIN_SHIFT(offset); + mask = DA9062AA_GPIO0_PIN_MASK << DA9062_PIN_SHIFT(offset); + + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), + mask, mode); +} + +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset) +{ + struct da9062_gpio *gpio = gpiochip_get_data(gc); + struct regmap *regmap = gpio->da9062->regmap; + int gpio_dir, val; + int ret; + + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); + if (gpio_dir < 0) + return gpio_dir; + + switch (gpio_dir) { + case DA9062_PIN_ALTERNATE: + return -ENOTSUPP; + case DA9062_PIN_GPI: + ret = regmap_read(regmap, DA9062AA_STATUS_B, &val); + if (ret < 0) + return ret; + break; + case DA9062_PIN_GPO_OD: + /* falltrough */ + case DA9062_PIN_GPO_PP: + ret = regmap_read(regmap, DA9062AA_GPIO_MODE0_4, &val); + if (ret < 0) + return ret; + } + + return val & BIT(offset); +} + +static void da9062_gpio_set(struct gpio_chip *gc, unsigned int offset, + int value) +{ + struct da9062_gpio *gpio = gpiochip_get_data(gc); + struct regmap *regmap = gpio->da9062->regmap; + + regmap_update_bits(regmap, DA9062AA_GPIO_MODE0_4, BIT(offset), + value << offset); +} + +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) +{ + struct da9062_gpio *gpio = gpiochip_get_data(gc); + struct regmap *regmap = gpio->da9062->regmap; + int gpio_dir; + + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); + if (gpio_dir < 0) + return gpio_dir; + + switch (gpio_dir) { + case DA9062_PIN_ALTERNATE: + return -ENOTSUPP; + case DA9062_PIN_GPI: + return 1; + case DA9062_PIN_GPO_OD: + /* falltrough */ + case DA9062_PIN_GPO_PP: + return 0; + } + + return -EINVAL; +} + +static int da9062_gpio_direction_input(struct gpio_chip *gc, + unsigned int offset) +{ + struct da9062_gpio *gpio = gpiochip_get_data(gc); + struct regmap *regmap = gpio->da9062->regmap; + struct gpio_desc *desc = gpiochip_get_desc(gc, offset); + unsigned int gpi_type; + int ret; + + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI); + if (ret) + return ret; + + /* + * If the gpio is active low we should set it in hw too. No worries + * about gpio_get() because we read and return the gpio-level. So the + * gpiolob active_low handling is still correct. + * + * 0 - active low, 1 - active high + */ + gpi_type = !gpiod_is_active_low(desc); + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1), + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset), + gpi_type << DA9062_TYPE(offset)); +} + +static int da9062_gpio_direction_output(struct gpio_chip *gc, + unsigned int offset, int value) +{ + /* Push-Pull / Open-Drain options are configured during set_config */ + da9062_gpio_set(gc, offset, value); + + return 0; +} + +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset, + unsigned long config) +{ + struct da9062_gpio *gpio = gpiochip_get_data(gc); + struct regmap *regmap = gpio->da9062->regmap; + int gpio_dir; + + switch (pinconf_to_config_param(config)) { + case PIN_CONFIG_BIAS_PULL_DOWN: + /* PD only if pin is input */ + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); + if (gpio_dir < 0) + return -EINVAL; + else if (gpio_dir != DA9062_PIN_GPI) + return -ENOTSUPP; + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, + BIT(offset), BIT(offset)); + case PIN_CONFIG_BIAS_PULL_UP: + /* PU only if pin is output open-drain */ + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset); + if (gpio_dir < 0) + return -EINVAL; + else if (gpio_dir != DA9062_PIN_GPO_OD) + return -ENOTSUPP; + return regmap_update_bits(regmap, DA9062AA_CONFIG_K, + BIT(offset), BIT(offset)); + case PIN_CONFIG_DRIVE_OPEN_DRAIN: + return da9062_gpio_set_pin_mode(regmap, offset, + DA9062_PIN_GPO_OD); + case PIN_CONFIG_DRIVE_PUSH_PULL: + return da9062_gpio_set_pin_mode(regmap, offset, + DA9062_PIN_GPO_PP); + default: + return -ENOTSUPP; + } +} + +static int da9062_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) +{ + struct da9062_gpio *gpio = gpiochip_get_data(gc); + struct da9062 *da9062 = gpio->da9062; + + return regmap_irq_get_virq(da9062->regmap_irq, + DA9062_IRQ_GPI0 + offset); +} + +static const struct gpio_chip reference_gp = { + .label = "da9062-gpio", + .owner = THIS_MODULE, + .get = da9062_gpio_get, + .set = da9062_gpio_set, + .get_direction = da9062_gpio_get_direction, + .direction_input = da9062_gpio_direction_input, + .direction_output = da9062_gpio_direction_output, + .set_config = da9062_gpio_set_config, + .to_irq = da9062_gpio_to_irq, + .can_sleep = true, + .ngpio = DA9062_GPIO_NUM, + .base = -1, +}; + +static int da9062_gpio_probe(struct platform_device *pdev) +{ + struct da9062_gpio *gpio; + + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); + if (!gpio) + return -ENOMEM; + + gpio->da9062 = dev_get_drvdata(pdev->dev.parent); + if (!gpio->da9062) + return -EINVAL; + + gpio->gp = reference_gp; + gpio->gp.parent = &pdev->dev; + + platform_set_drvdata(pdev, gpio); + + return devm_gpiochip_add_data(&pdev->dev, &gpio->gp, gpio); +} + +static const struct of_device_id da9062_compatible_id_table[] = { + { .compatible = "dlg,da9062-gpio" }, + { }, +}; +MODULE_DEVICE_TABLE(of, da9062_compatible_id_table); + +static struct platform_driver da9062_gpio_driver = { + .probe = da9062_gpio_probe, + .driver = { + .name = "da9062-gpio", + .of_match_table = da9062_compatible_id_table, + }, +}; +module_platform_driver(da9062_gpio_driver); + +MODULE_AUTHOR("Marco Felsch <kernel@pengutronix.de>"); +MODULE_DESCRIPTION("DA9062 GPIO Device Driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:da9062-gpio"); diff --git a/include/linux/mfd/da9062/gpio.h b/include/linux/mfd/da9062/gpio.h new file mode 100644 index 000000000000..67627ada1ad4 --- /dev/null +++ b/include/linux/mfd/da9062/gpio.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de> + */ + +#ifndef __MFD_DA9062_GPIO_H__ +#define __MFD_DA9062_GPIO_H__ + +struct gpio_desc; + +int da9062_gpio_get_hwgpio(struct gpio_desc *desc); + +#endif /* __MFD_DA9062_GPIO_H__ */
The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can be used as input, output or have a special use-case. The patch adds the support for the normal input/output use-case. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/gpio/Kconfig | 11 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-da9062.c | 265 ++++++++++++++++++++++++++++++++ include/linux/mfd/da9062/gpio.h | 13 ++ 4 files changed, 290 insertions(+) create mode 100644 drivers/gpio/gpio-da9062.c create mode 100644 include/linux/mfd/da9062/gpio.h