[PATCH] pinctrl: bc2835: Add brcm,level property

[PATCH] pinctrl: bc2835: Add brcm,level property

[Matheus Castello]

Property to set initial value of pin output buffer.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 .../bindings/pinctrl/brcm,bcm2835-gpio.txt         |  5 +-
 drivers/pinctrl/bcm/pinctrl-bcm2835.c              | 90 +++++++++++++++++-----
 2 files changed, 75 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
index 2569866..6834f1d 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
@@ -54,8 +54,11 @@ Optional subnode-properties:
   0: none
   1: down
   2: up
+- brcm,level: Integer, representing the value of output buffer to apply to the pin(s):
+  0: low
+  1: high
 
-Each of brcm,function and brcm,pull may contain either a single value which
+Each of brcm,function, brcm,pull and brcm,level may contain either a single value which
 will be applied to all pins in brcm,pins, or 1 value for each entry in
 brcm,pins.
 
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 785c366..0ace548 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -73,6 +73,8 @@
 enum bcm2835_pinconf_param {
 	/* argument: bcm2835_pinconf_pull */
 	BCM2835_PINCONF_PARAM_PULL,
+	/* argument: bcm2835_pinconf_level */
+	BCM2835_PINCONF_PARAM_LEVEL,
 };
 
 #define BCM2835_PINCONF_PACK(_param_, _arg_) ((_param_) << 16 | (_arg_))
@@ -725,16 +727,42 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
 	return 0;
 }
 
