[PATCH v3 00/12] add pinmuxing support for pins in AXP209 and AXP813 PMICs

[PATCH v3 00/12] add pinmuxing support for pins in AXP209 and AXP813 PMICs

[Quentin Schulz]

The AXP209 and AXP813 PMICs have several pins (respectively 3 and 2) that can
be used either as GPIOs or for other purposes (ADC or LDO here).

We already have a GPIO driver for the GPIO use of those pins on the AXP209.
Let's "upgrade" this driver to support all the functions these pins can have.

Then we add support to this driver for the AXP813 which is slighlty different
(basically a different offset in a register and one less pin).

I suggest patches 1 to 6 go through Linus's tree and 7 to 12 via Maxime or
Chen-Yu's tree.

This version of the patchset is based on Chen-Yu's patchset for AXP813/818
regulators[1].

v3:
  - add defines for GPIO funcs,
  - use again get_regs function instead of drv_data (which was implemented in
  v2),
  - use of_device_id data for device specific data (gpio_status_offset and pins
  description),
  - change compatible from axp813-pctl to axp813-gpio,
  - use axp81x DT label instead of axp813 since AXP813 and AXP818 are similar,
  - add dtsi include for all boards embedding axp813/axp818,
  - drastically simplify structures,
  - rebase on Chen-Yu's patch series for regulators,
  - add GPIOs pinmuxing for LDO regulators,

v2:
  - add support for AXP813 pins,
  - split into more patches so it is easier to follow the modifications,
  - reorder of some patches,
  - register all pins within the same range instead of a range per pin,

[1] https://www.spinics.net/lists/arm-kernel/msg608384.html

Thanks,
Quentin

Quentin Schulz (12):
  pinctrl: move gpio-axp209 to pinctrl
  pinctrl: axp209: add pinctrl features
  pinctrl: axp209: rename everything from gpio to pctl
  pinctrl: axp209: add programmable gpio_status_offset
  pinctrl: axp209: add support for AXP813 GPIOs
  mfd: axp20x: add pinctrl cell for AXP813
  ARM: dts: sun8i: a711: include axp81x dtsi
  ARM: dts: sun8i: bananapi-m3: include axp81x dtsi
  ARM: dts: sun8i: h8homlet-v2: include axp81x dtsi
  ARM: dts: sun8i: cubietruck-plus: include axp81x dtsi
  ARM: dtsi: axp81x: add GPIO DT node
  ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

 .../devicetree/bindings/gpio/gpio-axp209.txt       |  30 --
 .../devicetree/bindings/pinctrl/pinctrl-axp209.txt |  67 +++
 arch/arm/boot/dts/axp81x.dtsi                      |  20 +
 .../boot/dts/sun8i-a83t-allwinner-h8homlet-v2.dts  |   2 +
 arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts       |   2 +
 arch/arm/boot/dts/sun8i-a83t-cubietruck-plus.dts   |   2 +
 arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts          |   4 +-
 drivers/gpio/Kconfig                               |   6 -
 drivers/gpio/Makefile                              |   1 -
 drivers/gpio/gpio-axp209.c                         | 188 ---------
 drivers/mfd/axp20x.c                               |   3 +
 drivers/pinctrl/Kconfig                            |   6 +
 drivers/pinctrl/Makefile                           |   1 +
 drivers/pinctrl/pinctrl-axp209.c                   | 459 +++++++++++++++++++++
 14 files changed, 565 insertions(+), 226 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/gpio/gpio-axp209.txt
 create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
 delete mode 100644 drivers/gpio/gpio-axp209.c
 create mode 100644 drivers/pinctrl/pinctrl-axp209.c

-- 
2.11.0

[PATCH v3 01/12] pinctrl: move gpio-axp209 to pinctrl

[Quentin Schulz]

To prepare the driver for the upcoming pinctrl features, move the GPIO
driver AXP209 from GPIO to pinctrl subsystem.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 .../bindings/{gpio/gpio-axp209.txt => pinctrl/pinctrl-axp209.txt}   | 0
 drivers/gpio/Kconfig                                                | 6 ------
 drivers/gpio/Makefile                                               | 1 -
 drivers/pinctrl/Kconfig                                             | 6 ++++++
 drivers/pinctrl/Makefile                                            | 1 +
 drivers/{gpio/gpio-axp209.c => pinctrl/pinctrl-axp209.c}            | 0
 6 files changed, 7 insertions(+), 7 deletions(-)
 rename Documentation/devicetree/bindings/{gpio/gpio-axp209.txt => pinctrl/pinctrl-axp209.txt} (100%)
 rename drivers/{gpio/gpio-axp209.c => pinctrl/pinctrl-axp209.c} (100%)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-axp209.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
similarity index 100%
rename from Documentation/devicetree/bindings/gpio/gpio-axp209.txt
rename to Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 796b11c489ae..aae33f7d82ef 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -122,12 +122,6 @@ config GPIO_ATH79
 	  Select this option to enable GPIO driver for
 	  Atheros AR71XX/AR724X/AR913X SoC devices.
 
-config GPIO_AXP209
-	tristate "X-Powers AXP209 PMIC GPIO Support"
-	depends on MFD_AXP20X
-	help
-	  Say yes to enable GPIO support for the AXP209 PMIC
-
 config GPIO_BCM_KONA
 	bool "Broadcom Kona GPIO"
 	depends on OF_GPIO && (ARCH_BCM_MOBILE || COMPILE_TEST)
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index aeb70e9de6f2..f63631a63b09 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -31,7 +31,6 @@ obj-$(CONFIG_GPIO_AMDPT)	+= gpio-amdpt.o
 obj-$(CONFIG_GPIO_ARIZONA)	+= gpio-arizona.o
 obj-$(CONFIG_GPIO_ATH79)	+= gpio-ath79.o
 obj-$(CONFIG_GPIO_ASPEED)	+= gpio-aspeed.o
-obj-$(CONFIG_GPIO_AXP209)	+= gpio-axp209.o
 obj-$(CONFIG_GPIO_BCM_KONA)	+= gpio-bcm-kona.o
 obj-$(CONFIG_GPIO_BD9571MWV)	+= gpio-bd9571mwv.o
 obj-$(CONFIG_GPIO_BRCMSTB)	+= gpio-brcmstb.o
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 82cd8b08d71f..acda2e29167c 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -63,6 +63,12 @@ config PINCTRL_AS3722
 	  open drain configuration for the GPIO pins of AS3722 devices. It also
 	  supports the GPIO functionality through gpiolib.
 
+config PINCTRL_AXP209
+	tristate "X-Powers AXP209 PMIC pinctrl and GPIO Support"
+	depends on MFD_AXP20X
+	help
+	  Say yes to enable pinctrl and GPIO support for the AXP209 PMIC
+
 config PINCTRL_BF54x
 	def_bool y if BF54x
 	select PINCTRL_ADI2
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index c16e27900dbb..9f621e542f59 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
 obj-$(CONFIG_PINCTRL_ADI2)	+= pinctrl-adi2.o
 obj-$(CONFIG_PINCTRL_ARTPEC6)	+= pinctrl-artpec6.o
 obj-$(CONFIG_PINCTRL_AS3722)	+= pinctrl-as3722.o
+obj-$(CONFIG_PINCTRL_AXP209)	+= pinctrl-axp209.o
 obj-$(CONFIG_PINCTRL_BF54x)	+= pinctrl-adi2-bf54x.o
 obj-$(CONFIG_PINCTRL_BF60x)	+= pinctrl-adi2-bf60x.o
 obj-$(CONFIG_PINCTRL_AT91)	+= pinctrl-at91.o
diff --git a/drivers/gpio/gpio-axp209.c b/drivers/pinctrl/pinctrl-axp209.c
similarity index 100%
rename from drivers/gpio/gpio-axp209.c
rename to drivers/pinctrl/pinctrl-axp209.c
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 01/12] pinctrl: move gpio-axp209 to pinctrl

[Maxime Ripard]

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

On Mon, Oct 02, 2017 at 12:08:43PM +0000, Quentin Schulz wrote:
> To prepare the driver for the upcoming pinctrl features, move the GPIO
> driver AXP209 from GPIO to pinctrl subsystem.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v3 01/12] pinctrl: move gpio-axp209 to pinctrl

[Chen-Yu Tsai]

On Tue, Oct 3, 2017 at 4:18 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Mon, Oct 02, 2017 at 12:08:43PM +0000, Quentin Schulz wrote:
>> To prepare the driver for the upcoming pinctrl features, move the GPIO
>> driver AXP209 from GPIO to pinctrl subsystem.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>
> Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

[PATCH v3 02/12] pinctrl: axp209: add pinctrl features

[Quentin Schulz]

The X-Powers AXP209 has 3 GPIOs. GPIO0/1 can each act either as a GPIO,
an ADC or a LDO regulator. GPIO2 can only act as a GPIO.

This adds the pinctrl features to the driver so GPIO0/1 can be used as
ADC or LDO regulator.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 .../devicetree/bindings/pinctrl/pinctrl-axp209.txt |  28 +-
 drivers/pinctrl/pinctrl-axp209.c                   | 293 +++++++++++++++++++--
 2 files changed, 300 insertions(+), 21 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
index a6611304dd3c..388c04492afd 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
@@ -1,4 +1,4 @@
-AXP209 GPIO controller
+AXP209 GPIO & pinctrl controller
 
 This driver follows the usual GPIO bindings found in
 Documentation/devicetree/bindings/gpio/gpio.txt
@@ -28,3 +28,29 @@ axp209: pmic@34 {
 		#gpio-cells = <2>;
 	};
 };
+
+The GPIOs can be muxed to other functions and therefore, must be a subnode of
+axp_gpio.
+
+Example:
+
+&axp_gpio {
+	gpio0_adc: gpio0_adc {
+		pins = "GPIO0";
+		function = "adc";
+	};
+};
+
+&example_node {
+	pinctrl-names = "default";
+	pinctrl-0 = <&gpio0_adc>;
+};
+
+GPIOs and their functions
+-------------------------
+
+GPIO	|	Functions
+------------------------
+GPIO0	|	gpio_in, gpio_out, ldo, adc
+GPIO1	|	gpio_in, gpio_out, ldo, adc
+GPIO2	|	gpio_in, gpio_out
diff --git a/drivers/pinctrl/pinctrl-axp209.c b/drivers/pinctrl/pinctrl-axp209.c
index 4a346b7b4172..96ef0cc28762 100644
--- a/drivers/pinctrl/pinctrl-axp209.c
+++ b/drivers/pinctrl/pinctrl-axp209.c
@@ -1,7 +1,8 @@
 /*
- * AXP20x GPIO driver
+ * AXP20x pinctrl and GPIO driver
  *
  * Copyright (C) 2016 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ * Copyright (C) 2017 Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
  *
  * This program is free software; you can redistribute it and/or modify it
  * under  the terms of the GNU General  Public License as published by the
@@ -22,14 +23,57 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf-generic.h>
+
 #define AXP20X_GPIO_FUNCTIONS		0x7
 #define AXP20X_GPIO_FUNCTION_OUT_LOW	0
 #define AXP20X_GPIO_FUNCTION_OUT_HIGH	1
 #define AXP20X_GPIO_FUNCTION_INPUT	2
 
+#define AXP20X_FUNC_GPIO_OUT		0
+#define AXP20X_FUNC_GPIO_IN		1
+#define AXP20X_FUNC_LDO			2
+#define AXP20X_FUNC_ADC			3
+#define AXP20X_FUNCS_NB			4
+
+struct axp20x_pctrl_desc {
+	const struct pinctrl_pin_desc	*pins;
+	unsigned int			npins;
+	/* Stores the pins supporting LDO function. Bit offset is pin number. */
+	unsigned int			ldo_mask;
+	/* Stores the pins supporting ADC function. Bit offset is pin number. */
+	unsigned int			adc_mask;
+};
+
+struct axp20x_pinctrl_function {
+	const char	*name;
+	unsigned int	muxval;
+	const char	**groups;
+	unsigned int	ngroups;
+};
+
 struct axp20x_gpio {
 	struct gpio_chip	chip;
 	struct regmap		*regmap;
+	struct pinctrl_dev			*pctl_dev;
+	struct device				*dev;
+	const struct axp20x_pctrl_desc		*desc;
+	struct axp20x_pinctrl_function		funcs[AXP20X_FUNCS_NB];
+};
+
+static const struct pinctrl_pin_desc axp209_pins[] = {
+	PINCTRL_PIN(0, "GPIO0"),
+	PINCTRL_PIN(1, "GPIO1"),
+	PINCTRL_PIN(2, "GPIO2"),
+};
+
+static const struct axp20x_pctrl_desc axp20x_data = {
+	.pins	= axp209_pins,
+	.npins	= ARRAY_SIZE(axp209_pins),
+	.ldo_mask = BIT(0) | BIT(1),
+	.adc_mask = BIT(0) | BIT(1),
 };
 
 static int axp20x_gpio_get_reg(unsigned offset)
