[PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits

[PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits

[Peter Ujfalusi]

Hello,

When configuring pinmux with pinctrl-single there could be a case when one
register is used to configure mux for more than one pin.
In this case the use of pinctrl-single,pins is a bit problematic since we can
only update the whole register (restricted by the mask).
In such a situations the pinctrl-single,bits could provide a safe way to handle
the mux.

pinctrl-single,bits takes three parameters: <reg offset, value, sub-mask>
The sub mask is used to mask part of the register to make sure we do not change
bits outside of the scope of this pin.

The first patch in this series is to fix the previous pinctrl-since,pins
implementation because it was not using the mask on the value which could result
changed bits outside of the mask.

Regards,
Peter
---
Peter Ujfalusi (2):
  pinctrl: pinctrl-single: Make sure we do not change bits outside of
    mask
  pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux

 .../devicetree/bindings/pinctrl/pinctrl-single.txt |  9 +++++
 drivers/pinctrl/pinctrl-single.c                   | 42 ++++++++++++++++------
 2 files changed, 41 insertions(+), 10 deletions(-)

-- 
1.7.12

[PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask

[Peter Ujfalusi]

Use the pcs->fmask to make sure that the value is not changing (setting)
bits in areas where it should not.
To avoid situations like this:

pmx_dummy: pinmux@4a100040 {
	compatible = "pinctrl-single";
	reg = <0x4a100040 0x0196>;
	#address-cells = <1>;
	#size-cells = <0>;
	pinctrl-single,register-width = <16>;
	pinctrl-single,function-mask = <0x00ff>;
};

&pmx_dummy {
	pinctrl-names = "default";
	pinctrl-0 = <&board_pins>;

	board_pins: pinmux_board_pins {
		pinctrl-single,pins = <
			0x6c 0xf0f
			0x6e 0x10f
			0x70 0x23f
			0x72 0xa5f
		>;
	};
};

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/pinctrl/pinctrl-single.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 76a4260..3508631 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -337,7 +337,7 @@ static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector,
 		vals = &func->vals[i];
 		val = pcs->read(vals->reg);
 		val &= ~pcs->fmask;
-		val |= vals->val;
+		val |= (vals->val & pcs->fmask);
 		pcs->write(val, vals->reg);
 	}
 
-- 
1.7.12

Re: [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [120905 02:02]:
> Use the pcs->fmask to make sure that the value is not changing (setting)
> bits in areas where it should not.
> To avoid situations like this:
> 
> pmx_dummy: pinmux@4a100040 {
> 	compatible = "pinctrl-single";
> 	reg = <0x4a100040 0x0196>;
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	pinctrl-single,register-width = <16>;
> 	pinctrl-single,function-mask = <0x00ff>;
> };
> 
> &pmx_dummy {
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&board_pins>;
> 
> 	board_pins: pinmux_board_pins {
> 		pinctrl-single,pins = <
> 			0x6c 0xf0f
> 			0x6e 0x10f
> 			0x70 0x23f
> 			0x72 0xa5f
> 		>;
> 	};
> };
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Thanks this is a valid fix:

Acked-by: Tony Lindgren <tony@atomide.com>

> ---
>  drivers/pinctrl/pinctrl-single.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index 76a4260..3508631 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -337,7 +337,7 @@ static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector,
>  		vals = &func->vals[i];
>  		val = pcs->read(vals->reg);
>  		val &= ~pcs->fmask;
> -		val |= vals->val;
> +		val |= (vals->val & pcs->fmask);
>  		pcs->write(val, vals->reg);
>  	}
>  
> -- 
> 1.7.12
> 

Re: [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask

[Linus Walleij]

On Thu, Sep 6, 2012 at 8:59 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [120905 02:02]:
>> Use the pcs->fmask to make sure that the value is not changing (setting)
>> bits in areas where it should not.
>> To avoid situations like this:
>>
>> pmx_dummy: pinmux@4a100040 {
>>       compatible = "pinctrl-single";
>>       reg = <0x4a100040 0x0196>;
>>       #address-cells = <1>;
>>       #size-cells = <0>;
>>       pinctrl-single,register-width = <16>;
>>       pinctrl-single,function-mask = <0x00ff>;
>> };
>>
>> &pmx_dummy {
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&board_pins>;
>>
>>       board_pins: pinmux_board_pins {
>>               pinctrl-single,pins = <
>>                       0x6c 0xf0f
>>                       0x6e 0x10f
>>                       0x70 0x23f
>>                       0x72 0xa5f
>>               >;
>>       };
>> };
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>
> Thanks this is a valid fix:
>
> Acked-by: Tony Lindgren <tony@atomide.com>

Since nothing in v3.6 is using pinctrl-simple yet, it's not a regression
right?

So can you just group this with the other pinctrl things you are
harvesting in the OMAP tree? I was going to push it for
the v3.7 cycle otherwise.

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [120907 14:13]:
> On Thu, Sep 6, 2012 at 8:59 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Peter Ujfalusi <peter.ujfalusi@ti.com> [120905 02:02]:
> >> Use the pcs->fmask to make sure that the value is not changing (setting)
> >> bits in areas where it should not.
> >> To avoid situations like this:
> >>
> >> pmx_dummy: pinmux@4a100040 {
> >>       compatible = "pinctrl-single";
> >>       reg = <0x4a100040 0x0196>;
> >>       #address-cells = <1>;
> >>       #size-cells = <0>;
> >>       pinctrl-single,register-width = <16>;
> >>       pinctrl-single,function-mask = <0x00ff>;
> >> };
> >>
> >> &pmx_dummy {
> >>       pinctrl-names = "default";
> >>       pinctrl-0 = <&board_pins>;
> >>
> >>       board_pins: pinmux_board_pins {
> >>               pinctrl-single,pins = <
> >>                       0x6c 0xf0f
> >>                       0x6e 0x10f
> >>                       0x70 0x23f
> >>                       0x72 0xa5f
> >>               >;
> >>       };
> >> };
> >>
> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> >
> > Thanks this is a valid fix:
> >
> > Acked-by: Tony Lindgren <tony@atomide.com>
> 
> Since nothing in v3.6 is using pinctrl-simple yet, it's not a regression
> right?

Right.
 
> So can you just group this with the other pinctrl things you are
> harvesting in the OMAP tree? I was going to push it for
> the v3.7 cycle otherwise.

You can take this for v3.7, the changes I have are just adding .dts
entries to use the driver. The driver related changes are being
merged by the related driver lists.

Regards,

Tony

Re: [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask

[Linus Walleij]

On Wed, Sep 5, 2012 at 11:01 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Applied with Tony's ACK.

Yours,
Linus Walleij

[PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux

[Peter Ujfalusi]

With pinctrl-single,bits it is possible to update just part of the register
within the pinctrl-single,function-mask area.
This is useful when one register configures mmore than one pin's mux.

pinctrl-single,bits takes three parameters:
<reg offset, value, sub-mask>

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 .../devicetree/bindings/pinctrl/pinctrl-single.txt |  9 +++++
 drivers/pinctrl/pinctrl-single.c                   | 42 ++++++++++++++++------
 2 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
index 5187f0d..287801d 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
@@ -31,6 +31,15 @@ device pinctrl register, and 0x118 contains the desired value of the
 pinctrl register. See the device example and static board pins example
 below for more information.
 
+In case when one register changes more than one pin's mux the
+pinctrl-single,bits can be used which takes three parameters:
+
+	pinctrl-single,bits = <0xdc 0x18, 0xff>;
+
+Where 0xdc is the offset from the pinctrl register base address for the
+device pinctrl register, 0x18 is the desired value, and 0xff is the sub mask to
+be used when applying this change to the register.
+
 Example:
 
 /* SoC common file */
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 3508631..aec338e 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -26,7 +26,8 @@
 #include "core.h"
 
 #define DRIVER_NAME			"pinctrl-single"
-#define PCS_MUX_NAME			"pinctrl-single,pins"
+#define PCS_MUX_PINS_NAME		"pinctrl-single,pins"
+#define PCS_MUX_BITS_NAME		"pinctrl-single,bits"
 #define PCS_REG_NAME_LEN		((sizeof(unsigned long) * 2) + 1)
 #define PCS_OFF_DISABLED		~0U
 
@@ -54,6 +55,7 @@ struct pcs_pingroup {
 struct pcs_func_vals {
 	void __iomem *reg;
 	unsigned val;
+	unsigned mask;
 };
 
 /**
@@ -332,12 +334,17 @@ static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector,
 
 	for (i = 0; i < func->nvals; i++) {
 		struct pcs_func_vals *vals;
-		unsigned val;
+		unsigned val, mask;
 
 		vals = &func->vals[i];
 		val = pcs->read(vals->reg);
-		val &= ~pcs->fmask;
-		val |= (vals->val & pcs->fmask);
+		if (!vals->mask)
+			mask = pcs->fmask;
+		else
+			mask = pcs->fmask & vals->mask;
+
+		val &= ~mask;
+		val |= (vals->val & mask);
 		pcs->write(val, vals->reg);
 	}
 
@@ -657,18 +664,29 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 {
 	struct pcs_func_vals *vals;
 	const __be32 *mux;
-	int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
+	int size, params, rows, *pins, index = 0, found = 0, res = -ENOMEM;
 	struct pcs_function *function;
 
-	mux = of_get_property(np, PCS_MUX_NAME, &size);
-	if ((!mux) || (size < sizeof(*mux) * 2)) {
-		dev_err(pcs->dev, "bad data for mux %s\n",
-			np->name);
+	mux = of_get_property(np, PCS_MUX_PINS_NAME, &size);
+	if (mux) {
+		params = 2;
+	} else {
+		mux = of_get_property(np, PCS_MUX_BITS_NAME, &size);
+		if (!mux) {
+			dev_err(pcs->dev, "no valid property for %s\n",
+				np->name);
+			return -EINVAL;
+		}
+		params = 3;
+	}
+
+	if (size < (sizeof(*mux) * params)) {
+		dev_err(pcs->dev, "bad data for %s\n", np->name);
 		return -EINVAL;
 	}
 
 	size /= sizeof(*mux);	/* Number of elements in array */
-	rows = size / 2;	/* Each row is a key value pair */
+	rows = size / params;	/* Each row is a key value pair */
 
 	vals = devm_kzalloc(pcs->dev, sizeof(*vals) * rows, GFP_KERNEL);
 	if (!vals)
@@ -686,6 +704,10 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 		val = be32_to_cpup(mux + index++);
 		vals[found].reg = pcs->base + offset;
 		vals[found].val = val;
+		if (params == 3) {
+			val = be32_to_cpup(mux + index++);
+			vals[found].mask = val;
+		}
 
 		pin = pcs_get_pin_by_offset(pcs, offset);
 		if (pin < 0) {
-- 
1.7.12

Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux

[Tony Lindgren]

Hi Peter,

* Peter Ujfalusi <peter.ujfalusi@ti.com> [120905 02:02]:
> With pinctrl-single,bits it is possible to update just part of the register
> within the pinctrl-single,function-mask area.
> This is useful when one register configures mmore than one pin's mux.

You have a typo here:                         ^^^^^
 
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> @@ -31,6 +31,15 @@ device pinctrl register, and 0x118 contains the desired value of the
>  pinctrl register. See the device example and static board pins example
>  below for more information.
>  
> +In case when one register changes more than one pin's mux the
> +pinctrl-single,bits can be used which takes three parameters:
> +
> +	pinctrl-single,bits = <0xdc 0x18, 0xff>;
> +
> +Where 0xdc is the offset from the pinctrl register base address for the
> +device pinctrl register, 0x18 is the desired value, and 0xff is the sub mask to
> +be used when applying this change to the register.
> +

Is it now safe to assume that we always have width of three if
pinctrl-single,bits is specified? The reason I'm asking is..

> @@ -657,18 +664,29 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>  {
>  	struct pcs_func_vals *vals;
>  	const __be32 *mux;
> -	int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
> +	int size, params, rows, *pins, index = 0, found = 0, res = -ENOMEM;
>  	struct pcs_function *function;
>  
> -	mux = of_get_property(np, PCS_MUX_NAME, &size);
> -	if ((!mux) || (size < sizeof(*mux) * 2)) {
> -		dev_err(pcs->dev, "bad data for mux %s\n",
> -			np->name);
> +	mux = of_get_property(np, PCS_MUX_PINS_NAME, &size);
> +	if (mux) {
> +		params = 2;
> +	} else {
> +		mux = of_get_property(np, PCS_MUX_BITS_NAME, &size);
> +		if (!mux) {
> +			dev_err(pcs->dev, "no valid property for %s\n",
> +				np->name);
> +			return -EINVAL;
> +		}
> +		params = 3;
> +	}

..because here we could assume the default value for params is 2
if pinctrl-single,pins is specified, and otherwise params is 3
if pinctrl-single,bits is specified for the controller. That would
avoid querying a potentially non-exiting property for each entry.

> @@ -686,6 +704,10 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>  		val = be32_to_cpup(mux + index++);
>  		vals[found].reg = pcs->base + offset;
>  		vals[found].val = val;
> +		if (params == 3) {
> +			val = be32_to_cpup(mux + index++);
> +			vals[found].mask = val;
> +		}
>  
>  		pin = pcs_get_pin_by_offset(pcs, offset);
>  		if (pin < 0) {

Here params too would be then set during probe already.

Other than that, seems to still work for me for my test cases.

Regards,

Tony

Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux

[Peter Ujfalusi]

Hi Tony,

On 09/06/2012 10:10 PM, Tony Lindgren wrote:
> Hi Peter,
> 
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [120905 02:02]:
>> With pinctrl-single,bits it is possible to update just part of the register
>> within the pinctrl-single,function-mask area.
>> This is useful when one register configures mmore than one pin's mux.
> 
> You have a typo here:                         ^^^^^

Oh, I'll fix this up.

>> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
>> @@ -31,6 +31,15 @@ device pinctrl register, and 0x118 contains the desired value of the
>>  pinctrl register. See the device example and static board pins example
>>  below for more information.
>>  
>> +In case when one register changes more than one pin's mux the
>> +pinctrl-single,bits can be used which takes three parameters:
>> +
>> +	pinctrl-single,bits = <0xdc 0x18, 0xff>;
>> +
>> +Where 0xdc is the offset from the pinctrl register base address for the
>> +device pinctrl register, 0x18 is the desired value, and 0xff is the sub mask to
>> +be used when applying this change to the register.
>> +
> 
> Is it now safe to assume that we always have width of three if
> pinctrl-single,bits is specified? The reason I'm asking is..
> 
>> @@ -657,18 +664,29 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>>  {
>>  	struct pcs_func_vals *vals;
>>  	const __be32 *mux;
>> -	int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
>> +	int size, params, rows, *pins, index = 0, found = 0, res = -ENOMEM;
>>  	struct pcs_function *function;
>>  
>> -	mux = of_get_property(np, PCS_MUX_NAME, &size);
>> -	if ((!mux) || (size < sizeof(*mux) * 2)) {
>> -		dev_err(pcs->dev, "bad data for mux %s\n",
>> -			np->name);
>> +	mux = of_get_property(np, PCS_MUX_PINS_NAME, &size);
>> +	if (mux) {
>> +		params = 2;
>> +	} else {
>> +		mux = of_get_property(np, PCS_MUX_BITS_NAME, &size);
>> +		if (!mux) {
>> +			dev_err(pcs->dev, "no valid property for %s\n",
>> +				np->name);
>> +			return -EINVAL;
>> +		}
>> +		params = 3;
>> +	}
> 
> ..because here we could assume the default value for params is 2
> if pinctrl-single,pins is specified, and otherwise params is 3
> if pinctrl-single,bits is specified for the controller. That would
> avoid querying a potentially non-exiting property for each entry.
> 
>> @@ -686,6 +704,10 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>>  		val = be32_to_cpup(mux + index++);
>>  		vals[found].reg = pcs->base + offset;
>>  		vals[found].val = val;
>> +		if (params == 3) {
>> +			val = be32_to_cpup(mux + index++);
>> +			vals[found].mask = val;
>> +		}
>>  
>>  		pin = pcs_get_pin_by_offset(pcs, offset);
>>  		if (pin < 0) {
> 
> Here params too would be then set during probe already.

I'm afraid you lost me here...
We only know if the user specified the mux configuration with
pinctrl-single,pins or  pinctrl-single,bits in this function.

One thing I could do to make the code a bit better to look at is:
int params = 2;

mux = of_get_property(np, PCS_MUX_PINS_NAME, &size);
if (!mux) {
	mux = of_get_property(np, PCS_MUX_BITS_NAME, &size);
	if (!mux) {
		dev_err(pcs->dev, "no valid property for %s\n",
		np->name);
		return -EINVAL;
	}
	params = 3;
}

This might make the code a bit more compact but at the same time one might
need to spend few more seconds to understand it.

Regards,
Péter

Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [120907 08:13]:
> On 09/06/2012 10:10 PM, Tony Lindgren wrote:
> > 
> > Is it now safe to assume that we always have width of three if
> > pinctrl-single,bits is specified? The reason I'm asking is..
> > 
> >> @@ -657,18 +664,29 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
> >>  {
> >>  	struct pcs_func_vals *vals;
> >>  	const __be32 *mux;
> >> -	int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
> >> +	int size, params, rows, *pins, index = 0, found = 0, res = -ENOMEM;
> >>  	struct pcs_function *function;
> >>  
> >> -	mux = of_get_property(np, PCS_MUX_NAME, &size);
> >> -	if ((!mux) || (size < sizeof(*mux) * 2)) {
> >> -		dev_err(pcs->dev, "bad data for mux %s\n",
> >> -			np->name);
> >> +	mux = of_get_property(np, PCS_MUX_PINS_NAME, &size);
> >> +	if (mux) {
> >> +		params = 2;
> >> +	} else {
> >> +		mux = of_get_property(np, PCS_MUX_BITS_NAME, &size);
> >> +		if (!mux) {
> >> +			dev_err(pcs->dev, "no valid property for %s\n",
> >> +				np->name);
> >> +			return -EINVAL;
> >> +		}
> >> +		params = 3;
> >> +	}
> > 
> > ..because here we could assume the default value for params is 2
> > if pinctrl-single,pins is specified, and otherwise params is 3
> > if pinctrl-single,bits is specified for the controller. That would
> > avoid querying a potentially non-exiting property for each entry.
> > 
> >> @@ -686,6 +704,10 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
> >>  		val = be32_to_cpup(mux + index++);
> >>  		vals[found].reg = pcs->base + offset;
> >>  		vals[found].val = val;
> >> +		if (params == 3) {
> >> +			val = be32_to_cpup(mux + index++);
> >> +			vals[found].mask = val;
> >> +		}
> >>  
> >>  		pin = pcs_get_pin_by_offset(pcs, offset);
> >>  		if (pin < 0) {
> > 
> > Here params too would be then set during probe already.
> 
> I'm afraid you lost me here...
> We only know if the user specified the mux configuration with
> pinctrl-single,pins or  pinctrl-single,bits in this function.

Sorry right, yeah we don't know that at probe time currently.

I'd like to have something that specifies the controller type so
we don't need to mix two types of controllers and test for
non-existing properties when parsing the pins.

How about we require something specified for the pinctrl driver
in the "one-bit-per-mux" case like pinctrl-single,bit-per-mux?

And then in the pinctrl-single probe we set params = 3 if
pinctrl-single,bit-per-mux is specified. And if no
pinctrl-single,bit-per-mux is specified then set params = 2.

That way pcs_parse_one_pinctrl_entry() can just test for params.

Sorry I don't have a better name in mind than bit-per-mux..
 
> One thing I could do to make the code a bit better to look at is:
> int params = 2;
> 
> mux = of_get_property(np, PCS_MUX_PINS_NAME, &size);
> if (!mux) {
> 	mux = of_get_property(np, PCS_MUX_BITS_NAME, &size);
> 	if (!mux) {
> 		dev_err(pcs->dev, "no valid property for %s\n",
> 		np->name);
> 		return -EINVAL;
> 	}
> 	params = 3;
> }
> 
> This might make the code a bit more compact but at the same time one might
> need to spend few more seconds to understand it.

Yes well there's no need to do of_get_property second guessing
if we already know params from the pinctrl-single.c probe time.

I think we're safe to assume that we don't need to mix parsing
two different types of configuration for the same controller
as they can always be set up as separate controllers.

Regards,

Tony

Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux

[Peter Ujfalusi]

On 09/07/2012 07:55 PM, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [120907 08:13]:
>> On 09/06/2012 10:10 PM, Tony Lindgren wrote:
>>>
>>> Is it now safe to assume that we always have width of three if
>>> pinctrl-single,bits is specified? The reason I'm asking is..
>>>
>>>> @@ -657,18 +664,29 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>>>>  {
>>>>  	struct pcs_func_vals *vals;
>>>>  	const __be32 *mux;
>>>> -	int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
>>>> +	int size, params, rows, *pins, index = 0, found = 0, res = -ENOMEM;
>>>>  	struct pcs_function *function;
>>>>  
>>>> -	mux = of_get_property(np, PCS_MUX_NAME, &size);
>>>> -	if ((!mux) || (size < sizeof(*mux) * 2)) {
>>>> -		dev_err(pcs->dev, "bad data for mux %s\n",
>>>> -			np->name);
>>>> +	mux = of_get_property(np, PCS_MUX_PINS_NAME, &size);
>>>> +	if (mux) {
>>>> +		params = 2;
>>>> +	} else {
>>>> +		mux = of_get_property(np, PCS_MUX_BITS_NAME, &size);
>>>> +		if (!mux) {
>>>> +			dev_err(pcs->dev, "no valid property for %s\n",
>>>> +				np->name);
>>>> +			return -EINVAL;
>>>> +		}
>>>> +		params = 3;
>>>> +	}
>>>
>>> ..because here we could assume the default value for params is 2
>>> if pinctrl-single,pins is specified, and otherwise params is 3
>>> if pinctrl-single,bits is specified for the controller. That would
>>> avoid querying a potentially non-exiting property for each entry.
>>>
>>>> @@ -686,6 +704,10 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>>>>  		val = be32_to_cpup(mux + index++);
>>>>  		vals[found].reg = pcs->base + offset;
>>>>  		vals[found].val = val;
>>>> +		if (params == 3) {
>>>> +			val = be32_to_cpup(mux + index++);
>>>> +			vals[found].mask = val;
>>>> +		}
>>>>  
>>>>  		pin = pcs_get_pin_by_offset(pcs, offset);
>>>>  		if (pin < 0) {
>>>
>>> Here params too would be then set during probe already.
>>
>> I'm afraid you lost me here...
>> We only know if the user specified the mux configuration with
>> pinctrl-single,pins or  pinctrl-single,bits in this function.
> 
> Sorry right, yeah we don't know that at probe time currently.
> 
> I'd like to have something that specifies the controller type so
> we don't need to mix two types of controllers and test for
> non-existing properties when parsing the pins.
> 
> How about we require something specified for the pinctrl driver
> in the "one-bit-per-mux" case like pinctrl-single,bit-per-mux?
> 
> And then in the pinctrl-single probe we set params = 3 if
> pinctrl-single,bit-per-mux is specified. And if no
> pinctrl-single,bit-per-mux is specified then set params = 2.
> 
> That way pcs_parse_one_pinctrl_entry() can just test for params.
> 
> Sorry I don't have a better name in mind than bit-per-mux..

I'm not sure if this would make things more prone to error or make the DTS or
the code more clean in any ways.

Both pinctrl-single,pins and pinctrl-single,bits works on top of the same
pinctrl-single area.
In most cases we are going to use pinctrl-single,pins for the mux
configuration and only in few places we need to fall back to pinctrl-single,bits.

With this patch we will have maximum of two query to find out which type is
used, while if we add the 'bit-per-mux' property we will always have need to
have two of_get_property() call to figure out what is used.
IMHO in this way we could also have copy-paste errors coming at us when adding
the mux configurations for the pins and at the end we also need to do same
safety/sanity checks if the user really provided the correct type with
correlation to the 'bit-per-mux'.

This would just complicate the code.
The existence of pinctrl-single,pins or pinctrl-single,bits property alone
gives enough information for the number of parameters.

>  
>> One thing I could do to make the code a bit better to look at is:
>> int params = 2;
>>
>> mux = of_get_property(np, PCS_MUX_PINS_NAME, &size);
>> if (!mux) {
>> 	mux = of_get_property(np, PCS_MUX_BITS_NAME, &size);
>> 	if (!mux) {
>> 		dev_err(pcs->dev, "no valid property for %s\n",
>> 		np->name);
>> 		return -EINVAL;
>> 	}
>> 	params = 3;
>> }
>>
>> This might make the code a bit more compact but at the same time one might
>> need to spend few more seconds to understand it.
> 
> Yes well there's no need to do of_get_property second guessing
> if we already know params from the pinctrl-single.c probe time.
> 
> I think we're safe to assume that we don't need to mix parsing
> two different types of configuration for the same controller
> as they can always be set up as separate controllers.
> 
> Regards,
> 
> Tony
> 


-- 
Péter

Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [120910 04:55]:
> On 09/07/2012 07:55 PM, Tony Lindgren wrote:
> > 
> > I'd like to have something that specifies the controller type so
> > we don't need to mix two types of controllers and test for
> > non-existing properties when parsing the pins.
> > 
> > How about we require something specified for the pinctrl driver
> > in the "one-bit-per-mux" case like pinctrl-single,bit-per-mux?
> > 
> > And then in the pinctrl-single probe we set params = 3 if
> > pinctrl-single,bit-per-mux is specified. And if no
> > pinctrl-single,bit-per-mux is specified then set params = 2.
> > 
> > That way pcs_parse_one_pinctrl_entry() can just test for params.
> > 
> > Sorry I don't have a better name in mind than bit-per-mux..
> 
> I'm not sure if this would make things more prone to error or make the DTS or
> the code more clean in any ways.
> 
> Both pinctrl-single,pins and pinctrl-single,bits works on top of the same
> pinctrl-single area.
> In most cases we are going to use pinctrl-single,pins for the mux
> configuration and only in few places we need to fall back to pinctrl-single,bits.

What about hardware that has tons of one-bit-per-mux type of registers?
Then we end up trying to find the non-existing property potentially
hundreds of times as the pinctrl-single,pins is never specified.
 
> With this patch we will have maximum of two query to find out which type is
> used, while if we add the 'bit-per-mux' property we will always have need to
> have two of_get_property() call to figure out what is used.
> IMHO in this way we could also have copy-paste errors coming at us when adding
> the mux configurations for the pins and at the end we also need to do same
> safety/sanity checks if the user really provided the correct type with
> correlation to the 'bit-per-mux'.

Well I think we should specify the binding for the type of the controller,
not mix different types of bindings for the same controller. So we should
probably have something just like address-cells, gpio-cells and
interrupt-cells, although in this case it would be specific to pinctrl-single
only and not be called cells.

> This would just complicate the code.
> The existence of pinctrl-single,pins or pinctrl-single,bits property alone
> gives enough information for the number of parameters.

Yes but we don't want to allow mixing different kind of registers within
the same controller, they should be specified as separate controllers.

Regards,

Tony

Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux

[Linus Walleij]

On Wed, Sep 5, 2012 at 11:01 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> With pinctrl-single,bits it is possible to update just part of the register
> within the pinctrl-single,function-mask area.
> This is useful when one register configures mmore than one pin's mux.
>
> pinctrl-single,bits takes three parameters:
> <reg offset, value, sub-mask>
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Do we have a conclusion on this patch 2/2?

I'm holding it back until Tony explicitly ACKs it for now.

Yours,
Linus Walleij

Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [120910 00:11]:
> On Wed, Sep 5, 2012 at 11:01 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> 
> > With pinctrl-single,bits it is possible to update just part of the register
> > within the pinctrl-single,function-mask area.
> > This is useful when one register configures mmore than one pin's mux.
> >
> > pinctrl-single,bits takes three parameters:
> > <reg offset, value, sub-mask>
> >
> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> Do we have a conclusion on this patch 2/2?
> 
> I'm holding it back until Tony explicitly ACKs it for now.

I don't like the idea of mixing different types of controllers
here, so let's wait a little bit on 2/2.

Tony

Re: [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits

[Linus Walleij]

On Wed, Sep 5, 2012 at 11:01 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> When configuring pinmux with pinctrl-single there could be a case when one
> register is used to configure mux for more than one pin.
> In this case the use of pinctrl-single,pins is a bit problematic since we can
> only update the whole register (restricted by the mask).
> In such a situations the pinctrl-single,bits could provide a safe way to handle
> the mux.
>
> pinctrl-single,bits takes three parameters: <reg offset, value, sub-mask>
> The sub mask is used to mask part of the register to make sure we do not change
> bits outside of the scope of this pin.
>
> The first patch in this series is to fix the previous pinctrl-since,pins
> implementation because it was not using the mask on the value which could result
> changed bits outside of the mask.

This looks sane to me, but I'd like Tony to ACK before I apply it.

Yours,
Linus Walleij

Re: [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [120905 05:11]:
> On Wed, Sep 5, 2012 at 11:01 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> 
> > When configuring pinmux with pinctrl-single there could be a case when one
> > register is used to configure mux for more than one pin.
> > In this case the use of pinctrl-single,pins is a bit problematic since we can
> > only update the whole register (restricted by the mask).
> > In such a situations the pinctrl-single,bits could provide a safe way to handle
> > the mux.
> >
> > pinctrl-single,bits takes three parameters: <reg offset, value, sub-mask>
> > The sub mask is used to mask part of the register to make sure we do not change
> > bits outside of the scope of this pin.
> >
> > The first patch in this series is to fix the previous pinctrl-since,pins
> > implementation because it was not using the mask on the value which could result
> > changed bits outside of the mask.
> 
> This looks sane to me, but I'd like Tony to ACK before I apply it.

Cool, this should allow handling cases where some pinctrl devices can have
extra aux registers for signal strength etc for some of the pins.

Will take a look.

Tony