[PATCH 0/2] pinctrl: sx150x: set multiple pins at once

[PATCH 0/2] pinctrl: sx150x: set multiple pins at once

[Peter Rosin]

Hi!

I have only tested this on an 8-bit sx1502, so I'm uncertain if
the there needs to be locking for this to work as intended for
the bigger chips with an oscio pin? Probably.

So, I didn't add (or rather, removed) these lines at the end of
sx150x_gpio_set_multiple() and made the op optional instead.

	if (*mask & pctl->oscio_mask)
		sx150x_gpio_oscio_set(pctl, *bits & pctl->oscio_mask);

I mean, what happens if there are two users writing multiple pins
where one of the pins is the oscio pin, and this happens concurrently?
I get the feeling that there is nothing stopping interleaving in that
case, and you could end up with the desired values from user 1 for the
normal pins, and the desired value for the oscio pin from user 2.

But for the easy case (no oscio) where the existing regmap locking
holds, this is a nice speedup and desired behaviour without a lot
of individual pin changes.

Cheers,
Peter

Peter Rosin (2):
  pinctrl: sx150x: various spelling fixes and some white-space cleanup
  pinctrl: sx150x: support setting multiple pins at once

 drivers/pinctrl/pinctrl-sx150x.c | 50 ++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 15 deletions(-)

-- 
2.1.4

[PATCH 1/2] pinctrl: sx150x: various spelling fixes and some white-space cleanup

