diff mbox

[v2,4/7] pinctrl: armada-37xx: Add gpio support

Message ID 913cf9807c7c351bb7dabac9b3336431dac060e5.1490120798.git-series.gregory.clement@free-electrons.com
State New
Headers show

Commit Message

Gregory CLEMENT March 21, 2017, 6:28 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 | 134 ++++++++++++++++++---
 1 file changed, 119 insertions(+), 15 deletions(-)

Comments

Linus Walleij March 27, 2017, 8:57 a.m. UTC | #1
On Tue, Mar 21, 2017 at 7:28 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

You should add something to your Kconfig including:

select GPIOLIB
select OF_GPIO

or so... or depends on. You certainly need them.

> +static int armada_37xx_gpiochip_register(struct platform_device *pdev,
> +                                       struct armada_37xx_pinctrl *info)
> +{
> +       struct device_node *np;
> +       struct gpio_chip *gc;
> +       int ret = -ENODEV;
> +
> +       for_each_child_of_node(info->dev->of_node, np) {
> +               if (of_find_property(np, "gpio-controller", NULL)) {
> +                       ret = 0;
> +                       break;
> +               }
> +       };

OK so several GPIO chips as subnodes, why not one device per
chip? Or have we discussed this before? It seems a bit weird,
apparently there is just one node with a gpio-controller, as you're
just adding one pin range.

What happens if there would be two gpio-controllers? The second
is just ignored without error?

> +       ret = gpiochip_add_data(gc, info);
> +       if (ret)
> +               return ret;

Can't you use devm_gpiochip_add_data()?

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

Why can't you put the range(s) into the device tree?

We already have code in drivers/gpio/gpiolib-of.c to do this
for you. And generic range definition bindings.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gregory CLEMENT March 27, 2017, 9:46 a.m. UTC | #2
Hi Linus,
 
 On lun., mars 27 2017, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Tue, Mar 21, 2017 at 7:28 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>
> You should add something to your Kconfig including:
>
> select GPIOLIB
> select OF_GPIO
>
> or so... or depends on. You certainly need them.

I missed it I will do it in v4.

>
>> +static int armada_37xx_gpiochip_register(struct platform_device *pdev,
>> +                                       struct armada_37xx_pinctrl *info)
>> +{
>> +       struct device_node *np;
>> +       struct gpio_chip *gc;
>> +       int ret = -ENODEV;
>> +
>> +       for_each_child_of_node(info->dev->of_node, np) {
>> +               if (of_find_property(np, "gpio-controller", NULL)) {
>> +                       ret = 0;
>> +                       break;
>> +               }
>> +       };
>
> OK so several GPIO chips as subnodes, why not one device per
> chip? Or have we discussed this before? It seems a bit weird,
> apparently there is just one node with a gpio-controller, as you're
> just adding one pin range.

As you probably noticed pinctrl and gpio register are mixed in this
hardware. One of the register is alos use to get some clock freqeuncy,
so that's why I ended with a syscon node. The parent node is the
pinctrl. My main motivation to use a subnode was to be ablee to have a
phandle associated with the GPIO chip. In my first version I only have
one node but then I realized that I could not use GPIO in the device
tree without an phandle to point it.

If you have an other solution, I would be happy to remove this subnode.


>
> What happens if there would be two gpio-controllers? The second
> is just ignored without error?

There won't be a second gpio-controllers because we only have one gpio
controller by pin controller (as they are actually the same). Also as I
said above, the subnode is mainly here to provide a phandle.

But I can add a comment to emphasize it.


>
>> +       ret = gpiochip_add_data(gc, info);
>> +       if (ret)
>> +               return ret;
>
> Can't you use devm_gpiochip_add_data()?

I think I can.

>
>> +       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
>> +                                    pinbase, info->data->nr_pins);
>> +       if (ret)
>> +               return ret;
>
> Why can't you put the range(s) into the device tree?
>
> We already have code in drivers/gpio/gpiolib-of.c to do this
> for you. And generic range definition bindings.

It was done in the v3.

Tanks,

Gregory

>
> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox

Patch

diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index 98cde04e07e1..e41e2d02aca7 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
 
@@ -75,6 +78,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;
@@ -312,51 +316,99 @@  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 inline void aramda_37xx_update_reg(unsigned int *reg,
+					  unsigned int offset)
 {
-	unsigned int reg = OUTPUT_EN;
-	unsigned int mask;
-
+	/* We never have more than 2 registers */
 	if (offset >= GPIO_PER_REG) {
 		offset -= GPIO_PER_REG;
-		reg += sizeof(u32);
+		*reg += sizeof(u32);
 	}
+}
+
+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;
+
+	aramda_37xx_update_reg(&reg, offset);
 	mask = BIT(offset);
 
 	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;
+
+	aramda_37xx_update_reg(&reg, offset);
+	mask = BIT(offset);
+
+	regmap_read(info->regmap, reg, &val);
 
+	return !(val & mask);
+}
 
-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;
 
-	if (offset >= GPIO_PER_REG) {
-		offset -= GPIO_PER_REG;
-		reg += sizeof(u32);
-	}
+	aramda_37xx_update_reg(&reg, offset);
 	mask = BIT(offset);
 
 	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;
+
+	aramda_37xx_update_reg(&reg, offset);
+	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;
+
+	aramda_37xx_update_reg(&reg, offset);
+	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;
 }
@@ -386,6 +438,49 @@  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_gpiochip_register(struct platform_device *pdev,
+					struct armada_37xx_pinctrl *info)
+{
+	struct device_node *np;
+	struct gpio_chip *gc;
+	int ret = -ENODEV;
+
+	for_each_child_of_node(info->dev->of_node, np) {
+		if (of_find_property(np, "gpio-controller", NULL)) {
+			ret = 0;
+			break;
+		}
+	};
+	if (ret)
+		return 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 = np;
+	gc->label = info->data->name;
+
+	ret = gpiochip_add_data(gc, info);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int armada_37xx_add_function(struct armada_37xx_pmx_func *funcs,
 				    int *funcsize, const char *name)
 {
@@ -566,7 +661,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);
@@ -585,10 +680,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_gpiochip_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;