2018-02-22 18:08:46

by Matheus Castello

[permalink] [raw]
Subject: [PATCH] pinctrl: bc2835: Add brcm,level property

Property to set initial value of pin output buffer.

Signed-off-by: Matheus Castello <[email protected]>
---
.../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



2018-02-23 14:56:43

by Eric Anholt

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

Matheus Castello <[email protected]> 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?


Attachments:
signature.asc (847.00 B)

2018-02-23 16:48:10

by Matheus Castello

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

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 <[email protected]>
---
.../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


2018-03-02 10:15:52

by Linus Walleij

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

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
<[email protected]> wrote:
> Property to set initial value of pin output buffer.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> .../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
>

2018-03-03 14:23:19

by Stefan Wahren

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

Hi Matheus,

sorry for my late reply.

> Matheus Castello <[email protected]> hat am 22. Februar 2018 um 18:44 geschrieben:
>
>
> Property to set initial value of pin output buffer.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> .../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
>

2018-03-05 02:30:28

by Matheus Castello

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

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


2018-03-05 02:32:05

by Matheus Castello

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

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 <[email protected]>
---

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


2018-03-05 02:54:04

by Matheus Castello

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

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 <[email protected]>
---
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


2018-03-05 03:32:19

by Matheus Castello

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

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 <[email protected]>
---
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


2018-03-05 23:22:52

by Eric Anholt

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

Matheus Castello <[email protected]> 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 <[email protected]>
> ---
>
> 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


Attachments:
signature.asc (847.00 B)

2018-03-07 03:51:32

by Matheus Castello

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

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 <[email protected]> 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 <[email protected]>
>> ---
>>
>> 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

2018-03-07 11:53:23

by Stefan Wahren

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

Hi Matheus,

> Matheus Castello <[email protected]> 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 <[email protected]>
> ---
> 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
>

2018-03-07 12:01:05

by Stefan Wahren

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

Hi Matheus,

> Matheus Castello <[email protected]> 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
>

2018-03-08 00:38:22

by Matheus Castello

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

Hi Stefan,

On 03/07/2018 07:58 AM, Stefan Wahren wrote:
> Hi Matheus,
>
>> Matheus Castello <[email protected]> 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
>>

2018-03-08 08:03:00

by Stefan Wahren

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

Hi Matheus,

> Matheus Castello <[email protected]> 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 <[email protected]> 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
> >>

2018-03-08 20:12:14

by Matheus Castello

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

Hi Stefan,


On 03/08/2018 04:00 AM, Stefan Wahren wrote:
> Hi Matheus,
>
>> Matheus Castello <[email protected]> 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 <[email protected]> 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
>>>>

2018-03-09 04:19:03

by Matheus Castello

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

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 <[email protected]>
---

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


2018-03-09 04:41:04

by Matheus Castello

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

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


2018-03-09 04:41:18

by Matheus Castello

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

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 <[email protected]>
---
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


2018-03-09 04:43:23

by Matheus Castello

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

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

Signed-off-by: Matheus Castello <[email protected]>
---
.../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


2018-03-09 11:49:31

by Stefan Wahren

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

Hi Matheus,

> Matheus Castello <[email protected]> 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 <[email protected]>
> ---
> .../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
>

2018-03-09 17:16:55

by Matheus Castello

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

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 <[email protected]>
---

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


2018-03-09 17:17:19

by Matheus Castello

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

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


2018-03-09 17:17:22

by Matheus Castello

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

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 <[email protected]>
---
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


2018-03-09 17:36:27

by Matheus Castello

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

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

Signed-off-by: Matheus Castello <[email protected]>
---
.../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


2018-03-12 18:05:25

by Eric Anholt

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

Matheus Castello <[email protected]> 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 <[email protected]>

2-3 are:

Reviewed-by: Eric Anholt <[email protected]>


Attachments:
signature.asc (847.00 B)

2018-03-18 12:59:56

by Rob Herring (Arm)

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

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 <[email protected]>
> ---
> .../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
>

2018-03-18 15:25:22

by Matheus Castello

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

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 <[email protected]>
>> ---
>> .../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
>>