+static int bcm2835_pctl_dt_node_to_map_level(struct bcm2835_pinctrl *pc,
+		struct device_node *np, u32 pin, u32 level,
+		struct pinctrl_map **maps)
+{
+	struct pinctrl_map *map = *maps;
+	unsigned long *configs;
+
+	if (level > 2) {
+		dev_err(pc->dev, "%pOF: invalid brcm,level %d\n", np, level);
+		return -EINVAL;
+	}
+
+	configs = kzalloc(sizeof(*configs), GFP_KERNEL);
+	if (!configs)
+		return -ENOMEM;
+	configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_LEVEL, level);
+
+	map->type = PIN_MAP_TYPE_CONFIGS_PIN;
+	map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
+	map->data.configs.configs = configs;
+	map->data.configs.num_configs = 1;
+	(*maps)++;
+
+	return 0;
+}
+
 static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		struct device_node *np,
 		struct pinctrl_map **map, unsigned *num_maps)
 {
 	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
-	struct property *pins, *funcs, *pulls;
-	int num_pins, num_funcs, num_pulls, maps_per_pin;
+	struct property *pins, *funcs, *pulls, *levels;
+	int num_pins, num_funcs, num_pulls, num_levels, maps_per_pin;
 	struct pinctrl_map *maps, *cur_map;
 	int i, err;
-	u32 pin, func, pull;
+	u32 pin, func, pull, level;
 
 	pins = of_find_property(np, "brcm,pins", NULL);
 	if (!pins) {
@@ -744,6 +772,7 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 
 	funcs = of_find_property(np, "brcm,function", NULL);
 	pulls = of_find_property(np, "brcm,pull", NULL);
+	levels = of_find_property(np, "brcm,level", NULL);
 
 	if (!funcs && !pulls) {
 		dev_err(pc->dev,
@@ -755,6 +784,7 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 	num_pins = pins->length / 4;
 	num_funcs = funcs ? (funcs->length / 4) : 0;
 	num_pulls = pulls ? (pulls->length / 4) : 0;
+	num_levels = levels ? (levels->length / 4) : 0;
 
 	if (num_funcs > 1 && num_funcs != num_pins) {
 		dev_err(pc->dev,
@@ -770,11 +800,20 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		return -EINVAL;
 	}
 
+	if (num_levels > 1 && num_levels != num_pins) {
+		dev_err(pc->dev,
+			"%pOF: brcm,level must have 1 or %d entries\n",
+			np, num_pins);
+		return -EINVAL;
+	}
+
 	maps_per_pin = 0;
 	if (num_funcs)
 		maps_per_pin++;
 	if (num_pulls)
 		maps_per_pin++;
+	if (num_levels)
+		maps_per_pin++;
 	cur_map = maps = kzalloc(num_pins * maps_per_pin * sizeof(*maps),
 				GFP_KERNEL);
 	if (!maps)
@@ -811,6 +850,17 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 			if (err)
 				goto out;
 		}
+		if (num_levels) {
+			err = of_property_read_u32_index(np, "brcm,level",
+					(num_levels > 1) ? i : 0, &level);
+			if (err)
+				goto out;
+
+			err = bcm2835_pctl_dt_node_to_map_level(pc, np, pin,
+							level, &cur_map);
+			if (err)
+				goto out;
+		}
 	}
 
 	*map = maps;
@@ -931,23 +981,25 @@ static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 		param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
 		arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
 
-		if (param != BCM2835_PINCONF_PARAM_PULL)
+		if (param == BCM2835_PINCONF_PARAM_PULL) {
+			off = GPIO_REG_OFFSET(pin);
+			bit = GPIO_REG_SHIFT(pin);
+
+			bcm2835_gpio_wr(pc, GPPUD, arg & 3);
+			/*
+			* Docs say to wait 150 cycles, but not of what. We assume a
+			* 1 MHz clock here, which is pretty slow...
+			*/
+			udelay(150);
+			bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
+			udelay(150);
+			bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+		} else if (param == BCM2835_PINCONF_PARAM_LEVEL) {
+			/* write in register */
+			bcm2835_gpio_set_bit(pc, arg ? GPSET0 : GPCLR0, pin);
+		} else {
 			return -EINVAL;
-
-		off = GPIO_REG_OFFSET(pin);
-		bit = GPIO_REG_SHIFT(pin);
-
-		bcm2835_gpio_wr(pc, GPPUD, arg & 3);
-		/*
-		 * BCM2835 datasheet say to wait 150 cycles, but not of what.
-		 * But the VideoCore firmware delay for this operation
-		 * based nearly on the same amount of VPU cycles and this clock
-		 * runs at 250 MHz.
-		 */
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+		}
 	} /* for each config */
 
 	return 0;
-- 
2.7.4

Re: [PATCH] pinctrl: bc2835: Add brcm,level property

[Eric Anholt]

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

Matheus Castello <matheus@castello.eng.br> writes:

> Property to set initial value of pin output buffer.

In the commit message, could you expand on the motivation for adding
this support?  Also, couldn't we reuse some existing
pinctrl-bindings.txt generic property?

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

Re: [PATCH] pinctrl: bc2835: Add brcm,level property

[Matheus Castello]

Property to set initial value of pin output buffer.
This can be useful for configure hardware in overlay files, and in early boot 
for checking it states in QA sanity tests.

This modification maintain the legacy property style, but the idea is to 
continue working in this driver to implement support for the generic binding 
properties too. So in the future this can be used to implement the 
output-high and output-low generic properties.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 .../bindings/pinctrl/brcm,bcm2835-gpio.txt         |  5 +-
 drivers/pinctrl/bcm/pinctrl-bcm2835.c              | 90 +++++++++++++++++-----
 2 files changed, 75 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
index 2569866..6834f1d 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
@@ -54,8 +54,11 @@ Optional subnode-properties:
   0: none
   1: down
   2: up
+- brcm,level: Integer, representing the value of output buffer to apply to the pin(s):
+  0: low
+  1: high
 
-Each of brcm,function and brcm,pull may contain either a single value which
+Each of brcm,function, brcm,pull and brcm,level may contain either a single value which
 will be applied to all pins in brcm,pins, or 1 value for each entry in
 brcm,pins.
 
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 785c366..0ace548 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -73,6 +73,8 @@
 enum bcm2835_pinconf_param {
 	/* argument: bcm2835_pinconf_pull */
 	BCM2835_PINCONF_PARAM_PULL,
+	/* argument: bcm2835_pinconf_level */
+	BCM2835_PINCONF_PARAM_LEVEL,
 };
 
 #define BCM2835_PINCONF_PACK(_param_, _arg_) ((_param_) << 16 | (_arg_))
@@ -725,16 +727,42 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
 	return 0;
 }
 
+static int bcm2835_pctl_dt_node_to_map_level(struct bcm2835_pinctrl *pc,
+		struct device_node *np, u32 pin, u32 level,
+		struct pinctrl_map **maps)
+{
+	struct pinctrl_map *map = *maps;
+	unsigned long *configs;
+
+	if (level > 2) {
+		dev_err(pc->dev, "%pOF: invalid brcm,level %d\n", np, level);
+		return -EINVAL;
+	}
+
+	configs = kzalloc(sizeof(*configs), GFP_KERNEL);
+	if (!configs)
+		return -ENOMEM;
+	configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_LEVEL, level);
+
+	map->type = PIN_MAP_TYPE_CONFIGS_PIN;
+	map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
+	map->data.configs.configs = configs;
+	map->data.configs.num_configs = 1;
+	(*maps)++;
+
+	return 0;
+}
+
 static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		struct device_node *np,
 		struct pinctrl_map **map, unsigned *num_maps)
 {
 	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
-	struct property *pins, *funcs, *pulls;
-	int num_pins, num_funcs, num_pulls, maps_per_pin;
+	struct property *pins, *funcs, *pulls, *levels;
+	int num_pins, num_funcs, num_pulls, num_levels, maps_per_pin;
 	struct pinctrl_map *maps, *cur_map;
 	int i, err;
-	u32 pin, func, pull;
+	u32 pin, func, pull, level;
 
 	pins = of_find_property(np, "brcm,pins", NULL);
 	if (!pins) {
@@ -744,6 +772,7 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 
 	funcs = of_find_property(np, "brcm,function", NULL);
 	pulls = of_find_property(np, "brcm,pull", NULL);
+	levels = of_find_property(np, "brcm,level", NULL);
 
 	if (!funcs && !pulls) {
 		dev_err(pc->dev,
@@ -755,6 +784,7 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 	num_pins = pins->length / 4;
 	num_funcs = funcs ? (funcs->length / 4) : 0;
 	num_pulls = pulls ? (pulls->length / 4) : 0;
+	num_levels = levels ? (levels->length / 4) : 0;
 
 	if (num_funcs > 1 && num_funcs != num_pins) {
 		dev_err(pc->dev,
@@ -770,11 +800,20 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		return -EINVAL;
 	}
 
+	if (num_levels > 1 && num_levels != num_pins) {
+		dev_err(pc->dev,
+			"%pOF: brcm,level must have 1 or %d entries\n",
+			np, num_pins);
+		return -EINVAL;
+	}
+
 	maps_per_pin = 0;
 	if (num_funcs)
 		maps_per_pin++;
 	if (num_pulls)
 		maps_per_pin++;
+	if (num_levels)
+		maps_per_pin++;
 	cur_map = maps = kzalloc(num_pins * maps_per_pin * sizeof(*maps),
 				GFP_KERNEL);
 	if (!maps)
@@ -811,6 +850,17 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 			if (err)
 				goto out;
 		}
+		if (num_levels) {
+			err = of_property_read_u32_index(np, "brcm,level",
+					(num_levels > 1) ? i : 0, &level);
+			if (err)
+				goto out;
+
+			err = bcm2835_pctl_dt_node_to_map_level(pc, np, pin,
+							level, &cur_map);
+			if (err)
+				goto out;
+		}
 	}
 
 	*map = maps;
@@ -931,23 +981,25 @@ static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 		param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
 		arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
 
-		if (param != BCM2835_PINCONF_PARAM_PULL)
+		if (param == BCM2835_PINCONF_PARAM_PULL) {
+			off = GPIO_REG_OFFSET(pin);
+			bit = GPIO_REG_SHIFT(pin);
+
+			bcm2835_gpio_wr(pc, GPPUD, arg & 3);
+			/*
+			* Docs say to wait 150 cycles, but not of what. We assume a
+			* 1 MHz clock here, which is pretty slow...
+			*/
+			udelay(150);
+			bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
+			udelay(150);
+			bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+		} else if (param == BCM2835_PINCONF_PARAM_LEVEL) {
+			/* write in register */
+			bcm2835_gpio_set_bit(pc, arg ? GPSET0 : GPCLR0, pin);
+		} else {
 			return -EINVAL;
-
-		off = GPIO_REG_OFFSET(pin);
-		bit = GPIO_REG_SHIFT(pin);
-
-		bcm2835_gpio_wr(pc, GPPUD, arg & 3);
-		/*
-		 * BCM2835 datasheet say to wait 150 cycles, but not of what.
-		 * But the VideoCore firmware delay for this operation
-		 * based nearly on the same amount of VPU cycles and this clock
-		 * runs at 250 MHz.
-		 */
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+		}
 	} /* for each config */
 
 	return 0;
-- 
2.7.4

Re: [PATCH] pinctrl: bc2835: Add brcm,level property

[Linus Walleij]

Hi Matheus,

sorry for top-posting, but new DT properties have to be ACKed
by the device tree maintainers, so quoting it all.

I understand why you use the legacy bcm-specific bindings for this,
but in theory nothing really stops you from just using
"output-high" and "output-low" from the generic properties,
even if you are not using the generic code to handle them but
instead parse them just like in this driver.

The only reason I could think of not to use them would be consistency,
like it would look not so elegant.

But if we look at the pin control bindings from a birds eye view
the big problem with consistency is the proliferation of custom
vendor bindings like this.

So there is a bit of conflict of interest here.

Yours,
Linus Walleij


On Thu, Feb 22, 2018 at 6:44 PM, Matheus Castello
<matheus@castello.eng.br> wrote:
> Property to set initial value of pin output buffer.
>
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  .../bindings/pinctrl/brcm,bcm2835-gpio.txt         |  5 +-
>  drivers/pinctrl/bcm/pinctrl-bcm2835.c              | 90 +++++++++++++++++-----
>  2 files changed, 75 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
> index 2569866..6834f1d 100644
> --- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
> @@ -54,8 +54,11 @@ Optional subnode-properties:
>    0: none
>    1: down
>    2: up
> +- brcm,level: Integer, representing the value of output buffer to apply to the pin(s):
> +  0: low
> +  1: high
>
> -Each of brcm,function and brcm,pull may contain either a single value which
> +Each of brcm,function, brcm,pull and brcm,level may contain either a single value which
>  will be applied to all pins in brcm,pins, or 1 value for each entry in
>  brcm,pins.
>
> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> index 785c366..0ace548 100644
> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> @@ -73,6 +73,8 @@
>  enum bcm2835_pinconf_param {
>         /* argument: bcm2835_pinconf_pull */
>         BCM2835_PINCONF_PARAM_PULL,
> +       /* argument: bcm2835_pinconf_level */
> +       BCM2835_PINCONF_PARAM_LEVEL,
>  };
>
>  #define BCM2835_PINCONF_PACK(_param_, _arg_) ((_param_) << 16 | (_arg_))
> @@ -725,16 +727,42 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
>         return 0;
>  }
>
> +static int bcm2835_pctl_dt_node_to_map_level(struct bcm2835_pinctrl *pc,
> +               struct device_node *np, u32 pin, u32 level,
> +               struct pinctrl_map **maps)
> +{
> +       struct pinctrl_map *map = *maps;
> +       unsigned long *configs;
> +
> +       if (level > 2) {
> +               dev_err(pc->dev, "%pOF: invalid brcm,level %d\n", np, level);
> +               return -EINVAL;
> +       }
> +
> +       configs = kzalloc(sizeof(*configs), GFP_KERNEL);
> +       if (!configs)
> +               return -ENOMEM;
> +       configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_LEVEL, level);
> +
> +       map->type = PIN_MAP_TYPE_CONFIGS_PIN;
> +       map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
> +       map->data.configs.configs = configs;
> +       map->data.configs.num_configs = 1;
> +       (*maps)++;
> +
> +       return 0;
> +}
> +
>  static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
>                 struct device_node *np,
>                 struct pinctrl_map **map, unsigned *num_maps)
>  {
>         struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> -       struct property *pins, *funcs, *pulls;
> -       int num_pins, num_funcs, num_pulls, maps_per_pin;
> +       struct property *pins, *funcs, *pulls, *levels;
> +       int num_pins, num_funcs, num_pulls, num_levels, maps_per_pin;
>         struct pinctrl_map *maps, *cur_map;
>         int i, err;
> -       u32 pin, func, pull;
> +       u32 pin, func, pull, level;
>
>         pins = of_find_property(np, "brcm,pins", NULL);
>         if (!pins) {
> @@ -744,6 +772,7 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
>
>         funcs = of_find_property(np, "brcm,function", NULL);
>         pulls = of_find_property(np, "brcm,pull", NULL);
> +       levels = of_find_property(np, "brcm,level", NULL);
>
>         if (!funcs && !pulls) {
>                 dev_err(pc->dev,
> @@ -755,6 +784,7 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
>         num_pins = pins->length / 4;
>         num_funcs = funcs ? (funcs->length / 4) : 0;
>         num_pulls = pulls ? (pulls->length / 4) : 0;
> +       num_levels = levels ? (levels->length / 4) : 0;
>
>         if (num_funcs > 1 && num_funcs != num_pins) {
>                 dev_err(pc->dev,
> @@ -770,11 +800,20 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
>                 return -EINVAL;
>         }
>
> +       if (num_levels > 1 && num_levels != num_pins) {
> +               dev_err(pc->dev,
> +                       "%pOF: brcm,level must have 1 or %d entries\n",
> +                       np, num_pins);
> +               return -EINVAL;
> +       }
> +
>         maps_per_pin = 0;
>         if (num_funcs)
>                 maps_per_pin++;
>         if (num_pulls)
>                 maps_per_pin++;
> +       if (num_levels)
> +               maps_per_pin++;
>         cur_map = maps = kzalloc(num_pins * maps_per_pin * sizeof(*maps),
>                                 GFP_KERNEL);
>         if (!maps)
> @@ -811,6 +850,17 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
>                         if (err)
>                                 goto out;
>                 }
> +               if (num_levels) {
> +                       err = of_property_read_u32_index(np, "brcm,level",
> +                                       (num_levels > 1) ? i : 0, &level);
> +                       if (err)
> +                               goto out;
> +
> +                       err = bcm2835_pctl_dt_node_to_map_level(pc, np, pin,
> +                                                       level, &cur_map);
> +                       if (err)
> +                               goto out;
> +               }
>         }
>
>         *map = maps;
> @@ -931,23 +981,25 @@ static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
>                 param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
>                 arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
>
> -               if (param != BCM2835_PINCONF_PARAM_PULL)
> +               if (param == BCM2835_PINCONF_PARAM_PULL) {
> +                       off = GPIO_REG_OFFSET(pin);
> +                       bit = GPIO_REG_SHIFT(pin);
> +
> +                       bcm2835_gpio_wr(pc, GPPUD, arg & 3);
> +                       /*
> +                       * Docs say to wait 150 cycles, but not of what. We assume a
> +                       * 1 MHz clock here, which is pretty slow...
> +                       */
> +                       udelay(150);
> +                       bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
> +                       udelay(150);
> +                       bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
> +               } else if (param == BCM2835_PINCONF_PARAM_LEVEL) {
> +                       /* write in register */
> +                       bcm2835_gpio_set_bit(pc, arg ? GPSET0 : GPCLR0, pin);
> +               } else {
>                         return -EINVAL;
> -
> -               off = GPIO_REG_OFFSET(pin);
> -               bit = GPIO_REG_SHIFT(pin);
> -
> -               bcm2835_gpio_wr(pc, GPPUD, arg & 3);
> -               /*
> -                * BCM2835 datasheet say to wait 150 cycles, but not of what.
> -                * But the VideoCore firmware delay for this operation
> -                * based nearly on the same amount of VPU cycles and this clock
> -                * runs at 250 MHz.
> -                */
> -               udelay(1);
> -               bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
> -               udelay(1);
> -               bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
> +               }
>         } /* for each config */
>
>         return 0;
> --
> 2.7.4
>

Re: [PATCH] pinctrl: bc2835: Add brcm,level property

[Stefan Wahren]

Hi Matheus,

sorry for my late reply.

> Matheus Castello <matheus@castello.eng.br> hat am 22. Februar 2018 um 18:44 geschrieben:
> 
> 
> Property to set initial value of pin output buffer.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  .../bindings/pinctrl/brcm,bcm2835-gpio.txt         |  5 +-
>  drivers/pinctrl/bcm/pinctrl-bcm2835.c              | 90 +++++++++++++++++-----
>  2 files changed, 75 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
> index 2569866..6834f1d 100644
> --- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt

Changes to the devicetree binding should be a separate patch. Please read [1] and use the get_maintainers script for all patches.

[1] - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/submitting-patches.txt

> @@ -54,8 +54,11 @@ Optional subnode-properties:
>    0: none
>    1: down
>    2: up
> +- brcm,level: Integer, representing the value of output buffer to apply to the pin(s):
> +  0: low
> +  1: high

As Linus suggested please don't introduce new legacy properties. We already have generic ones for this. After mention the supported properties, simply make a reference to the generic binding.

>  
> -Each of brcm,function and brcm,pull may contain either a single value which
> +Each of brcm,function, brcm,pull and brcm,level may contain either a single value which
>  will be applied to all pins in brcm,pins, or 1 value for each entry in
>  brcm,pins.
>  
> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> index 785c366..0ace548 100644
> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> @@ -73,6 +73,8 @@
>  enum bcm2835_pinconf_param {
>  	/* argument: bcm2835_pinconf_pull */
>  	BCM2835_PINCONF_PARAM_PULL,
> +	/* argument: bcm2835_pinconf_level */
> +	BCM2835_PINCONF_PARAM_LEVEL,
>  };
>  
>  #define BCM2835_PINCONF_PACK(_param_, _arg_) ((_param_) << 16 | (_arg_))
> @@ -725,16 +727,42 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
>  	return 0;
>  }
>  
> +static int bcm2835_pctl_dt_node_to_map_level(struct bcm2835_pinctrl *pc,
> +		struct device_node *np, u32 pin, u32 level,
> +		struct pinctrl_map **maps)
> +{
> +	struct pinctrl_map *map = *maps;
> +	unsigned long *configs;
> +
> +	if (level > 2) {
> +		dev_err(pc->dev, "%pOF: invalid brcm,level %d\n", np, level);
> +		return -EINVAL;
> +	}
> +
> +	configs = kzalloc(sizeof(*configs), GFP_KERNEL);
> +	if (!configs)
> +		return -ENOMEM;
> +	configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_LEVEL, level);
> +
> +	map->type = PIN_MAP_TYPE_CONFIGS_PIN;
> +	map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
> +	map->data.configs.configs = configs;
> +	map->data.configs.num_configs = 1;
> +	(*maps)++;
> +
> +	return 0;
> +}
> +
>  static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
>  		struct device_node *np,
>  		struct pinctrl_map **map, unsigned *num_maps)
>  {
>  	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> -	struct property *pins, *funcs, *pulls;
> -	int num_pins, num_funcs, num_pulls, maps_per_pin;
> +	struct property *pins, *funcs, *pulls, *levels;
> +	int num_pins, num_funcs, num_pulls, num_levels, maps_per_pin;
>  	struct pinctrl_map *maps, *cur_map;
>  	int i, err;
> -	u32 pin, func, pull;
> +	u32 pin, func, pull, level;
>  
>  	pins = of_find_property(np, "brcm,pins", NULL);
>  	if (!pins) {
> @@ -744,6 +772,7 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
>  
>  	funcs = of_find_property(np, "brcm,function", NULL);
>  	pulls = of_find_property(np, "brcm,pull", NULL);
> +	levels = of_find_property(np, "brcm,level", NULL);
>  
>  	if (!funcs && !pulls) {
>  		dev_err(pc->dev,
> @@ -755,6 +784,7 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
>  	num_pins = pins->length / 4;
>  	num_funcs = funcs ? (funcs->length / 4) : 0;
>  	num_pulls = pulls ? (pulls->length / 4) : 0;
> +	num_levels = levels ? (levels->length / 4) : 0;
>  
>  	if (num_funcs > 1 && num_funcs != num_pins) {
>  		dev_err(pc->dev,
> @@ -770,11 +800,20 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
>  		return -EINVAL;
>  	}
>  
> +	if (num_levels > 1 && num_levels != num_pins) {
> +		dev_err(pc->dev,
> +			"%pOF: brcm,level must have 1 or %d entries\n",
> +			np, num_pins);
> +		return -EINVAL;
> +	}
> +
>  	maps_per_pin = 0;
>  	if (num_funcs)
>  		maps_per_pin++;
>  	if (num_pulls)
>  		maps_per_pin++;
> +	if (num_levels)
> +		maps_per_pin++;
>  	cur_map = maps = kzalloc(num_pins * maps_per_pin * sizeof(*maps),
>  				GFP_KERNEL);
>  	if (!maps)
> @@ -811,6 +850,17 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
>  			if (err)
>  				goto out;
>  		}
> +		if (num_levels) {
> +			err = of_property_read_u32_index(np, "brcm,level",
> +					(num_levels > 1) ? i : 0, &level);
> +			if (err)
> +				goto out;
> +
> +			err = bcm2835_pctl_dt_node_to_map_level(pc, np, pin,
> +							level, &cur_map);
> +			if (err)
> +				goto out;
> +		}
>  	}
>  
>  	*map = maps;
> @@ -931,23 +981,25 @@ static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
>  		param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
>  		arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
>  
> -		if (param != BCM2835_PINCONF_PARAM_PULL)
> +		if (param == BCM2835_PINCONF_PARAM_PULL) {
> +			off = GPIO_REG_OFFSET(pin);
> +			bit = GPIO_REG_SHIFT(pin);
> +
> +			bcm2835_gpio_wr(pc, GPPUD, arg & 3);
> +			/*
> +			* Docs say to wait 150 cycles, but not of what. We assume a
> +			* 1 MHz clock here, which is pretty slow...
> +			*/
> +			udelay(150);
> +			bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
> +			udelay(150);
> +			bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
> +		} else if (param == BCM2835_PINCONF_PARAM_LEVEL) {
> +			/* write in register */
> +			bcm2835_gpio_set_bit(pc, arg ? GPSET0 : GPCLR0, pin);
> +		} else {

Please separate the restructuring and the really new code into 2 patches. This make it easier to review.

Thanks
Stefan

>  			return -EINVAL;
> -
> -		off = GPIO_REG_OFFSET(pin);
> -		bit = GPIO_REG_SHIFT(pin);
> -
> -		bcm2835_gpio_wr(pc, GPPUD, arg & 3);
> -		/*
> -		 * BCM2835 datasheet say to wait 150 cycles, but not of what.
> -		 * But the VideoCore firmware delay for this operation
> -		 * based nearly on the same amount of VPU cycles and this clock
> -		 * runs at 250 MHz.
> -		 */
> -		udelay(1);
> -		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
> -		udelay(1);
> -		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
> +		}
>  	} /* for each config */
>  
>  	return 0;
> -- 
> 2.7.4
>

Re: [PATCH v2 0/3] pinctrl: bcm2835: Add brcm,level property

[Matheus Castello]

Hi Linus and Stefan,

thanks for the tips.

This series adds support for generic binding for pinctrl bcm2835 driver,
and add the code for set output buffer of a pin using the output-low and
output-high generic properties.

Tested on Raspberry Pi Zero W, based on bcm2835 SoC.

Matheus Castello (3):
  pinctrl: bcm2835: switch to GENERIC_PINCONF
  pinctrl: bcm2835: Add support for generic pinctrl binding
  pinctrl: bcm2835: Add support for output-low output-high properties

 drivers/pinctrl/bcm/Kconfig           |  1 +
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 88 +++++++++++++++++++++++------------
 2 files changed, 60 insertions(+), 29 deletions(-)

--
2.7.4

[PATCH v2 1/3] pinctrl: bcm2835: switch to GENERIC_PINCONF

[Matheus Castello]

To enable support for generic binding in the pinctrl-bcm2835 and use pinctrl
generic to parse properties the GENERIC_PINCONF have to be selected.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/pinctrl/bcm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig
index e8c4e4f..0f38d51 100644
--- a/drivers/pinctrl/bcm/Kconfig
+++ b/drivers/pinctrl/bcm/Kconfig
@@ -20,6 +20,7 @@ config PINCTRL_BCM2835
 	bool
 	select PINMUX
 	select PINCONF
+	select GENERIC_PINCONF
 	select GPIOLIB_IRQCHIP

 config PINCTRL_IPROC_GPIO
--
2.7.4

Re: [PATCH v2 1/3] pinctrl: bcm2835: switch to GENERIC_PINCONF

[Stefan Wahren]

Hi Matheus,

> Matheus Castello <matheus@castello.eng.br> hat am 5. März 2018 um 03:29 geschrieben:
> 
> 
> To enable support for generic binding in the pinctrl-bcm2835 and use pinctrl
> generic to parse properties the GENERIC_PINCONF have to be selected.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  drivers/pinctrl/bcm/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig
> index e8c4e4f..0f38d51 100644
> --- a/drivers/pinctrl/bcm/Kconfig
> +++ b/drivers/pinctrl/bcm/Kconfig
> @@ -20,6 +20,7 @@ config PINCTRL_BCM2835
>  	bool
>  	select PINMUX
>  	select PINCONF
> +	select GENERIC_PINCONF
>  	select GPIOLIB_IRQCHIP
> 
>  config PINCTRL_IPROC_GPIO

i think it's okay to fold this change into patch #2.

> --
> 2.7.4
>

[PATCH v2 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Matheus Castello]

To keep driver up to date we add generic pinctrl binding support, which covers
the features used in this driver and has additional node properties that this
SoC has compatibility, so enabling future implementations of these properties
without the need to create new node properties in the device trees.

The logic of this change maintain the old brcm legacy binding support in order
to keep the ABI stable.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---

A brief explanation of what I did:

Add pinconf-generic header for use the defines and pinctrl-generic API.

Add dt-bindings pinctrl bcm2835 header to use functions selections and
pulls definitions, which functions definitions where duplicated in the
enum bcm2835_fsel, I removed the duplicate defines from enum.

In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for
pack the legacy param and arguments, since it will be unpacked along with
generic properties that is packed with this same macro.

In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use
pinctrl-generic parse code instead of parsing it inside the driver, so code
first check for generic binding parse, if something is parsed then it is
assumed that are using the new generic style, and when nothing is found then
parse continues to search for legacy properties.

In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and
was added a switch for the parameter tests, since pinctrl generic uses 3
properties to define the states of the pull instead of one with arguments, that
was the reason too that bcm2835_pull_config_set function was added, for reuse
the code that set state of pull.

 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 83 +++++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 29 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 785c366..755ea90 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -36,11 +36,13 @@
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
+#include <dt-bindings/pinctrl/bcm2835.h>

 #define MODULE_NAME "pinctrl-bcm2835"
 #define BCM2835_NUM_GPIOS 54
@@ -213,14 +215,6 @@ static const char * const bcm2835_gpio_groups[] = {
 };

 enum bcm2835_fsel {
-	BCM2835_FSEL_GPIO_IN = 0,
-	BCM2835_FSEL_GPIO_OUT = 1,
-	BCM2835_FSEL_ALT0 = 4,
-	BCM2835_FSEL_ALT1 = 5,
-	BCM2835_FSEL_ALT2 = 6,
-	BCM2835_FSEL_ALT3 = 7,
-	BCM2835_FSEL_ALT4 = 3,
-	BCM2835_FSEL_ALT5 = 2,
 	BCM2835_FSEL_COUNT = 8,
 	BCM2835_FSEL_MASK = 0x7,
 };
@@ -714,7 +708,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
 	configs = kzalloc(sizeof(*configs), GFP_KERNEL);
 	if (!configs)
 		return -ENOMEM;
-	configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_PULL, pull);
+	configs[0] = PIN_CONF_PACKED(BCM2835_PINCONF_PARAM_PULL, pull);

 	map->type = PIN_MAP_TYPE_CONFIGS_PIN;
 	map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
@@ -736,6 +730,12 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 	int i, err;
 	u32 pin, func, pull;

+	/* Check for generic binding in this node */
+	err = pinconf_generic_dt_node_to_map_all(pctldev, np, map, num_maps);
+	if (err || *num_maps)
+		return err;
+
+	/* Generic binding did not find anything continue with legacy parse */
 	pins = of_find_property(np, "brcm,pins", NULL);
 	if (!pins) {
 		dev_err(pc->dev, "%pOF: missing brcm,pins property\n", np);
@@ -917,37 +917,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev,
 	return -ENOTSUPP;
 }

+static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc,
+		unsigned pin, unsigned arg)
+{
+	u32 off, bit;
+
+	off = GPIO_REG_OFFSET(pin);
+	bit = GPIO_REG_SHIFT(pin);
+
+	bcm2835_gpio_wr(pc, GPPUD, arg & 3);
+	/*
+	* BCM2835 datasheet say to wait 150 cycles, but not of what.
+	* But the VideoCore firmware delay for this operation
+	* based nearly on the same amount of VPU cycles and this clock
+	* runs at 250 MHz.
+	*/
+	udelay(1);
+	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
+	udelay(1);
+	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+}
+
 static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 			unsigned pin, unsigned long *configs,
 			unsigned num_configs)
 {
 	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
-	enum bcm2835_pinconf_param param;
-	u16 arg;
-	u32 off, bit;
+	u16 param, arg;
 	int i;

 	for (i = 0; i < num_configs; i++) {
-		param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
-		arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
+		param = configs[i] & 0xffUL;
+		arg = configs[i] >> 8;

-		if (param != BCM2835_PINCONF_PARAM_PULL)
-			return -EINVAL;
+		switch (param) {
+		/* Set legacy brcm,pull */
+		case BCM2835_PINCONF_PARAM_PULL:
+			bcm2835_pull_config_set(pc, pin, arg);
+			break;

-		off = GPIO_REG_OFFSET(pin);
-		bit = GPIO_REG_SHIFT(pin);
+		/* Set pull generic bindings */
+		case PIN_CONFIG_BIAS_DISABLE:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_OFF);
+			break;

-		bcm2835_gpio_wr(pc, GPPUD, arg & 3);
-		/*
-		 * BCM2835 datasheet say to wait 150 cycles, but not of what.
-		 * But the VideoCore firmware delay for this operation
-		 * based nearly on the same amount of VPU cycles and this clock
-		 * runs at 250 MHz.
-		 */
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_DOWN);
+			break;
+
+		case PIN_CONFIG_BIAS_PULL_UP:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
+			break;
+
+		default:
+			return -EINVAL;
+
+		} /* switch param type */
 	} /* for each config */

 	return 0;
