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

Subsystems: gpio subsystem, the rest

STALE1375d

3 messages, 2 authors, 2023-01-06 · open the first message on its own page

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

From: Radu Rendec <hidden>
Date: 2023-01-04 01:21:35

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 <redacted>
---
 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

From: Bartosz Golaszewski <hidden>
Date: 2023-01-06 14:17:25

On Wed, Jan 4, 2023 at 2:20 AM Radu Rendec [off-list ref] wrote:
quoted hunk
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 <redacted>
---
 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
quoted hunk
+
+       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

From: Radu Rendec <hidden>
Date: 2023-01-06 15:57:38

On Fri, 2023-01-06 at 15:17 +0100, Bartosz Golaszewski wrote:
On Wed, Jan 4, 2023 at 2:20 AM Radu Rendec [off-list ref] wrote:
quoted

+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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help