diff mbox

[3/6] pinctrl: armada-37xx: Add gpio support

Message ID 20161222172501.16121-4-gregory.clement@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gregory CLEMENT Dec. 22, 2016, 5:24 p.m. UTC
GPIO management is pretty simple and is part of the same IP than the pin
controller for the Armada 37xx SoCs.  This patch adds the GPIO support to
the pinctrl-armada-37xx.c file, it also allows sharing common functions
between the gpiolib and the pinctrl drivers.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 113 ++++++++++++++++++++++++++--
 1 file changed, 106 insertions(+), 7 deletions(-)

Comments

Linus Walleij Dec. 30, 2016, 8:51 a.m. UTC | #1
On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> GPIO management is pretty simple and is part of the same IP than the pin
> controller for the Armada 37xx SoCs.  This patch adds the GPIO support to
> the pinctrl-armada-37xx.c file, it also allows sharing common functions
> between the gpiolib and the pinctrl drivers.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Some remarks:

> +static int armada_37xx_gpio_get_direction(struct gpio_chip *chip,
> +                                         unsigned int offset)
> +{
> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
> +       unsigned int reg = OUTPUT_EN;
> +       unsigned int val, mask;
> +
> +       if (offset >= GPIO_PER_REG) {
> +               offset -= GPIO_PER_REG;
> +               reg += sizeof(u32);
> +       }

Add a comment saying we never have more than two registers?
If there would be three registers this would fail, right?

> +       mask = BIT(offset);
> +
> +       regmap_read(info->regmap, reg, &val);
>
> +       return (val & mask) == 0;

Use this:

return !(val & mask);

> +static int armada_37xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
> +       unsigned int reg = INPUT_VAL;
> +       unsigned int val, mask;
> +
> +       if (offset >= GPIO_PER_REG) {
> +               offset -= GPIO_PER_REG;
> +               reg += sizeof(u32);
> +       }
> +       mask = BIT(offset);

This code is repeating. Break out a static (inline?) helper to do this.

> +static int armada_37xx_gpiolib_register(struct platform_device *pdev,
> +                                       struct armada_37xx_pinctrl *info)

Nit: gpiochip_register or so is more to the point.

> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
> +                                    pinbase, info->data->nr_pins);
> +       if (ret)
> +               return ret;

Why do you do this?

Why not just put the ranges into the device tree? We already support
this in the gpiolib core and it is helpful.

See Documentation/devicetree/bindings/gpio/gpio.txt
and other DTS files for gpio-ranges.

Yours,
Linus Walleij
Gregory CLEMENT March 22, 2017, 11:54 a.m. UTC | #2
Hi Linus,
 
 On ven., déc. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>
>> GPIO management is pretty simple and is part of the same IP than the pin
>> controller for the Armada 37xx SoCs.  This patch adds the GPIO support to
>> the pinctrl-armada-37xx.c file, it also allows sharing common functions
>> between the gpiolib and the pinctrl drivers.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> Some remarks:
>
>> +static int armada_37xx_gpio_get_direction(struct gpio_chip *chip,
>> +                                         unsigned int offset)
>> +{
>> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
>> +       unsigned int reg = OUTPUT_EN;
>> +       unsigned int val, mask;
>> +
>> +       if (offset >= GPIO_PER_REG) {
>> +               offset -= GPIO_PER_REG;
>> +               reg += sizeof(u32);
>> +       }
>
> Add a comment saying we never have more than two registers?
> If there would be three registers this would fail, right?

I added the comment

>
>> +       mask = BIT(offset);
>> +
>> +       regmap_read(info->regmap, reg, &val);
>>
>> +       return (val & mask) == 0;
>
> Use this:
>
> return !(val & mask);

done but I could tou explain the advantage of doing it?

>
>> +static int armada_37xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
>> +       unsigned int reg = INPUT_VAL;
>> +       unsigned int val, mask;
>> +
>> +       if (offset >= GPIO_PER_REG) {
>> +               offset -= GPIO_PER_REG;
>> +               reg += sizeof(u32);
>> +       }
>> +       mask = BIT(offset);
>
> This code is repeating. Break out a static (inline?) helper to do
> this.

done

>
>> +static int armada_37xx_gpiolib_register(struct platform_device *pdev,
>> +                                       struct armada_37xx_pinctrl *info)
>
> Nit: gpiochip_register or so is more to the point.
>
>> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
>> +                                    pinbase, info->data->nr_pins);
>> +       if (ret)
>> +               return ret;
>
> Why do you do this?
>
> Why not just put the ranges into the device tree? We already support
> this in the gpiolib core and it is helpful.
>
> See Documentation/devicetree/bindings/gpio/gpio.txt
> and other DTS files for gpio-ranges.