[Peter Rosin]

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/pinctrl/pinctrl-sx150x.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index 63778058eec7..ef4ef88e0ee9 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -384,7 +384,7 @@ static int sx150x_gpio_oscio_set(struct sx150x_pinctrl *pctl,
 }
 
 static void sx150x_gpio_set(struct gpio_chip *chip, unsigned int offset,
-			       int value)
+			    int value)
 {
 	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
 
@@ -396,7 +396,7 @@ static void sx150x_gpio_set(struct gpio_chip *chip, unsigned int offset,
 }
 
 static int sx150x_gpio_direction_input(struct gpio_chip *chip,
-				      unsigned int offset)
+				       unsigned int offset)
 {
 	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
 
@@ -409,7 +409,7 @@ static int sx150x_gpio_direction_input(struct gpio_chip *chip,
 }
 
 static int sx150x_gpio_direction_output(struct gpio_chip *chip,
-				       unsigned int offset, int value)
+					unsigned int offset, int value)
 {
 	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
 	int ret;
@@ -880,7 +880,7 @@ static unsigned int sx150x_maybe_swizzle(struct sx150x_pinctrl *pctl,
 	 *	reg + 3 [ 3 3 2 2 1 1 0 0 ]
 	 *
 	 * SX1503 and SX1506 deviate from that data layout, instead storing
-	 * thier contents as follows:
+	 * their contents as follows:
 	 *
 	 *	reg     [ f f e e d d c c ]
 	 *	reg + 1 [ 7 7 6 6 5 5 4 4 ]
@@ -915,9 +915,8 @@ static unsigned int sx150x_maybe_swizzle(struct sx150x_pinctrl *pctl,
  *
  * This way the rest of the driver code, interfacing with the chip via
  * regmap API, can work assuming that each GPIO pin is represented by
- * a group of bits at an offset proportioan to GPIO number within a
+ * a group of bits at an offset proportional to GPIO number within a
  * given register.
- *
  */
 static int sx150x_regmap_reg_read(void *context, unsigned int reg,
 				  unsigned int *result)
@@ -929,7 +928,7 @@ static int sx150x_regmap_reg_read(void *context, unsigned int reg,
 	unsigned int idx, val;
 
 	/*
-	 * There are four potential cases coverd by this function:
+	 * There are four potential cases covered by this function:
 	 *
 	 * 1) 8-pin chip, single configuration bit register
 	 *
-- 
2.1.4

[PATCH 2/2] pinctrl: sx150x: support setting multiple pins at once

[Peter Rosin]

The mask of a possible oscio pin is cached, making it easier to test
for the exception.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/pinctrl/pinctrl-sx150x.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index ef4ef88e0ee9..5bcede2b2cd8 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -114,6 +114,7 @@ struct sx150x_pinctrl {
 	} irq;
 	struct mutex lock;
 	const struct sx150x_device_data *data;
+	unsigned long oscio_mask;
 };
 
 static const struct pinctrl_pin_desc sx150x_8_pins[] = {
@@ -290,14 +291,7 @@ static const struct pinctrl_ops sx150x_pinctrl_ops = {
 
 static bool sx150x_pin_is_oscio(struct sx150x_pinctrl *pctl, unsigned int pin)
 {
-	if (pin >= pctl->data->npins)
-		return false;
-
-	/* OSCIO pin is only present in 789 devices */
-	if (pctl->data->model != SX150X_789)
-		return false;
-
-	return !strcmp(pctl->data->pins[pin].name, "oscio");
+	return !!(BIT(pin) & pctl->oscio_mask);
 }
 
 static int sx150x_gpio_get_direction(struct gpio_chip *chip,
@@ -395,6 +389,15 @@ static void sx150x_gpio_set(struct gpio_chip *chip, unsigned int offset,
 
 }
 
+static void sx150x_gpio_set_multiple(struct gpio_chip *chip,
+				     unsigned long *mask,
+				     unsigned long *bits)
+{
+	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
+
+	regmap_write_bits(pctl->regmap, pctl->data->reg_data, *mask, *bits);
+}
+
 static int sx150x_gpio_direction_input(struct gpio_chip *chip,
 				       unsigned int offset)
 {
@@ -996,6 +999,20 @@ static int sx150x_regmap_reg_write(void *context, unsigned int reg,
 	return 0;
 }
 
+static void sx150x_oscio_mask_init(struct sx150x_pinctrl *pctl)
+{
+	int pin;
+
+	/* OSCIO pin is only present in 789 devices */
+	if (pctl->data->model != SX150X_789)
+		return;
+
+	for (pin = 0; pin < pctl->data->npins; ++pin) {
+		if (!strcmp(pctl->data->pins[pin].name, "oscio"))
+			pctl->oscio_mask |= BIT(pin);
+	}
+}
+
 static bool sx150x_reg_volatile(struct device *dev, unsigned int reg)
 {
 	struct sx150x_pinctrl *pctl = i2c_get_clientdata(to_i2c_client(dev));
@@ -1045,6 +1062,8 @@ static int sx150x_probe(struct i2c_client *client,
 	if (!pctl->data)
 		return -EINVAL;
 
+	sx150x_oscio_mask_init(pctl);
+
 	pctl->regmap = devm_regmap_init(dev, NULL, pctl,
 					&sx150x_regmap_config);
 	if (IS_ERR(pctl->regmap)) {
@@ -1069,6 +1088,8 @@ static int sx150x_probe(struct i2c_client *client,
 	pctl->gpio.direction_output = sx150x_gpio_direction_output;
 	pctl->gpio.get = sx150x_gpio_get;
 	pctl->gpio.set = sx150x_gpio_set;
+	if (!pctl->oscio_mask)
+		pctl->gpio.set_multiple = sx150x_gpio_set_multiple;
 	pctl->gpio.set_single_ended = sx150x_gpio_set_single_ended;
 	pctl->gpio.parent = dev;
 #ifdef CONFIG_OF_GPIO
-- 
2.1.4

Re: [PATCH 1/2] pinctrl: sx150x: various spelling fixes and some white-space cleanup

[Neil Armstrong]

On 11/22/2016 05:06 PM, Peter Rosin wrote:
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/pinctrl/pinctrl-sx150x.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
> index 63778058eec7..ef4ef88e0ee9 100644
> --- a/drivers/pinctrl/pinctrl-sx150x.c
> +++ b/drivers/pinctrl/pinctrl-sx150x.c
> @@ -384,7 +384,7 @@ static int sx150x_gpio_oscio_set(struct sx150x_pinctrl *pctl,
>  }
>  
>  static void sx150x_gpio_set(struct gpio_chip *chip, unsigned int offset,
> -			       int value)
> +			    int value)
>  {
>  	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
>  
> @@ -396,7 +396,7 @@ static void sx150x_gpio_set(struct gpio_chip *chip, unsigned int offset,
>  }
>  
>  static int sx150x_gpio_direction_input(struct gpio_chip *chip,
> -				      unsigned int offset)
> +				       unsigned int offset)
>  {
>  	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
>  
> @@ -409,7 +409,7 @@ static int sx150x_gpio_direction_input(struct gpio_chip *chip,
>  }
>  
>  static int sx150x_gpio_direction_output(struct gpio_chip *chip,
> -				       unsigned int offset, int value)
> +					unsigned int offset, int value)
>  {
>  	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
>  	int ret;
> @@ -880,7 +880,7 @@ static unsigned int sx150x_maybe_swizzle(struct sx150x_pinctrl *pctl,
>  	 *	reg + 3 [ 3 3 2 2 1 1 0 0 ]
>  	 *
>  	 * SX1503 and SX1506 deviate from that data layout, instead storing
> -	 * thier contents as follows:
> +	 * their contents as follows:
>  	 *
>  	 *	reg     [ f f e e d d c c ]
>  	 *	reg + 1 [ 7 7 6 6 5 5 4 4 ]
> @@ -915,9 +915,8 @@ static unsigned int sx150x_maybe_swizzle(struct sx150x_pinctrl *pctl,
>   *
>   * This way the rest of the driver code, interfacing with the chip via
>   * regmap API, can work assuming that each GPIO pin is represented by
> - * a group of bits at an offset proportioan to GPIO number within a
> + * a group of bits at an offset proportional to GPIO number within a
>   * given register.
> - *
>   */
>  static int sx150x_regmap_reg_read(void *context, unsigned int reg,
>  				  unsigned int *result)
> @@ -929,7 +928,7 @@ static int sx150x_regmap_reg_read(void *context, unsigned int reg,
>  	unsigned int idx, val;
>  
>  	/*
> -	 * There are four potential cases coverd by this function:
> +	 * There are four potential cases covered by this function:
>  	 *
>  	 * 1) 8-pin chip, single configuration bit register
>  	 *
> 

Acked-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 2/2] pinctrl: sx150x: support setting multiple pins at once

[Neil Armstrong]

On 11/22/2016 05:06 PM, Peter Rosin wrote:
> The mask of a possible oscio pin is cached, making it easier to test
> for the exception.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/pinctrl/pinctrl-sx150x.c | 37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
> index ef4ef88e0ee9..5bcede2b2cd8 100644
> --- a/drivers/pinctrl/pinctrl-sx150x.c
> +++ b/drivers/pinctrl/pinctrl-sx150x.c
> @@ -114,6 +114,7 @@ struct sx150x_pinctrl {
>  	} irq;
>  	struct mutex lock;
>  	const struct sx150x_device_data *data;
> +	unsigned long oscio_mask;
>  };
>  
>  static const struct pinctrl_pin_desc sx150x_8_pins[] = {
> @@ -290,14 +291,7 @@ static const struct pinctrl_ops sx150x_pinctrl_ops = {
>  
>  static bool sx150x_pin_is_oscio(struct sx150x_pinctrl *pctl, unsigned int pin)
>  {
> -	if (pin >= pctl->data->npins)
> -		return false;
> -
> -	/* OSCIO pin is only present in 789 devices */
> -	if (pctl->data->model != SX150X_789)
> -		return false;
> -
> -	return !strcmp(pctl->data->pins[pin].name, "oscio");
> +	return !!(BIT(pin) & pctl->oscio_mask);
>  }
>  
>  static int sx150x_gpio_get_direction(struct gpio_chip *chip,
> @@ -395,6 +389,15 @@ static void sx150x_gpio_set(struct gpio_chip *chip, unsigned int offset,
>  
>  }
>  
> +static void sx150x_gpio_set_multiple(struct gpio_chip *chip,
> +				     unsigned long *mask,
> +				     unsigned long *bits)
> +{
> +	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
> +
> +	regmap_write_bits(pctl->regmap, pctl->data->reg_data, *mask, *bits);
> +}
> +
>  static int sx150x_gpio_direction_input(struct gpio_chip *chip,
>  				       unsigned int offset)
>  {
> @@ -996,6 +999,20 @@ static int sx150x_regmap_reg_write(void *context, unsigned int reg,
>  	return 0;
>  }
>  
> +static void sx150x_oscio_mask_init(struct sx150x_pinctrl *pctl)
> +{
> +	int pin;
> +
> +	/* OSCIO pin is only present in 789 devices */
> +	if (pctl->data->model != SX150X_789)
> +		return;
> +
> +	for (pin = 0; pin < pctl->data->npins; ++pin) {
> +		if (!strcmp(pctl->data->pins[pin].name, "oscio"))
> +			pctl->oscio_mask |= BIT(pin);
> +	}
> +}
> +

This is quite over-engineered since we have one a maximum of a single oscio line, we could only store the pin number or -1...

>  static bool sx150x_reg_volatile(struct device *dev, unsigned int reg)
>  {
>  	struct sx150x_pinctrl *pctl = i2c_get_clientdata(to_i2c_client(dev));
> @@ -1045,6 +1062,8 @@ static int sx150x_probe(struct i2c_client *client,
>  	if (!pctl->data)
>  		return -EINVAL;
>  
> +	sx150x_oscio_mask_init(pctl);
> +
>  	pctl->regmap = devm_regmap_init(dev, NULL, pctl,
>  					&sx150x_regmap_config);
>  	if (IS_ERR(pctl->regmap)) {
> @@ -1069,6 +1088,8 @@ static int sx150x_probe(struct i2c_client *client,
>  	pctl->gpio.direction_output = sx150x_gpio_direction_output;
>  	pctl->gpio.get = sx150x_gpio_get;
>  	pctl->gpio.set = sx150x_gpio_set;
> +	if (!pctl->oscio_mask)
> +		pctl->gpio.set_multiple = sx150x_gpio_set_multiple;
>  	pctl->gpio.set_single_ended = sx150x_gpio_set_single_ended;
>  	pctl->gpio.parent = dev;
>  #ifdef CONFIG_OF_GPIO
> 

Here, a simple if (pctl->data->model != SX150X_789) would be enough, and please a comment to say why set_multiple is not acceptable for oscio.

Re: [PATCH 0/2] pinctrl: sx150x: set multiple pins at once

[Neil Armstrong]

On 11/22/2016 05:06 PM, Peter Rosin wrote:
> Hi!
> 
> I have only tested this on an 8-bit sx1502, so I'm uncertain if
> the there needs to be locking for this to work as intended for
> the bigger chips with an oscio pin? Probably.
> 
> So, I didn't add (or rather, removed) these lines at the end of
> sx150x_gpio_set_multiple() and made the op optional instead.
> 
> 	if (*mask & pctl->oscio_mask)
> 		sx150x_gpio_oscio_set(pctl, *bits & pctl->oscio_mask);
> 
> I mean, what happens if there are two users writing multiple pins
> where one of the pins is the oscio pin, and this happens concurrently?
> I get the feeling that there is nothing stopping interleaving in that
> case, and you could end up with the desired values from user 1 for the
> normal pins, and the desired value for the oscio pin from user 2.
> 
> But for the easy case (no oscio) where the existing regmap locking
> holds, this is a nice speedup and desired behaviour without a lot
> of individual pin changes.
> 
> Cheers,
> Peter
> 
> Peter Rosin (2):
>   pinctrl: sx150x: various spelling fixes and some white-space cleanup
>   pinctrl: sx150x: support setting multiple pins at once
> 
>  drivers/pinctrl/pinctrl-sx150x.c | 50 ++++++++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 15 deletions(-)
> 

Hi Peter,

Thanks for the patch, yes the oscio has a clearly separate register part of the clock management,
but it could be handled by the set_multiple by masking the oscio bit and managing it separately.

Using a simple integer to store the oscio pin number (or -1 for 456 devices) would simplify the process.

Neil

Re: [PATCH 0/2] pinctrl: sx150x: set multiple pins at once

[Peter Rosin]

On 2016-11-23 10:20, Neil Armstrong wrote:
> On 11/22/2016 05:06 PM, Peter Rosin wrote:
>> Hi!
>>
>> I have only tested this on an 8-bit sx1502, so I'm uncertain if
>> the there needs to be locking for this to work as intended for
>> the bigger chips with an oscio pin? Probably.
>>
>> So, I didn't add (or rather, removed) these lines at the end of
>> sx150x_gpio_set_multiple() and made the op optional instead.
>>
>> 	if (*mask & pctl->oscio_mask)
>> 		sx150x_gpio_oscio_set(pctl, *bits & pctl->oscio_mask);
>>
>> I mean, what happens if there are two users writing multiple pins
>> where one of the pins is the oscio pin, and this happens concurrently?
>> I get the feeling that there is nothing stopping interleaving in that
>> case, and you could end up with the desired values from user 1 for the
>> normal pins, and the desired value for the oscio pin from user 2.
>>
>> But for the easy case (no oscio) where the existing regmap locking
>> holds, this is a nice speedup and desired behaviour without a lot
>> of individual pin changes.
>>
>> Cheers,
>> Peter
>>
>> Peter Rosin (2):
>>   pinctrl: sx150x: various spelling fixes and some white-space cleanup
>>   pinctrl: sx150x: support setting multiple pins at once
>>
>>  drivers/pinctrl/pinctrl-sx150x.c | 50 ++++++++++++++++++++++++++++------------
>>  1 file changed, 35 insertions(+), 15 deletions(-)
>>
> 
> Hi Peter,
> 
> Thanks for the patch, yes the oscio has a clearly separate register part of the clock management,
> but it could be handled by the set_multiple by masking the oscio bit and managing it separately.

That would require locking. If process one wants pin 0 as 1 and oscio
as 1, and process two wants pin 0 as 0 and oscio as 0, and both
processes issue these commands simultaneously. The gpio chip is, to
my understanding, required to make sure that both pin 0 and oscio are
both 0 or both 1 at the end of this. Without locking, that would not
hold:

 process 1                               process 2
   regmap update to data reg.
                                           regmap update to data reg.
                                           regmap update to oscio reg.
   regmap update to oscio reg.

Broken end result: pin 0 is 1 and oscio is 0.

Each regmap access is locked, but individually. In this case we need
locking to span two regmap accesses, which brings us to resurrect all
the locking that Andrey removed, because we then need to lock *all*
regmap accesses with an external lock to prevent other stuff from
sneaking in between the above regmap access pairs. I did not want
to add back all that locking, and had no need for it, so I left the
exercise for someone else to complete.

> Using a simple integer to store the oscio pin number (or -1 for 456 devices) would simplify the process.

I'll send an update...

Cheers,
Peter