[V2,01/14] gpio: pca953x: Deduplicate the bank_shift
diff mbox series

Message ID 20181212014002.4753-1-marek.vasut+renesas@gmail.com
State New
Headers show
Series
  • [V2,01/14] gpio: pca953x: Deduplicate the bank_shift
Related show

Commit Message

Marek Vasut Dec. 12, 2018, 1:39 a.m. UTC
The bank_shift = fls(...) code was duplicated in the driver 5 times,
pull it into separate function.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
V2: Replace bank_size with bank_shift in commit message
---
 drivers/gpio/gpio-pca953x.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Kieran Bingham Dec. 13, 2018, 11:03 a.m. UTC | #1
Hi Marek,

On 12/12/2018 01:39, Marek Vasut wrote:
> The bank_shift = fls(...) code was duplicated in the driver 5 times,
> pull it into separate function.
> 

This looks like a good fix-up to me.

As it's just a single line, it might have warranted being a macro - but
I think the function helps a lot and is readable. The compiler will
likely inline it anyway.


> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> V2: Replace bank_size with bank_shift in commit message
> ---
>  drivers/gpio/gpio-pca953x.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 540166443c34..afe6de4c48c4 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -159,11 +159,16 @@ struct pca953x_chip {
>  	int (*read_regs)(struct pca953x_chip *, int, u8 *);
>  };
>  
> +static int pca953x_bank_shift(struct pca953x_chip *chip)
> +{
> +	return fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +}
> +
>  static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
>  				int off)
>  {
>  	int ret;
> -	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +	int bank_shift = pca953x_bank_shift(chip);
>  	int offset = off / BANK_SZ;
>  
>  	ret = i2c_smbus_read_byte_data(chip->client,
> @@ -182,7 +187,7 @@ static int pca953x_write_single(struct pca953x_chip *chip, int reg, u32 val,
>  				int off)
>  {
>  	int ret;
> -	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +	int bank_shift = pca953x_bank_shift(chip);
>  	int offset = off / BANK_SZ;
>  
>  	ret = i2c_smbus_write_byte_data(chip->client,
> @@ -221,7 +226,7 @@ static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>  
>  static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>  {
> -	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +	int bank_shift = pca953x_bank_shift(chip);
>  	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
>  	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>  
> @@ -265,7 +270,7 @@ static int pca953x_read_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>  
>  static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>  {
> -	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +	int bank_shift = pca953x_bank_shift(chip);
>  	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
>  	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>  
> @@ -402,13 +407,12 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
>  				      unsigned long *mask, unsigned long *bits)
>  {
>  	struct pca953x_chip *chip = gpiochip_get_data(gc);
> +	int bank_shift = pca953x_bank_shift(chip);
>  	unsigned int bank_mask, bank_val;
> -	int bank_shift, bank;
> +	int bank;
>  	u8 reg_val[MAX_BANK];
>  	int ret;
>  
> -	bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> -
>  	mutex_lock(&chip->i2c_lock);
>  	memcpy(reg_val, chip->reg_output, NBANK(chip));
>  	for (bank = 0; bank < NBANK(chip); bank++) {
>
Marek Vasut Dec. 13, 2018, 1:17 p.m. UTC | #2
On 12/13/2018 12:03 PM, Kieran Bingham wrote:
> Hi Marek,

Hi,

> On 12/12/2018 01:39, Marek Vasut wrote:
>> The bank_shift = fls(...) code was duplicated in the driver 5 times,
>> pull it into separate function.
>>
> 
> This looks like a good fix-up to me.
> 
> As it's just a single line, it might have warranted being a macro - but
> I think the function helps a lot and is readable. The compiler will
> likely inline it anyway.

The compiler will inline it , and the bonus point is that you get type
checking . There is no type checking with a macro.

>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> 
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>> V2: Replace bank_size with bank_shift in commit message
>> ---
>>  drivers/gpio/gpio-pca953x.c | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
>> index 540166443c34..afe6de4c48c4 100644
>> --- a/drivers/gpio/gpio-pca953x.c
>> +++ b/drivers/gpio/gpio-pca953x.c
>> @@ -159,11 +159,16 @@ struct pca953x_chip {
>>  	int (*read_regs)(struct pca953x_chip *, int, u8 *);
>>  };
>>  
>> +static int pca953x_bank_shift(struct pca953x_chip *chip)
>> +{
>> +	return fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>> +}
>> +
>>  static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
>>  				int off)
>>  {
>>  	int ret;
>> -	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>> +	int bank_shift = pca953x_bank_shift(chip);
>>  	int offset = off / BANK_SZ;
>>  
>>  	ret = i2c_smbus_read_byte_data(chip->client,
>> @@ -182,7 +187,7 @@ static int pca953x_write_single(struct pca953x_chip *chip, int reg, u32 val,
>>  				int off)
>>  {
>>  	int ret;
>> -	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>> +	int bank_shift = pca953x_bank_shift(chip);
>>  	int offset = off / BANK_SZ;
>>  
>>  	ret = i2c_smbus_write_byte_data(chip->client,
>> @@ -221,7 +226,7 @@ static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>>  
>>  static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>>  {
>> -	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>> +	int bank_shift = pca953x_bank_shift(chip);
>>  	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
>>  	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>>  
>> @@ -265,7 +270,7 @@ static int pca953x_read_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>>  
>>  static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>>  {
>> -	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>> +	int bank_shift = pca953x_bank_shift(chip);
>>  	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
>>  	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>>  
>> @@ -402,13 +407,12 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
>>  				      unsigned long *mask, unsigned long *bits)
>>  {
>>  	struct pca953x_chip *chip = gpiochip_get_data(gc);
>> +	int bank_shift = pca953x_bank_shift(chip);
>>  	unsigned int bank_mask, bank_val;
>> -	int bank_shift, bank;
>> +	int bank;
>>  	u8 reg_val[MAX_BANK];
>>  	int ret;
>>  
>> -	bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>> -
>>  	mutex_lock(&chip->i2c_lock);
>>  	memcpy(reg_val, chip->reg_output, NBANK(chip));
>>  	for (bank = 0; bank < NBANK(chip); bank++) {
>>
> 
>
Linus Walleij Dec. 14, 2018, 2:50 p.m. UTC | #3
I applied the whole series to an immutable branch and pushed to
kernelorg for testing in zeroday. If it builds fine I will push it
to devel.

Must say this series is impressive. It's doing away with a whole
slew of "technical debt" (old code not using modern frameworks).

Strangely this driver doesn't support any pin config, it even seems
to have per-pin debounce and everything. Some can be quickly
supported by just adding the .set_config() callback, others like
pull up/down are more dubious. If it supports open drain in hardware
we should definately make use of it. Just in case you're interested!

Yours,
Linus Walleij
Marek Vasut Dec. 15, 2018, 4:48 a.m. UTC | #4
On 12/14/2018 03:50 PM, Linus Walleij wrote:
> I applied the whole series to an immutable branch and pushed to
> kernelorg for testing in zeroday. If it builds fine I will push it
> to devel.
> 
> Must say this series is impressive. It's doing away with a whole
> slew of "technical debt" (old code not using modern frameworks).

Yeah, I gave up twice while trying to untangle the mess in that driver.
It had indeed a lot of technical debt accumulated over the years of
incremental development. I'm worried this series breaks some of the
chips though, as I couldn't test it on the whole lot of the supported ones.

I even wonder whether we shouldn't remove support for some of the chips
which have no users in the kernel and add extra complexity to the
driver, like the PCA957x .

> Strangely this driver doesn't support any pin config, it even seems
> to have per-pin debounce and everything.

That depends on the chip, really. Most of the PCA953x series and
compatible only have the In/Out/Direction/Invert registers, in various
quantities. There are some which have extra functionality (like the
integrated pull resistors) but those are rare.

> Some can be quickly
> supported by just adding the .set_config() callback, others like
> pull up/down are more dubious. If it supports open drain in hardware
> we should definately make use of it. Just in case you're interested!

I went through the hardware I have around and the datasheets to the
PCA953x chips on them, but I couldn't find a board which would have a
PCA953x chip with those extra features. I'll keep my eyes peeled for a
board with one.
Linus Walleij Dec. 16, 2018, 12:27 a.m. UTC | #5
On Sat, Dec 15, 2018 at 5:48 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 12/14/2018 03:50 PM, Linus Walleij wrote:

> > Some can be quickly
> > supported by just adding the .set_config() callback, others like
> > pull up/down are more dubious. If it supports open drain in hardware
> > we should definately make use of it. Just in case you're interested!
>
> I went through the hardware I have around and the datasheets to the
> PCA953x chips on them, but I couldn't find a board which would have a
> PCA953x chip with those extra features. I'll keep my eyes peeled for a
> board with one.

It appears Thomas needs those features so let's let him have a go
at it :D

Yours,
Linus Walleij
Marek Vasut Dec. 16, 2018, 5:18 p.m. UTC | #6
On 12/16/2018 01:27 AM, Linus Walleij wrote:
> On Sat, Dec 15, 2018 at 5:48 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 12/14/2018 03:50 PM, Linus Walleij wrote:
> 
>>> Some can be quickly
>>> supported by just adding the .set_config() callback, others like
>>> pull up/down are more dubious. If it supports open drain in hardware
>>> we should definately make use of it. Just in case you're interested!
>>
>> I went through the hardware I have around and the datasheets to the
>> PCA953x chips on them, but I couldn't find a board which would have a
>> PCA953x chip with those extra features. I'll keep my eyes peeled for a
>> board with one.
> 
> It appears Thomas needs those features so let's let him have a go
> at it :D

Awesome :-)

Patch
diff mbox series

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 540166443c34..afe6de4c48c4 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -159,11 +159,16 @@  struct pca953x_chip {
 	int (*read_regs)(struct pca953x_chip *, int, u8 *);
 };
 
+static int pca953x_bank_shift(struct pca953x_chip *chip)
+{
+	return fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+}
+
 static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
 				int off)
 {
 	int ret;
-	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+	int bank_shift = pca953x_bank_shift(chip);
 	int offset = off / BANK_SZ;
 
 	ret = i2c_smbus_read_byte_data(chip->client,
@@ -182,7 +187,7 @@  static int pca953x_write_single(struct pca953x_chip *chip, int reg, u32 val,
 				int off)
 {
 	int ret;
-	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+	int bank_shift = pca953x_bank_shift(chip);
 	int offset = off / BANK_SZ;
 
 	ret = i2c_smbus_write_byte_data(chip->client,
@@ -221,7 +226,7 @@  static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
 
 static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
 {
-	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+	int bank_shift = pca953x_bank_shift(chip);
 	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
 	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
 
@@ -265,7 +270,7 @@  static int pca953x_read_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
 
 static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
 {
-	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+	int bank_shift = pca953x_bank_shift(chip);
 	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
 	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
 
@@ -402,13 +407,12 @@  static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 				      unsigned long *mask, unsigned long *bits)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
+	int bank_shift = pca953x_bank_shift(chip);
 	unsigned int bank_mask, bank_val;
-	int bank_shift, bank;
+	int bank;
 	u8 reg_val[MAX_BANK];
 	int ret;
 
-	bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
-
 	mutex_lock(&chip->i2c_lock);
 	memcpy(reg_val, chip->reg_output, NBANK(chip));
 	for (bank = 0; bank < NBANK(chip); bank++) {