Following your review, I tried to use it but it didn't work for
me. When the second pin controller was probed then there was collision
for the gpio number. I tried several combination without any luck.

So for now I left it aside.

I can show you the errors message I get and the binding I used if you
are interested.


Thanks,

Gregory
Linus Walleij March 23, 2017, 10:28 a.m. UTC | #3
On Wed, Mar 22, 2017 at 12:54 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
>  On ven., déc. 30 2016, Linus Walleij <linus.walleij@linaro.org> wrote:

>>> +       mask = BIT(offset);
>>> +
>>> +       regmap_read(info->regmap, reg, &val);
>>>
>>> +       return (val & mask) == 0;
>>
>> Use this:
>>
>> return !(val & mask);
>
> done but I could tou explain the advantage of doing it?

The other controllers do it so makes the code easier to
perceptualize.

>>> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
>>> +                                    pinbase, info->data->nr_pins);
>>> +       if (ret)
>>> +               return ret;
>>
>> Why do you do this?
>>
>> Why not just put the ranges into the device tree? We already support
>> this in the gpiolib core and it is helpful.
>>
>> See Documentation/devicetree/bindings/gpio/gpio.txt
>> and other DTS files for gpio-ranges.
>
> Following your review, I tried to use it but it didn't work for
> me. When the second pin controller was probed then there was collision
> for the gpio number. I tried several combination without any luck.

That sounds like a bug. Are you using dynamic GPIO number
assignments?

Yours,
Linus Walleij
Gregory CLEMENT March 23, 2017, 2:47 p.m. UTC | #4
Hi Linus,
 
 On jeu., mars 23 2017, Linus Walleij <linus.walleij@linaro.org> wrote:


>>>> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
>>>> +                                    pinbase, info->data->nr_pins);
>>>> +       if (ret)
>>>> +               return ret;
>>>
>>> Why do you do this?
>>>
>>> Why not just put the ranges into the device tree? We already support
>>> this in the gpiolib core and it is helpful.
>>>
>>> See Documentation/devicetree/bindings/gpio/gpio.txt
>>> and other DTS files for gpio-ranges.
>>
>> Following your review, I tried to use it but it didn't work for
>> me. When the second pin controller was probed then there was collision
>> for the gpio number. I tried several combination without any luck.
>
> That sounds like a bug. Are you using dynamic GPIO number
> assignments?

I managed to use it, the issue was that I register the gpio before the
pinctrl. The call to gpiochip_add_pin_range() was done before both
registration in my initial call so it was not a problem. When I switched
to the gpio-ranges, this call was done from the gpiochip_add_data(), and
in this case it has tt be called after devm_pinctrl_register().

So I am about sending a new version using gpio-ranges.

Thanks,

Gregory
diff mbox

Patch

diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index 021bfe793af3..4d9571b49ad1 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -10,6 +10,7 @@ 
  * without any warranty of any kind, whether express or implied.
  */
 
+#include <linux/gpio/driver.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -24,6 +25,8 @@ 
 #include "../pinctrl-utils.h"
 
 #define OUTPUT_EN	0x0
+#define INPUT_VAL	0x10
+#define OUTPUT_VAL	0x18
 #define OUTPUT_CTL	0x20
 #define SELECTION	0x30
 
