[PATCH 0/3] pinctrl: sunxi: Support generic pinconf functions

[PATCH 0/3] pinctrl: sunxi: Support generic pinconf functions

[Chen-Yu Tsai]

Hi everyone,

This series fixes up generic pinconf support for the sunxi pinctrl driver
library. The driver was doing some bits wrong, like a) storing the pinconf
config value in its struct, and not actually reading the hardware to get
the current config, and b) not using the right arguments for the bias
parameters.

Patch 1 makes the driver read out pinconf settings from the hardware, and
returns the correct value for unsupported features and disable features.
With this in place it also declares itself as generic pinconf compatible,
which enables us to read the config through the debugfs pinconf interface.

Patch 2 fixes the pin bias parameter arguments.

Patch 3 makes the sunxi_pconf_group_set callback use the helper function
introduced in patch 1. 

The patches will likely conflict with Maxime's generic pinctrl bindings
series. Lets figure something out.


Regards
ChenYu


Chen-Yu Tsai (3):
  pinctrl: sunxi: Add support for fetching pinconf settings from
    hardware
  pinctrl: sunxi: Fix PIN_CONFIG_BIAS_PULL_{DOWN,UP} argument
  pinctrl: sunxi: Make sunxi_pconf_group_set use sunxi_pconf_reg helper

 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 152 ++++++++++++++++++++++++++--------
 drivers/pinctrl/sunxi/pinctrl-sunxi.h |   1 -
 2 files changed, 118 insertions(+), 35 deletions(-)

-- 
2.9.3

[PATCH 1/3] pinctrl: sunxi: Add support for fetching pinconf settings from hardware

[Chen-Yu Tsai]

The sunxi pinctrl driver only caches whatever pinconf setting was last
set on a given pingroup. This is not particularly helpful, nor is it
correct.

Fix this by actually reading the hardware registers and returning
the correct results or error codes. Also filter out unsupported
pinconf settings. Since this driver has a peculiar setup of 1 pin
per group, we can support both pin and pingroup pinconf setting
read back with the same code. The sunxi_pconf_reg helper and code
structure is inspired by pinctrl-msm.

With this done we can also claim to support generic pinconf, by
setting .is_generic = true in pinconf_ops.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 85 +++++++++++++++++++++++++++++++++--
 1 file changed, 82 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 54455af566ec..609843c9a65c 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -10,6 +10,7 @@
  * warranty of any kind, whether express or implied.
  */
 
+#include <dt-bindings/pinctrl/sun4i-a10.h>
 #include <linux/io.h>
 #include <linux/clk.h>
 #include <linux/gpio/driver.h>
@@ -269,15 +270,91 @@ static const struct pinctrl_ops sunxi_pctrl_ops = {
 	.get_group_pins		= sunxi_pctrl_get_group_pins,
 };
 