@@ -48,16 +92,7 @@ static int axp20x_gpio_get_reg(unsigned offset)
 
 static int axp20x_gpio_input(struct gpio_chip *chip, unsigned offset)
 {
-	struct axp20x_gpio *gpio = gpiochip_get_data(chip);
-	int reg;
-
-	reg = axp20x_gpio_get_reg(offset);
-	if (reg < 0)
-		return reg;
-
-	return regmap_update_bits(gpio->regmap, reg,
-				  AXP20X_GPIO_FUNCTIONS,
-				  AXP20X_GPIO_FUNCTION_INPUT);
+	return pinctrl_gpio_direction_input(chip->base + offset);
 }
 
 static int axp20x_gpio_get(struct gpio_chip *chip, unsigned offset)
@@ -105,29 +140,210 @@ static int axp20x_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 static int axp20x_gpio_output(struct gpio_chip *chip, unsigned offset,
 			      int value)
 {
+	chip->set(chip, offset, value);
+
+	return 0;
+}
+
+static void axp20x_gpio_set(struct gpio_chip *chip, unsigned offset,
+			    int value)
+{
 	struct axp20x_gpio *gpio = gpiochip_get_data(chip);
 	int reg;
 
 	reg = axp20x_gpio_get_reg(offset);
 	if (reg < 0)
+		return;
+
+	regmap_update_bits(gpio->regmap, reg,
+			   AXP20X_GPIO_FUNCTIONS,
+			   value ? AXP20X_GPIO_FUNCTION_OUT_HIGH :
+			   AXP20X_GPIO_FUNCTION_OUT_LOW);
+}
+
+static int axp20x_pmx_set(struct pinctrl_dev *pctldev, unsigned int offset,
+			  u8 config)
+{
+	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+	int reg;
+
+	reg = axp20x_gpio_get_reg(offset);
+	if (reg < 0)
 		return reg;
 
-	return regmap_update_bits(gpio->regmap, reg,
-				  AXP20X_GPIO_FUNCTIONS,
-				  value ? AXP20X_GPIO_FUNCTION_OUT_HIGH
-				  : AXP20X_GPIO_FUNCTION_OUT_LOW);
+	return regmap_update_bits(gpio->regmap, reg, AXP20X_GPIO_FUNCTIONS,
+				  config);
 }
 
-static void axp20x_gpio_set(struct gpio_chip *chip, unsigned offset,
-			    int value)
+static int axp20x_pmx_func_cnt(struct pinctrl_dev *pctldev)
+{
+	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+
+	return ARRAY_SIZE(gpio->funcs);
+}
+
+static const char *axp20x_pmx_func_name(struct pinctrl_dev *pctldev,
+					unsigned int selector)
 {
-	axp20x_gpio_output(chip, offset, value);
+	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+
+	return gpio->funcs[selector].name;
+}
+
+static int axp20x_pmx_func_groups(struct pinctrl_dev *pctldev,
+				  unsigned int selector,
+				  const char * const **groups,
+				  unsigned int *num_groups)
+{
+	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = gpio->funcs[selector].groups;
+	*num_groups = gpio->funcs[selector].ngroups;
+
+	return 0;
+}
+
+static int axp20x_pmx_set_mux(struct pinctrl_dev *pctldev,
+			      unsigned int function, unsigned int group)
+{
+	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+	unsigned int mask;
+
+	/* Every pin supports GPIO_OUT and GPIO_IN functions */
+	if (function <= AXP20X_FUNC_GPIO_IN)
+		return axp20x_pmx_set(pctldev, group,
+				      gpio->funcs[function].muxval);
+
+	if (function == AXP20X_FUNC_LDO)
+		mask = gpio->desc->ldo_mask;
+	else
+		mask = gpio->desc->adc_mask;
+
+	if (!(BIT(group) & mask))
+		return -EINVAL;
+
+	return axp20x_pmx_set(pctldev, group, gpio->funcs[function].muxval);
+}
+
+static int axp20x_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+					 struct pinctrl_gpio_range *range,
+					 unsigned int offset, bool input)
+{
+	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+
+	if (input)
+		return axp20x_pmx_set(pctldev, offset,
+				      gpio->funcs[AXP20X_FUNC_GPIO_IN].muxval);
+
+	return axp20x_pmx_set(pctldev, offset,
+			      gpio->funcs[AXP20X_FUNC_GPIO_OUT].muxval);
+}
+
+static const struct pinmux_ops axp20x_pmx_ops = {
+	.get_functions_count	= axp20x_pmx_func_cnt,
+	.get_function_name	= axp20x_pmx_func_name,
+	.get_function_groups	= axp20x_pmx_func_groups,
+	.set_mux		= axp20x_pmx_set_mux,
+	.gpio_set_direction	= axp20x_pmx_gpio_set_direction,
+	.strict			= true,
+};
+
+static int axp20x_groups_cnt(struct pinctrl_dev *pctldev)
+{
+	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+
+	return gpio->desc->npins;
+}
+
+static int axp20x_group_pins(struct pinctrl_dev *pctldev, unsigned int selector,
+			     const unsigned int **pins, unsigned int *num_pins)
+{
+	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+
+	*pins = (unsigned int *)&gpio->desc->pins[selector];
+	*num_pins = 1;
+
+	return 0;
+}
+
+static const char *axp20x_group_name(struct pinctrl_dev *pctldev,
+				     unsigned int selector)
+{
+	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+
+	return gpio->desc->pins[selector].name;
+}
+
+static const struct pinctrl_ops axp20x_pctrl_ops = {
+	.dt_node_to_map		= pinconf_generic_dt_node_to_map_group,
+	.dt_free_map		= pinconf_generic_dt_free_map,
+	.get_groups_count	= axp20x_groups_cnt,
+	.get_group_name		= axp20x_group_name,
+	.get_group_pins		= axp20x_group_pins,
+};
+
+static void axp20x_funcs_groups_from_mask(struct device *dev, unsigned int mask,
+					  unsigned int mask_len,
+					  struct axp20x_pinctrl_function *func,
+					  const struct pinctrl_pin_desc *pins)
+{
+	unsigned long int mask_cpy = mask;
+	const char **group;
+	unsigned int ngroups = hweight8(mask);
+	int bit;
+
+	func->ngroups = ngroups;
+	if (func->ngroups > 0) {
+		func->groups = devm_kzalloc(dev, ngroups * sizeof(const char *),
+					    GFP_KERNEL);
+		group = func->groups;
+		for_each_set_bit(bit, &mask_cpy, mask_len) {
+			*group = pins[bit].name;
+			group++;
+		}
+	}
+}
+
+static void axp20x_build_funcs_groups(struct platform_device *pdev)
+{
+	struct axp20x_gpio *gpio = platform_get_drvdata(pdev);
+	int i, pin;
+
+	gpio->funcs[AXP20X_FUNC_GPIO_OUT].name = "gpio_out";
+	gpio->funcs[AXP20X_FUNC_GPIO_OUT].muxval = 0x0;
+	gpio->funcs[AXP20X_FUNC_GPIO_IN].name = "gpio_in";
+	gpio->funcs[AXP20X_FUNC_GPIO_IN].muxval = 0x2;
+	gpio->funcs[AXP20X_FUNC_LDO].name = "ldo";
+	gpio->funcs[AXP20X_FUNC_LDO].muxval = 0x3;
+	gpio->funcs[AXP20X_FUNC_ADC].name = "adc";
+	gpio->funcs[AXP20X_FUNC_ADC].muxval = 0x4;
+
+	/* Every pin supports GPIO_OUT and GPIO_IN functions */
+	for (i = 0; i <= AXP20X_FUNC_GPIO_IN; i++) {
+		gpio->funcs[i].ngroups = gpio->desc->npins;
+		gpio->funcs[i].groups = devm_kzalloc(&pdev->dev,
+						     gpio->desc->npins * sizeof(const char *),
+						     GFP_KERNEL);
+		for (pin = 0; pin < gpio->desc->npins; pin++)
+			gpio->funcs[i].groups[pin] = gpio->desc->pins[pin].name;
+	}
+
+	axp20x_funcs_groups_from_mask(&pdev->dev, gpio->desc->ldo_mask,
+				      gpio->desc->npins,
+				      &gpio->funcs[AXP20X_FUNC_LDO],
+				      gpio->desc->pins);
+
+	axp20x_funcs_groups_from_mask(&pdev->dev, gpio->desc->adc_mask,
+				      gpio->desc->npins,
+				      &gpio->funcs[AXP20X_FUNC_ADC],
+				      gpio->desc->pins);
 }
 
 static int axp20x_gpio_probe(struct platform_device *pdev)
 {
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
 	struct axp20x_gpio *gpio;
+	struct pinctrl_desc *pctrl_desc;
 	int ret;
 
 	if (!of_device_is_available(pdev->dev.of_node))
@@ -144,6 +360,8 @@ static int axp20x_gpio_probe(struct platform_device *pdev)
 
 	gpio->chip.base			= -1;
 	gpio->chip.can_sleep		= true;
+	gpio->chip.request		= gpiochip_generic_request;
+	gpio->chip.free			= gpiochip_generic_free;
 	gpio->chip.parent		= &pdev->dev;
 	gpio->chip.label		= dev_name(&pdev->dev);
 	gpio->chip.owner		= THIS_MODULE;
@@ -154,15 +372,49 @@ static int axp20x_gpio_probe(struct platform_device *pdev)
 	gpio->chip.direction_output	= axp20x_gpio_output;
 	gpio->chip.ngpio		= 3;
 
+	gpio->desc = &axp20x_data;
+
 	gpio->regmap = axp20x->regmap;
 
+	gpio->dev = &pdev->dev;
+
+	platform_set_drvdata(pdev, gpio);
+
+	axp20x_build_funcs_groups(pdev);
+
+	pctrl_desc = devm_kzalloc(&pdev->dev, sizeof(*pctrl_desc), GFP_KERNEL);
+	if (!pctrl_desc)
+		return -ENOMEM;
+
+	pctrl_desc->name = dev_name(&pdev->dev);
+	pctrl_desc->owner = THIS_MODULE;
+	pctrl_desc->pins = gpio->desc->pins;
+	pctrl_desc->npins = gpio->desc->npins;
+	pctrl_desc->pctlops = &axp20x_pctrl_ops;
+	pctrl_desc->pmxops = &axp20x_pmx_ops;
+
+	gpio->pctl_dev = devm_pinctrl_register(&pdev->dev, pctrl_desc, gpio);
+	if (IS_ERR(gpio->pctl_dev)) {
+		dev_err(&pdev->dev, "couldn't register pinctrl driver\n");
+		return PTR_ERR(gpio->pctl_dev);
+	}
+
 	ret = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to register GPIO chip\n");
 		return ret;
 	}
 
-	dev_info(&pdev->dev, "AXP209 GPIO driver loaded\n");
+	ret = gpiochip_add_pin_range(&gpio->chip, dev_name(&pdev->dev),
+				     gpio->desc->pins->number,
+				     gpio->desc->pins->number,
+				     gpio->desc->npins);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add pin range\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "AXP209 pinctrl and GPIO driver loaded\n");
 
 	return 0;
 }