--
2.7.4

Re: [PATCH v2 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Eric Anholt]

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

Matheus Castello <matheus@castello.eng.br> writes:

> To keep driver up to date we add generic pinctrl binding support, which covers
> the features used in this driver and has additional node properties that this
> SoC has compatibility, so enabling future implementations of these properties
> without the need to create new node properties in the device trees.
>
> The logic of this change maintain the old brcm legacy binding support in order
> to keep the ABI stable.
>
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>
> A brief explanation of what I did:
>
> Add pinconf-generic header for use the defines and pinctrl-generic API.
>
> Add dt-bindings pinctrl bcm2835 header to use functions selections and
> pulls definitions, which functions definitions where duplicated in the
> enum bcm2835_fsel, I removed the duplicate defines from enum.
>
> In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for
> pack the legacy param and arguments, since it will be unpacked along with
> generic properties that is packed with this same macro.
>
> In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use
> pinctrl-generic parse code instead of parsing it inside the driver, so code
> first check for generic binding parse, if something is parsed then it is
> assumed that are using the new generic style, and when nothing is found then
> parse continues to search for legacy properties.
>
> In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and
> was added a switch for the parameter tests, since pinctrl generic uses 3
> properties to define the states of the pull instead of one with arguments, that
> was the reason too that bcm2835_pull_config_set function was added, for reuse
> the code that set state of pull.
>
>  drivers/pinctrl/bcm/pinctrl-bcm2835.c | 83 +++++++++++++++++++++++------------
>  1 file changed, 54 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> index 785c366..755ea90 100644
> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> @@ -36,11 +36,13 @@
>  #include <linux/pinctrl/pinconf.h>
>  #include <linux/pinctrl/pinctrl.h>
>  #include <linux/pinctrl/pinmux.h>
> +#include <linux/pinctrl/pinconf-generic.h>
>  #include <linux/platform_device.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/types.h>
> +#include <dt-bindings/pinctrl/bcm2835.h>
>
>  #define MODULE_NAME "pinctrl-bcm2835"
>  #define BCM2835_NUM_GPIOS 54
> @@ -213,14 +215,6 @@ static const char * const bcm2835_gpio_groups[] = {
>  };
>
>  enum bcm2835_fsel {
> -	BCM2835_FSEL_GPIO_IN = 0,
> -	BCM2835_FSEL_GPIO_OUT = 1,
> -	BCM2835_FSEL_ALT0 = 4,
> -	BCM2835_FSEL_ALT1 = 5,
> -	BCM2835_FSEL_ALT2 = 6,
> -	BCM2835_FSEL_ALT3 = 7,
> -	BCM2835_FSEL_ALT4 = 3,
> -	BCM2835_FSEL_ALT5 = 2,
>  	BCM2835_FSEL_COUNT = 8,
>  	BCM2835_FSEL_MASK = 0x7,
>  };
> @@ -714,7 +708,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
>  	configs = kzalloc(sizeof(*configs), GFP_KERNEL);
>  	if (!configs)
>  		return -ENOMEM;
> -	configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_PULL, pull);
> +	configs[0] = PIN_CONF_PACKED(BCM2835_PINCONF_PARAM_PULL, pull);
>
>  	map->type = PIN_MAP_TYPE_CONFIGS_PIN;
>  	map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
> @@ -736,6 +730,12 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
>  	int i, err;
>  	u32 pin, func, pull;
>
> +	/* Check for generic binding in this node */
> +	err = pinconf_generic_dt_node_to_map_all(pctldev, np, map, num_maps);
> +	if (err || *num_maps)
> +		return err;
> +
> +	/* Generic binding did not find anything continue with legacy parse */
>  	pins = of_find_property(np, "brcm,pins", NULL);
>  	if (!pins) {
>  		dev_err(pc->dev, "%pOF: missing brcm,pins property\n", np);
> @@ -917,37 +917,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev,
>  	return -ENOTSUPP;
>  }
>
> +static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc,
> +		unsigned pin, unsigned arg)
> +{
> +	u32 off, bit;
> +
> +	off = GPIO_REG_OFFSET(pin);
> +	bit = GPIO_REG_SHIFT(pin);
> +
> +	bcm2835_gpio_wr(pc, GPPUD, arg & 3);
> +	/*
> +	* BCM2835 datasheet say to wait 150 cycles, but not of what.
> +	* But the VideoCore firmware delay for this operation
> +	* based nearly on the same amount of VPU cycles and this clock
> +	* runs at 250 MHz.
> +	*/
> +	udelay(1);
> +	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
> +	udelay(1);
> +	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
> +}
> +
>  static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
>  			unsigned pin, unsigned long *configs,
>  			unsigned num_configs)
>  {
>  	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> -	enum bcm2835_pinconf_param param;
> -	u16 arg;
> -	u32 off, bit;
> +	u16 param, arg;
>  	int i;
>
>  	for (i = 0; i < num_configs; i++) {
> -		param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
> -		arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
> +		param = configs[i] & 0xffUL;
> +		arg = configs[i] >> 8;

If you're removing the usage of BCM2835_PINCONF_PACK() and UNPACK, let's
delete those macros, too.  Also, it looks like there's
"pinconf_to_config_param()" for unpacking, instead of rolling your own.

Other than that, it looks good to me.  I'll let Linus comment on whether
this is handling the generic properties correctly.

>
> -		if (param != BCM2835_PINCONF_PARAM_PULL)
> -			return -EINVAL;
> +		switch (param) {
> +		/* Set legacy brcm,pull */
> +		case BCM2835_PINCONF_PARAM_PULL:
> +			bcm2835_pull_config_set(pc, pin, arg);
> +			break;
>
> -		off = GPIO_REG_OFFSET(pin);
> -		bit = GPIO_REG_SHIFT(pin);
> +		/* Set pull generic bindings */
> +		case PIN_CONFIG_BIAS_DISABLE:
> +			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_OFF);
> +			break;
>
> -		bcm2835_gpio_wr(pc, GPPUD, arg & 3);
> -		/*
> -		 * BCM2835 datasheet say to wait 150 cycles, but not of what.
> -		 * But the VideoCore firmware delay for this operation
> -		 * based nearly on the same amount of VPU cycles and this clock
> -		 * runs at 250 MHz.
> -		 */
> -		udelay(1);
> -		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
> -		udelay(1);
> -		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
> +		case PIN_CONFIG_BIAS_PULL_DOWN:
> +			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_DOWN);
> +			break;
> +
> +		case PIN_CONFIG_BIAS_PULL_UP:
> +			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
> +			break;
> +
> +		default:
> +			return -EINVAL;
> +
> +		} /* switch param type */
>  	} /* for each config */
>
>  	return 0;
> --
> 2.7.4

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

Re: [PATCH v2 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Matheus Castello]

Hi Eric,

thanks for reviewing it. I will send the v3 of the patch with your notes. 
I will just wait for the Linus considerations.

Best Regards
Matheus Castello

On 03/05/2018 07:21 PM, Eric Anholt wrote:
> Matheus Castello <matheus@castello.eng.br> writes:
> 
>> To keep driver up to date we add generic pinctrl binding support, which covers
>> the features used in this driver and has additional node properties that this
>> SoC has compatibility, so enabling future implementations of these properties
>> without the need to create new node properties in the device trees.
>>
>> The logic of this change maintain the old brcm legacy binding support in order
>> to keep the ABI stable.
>>
>> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
>> ---
>>
>> A brief explanation of what I did:
>>
>> Add pinconf-generic header for use the defines and pinctrl-generic API.
>>
>> Add dt-bindings pinctrl bcm2835 header to use functions selections and
>> pulls definitions, which functions definitions where duplicated in the
>> enum bcm2835_fsel, I removed the duplicate defines from enum.
>>
>> In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for
>> pack the legacy param and arguments, since it will be unpacked along with
>> generic properties that is packed with this same macro.
>>
>> In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use
>> pinctrl-generic parse code instead of parsing it inside the driver, so code
>> first check for generic binding parse, if something is parsed then it is
>> assumed that are using the new generic style, and when nothing is found then
>> parse continues to search for legacy properties.
>>
>> In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and
>> was added a switch for the parameter tests, since pinctrl generic uses 3
>> properties to define the states of the pull instead of one with arguments, that
>> was the reason too that bcm2835_pull_config_set function was added, for reuse
>> the code that set state of pull.
>>
>>  drivers/pinctrl/bcm/pinctrl-bcm2835.c | 83 +++++++++++++++++++++++------------
>>  1 file changed, 54 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>> index 785c366..755ea90 100644
>> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>> @@ -36,11 +36,13 @@
>>  #include <linux/pinctrl/pinconf.h>
>>  #include <linux/pinctrl/pinctrl.h>
>>  #include <linux/pinctrl/pinmux.h>
>> +#include <linux/pinctrl/pinconf-generic.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/seq_file.h>
>>  #include <linux/slab.h>
>>  #include <linux/spinlock.h>
>>  #include <linux/types.h>
>> +#include <dt-bindings/pinctrl/bcm2835.h>
>>
>>  #define MODULE_NAME "pinctrl-bcm2835"
>>  #define BCM2835_NUM_GPIOS 54
>> @@ -213,14 +215,6 @@ static const char * const bcm2835_gpio_groups[] = {
>>  };
>>
>>  enum bcm2835_fsel {
>> -	BCM2835_FSEL_GPIO_IN = 0,
>> -	BCM2835_FSEL_GPIO_OUT = 1,
>> -	BCM2835_FSEL_ALT0 = 4,
>> -	BCM2835_FSEL_ALT1 = 5,
>> -	BCM2835_FSEL_ALT2 = 6,
>> -	BCM2835_FSEL_ALT3 = 7,
>> -	BCM2835_FSEL_ALT4 = 3,
>> -	BCM2835_FSEL_ALT5 = 2,
>>  	BCM2835_FSEL_COUNT = 8,
>>  	BCM2835_FSEL_MASK = 0x7,
>>  };
>> @@ -714,7 +708,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
>>  	configs = kzalloc(sizeof(*configs), GFP_KERNEL);
>>  	if (!configs)
>>  		return -ENOMEM;
>> -	configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_PULL, pull);
>> +	configs[0] = PIN_CONF_PACKED(BCM2835_PINCONF_PARAM_PULL, pull);
>>
>>  	map->type = PIN_MAP_TYPE_CONFIGS_PIN;
>>  	map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
>> @@ -736,6 +730,12 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
>>  	int i, err;
>>  	u32 pin, func, pull;
>>
>> +	/* Check for generic binding in this node */
>> +	err = pinconf_generic_dt_node_to_map_all(pctldev, np, map, num_maps);
>> +	if (err || *num_maps)
>> +		return err;
>> +
>> +	/* Generic binding did not find anything continue with legacy parse */
>>  	pins = of_find_property(np, "brcm,pins", NULL);
>>  	if (!pins) {
>>  		dev_err(pc->dev, "%pOF: missing brcm,pins property\n", np);
>> @@ -917,37 +917,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev,
>>  	return -ENOTSUPP;
>>  }
>>
>> +static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc,
>> +		unsigned pin, unsigned arg)
>> +{
>> +	u32 off, bit;
>> +
>> +	off = GPIO_REG_OFFSET(pin);
>> +	bit = GPIO_REG_SHIFT(pin);
>> +
>> +	bcm2835_gpio_wr(pc, GPPUD, arg & 3);
>> +	/*
>> +	* BCM2835 datasheet say to wait 150 cycles, but not of what.
>> +	* But the VideoCore firmware delay for this operation
>> +	* based nearly on the same amount of VPU cycles and this clock
>> +	* runs at 250 MHz.
>> +	*/
>> +	udelay(1);
>> +	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
>> +	udelay(1);
>> +	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
>> +}
>> +
>>  static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
>>  			unsigned pin, unsigned long *configs,
>>  			unsigned num_configs)
>>  {
>>  	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
>> -	enum bcm2835_pinconf_param param;
>> -	u16 arg;
>> -	u32 off, bit;
>> +	u16 param, arg;
>>  	int i;
>>
>>  	for (i = 0; i < num_configs; i++) {
>> -		param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
>> -		arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
>> +		param = configs[i] & 0xffUL;
>> +		arg = configs[i] >> 8;
> 
> If you're removing the usage of BCM2835_PINCONF_PACK() and UNPACK, let's
> delete those macros, too.  Also, it looks like there's
> "pinconf_to_config_param()" for unpacking, instead of rolling your own.
> 
> Other than that, it looks good to me.  I'll let Linus comment on whether
> this is handling the generic properties correctly.
> 
>>
>> -		if (param != BCM2835_PINCONF_PARAM_PULL)
>> -			return -EINVAL;
>> +		switch (param) {
>> +		/* Set legacy brcm,pull */
>> +		case BCM2835_PINCONF_PARAM_PULL:
>> +			bcm2835_pull_config_set(pc, pin, arg);
>> +			break;
>>
>> -		off = GPIO_REG_OFFSET(pin);
>> -		bit = GPIO_REG_SHIFT(pin);
>> +		/* Set pull generic bindings */
>> +		case PIN_CONFIG_BIAS_DISABLE:
>> +			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_OFF);
>> +			break;
>>
>> -		bcm2835_gpio_wr(pc, GPPUD, arg & 3);
>> -		/*
>> -		 * BCM2835 datasheet say to wait 150 cycles, but not of what.
>> -		 * But the VideoCore firmware delay for this operation
>> -		 * based nearly on the same amount of VPU cycles and this clock
>> -		 * runs at 250 MHz.
>> -		 */
>> -		udelay(1);
>> -		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
>> -		udelay(1);
>> -		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
>> +		case PIN_CONFIG_BIAS_PULL_DOWN:
>> +			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_DOWN);
>> +			break;
>> +
>> +		case PIN_CONFIG_BIAS_PULL_UP:
>> +			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
>> +			break;
>> +
>> +		default:
>> +			return -EINVAL;
>> +
>> +		} /* switch param type */
>>  	} /* for each config */
>>
>>  	return 0;
>> --
>> 2.7.4

