mbox series

[v2,00/10] add pinmuxing support for pins in AXP209 and AXP813 PMICs

Message ID cover.1c314f4154a6d27354625f03d0a5269eee55a9c5.1506428208.git-series.quentin.schulz@free-electrons.com
Headers show
Series add pinmuxing support for pins in AXP209 and AXP813 PMICs | expand

Message

Quentin Schulz Sept. 26, 2017, 12:17 p.m. UTC 
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).

This is a v2 to a first version that was sent in November 2016.

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,

Thanks,
Quentin

Maxime Ripard (1):
  ARM: dts: add dtsi for AXP813 PMIC

Quentin Schulz (9):
  pinctrl: move gpio-axp209 to pinctrl
  pinctrl: axp209: add pinctrl features
  pinctrl: axp209: use drv_data of pinctrl_pin_desc to store pin reg
  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 axp813 dtsi
  ARM: dts: sun8i: bananapi-m3: include axp813 dtsi

 Documentation/devicetree/bindings/gpio/gpio-axp209.txt       |  30 +-
 Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt |  67 +-
 arch/arm/boot/dts/axp813.dtsi                                |  58 +-
 arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts                 |   4 +-
 arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts                    |   2 +-
 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                             | 610 +++++++-
 12 files changed, 750 insertions(+), 226 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/gpio/gpio-axp209.txt
 create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt
 create mode 100644 arch/arm/boot/dts/axp813.dtsi
 delete mode 100644 drivers/gpio/gpio-axp209.c
 create mode 100644 drivers/pinctrl/pinctrl-axp209.c

base-commit: 73527316e3fdde8a210b8ab66c1bf48538cf6b09
-- 
git-series 0.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Quentin Schulz Sept. 26, 2017, 12:59 p.m. UTC | #1 
Hi Maxime,

On 26/09/2017 14:55, Maxime Ripard wrote:
> On Tue, Sep 26, 2017 at 12:17:11PM +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@free-electrons.com>

> 

> I'm not sure we actually need to do this. Can't we just keep the

> driver here?

> 


That's not what I understood from:
https://lkml.org/lkml/2016/11/24/360 and the following answers from
Linus on the first version.

Quentin
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Maxime Ripard Sept. 26, 2017, 1 p.m. UTC | #2 
On Tue, Sep 26, 2017 at 12:17:12PM +0000, Quentin Schulz wrote:
> +static const struct axp20x_desc_pin axp209_pins[] = {

> +	AXP20X_PIN(AXP20X_PINCTRL_PIN(0, "GPIO0"),

> +		   AXP20X_FUNCTION(0x0, "gpio_out"),

> +		   AXP20X_FUNCTION(0x2, "gpio_in"),

> +		   AXP20X_FUNCTION(0x3, "ldo"),

> +		   AXP20X_FUNCTION(0x4, "adc")),

> +	AXP20X_PIN(AXP20X_PINCTRL_PIN(1, "GPIO1"),

> +		   AXP20X_FUNCTION(0x0, "gpio_out"),

> +		   AXP20X_FUNCTION(0x2, "gpio_in"),

> +		   AXP20X_FUNCTION(0x3, "ldo"),

> +		   AXP20X_FUNCTION(0x4, "adc")),

> +	AXP20X_PIN(AXP20X_PINCTRL_PIN(2, "GPIO2"),

> +		   AXP20X_FUNCTION(0x0, "gpio_out"),

> +		   AXP20X_FUNCTION(0x2, "gpio_in")),

> +};


If all the functions are the same, and at the same offset, can't we
just hardcode it, instead of having (and duplicate) all the logic
below?

> +	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 = pins;

> +	pctrl_desc->npins = gpio->desc->npins;

> +	pctrl_desc->pctlops = &axp20x_pctrl_ops;

> +	pctrl_desc->pmxops = &axp20x_pmx_ops;


The strict flag needs to be set too in order to avoid concurrent uses
of GPIO and other functions.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Maxime Ripard Sept. 26, 2017, 1:12 p.m. UTC | #3 
On Tue, Sep 26, 2017 at 12:17:16PM +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@free-electrons.com>

> ---

>  Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt | 13 ++-

>  drivers/pinctrl/pinctrl-axp209.c                             | 30 ++++++-

>  2 files changed, 39 insertions(+), 4 deletions(-)

> 

> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-axp209.txt

> index a5bfe87..a1d5dec 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-pctl"

>  - #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 11f871e..500862b 100644

> --- a/drivers/pinctrl/pinctrl-axp209.c

> +++ b/drivers/pinctrl/pinctrl-axp209.c

> @@ -108,11 +108,28 @@ static const struct axp20x_desc_pin axp209_pins[] = {

>  		   AXP20X_FUNCTION(0x2, "gpio_in")),

>  };

>  

> +static const struct axp20x_desc_pin axp813_pins[] = {

> +	AXP20X_PIN(AXP20X_PINCTRL_PIN(0, "GPIO0", (void *)AXP20X_GPIO0_CTRL),

> +		   AXP20X_FUNCTION(0x0, "gpio_out"),

> +		   AXP20X_FUNCTION(0x2, "gpio_in"),

> +		   AXP20X_FUNCTION(0x3, "ldo"),

> +		   AXP20X_FUNCTION(0x4, "adc")),

> +	AXP20X_PIN(AXP20X_PINCTRL_PIN(1, "GPIO1", (void *)AXP20X_GPIO1_CTRL),

> +		   AXP20X_FUNCTION(0x0, "gpio_out"),

> +		   AXP20X_FUNCTION(0x2, "gpio_in"),

> +		   AXP20X_FUNCTION(0x3, "ldo")),

> +};