2018-04-11 05:24:05

by Matheus Castello

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

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


2018-04-11 05:26:08

by Matheus Castello

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

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 <[email protected]>
---

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


2018-04-11 05:26:26

by Matheus Castello

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

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 <[email protected]>
---
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


2018-04-11 05:27:42

by Matheus Castello

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

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

Signed-off-by: Matheus Castello <[email protected]>
---
.../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


2018-04-11 16:06:14

by Eric Anholt

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

Matheus Castello <[email protected]> writes:

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

All 3 patches are:

Reviewed-by: Eric Anholt <[email protected]>

Thanks for doing this!


Attachments:
signature.asc (847.00 B)

2018-04-16 15:09:29

by Rob Herring (Arm)

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

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 <[email protected]>
> ---
> .../devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)

Reviewed-by: Rob Herring <[email protected]>

2018-04-21 10:35:06

by Stefan Wahren

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

Hi Matheus,

> Matheus Castello <[email protected]> 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 <[email protected]>
> ---
>
> 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

2018-04-22 12:46:03

by Stefan Wahren

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

Hi Matheus,

> Matheus Castello <[email protected]> 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 <[email protected]>
> ---
>
> 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
>

2018-04-26 12:22:50

by Linus Walleij

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

On Wed, Apr 11, 2018 at 6:58 AM, Matheus Castello
<[email protected]> wrote:

> Added generic pin configuration and multiplexing support,
> and should be preferred than brcm legacy one.
>
> Signed-off-by: Matheus Castello <[email protected]>

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

Yours,
Linus Walleij

2018-04-26 12:26:08

by Linus Walleij

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

On Wed, Apr 11, 2018 at 6:58 AM, Matheus Castello
<[email protected]> 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 <[email protected]>

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

2018-04-26 12:27:17

by Linus Walleij

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

On Wed, Apr 11, 2018 at 6:58 AM, Matheus Castello
<[email protected]> 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 <[email protected]>

Will apply this when I apply 2/3.

You only need to resend patch 2+3.

Yours,
Linus Walleij

2018-04-29 19:52:31

by Matheus Castello

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

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 <[email protected]>
---

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


2018-04-29 19:54:43

by Matheus Castello

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

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


2018-04-29 20:10:09

by Matheus Castello

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

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 <[email protected]>
---
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


2018-05-01 00:43:36

by Matheus Castello

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

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 <[email protected]>
Reviewed-by: Eric Anholt <[email protected]>
---
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

2018-05-01 00:44:25

by Matheus Castello

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

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 <[email protected]>
Reviewed-by: Eric Anholt <[email protected]>
---
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

2018-05-01 01:06:05

by Matheus Castello

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

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

2018-05-03 17:39:32

by Stefan Wahren

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


> Matheus Castello <[email protected]> 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 <[email protected]>
> Reviewed-by: Eric Anholt <[email protected]>

Patches 2 and 3 are

Acked-by: Stefan Wahren <[email protected]>

Thanks
Stefan

Btw Stephen isn't BCM2835 maintainer anymore

2018-05-12 09:09:36

by Stefan Wahren

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

Hi Linus,

> Stefan Wahren <[email protected]> hat am 3. Mai 2018 um 19:38 geschrieben:
>
>
>
> > Matheus Castello <[email protected]> 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 <[email protected]>
> > Reviewed-by: Eric Anholt <[email protected]>
>
> Patches 2 and 3 are
>
> Acked-by: Stefan Wahren <[email protected]>
>
> Thanks
> Stefan

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

2018-05-16 12:01:28

by Linus Walleij

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

On Tue, May 1, 2018 at 2:42 AM, Matheus Castello
<[email protected]> 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 <[email protected]>
> Reviewed-by: Eric Anholt <[email protected]>

Patch applied with Stefan's ACK!

Yours,
Linus Walleij

2018-05-16 12:03:02

by Linus Walleij

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

On Tue, May 1, 2018 at 2:42 AM, Matheus Castello
<[email protected]> 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 <[email protected]>
> Reviewed-by: Eric Anholt <[email protected]>

Patch applied with Stefan's ACK.

Yours,
Linus Walleij