[PATCH v2 3/3] pinctrl: bcm2835: Add support for output-low output-high properties

[Matheus Castello]

Properties to set initial value of pin output buffer.
This can be useful for configure hardware in overlay files, and in early boot
for checking it states in QA sanity tests.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 755ea90..a7a8199 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -969,6 +969,11 @@ static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
 			break;

+		/* Set output-high or output-low */
+		case PIN_CONFIG_OUTPUT:
+			bcm2835_gpio_set_bit(pc, arg ? GPSET0 : GPCLR0, pin);
+			break;
+
 		default:
 			return -EINVAL;

--
2.7.4

Re: [PATCH v2 0/3] pinctrl: bcm2835: Add brcm,level property

[Stefan Wahren]

Hi Matheus,

> Matheus Castello <matheus@castello.eng.br> hat am 5. März 2018 um 03:29 geschrieben:
> 
> 
> Hi Linus and Stefan,
> 
> thanks for the tips.
> 
> This series adds support for generic binding for pinctrl bcm2835 driver,
> and add the code for set output buffer of a pin using the output-low and
> output-high generic properties.
> 
> Tested on Raspberry Pi Zero W, based on bcm2835 SoC.
> 
> Matheus Castello (3):

looks like you missed the changes to the dt binding. Please also add Rob Herring and Mark Rutland to CC. We need a ACK from them.

>   pinctrl: bcm2835: switch to GENERIC_PINCONF
>   pinctrl: bcm2835: Add support for generic pinctrl binding
>   pinctrl: bcm2835: Add support for output-low output-high properties
> 
>  drivers/pinctrl/bcm/Kconfig           |  1 +
>  drivers/pinctrl/bcm/pinctrl-bcm2835.c | 88 +++++++++++++++++++++++------------
>  2 files changed, 60 insertions(+), 29 deletions(-)
> 
> --
> 2.7.4
>

Re: [PATCH v2 0/3] pinctrl: bcm2835: Add brcm,level property

[Matheus Castello]

Hi Stefan,

On 03/07/2018 07:58 AM, Stefan Wahren wrote:
> Hi Matheus,
> 
>> Matheus Castello <matheus@castello.eng.br> hat am 5. März 2018 um 03:29 geschrieben:
>>
>>
>> Hi Linus and Stefan,
>>
>> thanks for the tips.
>>
>> This series adds support for generic binding for pinctrl bcm2835 driver,
>> and add the code for set output buffer of a pin using the output-low and
>> output-high generic properties.
>>
>> Tested on Raspberry Pi Zero W, based on bcm2835 SoC.
>>
>> Matheus Castello (3):
> 
> looks like you missed the changes to the dt binding. Please also add Rob Herring and Mark Rutland to CC. We need a ACK from them.

are you talking about the .dts files? I add an overlay file for my tests. 
I was thinking it would be better to start with this changes in the driver and then in another series to convert to the generic style in the .dts.
If you find it better already add the conversion also in this series let me know.

I understood that I should put the device tree maintainers in thread only if I were to introduce a new property. As I am using generic properties and not introducing new ones I did not add them.
Let me know if it is still necessary I stayed in this doubt.

Best Regards
Matheus Castello
> 
>>   pinctrl: bcm2835: switch to GENERIC_PINCONF
>>   pinctrl: bcm2835: Add support for generic pinctrl binding
>>   pinctrl: bcm2835: Add support for output-low output-high properties
>>
>>  drivers/pinctrl/bcm/Kconfig           |  1 +
>>  drivers/pinctrl/bcm/pinctrl-bcm2835.c | 88 +++++++++++++++++++++++------------
>>  2 files changed, 60 insertions(+), 29 deletions(-)
>>
>> --
>> 2.7.4
>>

Re: [PATCH v2 0/3] pinctrl: bcm2835: Add brcm,level property

[Stefan Wahren]

Hi Matheus,

> Matheus Castello <matheus@castello.eng.br> hat am 8. März 2018 um 01:12 geschrieben:
> 
> 
> Hi Stefan,
> 
> On 03/07/2018 07:58 AM, Stefan Wahren wrote:
> > Hi Matheus,
> > 
> >> Matheus Castello <matheus@castello.eng.br> hat am 5. März 2018 um 03:29 geschrieben:
> >>
> >>
> >> Hi Linus and Stefan,
> >>
> >> thanks for the tips.
> >>
> >> This series adds support for generic binding for pinctrl bcm2835 driver,
> >> and add the code for set output buffer of a pin using the output-low and
> >> output-high generic properties.
> >>
> >> Tested on Raspberry Pi Zero W, based on bcm2835 SoC.
> >>
> >> Matheus Castello (3):
> > 
> > looks like you missed the changes to the dt binding. Please also add Rob Herring and Mark Rutland to CC. We need a ACK from them.
> 
> are you talking about the .dts files? I add an overlay file for my tests. 
> I was thinking it would be better to start with this changes in the driver and then in another series to convert to the generic style in the .dts.
> If you find it better already add the conversion also in this series let me know.
> 
> I understood that I should put the device tree maintainers in thread only if I were to introduce a new property. As I am using generic properties and not introducing new ones I did not add them.
> Let me know if it is still necessary I stayed in this doubt.

no this is a misunderstanding. You are changing the DT interface because the driver supports new properties (from driver point of view). The DT users usually look at the binding document [1] not the source code. So you need to extend the binding document before changing the source code within a patch series. At the end we need a DT maintainer's ACK for the binding change. Using the generic properties doesn't allow us to skip this, but it increases the chance of an ACK.

Btw it's okay to keep the DTS files.

Stefan

[1] - https://elixir.bootlin.com/linux/v4.16-rc4/source/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt

> 
> Best Regards
> Matheus Castello
> > 
> >>   pinctrl: bcm2835: switch to GENERIC_PINCONF
> >>   pinctrl: bcm2835: Add support for generic pinctrl binding
> >>   pinctrl: bcm2835: Add support for output-low output-high properties
> >>
> >>  drivers/pinctrl/bcm/Kconfig           |  1 +
> >>  drivers/pinctrl/bcm/pinctrl-bcm2835.c | 88 +++++++++++++++++++++++------------
> >>  2 files changed, 60 insertions(+), 29 deletions(-)
> >>
> >> --
> >> 2.7.4
> >>

Re: [PATCH v2 0/3] pinctrl: bcm2835: Add brcm,level property

[Matheus Castello]

Hi Stefan,


On 03/08/2018 04:00 AM, Stefan Wahren wrote:
> Hi Matheus,
> 
>> Matheus Castello <matheus@castello.eng.br> hat am 8. März 2018 um 01:12 geschrieben:
>>
>>
>> Hi Stefan,
>>
>> On 03/07/2018 07:58 AM, Stefan Wahren wrote:
>>> Hi Matheus,
>>>
>>>> Matheus Castello <matheus@castello.eng.br> hat am 5. März 2018 um 03:29 geschrieben:
>>>>
>>>>
>>>> Hi Linus and Stefan,
>>>>
>>>> thanks for the tips.
>>>>
>>>> This series adds support for generic binding for pinctrl bcm2835 driver,
>>>> and add the code for set output buffer of a pin using the output-low and
>>>> output-high generic properties.
>>>>
>>>> Tested on Raspberry Pi Zero W, based on bcm2835 SoC.
>>>>
>>>> Matheus Castello (3):
>>>
>>> looks like you missed the changes to the dt binding. Please also add Rob Herring and Mark Rutland to CC. We need a ACK from them.
>>
>> are you talking about the .dts files? I add an overlay file for my tests. 
>> I was thinking it would be better to start with this changes in the driver and then in another series to convert to the generic style in the .dts.
>> If you find it better already add the conversion also in this series let me know.
>>
>> I understood that I should put the device tree maintainers in thread only if I were to introduce a new property. As I am using generic properties and not introducing new ones I did not add them.
>> Let me know if it is still necessary I stayed in this doubt.
> 
> no this is a misunderstanding. You are changing the DT interface because the driver supports new properties (from driver point of view). The DT users usually look at the binding document [1] not the source code. So you need to extend the binding document before changing the source code within a patch series. At the end we need a DT maintainer's ACK for the binding change. Using the generic properties doesn't allow us to skip this, but it increases the chance of an ACK.
> 
> Btw it's okay to keep the DTS files.
> 
> Stefan
> 
> [1] - https://elixir.bootlin.com/linux/v4.16-rc4/source/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
> 

Ok, I got it, thank you, I will work on the patch v3.

Best Regards
Matheus Castello

>>
>> Best Regards
>> Matheus Castello
>>>
>>>>   pinctrl: bcm2835: switch to GENERIC_PINCONF
>>>>   pinctrl: bcm2835: Add support for generic pinctrl binding
>>>>   pinctrl: bcm2835: Add support for output-low output-high properties
>>>>
>>>>  drivers/pinctrl/bcm/Kconfig           |  1 +
>>>>  drivers/pinctrl/bcm/pinctrl-bcm2835.c | 88 +++++++++++++++++++++++------------
>>>>  2 files changed, 60 insertions(+), 29 deletions(-)
>>>>
>>>> --
>>>> 2.7.4
>>>>

Re: [PATCH v3 0/3] pinctrl: bcm2835: Add brcm,level property

[Matheus Castello]

This series adds support for generic binding for pinctrl bcm2835 driver,
and add the code for set output buffer of a pin using the output-low and
output-high generic properties.

Tested on Raspberry Pi Zero W, based on bcm2835 SoC.

Changes since v2:
(Suggested by Eric Anholt)
- Remove PACK and UNPACK macros
- Use pinconf_to_config_* functions
(Suggested by Stefan Wahren)
- Fold Kconfig changes with the driver changes in a single patch
- Add devicetree bindings documentations about generic properties support
- Add devicetree bindings maintainers

Matheus Castello (3):
  pinctrl: bcm2835: Add support for generic pinctrl binding
  pinctrl: bcm2835: Add support for output-low output-high properties
  pinctrl: bcm2835: Update docs about generic pinctrl bindings support

 .../bindings/pinctrl/brcm,bcm2835-gpio.txt         | 19 +++++
 drivers/pinctrl/bcm/Kconfig                        |  1 +
 drivers/pinctrl/bcm/pinctrl-bcm2835.c              | 92 ++++++++++++++--------
 3 files changed, 79 insertions(+), 33 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Matheus Castello]

To keep driver up to date we add generic pinctrl binding support, which covers
the features used in this driver and has additional node properties that this
SoC has compatibility, so enabling future implementations of these properties
without the need to create new node properties in the device trees.

The logic of this change maintain the old brcm legacy binding support in order
to keep the ABI stable.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---

A brief explanation of what I did:

Add pinconf-generic header for use the defines and pinctrl-generic API.

Add dt-bindings pinctrl bcm2835 header to use functions selections and
pulls definitions, which functions definitions where duplicated in the
enum bcm2835_fsel, I removed the duplicate defines from enum.

In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for
pack the legacy param and arguments, since it will be unpacked along with
generic properties that is packed with this same macro.

In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use
pinctrl-generic parse code instead of parsing it inside the driver, so code
first check for generic binding parse, if something is parsed then it is
assumed that are using the new generic style, and when nothing is found then
parse continues to search for legacy properties.

In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and
was added a switch for the parameter tests, since pinctrl generic uses 3
properties to define the states of the pull instead of one with arguments, that
was the reason too that bcm2835_pull_config_set function was added, for reuse
the code that set state of pull.

 drivers/pinctrl/bcm/Kconfig           |  1 +
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 87 ++++++++++++++++++++++-------------
 2 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig
index e8c4e4f..0f38d51 100644
--- a/drivers/pinctrl/bcm/Kconfig
+++ b/drivers/pinctrl/bcm/Kconfig
@@ -20,6 +20,7 @@ config PINCTRL_BCM2835
 	bool
 	select PINMUX
 	select PINCONF
+	select GENERIC_PINCONF
 	select GPIOLIB_IRQCHIP
 
 config PINCTRL_IPROC_GPIO
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 785c366..010c565 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -36,11 +36,13 @@
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
+#include <dt-bindings/pinctrl/bcm2835.h>
 
 #define MODULE_NAME "pinctrl-bcm2835"
 #define BCM2835_NUM_GPIOS 54
@@ -75,10 +77,6 @@ enum bcm2835_pinconf_param {
 	BCM2835_PINCONF_PARAM_PULL,
 };
 
