LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/1] input: adp5589: Add gpio_set_multiple interface
@ 2019-04-15 12:25 Bogdan Togorean
  2019-05-21  8:38 ` [PATCH RESEND] " Bogdan Togorean
  0 siblings, 1 reply; 4+ messages in thread
From: Bogdan Togorean @ 2019-04-15 12:25 UTC (permalink / raw)
  To: linux-input; +Cc: dmitry.torokhov, gustavo, linux-kernel, Bogdan Togorean

This patch implements the gpio_set_multiple interface for ADP558x chip.

Signed-off-by: Bogdan Togorean <bogdan.togorean@analog.com>
---
 drivers/input/keyboard/adp5589-keys.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
index 2835fba71c33..143871bd60ef 100644
--- a/drivers/input/keyboard/adp5589-keys.c
+++ b/drivers/input/keyboard/adp5589-keys.c
@@ -416,6 +416,30 @@ static void adp5589_gpio_set_value(struct gpio_chip *chip,
 	mutex_unlock(&kpad->gpio_lock);
 }
 
+static void adp5589_gpio_set_multiple(struct gpio_chip *chip,
+				      unsigned long *mask, unsigned long *bits)
+{
+	struct adp5589_kpad *kpad = container_of(chip, struct adp5589_kpad, gc);
+	u8 bank, reg_mask, reg_bits;
+
+	mutex_lock(&kpad->gpio_lock);
+
+	for (bank = 0; bank <= kpad->var->bank(kpad->var->maxgpio); bank++) {
+		if (bank > kpad->var->bank(get_bitmask_order(*mask) - 1))
+			break;
+		reg_mask = mask[bank / sizeof(*mask)] >>
+			   ((bank % sizeof(*mask)) * BITS_PER_BYTE);
+		reg_bits = bits[bank / sizeof(*bits)] >>
+			   ((bank % sizeof(*bits)) * BITS_PER_BYTE);
+		kpad->dat_out[bank] &= ~reg_mask;
+		kpad->dat_out[bank] |= reg_bits & reg_mask;
+		adp5589_write(kpad->client, kpad->var->reg(ADP5589_GPO_DATA_OUT_A) + bank,
+			      kpad->dat_out[bank]);
+	}
+
+	mutex_unlock(&kpad->gpio_lock);
+}
+
 static int adp5589_gpio_direction_input(struct gpio_chip *chip, unsigned off)
 {
 	struct adp5589_kpad *kpad = gpiochip_get_data(chip);
@@ -517,6 +541,7 @@ static int adp5589_gpio_add(struct adp5589_kpad *kpad)
 	kpad->gc.direction_output = adp5589_gpio_direction_output;
 	kpad->gc.get = adp5589_gpio_get_value;
 	kpad->gc.set = adp5589_gpio_set_value;
+	kpad->gc.set_multiple = adp5589_gpio_set_multiple;
 	kpad->gc.can_sleep = 1;
 
 	kpad->gc.base = gpio_data->gpio_start;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH RESEND] input: adp5589: Add gpio_set_multiple interface
  2019-04-15 12:25 [PATCH 1/1] input: adp5589: Add gpio_set_multiple interface Bogdan Togorean
@ 2019-05-21  8:38 ` Bogdan Togorean
  2019-05-23  7:18   ` Dmitry Torokhov
  0 siblings, 1 reply; 4+ messages in thread
From: Bogdan Togorean @ 2019-05-21  8:38 UTC (permalink / raw)
  To: linux-input
  Cc: dmitry.torokhov, gustavo, linux-kernel, Michael.Hennerich,
	Bogdan Togorean

This patch implements the gpio_set_multiple interface for ADP558x chip.

Signed-off-by: Bogdan Togorean <bogdan.togorean@analog.com>
---
 drivers/input/keyboard/adp5589-keys.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
index 2835fba71c33..143871bd60ef 100644
--- a/drivers/input/keyboard/adp5589-keys.c
+++ b/drivers/input/keyboard/adp5589-keys.c
@@ -416,6 +416,30 @@ static void adp5589_gpio_set_value(struct gpio_chip *chip,
 	mutex_unlock(&kpad->gpio_lock);
 }
 
+static void adp5589_gpio_set_multiple(struct gpio_chip *chip,
+				      unsigned long *mask, unsigned long *bits)
+{
+	struct adp5589_kpad *kpad = container_of(chip, struct adp5589_kpad, gc);
+	u8 bank, reg_mask, reg_bits;
+
+	mutex_lock(&kpad->gpio_lock);
+
+	for (bank = 0; bank <= kpad->var->bank(kpad->var->maxgpio); bank++) {
+		if (bank > kpad->var->bank(get_bitmask_order(*mask) - 1))
+			break;
+		reg_mask = mask[bank / sizeof(*mask)] >>
+			   ((bank % sizeof(*mask)) * BITS_PER_BYTE);
+		reg_bits = bits[bank / sizeof(*bits)] >>
+			   ((bank % sizeof(*bits)) * BITS_PER_BYTE);
+		kpad->dat_out[bank] &= ~reg_mask;
+		kpad->dat_out[bank] |= reg_bits & reg_mask;
+		adp5589_write(kpad->client, kpad->var->reg(ADP5589_GPO_DATA_OUT_A) + bank,
+			      kpad->dat_out[bank]);
+	}
+
+	mutex_unlock(&kpad->gpio_lock);
+}
+
 static int adp5589_gpio_direction_input(struct gpio_chip *chip, unsigned off)
 {
 	struct adp5589_kpad *kpad = gpiochip_get_data(chip);
@@ -517,6 +541,7 @@ static int adp5589_gpio_add(struct adp5589_kpad *kpad)
 	kpad->gc.direction_output = adp5589_gpio_direction_output;
 	kpad->gc.get = adp5589_gpio_get_value;
 	kpad->gc.set = adp5589_gpio_set_value;
+	kpad->gc.set_multiple = adp5589_gpio_set_multiple;
 	kpad->gc.can_sleep = 1;
 
 	kpad->gc.base = gpio_data->gpio_start;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH RESEND] input: adp5589: Add gpio_set_multiple interface
  2019-05-21  8:38 ` [PATCH RESEND] " Bogdan Togorean