@@ -184,5 +436,6 @@ static struct platform_driver axp20x_gpio_driver = {
 module_platform_driver(axp20x_gpio_driver);
 
 MODULE_AUTHOR("Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
-MODULE_DESCRIPTION("AXP20x PMIC GPIO driver");
+MODULE_AUTHOR("Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
+MODULE_DESCRIPTION("AXP20x PMIC pinctrl and GPIO driver");
 MODULE_LICENSE("GPL");
-- 
2.11.0

Re: [PATCH v3 02/12] pinctrl: axp209: add pinctrl features

[Maxime Ripard]

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

On Mon, Oct 02, 2017 at 12:08:44PM +0000, Quentin Schulz wrote:
> The X-Powers AXP209 has 3 GPIOs. GPIO0/1 can each act either as a GPIO,
> an ADC or a LDO regulator. GPIO2 can only act as a GPIO.
> 
> This adds the pinctrl features to the driver so GPIO0/1 can be used as
> ADC or LDO regulator.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../devicetree/bindings/pinctrl/pinctrl-axp209.txt |  28 +-
>  drivers/pinctrl/pinctrl-axp209.c                   | 293 +++++++++++++++++++--
>  2 files changed, 300 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
> index a6611304dd3c..388c04492afd 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
> @@ -1,4 +1,4 @@
> -AXP209 GPIO controller
> +AXP209 GPIO & pinctrl controller
>  
>  This driver follows the usual GPIO bindings found in
>  Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -28,3 +28,29 @@ axp209: pmic@34 {
>  		#gpio-cells = <2>;
>  	};
>  };
> +
> +The GPIOs can be muxed to other functions and therefore, must be a subnode of
> +axp_gpio.
> +
> +Example:
> +
> +&axp_gpio {
> +	gpio0_adc: gpio0_adc {
> +		pins = "GPIO0";
> +		function = "adc";
> +	};
> +};
> +
> +&example_node {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gpio0_adc>;
> +};
> +
> +GPIOs and their functions
> +-------------------------
> +
> +GPIO	|	Functions
> +------------------------
> +GPIO0	|	gpio_in, gpio_out, ldo, adc
> +GPIO1	|	gpio_in, gpio_out, ldo, adc
> +GPIO2	|	gpio_in, gpio_out
> diff --git a/drivers/pinctrl/pinctrl-axp209.c b/drivers/pinctrl/pinctrl-axp209.c
> index 4a346b7b4172..96ef0cc28762 100644
> --- a/drivers/pinctrl/pinctrl-axp209.c
> +++ b/drivers/pinctrl/pinctrl-axp209.c
> @@ -1,7 +1,8 @@
>  /*
> - * AXP20x GPIO driver
> + * AXP20x pinctrl and GPIO driver
>   *
>   * Copyright (C) 2016 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + * Copyright (C) 2017 Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under  the terms of the GNU General  Public License as published by the
> @@ -22,14 +23,57 @@
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
>  
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +

You should order the headers by alphabetical order.

>  #define AXP20X_GPIO_FUNCTIONS		0x7
>  #define AXP20X_GPIO_FUNCTION_OUT_LOW	0
>  #define AXP20X_GPIO_FUNCTION_OUT_HIGH	1
>  #define AXP20X_GPIO_FUNCTION_INPUT	2
>  
> +#define AXP20X_FUNC_GPIO_OUT		0
> +#define AXP20X_FUNC_GPIO_IN		1
> +#define AXP20X_FUNC_LDO			2
> +#define AXP20X_FUNC_ADC			3
> +#define AXP20X_FUNCS_NB			4
> +
> +struct axp20x_pctrl_desc {
> +	const struct pinctrl_pin_desc	*pins;
> +	unsigned int			npins;
> +	/* Stores the pins supporting LDO function. Bit offset is pin number. */
> +	unsigned int			ldo_mask;
> +	/* Stores the pins supporting ADC function. Bit offset is pin number. */
> +	unsigned int			adc_mask;

You can use u8 here.

> +};
> +
> +struct axp20x_pinctrl_function {
> +	const char	*name;
> +	unsigned int	muxval;
> +	const char	**groups;
> +	unsigned int	ngroups;
> +};
> +
>  struct axp20x_gpio {
>  	struct gpio_chip	chip;
>  	struct regmap		*regmap;
> +	struct pinctrl_dev			*pctl_dev;
> +	struct device				*dev;
> +	const struct axp20x_pctrl_desc		*desc;
> +	struct axp20x_pinctrl_function		funcs[AXP20X_FUNCS_NB];
> +};
> +
> +static const struct pinctrl_pin_desc axp209_pins[] = {
> +	PINCTRL_PIN(0, "GPIO0"),
> +	PINCTRL_PIN(1, "GPIO1"),
> +	PINCTRL_PIN(2, "GPIO2"),
> +};
> +
> +static const struct axp20x_pctrl_desc axp20x_data = {
> +	.pins	= axp209_pins,
> +	.npins	= ARRAY_SIZE(axp209_pins),
> +	.ldo_mask = BIT(0) | BIT(1),
> +	.adc_mask = BIT(0) | BIT(1),
>  };
>  
>  static int axp20x_gpio_get_reg(unsigned offset)
> @@ -48,16 +92,7 @@ static int axp20x_gpio_get_reg(unsigned offset)
>  
>  static int axp20x_gpio_input(struct gpio_chip *chip, unsigned offset)
>  {
> -	struct axp20x_gpio *gpio = gpiochip_get_data(chip);
> -	int reg;
> -
> -	reg = axp20x_gpio_get_reg(offset);
> -	if (reg < 0)
> -		return reg;
> -
> -	return regmap_update_bits(gpio->regmap, reg,
> -				  AXP20X_GPIO_FUNCTIONS,
> -				  AXP20X_GPIO_FUNCTION_INPUT);
> +	return pinctrl_gpio_direction_input(chip->base + offset);
>  }
>  
>  static int axp20x_gpio_get(struct gpio_chip *chip, unsigned offset)
> @@ -105,29 +140,210 @@ static int axp20x_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
>  static int axp20x_gpio_output(struct gpio_chip *chip, unsigned offset,
>  			      int value)
>  {
> +	chip->set(chip, offset, value);
> +
> +	return 0;
> +}
> +
> +static void axp20x_gpio_set(struct gpio_chip *chip, unsigned offset,
> +			    int value)
> +{
>  	struct axp20x_gpio *gpio = gpiochip_get_data(chip);
>  	int reg;
>  
>  	reg = axp20x_gpio_get_reg(offset);
>  	if (reg < 0)
> +		return;
> +
> +	regmap_update_bits(gpio->regmap, reg,
> +			   AXP20X_GPIO_FUNCTIONS,
> +			   value ? AXP20X_GPIO_FUNCTION_OUT_HIGH :
> +			   AXP20X_GPIO_FUNCTION_OUT_LOW);
> +}
> +
> +static int axp20x_pmx_set(struct pinctrl_dev *pctldev, unsigned int offset,
> +			  u8 config)
> +{
> +	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
> +	int reg;
> +
> +	reg = axp20x_gpio_get_reg(offset);
> +	if (reg < 0)
>  		return reg;
>  
> -	return regmap_update_bits(gpio->regmap, reg,
> -				  AXP20X_GPIO_FUNCTIONS,
> -				  value ? AXP20X_GPIO_FUNCTION_OUT_HIGH
> -				  : AXP20X_GPIO_FUNCTION_OUT_LOW);
> +	return regmap_update_bits(gpio->regmap, reg, AXP20X_GPIO_FUNCTIONS,
> +				  config);
>  }
>  
> -static void axp20x_gpio_set(struct gpio_chip *chip, unsigned offset,
> -			    int value)
> +static int axp20x_pmx_func_cnt(struct pinctrl_dev *pctldev)
> +{
> +	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return ARRAY_SIZE(gpio->funcs);
> +}
> +
> +static const char *axp20x_pmx_func_name(struct pinctrl_dev *pctldev,
> +					unsigned int selector)
>  {
> -	axp20x_gpio_output(chip, offset, value);
> +	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return gpio->funcs[selector].name;
> +}
> +
> +static int axp20x_pmx_func_groups(struct pinctrl_dev *pctldev,
> +				  unsigned int selector,
> +				  const char * const **groups,
> +				  unsigned int *num_groups)
> +{
> +	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*groups = gpio->funcs[selector].groups;
> +	*num_groups = gpio->funcs[selector].ngroups;
> +
> +	return 0;
> +}
> +
> +static int axp20x_pmx_set_mux(struct pinctrl_dev *pctldev,
> +			      unsigned int function, unsigned int group)
> +{
> +	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
> +	unsigned int mask;
> +
> +	/* Every pin supports GPIO_OUT and GPIO_IN functions */
> +	if (function <= AXP20X_FUNC_GPIO_IN)
> +		return axp20x_pmx_set(pctldev, group,
> +				      gpio->funcs[function].muxval);
> +
> +	if (function == AXP20X_FUNC_LDO)
> +		mask = gpio->desc->ldo_mask;
> +	else
> +		mask = gpio->desc->adc_mask;
> +
> +	if (!(BIT(group) & mask))
> +		return -EINVAL;
> +
> +	return axp20x_pmx_set(pctldev, group, gpio->funcs[function].muxval);
> +}
> +
> +static int axp20x_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
> +					 struct pinctrl_gpio_range *range,
> +					 unsigned int offset, bool input)
> +{
> +	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (input)
> +		return axp20x_pmx_set(pctldev, offset,
> +				      gpio->funcs[AXP20X_FUNC_GPIO_IN].muxval);
> +
> +	return axp20x_pmx_set(pctldev, offset,
> +			      gpio->funcs[AXP20X_FUNC_GPIO_OUT].muxval);
> +}
> +
> +static const struct pinmux_ops axp20x_pmx_ops = {
> +	.get_functions_count	= axp20x_pmx_func_cnt,
> +	.get_function_name	= axp20x_pmx_func_name,
> +	.get_function_groups	= axp20x_pmx_func_groups,
> +	.set_mux		= axp20x_pmx_set_mux,
> +	.gpio_set_direction	= axp20x_pmx_gpio_set_direction,
> +	.strict			= true,
> +};
> +
> +static int axp20x_groups_cnt(struct pinctrl_dev *pctldev)
> +{
> +	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return gpio->desc->npins;
> +}
> +
> +static int axp20x_group_pins(struct pinctrl_dev *pctldev, unsigned int selector,
> +			     const unsigned int **pins, unsigned int *num_pins)
> +{
> +	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*pins = (unsigned int *)&gpio->desc->pins[selector];
> +	*num_pins = 1;
> +
> +	return 0;
> +}
> +
> +static const char *axp20x_group_name(struct pinctrl_dev *pctldev,
> +				     unsigned int selector)
> +{
> +	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return gpio->desc->pins[selector].name;
> +}
> +
> +static const struct pinctrl_ops axp20x_pctrl_ops = {
> +	.dt_node_to_map		= pinconf_generic_dt_node_to_map_group,
> +	.dt_free_map		= pinconf_generic_dt_free_map,
> +	.get_groups_count	= axp20x_groups_cnt,
> +	.get_group_name		= axp20x_group_name,
> +	.get_group_pins		= axp20x_group_pins,
> +};
> +
> +static void axp20x_funcs_groups_from_mask(struct device *dev, unsigned int mask,
> +					  unsigned int mask_len,
> +					  struct axp20x_pinctrl_function *func,
> +					  const struct pinctrl_pin_desc *pins)
> +{
> +	unsigned long int mask_cpy = mask;
> +	const char **group;
> +	unsigned int ngroups = hweight8(mask);
> +	int bit;
> +
> +	func->ngroups = ngroups;
> +	if (func->ngroups > 0) {
> +		func->groups = devm_kzalloc(dev, ngroups * sizeof(const char *),
> +					    GFP_KERNEL);
> +		group = func->groups;
> +		for_each_set_bit(bit, &mask_cpy, mask_len) {
> +			*group = pins[bit].name;
> +			group++;
> +		}
> +	}
> +}
> +
> +static void axp20x_build_funcs_groups(struct platform_device *pdev)
> +{
> +	struct axp20x_gpio *gpio = platform_get_drvdata(pdev);
> +	int i, pin;
> +
> +	gpio->funcs[AXP20X_FUNC_GPIO_OUT].name = "gpio_out";
> +	gpio->funcs[AXP20X_FUNC_GPIO_OUT].muxval = 0x0;
> +	gpio->funcs[AXP20X_FUNC_GPIO_IN].name = "gpio_in";
> +	gpio->funcs[AXP20X_FUNC_GPIO_IN].muxval = 0x2;

Why do you need hexadecimal values here?

> +	gpio->funcs[AXP20X_FUNC_LDO].name = "ldo";
> +	gpio->funcs[AXP20X_FUNC_LDO].muxval = 0x3;
> +	gpio->funcs[AXP20X_FUNC_ADC].name = "adc";
> +	gpio->funcs[AXP20X_FUNC_ADC].muxval = 0x4;

And you probably can define that statically.

> +	/* Every pin supports GPIO_OUT and GPIO_IN functions */
> +	for (i = 0; i <= AXP20X_FUNC_GPIO_IN; i++) {
> +		gpio->funcs[i].ngroups = gpio->desc->npins;
> +		gpio->funcs[i].groups = devm_kzalloc(&pdev->dev,
> +						     gpio->desc->npins * sizeof(const char *),
> +						     GFP_KERNEL);
> +		for (pin = 0; pin < gpio->desc->npins; pin++)
> +			gpio->funcs[i].groups[pin] = gpio->desc->pins[pin].name;
> +	}
> +
> +	axp20x_funcs_groups_from_mask(&pdev->dev, gpio->desc->ldo_mask,
> +				      gpio->desc->npins,
> +				      &gpio->funcs[AXP20X_FUNC_LDO],
> +				      gpio->desc->pins);
> +
> +	axp20x_funcs_groups_from_mask(&pdev->dev, gpio->desc->adc_mask,
> +				      gpio->desc->npins,
> +				      &gpio->funcs[AXP20X_FUNC_ADC],
> +				      gpio->desc->pins);
>  }
>  
>  static int axp20x_gpio_probe(struct platform_device *pdev)
>  {
>  	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>  	struct axp20x_gpio *gpio;
> +	struct pinctrl_desc *pctrl_desc;
>  	int ret;
>  
>  	if (!of_device_is_available(pdev->dev.of_node))
> @@ -144,6 +360,8 @@ static int axp20x_gpio_probe(struct platform_device *pdev)
>  
>  	gpio->chip.base			= -1;
>  	gpio->chip.can_sleep		= true;
> +	gpio->chip.request		= gpiochip_generic_request;
> +	gpio->chip.free			= gpiochip_generic_free;
>  	gpio->chip.parent		= &pdev->dev;
>  	gpio->chip.label		= dev_name(&pdev->dev);
>  	gpio->chip.owner		= THIS_MODULE;
> @@ -154,15 +372,49 @@ static int axp20x_gpio_probe(struct platform_device *pdev)
>  	gpio->chip.direction_output	= axp20x_gpio_output;
>  	gpio->chip.ngpio		= 3;
>  
> +	gpio->desc = &axp20x_data;
> +
>  	gpio->regmap = axp20x->regmap;
>  
> +	gpio->dev = &pdev->dev;
> +

Why do you need new lines between each affectation?

> +	platform_set_drvdata(pdev, gpio);
> +
> +	axp20x_build_funcs_groups(pdev);
> +
> +	pctrl_desc = devm_kzalloc(&pdev->dev, sizeof(*pctrl_desc), GFP_KERNEL);
> +	if (!pctrl_desc)
> +		return -ENOMEM;
> +
> +	pctrl_desc->name = dev_name(&pdev->dev);
> +	pctrl_desc->owner = THIS_MODULE;
> +	pctrl_desc->pins = gpio->desc->pins;
> +	pctrl_desc->npins = gpio->desc->npins;
> +	pctrl_desc->pctlops = &axp20x_pctrl_ops;
> +	pctrl_desc->pmxops = &axp20x_pmx_ops;
> +
> +	gpio->pctl_dev = devm_pinctrl_register(&pdev->dev, pctrl_desc, gpio);
> +	if (IS_ERR(gpio->pctl_dev)) {
> +		dev_err(&pdev->dev, "couldn't register pinctrl driver\n");
> +		return PTR_ERR(gpio->pctl_dev);
> +	}
> +
>  	ret = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to register GPIO chip\n");
>  		return ret;
>  	}
>  
> -	dev_info(&pdev->dev, "AXP209 GPIO driver loaded\n");
> +	ret = gpiochip_add_pin_range(&gpio->chip, dev_name(&pdev->dev),
> +				     gpio->desc->pins->number,
> +				     gpio->desc->pins->number,
> +				     gpio->desc->npins);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add pin range\n");
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "AXP209 pinctrl and GPIO driver loaded\n");
>  
>  	return 0;
>  }
> @@ -184,5 +436,6 @@ static struct platform_driver axp20x_gpio_driver = {
>  module_platform_driver(axp20x_gpio_driver);
>  
>  MODULE_AUTHOR("Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
> -MODULE_DESCRIPTION("AXP20x PMIC GPIO driver");
> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
> +MODULE_DESCRIPTION("AXP20x PMIC pinctrl and GPIO driver");
>  MODULE_LICENSE("GPL");

Looks much better otherwise, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v3 02/12] pinctrl: axp209: add pinctrl features

[Rob Herring]

On Mon, Oct 02, 2017 at 02:08:44PM +0200, Quentin Schulz wrote:
> The X-Powers AXP209 has 3 GPIOs. GPIO0/1 can each act either as a GPIO,
> an ADC or a LDO regulator. GPIO2 can only act as a GPIO.
> 
> This adds the pinctrl features to the driver so GPIO0/1 can be used as
> ADC or LDO regulator.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../devicetree/bindings/pinctrl/pinctrl-axp209.txt |  28 +-

In the future, please split bindings to separate patch.

>  drivers/pinctrl/pinctrl-axp209.c                   | 293 +++++++++++++++++++--
>  2 files changed, 300 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
> index a6611304dd3c..388c04492afd 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
> @@ -1,4 +1,4 @@
> -AXP209 GPIO controller
> +AXP209 GPIO & pinctrl controller
>  
>  This driver follows the usual GPIO bindings found in
>  Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -28,3 +28,29 @@ axp209: pmic@34 {
>  		#gpio-cells = <2>;
>  	};
>  };
> +
> +The GPIOs can be muxed to other functions and therefore, must be a subnode of
> +axp_gpio.
> +
> +Example:
> +
> +&axp_gpio {
> +	gpio0_adc: gpio0_adc {

Use '-' rather than '_' for node names.

With that,

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

> +		pins = "GPIO0";
> +		function = "adc";
> +	};
> +};
> +
> +&example_node {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gpio0_adc>;
> +};
> +
> +GPIOs and their functions
> +-------------------------
> +
> +GPIO	|	Functions
> +------------------------
> +GPIO0	|	gpio_in, gpio_out, ldo, adc
> +GPIO1	|	gpio_in, gpio_out, ldo, adc
> +GPIO2	|	gpio_in, gpio_out