@@ -60,6 +63,7 @@  struct armada_37xx_pinctrl {
 	struct regmap			*regmap;
 	struct armada_37xx_pin_data	*data;
 	struct device			*dev;
+	struct gpio_chip		gpio_chip;
 	struct pinctrl_desc		pctl;
 	struct pinctrl_dev		*pctl_dev;
 	struct armada_37xx_pin_group	*groups;
@@ -297,9 +301,10 @@  static int armada_37xx_pmx_set(struct pinctrl_dev *pctldev,
 	return armada_37xx_pmx_set_by_name(pctldev, name, grp);
 }
 
-static int armada_37xx_pmx_direction_input(struct armada_37xx_pinctrl *info,
-					   unsigned int offset)
+static int armada_37xx_gpio_direction_input(struct gpio_chip *chip,
+					    unsigned int offset)
 {
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
 	unsigned int reg = OUTPUT_EN;
 	unsigned int mask;
 
@@ -312,11 +317,28 @@  static int armada_37xx_pmx_direction_input(struct armada_37xx_pinctrl *info,
 	return regmap_update_bits(info->regmap, reg, mask, 0);
 }
 
+static int armada_37xx_gpio_get_direction(struct gpio_chip *chip,
+					  unsigned int offset)
+{
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+	unsigned int reg = OUTPUT_EN;
+	unsigned int val, mask;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+
+	regmap_read(info->regmap, reg, &val);
 
+	return (val & mask) == 0;
+}
 
-static int armada_37xx_pmx_direction_output(struct armada_37xx_pinctrl *info,
-					    unsigned int offset, int value)
+static int armada_37xx_gpio_direction_output(struct gpio_chip *chip,
+					     unsigned int offset, int value)
 {
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
 	unsigned int reg = OUTPUT_EN;
 	unsigned int mask;
 
@@ -329,19 +351,54 @@  static int armada_37xx_pmx_direction_output(struct armada_37xx_pinctrl *info,
 	return regmap_update_bits(info->regmap, reg, mask, mask);
 }
 
+static int armada_37xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+	unsigned int reg = INPUT_VAL;
+	unsigned int val, mask;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+
+	regmap_read(info->regmap, reg, &val);
+
+	return (val & mask) != 0;
+}
+
+static void armada_37xx_gpio_set(struct gpio_chip *chip, unsigned int offset,
+				 int value)
+{
+	struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+	unsigned int reg = OUTPUT_VAL;
+	unsigned int mask, val;
+
+	if (offset >= GPIO_PER_REG) {
+		offset -= GPIO_PER_REG;
+		reg += sizeof(u32);
+	}
+	mask = BIT(offset);
+	val = value ? mask : 0;
+
+	regmap_update_bits(info->regmap, reg, mask, val);
+}
+
 static int armada_37xx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 					      struct pinctrl_gpio_range *range,
 					      unsigned int offset, bool input)
 {
 	struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	struct gpio_chip *chip = range->gc;
 
 	dev_dbg(info->dev, "gpio_direction for pin %u as %s-%d to %s\n",
 		offset, range->name, offset, input ? "input" : "output");
 
 	if (input)
-		armada_37xx_pmx_direction_input(info, offset);
+		armada_37xx_gpio_direction_input(chip, offset);
 	else
-		armada_37xx_pmx_direction_output(info, offset, 0);
+		armada_37xx_gpio_direction_output(chip, offset, 0);
 
 	return 0;
 }
@@ -371,6 +428,39 @@  static const struct pinmux_ops armada_37xx_pmx_ops = {
 	.gpio_set_direction	= armada_37xx_pmx_gpio_set_direction,
 };
 
+static const struct gpio_chip armada_37xx_gpiolib_chip = {
+	.request = gpiochip_generic_request,
+	.free = gpiochip_generic_free,
+	.set = armada_37xx_gpio_set,
+	.get = armada_37xx_gpio_get,
+	.get_direction	= armada_37xx_gpio_get_direction,
+	.direction_input = armada_37xx_gpio_direction_input,
+	.direction_output = armada_37xx_gpio_direction_output,
+	.owner = THIS_MODULE,
+};
+
+static int armada_37xx_gpiolib_register(struct platform_device *pdev,
+					struct armada_37xx_pinctrl *info)
+{
+	struct gpio_chip *gc;
+	int ret;
+
+	info->gpio_chip = armada_37xx_gpiolib_chip;
+
+	gc = &info->gpio_chip;
+	gc->ngpio = info->data->nr_pins;
+	gc->parent = &pdev->dev;
+	gc->base = -1;
+	gc->of_node = info->dev->of_node;
+	gc->label = info->data->name;
+
+	ret = gpiochip_add_data(gc, info);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int _add_function(struct armada_37xx_pmx_func *funcs, int *funcsize,
 			 const char *name)
 {
@@ -551,7 +641,7 @@  static int armada_37xx_pinctrl_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct regmap *regmap;
-	int ret;
+	int ret, pinbase = global_pin;
 
 	info = devm_kzalloc(dev, sizeof(struct armada_37xx_pinctrl),
 			    GFP_KERNEL);
@@ -570,10 +660,19 @@  static int armada_37xx_pinctrl_probe(struct platform_device *pdev)
 	match = of_match_node(armada_37xx_pinctrl_of_match, np);
 	info->data = (struct armada_37xx_pin_data *)match->data;
 
+	ret = armada_37xx_gpiolib_register(pdev, info);
+	if (ret)
+		return ret;
+
 	ret = armada_37xx_pinctrl_register(pdev, info);
 	if (ret)
 		return ret;
 
+	ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
+				     pinbase, info->data->nr_pins);
+	if (ret)
+		return ret;
+
 	platform_set_drvdata(pdev, info);
 
 	return 0;