* [PATCH 0/3] Add DA9062 GPIO support
@ 2019-09-17 10:58 Marco Felsch
2019-09-17 10:59 ` [PATCH 1/3] dt-bindings: mfd: da9062: add gpio bindings Marco Felsch
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Marco Felsch @ 2019-09-17 10:58 UTC (permalink / raw)
To: linus.walleij, bgolaszewski, robh+dt, support.opensource,
Adam.Thomson.Opensource
Cc: devicetree, linux-gpio, kernel
Hi,
this series adds the PMIC mfd GPIO support. In addition to the other
DA90* implementations it adds a small helper function which can be used
by the other da9062 drivers. Also instead of setting the GPIO input
logic statically to active-low as the da9052/53 drivers do we extract
the logic from the device tree.
Regards,
Marco
Marco Felsch (3):
dt-bindings: mfd: da9062: add gpio bindings
mfd: da9062: add support for the DA9062 GPIOs in the core
gpio: da9062: add driver support
.../devicetree/bindings/gpio/gpio-da9062.txt | 26 ++
.../devicetree/bindings/mfd/da9062.txt | 3 +
drivers/gpio/Kconfig | 11 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-da9062.c | 265 ++++++++++++++++++
drivers/mfd/da9062-core.c | 16 +-
include/linux/mfd/da9062/gpio.h | 13 +
7 files changed, 334 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-da9062.txt
create mode 100644 drivers/gpio/gpio-da9062.c
create mode 100644 include/linux/mfd/da9062/gpio.h
--
2.20.1
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/3] dt-bindings: mfd: da9062: add gpio bindings 2019-09-17 10:58 [PATCH 0/3] Add DA9062 GPIO support Marco Felsch @ 2019-09-17 10:59 ` Marco Felsch 2019-09-24 10:53 ` Adam Thomson 2019-09-17 10:59 ` [PATCH 2/3] mfd: da9062: add support for the DA9062 GPIOs in the core Marco Felsch 2019-09-17 10:59 ` [PATCH 3/3] gpio: da9062: add driver support Marco Felsch 2 siblings, 1 reply; 18+ messages in thread From: Marco Felsch @ 2019-09-17 10:59 UTC (permalink / raw) To: linus.walleij, bgolaszewski, robh+dt, support.opensource, Adam.Thomson.Opensource Cc: devicetree, linux-gpio, kernel Add gpio device documentation to make the da9062 gpios available for users. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- .../devicetree/bindings/gpio/gpio-da9062.txt | 26 +++++++++++++++++++ .../devicetree/bindings/mfd/da9062.txt | 3 +++ 2 files changed, 29 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-da9062.txt diff --git a/Documentation/devicetree/bindings/gpio/gpio-da9062.txt b/Documentation/devicetree/bindings/gpio/gpio-da9062.txt new file mode 100644 index 000000000000..b0f9236a7176 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-da9062.txt @@ -0,0 +1,26 @@ +GPIO driver for DA9062 Power management IC (PMIC) + +Device has 5 GPIO pins which can be configured as GPIO as well as the +special IO functions. + +Required properties: +------------------- +- compatible : Should be "dlg,da9062-gpio". +- gpio-controller : Marks the device node as a gpio controller. +- #gpio-cells : Should be two. The first cell is the pin number and the + second cell is used to specify the gpio polarity. + +See Documentation/devicetree/bindings/gpio/gpio.txt for further information on +GPIO bindings. + +Example: +-------- + pmic@58 { + ... + pmic_gpio: gpio { + compatible = "dlg,da9062-gpio"; + gpio-controller; + #gpio-cells = <2>; + }; + ... + }; diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt b/Documentation/devicetree/bindings/mfd/da9062.txt index edca653a5777..eec69c4f0697 100644 --- a/Documentation/devicetree/bindings/mfd/da9062.txt +++ b/Documentation/devicetree/bindings/mfd/da9062.txt @@ -13,6 +13,7 @@ da9062-rtc : : Real-Time Clock da9062-onkey : : On Key da9062-watchdog : : Watchdog Timer da9062-thermal : : Thermal +da9062-gpio : : GPIOs The DA9061 PMIC consists of: @@ -76,6 +77,8 @@ Sub-nodes: - thermal : See ../thermal/da9062-thermal.txt +- gpio : See ../gpio/gpio-da9062.txt + Example: pmic0: da9062@58 { -- 2.20.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* RE: [PATCH 1/3] dt-bindings: mfd: da9062: add gpio bindings 2019-09-17 10:59 ` [PATCH 1/3] dt-bindings: mfd: da9062: add gpio bindings Marco Felsch @ 2019-09-24 10:53 ` Adam Thomson 2019-10-04 19:06 ` Linus Walleij 0 siblings, 1 reply; 18+ messages in thread From: Adam Thomson @ 2019-09-24 10:53 UTC (permalink / raw) To: Marco Felsch, linus.walleij@linaro.org, bgolaszewski@baylibre.com, robh+dt@kernel.org, Support Opensource, Adam Thomson Cc: devicetree@vger.kernel.org, linux-gpio@vger.kernel.org, kernel@pengutronix.de On 17 September 2019 11:59, Marco Felsch wrote: > Add gpio device documentation to make the da9062 gpios available for > users. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > .../devicetree/bindings/gpio/gpio-da9062.txt | 26 +++++++++++++++++++ > .../devicetree/bindings/mfd/da9062.txt | 3 +++ > 2 files changed, 29 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-da9062.txt > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-da9062.txt > b/Documentation/devicetree/bindings/gpio/gpio-da9062.txt > new file mode 100644 > index 000000000000..b0f9236a7176 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/gpio-da9062.txt > @@ -0,0 +1,26 @@ > +GPIO driver for DA9062 Power management IC (PMIC) > + > +Device has 5 GPIO pins which can be configured as GPIO as well as the > +special IO functions. > + > +Required properties: > +------------------- > +- compatible : Should be "dlg,da9062-gpio". > +- gpio-controller : Marks the device node as a gpio controller. > +- #gpio-cells : Should be two. The first cell is the pin number and the > + second cell is used to specify the gpio polarity. > + > +See Documentation/devicetree/bindings/gpio/gpio.txt for further information > on > +GPIO bindings. As mentioned for your other patch set where the regulator driver makes use of GPIOs for control, I think here you should probably have pinctrl to define alternate functions of each GPIO. It seems that mostly pinctrl drivers support GPIO functionality rather than the other way, so maybe that's the direction to go in? Maybe Linus has some input on this too. > + > +Example: > +-------- > + pmic@58 { > + ... > + pmic_gpio: gpio { > + compatible = "dlg,da9062-gpio"; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + ... > + }; > diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt > b/Documentation/devicetree/bindings/mfd/da9062.txt > index edca653a5777..eec69c4f0697 100644 > --- a/Documentation/devicetree/bindings/mfd/da9062.txt > +++ b/Documentation/devicetree/bindings/mfd/da9062.txt > @@ -13,6 +13,7 @@ da9062-rtc : : Real-Time Clock > da9062-onkey : : On Key > da9062-watchdog : : Watchdog Timer > da9062-thermal : : Thermal > +da9062-gpio : : GPIOs > > The DA9061 PMIC consists of: > > @@ -76,6 +77,8 @@ Sub-nodes: > > - thermal : See ../thermal/da9062-thermal.txt > > +- gpio : See ../gpio/gpio-da9062.txt > + > Example: > > pmic0: da9062@58 { > -- > 2.20.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] dt-bindings: mfd: da9062: add gpio bindings 2019-09-24 10:53 ` Adam Thomson @ 2019-10-04 19:06 ` Linus Walleij 0 siblings, 0 replies; 18+ messages in thread From: Linus Walleij @ 2019-10-04 19:06 UTC (permalink / raw) To: Adam Thomson Cc: Marco Felsch, bgolaszewski@baylibre.com, robh+dt@kernel.org, Support Opensource, devicetree@vger.kernel.org, linux-gpio@vger.kernel.org, kernel@pengutronix.de On Tue, Sep 24, 2019 at 12:53 PM Adam Thomson <Adam.Thomson.Opensource@diasemi.com> wrote: > As mentioned for your other patch set where the regulator driver makes use of > GPIOs for control, I think here you should probably have pinctrl to define > alternate functions of each GPIO. It seems that mostly pinctrl drivers support > GPIO functionality rather than the other way, so maybe that's the direction > to go in? Maybe Linus has some input on this too. Usually the both functions can coexist but the simplest if often to put them in the same driver. drivers/pinctrl/pinctrl-stmfx.c Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt is a recent slow bus driver doing this. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/3] mfd: da9062: add support for the DA9062 GPIOs in the core 2019-09-17 10:58 [PATCH 0/3] Add DA9062 GPIO support Marco Felsch 2019-09-17 10:59 ` [PATCH 1/3] dt-bindings: mfd: da9062: add gpio bindings Marco Felsch @ 2019-09-17 10:59 ` Marco Felsch 2019-09-17 10:59 ` [PATCH 3/3] gpio: da9062: add driver support Marco Felsch 2 siblings, 0 replies; 18+ messages in thread From: Marco Felsch @ 2019-09-17 10:59 UTC (permalink / raw) To: linus.walleij, bgolaszewski, robh+dt, support.opensource, Adam.Thomson.Opensource Cc: devicetree, linux-gpio, kernel Currently the da9062 GPIO's aren't available. The patch adds the support to make these available by adding a gpio device with the corresponding irq resources. Furthermore the patch fixes a minor style issue for the onkey device. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/mfd/da9062-core.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c index e69626867c26..5290bdc0ddcd 100644 --- a/drivers/mfd/da9062-core.c +++ b/drivers/mfd/da9062-core.c @@ -233,6 +233,14 @@ static struct resource da9062_onkey_resources[] = { DEFINE_RES_NAMED(DA9062_IRQ_ONKEY, 1, "ONKEY", IORESOURCE_IRQ), }; +static struct resource da9062_gpio_resources[] = { + DEFINE_RES_NAMED(DA9062_IRQ_GPI0, 1, "GPI0", IORESOURCE_IRQ), + DEFINE_RES_NAMED(DA9062_IRQ_GPI1, 1, "GPI1", IORESOURCE_IRQ), + DEFINE_RES_NAMED(DA9062_IRQ_GPI2, 1, "GPI2", IORESOURCE_IRQ), + DEFINE_RES_NAMED(DA9062_IRQ_GPI3, 1, "GPI3", IORESOURCE_IRQ), + DEFINE_RES_NAMED(DA9062_IRQ_GPI4, 1, "GPI4", IORESOURCE_IRQ), +}; + static const struct mfd_cell da9062_devs[] = { { .name = "da9062-core", @@ -266,7 +274,13 @@ static const struct mfd_cell da9062_devs[] = { .name = "da9062-onkey", .num_resources = ARRAY_SIZE(da9062_onkey_resources), .resources = da9062_onkey_resources, - .of_compatible = "dlg,da9062-onkey", + .of_compatible = "dlg,da9062-onkey", + }, + { + .name = "da9062-gpio", + .num_resources = ARRAY_SIZE(da9062_gpio_resources), + .resources = da9062_gpio_resources, + .of_compatible = "dlg,da9062-gpio", }, }; -- 2.20.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/3] gpio: da9062: add driver support 2019-09-17 10:58 [PATCH 0/3] Add DA9062 GPIO support Marco Felsch 2019-09-17 10:59 ` [PATCH 1/3] dt-bindings: mfd: da9062: add gpio bindings Marco Felsch 2019-09-17 10:59 ` [PATCH 2/3] mfd: da9062: add support for the DA9062 GPIOs in the core Marco Felsch @ 2019-09-17 10:59 ` Marco Felsch 2019-09-18 7:04 ` Bartosz Golaszewski 2019-10-04 19:27 ` Linus Walleij 2 siblings, 2 replies; 18+ messages in thread From: Marco Felsch @ 2019-09-17 10:59 UTC (permalink / raw) To: linus.walleij, bgolaszewski, robh+dt, support.opensource, Adam.Thomson.Opensource Cc: devicetree, linux-gpio, kernel 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); + +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__ */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] gpio: da9062: add driver support 2019-09-17 10:59 ` [PATCH 3/3] gpio: da9062: add driver support Marco Felsch @ 2019-09-18 7:04 ` Bartosz Golaszewski 2019-09-18 12:06 ` Marco Felsch 2019-10-04 19:27 ` Linus Walleij 1 sibling, 1 reply; 18+ messages in thread From: Bartosz Golaszewski @ 2019-09-18 7:04 UTC (permalink / raw) To: Marco Felsch Cc: Linus Walleij, Rob Herring, Support Opensource, Adam.Thomson.Opensource, linux-devicetree, linux-gpio, kernel 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] gpio: da9062: add driver support 2019-09-18 7:04 ` Bartosz Golaszewski @ 2019-09-18 12:06 ` Marco Felsch 2019-09-19 8:24 ` Bartosz Golaszewski 0 siblings, 1 reply; 18+ messages in thread From: Marco Felsch @ 2019-09-18 12:06 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Linus Walleij, Rob Herring, Support Opensource, Adam.Thomson.Opensource, linux-devicetree, linux-gpio, kernel 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 > > > -- 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 | ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] gpio: da9062: add driver support 2019-09-18 12:06 ` Marco Felsch @ 2019-09-19 8:24 ` Bartosz Golaszewski 2019-09-19 8:38 ` Marco Felsch 0 siblings, 1 reply; 18+ messages in thread From: Bartosz Golaszewski @ 2019-09-19 8:24 UTC (permalink / raw) To: Marco Felsch Cc: Linus Walleij, Rob Herring, Support Opensource, Adam.Thomson.Opensource, linux-devicetree, linux-gpio, kernel ś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 | ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] gpio: da9062: add driver support 2019-09-19 8:24 ` Bartosz Golaszewski @ 2019-09-19 8:38 ` Marco Felsch 2019-09-30 18:42 ` Marco Felsch 0 siblings, 1 reply; 18+ messages in thread From: Marco Felsch @ 2019-09-19 8:38 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Linus Walleij, Rob Herring, Support Opensource, Adam.Thomson.Opensource, linux-devicetree, linux-gpio, kernel 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 | > -- 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 | ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] gpio: da9062: add driver support 2019-09-19 8:38 ` Marco Felsch @ 2019-09-30 18:42 ` Marco Felsch 2019-10-03 8:12 ` Bartosz Golaszewski 0 siblings, 1 reply; 18+ messages in thread From: Marco Felsch @ 2019-09-30 18:42 UTC (permalink / raw) To: Bartosz Golaszewski Cc: linux-devicetree, Support Opensource, Linus Walleij, linux-gpio, Rob Herring, kernel, Adam.Thomson.Opensource 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 | > > -- 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 | ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] gpio: da9062: add driver support 2019-09-30 18:42 ` Marco Felsch @ 2019-10-03 8:12 ` Bartosz Golaszewski 2019-10-04 8:25 ` Bartosz Golaszewski 0 siblings, 1 reply; 18+ messages in thread From: Bartosz Golaszewski @ 2019-10-03 8:12 UTC (permalink / raw) To: Marco Felsch Cc: linux-devicetree, Support Opensource, Linus Walleij, linux-gpio, Rob Herring, kernel, Adam.Thomson.Opensource 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 | ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] gpio: da9062: add driver support 2019-10-03 8:12 ` Bartosz Golaszewski @ 2019-10-04 8:25 ` Bartosz Golaszewski 0 siblings, 0 replies; 18+ messages in thread From: Bartosz Golaszewski @ 2019-10-04 8:25 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Marco Felsch, linux-devicetree, Support Opensource, Linus Walleij, linux-gpio, Rob Herring, Sascha Hauer, Adam.Thomson.Opensource 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 | ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] gpio: da9062: add driver support 2019-09-17 10:59 ` [PATCH 3/3] gpio: da9062: add driver support Marco Felsch 2019-09-18 7:04 ` Bartosz Golaszewski @ 2019-10-04 19:27 ` Linus Walleij 2019-10-07 8:51 ` Marco Felsch 1 sibling, 1 reply; 18+ messages in thread From: Linus Walleij @ 2019-10-04 19:27 UTC (permalink / raw) To: Marco Felsch Cc: Bartosz Golaszewski, Rob Herring, Support Opensource, Adam Thomson, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] gpio: da9062: add driver support 2019-10-04 19:27 ` Linus Walleij @ 2019-10-07 8:51 ` Marco Felsch 2019-10-09 9:56 ` Marco Felsch 0 siblings, 1 reply; 18+ messages in thread From: Marco Felsch @ 2019-10-07 8:51 UTC (permalink / raw) To: Linus Walleij Cc: Bartosz Golaszewski, Rob Herring, Support Opensource, Adam Thomson, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Sascha Hauer 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 > -- 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 | ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] gpio: da9062: add driver support 2019-10-07 8:51 ` Marco Felsch @ 2019-10-09 9:56 ` Marco Felsch 2019-10-09 10:17 ` Marco Felsch 0 siblings, 1 reply; 18+ messages in thread From: Marco Felsch @ 2019-10-09 9:56 UTC (permalink / raw) To: Linus Walleij Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Support Opensource, open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Rob Herring, Sascha Hauer, Adam Thomson 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 :) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] gpio: da9062: add driver support 2019-10-09 9:56 ` Marco Felsch @ 2019-10-09 10:17 ` Marco Felsch 2019-10-16 11:44 ` Linus Walleij 0 siblings, 1 reply; 18+ messages in thread From: Marco Felsch @ 2019-10-09 10:17 UTC (permalink / raw) To: Linus Walleij Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Support Opensource, open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Rob Herring, Sascha Hauer, Adam Thomson 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 :) > > -- 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 | ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] gpio: da9062: add driver support 2019-10-09 10:17 ` Marco Felsch @ 2019-10-16 11:44 ` Linus Walleij 0 siblings, 0 replies; 18+ messages in thread From: Linus Walleij @ 2019-10-16 11:44 UTC (permalink / raw) To: Marco Felsch Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Support Opensource, open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Rob Herring, Sascha Hauer, Adam Thomson 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-10-16 11:44 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-09-17 10:58 [PATCH 0/3] Add DA9062 GPIO support Marco Felsch 2019-09-17 10:59 ` [PATCH 1/3] dt-bindings: mfd: da9062: add gpio bindings Marco Felsch 2019-09-24 10:53 ` Adam Thomson 2019-10-04 19:06 ` Linus Walleij 2019-09-17 10:59 ` [PATCH 2/3] mfd: da9062: add support for the DA9062 GPIOs in the core Marco Felsch 2019-09-17 10:59 ` [PATCH 3/3] gpio: da9062: add driver support Marco Felsch 2019-09-18 7:04 ` Bartosz Golaszewski 2019-09-18 12:06 ` Marco Felsch 2019-09-19 8:24 ` Bartosz Golaszewski 2019-09-19 8:38 ` Marco Felsch 2019-09-30 18:42 ` Marco Felsch 2019-10-03 8:12 ` Bartosz Golaszewski 2019-10-04 8:25 ` Bartosz Golaszewski 2019-10-04 19:27 ` Linus Walleij 2019-10-07 8:51 ` Marco Felsch 2019-10-09 9:56 ` Marco Felsch 2019-10-09 10:17 ` Marco Felsch 2019-10-16 11:44 ` Linus Walleij
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).