[PATCH v3 03/12] pinctrl: axp209: rename everything from gpio to pctl

[Quentin Schulz]

This driver used to do only GPIO features of the GPIOs in X-Powers
AXP20X. Now that we have migrated everything to the pinctrl subsystem
and added pinctrl features, rename everything related to pinctrl from
gpio to pctl to ease the understanding of differences between GPIO
and pinctrl features.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/pinctrl/pinctrl-axp209.c | 178 +++++++++++++++++++--------------------
 1 file changed, 89 insertions(+), 89 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-axp209.c b/drivers/pinctrl/pinctrl-axp209.c
index 96ef0cc28762..3ddeba45feed 100644
--- a/drivers/pinctrl/pinctrl-axp209.c
+++ b/drivers/pinctrl/pinctrl-axp209.c
@@ -54,7 +54,7 @@ struct axp20x_pinctrl_function {
 	unsigned int	ngroups;
 };
 
-struct axp20x_gpio {
+struct axp20x_pctl {
 	struct gpio_chip	chip;
 	struct regmap		*regmap;
 	struct pinctrl_dev			*pctl_dev;
@@ -97,11 +97,11 @@ static int axp20x_gpio_input(struct gpio_chip *chip, unsigned offset)
 
 static int axp20x_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
-	struct axp20x_gpio *gpio = gpiochip_get_data(chip);
+	struct axp20x_pctl *pctl = gpiochip_get_data(chip);
 	unsigned int val;
 	int ret;
 
-	ret = regmap_read(gpio->regmap, AXP20X_GPIO20_SS, &val);
+	ret = regmap_read(pctl->regmap, AXP20X_GPIO20_SS, &val);
 	if (ret)
 		return ret;
 
@@ -110,7 +110,7 @@ static int axp20x_gpio_get(struct gpio_chip *chip, unsigned offset)
 
 static int axp20x_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 {
-	struct axp20x_gpio *gpio = gpiochip_get_data(chip);
+	struct axp20x_pctl *pctl = gpiochip_get_data(chip);
 	unsigned int val;
 	int reg, ret;
 
@@ -118,7 +118,7 @@ static int axp20x_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 	if (reg < 0)
 		return reg;
 
-	ret = regmap_read(gpio->regmap, reg, &val);
+	ret = regmap_read(pctl->regmap, reg, &val);
 	if (ret)
 		return ret;
 
@@ -148,14 +148,14 @@ static int axp20x_gpio_output(struct gpio_chip *chip, unsigned offset,
 static void axp20x_gpio_set(struct gpio_chip *chip, unsigned offset,
 			    int value)
 {
-	struct axp20x_gpio *gpio = gpiochip_get_data(chip);
+	struct axp20x_pctl *pctl = gpiochip_get_data(chip);
 	int reg;
 
 	reg = axp20x_gpio_get_reg(offset);
 	if (reg < 0)
 		return;
 
-	regmap_update_bits(gpio->regmap, reg,
+	regmap_update_bits(pctl->regmap, reg,
 			   AXP20X_GPIO_FUNCTIONS,
 			   value ? AXP20X_GPIO_FUNCTION_OUT_HIGH :
 			   AXP20X_GPIO_FUNCTION_OUT_LOW);
@@ -164,30 +164,30 @@ static void axp20x_gpio_set(struct gpio_chip *chip, unsigned offset,
 static int axp20x_pmx_set(struct pinctrl_dev *pctldev, unsigned int offset,
 			  u8 config)
 {
-	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+	struct axp20x_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
 	int reg;
 
 	reg = axp20x_gpio_get_reg(offset);
 	if (reg < 0)
 		return reg;
 
-	return regmap_update_bits(gpio->regmap, reg, AXP20X_GPIO_FUNCTIONS,
+	return regmap_update_bits(pctl->regmap, reg, AXP20X_GPIO_FUNCTIONS,
 				  config);
 }
 
 static int axp20x_pmx_func_cnt(struct pinctrl_dev *pctldev)
 {
-	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+	struct axp20x_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
 
-	return ARRAY_SIZE(gpio->funcs);
+	return ARRAY_SIZE(pctl->funcs);
 }
 
 static const char *axp20x_pmx_func_name(struct pinctrl_dev *pctldev,
 					unsigned int selector)
 {
-	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+	struct axp20x_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
 
-	return gpio->funcs[selector].name;
+	return pctl->funcs[selector].name;
 }
 
 static int axp20x_pmx_func_groups(struct pinctrl_dev *pctldev,
@@ -195,10 +195,10 @@ static int axp20x_pmx_func_groups(struct pinctrl_dev *pctldev,
 				  const char * const **groups,
 				  unsigned int *num_groups)
 {
-	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+	struct axp20x_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
 
-	*groups = gpio->funcs[selector].groups;
-	*num_groups = gpio->funcs[selector].ngroups;
+	*groups = pctl->funcs[selector].groups;
+	*num_groups = pctl->funcs[selector].ngroups;
 
 	return 0;
 }
@@ -206,37 +206,37 @@ static int axp20x_pmx_func_groups(struct pinctrl_dev *pctldev,
 static int axp20x_pmx_set_mux(struct pinctrl_dev *pctldev,
 			      unsigned int function, unsigned int group)
 {
-	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+	struct axp20x_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
 	unsigned int mask;
 
 	/* Every pin supports GPIO_OUT and GPIO_IN functions */
 	if (function <= AXP20X_FUNC_GPIO_IN)
 		return axp20x_pmx_set(pctldev, group,
-				      gpio->funcs[function].muxval);
+				      pctl->funcs[function].muxval);
 
 	if (function == AXP20X_FUNC_LDO)
-		mask = gpio->desc->ldo_mask;
+		mask = pctl->desc->ldo_mask;
 	else
-		mask = gpio->desc->adc_mask;
+		mask = pctl->desc->adc_mask;
 
 	if (!(BIT(group) & mask))
 		return -EINVAL;
 
-	return axp20x_pmx_set(pctldev, group, gpio->funcs[function].muxval);
+	return axp20x_pmx_set(pctldev, group, pctl->funcs[function].muxval);
 }
 
 static int axp20x_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 					 struct pinctrl_gpio_range *range,
 					 unsigned int offset, bool input)
 {
-	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+	struct axp20x_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
 
 	if (input)
 		return axp20x_pmx_set(pctldev, offset,
-				      gpio->funcs[AXP20X_FUNC_GPIO_IN].muxval);
+				      pctl->funcs[AXP20X_FUNC_GPIO_IN].muxval);
 
 	return axp20x_pmx_set(pctldev, offset,
-			      gpio->funcs[AXP20X_FUNC_GPIO_OUT].muxval);
+			      pctl->funcs[AXP20X_FUNC_GPIO_OUT].muxval);
 }
 
 static const struct pinmux_ops axp20x_pmx_ops = {
@@ -250,17 +250,17 @@ static const struct pinmux_ops axp20x_pmx_ops = {
 
 static int axp20x_groups_cnt(struct pinctrl_dev *pctldev)
 {
-	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+	struct axp20x_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
 
-	return gpio->desc->npins;
+	return pctl->desc->npins;
 }
 
 static int axp20x_group_pins(struct pinctrl_dev *pctldev, unsigned int selector,
 			     const unsigned int **pins, unsigned int *num_pins)
 {
-	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+	struct axp20x_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
 
-	*pins = (unsigned int *)&gpio->desc->pins[selector];
+	*pins = (unsigned int *)&pctl->desc->pins[selector];
 	*num_pins = 1;
 
 	return 0;
@@ -269,9 +269,9 @@ static int axp20x_group_pins(struct pinctrl_dev *pctldev, unsigned int selector,
 static const char *axp20x_group_name(struct pinctrl_dev *pctldev,
 				     unsigned int selector)
 {
-	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
+	struct axp20x_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
 
-	return gpio->desc->pins[selector].name;
+	return pctl->desc->pins[selector].name;
 }
 
 static const struct pinctrl_ops axp20x_pctrl_ops = {
@@ -306,43 +306,43 @@ static void axp20x_funcs_groups_from_mask(struct device *dev, unsigned int mask,
 
 static void axp20x_build_funcs_groups(struct platform_device *pdev)
 {
-	struct axp20x_gpio *gpio = platform_get_drvdata(pdev);
+	struct axp20x_pctl *pctl = platform_get_drvdata(pdev);
 	int i, pin;
 
-	gpio->funcs[AXP20X_FUNC_GPIO_OUT].name = "gpio_out";
-	gpio->funcs[AXP20X_FUNC_GPIO_OUT].muxval = 0x0;
-	gpio->funcs[AXP20X_FUNC_GPIO_IN].name = "gpio_in";
-	gpio->funcs[AXP20X_FUNC_GPIO_IN].muxval = 0x2;
-	gpio->funcs[AXP20X_FUNC_LDO].name = "ldo";
-	gpio->funcs[AXP20X_FUNC_LDO].muxval = 0x3;
-	gpio->funcs[AXP20X_FUNC_ADC].name = "adc";
-	gpio->funcs[AXP20X_FUNC_ADC].muxval = 0x4;
+	pctl->funcs[AXP20X_FUNC_GPIO_OUT].name = "gpio_out";
+	pctl->funcs[AXP20X_FUNC_GPIO_OUT].muxval = 0x0;
+	pctl->funcs[AXP20X_FUNC_GPIO_IN].name = "gpio_in";
+	pctl->funcs[AXP20X_FUNC_GPIO_IN].muxval = 0x2;
+	pctl->funcs[AXP20X_FUNC_LDO].name = "ldo";
+	pctl->funcs[AXP20X_FUNC_LDO].muxval = 0x3;
+	pctl->funcs[AXP20X_FUNC_ADC].name = "adc";
+	pctl->funcs[AXP20X_FUNC_ADC].muxval = 0x4;
 
 	/* Every pin supports GPIO_OUT and GPIO_IN functions */
 	for (i = 0; i <= AXP20X_FUNC_GPIO_IN; i++) {
-		gpio->funcs[i].ngroups = gpio->desc->npins;
-		gpio->funcs[i].groups = devm_kzalloc(&pdev->dev,
-						     gpio->desc->npins * sizeof(const char *),
+		pctl->funcs[i].ngroups = pctl->desc->npins;
+		pctl->funcs[i].groups = devm_kzalloc(&pdev->dev,
+						     pctl->desc->npins * sizeof(const char *),
 						     GFP_KERNEL);
-		for (pin = 0; pin < gpio->desc->npins; pin++)
-			gpio->funcs[i].groups[pin] = gpio->desc->pins[pin].name;
+		for (pin = 0; pin < pctl->desc->npins; pin++)
+			pctl->funcs[i].groups[pin] = pctl->desc->pins[pin].name;
 	}
 
-	axp20x_funcs_groups_from_mask(&pdev->dev, gpio->desc->ldo_mask,
-				      gpio->desc->npins,
-				      &gpio->funcs[AXP20X_FUNC_LDO],
-				      gpio->desc->pins);
+	axp20x_funcs_groups_from_mask(&pdev->dev, pctl->desc->ldo_mask,
+				      pctl->desc->npins,
+				      &pctl->funcs[AXP20X_FUNC_LDO],
+				      pctl->desc->pins);
 
-	axp20x_funcs_groups_from_mask(&pdev->dev, gpio->desc->adc_mask,
-				      gpio->desc->npins,
-				      &gpio->funcs[AXP20X_FUNC_ADC],
-				      gpio->desc->pins);
+	axp20x_funcs_groups_from_mask(&pdev->dev, pctl->desc->adc_mask,
+				      pctl->desc->npins,
+				      &pctl->funcs[AXP20X_FUNC_ADC],
+				      pctl->desc->pins);
 }
 
-static int axp20x_gpio_probe(struct platform_device *pdev)
+static int axp20x_pctl_probe(struct platform_device *pdev)
 {
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
-	struct axp20x_gpio *gpio;
+	struct axp20x_pctl *pctl;
 	struct pinctrl_desc *pctrl_desc;
 	int ret;
 
@@ -354,31 +354,31 @@ static int axp20x_gpio_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
-	if (!gpio)
+	pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
+	if (!pctl)
 		return -ENOMEM;
 
-	gpio->chip.base			= -1;
-	gpio->chip.can_sleep		= true;
-	gpio->chip.request		= gpiochip_generic_request;
-	gpio->chip.free			= gpiochip_generic_free;
-	gpio->chip.parent		= &pdev->dev;
-	gpio->chip.label		= dev_name(&pdev->dev);
-	gpio->chip.owner		= THIS_MODULE;
-	gpio->chip.get			= axp20x_gpio_get;
-	gpio->chip.get_direction	= axp20x_gpio_get_direction;
-	gpio->chip.set			= axp20x_gpio_set;
-	gpio->chip.direction_input	= axp20x_gpio_input;
-	gpio->chip.direction_output	= axp20x_gpio_output;
-	gpio->chip.ngpio		= 3;
+	pctl->chip.base			= -1;
+	pctl->chip.can_sleep		= true;
+	pctl->chip.request		= gpiochip_generic_request;
+	pctl->chip.free			= gpiochip_generic_free;
+	pctl->chip.parent		= &pdev->dev;
+	pctl->chip.label		= dev_name(&pdev->dev);
+	pctl->chip.owner		= THIS_MODULE;
+	pctl->chip.get			= axp20x_gpio_get;
+	pctl->chip.get_direction	= axp20x_gpio_get_direction;
+	pctl->chip.set			= axp20x_gpio_set;
+	pctl->chip.direction_input	= axp20x_gpio_input;
+	pctl->chip.direction_output	= axp20x_gpio_output;
+	pctl->chip.ngpio		= 3;
 
-	gpio->desc = &axp20x_data;
+	pctl->desc = &axp20x_data;
 
-	gpio->regmap = axp20x->regmap;
+	pctl->regmap = axp20x->regmap;
 
-	gpio->dev = &pdev->dev;
+	pctl->dev = &pdev->dev;
 
-	platform_set_drvdata(pdev, gpio);
+	platform_set_drvdata(pdev, pctl);
 
 	axp20x_build_funcs_groups(pdev);
 
@@ -388,27 +388,27 @@ static int axp20x_gpio_probe(struct platform_device *pdev)
 
 	pctrl_desc->name = dev_name(&pdev->dev);
 	pctrl_desc->owner = THIS_MODULE;
-	pctrl_desc->pins = gpio->desc->pins;
-	pctrl_desc->npins = gpio->desc->npins;
+	pctrl_desc->pins = pctl->desc->pins;
+	pctrl_desc->npins = pctl->desc->npins;
 	pctrl_desc->pctlops = &axp20x_pctrl_ops;
 	pctrl_desc->pmxops = &axp20x_pmx_ops;
 
-	gpio->pctl_dev = devm_pinctrl_register(&pdev->dev, pctrl_desc, gpio);
-	if (IS_ERR(gpio->pctl_dev)) {
+	pctl->pctl_dev = devm_pinctrl_register(&pdev->dev, pctrl_desc, pctl);
+	if (IS_ERR(pctl->pctl_dev)) {
 		dev_err(&pdev->dev, "couldn't register pinctrl driver\n");
-		return PTR_ERR(gpio->pctl_dev);
+		return PTR_ERR(pctl->pctl_dev);
 	}
 
-	ret = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
+	ret = devm_gpiochip_add_data(&pdev->dev, &pctl->chip, pctl);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to register GPIO chip\n");
 		return ret;
 	}
 
-	ret = gpiochip_add_pin_range(&gpio->chip, dev_name(&pdev->dev),
-				     gpio->desc->pins->number,
-				     gpio->desc->pins->number,
-				     gpio->desc->npins);
+	ret = gpiochip_add_pin_range(&pctl->chip, dev_name(&pdev->dev),
+				     pctl->desc->pins->number,
+				     pctl->desc->pins->number,
+				     pctl->desc->npins);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to add pin range\n");
 		return ret;
@@ -419,21 +419,21 @@ static int axp20x_gpio_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id axp20x_gpio_match[] = {
+static const struct of_device_id axp20x_pctl_match[] = {
 	{ .compatible = "x-powers,axp209-gpio" },
 	{ }
 };
-MODULE_DEVICE_TABLE(of, axp20x_gpio_match);
+MODULE_DEVICE_TABLE(of, axp20x_pctl_match);
 
-static struct platform_driver axp20x_gpio_driver = {
-	.probe		= axp20x_gpio_probe,
+static struct platform_driver axp20x_pctl_driver = {
+	.probe		= axp20x_pctl_probe,
 	.driver = {
 		.name		= "axp20x-gpio",
-		.of_match_table	= axp20x_gpio_match,
+		.of_match_table	= axp20x_pctl_match,
 	},
 };
 
-module_platform_driver(axp20x_gpio_driver);
+module_platform_driver(axp20x_pctl_driver);
 
 MODULE_AUTHOR("Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
 MODULE_AUTHOR("Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
-- 
2.11.0

[PATCH v3 04/12] pinctrl: axp209: add programmable gpio_status_offset

[Quentin Schulz]

To prepare for patches that will add support for a new PMIC that has a
different GPIO input status register, add a gpio_status_offset within
axp20x_pctl structure and use it.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/pinctrl/pinctrl-axp209.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-axp209.c b/drivers/pinctrl/pinctrl-axp209.c
index 3ddeba45feed..17146496b22a 100644
--- a/drivers/pinctrl/pinctrl-axp209.c
+++ b/drivers/pinctrl/pinctrl-axp209.c
@@ -45,6 +45,7 @@ struct axp20x_pctrl_desc {
 	unsigned int			ldo_mask;
 	/* Stores the pins supporting ADC function. Bit offset is pin number. */
 	unsigned int			adc_mask;
+	unsigned int			gpio_status_offset;
 };
 
 struct axp20x_pinctrl_function {
@@ -74,6 +75,7 @@ static const struct axp20x_pctrl_desc axp20x_data = {
 	.npins	= ARRAY_SIZE(axp209_pins),
 	.ldo_mask = BIT(0) | BIT(1),
 	.adc_mask = BIT(0) | BIT(1),
+	.gpio_status_offset = 4,
 };
 
 static int axp20x_gpio_get_reg(unsigned offset)
@@ -105,7 +107,7 @@ static int axp20x_gpio_get(struct gpio_chip *chip, unsigned offset)
 	if (ret)
 		return ret;
 
-	return !!(val & BIT(offset + 4));
+	return !!(val & BIT(offset + pctl->desc->gpio_status_offset));
 }
 
 static int axp20x_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
-- 
2.11.0

Re: [PATCH v3 04/12] pinctrl: axp209: add programmable gpio_status_offset

[Maxime Ripard]

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

On Mon, Oct 02, 2017 at 12:08:46PM +0000, Quentin Schulz wrote:
> To prepare for patches that will add support for a new PMIC that has a
> different GPIO input status register, add a gpio_status_offset within
> axp20x_pctl structure and use it.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v3 04/12] pinctrl: axp209: add programmable gpio_status_offset

[Chen-Yu Tsai]

On Tue, Oct 3, 2017 at 4:38 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Mon, Oct 02, 2017 at 12:08:46PM +0000, Quentin Schulz wrote:
>> To prepare for patches that will add support for a new PMIC that has a
>> different GPIO input status register, add a gpio_status_offset within
>> axp20x_pctl structure and use it.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>
> Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

[PATCH v3 05/12] pinctrl: axp209: add support for AXP813 GPIOs

[Quentin Schulz]

The AXP813 has only two GPIOs. GPIO0 can either be used as a GPIO, an
LDO regulator or an ADC. GPIO1 can be used either as a GPIO or an LDO
regulator.

Moreover, the status bit of the GPIOs when in input mode is not offset
by 4 unlike the AXP209.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 .../devicetree/bindings/pinctrl/pinctrl-axp209.txt | 13 ++++++++-
 drivers/pinctrl/pinctrl-axp209.c                   | 32 ++++++++++++++++------
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
index 388c04492afd..a4e4dbef65d6 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
@@ -4,7 +4,9 @@ This driver follows the usual GPIO bindings found in
 Documentation/devicetree/bindings/gpio/gpio.txt
 
 Required properties:
-- compatible: Should be "x-powers,axp209-gpio"
+- compatible: Should be one of:
+	- "x-powers,axp209-gpio"
+	- "x-powers,axp813-gpio"
 - #gpio-cells: Should be two. The first cell is the pin number and the
   second is the GPIO flags.
 - gpio-controller: Marks the device node as a GPIO controller.
@@ -49,8 +51,17 @@ Example:
 GPIOs and their functions
 -------------------------
 
+axp209
+------
 GPIO	|	Functions
 ------------------------
 GPIO0	|	gpio_in, gpio_out, ldo, adc
 GPIO1	|	gpio_in, gpio_out, ldo, adc
 GPIO2	|	gpio_in, gpio_out
+
+axp813
+------
+GPIO	|	Functions
+------------------------
+GPIO0	|	gpio_in, gpio_out, ldo, adc
+GPIO1	|	gpio_in, gpio_out, ldo
diff --git a/drivers/pinctrl/pinctrl-axp209.c b/drivers/pinctrl/pinctrl-axp209.c
index 17146496b22a..4466b2541137 100644
--- a/drivers/pinctrl/pinctrl-axp209.c
+++ b/drivers/pinctrl/pinctrl-axp209.c
@@ -19,6 +19,7 @@
 #include <linux/mfd/axp20x.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
@@ -70,6 +71,11 @@ static const struct pinctrl_pin_desc axp209_pins[] = {
 	PINCTRL_PIN(2, "GPIO2"),
 };
 
+static const struct pinctrl_pin_desc axp813_pins[] = {
+	PINCTRL_PIN(0, "GPIO0"),
+	PINCTRL_PIN(1, "GPIO1"),
+};
+
 static const struct axp20x_pctrl_desc axp20x_data = {
 	.pins	= axp209_pins,
 	.npins	= ARRAY_SIZE(axp209_pins),
@@ -78,6 +84,14 @@ static const struct axp20x_pctrl_desc axp20x_data = {
 	.gpio_status_offset = 4,
 };
 
+static const struct axp20x_pctrl_desc axp813_data = {
+	.pins	= axp813_pins,
+	.npins	= ARRAY_SIZE(axp813_pins),
+	.ldo_mask = BIT(0) | BIT(1),
+	.adc_mask = BIT(0),
+	.gpio_status_offset = 0,
+};
+
 static int axp20x_gpio_get_reg(unsigned offset)
 {
 	switch (offset) {
@@ -341,10 +355,18 @@ static void axp20x_build_funcs_groups(struct platform_device *pdev)
 				      pctl->desc->pins);
 }
 
+static const struct of_device_id axp20x_pctl_match[] = {
+	{ .compatible = "x-powers,axp209-gpio", .data = &axp20x_data, },
+	{ .compatible = "x-powers,axp813-gpio", .data = &axp813_data, },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, axp20x_pctl_match);
+
 static int axp20x_pctl_probe(struct platform_device *pdev)
 {
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
 	struct axp20x_pctl *pctl;
+	struct device *dev = &pdev->dev;
 	struct pinctrl_desc *pctrl_desc;
 	int ret;
 
@@ -372,9 +394,9 @@ static int axp20x_pctl_probe(struct platform_device *pdev)
 	pctl->chip.set			= axp20x_gpio_set;
 	pctl->chip.direction_input	= axp20x_gpio_input;
 	pctl->chip.direction_output	= axp20x_gpio_output;
-	pctl->chip.ngpio		= 3;
+	pctl->chip.ngpio		= pctl->desc->npins;
 
-	pctl->desc = &axp20x_data;
+	pctl->desc = (struct axp20x_pctrl_desc *)of_device_get_match_data(dev);
 
 	pctl->regmap = axp20x->regmap;
 
@@ -421,12 +443,6 @@ static int axp20x_pctl_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id axp20x_pctl_match[] = {
-	{ .compatible = "x-powers,axp209-gpio" },
-	{ }
-};
-MODULE_DEVICE_TABLE(of, axp20x_pctl_match);
-
 static struct platform_driver axp20x_pctl_driver = {
 	.probe		= axp20x_pctl_probe,
 	.driver = {
-- 
2.11.0

Re: [PATCH v3 05/12] pinctrl: axp209: add support for AXP813 GPIOs

[Maxime Ripard]

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

On Mon, Oct 02, 2017 at 12:08:47PM +0000, Quentin Schulz wrote:
> The AXP813 has only two GPIOs. GPIO0 can either be used as a GPIO, an
> LDO regulator or an ADC. GPIO1 can be used either as a GPIO or an LDO
> regulator.
> 
> Moreover, the status bit of the GPIOs when in input mode is not offset
> by 4 unlike the AXP209.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v3 05/12] pinctrl: axp209: add support for AXP813 GPIOs

[Rob Herring]

On Mon, Oct 02, 2017 at 02:08:47PM +0200, Quentin Schulz wrote:
> The AXP813 has only two GPIOs. GPIO0 can either be used as a GPIO, an
> LDO regulator or an ADC. GPIO1 can be used either as a GPIO or an LDO
> regulator.
> 
> Moreover, the status bit of the GPIOs when in input mode is not offset
> by 4 unlike the AXP209.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../devicetree/bindings/pinctrl/pinctrl-axp209.txt | 13 ++++++++-

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

>  drivers/pinctrl/pinctrl-axp209.c                   | 32 ++++++++++++++++------
>  2 files changed, 36 insertions(+), 9 deletions(-)

[PATCH v3 06/12] mfd: axp20x: add pinctrl cell for AXP813

[Quentin Schulz]

As GPIO/pinctrl driver now supports AXP813, add a cell for it.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/mfd/axp20x.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 2468b431bb22..d8c92fbbd170 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -878,6 +878,9 @@ static struct mfd_cell axp813_cells[] = {
 		.resources		= axp803_pek_resources,
 	}, {
 		.name			= "axp20x-regulator",
+	}, {
+		.name			= "axp20x-gpio",
+		.of_compatible		= "x-powers,axp813-gpio",
 	}
 };
 
-- 
2.11.0

Re: [PATCH v3 06/12] mfd: axp20x: add pinctrl cell for AXP813

[Maxime Ripard]

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

On Mon, Oct 02, 2017 at 12:08:48PM +0000, Quentin Schulz wrote:
> As GPIO/pinctrl driver now supports AXP813, add a cell for it.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v3 07/12] ARM: dts: sun8i: a711: include axp81x dtsi

[Quentin Schulz]

This board has an AXP813 PMIC so let's include its dtsi.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
index 723641f56a74..4f4db07ca19f 100644
--- a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
@@ -146,7 +146,7 @@
 &r_rsb {
 	status = "okay";
 
-	axp813: pmic@3a3 {
+	axp81x: pmic@3a3 {
 		compatible = "x-powers,axp813";
 		reg = <0x3a3>;
 		interrupt-parent = <&r_intc>;
@@ -179,6 +179,8 @@
 
 };
 
+#include "axp81x.dtsi"
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pb_pins>;
-- 
2.11.0

Re: [PATCH v3 07/12] ARM: dts: sun8i: a711: include axp81x dtsi

[Maxime Ripard]

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

On Mon, Oct 02, 2017 at 12:08:49PM +0000, Quentin Schulz wrote:
> This board has an AXP813 PMIC so let's include its dtsi.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
> index 723641f56a74..4f4db07ca19f 100644
> --- a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
> +++ b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
> @@ -146,7 +146,7 @@
>  &r_rsb {
>  	status = "okay";
>  
> -	axp813: pmic@3a3 {
> +	axp81x: pmic@3a3 {

Aren't we going to have an axp813 DTSI? What's your plan here

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v3 08/12] ARM: dts: sun8i: bananapi-m3: include axp81x dtsi

[Quentin Schulz]

This board has an AXP813 PMIC so let's include its dtsi.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
index 2bafd7e99ef7..bf8f3b9a65dd 100644
--- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
@@ -123,6 +123,8 @@
 	};
 };
 
+#include "axp81x.dtsi"
+
 &reg_usb1_vbus {
 	gpio = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* PD24 */
 	status = "okay";
-- 
2.11.0

[PATCH v3 09/12] ARM: dts: sun8i: h8homlet-v2: include axp81x dtsi

[Quentin Schulz]

This board has an AXP818 PMIC so let's include its dtsi.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/sun8i-a83t-allwinner-h8homlet-v2.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t-allwinner-h8homlet-v2.dts b/arch/arm/boot/dts/sun8i-a83t-allwinner-h8homlet-v2.dts
index 1f0d60afb25b..222a983c3c6d 100644
--- a/arch/arm/boot/dts/sun8i-a83t-allwinner-h8homlet-v2.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-allwinner-h8homlet-v2.dts
@@ -131,6 +131,8 @@
 	};
 };
 
+#include "axp81x.dtsi"
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pb_pins>;
-- 
2.11.0

[PATCH v3 10/12] ARM: dts: sun8i: cubietruck-plus: include axp81x dtsi

[Quentin Schulz]

This board has an AXP818 PMIC so let's include its dtsi.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/sun8i-a83t-cubietruck-plus.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t-cubietruck-plus.dts b/arch/arm/boot/dts/sun8i-a83t-cubietruck-plus.dts
index 716a205c6dbb..5bef16d949f6 100644
--- a/arch/arm/boot/dts/sun8i-a83t-cubietruck-plus.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-cubietruck-plus.dts
@@ -179,6 +179,8 @@
 	};
 };
 
+#include "axp81x.dtsi"
+
 &reg_usb1_vbus {
 	gpio = <&pio 3 29 GPIO_ACTIVE_HIGH>; /* PD29 */
 	status = "okay";
-- 
2.11.0

[PATCH v3 11/12] ARM: dtsi: axp81x: add GPIO DT node

[Quentin Schulz]

This adds DT node for the GPIO/pinctrl part present in AXP813/AXP818.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/axp81x.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi
index 73b761f850c5..f90f257130d5 100644
--- a/arch/arm/boot/dts/axp81x.dtsi
+++ b/arch/arm/boot/dts/axp81x.dtsi
@@ -48,6 +48,12 @@
 	interrupt-controller;
 	#interrupt-cells = <1>;
 
+	axp_gpio: axp_gpio {
+		compatible = "x-powers,axp813-gpio";
+		gpio-controller;
+		#gpio-cells = <2>;
+	};
+
 	regulators {
 		/* Default work frequency for buck regulators */
 		x-powers,dcdc-freq = <3000>;
-- 
2.11.0

[PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Quentin Schulz]

On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively)
ldo_io0 and ldo_io1.

Let's add the pinctrl properties to the said regulators.

Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/axp81x.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi
index f90f257130d5..099b0ddc1bbb 100644
--- a/arch/arm/boot/dts/axp81x.dtsi
+++ b/arch/arm/boot/dts/axp81x.dtsi
@@ -52,6 +52,16 @@
 		compatible = "x-powers,axp813-gpio";
 		gpio-controller;
 		#gpio-cells = <2>;
+
+		gpio0_ldo: gpio0_ldo {
+			pins = "GPIO0";
+			function = "ldo";
+		};
+
+		gpio1_ldo: gpio1_ldo {
+			pins = "GPIO1";
+			function = "ldo";
+		};
 	};
 
 	regulators {
@@ -119,11 +129,15 @@
 		};
 
 		reg_ldo_io0: ldo-io0 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&gpio0_ldo>;
 			/* Disable by default to avoid conflicts with GPIO */
 			status = "disabled";
 		};
 
 		reg_ldo_io1: ldo-io1 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&gpio1_ldo>;
 			/* Disable by default to avoid conflicts with GPIO */
 			status = "disabled";
 		};
-- 
2.11.0

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Maxime Ripard]

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

On Mon, Oct 02, 2017 at 12:08:54PM +0000, Quentin Schulz wrote:
> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively)
> ldo_io0 and ldo_io1.
> 
> Let's add the pinctrl properties to the said regulators.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
>  arch/arm/boot/dts/axp81x.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi
> index f90f257130d5..099b0ddc1bbb 100644
> --- a/arch/arm/boot/dts/axp81x.dtsi
> +++ b/arch/arm/boot/dts/axp81x.dtsi
> @@ -52,6 +52,16 @@
>  		compatible = "x-powers,axp813-gpio";
>  		gpio-controller;
>  		#gpio-cells = <2>;
> +
> +		gpio0_ldo: gpio0_ldo {
> +			pins = "GPIO0";
> +			function = "ldo";
> +		};
> +
> +		gpio1_ldo: gpio1_ldo {
> +			pins = "GPIO1";
> +			function = "ldo";
> +		};

The node names are not supposed to contain any hyphens.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Chen-Yu Tsai]

On Tue, Oct 3, 2017 at 4:42 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Mon, Oct 02, 2017 at 12:08:54PM +0000, Quentin Schulz wrote:
>> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively)
>> ldo_io0 and ldo_io1.
>>
>> Let's add the pinctrl properties to the said regulators.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/axp81x.dtsi | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi
>> index f90f257130d5..099b0ddc1bbb 100644
>> --- a/arch/arm/boot/dts/axp81x.dtsi
>> +++ b/arch/arm/boot/dts/axp81x.dtsi
>> @@ -52,6 +52,16 @@
>>               compatible = "x-powers,axp813-gpio";
>>               gpio-controller;
>>               #gpio-cells = <2>;
>> +
>> +             gpio0_ldo: gpio0_ldo {
>> +                     pins = "GPIO0";
>> +                     function = "ldo";
>> +             };
>> +
>> +             gpio1_ldo: gpio1_ldo {
>> +                     pins = "GPIO1";
>> +                     function = "ldo";
>> +             };
>
> The node names are not supposed to contain any hyphens.

Hmm, I was under the impression that hyphens were preferred in
node names, and a warning would be added to dtc later on.
I might be wrong though.

ChenYu

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Russell King - ARM Linux]

On Tue, Oct 03, 2017 at 10:06:29AM +0800, Chen-Yu Tsai wrote:
> On Tue, Oct 3, 2017 at 4:42 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Mon, Oct 02, 2017 at 12:08:54PM +0000, Quentin Schulz wrote:
> >> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively)
> >> ldo_io0 and ldo_io1.
> >>
> >> Let's add the pinctrl properties to the said regulators.
> >>
> >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> >> ---
> >>  arch/arm/boot/dts/axp81x.dtsi | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi
> >> index f90f257130d5..099b0ddc1bbb 100644
> >> --- a/arch/arm/boot/dts/axp81x.dtsi
> >> +++ b/arch/arm/boot/dts/axp81x.dtsi
> >> @@ -52,6 +52,16 @@
> >>               compatible = "x-powers,axp813-gpio";
> >>               gpio-controller;
> >>               #gpio-cells = <2>;
> >> +
> >> +             gpio0_ldo: gpio0_ldo {
> >> +                     pins = "GPIO0";
> >> +                     function = "ldo";
> >> +             };
> >> +
> >> +             gpio1_ldo: gpio1_ldo {
> >> +                     pins = "GPIO1";
> >> +                     function = "ldo";
> >> +             };
> >
> > The node names are not supposed to contain any hyphens.
> 
> Hmm, I was under the impression that hyphens were preferred in
> node names, and a warning would be added to dtc later on.
> I might be wrong though.

I think there's a terminology issue here.

"-" is a hyphen or minus sign.
"_" is an underscore.

Underscores are not supposed to be used for node names, instead hyphens
are preferred.  I think Maxime means "underscore".

Here's the list from dtc/checks.c:

#define LOWERCASE       "abcdefghijklmnopqrstuvwxyz"
#define UPPERCASE       "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
#define DIGITS          "0123456789"
#define PROPNODECHARS   LOWERCASE UPPERCASE DIGITS ",._+*#?-"
#define PROPNODECHARSSTRICT     LOWERCASE UPPERCASE DIGITS ",-"

If strict mode is enabled, use of any of "._+#?" in the node name will
produce a warning.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Maxime Ripard]

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

On Tue, Oct 03, 2017 at 09:18:37AM +0000, Russell King - ARM Linux wrote:
> On Tue, Oct 03, 2017 at 10:06:29AM +0800, Chen-Yu Tsai wrote:
> > On Tue, Oct 3, 2017 at 4:42 AM, Maxime Ripard
> > <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > > On Mon, Oct 02, 2017 at 12:08:54PM +0000, Quentin Schulz wrote:
> > >> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively)
> > >> ldo_io0 and ldo_io1.
> > >>
> > >> Let's add the pinctrl properties to the said regulators.
> > >>
> > >> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > >> ---
> > >>  arch/arm/boot/dts/axp81x.dtsi | 14 ++++++++++++++
> > >>  1 file changed, 14 insertions(+)
> > >>
> > >> diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi
> > >> index f90f257130d5..099b0ddc1bbb 100644
> > >> --- a/arch/arm/boot/dts/axp81x.dtsi
> > >> +++ b/arch/arm/boot/dts/axp81x.dtsi
> > >> @@ -52,6 +52,16 @@
> > >>               compatible = "x-powers,axp813-gpio";
> > >>               gpio-controller;
> > >>               #gpio-cells = <2>;
> > >> +
> > >> +             gpio0_ldo: gpio0_ldo {
> > >> +                     pins = "GPIO0";
> > >> +                     function = "ldo";
> > >> +             };
> > >> +
> > >> +             gpio1_ldo: gpio1_ldo {
> > >> +                     pins = "GPIO1";
> > >> +                     function = "ldo";
> > >> +             };
> > >
> > > The node names are not supposed to contain any hyphens.
> > 
> > Hmm, I was under the impression that hyphens were preferred in
> > node names, and a warning would be added to dtc later on.
> > I might be wrong though.
> 
> I think there's a terminology issue here.
> 
> "-" is a hyphen or minus sign.
> "_" is an underscore.
> 
> Underscores are not supposed to be used for node names, instead hyphens
> are preferred.  I think Maxime means "underscore".
> 
> Here's the list from dtc/checks.c:
> 
> #define LOWERCASE       "abcdefghijklmnopqrstuvwxyz"
> #define UPPERCASE       "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> #define DIGITS          "0123456789"
> #define PROPNODECHARS   LOWERCASE UPPERCASE DIGITS ",._+*#?-"
> #define PROPNODECHARSSTRICT     LOWERCASE UPPERCASE DIGITS ",-"
> 
> If strict mode is enabled, use of any of "._+#?" in the node name will
> produce a warning.

Right, sorry, I meant underscores..

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Linus Walleij]

On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz
<quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively)
> ldo_io0 and ldo_io1.
(...)
> +               gpio0_ldo: gpio0_ldo {
> +                       pins = "GPIO0";
> +                       function = "ldo";
> +               };
(...)
> +                       pinctrl-names = "default";
> +                       pinctrl-0 = <&gpio0_ldo>;
>                         /* Disable by default to avoid conflicts with GPIO */
>                         status = "disabled";

So this is still by default disabled, but you make the default
mode something called "ldo".

And I think that is to be understood as a low-dropout regulator?

So is the idea that this should be represented as a regulator
in the end?

Then I think the state name should not be "default" rather
something like "regulator" and "default" should be the GPIO
mode, as I guess something like that exists.

Activating a regulator using pin control "default" mode is
not very pretty. It would probably be unintuitive and end
up wasting power because people will get confused about
what is going on.

Instead, call this state "regulator" and when using, in Linux
create a regulator device that set the pin into "regulator" state to
start using it as a LDO, and "default" to deactivate it as
LDO, if that is how the usage is intended.

Yours,
Linus Walleij

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Maxime Ripard]

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

Hi Linus,

On Tue, Oct 03, 2017 at 09:27:17AM +0000, Linus Walleij wrote:
> On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz
> <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> 
> > On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively)
> > ldo_io0 and ldo_io1.
> (...)
> > +               gpio0_ldo: gpio0_ldo {
> > +                       pins = "GPIO0";
> > +                       function = "ldo";
> > +               };
> (...)
> > +                       pinctrl-names = "default";
> > +                       pinctrl-0 = <&gpio0_ldo>;
> >                         /* Disable by default to avoid conflicts with GPIO */
> >                         status = "disabled";
> 
> So this is still by default disabled, but you make the default
> mode something called "ldo".
> 
> And I think that is to be understood as a low-dropout regulator?
> 
> So is the idea that this should be represented as a regulator
> in the end?
> 
> Then I think the state name should not be "default" rather
> something like "regulator" and "default" should be the GPIO
> mode, as I guess something like that exists.
> 
> Activating a regulator using pin control "default" mode is
> not very pretty. It would probably be unintuitive and end
> up wasting power because people will get confused about
> what is going on.

That's not really it. The PMIC has pins that can be muxed either to
(regular) GPIOs, an ADC or to an LDO regulator.

This is just muxing, the regulator will be enabled and disabled
separately through another register. If it wasn't the case, it would
indeed be very messy.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Chen-Yu Tsai]

On Tue, Oct 3, 2017 at 10:47 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi Linus,
>
> On Tue, Oct 03, 2017 at 09:27:17AM +0000, Linus Walleij wrote:
>> On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz
>> <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>
>> > On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively)
>> > ldo_io0 and ldo_io1.
>> (...)
>> > +               gpio0_ldo: gpio0_ldo {
>> > +                       pins = "GPIO0";
>> > +                       function = "ldo";
>> > +               };
>> (...)
>> > +                       pinctrl-names = "default";
>> > +                       pinctrl-0 = <&gpio0_ldo>;
>> >                         /* Disable by default to avoid conflicts with GPIO */
>> >                         status = "disabled";
>>
>> So this is still by default disabled, but you make the default
>> mode something called "ldo".
>>
>> And I think that is to be understood as a low-dropout regulator?
>>
>> So is the idea that this should be represented as a regulator
>> in the end?
>>
>> Then I think the state name should not be "default" rather
>> something like "regulator" and "default" should be the GPIO
>> mode, as I guess something like that exists.
>>
>> Activating a regulator using pin control "default" mode is
>> not very pretty. It would probably be unintuitive and end
>> up wasting power because people will get confused about
>> what is going on.
>
> That's not really it. The PMIC has pins that can be muxed either to
> (regular) GPIOs, an ADC or to an LDO regulator.
>
> This is just muxing, the regulator will be enabled and disabled
> separately through another register. If it wasn't the case, it would
> indeed be very messy.

No. Actually they are controlled in the same register, so it is
very messy. The muxing options are:

    - 0: drive low
    - 1: drive high
    - 2: input with interrupt triggering
    - 3: LDO on
    - 4: LDO off
    - 5~7: floating (or ADC)

ChenYu

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Quentin Schulz]

