[PATCH] gpio: pcf857x: Implement get_multiple/set_multiple methods

[PATCH] gpio: pcf857x: Implement get_multiple/set_multiple methods

[Radu Rendec]

This change allows the GPIO core to read/change multiple pins in a
single driver call and subsequent I2C transfer. It helps a lot with
PCF857x devices, since their I2C protocol always reads/changes all
existing pins anyway. Therefore, when the GPIO client code does a bulk
operation on multiple pins, the driver makes a single I2C transfer.

Signed-off-by: Radu Rendec <radu.rendec@gmail.com>
---
 drivers/gpio/gpio-pcf857x.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index cec2f2c78255..8a8ffeaa8b22 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -141,6 +141,21 @@ static int pcf857x_get(struct gpio_chip *chip, unsigned offset)
 	return (value < 0) ? value : !!(value & (1 << offset));
 }
 
+static int pcf857x_get_multiple(struct gpio_chip *chip, unsigned long *mask,
+				unsigned long *bits)
+{
+	struct pcf857x	*gpio = gpiochip_get_data(chip);
+	int		value = gpio->read(gpio->client);
+
+	if (value < 0)
+		return value;
+
+	*bits &= ~*mask;
+	*bits |= value & *mask;
+
+	return 0;
+}
+
 static int pcf857x_output(struct gpio_chip *chip, unsigned offset, int value)
 {
 	struct pcf857x	*gpio = gpiochip_get_data(chip);
@@ -163,6 +178,18 @@ static void pcf857x_set(struct gpio_chip *chip, unsigned offset, int value)
 	pcf857x_output(chip, offset, value);
 }
 
+static void pcf857x_set_multiple(struct gpio_chip *chip, unsigned long *mask,
+				 unsigned long *bits)
+{
+	struct pcf857x *gpio = gpiochip_get_data(chip);
+
+	mutex_lock(&gpio->lock);
+	gpio->out &= ~*mask;
+	gpio->out |= *bits & *mask;
+	gpio->write(gpio->client, gpio->out);
+	mutex_unlock(&gpio->lock);
+}
+
 /*-------------------------------------------------------------------------*/
 
 static irqreturn_t pcf857x_irq(int irq, void *data)
@@ -275,7 +302,9 @@ static int pcf857x_probe(struct i2c_client *client)
 	gpio->chip.parent		= &client->dev;
 	gpio->chip.owner		= THIS_MODULE;
 	gpio->chip.get			= pcf857x_get;
+	gpio->chip.get_multiple		= pcf857x_get_multiple;
 	gpio->chip.set			= pcf857x_set;
+	gpio->chip.set_multiple		= pcf857x_set_multiple;
 	gpio->chip.direction_input	= pcf857x_input;
 	gpio->chip.direction_output	= pcf857x_output;
 	gpio->chip.ngpio		= id->driver_data;
-- 
2.39.0

Re: [PATCH] gpio: pcf857x: Implement get_multiple/set_multiple methods

[Bartosz Golaszewski]

On Wed, Jan 4, 2023 at 2:20 AM Radu Rendec <radu.rendec@gmail.com> wrote:
>
> This change allows the GPIO core to read/change multiple pins in a
> single driver call and subsequent I2C transfer. It helps a lot with
> PCF857x devices, since their I2C protocol always reads/changes all
> existing pins anyway. Therefore, when the GPIO client code does a bulk
> operation on multiple pins, the driver makes a single I2C transfer.
>
> Signed-off-by: Radu Rendec <radu.rendec@gmail.com>
> ---
>  drivers/gpio/gpio-pcf857x.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
> index cec2f2c78255..8a8ffeaa8b22 100644
> --- a/drivers/gpio/gpio-pcf857x.c
> +++ b/drivers/gpio/gpio-pcf857x.c
> @@ -141,6 +141,21 @@ static int pcf857x_get(struct gpio_chip *chip, unsigned offset)
>         return (value < 0) ? value : !!(value & (1 << offset));
>  }
>
> +static int pcf857x_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> +                               unsigned long *bits)
> +{
> +       struct pcf857x  *gpio = gpiochip_get_data(chip);
> +       int             value = gpio->read(gpio->client);

Ugh, this whitespacing is awful but I get it that you only tried to
stay consistent with the rest of the code. Could you add a patch
before this one that removes those tabs from local variables? IMO This
makes the code less readable, not more. While at it: you can also
change all instances of 'unsigned' to 'unsigned int' as per the
checkpatch rule.

Otherwise it looks good!

Bart

> +
> +       if (value < 0)
> +               return value;
> +
> +       *bits &= ~*mask;
> +       *bits |= value & *mask;
> +
> +       return 0;
> +}
> +
>  static int pcf857x_output(struct gpio_chip *chip, unsigned offset, int value)
>  {
>         struct pcf857x  *gpio = gpiochip_get_data(chip);
> @@ -163,6 +178,18 @@ static void pcf857x_set(struct gpio_chip *chip, unsigned offset, int value)
>         pcf857x_output(chip, offset, value);
>  }
>
> +static void pcf857x_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> +                                unsigned long *bits)
> +{
> +       struct pcf857x *gpio = gpiochip_get_data(chip);
> +
> +       mutex_lock(&gpio->lock);
> +       gpio->out &= ~*mask;
> +       gpio->out |= *bits & *mask;
> +       gpio->write(gpio->client, gpio->out);
> +       mutex_unlock(&gpio->lock);
> +}
> +
>  /*-------------------------------------------------------------------------*/
>
>  static irqreturn_t pcf857x_irq(int irq, void *data)
> @@ -275,7 +302,9 @@ static int pcf857x_probe(struct i2c_client *client)
>         gpio->chip.parent               = &client->dev;
>         gpio->chip.owner                = THIS_MODULE;
>         gpio->chip.get                  = pcf857x_get;
> +       gpio->chip.get_multiple         = pcf857x_get_multiple;
>         gpio->chip.set                  = pcf857x_set;
> +       gpio->chip.set_multiple         = pcf857x_set_multiple;
>         gpio->chip.direction_input      = pcf857x_input;
>         gpio->chip.direction_output     = pcf857x_output;
>         gpio->chip.ngpio                = id->driver_data;
> --
> 2.39.0
>

Re: [PATCH] gpio: pcf857x: Implement get_multiple/set_multiple methods

[Radu Rendec]

On Fri, 2023-01-06 at 15:17 +0100, Bartosz Golaszewski wrote:
> On Wed, Jan 4, 2023 at 2:20 AM Radu Rendec <radu.rendec@gmail.com> wrote:
> > 
> > +static int pcf857x_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> > +                               unsigned long *bits)
> > +{
> > +       struct pcf857x  *gpio = gpiochip_get_data(chip);
> > +       int             value = gpio->read(gpio->client);
> 
> Ugh, this whitespacing is awful but I get it that you only tried to
> stay consistent with the rest of the code. Could you add a patch
> before this one that removes those tabs from local variables? IMO This
> makes the code less readable, not more. While at it: you can also
> change all instances of 'unsigned' to 'unsigned int' as per the
> checkpatch rule.
> 
> Otherwise it looks good!

Thanks for reviewing the patch. Happy to remove those weird tabs. I'm
not a fan either, and as you guessed I just tried to stay consistent.
It's not just the local variables though, it's also the definition of
struct pcf857x and some initialization in pcf857x_probe(). 

I will send a v2 patch series shortly. If you think it's better to fix
the other tabs as well, please just let me know. Thanks!

Radu