> +

>  static const struct axp20x_pinctrl_desc axp20x_pinctrl_data = {

>  	.pins	= axp209_pins,

>  	.npins	= ARRAY_SIZE(axp209_pins),

>  };

>  

> +static const struct axp20x_pinctrl_desc axp813_pinctrl_data = {

> +	.pins	= axp813_pins,

> +	.npins	= ARRAY_SIZE(axp813_pins),

> +};

> +

>  static int axp20x_gpio_input(struct gpio_chip *chip, unsigned offset)

>  {

>  	return pinctrl_gpio_direction_input(chip->base + offset);

> @@ -479,6 +496,7 @@ static int axp20x_pctl_probe(struct platform_device *pdev)

>  	struct axp20x_pctl *pctl;

>  	struct pinctrl_desc *pctrl_desc;

>  	struct pinctrl_pin_desc *pins;

> +	struct device_node *np = pdev->dev.of_node;

>  	int ret, i;

>  

>  	if (!of_device_is_available(pdev->dev.of_node))

> @@ -505,13 +523,18 @@ 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->regmap = axp20x->regmap;

>  

> -	pctl->desc = &axp20x_pinctrl_data;

> -	pctl->gpio_status_offset = 4;

> +	if (of_device_is_compatible(np, "x-powers,axp209-gpio")) {

> +		pctl->desc = &axp20x_pinctrl_data;

> +		pctl->gpio_status_offset = 4;

> +	} else {

> +		pctl->desc = &axp813_pinctrl_data;

> +		pctl->gpio_status_offset = 0;

> +	}

>  	pctl->dev = &pdev->dev;

> +	pctl->chip.ngpio = pctl->desc->npins;


This should be part of a structure that would be attached to the
compatible.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Maxime Ripard Sept. 26, 2017, 1:16 p.m. UTC | #4 
On Tue, Sep 26, 2017 at 12:17:18PM +0000, Quentin Schulz wrote:
> From: Maxime Ripard <maxime.ripard@free-electrons.com>

> 

> The AXP813 PMIC is used with some Allwinner SoCs. Create a dtsi to

> include in each board embedding it.

> 

> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>


There must be my Signed-off-by here.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Maxime Ripard Sept. 26, 2017, 1:27 p.m. UTC | #5 
On Tue, Sep 26, 2017 at 01:08:21PM +0000, Quentin Schulz wrote:
> Hi Maxime,

> 

> On 26/09/2017 15:00, Maxime Ripard wrote:

> > On Tue, Sep 26, 2017 at 12:17:12PM +0000, Quentin Schulz wrote:

> >> +static const struct axp20x_desc_pin axp209_pins[] = {

> >> +	AXP20X_PIN(AXP20X_PINCTRL_PIN(0, "GPIO0"),

> >> +		   AXP20X_FUNCTION(0x0, "gpio_out"),

> >> +		   AXP20X_FUNCTION(0x2, "gpio_in"),

> >> +		   AXP20X_FUNCTION(0x3, "ldo"),

> >> +		   AXP20X_FUNCTION(0x4, "adc")),

> >> +	AXP20X_PIN(AXP20X_PINCTRL_PIN(1, "GPIO1"),

> >> +		   AXP20X_FUNCTION(0x0, "gpio_out"),

> >> +		   AXP20X_FUNCTION(0x2, "gpio_in"),

> >> +		   AXP20X_FUNCTION(0x3, "ldo"),

> >> +		   AXP20X_FUNCTION(0x4, "adc")),

> >> +	AXP20X_PIN(AXP20X_PINCTRL_PIN(2, "GPIO2"),

> >> +		   AXP20X_FUNCTION(0x0, "gpio_out"),

> >> +		   AXP20X_FUNCTION(0x2, "gpio_in")),

> >> +};

> > 

> > If all the functions are the same, and at the same offset, can't we

> > just hardcode it, instead of having (and duplicate) all the logic

> > below?

> > 

> 

> AXP20X_PIN(AXP20X_PINCTRL_PIN(0, "GPIO0"),

> 		AXP20X_GPIO_OUT,

> 		AXP20X_GPIO_IN,

> 		AXP20X_LDO,

> 		AXP20X_ADC))

> 

> That's what you mean?


What I mean is:

static int axp20x_get_func(char *func)
{
	if (!strcmp(func, "gpio_out"))
		return 0;

	if (!strcmp(func, "gpio_in"))
		return 2;
 
	if (!strcmp(func, "ldo"))
 		return 3;
 
	if (!strcmp(func, "adc"))
 		return 4;

	return -EINVAL;
}

> >> +	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 = pins;

> >> +	pctrl_desc->npins = gpio->desc->npins;

> >> +	pctrl_desc->pctlops = &axp20x_pctrl_ops;

> >> +	pctrl_desc->pmxops = &axp20x_pmx_ops;

> > 

> > The strict flag needs to be set too in order to avoid concurrent uses

> > of GPIO and other functions.

> > 

> 

> Strict is a property of pinmux_ops struct (pmxops) and it is set.


Ah, right, my bad.

Maxime

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