Hi Chen-Yu, Linus,

On 03/10/2017 17:08, Chen-Yu Tsai wrote:
> On Tue, Oct 3, 2017 at 10:47 PM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> Hi Linus,
>>
>> On Tue, Oct 03, 2017 at 09:27:17AM +0000, Linus Walleij wrote:
>>> On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz
>>> <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>
>>>> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively)
>>>> ldo_io0 and ldo_io1.
>>> (...)
>>>> +               gpio0_ldo: gpio0_ldo {
>>>> +                       pins = "GPIO0";
>>>> +                       function = "ldo";
>>>> +               };
>>> (...)
>>>> +                       pinctrl-names = "default";
>>>> +                       pinctrl-0 = <&gpio0_ldo>;
>>>>                         /* Disable by default to avoid conflicts with GPIO */
>>>>                         status = "disabled";
>>>
>>> So this is still by default disabled, but you make the default
>>> mode something called "ldo".
>>>
>>> And I think that is to be understood as a low-dropout regulator?
>>>
>>> So is the idea that this should be represented as a regulator
>>> in the end?
>>>
>>> Then I think the state name should not be "default" rather
>>> something like "regulator" and "default" should be the GPIO
>>> mode, as I guess something like that exists.
>>>
>>> Activating a regulator using pin control "default" mode is
>>> not very pretty. It would probably be unintuitive and end
>>> up wasting power because people will get confused about
>>> what is going on.
>>
>> That's not really it. The PMIC has pins that can be muxed either to
>> (regular) GPIOs, an ADC or to an LDO regulator.
>>
>> This is just muxing, the regulator will be enabled and disabled
>> separately through another register. If it wasn't the case, it would
>> indeed be very messy.
> 
> No. Actually they are controlled in the same register, so it is
> very messy. The muxing options are:
> 
>     - 0: drive low
>     - 1: drive high
>     - 2: input with interrupt triggering
>     - 3: LDO on
>     - 4: LDO off
>     - 5~7: floating (or ADC)
> 