-#define BCM2835_PINCONF_PACK(_param_, _arg_) ((_param_) << 16 | (_arg_))
-#define BCM2835_PINCONF_UNPACK_PARAM(_conf_) ((_conf_) >> 16)
-#define BCM2835_PINCONF_UNPACK_ARG(_conf_) ((_conf_) & 0xffff)
-
 struct bcm2835_pinctrl {
 	struct device *dev;
 	void __iomem *base;
@@ -213,14 +211,6 @@ static const char * const bcm2835_gpio_groups[] = {
 };
 
 enum bcm2835_fsel {
-	BCM2835_FSEL_GPIO_IN = 0,
-	BCM2835_FSEL_GPIO_OUT = 1,
-	BCM2835_FSEL_ALT0 = 4,
-	BCM2835_FSEL_ALT1 = 5,
-	BCM2835_FSEL_ALT2 = 6,
-	BCM2835_FSEL_ALT3 = 7,
-	BCM2835_FSEL_ALT4 = 3,
-	BCM2835_FSEL_ALT5 = 2,
 	BCM2835_FSEL_COUNT = 8,
 	BCM2835_FSEL_MASK = 0x7,
 };
@@ -714,7 +704,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
 	configs = kzalloc(sizeof(*configs), GFP_KERNEL);
 	if (!configs)
 		return -ENOMEM;
-	configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_PULL, pull);
+	configs[0] = pinconf_to_config_packed(BCM2835_PINCONF_PARAM_PULL, pull);
 
 	map->type = PIN_MAP_TYPE_CONFIGS_PIN;
 	map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
@@ -736,6 +726,12 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 	int i, err;
 	u32 pin, func, pull;
 
+	/* Check for generic binding in this node */
+	err = pinconf_generic_dt_node_to_map_all(pctldev, np, map, num_maps);
+	if (err || *num_maps)
+		return err;
+
+	/* Generic binding did not find anything continue with legacy parse */
 	pins = of_find_property(np, "brcm,pins", NULL);
 	if (!pins) {
 		dev_err(pc->dev, "%pOF: missing brcm,pins property\n", np);
@@ -917,37 +913,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev,
 	return -ENOTSUPP;
 }
 
+static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc,
+		unsigned pin, unsigned arg)
+{
+	u32 off, bit;
+
+	off = GPIO_REG_OFFSET(pin);
+	bit = GPIO_REG_SHIFT(pin);
+
+	bcm2835_gpio_wr(pc, GPPUD, arg & 3);
+	/*
+	* BCM2835 datasheet say to wait 150 cycles, but not of what.
+	* But the VideoCore firmware delay for this operation
+	* based nearly on the same amount of VPU cycles and this clock
+	* runs at 250 MHz.
+	*/
+	udelay(1);
+	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
+	udelay(1);
+	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+}
+
 static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 			unsigned pin, unsigned long *configs,
 			unsigned num_configs)
 {
 	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
-	enum bcm2835_pinconf_param param;
-	u16 arg;
-	u32 off, bit;
+	u32 param, arg;
 	int i;
 
 	for (i = 0; i < num_configs; i++) {
-		param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
-		arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
 
-		if (param != BCM2835_PINCONF_PARAM_PULL)
-			return -EINVAL;
+		switch (param) {
+		/* Set legacy brcm,pull */
+		case BCM2835_PINCONF_PARAM_PULL:
+			bcm2835_pull_config_set(pc, pin, arg);
+			break;
 
-		off = GPIO_REG_OFFSET(pin);
-		bit = GPIO_REG_SHIFT(pin);
+		/* Set pull generic bindings */
+		case PIN_CONFIG_BIAS_DISABLE:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_OFF);
+			break;
 
-		bcm2835_gpio_wr(pc, GPPUD, arg & 3);
-		/*
-		 * BCM2835 datasheet say to wait 150 cycles, but not of what.
-		 * But the VideoCore firmware delay for this operation
-		 * based nearly on the same amount of VPU cycles and this clock
-		 * runs at 250 MHz.
-		 */
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_DOWN);
+			break;
+
+		case PIN_CONFIG_BIAS_PULL_UP:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
+			break;
+
+		default:
+			return -EINVAL;
+
+		} /* switch param type */
 	} /* for each config */
 
 	return 0;
-- 
2.7.4

[PATCH v3 2/3] pinctrl: bcm2835: Add support for output-low output-high properties

[Matheus Castello]

Properties to set initial value of pin output buffer.
This can be useful for configure hardware in overlay files, and in early boot
for checking it states in QA sanity tests.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 010c565..28acd06 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -965,6 +965,11 @@ static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
 			break;
 
+		/* Set output-high or output-low */
+		case PIN_CONFIG_OUTPUT:
+			bcm2835_gpio_set_bit(pc, arg ? GPSET0 : GPCLR0, pin);
+			break;
+
 		default:
 			return -EINVAL;
 
-- 
2.7.4

[PATCH v3 3/3] pinctrl: bcm2835: Update docs about generic pinctrl bindings support

[Matheus Castello]

Now we have generic pin configuration and multiplexing support,
ahd shoud be preferred than brcm legacy one.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 .../devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
index 2569866..58b4720 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
@@ -36,6 +36,25 @@ listed. In other words, a subnode that lists only a mux function implies no
 information about any pull configuration. Similarly, a subnode that lists only
 a pul parameter implies no information about the mux function.
 
+This driver supports the generic pin multiplexing and configuration
+bindings. For details on each properties, you can refer to
+./pinctrl-bindings.txt.
+
+Required sub-node properties:
+  - pins
+  - function
+
+Optional sub-node properties:
+  - bias-disable
+  - bias-pull-up
+  - bias-pull-down
+  - output-high
+  - output-low
+
+Legacy pin configuration and multiplexing binding:
+*** (Its use is deprecated, use generic multiplexing and configuration
+bindings instead)
+
 Required subnode-properties:
 - brcm,pins: An array of cells. Each cell contains the ID of a pin. Valid IDs
   are the integer GPIO IDs; 0==GPIO0, 1==GPIO1, ... 53==GPIO53.
-- 
2.7.4

Re: [PATCH v3 3/3] pinctrl: bcm2835: Update docs about generic pinctrl bindings support

[Stefan Wahren]

Hi Matheus,

> Matheus Castello <matheus@castello.eng.br> hat am 9. März 2018 um 05:16 geschrieben:
> 
> 
> Now we have generic pin configuration and multiplexing support,
> ahd shoud be preferred than brcm legacy one.

i suspect this patch won't get noticed by the DT maintainer because of the wrong subject.

Please try something like this:

dt-bindings: pinctrl: bcm2835-gpio: Add generic pinctrl support

Even this sounds strange to you, but this patch must be the first patch of series (1/3).

Stefan

> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  .../devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
> index 2569866..58b4720 100644
> --- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
> @@ -36,6 +36,25 @@ listed. In other words, a subnode that lists only a mux function implies no
>  information about any pull configuration. Similarly, a subnode that lists only
>  a pul parameter implies no information about the mux function.
>  
> +This driver supports the generic pin multiplexing and configuration
> +bindings. For details on each properties, you can refer to
> +./pinctrl-bindings.txt.
> +
> +Required sub-node properties:
> +  - pins
> +  - function
> +
> +Optional sub-node properties:
> +  - bias-disable
> +  - bias-pull-up
> +  - bias-pull-down
> +  - output-high
> +  - output-low
> +
> +Legacy pin configuration and multiplexing binding:
> +*** (Its use is deprecated, use generic multiplexing and configuration
> +bindings instead)
> +
>  Required subnode-properties:
>  - brcm,pins: An array of cells. Each cell contains the ID of a pin. Valid IDs
>    are the integer GPIO IDs; 0==GPIO0, 1==GPIO1, ... 53==GPIO53.
> -- 
> 2.7.4
>

Re: [PATCH v4 0/3] pinctrl: bcm2835: Add generic pinctrl support

[Matheus Castello]

This series adds support for generic binding for pinctrl bcm2835 driver,
and add the code for set output buffer of a pin using the output-low and
output-high generic properties.

Tested on Raspberry Pi Zero W, based on bcm2835 SoC.

Changes since v3:
(Suggested by Stefan Wahren)
- Change dt-bindings docs patch order and subject

Changes since v2:
(Suggested by Eric Anholt)
- Remove PACK and UNPACK macros
- Use pinconf_to_config_* functions
(Suggested by Stefan Wahren)
- Fold Kconfig changes with the driver changes in a single patch
- Add devicetree bindings documentations about generic properties support
- Add devicetree bindings maintainers

Matheus Castello (3):
  dt-bindings: pinctrl: bcm2835-gpio: Add generic pinctrl support
  pinctrl: bcm2835: Add support for generic pinctrl binding
  pinctrl: bcm2835: Add support for output-low output-high properties

 drivers/pinctrl/bcm/Kconfig                        |  1 +
 drivers/pinctrl/bcm/pinctrl-bcm2835.c              | 92 ++++++++++++++--------
 .../bindings/pinctrl/brcm,bcm2835-gpio.txt         | 19 +++++
 3 files changed, 79 insertions(+), 33 deletions(-)

-- 
2.7.4

[PATCH v4 1/3] dt-bindings: pinctrl: bcm2835-gpio: Add generic pinctrl support

[Matheus Castello]

Added generic pin configuration and multiplexing support,
and shoud be preferred than brcm legacy one.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 .../devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
index 2569866..58b4720 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
@@ -36,6 +36,25 @@ listed. In other words, a subnode that lists only a mux function implies no
 information about any pull configuration. Similarly, a subnode that lists only
 a pul parameter implies no information about the mux function.
 
+This driver supports the generic pin multiplexing and configuration
+bindings. For details on each properties, you can refer to
+./pinctrl-bindings.txt.
+
+Required sub-node properties:
+  - pins
+  - function
+
+Optional sub-node properties:
+  - bias-disable
+  - bias-pull-up
+  - bias-pull-down
+  - output-high
+  - output-low
+
+Legacy pin configuration and multiplexing binding:
+*** (Its use is deprecated, use generic multiplexing and configuration
+bindings instead)
+
 Required subnode-properties:
 - brcm,pins: An array of cells. Each cell contains the ID of a pin. Valid IDs
   are the integer GPIO IDs; 0==GPIO0, 1==GPIO1, ... 53==GPIO53.
-- 
2.7.4

Re: [PATCH v4 1/3] dt-bindings: pinctrl: bcm2835-gpio: Add generic pinctrl support

[Rob Herring]

On Fri, Mar 09, 2018 at 01:13:58PM -0400, Matheus Castello wrote:
> Added generic pin configuration and multiplexing support,
> and shoud be preferred than brcm legacy one.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  .../devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
> index 2569866..58b4720 100644
> --- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
> @@ -36,6 +36,25 @@ listed. In other words, a subnode that lists only a mux function implies no
>  information about any pull configuration. Similarly, a subnode that lists only
>  a pul parameter implies no information about the mux function.
>  
> +This driver supports the generic pin multiplexing and configuration

Bindings describe h/w, not drivers.

> +bindings. For details on each properties, you can refer to
> +./pinctrl-bindings.txt.
> +
> +Required sub-node properties:
> +  - pins
> +  - function
> +
> +Optional sub-node properties:
> +  - bias-disable
> +  - bias-pull-up
> +  - bias-pull-down
> +  - output-high
> +  - output-low
> +
> +Legacy pin configuration and multiplexing binding:
> +*** (Its use is deprecated, use generic multiplexing and configuration
> +bindings instead)
> +
>  Required subnode-properties:
>  - brcm,pins: An array of cells. Each cell contains the ID of a pin. Valid IDs
>    are the integer GPIO IDs; 0==GPIO0, 1==GPIO1, ... 53==GPIO53.
> -- 
> 2.7.4
> 

Re: [PATCH v4 1/3] dt-bindings: pinctrl: bcm2835-gpio: Add generic pinctrl support

[Matheus Castello]

Hi Rob,

sorry I used allwinner,sunxi-pinctrl.txt from Maxime Ripard as a base, I 
used the same words, I thought it would be correct.

I will modify this to:

The BCM2835 pin configuration and multiplexing supports the generic 
bindings. For details on each properties, you can refer to
./pinctrl-bindings.txt.

If it's okay for you let me know, so I can send the v5 patch.

Best Regards
Matheus Castello


On 03/18/2018 08:48 AM, Rob Herring wrote:
> On Fri, Mar 09, 2018 at 01:13:58PM -0400, Matheus Castello wrote:
>> Added generic pin configuration and multiplexing support,
>> and shoud be preferred than brcm legacy one.
>>
>> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
>> ---
>>   .../devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
>> index 2569866..58b4720 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
>> @@ -36,6 +36,25 @@ listed. In other words, a subnode that lists only a mux function implies no
>>   information about any pull configuration. Similarly, a subnode that lists only
>>   a pul parameter implies no information about the mux function.
>>   
>> +This driver supports the generic pin multiplexing and configuration
> 
> Bindings describe h/w, not drivers.
> 
>> +bindings. For details on each properties, you can refer to
>> +./pinctrl-bindings.txt.
>> +
>> +Required sub-node properties:
>> +  - pins
>> +  - function
>> +
>> +Optional sub-node properties:
>> +  - bias-disable
>> +  - bias-pull-up
>> +  - bias-pull-down
>> +  - output-high
>> +  - output-low
>> +
>> +Legacy pin configuration and multiplexing binding:
>> +*** (Its use is deprecated, use generic multiplexing and configuration
>> +bindings instead)
>> +
>>   Required subnode-properties:
>>   - brcm,pins: An array of cells. Each cell contains the ID of a pin. Valid IDs
>>     are the integer GPIO IDs; 0==GPIO0, 1==GPIO1, ... 53==GPIO53.
>> -- 
>> 2.7.4
>>

Re: [PATCH v5 0/3] pinctrl: bcm2835: Add generic pinctrl support

[Matheus Castello]

This series adds support for generic binding for pinctrl bcm2835 driver,
and add the code for set output buffer of a pin using the output-low and
output-high generic properties.

Tested on Raspberry Pi Zero W, based on bcm2835 SoC.

Changes since v4:
(Suggested by Rob Herring)
- Change dt-bindings docs driver reference to hardware in case the BCM2835 pin
configuration and multiplexing

Changes since v3:
(Suggested by Stefan Wahren)
- Change dt-bindings docs patch order and subject

Changes since v2:
(Suggested by Eric Anholt)
- Remove PACK and UNPACK macros
- Use pinconf_to_config_* functions
(Suggested by Stefan Wahren)
- Fold Kconfig changes with the driver changes in a single patch
- Add devicetree bindings documentations about generic properties support
- Add devicetree bindings maintainers

Matheus Castello (3):
  dt-bindings: pinctrl: bcm2835-gpio: Add generic pinctrl support
  pinctrl: bcm2835: Add support for generic pinctrl binding
  pinctrl: bcm2835: Add support for output-low output-high properties

 .../bindings/pinctrl/brcm,bcm2835-gpio.txt         | 18 +++++
 drivers/pinctrl/bcm/Kconfig                        |  1 +
 drivers/pinctrl/bcm/pinctrl-bcm2835.c              | 92 ++++++++++++++--------
 3 files changed, 78 insertions(+), 33 deletions(-)

--
2.7.4

[PATCH v5 1/3] dt-bindings: pinctrl: bcm2835-gpio: Add generic pinctrl support

[Matheus Castello]

Added generic pin configuration and multiplexing support,
and should be preferred than brcm legacy one.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 .../devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt  | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
index 2569866..51fe305 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
@@ -36,6 +36,24 @@ listed. In other words, a subnode that lists only a mux function implies no
 information about any pull configuration. Similarly, a subnode that lists only
 a pul parameter implies no information about the mux function.

