[PATCH v2 0/3] Add DA9062 GPIO support

[PATCH v2 0/3] Add DA9062 GPIO support

[Marco Felsch]

Hi,

this update address all comments made on [1], for further details see
the patch based changelog.

[1] https://patchwork.ozlabs.org/cover/1163308/

Marco Felsch (3):
  dt-bindings: mfd: da9062: add gpio bindings
  mfd: da9062: add support for the DA9062 GPIOs in the core
  pinctrl: da9062: add driver support

 .../devicetree/bindings/mfd/da9062.txt        |  10 +
 drivers/mfd/da9062-core.c                     |  16 +-
 drivers/pinctrl/Kconfig                       |  12 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-da9062.c              | 272 ++++++++++++++++++
 5 files changed, 310 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pinctrl/pinctrl-da9062.c

-- 
2.20.1

[PATCH v2 1/3] dt-bindings: mfd: da9062: add gpio bindings

[Marco Felsch]

Add gpio device documentation to make the da9062 gpios available for
users.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:

v2:
- remove sub-node documentation
- squash gpio properties into mfd documentation
---
 Documentation/devicetree/bindings/mfd/da9062.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt b/Documentation/devicetree/bindings/mfd/da9062.txt
index edca653a5777..089a28bc77a4 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:
 
@@ -38,6 +39,15 @@ Required properties:
 See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt for
 further information on IRQ bindings.
 
+Optional properties:
+
+- gpio-controller : Marks the device 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.
+
 Sub-nodes:
 
 - regulators : This node defines the settings for the LDOs and BUCKs.
-- 
2.20.1

[PATCH v2 2/3] mfd: da9062: add support for the DA9062 GPIOs in the core

[Marco Felsch]

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

[PATCH v2 3/3] pinctrl: da9062: add driver support

[Marco Felsch]

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>
---
Changelog:

v2:
- fix minor style issue
- move from drivers/gpio to drivers/pinctrl
- Fix spelling issue
- rename local gpio_dir to gpio_mode
- Add datasheet reference and TODO notes
- move gpio to mfd-root node to avoid hierarchical interrupt chips
- Add gpio-controller property check
- remove of_device_id since we drop the gpio of-subnode
- Drop da9062_gpio_get_hwgpio
---
 drivers/pinctrl/Kconfig          |  12 ++
 drivers/pinctrl/Makefile         |   1 +
 drivers/pinctrl/pinctrl-da9062.c | 272 +++++++++++++++++++++++++++++++
 3 files changed, 285 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-da9062.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index b372419d61f2..977787c158cc 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -126,6 +126,18 @@ config PINCTRL_DA850_PUPD
 	  Driver for TI DA850/OMAP-L138/AM18XX pinconf. Used to control
 	  pullup/pulldown pin groups.
 
+config PINCTRL_DA9062
+	tristate "Dialog Semiconductor DA9062 PMIC pinctrl and GPIO Support"
+	depends on MFD_DA9062
+	select GPIOLIB
+	help
+	  The Dialog DA9062 PMIC provides multiple GPIOs that can be muxed for
+	  different functions. This driver bundles a pinctrl driver to select the
+	  function muxing and a GPIO driver to handle the GPIO when the GPIO
+	  function is selected.
+
+	  Say yes to enable pinctrl and GPIO support for the DA9062 PMIC.
+
 config PINCTRL_DIGICOLOR
 	bool
 	depends on OF && (ARCH_DIGICOLOR || COMPILE_TEST)
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index ac537fdbc998..2397684cbe11 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_PINCTRL_AT91PIO4)	+= pinctrl-at91-pio4.o
 obj-$(CONFIG_PINCTRL_AMD)	+= pinctrl-amd.o
 obj-$(CONFIG_PINCTRL_BM1880)	+= pinctrl-bm1880.o
 obj-$(CONFIG_PINCTRL_DA850_PUPD) += pinctrl-da850-pupd.o
+obj-$(CONFIG_PINCTRL_DA9062)	+= pinctrl-da9062.o
 obj-$(CONFIG_PINCTRL_DIGICOLOR)	+= pinctrl-digicolor.o
 obj-$(CONFIG_PINCTRL_FALCON)	+= pinctrl-falcon.o
 obj-$(CONFIG_PINCTRL_GEMINI)	+= pinctrl-gemini.o