Just to be a little more precise,
     - 0: drive low
     - 1: drive high
     - 2: input with interrupt triggering
     - 3: LDO on
     - 4: LDO off
     - 5~7: floating (or ADC)

for AXP813, and
     - 0: drive low
     - 1: drive high
     - 2: input with interrupt triggering
     - 3: LDO on
     - 4: ADC
     - 5~7: floating

for AXP209.

So I think what you suggested Linus is not really relevant here as the
regulator framework will take care of disabling the regulator when
needed (for AXP813 via the ldo_off "muxing" selected by the regulator
framework).

However, there is no LDO off bit for AXP209 and the LDO can't be set to
0V in any other register. What's done now in the regulator driver for
AXP209 is to select the floating "muxing" for the pin when wanting to
disable the regulator. So I guess that's a way to handle it. Should we
do it another way?

Thanks for raising the issue, I frankly haven't thought about that at all.

I have to send a v4 to update the support for AXP813 (basically setting
ADC muxing to 0x5 instead of 0x4, for AXP813) as I misread the muxing
register description when adding support for it.

Thanks,
Quentin
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Chen-Yu Tsai]

On Wed, Oct 4, 2017 at 3:35 PM, Quentin Schulz
<quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi Chen-Yu, Linus,
>
> On 03/10/2017 17:08, Chen-Yu Tsai wrote:
>> On Tue, Oct 3, 2017 at 10:47 PM, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>> Hi Linus,
>>>
>>> On Tue, Oct 03, 2017 at 09:27:17AM +0000, Linus Walleij wrote:
>>>> On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz
>>>> <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>>
>>>>> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively)
>>>>> ldo_io0 and ldo_io1.
>>>> (...)
>>>>> +               gpio0_ldo: gpio0_ldo {
>>>>> +                       pins = "GPIO0";
>>>>> +                       function = "ldo";
>>>>> +               };
>>>> (...)
>>>>> +                       pinctrl-names = "default";
>>>>> +                       pinctrl-0 = <&gpio0_ldo>;
>>>>>                         /* Disable by default to avoid conflicts with GPIO */
>>>>>                         status = "disabled";
>>>>
>>>> So this is still by default disabled, but you make the default
>>>> mode something called "ldo".
>>>>
>>>> And I think that is to be understood as a low-dropout regulator?
>>>>
>>>> So is the idea that this should be represented as a regulator
>>>> in the end?
>>>>
>>>> Then I think the state name should not be "default" rather
>>>> something like "regulator" and "default" should be the GPIO
>>>> mode, as I guess something like that exists.
>>>>
>>>> Activating a regulator using pin control "default" mode is
>>>> not very pretty. It would probably be unintuitive and end
>>>> up wasting power because people will get confused about
>>>> what is going on.
>>>
>>> That's not really it. The PMIC has pins that can be muxed either to
>>> (regular) GPIOs, an ADC or to an LDO regulator.
>>>
>>> This is just muxing, the regulator will be enabled and disabled
>>> separately through another register. If it wasn't the case, it would
>>> indeed be very messy.
>>
>> No. Actually they are controlled in the same register, so it is
>> very messy. The muxing options are:
>>
>>     - 0: drive low
>>     - 1: drive high
>>     - 2: input with interrupt triggering
>>     - 3: LDO on
>>     - 4: LDO off
>>     - 5~7: floating (or ADC)
>>
>
> Just to be a little more precise,
>      - 0: drive low
>      - 1: drive high
>      - 2: input with interrupt triggering
>      - 3: LDO on
>      - 4: LDO off
>      - 5~7: floating (or ADC)
>
> for AXP813, and
>      - 0: drive low
>      - 1: drive high
>      - 2: input with interrupt triggering
>      - 3: LDO on
>      - 4: ADC
>      - 5~7: floating
>
> for AXP209.
>
> So I think what you suggested Linus is not really relevant here as the
> regulator framework will take care of disabling the regulator when
> needed (for AXP813 via the ldo_off "muxing" selected by the regulator
> framework).