+The BCM2835 pin configuration and multiplexing supports the generic bindings.
+For details on each properties, you can refer to ./pinctrl-bindings.txt.
+
+Required sub-node properties:
+  - pins
+  - function
+
+Optional sub-node properties:
+  - bias-disable
+  - bias-pull-up
+  - bias-pull-down
+  - output-high
+  - output-low
+
+Legacy pin configuration and multiplexing binding:
+*** (Its use is deprecated, use generic multiplexing and configuration
+bindings instead)
+
 Required subnode-properties:
 - brcm,pins: An array of cells. Each cell contains the ID of a pin. Valid IDs
   are the integer GPIO IDs; 0==GPIO0, 1==GPIO1, ... 53==GPIO53.
--
2.7.4

Re: [PATCH v5 1/3] dt-bindings: pinctrl: bcm2835-gpio: Add generic pinctrl support

[Eric Anholt]

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

Matheus Castello <matheus@castello.eng.br> writes:

> Added generic pin configuration and multiplexing support,
> and should be preferred than brcm legacy one.
>
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  .../devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt  | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

All 3 patches are:

Reviewed-by: Eric Anholt <eric@anholt.net>

Thanks for doing this!

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

Re: [PATCH v5 1/3] dt-bindings: pinctrl: bcm2835-gpio: Add generic pinctrl support

[Rob Herring]

On Wed, Apr 11, 2018 at 12:58:44AM -0400, Matheus Castello wrote:
> Added generic pin configuration and multiplexing support,
> and should be preferred than brcm legacy one.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  .../devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt  | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 1/3] dt-bindings: pinctrl: bcm2835-gpio: Add generic pinctrl support

[Linus Walleij]

On Wed, Apr 11, 2018 at 6:58 AM, Matheus Castello
<matheus@castello.eng.br> wrote:

> Added generic pin configuration and multiplexing support,
> and should be preferred than brcm legacy one.
>
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>

Patch applied with Eric's and Rob's ACKs.

Yours,
Linus Walleij

[PATCH v5 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Matheus Castello]

To keep driver up to date we add generic pinctrl binding support, which covers
the features used in this driver and has additional node properties that this
SoC has compatibility, so enabling future implementations of these properties
without the need to create new node properties in the device trees.

The logic of this change maintain the old brcm legacy binding support in order
to keep the ABI stable.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---

A brief explanation of what I did:

Add pinconf-generic header for use the defines and pinctrl-generic API.

Add dt-bindings pinctrl bcm2835 header to use functions selections and
pulls definitions, which functions definitions where duplicated in the
enum bcm2835_fsel, I removed the duplicate defines from enum.

In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for
pack the legacy param and arguments, since it will be unpacked along with
generic properties that is packed with this same macro.

In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use
pinctrl-generic parse code instead of parsing it inside the driver, so code
first check for generic binding parse, if something is parsed then it is
assumed that are using the new generic style, and when nothing is found then
parse continues to search for legacy properties.

In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and
was added a switch for the parameter tests, since pinctrl generic uses 3
properties to define the states of the pull instead of one with arguments, that
was the reason too that bcm2835_pull_config_set function was added, for reuse
the code that set state of pull.

 drivers/pinctrl/bcm/Kconfig           |  1 +
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 87 ++++++++++++++++++++++-------------
 2 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig
index e8c4e4f..0f38d51 100644
--- a/drivers/pinctrl/bcm/Kconfig
+++ b/drivers/pinctrl/bcm/Kconfig
@@ -20,6 +20,7 @@ config PINCTRL_BCM2835
 	bool
 	select PINMUX
 	select PINCONF
+	select GENERIC_PINCONF
 	select GPIOLIB_IRQCHIP

 config PINCTRL_IPROC_GPIO
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 785c366..010c565 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -36,11 +36,13 @@
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
+#include <dt-bindings/pinctrl/bcm2835.h>

 #define MODULE_NAME "pinctrl-bcm2835"
 #define BCM2835_NUM_GPIOS 54
@@ -75,10 +77,6 @@ enum bcm2835_pinconf_param {
 	BCM2835_PINCONF_PARAM_PULL,
 };

-#define BCM2835_PINCONF_PACK(_param_, _arg_) ((_param_) << 16 | (_arg_))
-#define BCM2835_PINCONF_UNPACK_PARAM(_conf_) ((_conf_) >> 16)
-#define BCM2835_PINCONF_UNPACK_ARG(_conf_) ((_conf_) & 0xffff)
-
 struct bcm2835_pinctrl {
 	struct device *dev;
 	void __iomem *base;
@@ -213,14 +211,6 @@ static const char * const bcm2835_gpio_groups[] = {
 };

 enum bcm2835_fsel {
-	BCM2835_FSEL_GPIO_IN = 0,
-	BCM2835_FSEL_GPIO_OUT = 1,
-	BCM2835_FSEL_ALT0 = 4,
-	BCM2835_FSEL_ALT1 = 5,
-	BCM2835_FSEL_ALT2 = 6,
-	BCM2835_FSEL_ALT3 = 7,
-	BCM2835_FSEL_ALT4 = 3,
-	BCM2835_FSEL_ALT5 = 2,
 	BCM2835_FSEL_COUNT = 8,
 	BCM2835_FSEL_MASK = 0x7,
 };
@@ -714,7 +704,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
 	configs = kzalloc(sizeof(*configs), GFP_KERNEL);
 	if (!configs)
 		return -ENOMEM;
-	configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_PULL, pull);
+	configs[0] = pinconf_to_config_packed(BCM2835_PINCONF_PARAM_PULL, pull);

 	map->type = PIN_MAP_TYPE_CONFIGS_PIN;
 	map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
@@ -736,6 +726,12 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 	int i, err;
 	u32 pin, func, pull;

+	/* Check for generic binding in this node */
+	err = pinconf_generic_dt_node_to_map_all(pctldev, np, map, num_maps);
+	if (err || *num_maps)
+		return err;
+
+	/* Generic binding did not find anything continue with legacy parse */
 	pins = of_find_property(np, "brcm,pins", NULL);
 	if (!pins) {
 		dev_err(pc->dev, "%pOF: missing brcm,pins property\n", np);
@@ -917,37 +913,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev,
 	return -ENOTSUPP;
 }

+static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc,
+		unsigned pin, unsigned arg)
+{
+	u32 off, bit;
+
+	off = GPIO_REG_OFFSET(pin);
+	bit = GPIO_REG_SHIFT(pin);
+
+	bcm2835_gpio_wr(pc, GPPUD, arg & 3);
+	/*
+	* BCM2835 datasheet say to wait 150 cycles, but not of what.
+	* But the VideoCore firmware delay for this operation
+	* based nearly on the same amount of VPU cycles and this clock
+	* runs at 250 MHz.
+	*/
+	udelay(1);
+	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
+	udelay(1);
+	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+}
+
 static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 			unsigned pin, unsigned long *configs,
 			unsigned num_configs)
 {
 	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
-	enum bcm2835_pinconf_param param;
-	u16 arg;
-	u32 off, bit;
+	u32 param, arg;
 	int i;

 	for (i = 0; i < num_configs; i++) {
-		param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
-		arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);

-		if (param != BCM2835_PINCONF_PARAM_PULL)
-			return -EINVAL;
+		switch (param) {
+		/* Set legacy brcm,pull */
+		case BCM2835_PINCONF_PARAM_PULL:
+			bcm2835_pull_config_set(pc, pin, arg);
+			break;

-		off = GPIO_REG_OFFSET(pin);
-		bit = GPIO_REG_SHIFT(pin);
+		/* Set pull generic bindings */
+		case PIN_CONFIG_BIAS_DISABLE:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_OFF);
+			break;

-		bcm2835_gpio_wr(pc, GPPUD, arg & 3);
-		/*
-		 * BCM2835 datasheet say to wait 150 cycles, but not of what.
-		 * But the VideoCore firmware delay for this operation
-		 * based nearly on the same amount of VPU cycles and this clock
-		 * runs at 250 MHz.
-		 */
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_DOWN);
+			break;
+
+		case PIN_CONFIG_BIAS_PULL_UP:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
+			break;
+
+		default:
+			return -EINVAL;
+
+		} /* switch param type */
 	} /* for each config */

 	return 0;
--
2.7.4

Re: [PATCH v5 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Stefan Wahren]

Hi Matheus,

> Matheus Castello <matheus@castello.eng.br> hat am 11. April 2018 um 06:58 geschrieben:
> 
> 
> To keep driver up to date we add generic pinctrl binding support, which covers
> the features used in this driver and has additional node properties that this
> SoC has compatibility, so enabling future implementations of these properties
> without the need to create new node properties in the device trees.
> 
> The logic of this change maintain the old brcm legacy binding support in order
> to keep the ABI stable.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
> 
> A brief explanation of what I did:
> 
> Add pinconf-generic header for use the defines and pinctrl-generic API.
> 
> Add dt-bindings pinctrl bcm2835 header to use functions selections and
> pulls definitions, which functions definitions where duplicated in the
> enum bcm2835_fsel, I removed the duplicate defines from enum.
> 
> In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for
> pack the legacy param and arguments, since it will be unpacked along with
> generic properties that is packed with this same macro.
> 
> In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use
> pinctrl-generic parse code instead of parsing it inside the driver, so code
> first check for generic binding parse, if something is parsed then it is
> assumed that are using the new generic style, and when nothing is found then
> parse continues to search for legacy properties.
> 
> In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and
> was added a switch for the parameter tests, since pinctrl generic uses 3
> properties to define the states of the pull instead of one with arguments, that
> was the reason too that bcm2835_pull_config_set function was added, for reuse
> the code that set state of pull.
> 
>  drivers/pinctrl/bcm/Kconfig           |  1 +
>  drivers/pinctrl/bcm/pinctrl-bcm2835.c | 87 ++++++++++++++++++++++-------------
>  2 files changed, 55 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig
> index e8c4e4f..0f38d51 100644
> --- a/drivers/pinctrl/bcm/Kconfig
> +++ b/drivers/pinctrl/bcm/Kconfig
> @@ -20,6 +20,7 @@ config PINCTRL_BCM2835
>  	bool
>  	select PINMUX
>  	select PINCONF
> +	select GENERIC_PINCONF
>  	select GPIOLIB_IRQCHIP
> 
>  config PINCTRL_IPROC_GPIO
> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> index 785c366..010c565 100644
> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> @@ -36,11 +36,13 @@
>  #include <linux/pinctrl/pinconf.h>
>  #include <linux/pinctrl/pinctrl.h>
>  #include <linux/pinctrl/pinmux.h>
> +#include <linux/pinctrl/pinconf-generic.h>
>  #include <linux/platform_device.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/types.h>
> +#include <dt-bindings/pinctrl/bcm2835.h>
> 
>  #define MODULE_NAME "pinctrl-bcm2835"
>  #define BCM2835_NUM_GPIOS 54
> @@ -75,10 +77,6 @@ enum bcm2835_pinconf_param {
>  	BCM2835_PINCONF_PARAM_PULL,

Since we use bcm2835_pinconf_param and pin_config_param in bcm2835_pinconf_set, there are potential conflicts. According to include/linux/pinctrl/pinconf-generic.h:

 * @PIN_CONFIG_END: this is the last enumerator for pin configurations, if
 *	you need to pass in custom configurations to the pin controller, use
 *	PIN_CONFIG_END+1 as the base offset.

Please adjust BCM2835_PINCONF_PARAM_PULL accordingly.

Sorry for not notice this before.

Stefan

Re: [PATCH v5 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Stefan Wahren]

Hi Matheus,

> Matheus Castello <matheus@castello.eng.br> hat am 11. April 2018 um 06:58 geschrieben:
> 
> 
> To keep driver up to date we add generic pinctrl binding support, which covers
> the features used in this driver and has additional node properties that this
> SoC has compatibility, so enabling future implementations of these properties
> without the need to create new node properties in the device trees.
> 
> The logic of this change maintain the old brcm legacy binding support in order
> to keep the ABI stable.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
> 
> A brief explanation of what I did:
> 
> Add pinconf-generic header for use the defines and pinctrl-generic API.
> 
> Add dt-bindings pinctrl bcm2835 header to use functions selections and
> pulls definitions, which functions definitions where duplicated in the
> enum bcm2835_fsel, I removed the duplicate defines from enum.
> 
> In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for
> pack the legacy param and arguments, since it will be unpacked along with
> generic properties that is packed with this same macro.
> 
> In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use
> pinctrl-generic parse code instead of parsing it inside the driver, so code
> first check for generic binding parse, if something is parsed then it is
> assumed that are using the new generic style, and when nothing is found then
> parse continues to search for legacy properties.
> 
> In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and
> was added a switch for the parameter tests, since pinctrl generic uses 3
> properties to define the states of the pull instead of one with arguments, that
> was the reason too that bcm2835_pull_config_set function was added, for reuse
> the code that set state of pull.
> 
>  drivers/pinctrl/bcm/Kconfig           |  1 +
>  drivers/pinctrl/bcm/pinctrl-bcm2835.c | 87 ++++++++++++++++++++++-------------
>  2 files changed, 55 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig
> index e8c4e4f..0f38d51 100644
> --- a/drivers/pinctrl/bcm/Kconfig
> +++ b/drivers/pinctrl/bcm/Kconfig
> @@ -20,6 +20,7 @@ config PINCTRL_BCM2835
>  	bool
>  	select PINMUX
>  	select PINCONF
> +	select GENERIC_PINCONF
>  	select GPIOLIB_IRQCHIP
> 
>  config PINCTRL_IPROC_GPIO
> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> index 785c366..010c565 100644
> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> ...
> @@ -917,37 +913,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev,
>  	return -ENOTSUPP;
>  }
> 
> +static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc,
> +		unsigned pin, unsigned arg)

checkpatch is complaining about missing "int" here. Please fix this here.

> +{
> +	u32 off, bit;
> +
> +	off = GPIO_REG_OFFSET(pin);
> +	bit = GPIO_REG_SHIFT(pin);
> +
> +	bcm2835_gpio_wr(pc, GPPUD, arg & 3);
> +	/*
> +	* BCM2835 datasheet say to wait 150 cycles, but not of what.
> +	* But the VideoCore firmware delay for this operation
> +	* based nearly on the same amount of VPU cycles and this clock
> +	* runs at 250 MHz.
> +	*/

checkpatch complains here about comment alignment. Please fix it.

Stefan

> +	udelay(1);
> +	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
> +	udelay(1);
> +	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
> +}
> +
>  static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
>  			unsigned pin, unsigned long *configs,
>  			unsigned num_configs)
>  {
>  	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> -	enum bcm2835_pinconf_param param;
> -	u16 arg;
> -	u32 off, bit;
> +	u32 param, arg;
>  	int i;
> 
>  	for (i = 0; i < num_configs; i++) {
> -		param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
> -		arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
> +		param = pinconf_to_config_param(configs[i]);
> +		arg = pinconf_to_config_argument(configs[i]);
> 
> -		if (param != BCM2835_PINCONF_PARAM_PULL)
> -			return -EINVAL;
> +		switch (param) {
> +		/* Set legacy brcm,pull */
> +		case BCM2835_PINCONF_PARAM_PULL:
> +			bcm2835_pull_config_set(pc, pin, arg);
> +			break;
> 
> -		off = GPIO_REG_OFFSET(pin);
> -		bit = GPIO_REG_SHIFT(pin);
> +		/* Set pull generic bindings */
> +		case PIN_CONFIG_BIAS_DISABLE:
> +			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_OFF);
> +			break;
> 
> -		bcm2835_gpio_wr(pc, GPPUD, arg & 3);
> -		/*
> -		 * BCM2835 datasheet say to wait 150 cycles, but not of what.
> -		 * But the VideoCore firmware delay for this operation
> -		 * based nearly on the same amount of VPU cycles and this clock
> -		 * runs at 250 MHz.
> -		 */
> -		udelay(1);
> -		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
> -		udelay(1);
> -		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
> +		case PIN_CONFIG_BIAS_PULL_DOWN:
> +			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_DOWN);
> +			break;
> +
> +		case PIN_CONFIG_BIAS_PULL_UP:
> +			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
> +			break;
> +
> +		default:
> +			return -EINVAL;
> +
> +		} /* switch param type */
>  	} /* for each config */
> 
>  	return 0;
> --
> 2.7.4
>