diff --git a/drivers/pinctrl/pinctrl-da9062.c b/drivers/pinctrl/pinctrl-da9062.c
new file mode 100644
index 000000000000..35ea8b488162
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-da9062.c
@@ -0,0 +1,272 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Dialog DA9062 pinctrl and GPIO driver.
+ * Based on DA9055 GPIO driver.
+ *
+ * TODO:
+ *   - add pinmux and pinctrl support (gpio alternate mode)
+ *
+ * Documents:
+ * [1] https://www.dialog-semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf
+ *
+ * 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/mfd/da9062/core.h>
+#include <linux/mfd/da9062/registers.h>
+
+#include <../gpio/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_pctl {
+	struct da9062 *da9062;
+	struct gpio_chip gc;
+};
+
+static int da9062_pctl_get_pin_mode(struct regmap *regmap, unsigned int offset)
+{
+	int ret, 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_pctl_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_pctl *pctl = gpiochip_get_data(gc);
+	struct regmap *regmap = pctl->da9062->regmap;
+	int gpio_mode, val;
+	int ret;
+
+	gpio_mode = da9062_pctl_get_pin_mode(regmap, offset);
+	if (gpio_mode < 0)
+		return gpio_mode;
+
+	switch (gpio_mode) {
+	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:
+	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_pctl *pctl = gpiochip_get_data(gc);
+	struct regmap *regmap = pctl->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_pctl *pctl = gpiochip_get_data(gc);
+	struct regmap *regmap = pctl->da9062->regmap;
+	int gpio_mode;
+
+	gpio_mode = da9062_pctl_get_pin_mode(regmap, offset);
+	if (gpio_mode < 0)
+		return gpio_mode;
+
+	switch (gpio_mode) {
+	case DA9062_PIN_ALTERNATE:
+		return -ENOTSUPP;
+	case DA9062_PIN_GPI:
+		return 1;
+	case DA9062_PIN_GPO_OD:
+	case DA9062_PIN_GPO_PP:
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int da9062_gpio_direction_input(struct gpio_chip *gc,
+				       unsigned int offset)
+{
+	struct da9062_pctl *pctl = gpiochip_get_data(gc);
+	struct regmap *regmap = pctl->da9062->regmap;
+	struct gpio_desc *desc = gpiochip_get_desc(gc, offset);
+	unsigned int gpi_type;
+	int ret;
+
+	ret = da9062_pctl_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
+	 * gpiolib 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_pctl *pctl = gpiochip_get_data(gc);
+	struct regmap *regmap = pctl->da9062->regmap;
+	int gpio_mode;
+
+	/*
+	 * We need to meet the following restrictions [1, Figure 18]:
+	 * - PIN_CONFIG_BIAS_PULL_DOWN -> only allowed of the pin is used as
+	 *				  gpio input
+	 * - PIN_CONFIG_BIAS_PULL_UP   -> only allowed of the pin is used as
+	 *				  gpio output open-drain.
+	 */
+
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		gpio_mode = da9062_pctl_get_pin_mode(regmap, offset);
+		if (gpio_mode < 0)
+			return -EINVAL;
+		else if (gpio_mode != DA9062_PIN_GPI)
+			return -ENOTSUPP;
+		return regmap_update_bits(regmap, DA9062AA_CONFIG_K,
+					  BIT(offset), BIT(offset));
+	case PIN_CONFIG_BIAS_PULL_UP:
+		gpio_mode = da9062_pctl_get_pin_mode(regmap, offset);
+		if (gpio_mode < 0)
+			return -EINVAL;
+		else if (gpio_mode != 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_pctl_set_pin_mode(regmap, offset,
+						DA9062_PIN_GPO_OD);
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		return da9062_pctl_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_pctl *pctl = gpiochip_get_data(gc);
+	struct da9062 *da9062 = pctl->da9062;
+
+	return regmap_irq_get_virq(da9062->regmap_irq,
+				   DA9062_IRQ_GPI0 + offset);
+}
+
+static const struct gpio_chip reference_gc = {
+	.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_pctl_probe(struct platform_device *pdev)
+{
+	struct device *parent = pdev->dev.parent;
+	struct da9062_pctl *pctl;
+
+	pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
+	if (!pctl)
+		return -ENOMEM;
+
+	pctl->da9062 = dev_get_drvdata(parent);
+	if (!pctl->da9062)
+		return -EINVAL;
+
+	if (!device_property_present(parent, "gpio-controller"))
+		return 0;
+
+	/*
+	 * Currently the driver handles only the GPIO support. The
+	 * pinctrl/pinmux support can be added later if needed.
+	 */
+	pctl->gc = reference_gc;
+	pctl->gc.label = dev_name(&pdev->dev);
+	pctl->gc.parent = &pdev->dev;
+#ifdef CONFIG_OF_GPIO
+	pctl->gc.of_node = parent->of_node;
+#endif
+
+	platform_set_drvdata(pdev, pctl);
+
+	return devm_gpiochip_add_data(&pdev->dev, &pctl->gc, pctl);
+}
+
+static struct platform_driver da9062_pctl_driver = {
+	.probe = da9062_pctl_probe,
+	.driver = {
+		.name	= "da9062-gpio",
+	},
+};
+module_platform_driver(da9062_pctl_driver);
+
+MODULE_AUTHOR("Marco Felsch <kernel@pengutronix.de>");
+MODULE_DESCRIPTION("DA9062 PMIC pinctrl and GPIO Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:da9062-gpio");
-- 
2.20.1

Re: [PATCH v2 2/3] mfd: da9062: add support for the DA9062 GPIOs in the core

[Linus Walleij]

On Wed, Nov 27, 2019 at 12:56 PM Marco Felsch <m.felsch@pengutronix.de> wrote:

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

This is a regmap irqchip so I guess not much to say about it.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

HOWEVER: this looks very much hierarchical does it not?

I can clearly see that regmap's irqchip does not support
hierarchical interrupt domains, so we should just make a
mental note somewhere that "oh yeah and then one day
we should make regmap irqchips play well with hierarchical
interrupts".

Yours,
Linus Walleij

Re: [PATCH v2 3/3] pinctrl: da9062: add driver support

[Linus Walleij]

Hi Marco,

thanks for your patch!

On Wed, Nov 27, 2019 at 12:56 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>
(...)

> +config PINCTRL_DA9062
> +       tristate "Dialog Semiconductor DA9062 PMIC pinctrl and GPIO Support"
> +       depends on MFD_DA9062
> +       select GPIOLIB

Hm this would be one of those that could use GENERIC_REGMAP_GPIO
the day we invent it but we haven't invented it yet.

> +#include <../gpio/gpiolib.h>

Put a comment above this telling us why you do this thing.

> +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
(...)
> +       return val & BIT(offset);

You should #include <linux/bits.h> since you use BIT()

Usually I clamp it like this:
return !!(val & BIT(offset));

> +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct da9062_pctl *pctl = gpiochip_get_data(gc);
> +       struct regmap *regmap = pctl->da9062->regmap;
> +       int gpio_mode;
> +
> +       gpio_mode = da9062_pctl_get_pin_mode(regmap, offset);
> +       if (gpio_mode < 0)
> +               return gpio_mode;
> +
> +       switch (gpio_mode) {
> +       case DA9062_PIN_ALTERNATE:
> +               return -ENOTSUPP;
> +       case DA9062_PIN_GPI:
> +               return 1;
> +       case DA9062_PIN_GPO_OD:
> +       case DA9062_PIN_GPO_PP:
> +               return 0;

We recently added defines for these directions in
<linux/gpio/driver.h>:

#define GPIO_LINE_DIRECTION_IN  1
#define GPIO_LINE_DIRECTION_OUT 0

Please use these. (Soon in Torvald's tree, else
in my "devel" branch.)

> +static int da9062_gpio_direction_input(struct gpio_chip *gc,
> +                                      unsigned int offset)
> +{
> +       struct da9062_pctl *pctl = gpiochip_get_data(gc);
> +       struct regmap *regmap = pctl->da9062->regmap;
> +       struct gpio_desc *desc = gpiochip_get_desc(gc, offset);
> +       unsigned int gpi_type;
> +       int ret;
> +
> +       ret = da9062_pctl_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
> +        * gpiolib active_low handling is still correct.
> +        *
> +        * 0 - active low, 1 - active high
> +        */
> +       gpi_type = !gpiod_is_active_low(desc);

That's interesting. Correct too, I guess.

> +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);

That looks dangerous. There is no guarantee that .set_config()
always gets called.

Please create a local state container for the mode of each pin in
struct da9062_pctl and set it to push-pull by default at probe, then
set this to whatever the state is here and let the .set_config()
alter it later if need be.

If we don't do that you will get boot-time defaults I think and that
might create bugs.

> +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
> +                                 unsigned long config)
> +{
(...)
> +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +               return da9062_pctl_set_pin_mode(regmap, offset,
> +                                               DA9062_PIN_GPO_OD);
> +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> +               return da9062_pctl_set_pin_mode(regmap, offset,
> +                                               DA9062_PIN_GPO_PP);

So also store this in the per-pin state.

Yours,
Linus Walleij

Re: [PATCH v2 2/3] mfd: da9062: add support for the DA9062 GPIOs in the core

[Marco Felsch]

Hi Linus,

thanks for your feedback.

On 19-11-27 14:35, Linus Walleij wrote:
> On Wed, Nov 27, 2019 at 12:56 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
> 
> > 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>
> 
> This is a regmap irqchip so I guess not much to say about it.
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> HOWEVER: this looks very much hierarchical does it not?

Yes it that's right and I converted it upon Bartosz comments.

> I can clearly see that regmap's irqchip does not support
> hierarchical interrupt domains, so we should just make a
> mental note somewhere that "oh yeah and then one day
> we should make regmap irqchips play well with hierarchical
> interrupts".

That's right, should I add this somewhere and if the answer is yes then
where?

Regards,
  Marco

> 
> Yours,
> Linus Walleij
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 3/3] pinctrl: da9062: add driver support

[Marco Felsch]

Hi Linus,

On 19-11-27 14:49, Linus Walleij wrote:
> Hi Marco,
> 
> thanks for your patch!

thanks for your fast response.

> On Wed, Nov 27, 2019 at 12:56 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>
> (...)
> 
> > +config PINCTRL_DA9062
> > +       tristate "Dialog Semiconductor DA9062 PMIC pinctrl and GPIO Support"
> > +       depends on MFD_DA9062
> > +       select GPIOLIB
> 
> Hm this would be one of those that could use GENERIC_REGMAP_GPIO
> the day we invent it but we haven't invented it yet.

Yes it is. Is there a plan for GENERIC_REGMAP_GPIO?

> > +#include <../gpio/gpiolib.h>
> 
> Put a comment above this telling us why you do this thing.

Okay.

> > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset)
> > +{
> (...)
> > +       return val & BIT(offset);
> 
> You should #include <linux/bits.h> since you use BIT()

Argh.. of course I will add the include.

> Usually I clamp it like this:
> return !!(val & BIT(offset));

Okay, I can change that too.

> > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +       struct da9062_pctl *pctl = gpiochip_get_data(gc);
> > +       struct regmap *regmap = pctl->da9062->regmap;
> > +       int gpio_mode;
> > +
> > +       gpio_mode = da9062_pctl_get_pin_mode(regmap, offset);
> > +       if (gpio_mode < 0)
> > +               return gpio_mode;
> > +
> > +       switch (gpio_mode) {
> > +       case DA9062_PIN_ALTERNATE:
> > +               return -ENOTSUPP;
> > +       case DA9062_PIN_GPI:
> > +               return 1;
> > +       case DA9062_PIN_GPO_OD:
> > +       case DA9062_PIN_GPO_PP:
> > +               return 0;
> 
> We recently added defines for these directions in
> <linux/gpio/driver.h>:
> 
> #define GPIO_LINE_DIRECTION_IN  1
> #define GPIO_LINE_DIRECTION_OUT 0
> 
> Please use these. (Soon in Torvald's tree, else
> in my "devel" branch.)

I rebased it onto your devel, thanks for the hint.

> > +static int da9062_gpio_direction_input(struct gpio_chip *gc,
> > +                                      unsigned int offset)
> > +{
> > +       struct da9062_pctl *pctl = gpiochip_get_data(gc);
> > +       struct regmap *regmap = pctl->da9062->regmap;
> > +       struct gpio_desc *desc = gpiochip_get_desc(gc, offset);
> > +       unsigned int gpi_type;
> > +       int ret;
> > +
> > +       ret = da9062_pctl_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
> > +        * gpiolib active_low handling is still correct.
> > +        *
> > +        * 0 - active low, 1 - active high
> > +        */
> > +       gpi_type = !gpiod_is_active_low(desc);
> 
> That's interesting. Correct too, I guess.

Double checked it again and the datasheet calls it gpio-level so I
assume that this is correct.

> > +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);
> 
> That looks dangerous. There is no guarantee that .set_config()
> always gets called.

Hm.. it seems that other drivers using this assumption too:
  - gpio-lp87565.c
  - gpio-tps65218.c
  - ...

But you're right I missed the possible users of
gpiod_direction_output_raw().

> Please create a local state container for the mode of each pin in
> struct da9062_pctl and set it to push-pull by default at probe, then
> set this to whatever the state is here and let the .set_config()
> alter it later if need be.
> 
> If we don't do that you will get boot-time defaults I think and that
> might create bugs.

I will add a container for each pin, thanks for covering that.

> > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
> > +                                 unsigned long config)
> > +{
> (...)
> > +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > +               return da9062_pctl_set_pin_mode(regmap, offset,
> > +                                               DA9062_PIN_GPO_OD);
> > +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> > +               return da9062_pctl_set_pin_mode(regmap, offset,
> > +                                               DA9062_PIN_GPO_PP);
> 
> So also store this in the per-pin state.

Of course.

Regards,
  Marco

> 
> Yours,
> Linus Walleij
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 2/3] mfd: da9062: add support for the DA9062 GPIOs in the core

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

On Wed, Nov 27, 2019 at 02:35:33PM +0100, Linus Walleij wrote:

> I can clearly see that regmap's irqchip does not support
> hierarchical interrupt domains, so we should just make a
> mental note somewhere that "oh yeah and then one day
> we should make regmap irqchips play well with hierarchical
> interrupts".

Why, what do we need to do?  We're just doing all the default stuff,
it's not something we've opted out of, and the whole point with using
the frameworks should be that we should get software features like this
for free :(

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/3] pinctrl: da9062: add driver support

[Bartosz Golaszewski]

śr., 27 lis 2019 o 16:01 Marco Felsch <m.felsch@pengutronix.de> napisał(a):
>
> Hi Linus,
>
> On 19-11-27 14:49, Linus Walleij wrote:
> > Hi Marco,
> >
> > thanks for your patch!
>
> thanks for your fast response.
>
> > On Wed, Nov 27, 2019 at 12:56 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>
> > (...)
> >
> > > +config PINCTRL_DA9062
> > > +       tristate "Dialog Semiconductor DA9062 PMIC pinctrl and GPIO Support"
> > > +       depends on MFD_DA9062
> > > +       select GPIOLIB
> >
> > Hm this would be one of those that could use GENERIC_REGMAP_GPIO
> > the day we invent it but we haven't invented it yet.
>
> Yes it is. Is there a plan for GENERIC_REGMAP_GPIO?
>

Yes, it's the second item on my TODO after the LINEINFO_FD series. I
just got a board I can use for developing this so I should have
something shortly.

Bart

> > > +#include <../gpio/gpiolib.h>
> >
> > Put a comment above this telling us why you do this thing.
>
> Okay.
>
> > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset)
> > > +{
> > (...)
> > > +       return val & BIT(offset);
> >
> > You should #include <linux/bits.h> since you use BIT()
>
> Argh.. of course I will add the include.
>
> > Usually I clamp it like this:
> > return !!(val & BIT(offset));
>
> Okay, I can change that too.
>
> > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> > > +{
> > > +       struct da9062_pctl *pctl = gpiochip_get_data(gc);
> > > +       struct regmap *regmap = pctl->da9062->regmap;
> > > +       int gpio_mode;
> > > +
> > > +       gpio_mode = da9062_pctl_get_pin_mode(regmap, offset);
> > > +       if (gpio_mode < 0)
> > > +               return gpio_mode;
> > > +
> > > +       switch (gpio_mode) {
> > > +       case DA9062_PIN_ALTERNATE:
> > > +               return -ENOTSUPP;
> > > +       case DA9062_PIN_GPI:
> > > +               return 1;
> > > +       case DA9062_PIN_GPO_OD:
> > > +       case DA9062_PIN_GPO_PP:
> > > +               return 0;
> >
> > We recently added defines for these directions in
> > <linux/gpio/driver.h>:
> >
> > #define GPIO_LINE_DIRECTION_IN  1
> > #define GPIO_LINE_DIRECTION_OUT 0
> >
> > Please use these. (Soon in Torvald's tree, else
> > in my "devel" branch.)
>
> I rebased it onto your devel, thanks for the hint.
>
> > > +static int da9062_gpio_direction_input(struct gpio_chip *gc,
> > > +                                      unsigned int offset)
> > > +{
> > > +       struct da9062_pctl *pctl = gpiochip_get_data(gc);
> > > +       struct regmap *regmap = pctl->da9062->regmap;
> > > +       struct gpio_desc *desc = gpiochip_get_desc(gc, offset);
> > > +       unsigned int gpi_type;
> > > +       int ret;
> > > +
> > > +       ret = da9062_pctl_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
> > > +        * gpiolib active_low handling is still correct.
> > > +        *
> > > +        * 0 - active low, 1 - active high
> > > +        */
> > > +       gpi_type = !gpiod_is_active_low(desc);
> >
> > That's interesting. Correct too, I guess.
>
> Double checked it again and the datasheet calls it gpio-level so I
> assume that this is correct.
>
> > > +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);
> >
> > That looks dangerous. There is no guarantee that .set_config()
> > always gets called.
>
> Hm.. it seems that other drivers using this assumption too:
>   - gpio-lp87565.c
>   - gpio-tps65218.c
>   - ...
>
> But you're right I missed the possible users of
> gpiod_direction_output_raw().
>
> > Please create a local state container for the mode of each pin in
> > struct da9062_pctl and set it to push-pull by default at probe, then
> > set this to whatever the state is here and let the .set_config()
> > alter it later if need be.
> >
> > If we don't do that you will get boot-time defaults I think and that
> > might create bugs.
>
> I will add a container for each pin, thanks for covering that.
>
> > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
> > > +                                 unsigned long config)
> > > +{
> > (...)
> > > +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > > +               return da9062_pctl_set_pin_mode(regmap, offset,
> > > +                                               DA9062_PIN_GPO_OD);
> > > +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> > > +               return da9062_pctl_set_pin_mode(regmap, offset,
> > > +                                               DA9062_PIN_GPO_PP);
> >
> > So also store this in the per-pin state.
>
> Of course.
>
> Regards,
>   Marco
>
> >
> > Yours,
> > Linus Walleij
> >
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 2/3] mfd: da9062: add support for the DA9062 GPIOs in the core

[Linus Walleij]

On Wed, Nov 27, 2019 at 4:19 PM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Nov 27, 2019 at 02:35:33PM +0100, Linus Walleij wrote:
>
> > I can clearly see that regmap's irqchip does not support
> > hierarchical interrupt domains, so we should just make a
> > mental note somewhere that "oh yeah and then one day
> > we should make regmap irqchips play well with hierarchical
> > interrupts".
>
> Why, what do we need to do?  We're just doing all the default stuff,
> it's not something we've opted out of, and the whole point with using
> the frameworks should be that we should get software features like this
> for free :(

I guess it is a bit about moving targets.

The regmap irq thing was covering all reasonable cases until
the hierarchical interrupts were introduced some years ago.

The hallmark of these are that the irq_domain_ops implement
.translate() .alloc() and .free() rather than .map() and .xlate()
as the irqdomain in reqmap-irq currently does.

The problem with hierarchical domains is that the system using
them need to be hierarchical "all the way up" to the overarching
top-level irqchip. Currently only the ARM GIC and the IXP4xx
irq top-level irq controllers supports this, ruling out some
obvious users like all non-ARM systems (for now).

I think the assumption in hierarchical irq is that you have
a few hardware-specific irchips and you know exactly which
irqchip that goes on top of another one, as well as which
hardware irq line is connected to which hardware irq line
on the parent.

Since we know the specific hardware (from a compatible
string or so) we can hardcode the parent-to-child mappings
of interrupt lines in the driver. These mappings are not
in the device tree for example.

Therefore supporting hierarchical and nonhierarchical alike
in a very generic plug-in irqchip like the regmap-irq is a bit
of a challenge as it has to support both hierarchical and
non-hierarchical, because it is not possible to just
convert this to hierarchical callbacks: it has to check what
its parent is doing and adapt, essentially implement both
hierarchical and non-hierarchical at this time.

Also we need to pass mappings between parent and child
somehow. In the gpiolib we did this with a callback to the
GPIO-chip-specific driver. How to do it for something
generic like regmap-irq is an open question.

So it is a bit complex.

(Marc may correct me here.)

Yours,
Linus Walleij

Re: [PATCH v2 3/3] pinctrl: da9062: add driver support

[Marco Felsch]

On 19-11-28 11:47, Bartosz Golaszewski wrote:
> śr., 27 lis 2019 o 16:01 Marco Felsch <m.felsch@pengutronix.de> napisał(a):
> >
> > Hi Linus,
> >
> > On 19-11-27 14:49, Linus Walleij wrote:
> > > Hi Marco,
> > >
> > > thanks for your patch!
> >
> > thanks for your fast response.
> >
> > > On Wed, Nov 27, 2019 at 12:56 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>
> > > (...)
> > >
> > > > +config PINCTRL_DA9062
> > > > +       tristate "Dialog Semiconductor DA9062 PMIC pinctrl and GPIO Support"
> > > > +       depends on MFD_DA9062
> > > > +       select GPIOLIB
> > >
> > > Hm this would be one of those that could use GENERIC_REGMAP_GPIO
> > > the day we invent it but we haven't invented it yet.
> >
> > Yes it is. Is there a plan for GENERIC_REGMAP_GPIO?
> >
> 
> Yes, it's the second item on my TODO after the LINEINFO_FD series. I
> just got a board I can use for developing this so I should have
> something shortly.

So it is okay to keep the above select and change it later?

Regards,
  Marco

> Bart
> 
> > > > +#include <../gpio/gpiolib.h>
> > >
> > > Put a comment above this telling us why you do this thing.
> >
> > Okay.
> >
> > > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset)
> > > > +{
> > > (...)
> > > > +       return val & BIT(offset);
> > >
> > > You should #include <linux/bits.h> since you use BIT()
> >
> > Argh.. of course I will add the include.
> >
> > > Usually I clamp it like this:
> > > return !!(val & BIT(offset));
> >
> > Okay, I can change that too.
> >
> > > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> > > > +{
> > > > +       struct da9062_pctl *pctl = gpiochip_get_data(gc);
> > > > +       struct regmap *regmap = pctl->da9062->regmap;
> > > > +       int gpio_mode;
> > > > +
> > > > +       gpio_mode = da9062_pctl_get_pin_mode(regmap, offset);
> > > > +       if (gpio_mode < 0)
> > > > +               return gpio_mode;
> > > > +
> > > > +       switch (gpio_mode) {
> > > > +       case DA9062_PIN_ALTERNATE:
> > > > +               return -ENOTSUPP;
> > > > +       case DA9062_PIN_GPI:
> > > > +               return 1;
> > > > +       case DA9062_PIN_GPO_OD:
> > > > +       case DA9062_PIN_GPO_PP:
> > > > +               return 0;
> > >
> > > We recently added defines for these directions in
> > > <linux/gpio/driver.h>:
> > >
> > > #define GPIO_LINE_DIRECTION_IN  1
> > > #define GPIO_LINE_DIRECTION_OUT 0
> > >
> > > Please use these. (Soon in Torvald's tree, else
> > > in my "devel" branch.)
> >
> > I rebased it onto your devel, thanks for the hint.
> >
> > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc,
> > > > +                                      unsigned int offset)
> > > > +{
> > > > +       struct da9062_pctl *pctl = gpiochip_get_data(gc);
> > > > +       struct regmap *regmap = pctl->da9062->regmap;
> > > > +       struct gpio_desc *desc = gpiochip_get_desc(gc, offset);
> > > > +       unsigned int gpi_type;
> > > > +       int ret;
> > > > +
> > > > +       ret = da9062_pctl_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
> > > > +        * gpiolib active_low handling is still correct.
> > > > +        *
> > > > +        * 0 - active low, 1 - active high
> > > > +        */
> > > > +       gpi_type = !gpiod_is_active_low(desc);
> > >
> > > That's interesting. Correct too, I guess.
> >
> > Double checked it again and the datasheet calls it gpio-level so I
> > assume that this is correct.
> >
> > > > +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);
> > >
> > > That looks dangerous. There is no guarantee that .set_config()
> > > always gets called.
> >
> > Hm.. it seems that other drivers using this assumption too:
> >   - gpio-lp87565.c
> >   - gpio-tps65218.c
> >   - ...
> >
> > But you're right I missed the possible users of
> > gpiod_direction_output_raw().
> >
> > > Please create a local state container for the mode of each pin in
> > > struct da9062_pctl and set it to push-pull by default at probe, then
> > > set this to whatever the state is here and let the .set_config()
> > > alter it later if need be.
> > >
> > > If we don't do that you will get boot-time defaults I think and that
> > > might create bugs.
> >
> > I will add a container for each pin, thanks for covering that.
> >
> > > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
> > > > +                                 unsigned long config)
> > > > +{
> > > (...)
> > > > +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > > > +               return da9062_pctl_set_pin_mode(regmap, offset,
> > > > +                                               DA9062_PIN_GPO_OD);
> > > > +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> > > > +               return da9062_pctl_set_pin_mode(regmap, offset,
> > > > +                                               DA9062_PIN_GPO_PP);
> > >
> > > So also store this in the per-pin state.
> >
> > Of course.
> >
> > Regards,
> >   Marco
> >
> > >
> > > Yours,
> > > Linus Walleij
> > >
> >
> > --
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 3/3] pinctrl: da9062: add driver support

[Bartosz Golaszewski]

pt., 29 lis 2019 o 10:07 Marco Felsch <m.felsch@pengutronix.de> napisał(a):
>
> On 19-11-28 11:47, Bartosz Golaszewski wrote:
> > śr., 27 lis 2019 o 16:01 Marco Felsch <m.felsch@pengutronix.de> napisał(a):
> > >
> > > Hi Linus,
> > >
> > > On 19-11-27 14:49, Linus Walleij wrote:
> > > > Hi Marco,
> > > >
> > > > thanks for your patch!
> > >
> > > thanks for your fast response.
> > >
> > > > On Wed, Nov 27, 2019 at 12:56 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>
> > > > (...)
> > > >
> > > > > +config PINCTRL_DA9062
> > > > > +       tristate "Dialog Semiconductor DA9062 PMIC pinctrl and GPIO Support"
> > > > > +       depends on MFD_DA9062
> > > > > +       select GPIOLIB
> > > >
> > > > Hm this would be one of those that could use GENERIC_REGMAP_GPIO
> > > > the day we invent it but we haven't invented it yet.
> > >
> > > Yes it is. Is there a plan for GENERIC_REGMAP_GPIO?
> > >
> >
> > Yes, it's the second item on my TODO after the LINEINFO_FD series. I
> > just got a board I can use for developing this so I should have
> > something shortly.
>
> So it is okay to keep the above select and change it later?
>

Yes, we don't want to stop you from getting this upstream. We'll
convert it once the new module is ready.

Bart

> Regards,
>   Marco
>
> > Bart
> >
> > > > > +#include <../gpio/gpiolib.h>
> > > >
> > > > Put a comment above this telling us why you do this thing.
> > >
> > > Okay.
> > >
> > > > > +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset)
> > > > > +{
> > > > (...)
> > > > > +       return val & BIT(offset);
> > > >
> > > > You should #include <linux/bits.h> since you use BIT()
> > >
> > > Argh.. of course I will add the include.
> > >
> > > > Usually I clamp it like this:
> > > > return !!(val & BIT(offset));
> > >
> > > Okay, I can change that too.
> > >
> > > > > +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> > > > > +{
> > > > > +       struct da9062_pctl *pctl = gpiochip_get_data(gc);
> > > > > +       struct regmap *regmap = pctl->da9062->regmap;
> > > > > +       int gpio_mode;
> > > > > +
> > > > > +       gpio_mode = da9062_pctl_get_pin_mode(regmap, offset);
> > > > > +       if (gpio_mode < 0)
> > > > > +               return gpio_mode;
> > > > > +
> > > > > +       switch (gpio_mode) {
> > > > > +       case DA9062_PIN_ALTERNATE:
> > > > > +               return -ENOTSUPP;
> > > > > +       case DA9062_PIN_GPI:
> > > > > +               return 1;
> > > > > +       case DA9062_PIN_GPO_OD:
> > > > > +       case DA9062_PIN_GPO_PP:
> > > > > +               return 0;
> > > >
> > > > We recently added defines for these directions in
> > > > <linux/gpio/driver.h>:
> > > >
> > > > #define GPIO_LINE_DIRECTION_IN  1
> > > > #define GPIO_LINE_DIRECTION_OUT 0
> > > >
> > > > Please use these. (Soon in Torvald's tree, else
> > > > in my "devel" branch.)
> > >
> > > I rebased it onto your devel, thanks for the hint.
> > >
> > > > > +static int da9062_gpio_direction_input(struct gpio_chip *gc,
> > > > > +                                      unsigned int offset)
> > > > > +{
> > > > > +       struct da9062_pctl *pctl = gpiochip_get_data(gc);
> > > > > +       struct regmap *regmap = pctl->da9062->regmap;
> > > > > +       struct gpio_desc *desc = gpiochip_get_desc(gc, offset);
> > > > > +       unsigned int gpi_type;
> > > > > +       int ret;
> > > > > +
> > > > > +       ret = da9062_pctl_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
> > > > > +        * gpiolib active_low handling is still correct.
> > > > > +        *
> > > > > +        * 0 - active low, 1 - active high
> > > > > +        */
> > > > > +       gpi_type = !gpiod_is_active_low(desc);
> > > >
> > > > That's interesting. Correct too, I guess.
> > >
> > > Double checked it again and the datasheet calls it gpio-level so I
> > > assume that this is correct.
> > >
> > > > > +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);
> > > >
> > > > That looks dangerous. There is no guarantee that .set_config()
> > > > always gets called.
> > >
> > > Hm.. it seems that other drivers using this assumption too:
> > >   - gpio-lp87565.c
> > >   - gpio-tps65218.c
> > >   - ...
> > >
> > > But you're right I missed the possible users of
> > > gpiod_direction_output_raw().
> > >
> > > > Please create a local state container for the mode of each pin in
> > > > struct da9062_pctl and set it to push-pull by default at probe, then
> > > > set this to whatever the state is here and let the .set_config()
> > > > alter it later if need be.
> > > >
> > > > If we don't do that you will get boot-time defaults I think and that
> > > > might create bugs.
> > >
> > > I will add a container for each pin, thanks for covering that.
> > >
> > > > > +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
> > > > > +                                 unsigned long config)
> > > > > +{
> > > > (...)
> > > > > +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > > > > +               return da9062_pctl_set_pin_mode(regmap, offset,
> > > > > +                                               DA9062_PIN_GPO_OD);
> > > > > +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> > > > > +               return da9062_pctl_set_pin_mode(regmap, offset,
> > > > > +                                               DA9062_PIN_GPO_PP);
> > > >
> > > > So also store this in the per-pin state.
> > >
> > > Of course.
> > >
> > > Regards,
> > >   Marco
> > >
> > > >
> > > > Yours,
> > > > Linus Walleij
> > > >
> > >
> > > --
> > > Pengutronix e.K.                           |                             |
> > > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> >
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 2/3] mfd: da9062: add support for the DA9062 GPIOs in the core

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3412 bytes --]

On Fri, Nov 29, 2019 at 09:49:56AM +0100, Linus Walleij wrote:
> On Wed, Nov 27, 2019 at 4:19 PM Mark Brown <broonie@kernel.org> wrote:

> > Why, what do we need to do?  We're just doing all the default stuff,
> > it's not something we've opted out of, and the whole point with using
> > the frameworks should be that we should get software features like this
> > for free :(

> I guess it is a bit about moving targets.

> The regmap irq thing was covering all reasonable cases until
> the hierarchical interrupts were introduced some years ago.

> The hallmark of these are that the irq_domain_ops implement
> .translate() .alloc() and .free() rather than .map() and .xlate()
> as the irqdomain in reqmap-irq currently does.

So there's two completely different APIs.  Are all the drivers supposed
to be being updated to implement both?  It looks like the second API is
ifdefed out when not in use so I guess so but...

> The problem with hierarchical domains is that the system using
> them need to be hierarchical "all the way up" to the overarching
> top-level irqchip. Currently only the ARM GIC and the IXP4xx
> irq top-level irq controllers supports this, ruling out some
> obvious users like all non-ARM systems (for now).

Are you sure?  It looks like the API was introduced by Intel and io_apic
appears to be using the new interfaces.

> I think the assumption in hierarchical irq is that you have
> a few hardware-specific irchips and you know exactly which
> irqchip that goes on top of another one, as well as which
> hardware irq line is connected to which hardware irq line
> on the parent.

The documentation says that this is for systems where "there may be
multiple interrupt controllers involved in delivering an interrupt from
the device to the target CPU" which describes more or less every single
regmap-irq user, though the threading might mean it doesn't quite map
onto what was being thought of there.  But basically everything I can
find suggests that regmap-irq should be a hierarchical controller apart
from how we figure out the primary IRQ.

> Since we know the specific hardware (from a compatible
> string or so) we can hardcode the parent-to-child mappings
> of interrupt lines in the driver. These mappings are not
> in the device tree for example.

That seems to be part of it, yes, which seems unfortunate.

> Therefore supporting hierarchical and nonhierarchical alike
> in a very generic plug-in irqchip like the regmap-irq is a bit
> of a challenge as it has to support both hierarchical and
> non-hierarchical, because it is not possible to just
> convert this to hierarchical callbacks: it has to check what
> its parent is doing and adapt, essentially implement both
> hierarchical and non-hierarchical at this time.

It looks that way, yes.

> Also we need to pass mappings between parent and child
> somehow. In the gpiolib we did this with a callback to the
> GPIO-chip-specific driver. How to do it for something
> generic like regmap-irq is an open question.

Currently regmap-irq just gets a parent interrupt from whatever is using
it so yeah, this doesn't map on at all well especially since the user is
likely to be using just a normal interrupt binding for this which is
apparently explicitly not allowed[1].  I'm not sure what to do here.

[1] https://elinux.org/images/8/8c/Zyngier.pdf

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]