+static int sunxi_pconf_reg(unsigned pin, enum pin_config_param param,
+			   u32 *offset, u32 *shift, u32 *mask)
+{
+	switch (param) {
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		*offset = sunxi_dlevel_reg(pin);
+		*shift = sunxi_dlevel_offset(pin);
+		*mask = DLEVEL_PINS_MASK;
+		break;
+
+	case PIN_CONFIG_BIAS_PULL_UP:
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+	case PIN_CONFIG_BIAS_DISABLE:
+		*offset = sunxi_pull_reg(pin);
+		*shift = sunxi_pull_offset(pin);
+		*mask = PULL_PINS_MASK;
+		break;
+
+	default:
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+static int sunxi_pconf_get(struct pinctrl_dev *pctldev, unsigned pin,
+			   unsigned long *config)
+{
+	struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	u32 offset, shift, mask, val;
+	u16 arg;
+	int ret;
+
+	pin -= pctl->desc->pin_base;
+
+	ret = sunxi_pconf_reg(pin, param, &offset, &shift, &mask);
+	if (ret < 0)
+		return ret;
+
+	val = (readl(pctl->membase + offset) >> shift) & mask;
+
+	switch (pinconf_to_config_param(*config)) {
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		arg = (val + 1) * 10;
+		break;
+
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (val != SUN4I_PINCTRL_PULL_UP)
+			return -EINVAL;
+		arg = 1; /* hardware is weak pull-up */
+		break;
+
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (val != SUN4I_PINCTRL_PULL_DOWN)
+			return -EINVAL;
+		arg = 1; /* hardware is weak pull-down */
+		break;
+
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (val != SUN4I_PINCTRL_NO_PULL)
+			return -EINVAL;
+		arg = 0;
+		break;
+
+	default:
+		/* sunxi_pconf_reg should catch anything unsupported */
+		WARN_ON(1);
+		return -ENOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return 0;
+}
+
 static int sunxi_pconf_group_get(struct pinctrl_dev *pctldev,
 				 unsigned group,
 				 unsigned long *config)
 {
 	struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct sunxi_pinctrl_group *g = &pctl->groups[group];
 
-	*config = pctl->groups[group].config;
-
-	return 0;
+	/* We only support 1 pin per group. Chain it to the pin callback */
+	return sunxi_pconf_get(pctldev, g->pin, config);
 }
 
 static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
@@ -343,6 +420,8 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
 }
 
 static const struct pinconf_ops sunxi_pconf_ops = {
+	.is_generic		= true,
+	.pin_config_get		= sunxi_pconf_get,
 	.pin_config_group_get	= sunxi_pconf_group_get,
 	.pin_config_group_set	= sunxi_pconf_group_set,
 };
-- 
2.9.3

Re: [PATCH 1/3] pinctrl: sunxi: Add support for fetching pinconf settings from hardware

[Maxime Ripard]

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

On Tue, Oct 04, 2016 at 09:51:10AM +0800, Chen-Yu Tsai wrote:
> The sunxi pinctrl driver only caches whatever pinconf setting was last
> set on a given pingroup. This is not particularly helpful, nor is it
> correct.
> 
> Fix this by actually reading the hardware registers and returning
> the correct results or error codes. Also filter out unsupported
> pinconf settings. Since this driver has a peculiar setup of 1 pin
> per group, we can support both pin and pingroup pinconf setting
> read back with the same code. The sunxi_pconf_reg helper and code
> structure is inspired by pinctrl-msm.
> 
> With this done we can also claim to support generic pinconf, by
> setting .is_generic = true in pinconf_ops.
> 
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> ---
>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 85 +++++++++++++++++++++++++++++++++--
>  1 file changed, 82 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 54455af566ec..609843c9a65c 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -10,6 +10,7 @@
>   * warranty of any kind, whether express or implied.
>   */
>  
> +#include <dt-bindings/pinctrl/sun4i-a10.h>
>  #include <linux/io.h>
>  #include <linux/clk.h>
>  #include <linux/gpio/driver.h>
> @@ -269,15 +270,91 @@ static const struct pinctrl_ops sunxi_pctrl_ops = {
>  	.get_group_pins		= sunxi_pctrl_get_group_pins,
>  };
>  
> +static int sunxi_pconf_reg(unsigned pin, enum pin_config_param param,
> +			   u32 *offset, u32 *shift, u32 *mask)
> +{
> +	switch (param) {
> +	case PIN_CONFIG_DRIVE_STRENGTH:
> +		*offset = sunxi_dlevel_reg(pin);
> +		*shift = sunxi_dlevel_offset(pin);
> +		*mask = DLEVEL_PINS_MASK;
> +		break;
> +
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		*offset = sunxi_pull_reg(pin);
> +		*shift = sunxi_pull_offset(pin);
> +		*mask = PULL_PINS_MASK;
> +		break;
> +
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sunxi_pconf_get(struct pinctrl_dev *pctldev, unsigned pin,
> +			   unsigned long *config)
> +{
> +	struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +	enum pin_config_param param = pinconf_to_config_param(*config);
> +	u32 offset, shift, mask, val;
> +	u16 arg;
> +	int ret;
> +
> +	pin -= pctl->desc->pin_base;
> +
> +	ret = sunxi_pconf_reg(pin, param, &offset, &shift, &mask);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = (readl(pctl->membase + offset) >> shift) & mask;
> +
> +	switch (pinconf_to_config_param(*config)) {
> +	case PIN_CONFIG_DRIVE_STRENGTH:
> +		arg = (val + 1) * 10;
> +		break;
> +
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		if (val != SUN4I_PINCTRL_PULL_UP)
> +			return -EINVAL;
> +		arg = 1; /* hardware is weak pull-up */
> +		break;
> +
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		if (val != SUN4I_PINCTRL_PULL_DOWN)
> +			return -EINVAL;
> +		arg = 1; /* hardware is weak pull-down */
> +		break;
> +
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		if (val != SUN4I_PINCTRL_NO_PULL)
> +			return -EINVAL;
> +		arg = 0;
> +		break;
> +
> +	default:
> +		/* sunxi_pconf_reg should catch anything unsupported */
> +		WARN_ON(1);
> +		return -ENOTSUPP;

This should be EINVAL. The operation is supported, but one of the
argument is not valid.

Maxime

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

Re: [PATCH 1/3] pinctrl: sunxi: Add support for fetching pinconf settings from hardware

[Chen-Yu Tsai]

On Tue, Oct 4, 2016 at 4:02 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Tue, Oct 04, 2016 at 09:51:10AM +0800, Chen-Yu Tsai wrote:
>> The sunxi pinctrl driver only caches whatever pinconf setting was last
>> set on a given pingroup. This is not particularly helpful, nor is it
>> correct.
>>
>> Fix this by actually reading the hardware registers and returning
>> the correct results or error codes. Also filter out unsupported
>> pinconf settings. Since this driver has a peculiar setup of 1 pin
>> per group, we can support both pin and pingroup pinconf setting
>> read back with the same code. The sunxi_pconf_reg helper and code
>> structure is inspired by pinctrl-msm.
>>
>> With this done we can also claim to support generic pinconf, by
>> setting .is_generic = true in pinconf_ops.
>>
>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 85 +++++++++++++++++++++++++++++++++--
>>  1 file changed, 82 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> index 54455af566ec..609843c9a65c 100644
>> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> @@ -10,6 +10,7 @@
>>   * warranty of any kind, whether express or implied.
>>   */
>>
>> +#include <dt-bindings/pinctrl/sun4i-a10.h>
>>  #include <linux/io.h>
>>  #include <linux/clk.h>
>>  #include <linux/gpio/driver.h>
>> @@ -269,15 +270,91 @@ static const struct pinctrl_ops sunxi_pctrl_ops = {
>>       .get_group_pins         = sunxi_pctrl_get_group_pins,
>>  };
>>
>> +static int sunxi_pconf_reg(unsigned pin, enum pin_config_param param,
>> +                        u32 *offset, u32 *shift, u32 *mask)
>> +{
>> +     switch (param) {
>> +     case PIN_CONFIG_DRIVE_STRENGTH:
>> +             *offset = sunxi_dlevel_reg(pin);
>> +             *shift = sunxi_dlevel_offset(pin);
>> +             *mask = DLEVEL_PINS_MASK;
>> +             break;
>> +
>> +     case PIN_CONFIG_BIAS_PULL_UP:
>> +     case PIN_CONFIG_BIAS_PULL_DOWN:
>> +     case PIN_CONFIG_BIAS_DISABLE:
>> +             *offset = sunxi_pull_reg(pin);
>> +             *shift = sunxi_pull_offset(pin);
>> +             *mask = PULL_PINS_MASK;
>> +             break;
>> +
>> +     default:
>> +             return -ENOTSUPP;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int sunxi_pconf_get(struct pinctrl_dev *pctldev, unsigned pin,
>> +                        unsigned long *config)
>> +{
>> +     struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
>> +     enum pin_config_param param = pinconf_to_config_param(*config);
>> +     u32 offset, shift, mask, val;
>> +     u16 arg;
>> +     int ret;
>> +
>> +     pin -= pctl->desc->pin_base;
>> +
>> +     ret = sunxi_pconf_reg(pin, param, &offset, &shift, &mask);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     val = (readl(pctl->membase + offset) >> shift) & mask;
>> +
>> +     switch (pinconf_to_config_param(*config)) {
>> +     case PIN_CONFIG_DRIVE_STRENGTH:
>> +             arg = (val + 1) * 10;
>> +             break;
>> +
>> +     case PIN_CONFIG_BIAS_PULL_UP:
>> +             if (val != SUN4I_PINCTRL_PULL_UP)
>> +                     return -EINVAL;
>> +             arg = 1; /* hardware is weak pull-up */
>> +             break;
>> +
>> +     case PIN_CONFIG_BIAS_PULL_DOWN:
>> +             if (val != SUN4I_PINCTRL_PULL_DOWN)
>> +                     return -EINVAL;
>> +             arg = 1; /* hardware is weak pull-down */
>> +             break;
>> +
>> +     case PIN_CONFIG_BIAS_DISABLE:
>> +             if (val != SUN4I_PINCTRL_NO_PULL)
>> +                     return -EINVAL;
>> +             arg = 0;
>> +             break;
>> +
>> +     default:
>> +             /* sunxi_pconf_reg should catch anything unsupported */
>> +             WARN_ON(1);
>> +             return -ENOTSUPP;
>
> This should be EINVAL. The operation is supported, but one of the
> argument is not valid.

According to include/linux/pinctrl/pinconf.h

 * @pin_config_get: get the config of a certain pin, if the requested config
 *      is not available on this controller this should return -ENOTSUPP
 *      and if it is available but disabled it should return -EINVAL

So I think it is correct to return -ENOTSUPP here.

ChenYu

[PATCH 2/3] pinctrl: sunxi: Fix PIN_CONFIG_BIAS_PULL_{DOWN,UP} argument

[Chen-Yu Tsai]

According to pinconf-generic.h, the argument for
PIN_CONFIG_BIAS_PULL_{DOWN,UP} is non-zero if the bias is enabled
with a pull up/down resistor, zero if it is directly connected
to VDD or ground.

Since Allwinner hardware uses a weak pull resistor internally,
the argument should be 1.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 609843c9a65c..236272a2339d 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -234,7 +234,7 @@ static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
 				pull = PIN_CONFIG_BIAS_PULL_UP;
 			else if (val == 2)
 				pull = PIN_CONFIG_BIAS_PULL_DOWN;
-			pinconfig[j++] = pinconf_to_config_packed(pull, 0);
+			pinconfig[j++] = pinconf_to_config_packed(pull, 1);
 		}
 
 		(*map)[i].data.configs.configs = pinconfig;
-- 
2.9.3

[PATCH 3/3] pinctrl: sunxi: Make sunxi_pconf_group_set use sunxi_pconf_reg helper

[Chen-Yu Tsai]

The sunxi_pconf_reg helper introduced in the last patch gives us the
chance to rework sunxi_pconf_group_set to have it match the structure
of sunxi_pconf_(group_)get and make it easier to understand.

For each config to set, it:

    1. checks if the parameter is supported.
    2. checks if the argument is within limits.
    3. converts argument to the register value.
    4. writes to the register with spinlock held.

As a result the function now blocks unsupported config parameters,
instead of silently ignoring them.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 65 +++++++++++++++++++----------------
 drivers/pinctrl/sunxi/pinctrl-sunxi.h |  1 -
 2 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 236272a2339d..1f02c4cd55c7 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -364,23 +364,27 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
 {
 	struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
 	struct sunxi_pinctrl_group *g = &pctl->groups[group];
-	unsigned long flags;
 	unsigned pin = g->pin - pctl->desc->pin_base;
-	u32 val, mask;
-	u16 strength;
-	u8 dlevel;
 	int i;
 
-	spin_lock_irqsave(&pctl->lock, flags);
-
 	for (i = 0; i < num_configs; i++) {
-		switch (pinconf_to_config_param(configs[i])) {
+		enum pin_config_param param;
+		unsigned long flags;
+		u32 offset, shift, mask, val;
+		u16 arg;
+		int ret;
+
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		ret = sunxi_pconf_reg(pin, param, &offset, &shift, &mask);
+		if (ret < 0)
+			return ret;
+
+		switch (param) {
 		case PIN_CONFIG_DRIVE_STRENGTH:
-			strength = pinconf_to_config_argument(configs[i]);
-			if (strength > 40) {
-				spin_unlock_irqrestore(&pctl->lock, flags);
+			if (arg < 10 || arg > 40)
 				return -EINVAL;
-			}
 			/*
 			 * We convert from mA to what the register expects:
 			 *   0: 10mA
@@ -388,33 +392,34 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
 			 *   2: 30mA
 			 *   3: 40mA
 			 */
-			dlevel = strength / 10 - 1;
-			val = readl(pctl->membase + sunxi_dlevel_reg(pin));
-			mask = DLEVEL_PINS_MASK << sunxi_dlevel_offset(pin);
-			writel((val & ~mask)
-				| dlevel << sunxi_dlevel_offset(pin),
-				pctl->membase + sunxi_dlevel_reg(pin));
+			arg = arg / 10 - 1;
 			break;
 		case PIN_CONFIG_BIAS_PULL_UP:
-			val = readl(pctl->membase + sunxi_pull_reg(pin));
-			mask = PULL_PINS_MASK << sunxi_pull_offset(pin);
-			writel((val & ~mask) | 1 << sunxi_pull_offset(pin),
-				pctl->membase + sunxi_pull_reg(pin));
+			if (arg == 0)
+				return -EINVAL;
+			arg = 1;
 			break;
 		case PIN_CONFIG_BIAS_PULL_DOWN:
-			val = readl(pctl->membase + sunxi_pull_reg(pin));
-			mask = PULL_PINS_MASK << sunxi_pull_offset(pin);
-			writel((val & ~mask) | 2 << sunxi_pull_offset(pin),
-				pctl->membase + sunxi_pull_reg(pin));
+			if (arg == 0)
+				return -EINVAL;
+			arg = 2;
 			break;
-		default:
+		case PIN_CONFIG_BIAS_DISABLE:
+			arg = 0;
 			break;
+
+		default:
+			/* sunxi_pconf_reg should catch anything unsupported */
+			WARN_ON(1);
+			return -ENOTSUPP;
 		}
-		/* cache the config value */
-		g->config = configs[i];
-	} /* for each config */
 
-	spin_unlock_irqrestore(&pctl->lock, flags);
+		spin_lock_irqsave(&pctl->lock, flags);
+		val = readl(pctl->membase + offset);
+		val &= ~(mask << shift);
+		writel(val | arg << shift, pctl->membase + offset);
+		spin_unlock_irqrestore(&pctl->lock, flags);
+	} /* for each config */
 
 	return 0;
 }
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.h b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
index 0afce1ab12d0..a7efb31d6523 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
@@ -109,7 +109,6 @@ struct sunxi_pinctrl_function {
 
 struct sunxi_pinctrl_group {
 	const char	*name;
-	unsigned long	config;
 	unsigned	pin;
 };
 
-- 
2.9.3

Re: [PATCH 3/3] pinctrl: sunxi: Make sunxi_pconf_group_set use sunxi_pconf_reg helper

[Maxime Ripard]

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

On Tue, Oct 04, 2016 at 09:51:12AM +0800, Chen-Yu Tsai wrote:
> The sunxi_pconf_reg helper introduced in the last patch gives us the
> chance to rework sunxi_pconf_group_set to have it match the structure
> of sunxi_pconf_(group_)get and make it easier to understand.
> 
> For each config to set, it:
> 
>     1. checks if the parameter is supported.
>     2. checks if the argument is within limits.
>     3. converts argument to the register value.
>     4. writes to the register with spinlock held.
> 
> As a result the function now blocks unsupported config parameters,
> instead of silently ignoring them.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 65 +++++++++++++++++++----------------
>  drivers/pinctrl/sunxi/pinctrl-sunxi.h |  1 -
>  2 files changed, 35 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 236272a2339d..1f02c4cd55c7 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -364,23 +364,27 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
>  {
>  	struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
>  	struct sunxi_pinctrl_group *g = &pctl->groups[group];
> -	unsigned long flags;
>  	unsigned pin = g->pin - pctl->desc->pin_base;
> -	u32 val, mask;
> -	u16 strength;
> -	u8 dlevel;
>  	int i;
>  
> -	spin_lock_irqsave(&pctl->lock, flags);
> -
>  	for (i = 0; i < num_configs; i++) {
> -		switch (pinconf_to_config_param(configs[i])) {
> +		enum pin_config_param param;
> +		unsigned long flags;
> +		u32 offset, shift, mask, val;
> +		u16 arg;
> +		int ret;
> +
> +		param = pinconf_to_config_param(configs[i]);
> +		arg = pinconf_to_config_argument(configs[i]);
> +
> +		ret = sunxi_pconf_reg(pin, param, &offset, &shift, &mask);
> +		if (ret < 0)
> +			return ret;
> +
> +		switch (param) {
>  		case PIN_CONFIG_DRIVE_STRENGTH:
> -			strength = pinconf_to_config_argument(configs[i]);
> -			if (strength > 40) {
> -				spin_unlock_irqrestore(&pctl->lock, flags);
> +			if (arg < 10 || arg > 40)

This is a nitpick, but I'd really like to store the value in a
separate variable, to have a distinction between the value that was
given us as an argument, and what we're going to write.

Thanks!
Maxime

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

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

Re: [PATCH 0/3] pinctrl: sunxi: Support generic pinconf functions

[Linus Walleij]

On Tue, Oct 4, 2016 at 3:51 AM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:

> This series fixes up generic pinconf support for the sunxi pinctrl driver
> library. The driver was doing some bits wrong, like a) storing the pinconf
> config value in its struct, and not actually reading the hardware to get
> the current config, and b) not using the right arguments for the bias
> parameters.

Looks OK to me, Maxime are you OK with me applying this, or
are we waiting for a new iteration of this patch set?

I have started to apply Maxime's generic binding rework, will it
clash with these?

Yours,
Linus Walleij

Re: Re: [PATCH 0/3] pinctrl: sunxi: Support generic pinconf functions

[Chen-Yu Tsai]

On Thu, Oct 20, 2016 at 9:13 PM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Oct 4, 2016 at 3:51 AM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
>
>> This series fixes up generic pinconf support for the sunxi pinctrl driver
>> library. The driver was doing some bits wrong, like a) storing the pinconf
>> config value in its struct, and not actually reading the hardware to get
>> the current config, and b) not using the right arguments for the bias
>> parameters.
>
> Looks OK to me, Maxime are you OK with me applying this, or
> are we waiting for a new iteration of this patch set?
>
> I have started to apply Maxime's generic binding rework, will it
> clash with these?

They will, as my patches also touch the pinconf .get/.set functions.

ChenYu

>
> Yours,
> Linus Walleij
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> For more options, visit https://groups.google.com/d/optout.