Re: [PATCH v5 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Linus Walleij]

On Wed, Apr 11, 2018 at 6:58 AM, Matheus Castello
<matheus@castello.eng.br> wrote:

> To keep driver up to date we add generic pinctrl binding support, which covers
> the features used in this driver and has additional node properties that this
> SoC has compatibility, so enabling future implementations of these properties
> without the need to create new node properties in the device trees.
>
> The logic of this change maintain the old brcm legacy binding support in order
> to keep the ABI stable.
>
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>

Looking good, just need to address Stefan's comments.

(I already merged patch 1 so you do not need to resend this.)

The driver is kind of part of what Stefan and Eric maintains but
it would be nice to get a nod from Stephen Warren as well
as in my mind he is also maintainer for this. Please include him
on subsequent postings as well.

Yours,
Linus Walleij

[PATCH v5 3/3] pinctrl: bcm2835: Add support for output-low output-high properties

[Matheus Castello]

Properties to set initial value of pin output buffer.
This can be useful for configure hardware in overlay files, and in early boot
for checking it states in QA sanity tests.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 010c565..28acd06 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -965,6 +965,11 @@ static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
 			break;

+		/* Set output-high or output-low */
+		case PIN_CONFIG_OUTPUT:
+			bcm2835_gpio_set_bit(pc, arg ? GPSET0 : GPCLR0, pin);
+			break;
+
 		default:
 			return -EINVAL;

--
2.7.4

Re: [PATCH v5 3/3] pinctrl: bcm2835: Add support for output-low output-high properties

[Linus Walleij]

On Wed, Apr 11, 2018 at 6:58 AM, Matheus Castello
<matheus@castello.eng.br> wrote:

> Properties to set initial value of pin output buffer.
> This can be useful for configure hardware in overlay files, and in early boot
> for checking it states in QA sanity tests.
>
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>

Will apply this when I apply 2/3.

You only need to resend patch 2+3.

Yours,
Linus Walleij

Re: [PATCH v6 0/3] bcm2835: Add generic pinctrl support

[Matheus Castello]

This series adds support for generic binding for pinctrl bcm2835 driver,
and add the code for set output buffer of a pin using the output-low and
output-high generic properties.

Tested on Raspberry Pi Zero W, based on bcm2835 SoC.

Changes since v5:
(Suggested by Stefan Wahren)
- Fix checkpatch warnings
- Use PIN_CONFIG_END+1 for bcm2835_pinconf_param for our pull legacy
configuration
(Suggested by Linus Walleij)
- Only resend patch 2+3
- Add Stephen Warren for subsequent postings

Changes since v4:
(Suggested by Rob Herring)
- Change dt-bindings docs driver reference to hardware in case the BCM2835
pin configuration and multiplexing

Changes since v3:
(Suggested by Stefan Wahren)
- Change dt-bindings docs patch order and subject

Changes since v2:
(Suggested by Eric Anholt)
- Remove PACK and UNPACK macros
- Use pinconf_to_config_* functions
(Suggested by Stefan Wahren)
- Fold Kconfig changes with the driver changes in a single patch
- Add devicetree bindings documentations about generic properties support
- Add devicetree bindings maintainers

Matheus Castello (2):
  pinctrl: bcm2835: Add support for generic pinctrl binding
  pinctrl: bcm2835: Add support for output-low output-high properties

 drivers/pinctrl/bcm/Kconfig           |   1 +
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 100 +++++++++++++++++++++-------------
 2 files changed, 64 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH v6 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Matheus Castello]

To keep driver up to date we add generic pinctrl binding support, which
covers the features used in this driver and has additional node properties
that this SoC has compatibility, so enabling future implementations of
these properties without the need to create new node properties in the
device trees.

The logic of this change maintain the old brcm legacy binding support in
order to keep the ABI stable.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---

A brief explanation of what I did:

Add pinconf-generic header for use the defines and pinctrl-generic API.

Add dt-bindings pinctrl bcm2835 header to use functions selections and
pulls definitions, which functions definitions where duplicated in the
enum bcm2835_fsel, I removed the duplicate defines from enum.

Assign PIN_CONFIG_END+1 offset for bcm2835_pinconf_param pull legacy param to
avoid potential conflicts.

In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for
pack the legacy param and arguments, since it will be unpacked along with
generic properties that is packed with this same macro.

In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use
pinctrl-generic parse code instead of parsing it inside the driver, so code
first check for generic binding parse, if something is parsed then it is
assumed that are using the new generic style, and when nothing is found then
parse continues to search for legacy properties.

In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and
was added a switch for the parameter tests, since pinctrl generic uses 3
properties to define the states of the pull instead of one with arguments, that
was the reason too that bcm2835_pull_config_set function was added, for reuse
the code that set state of pull.

 drivers/pinctrl/bcm/Kconfig           |  1 +
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 95 +++++++++++++++++++++--------------
 2 files changed, 59 insertions(+), 37 deletions(-)

diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig
index e8c4e4f..0f38d51 100644
--- a/drivers/pinctrl/bcm/Kconfig
+++ b/drivers/pinctrl/bcm/Kconfig
@@ -20,6 +20,7 @@ config PINCTRL_BCM2835
 	bool
 	select PINMUX
 	select PINCONF
+	select GENERIC_PINCONF
 	select GPIOLIB_IRQCHIP
 
 config PINCTRL_IPROC_GPIO
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 785c366..0231107 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -36,11 +36,13 @@
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
+#include <dt-bindings/pinctrl/bcm2835.h>
 
 #define MODULE_NAME "pinctrl-bcm2835"
 #define BCM2835_NUM_GPIOS 54
@@ -72,13 +74,9 @@
 
 enum bcm2835_pinconf_param {
 	/* argument: bcm2835_pinconf_pull */
-	BCM2835_PINCONF_PARAM_PULL,
+	BCM2835_PINCONF_PARAM_PULL = 0x80,
 };
 
-#define BCM2835_PINCONF_PACK(_param_, _arg_) ((_param_) << 16 | (_arg_))
-#define BCM2835_PINCONF_UNPACK_PARAM(_conf_) ((_conf_) >> 16)
-#define BCM2835_PINCONF_UNPACK_ARG(_conf_) ((_conf_) & 0xffff)
-
 struct bcm2835_pinctrl {
 	struct device *dev;
 	void __iomem *base;
@@ -213,14 +211,6 @@ static const char * const bcm2835_gpio_groups[] = {
 };
 
 enum bcm2835_fsel {
-	BCM2835_FSEL_GPIO_IN = 0,
-	BCM2835_FSEL_GPIO_OUT = 1,
-	BCM2835_FSEL_ALT0 = 4,
-	BCM2835_FSEL_ALT1 = 5,
-	BCM2835_FSEL_ALT2 = 6,
-	BCM2835_FSEL_ALT3 = 7,
-	BCM2835_FSEL_ALT4 = 3,
-	BCM2835_FSEL_ALT5 = 2,
 	BCM2835_FSEL_COUNT = 8,
 	BCM2835_FSEL_MASK = 0x7,
 };
@@ -714,7 +704,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
 	configs = kzalloc(sizeof(*configs), GFP_KERNEL);
 	if (!configs)
 		return -ENOMEM;
-	configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_PULL, pull);
+	configs[0] = pinconf_to_config_packed(BCM2835_PINCONF_PARAM_PULL, pull);
 
 	map->type = PIN_MAP_TYPE_CONFIGS_PIN;
 	map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
@@ -727,7 +717,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
 
 static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		struct device_node *np,
-		struct pinctrl_map **map, unsigned *num_maps)
+		struct pinctrl_map **map, unsigned int *num_maps)
 {
 	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
 	struct property *pins, *funcs, *pulls;
@@ -736,6 +726,12 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 	int i, err;
 	u32 pin, func, pull;
 
+	/* Check for generic binding in this node */
+	err = pinconf_generic_dt_node_to_map_all(pctldev, np, map, num_maps);
+	if (err || *num_maps)
+		return err;
+
+	/* Generic binding did not find anything continue with legacy parse */
 	pins = of_find_property(np, "brcm,pins", NULL);
 	if (!pins) {
 		dev_err(pc->dev, "%pOF: missing brcm,pins property\n", np);
@@ -917,37 +913,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev,
 	return -ENOTSUPP;
 }
 
+static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc,
+		unsigned int pin, unsigned int arg)
+{
+	u32 off, bit;
+
+	off = GPIO_REG_OFFSET(pin);
+	bit = GPIO_REG_SHIFT(pin);
+
+	bcm2835_gpio_wr(pc, GPPUD, arg & 3);
+	/*
+	 * BCM2835 datasheet say to wait 150 cycles, but not of what.
+	 * But the VideoCore firmware delay for this operation
+	 * based nearly on the same amount of VPU cycles and this clock
+	 * runs at 250 MHz.
+	 */
+	udelay(1);
+	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
+	udelay(1);
+	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+}
+
 static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
-			unsigned pin, unsigned long *configs,
-			unsigned num_configs)
+			unsigned int pin, unsigned long *configs,
+			unsigned int num_configs)
 {
 	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
-	enum bcm2835_pinconf_param param;
-	u16 arg;
-	u32 off, bit;
+	u32 param, arg;
 	int i;
 
 	for (i = 0; i < num_configs; i++) {
-		param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
-		arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
 
-		if (param != BCM2835_PINCONF_PARAM_PULL)
-			return -EINVAL;
+		switch (param) {
+		/* Set legacy brcm,pull */
+		case BCM2835_PINCONF_PARAM_PULL:
+			bcm2835_pull_config_set(pc, pin, arg);
+			break;
 
-		off = GPIO_REG_OFFSET(pin);
-		bit = GPIO_REG_SHIFT(pin);
+		/* Set pull generic bindings */
+		case PIN_CONFIG_BIAS_DISABLE:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_OFF);
+			break;
 
-		bcm2835_gpio_wr(pc, GPPUD, arg & 3);
-		/*
-		 * BCM2835 datasheet say to wait 150 cycles, but not of what.
-		 * But the VideoCore firmware delay for this operation
-		 * based nearly on the same amount of VPU cycles and this clock
-		 * runs at 250 MHz.
-		 */
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_DOWN);
+			break;
+
+		case PIN_CONFIG_BIAS_PULL_UP:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
+			break;
+
+		default:
+			return -EINVAL;
+
+		} /* switch param type */
 	} /* for each config */
 
 	return 0;
-- 
2.7.4

[PATCH v6 3/3] pinctrl: bcm2835: Add support for output-low output-high properties

[Matheus Castello]

Properties to set initial value of pin output buffer.
This can be useful for configure hardware in overlay files, and in early
boot for checking it states in QA sanity tests.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 0231107..3bc0d04 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -965,6 +965,11 @@ static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
 			break;
 
+		/* Set output-high or output-low */
+		case PIN_CONFIG_OUTPUT:
+			bcm2835_gpio_set_bit(pc, arg ? GPSET0 : GPCLR0, pin);
+			break;
+
 		default:
 			return -EINVAL;
 
-- 
2.7.4

Re: [PATCH v7 0/3] bcm2835: Add generic pinctrl support

[Matheus Castello]

This series adds support for generic binding for pinctrl bcm2835 driver,
and add the code for set output buffer of a pin using the output-low and
output-high generic properties.

Tested on Raspberry Pi Zero W, based on bcm2835 SoC.

Changes since v6:
- I was dumb and calculate the PIN_CONFIG_END (x7f) +1 (x80) and hard coded it.
Fix it using recommended define from pinconf-generic.h PIN_CONFIG_END + 1
- In v6 I did not keep the review tags

Changes since v5:
(Suggested by Stefan Wahren)
- Fix checkpatch warnings
- Use PIN_CONFIG_END+1 for bcm2835_pinconf_param for our pull legacy
configuration
(Suggested by Linus Walleij)
- Only resend patch 2+3
- Add Stephen Warren for subsequent postings

Changes since v4:
(Suggested by Rob Herring)
- Change dt-bindings docs driver reference to hardware in case the BCM2835
pin configuration and multiplexing

Changes since v3:
(Suggested by Stefan Wahren)
- Change dt-bindings docs patch order and subject

Changes since v2:
(Suggested by Eric Anholt)
- Remove PACK and UNPACK macros
- Use pinconf_to_config_* functions
(Suggested by Stefan Wahren)
- Fold Kconfig changes with the driver changes in a single patch
- Add devicetree bindings documentations about generic properties support
- Add devicetree bindings maintainers

Matheus Castello (2):
  pinctrl: bcm2835: Add support for generic pinctrl binding
  pinctrl: bcm2835: Add support for output-low output-high properties

 drivers/pinctrl/bcm/Kconfig           |   1 +
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 100 +++++++++++++++++++++-------------
 2 files changed, 64 insertions(+), 37 deletions(-)

--
2.7.4

[PATCH v7 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Matheus Castello]

To keep driver up to date we add generic pinctrl binding support, which
covers the features used in this driver and has additional node properties
that this SoC has compatibility, so enabling future implementations of
these properties without the need to create new node properties in the
device trees.

The logic of this change maintain the old brcm legacy binding support in
order to keep the ABI stable.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
 drivers/pinctrl/bcm/Kconfig           |  1 +
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 95 +++++++++++++++++++++--------------
 2 files changed, 59 insertions(+), 37 deletions(-)

diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig
index e8c4e4f..0f38d51 100644
--- a/drivers/pinctrl/bcm/Kconfig
+++ b/drivers/pinctrl/bcm/Kconfig
@@ -20,6 +20,7 @@ config PINCTRL_BCM2835
 	bool
 	select PINMUX
 	select PINCONF
+	select GENERIC_PINCONF
 	select GPIOLIB_IRQCHIP
 
 config PINCTRL_IPROC_GPIO
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 785c366..a0b1f5f 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -36,11 +36,13 @@
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
+#include <dt-bindings/pinctrl/bcm2835.h>
 
 #define MODULE_NAME "pinctrl-bcm2835"
 #define BCM2835_NUM_GPIOS 54
@@ -72,13 +74,9 @@
 
 enum bcm2835_pinconf_param {
 	/* argument: bcm2835_pinconf_pull */
-	BCM2835_PINCONF_PARAM_PULL,
+	BCM2835_PINCONF_PARAM_PULL = (PIN_CONFIG_END + 1),
 };
 