Linus is suggesting that we use (switching between) pinctrl states to
control the regulator, as opposed to overriding the register value
directly. That would be nice, as both subsystems would have the same
idea of what's actually happening in the hardware.

As Linus mentioned, having the LDO on or off as the default pinctrl state
is not pretty. It also means as soon as the device is brought up, the
regulator state gets overridden. That would not work well for regulators
that have/want the "always-on" or "boot-on" properties.

> However, there is no LDO off bit for AXP209 and the LDO can't be set to
> 0V in any other register. What's done now in the regulator driver for
> AXP209 is to select the floating "muxing" for the pin when wanting to
> disable the regulator. So I guess that's a way to handle it. Should we
> do it another way?

I can't think of one. :)

You could have an actual pinmux option for "floating". If there's no
"LDO off" state, the regulator driver could use the "floating" state
instead.

ChenYu

> Thanks for raising the issue, I frankly haven't thought about that at all.
>
> I have to send a v4 to update the support for AXP813 (basically setting
> ADC muxing to 0x5 instead of 0x4, for AXP813) as I misread the muxing
> register description when adding support for it.
>
> Thanks,
> Quentin
> --
> Quentin Schulz, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Maxime Ripard]

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

On Tue, Oct 10, 2017 at 03:09:11AM +0000, Chen-Yu Tsai wrote:
> On Wed, Oct 4, 2017 at 3:35 PM, Quentin Schulz
> <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > Hi Chen-Yu, Linus,
> >
> > On 03/10/2017 17:08, Chen-Yu Tsai wrote:
> >> On Tue, Oct 3, 2017 at 10:47 PM, Maxime Ripard
> >> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >>> Hi Linus,
> >>>
> >>> On Tue, Oct 03, 2017 at 09:27:17AM +0000, Linus Walleij wrote:
> >>>> On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz
> >>>> <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >>>>
> >>>>> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively)
> >>>>> ldo_io0 and ldo_io1.
> >>>> (...)
> >>>>> +               gpio0_ldo: gpio0_ldo {
> >>>>> +                       pins = "GPIO0";
> >>>>> +                       function = "ldo";
> >>>>> +               };
> >>>> (...)
> >>>>> +                       pinctrl-names = "default";
> >>>>> +                       pinctrl-0 = <&gpio0_ldo>;
> >>>>>                         /* Disable by default to avoid conflicts with GPIO */
> >>>>>                         status = "disabled";
> >>>>
> >>>> So this is still by default disabled, but you make the default
> >>>> mode something called "ldo".
> >>>>
> >>>> And I think that is to be understood as a low-dropout regulator?
> >>>>
> >>>> So is the idea that this should be represented as a regulator
> >>>> in the end?
> >>>>
> >>>> Then I think the state name should not be "default" rather
> >>>> something like "regulator" and "default" should be the GPIO
> >>>> mode, as I guess something like that exists.
> >>>>
> >>>> Activating a regulator using pin control "default" mode is
> >>>> not very pretty. It would probably be unintuitive and end
> >>>> up wasting power because people will get confused about
> >>>> what is going on.
> >>>
> >>> That's not really it. The PMIC has pins that can be muxed either to
> >>> (regular) GPIOs, an ADC or to an LDO regulator.
> >>>
> >>> This is just muxing, the regulator will be enabled and disabled
> >>> separately through another register. If it wasn't the case, it would
> >>> indeed be very messy.
> >>
> >> No. Actually they are controlled in the same register, so it is
> >> very messy. The muxing options are:
> >>
> >>     - 0: drive low
> >>     - 1: drive high
> >>     - 2: input with interrupt triggering
> >>     - 3: LDO on
> >>     - 4: LDO off
> >>     - 5~7: floating (or ADC)
> >>
> >
> > Just to be a little more precise,
> >      - 0: drive low
> >      - 1: drive high
> >      - 2: input with interrupt triggering
> >      - 3: LDO on
> >      - 4: LDO off
> >      - 5~7: floating (or ADC)
> >
> > for AXP813, and
> >      - 0: drive low
> >      - 1: drive high
> >      - 2: input with interrupt triggering
> >      - 3: LDO on
> >      - 4: ADC
> >      - 5~7: floating
> >
> > for AXP209.
> >
> > So I think what you suggested Linus is not really relevant here as the
> > regulator framework will take care of disabling the regulator when
> > needed (for AXP813 via the ldo_off "muxing" selected by the regulator
> > framework).
> 
> Linus is suggesting that we use (switching between) pinctrl states to
> control the regulator, as opposed to overriding the register value
> directly. That would be nice, as both subsystems would have the same
> idea of what's actually happening in the hardware.
> 
> As Linus mentioned, having the LDO on or off as the default pinctrl state
> is not pretty. It also means as soon as the device is brought up, the
> regulator state gets overridden. That would not work well for regulators
> that have/want the "always-on" or "boot-on" properties.