@ 2019-05-23  7:18   ` Dmitry Torokhov
  2019-05-27  8:40     ` Togorean, Bogdan
  0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Torokhov @ 2019-05-23  7:18 UTC (permalink / raw)
  To: Bogdan Togorean; +Cc: linux-input, gustavo, linux-kernel, Michael.Hennerich

Hi Bogdan,

On Tue, May 21, 2019 at 11:38:22AM +0300, Bogdan Togorean wrote:
> This patch implements the gpio_set_multiple interface for ADP558x chip.
> 
> Signed-off-by: Bogdan Togorean <bogdan.togorean@analog.com>
> ---
>  drivers/input/keyboard/adp5589-keys.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
> index 2835fba71c33..143871bd60ef 100644
> --- a/drivers/input/keyboard/adp5589-keys.c
> +++ b/drivers/input/keyboard/adp5589-keys.c
> @@ -416,6 +416,30 @@ static void adp5589_gpio_set_value(struct gpio_chip *chip,
>  	mutex_unlock(&kpad->gpio_lock);
>  }
>  
> +static void adp5589_gpio_set_multiple(struct gpio_chip *chip,
> +				      unsigned long *mask, unsigned long *bits)
> +{
> +	struct adp5589_kpad *kpad = container_of(chip, struct adp5589_kpad, gc);
> +	u8 bank, reg_mask, reg_bits;
> +
> +	mutex_lock(&kpad->gpio_lock);
> +
> +	for (bank = 0; bank <= kpad->var->bank(kpad->var->maxgpio); bank++) {
> +		if (bank > kpad->var->bank(get_bitmask_order(*mask) - 1))
> +			break;

I wonder if we should have:

	last_gpio = min(kpad->var->maxgpio, get_bitmask_order(*mask) - 1);
	last_bank = kpad->var->bank(last_bank);
	for (bank = 0; bank <= last_bank; bank++) {
		...
	}

> +		reg_mask = mask[bank / sizeof(*mask)] >>
> +			   ((bank % sizeof(*mask)) * BITS_PER_BYTE);
> +		reg_bits = bits[bank / sizeof(*bits)] >>
> +			   ((bank % sizeof(*bits)) * BITS_PER_BYTE);

This s really hard to parse. We know that "bank" is a byte, and mask is
long, we do not have to be this roundabout it.

> +		kpad->dat_out[bank] &= ~reg_mask;
> +		kpad->dat_out[bank] |= reg_bits & reg_mask;
> +		adp5589_write(kpad->client, kpad->var->reg(ADP5589_GPO_DATA_OUT_A) + bank,
> +			      kpad->dat_out[bank]);
> +	}

However the biggest issue is that this implementation seems to ignore
the kpad->gpiomap that translates GPIO numbers as seen by gpiolib to
GPIO numbers used by the chip. You need to reshuffle the mask and bits,
and only then do the writes.

Given the complexities, does set_multiple really save anything?

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH RESEND] input: adp5589: Add gpio_set_multiple interface
  2019-05-23  7:18   ` Dmitry Torokhov
@ 2019-05-27  8:40     ` Togorean, Bogdan
  0 siblings, 0 replies; 4+ messages in thread