-#define BCM2835_PINCONF_PACK(_param_, _arg_) ((_param_) << 16 | (_arg_))
-#define BCM2835_PINCONF_UNPACK_PARAM(_conf_) ((_conf_) >> 16)
-#define BCM2835_PINCONF_UNPACK_ARG(_conf_) ((_conf_) & 0xffff)
-
 struct bcm2835_pinctrl {
 	struct device *dev;
 	void __iomem *base;
@@ -213,14 +211,6 @@ static const char * const bcm2835_gpio_groups[] = {
 };
 
 enum bcm2835_fsel {
-	BCM2835_FSEL_GPIO_IN = 0,
-	BCM2835_FSEL_GPIO_OUT = 1,
-	BCM2835_FSEL_ALT0 = 4,
-	BCM2835_FSEL_ALT1 = 5,
-	BCM2835_FSEL_ALT2 = 6,
-	BCM2835_FSEL_ALT3 = 7,
-	BCM2835_FSEL_ALT4 = 3,
-	BCM2835_FSEL_ALT5 = 2,
 	BCM2835_FSEL_COUNT = 8,
 	BCM2835_FSEL_MASK = 0x7,
 };
@@ -714,7 +704,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
 	configs = kzalloc(sizeof(*configs), GFP_KERNEL);
 	if (!configs)
 		return -ENOMEM;
-	configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_PULL, pull);
+	configs[0] = pinconf_to_config_packed(BCM2835_PINCONF_PARAM_PULL, pull);
 
 	map->type = PIN_MAP_TYPE_CONFIGS_PIN;
 	map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
@@ -727,7 +717,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
 
 static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		struct device_node *np,
-		struct pinctrl_map **map, unsigned *num_maps)
+		struct pinctrl_map **map, unsigned int *num_maps)
 {
 	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
 	struct property *pins, *funcs, *pulls;
@@ -736,6 +726,12 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 	int i, err;
 	u32 pin, func, pull;
 
+	/* Check for generic binding in this node */
+	err = pinconf_generic_dt_node_to_map_all(pctldev, np, map, num_maps);
+	if (err || *num_maps)
+		return err;
+
+	/* Generic binding did not find anything continue with legacy parse */
 	pins = of_find_property(np, "brcm,pins", NULL);
 	if (!pins) {
 		dev_err(pc->dev, "%pOF: missing brcm,pins property\n", np);
@@ -917,37 +913,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev,
 	return -ENOTSUPP;
 }
 
+static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc,
+		unsigned int pin, unsigned int arg)
+{
+	u32 off, bit;
+
+	off = GPIO_REG_OFFSET(pin);
+	bit = GPIO_REG_SHIFT(pin);
+
+	bcm2835_gpio_wr(pc, GPPUD, arg & 3);
+	/*
+	 * BCM2835 datasheet say to wait 150 cycles, but not of what.
+	 * But the VideoCore firmware delay for this operation
+	 * based nearly on the same amount of VPU cycles and this clock
+	 * runs at 250 MHz.
+	 */
+	udelay(1);
+	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
+	udelay(1);
+	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+}
+
 static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
-			unsigned pin, unsigned long *configs,
-			unsigned num_configs)
+			unsigned int pin, unsigned long *configs,
+			unsigned int num_configs)
 {
 	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
-	enum bcm2835_pinconf_param param;
-	u16 arg;
-	u32 off, bit;
+	u32 param, arg;
 	int i;
 
 	for (i = 0; i < num_configs; i++) {
-		param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
-		arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
 
-		if (param != BCM2835_PINCONF_PARAM_PULL)
-			return -EINVAL;
+		switch (param) {
+		/* Set legacy brcm,pull */
+		case BCM2835_PINCONF_PARAM_PULL:
+			bcm2835_pull_config_set(pc, pin, arg);
+			break;
 
-		off = GPIO_REG_OFFSET(pin);
-		bit = GPIO_REG_SHIFT(pin);
+		/* Set pull generic bindings */
+		case PIN_CONFIG_BIAS_DISABLE:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_OFF);
+			break;
 
-		bcm2835_gpio_wr(pc, GPPUD, arg & 3);
-		/*
-		 * BCM2835 datasheet say to wait 150 cycles, but not of what.
-		 * But the VideoCore firmware delay for this operation
-		 * based nearly on the same amount of VPU cycles and this clock
-		 * runs at 250 MHz.
-		 */
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_DOWN);
+			break;
+
+		case PIN_CONFIG_BIAS_PULL_UP:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
+			break;
+
+		default:
+			return -EINVAL;
+
+		} /* switch param type */
 	} /* for each config */
 
 	return 0;
-- 
2.7.4

Re: [PATCH v7 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Stefan Wahren]

> Matheus Castello <matheus@castello.eng.br> hat am 1. Mai 2018 um 02:42 geschrieben:
> 
> 
> To keep driver up to date we add generic pinctrl binding support, which
> covers the features used in this driver and has additional node properties
> that this SoC has compatibility, so enabling future implementations of
> these properties without the need to create new node properties in the
> device trees.
> 
> The logic of this change maintain the old brcm legacy binding support in
> order to keep the ABI stable.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> Reviewed-by: Eric Anholt <eric@anholt.net>

Patches 2 and 3 are

Acked-by: Stefan Wahren <stefan.wahren@i2se.com>

Thanks
Stefan

Btw Stephen isn't BCM2835 maintainer anymore

Re: [PATCH v7 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Stefan Wahren]

Hi Linus,

> Stefan Wahren <stefan.wahren@i2se.com> hat am 3. Mai 2018 um 19:38 geschrieben:
> 
> 
> 
> > Matheus Castello <matheus@castello.eng.br> hat am 1. Mai 2018 um 02:42 geschrieben:
> > 
> > 
> > To keep driver up to date we add generic pinctrl binding support, which
> > covers the features used in this driver and has additional node properties
> > that this SoC has compatibility, so enabling future implementations of
> > these properties without the need to create new node properties in the
> > device trees.
> > 
> > The logic of this change maintain the old brcm legacy binding support in
> > order to keep the ABI stable.
> > 
> > Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> > Reviewed-by: Eric Anholt <eric@anholt.net>
> 
> Patches 2 and 3 are
> 
> Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
> 
> Thanks
> Stefan

gentle ping ...
> 
> Btw Stephen isn't BCM2835 maintainer anymore

Re: [PATCH v7 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Linus Walleij]

On Tue, May 1, 2018 at 2:42 AM, Matheus Castello
<matheus@castello.eng.br> wrote:

> To keep driver up to date we add generic pinctrl binding support, which
> covers the features used in this driver and has additional node properties
> that this SoC has compatibility, so enabling future implementations of
> these properties without the need to create new node properties in the
> device trees.
>
> The logic of this change maintain the old brcm legacy binding support in
> order to keep the ABI stable.
>
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> Reviewed-by: Eric Anholt <eric@anholt.net>

Patch applied with Stefan's ACK!

Yours,
Linus Walleij

[PATCH v7 3/3] pinctrl: bcm2835: Add support for output-low output-high properties

[Matheus Castello]

Properties to set initial value of pin output buffer.
This can be useful for configure hardware in overlay files, and in early
boot for checking it states in QA sanity tests.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index a0b1f5f..136ccaf 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -965,6 +965,11 @@ static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
 			break;
 
+		/* Set output-high or output-low */
+		case PIN_CONFIG_OUTPUT:
+			bcm2835_gpio_set_bit(pc, arg ? GPSET0 : GPCLR0, pin);
+			break;
+
 		default:
 			return -EINVAL;
 
-- 
2.7.4

Re: [PATCH v7 3/3] pinctrl: bcm2835: Add support for output-low output-high properties

[Linus Walleij]

On Tue, May 1, 2018 at 2:42 AM, Matheus Castello
<matheus@castello.eng.br> wrote:

> Properties to set initial value of pin output buffer.
> This can be useful for configure hardware in overlay files, and in early
> boot for checking it states in QA sanity tests.
>
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> Reviewed-by: Eric Anholt <eric@anholt.net>

Patch applied with Stefan's ACK.

Yours,
Linus Walleij

[PATCH v4 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Matheus Castello]

To keep driver up to date we add generic pinctrl binding support, which covers
the features used in this driver and has additional node properties that this
SoC has compatibility, so enabling future implementations of these properties
without the need to create new node properties in the device trees.

The logic of this change maintain the old brcm legacy binding support in order
to keep the ABI stable.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---

A brief explanation of what I did:

Add pinconf-generic header for use the defines and pinctrl-generic API.

Add dt-bindings pinctrl bcm2835 header to use functions selections and
pulls definitions, which functions definitions where duplicated in the
enum bcm2835_fsel, I removed the duplicate defines from enum.

In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for
pack the legacy param and arguments, since it will be unpacked along with
generic properties that is packed with this same macro.

In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use
pinctrl-generic parse code instead of parsing it inside the driver, so code
first check for generic binding parse, if something is parsed then it is
assumed that are using the new generic style, and when nothing is found then
parse continues to search for legacy properties.

In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and
was added a switch for the parameter tests, since pinctrl generic uses 3
properties to define the states of the pull instead of one with arguments, that
was the reason too that bcm2835_pull_config_set function was added, for reuse
the code that set state of pull.

 drivers/pinctrl/bcm/Kconfig           |  1 +
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 87 ++++++++++++++++++++++-------------
 2 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig
index e8c4e4f..0f38d51 100644
--- a/drivers/pinctrl/bcm/Kconfig
+++ b/drivers/pinctrl/bcm/Kconfig
@@ -20,6 +20,7 @@ config PINCTRL_BCM2835
 	bool
 	select PINMUX
 	select PINCONF
+	select GENERIC_PINCONF
 	select GPIOLIB_IRQCHIP
 
 config PINCTRL_IPROC_GPIO
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 785c366..010c565 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -36,11 +36,13 @@
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
+#include <dt-bindings/pinctrl/bcm2835.h>
 
 #define MODULE_NAME "pinctrl-bcm2835"
 #define BCM2835_NUM_GPIOS 54
@@ -75,10 +77,6 @@ enum bcm2835_pinconf_param {
 	BCM2835_PINCONF_PARAM_PULL,
 };
 
-#define BCM2835_PINCONF_PACK(_param_, _arg_) ((_param_) << 16 | (_arg_))
-#define BCM2835_PINCONF_UNPACK_PARAM(_conf_) ((_conf_) >> 16)
-#define BCM2835_PINCONF_UNPACK_ARG(_conf_) ((_conf_) & 0xffff)
-
 struct bcm2835_pinctrl {
 	struct device *dev;
 	void __iomem *base;
@@ -213,14 +211,6 @@ static const char * const bcm2835_gpio_groups[] = {
 };
 
 enum bcm2835_fsel {
-	BCM2835_FSEL_GPIO_IN = 0,
-	BCM2835_FSEL_GPIO_OUT = 1,
-	BCM2835_FSEL_ALT0 = 4,
-	BCM2835_FSEL_ALT1 = 5,
-	BCM2835_FSEL_ALT2 = 6,
-	BCM2835_FSEL_ALT3 = 7,
-	BCM2835_FSEL_ALT4 = 3,
-	BCM2835_FSEL_ALT5 = 2,
 	BCM2835_FSEL_COUNT = 8,
 	BCM2835_FSEL_MASK = 0x7,
 };
@@ -714,7 +704,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
 	configs = kzalloc(sizeof(*configs), GFP_KERNEL);
 	if (!configs)
 		return -ENOMEM;
-	configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_PULL, pull);
+	configs[0] = pinconf_to_config_packed(BCM2835_PINCONF_PARAM_PULL, pull);
 
 	map->type = PIN_MAP_TYPE_CONFIGS_PIN;
 	map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
@@ -736,6 +726,12 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 	int i, err;
 	u32 pin, func, pull;
 
+	/* Check for generic binding in this node */
+	err = pinconf_generic_dt_node_to_map_all(pctldev, np, map, num_maps);
+	if (err || *num_maps)
+		return err;
+
+	/* Generic binding did not find anything continue with legacy parse */
 	pins = of_find_property(np, "brcm,pins", NULL);
 	if (!pins) {
 		dev_err(pc->dev, "%pOF: missing brcm,pins property\n", np);
@@ -917,37 +913,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev,
 	return -ENOTSUPP;
 }
 
+static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc,
+		unsigned pin, unsigned arg)
+{
+	u32 off, bit;
+
+	off = GPIO_REG_OFFSET(pin);
+	bit = GPIO_REG_SHIFT(pin);
+
+	bcm2835_gpio_wr(pc, GPPUD, arg & 3);
+	/*
+	* BCM2835 datasheet say to wait 150 cycles, but not of what.
+	* But the VideoCore firmware delay for this operation
+	* based nearly on the same amount of VPU cycles and this clock
+	* runs at 250 MHz.
+	*/
+	udelay(1);
+	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
+	udelay(1);
+	bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+}
+
 static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 			unsigned pin, unsigned long *configs,
 			unsigned num_configs)
 {
 	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
-	enum bcm2835_pinconf_param param;
-	u16 arg;
-	u32 off, bit;
+	u32 param, arg;
 	int i;
 
 	for (i = 0; i < num_configs; i++) {
-		param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]);
-		arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]);
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
 
-		if (param != BCM2835_PINCONF_PARAM_PULL)
-			return -EINVAL;
+		switch (param) {
+		/* Set legacy brcm,pull */
+		case BCM2835_PINCONF_PARAM_PULL:
+			bcm2835_pull_config_set(pc, pin, arg);
+			break;
 
-		off = GPIO_REG_OFFSET(pin);
-		bit = GPIO_REG_SHIFT(pin);
+		/* Set pull generic bindings */
+		case PIN_CONFIG_BIAS_DISABLE:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_OFF);
+			break;
 
-		bcm2835_gpio_wr(pc, GPPUD, arg & 3);
-		/*
-		 * BCM2835 datasheet say to wait 150 cycles, but not of what.
-		 * But the VideoCore firmware delay for this operation
-		 * based nearly on the same amount of VPU cycles and this clock
-		 * runs at 250 MHz.
-		 */
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
-		udelay(1);
-		bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_DOWN);
+			break;
+
+		case PIN_CONFIG_BIAS_PULL_UP:
+			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
+			break;
+
+		default:
+			return -EINVAL;
+
+		} /* switch param type */
 	} /* for each config */
 
 	return 0;
-- 
2.7.4

Re: [PATCH v4 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding

[Eric Anholt]

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

Matheus Castello <matheus@castello.eng.br> writes:

> To keep driver up to date we add generic pinctrl binding support, which covers
> the features used in this driver and has additional node properties that this
> SoC has compatibility, so enabling future implementations of these properties
> without the need to create new node properties in the device trees.
>
> The logic of this change maintain the old brcm legacy binding support in order
> to keep the ABI stable.
>
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>

2-3 are:

Reviewed-by: Eric Anholt <eric@anholt.net>

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

[PATCH v4 3/3] pinctrl: bcm2835: Add support for output-low output-high properties

[Matheus Castello]

Properties to set initial value of pin output buffer.
This can be useful for configure hardware in overlay files, and in early boot
for checking it states in QA sanity tests.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 010c565..28acd06 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -965,6 +965,11 @@ static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
 			bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP);
 			break;
 
+		/* Set output-high or output-low */
+		case PIN_CONFIG_OUTPUT:
+			bcm2835_gpio_set_bit(pc, arg ? GPSET0 : GPCLR0, pin);
+			break;
+
 		default:
 			return -EINVAL;
 
-- 
2.7.4