What about not enforcing any muxing state when we want to mux to the
"ldo" function? We just leave it to whatever value it is, that way we
keep it under the regulator framework's control, and we don't disrupt
anything when the pin is requested.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Linus Walleij]

On Wed, Oct 11, 2017 at 2:00 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:

> What about not enforcing any muxing state when we want to mux to the
> "ldo" function? We just leave it to whatever value it is, that way we
> keep it under the regulator framework's control, and we don't disrupt
> anything when the pin is requested.

In a way since setting the bits one way means "LDO on" and another
setting means "LDO off" those bits should be handled by the
regulator framework when used as a regulator, not pin control.

So I would say yes.

Yours,
Linus Walleij

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Chen-Yu Tsai]

On Thu, Oct 12, 2017 at 3:09 AM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Oct 11, 2017 at 2:00 PM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>
>> What about not enforcing any muxing state when we want to mux to the
>> "ldo" function? We just leave it to whatever value it is, that way we
>> keep it under the regulator framework's control, and we don't disrupt
>> anything when the pin is requested.
>
> In a way since setting the bits one way means "LDO on" and another
> setting means "LDO off" those bits should be handled by the
> regulator framework when used as a regulator, not pin control.
>
> So I would say yes.

I agree. That would be the best solution.

ChenYu

Re: [PATCH v3 12/12] ARM: dtsi: axp81x: set pinmux for GPIO0/1 when used as LDOs

[Linus Walleij]

On Wed, Oct 4, 2017 at 9:35 AM, Quentin Schulz
<quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> Just to be a little more precise,
>      - 0: drive low
>      - 1: drive high
>      - 2: input with interrupt triggering
>      - 3: LDO on
>      - 4: LDO off
>      - 5~7: floating (or ADC)
>
> for AXP813, and
>      - 0: drive low
>      - 1: drive high
>      - 2: input with interrupt triggering
>      - 3: LDO on
>      - 4: ADC
>      - 5~7: floating

Fair enough, it's mux modes that the pin supports, no big surprises.

> So I think what you suggested Linus is not really relevant here as the
> regulator framework will take care of disabling the regulator when
> needed (for AXP813 via the ldo_off "muxing" selected by the regulator
> framework).

I think I see why I got confused.

The point is that your mode for setting it to "LDO on" should have the
pin control state connected to the relevant device.

It should be connected to the regulator and nothing else, so if there is a fixed
regulator or whatever in the device tree, it should have pinctrl-0
and pinctrl-names = ".."; here is is for some obscure reason connected
to the GPIO controller (!) instead, and the actual consumer of this state
is NOT the GPIO controller, but quite obviously the regulator, so
put the pinctrl business in that regulator node instead.

"default" mode is OK on a regulator, as that can be expected to make the
pin precisely a regulator pin. Forget my ramblings about a "regulator"
state.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 00/12] add pinmuxing support for pins in AXP209 and AXP813 PMICs

[Linus Walleij]

On Mon, Oct 2, 2017 at 2:08 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:

> The AXP209 and AXP813 PMICs have several pins (respectively 3 and 2) that can
> be used either as GPIOs or for other purposes (ADC or LDO here).
>
> We already have a GPIO driver for the GPIO use of those pins on the AXP209.
> Let's "upgrade" this driver to support all the functions these pins can have.
>
> Then we add support to this driver for the AXP813 which is slighlty different
> (basically a different offset in a register and one less pin).
>
> I suggest patches 1 to 6 go through Linus's tree and 7 to 12 via Maxime or
> Chen-Yu's tree.
>
> This version of the patchset is based on Chen-Yu's patchset for AXP813/818
> regulators[1].
>
> v3:

This is starting to look really good, but I see there are still comments.

Keep the series going, we merge once Maxime and Chen-Yu are happy.

Yours,
Linus Walleij