From: Togorean, Bogdan @ 2019-05-27  8:40 UTC (permalink / raw)
  To: dmitry.torokhov; +Cc: linux-input, Hennerich, Michael, linux-kernel, gustavo

Hi Dmitry,

Thank you for your review

On Thu, 2019-05-23 at 00:18 -0700, Dmitry Torokhov wrote:
> [External]
> 
> 
> Hi Bogdan,
> 
> On Tue, May 21, 2019 at 11:38:22AM +0300, Bogdan Togorean wrote:
> > This patch implements the gpio_set_multiple interface for ADP558x
> > chip.
> > 
> > Signed-off-by: Bogdan Togorean <bogdan.togorean@analog.com>
> > ---
> >  drivers/input/keyboard/adp5589-keys.c | 25
> > +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/drivers/input/keyboard/adp5589-keys.c
> > b/drivers/input/keyboard/adp5589-keys.c
> > index 2835fba71c33..143871bd60ef 100644
> > --- a/drivers/input/keyboard/adp5589-keys.c
> > +++ b/drivers/input/keyboard/adp5589-keys.c
> > @@ -416,6 +416,30 @@ static void adp5589_gpio_set_value(struct
> > gpio_chip *chip,
> >       mutex_unlock(&kpad->gpio_lock);
> >  }
> > 
> > +static void adp5589_gpio_set_multiple(struct gpio_chip *chip,
> > +                                   unsigned long *mask, unsigned
> > long *bits)
> > +{
> > +     struct adp5589_kpad *kpad = container_of(chip, struct
> > adp5589_kpad, gc);
> > +     u8 bank, reg_mask, reg_bits;
> > +
> > +     mutex_lock(&kpad->gpio_lock);
> > +
> > +     for (bank = 0; bank <= kpad->var->bank(kpad->var->maxgpio);
> > bank++) {
> > +             if (bank > kpad->var->bank(get_bitmask_order(*mask) -
> > 1))
> > +                     break;
> 
> I wonder if we should have:
> 
>         last_gpio = min(kpad->var->maxgpio, get_bitmask_order(*mask)
> - 1);
>         last_bank = kpad->var->bank(last_bank);
>         for (bank = 0; bank <= last_bank; bank++) {
>                 ...
>         }
I agree this can be made more clear in the way you proposed.
> 
> > +             reg_mask = mask[bank / sizeof(*mask)] >>
> > +                        ((bank % sizeof(*mask)) * BITS_PER_BYTE);
> > +             reg_bits = bits[bank / sizeof(*bits)] >>
> > +                        ((bank % sizeof(*bits)) * BITS_PER_BYTE);
> 
> This s really hard to parse. We know that "bank" is a byte, and mask
> is
> long, we do not have to be this roundabout it.
Here main reasons for doing this complexity were to support 64bit/32bit
architectures (differet long size) and to avoid use of magic values
(BITS_PER_BYTE)
> 
> > +             kpad->dat_out[bank] &= ~reg_mask;
> > +             kpad->dat_out[bank] |= reg_bits & reg_mask;
> > +             adp5589_write(kpad->client, kpad->var-
> > >reg(ADP5589_GPO_DATA_OUT_A) + bank,
> > +                           kpad->dat_out[bank]);
> > +     }
> 
> However the biggest issue is that this implementation seems to ignore
> the kpad->gpiomap that translates GPIO numbers as seen by gpiolib to
> GPIO numbers used by the chip. You need to reshuffle the mask and
> bits,
> and only then do the writes.
> 
> Given the complexities, does set_multiple really save anything?
> 
We have a critical application where we need to set multiple GPIOs that
are on the same bank simultaneously. No delay accepted. So this was the
usecase which led to implementation of set_multiple_interface for this
chip.
> Thanks.
> 
> --
> Dmitry
Thank you,
Bogdan


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-05-27  8:40 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-15 12:25 [PATCH 1/1] input: adp5589: Add gpio_set_multiple interface Bogdan Togorean
2019-05-21  8:38 ` [PATCH RESEND] " Bogdan Togorean
2019-05-23  7:18   ` Dmitry Torokhov
2019-05-27  8:40     ` Togorean, Bogdan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).