Re: [PATCH v3 1/3] gpio: pca953x: Rewrite ->get_multiple() function

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Apr 20, 2020 at 08:27:50PM +0300, Andy Shevchenko wrote:
> The commit 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function")
> basically did everything wrong from style and code reuse perspective, i.e.
> - it didn't utilize existing PCA953x internal helpers
> - it didn't utilize bitmap API
> - it misses the point that ilog2(), besides that BANK_SFT is useless,
>   can be used in macros
> - it has indentation issues.
> 
> Rewrite the function completely.

Bart, Linus, please, consider this series to be applied, because it has Uwe's fixes.
We may still discuss the approach with ->get_multiple(), though.

For the record, should some of us volunteer to be a reviewer for this driver.
It's awful that almost every release we get something either ugly or broken in it.
Uwe, would you like to be a designated reviewer (I would also support)?

> Cc: Paul Thomas <pthomas8589@xxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  drivers/gpio/gpio-pca953x.c | 41 ++++++++++---------------------------
>  1 file changed, 11 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 60ae18e4b5f5a..41be681ae77c2 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -115,7 +115,6 @@ MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids);
>  
>  #define MAX_BANK 5
>  #define BANK_SZ 8
> -#define BANK_SFT 3 /* ilog2(BANK_SZ) */
>  #define MAX_LINE	(MAX_BANK * BANK_SZ)
>  
>  #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ)
> @@ -469,38 +468,20 @@ static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off)
>  }
>  
>  static int pca953x_gpio_get_multiple(struct gpio_chip *gc,
> -				      unsigned long *mask, unsigned long *bits)
> +				     unsigned long *mask, unsigned long *bits)
>  {
>  	struct pca953x_chip *chip = gpiochip_get_data(gc);
> -	unsigned int reg_val;
> -	int offset, value, i, ret = 0;
> -	u8 inreg;
> +	DECLARE_BITMAP(reg_val, MAX_LINE);
> +	int ret;
>  
> -	/* Force offset outside the range of i so that
> -	 * at least the first relevant register is read
> -	 */
> -	offset = gc->ngpio;
> -	for_each_set_bit(i, mask, gc->ngpio) {
> -		/* whenever i goes into a new bank update inreg
> -		 * and read the register
> -		 */
> -		if ((offset >> BANK_SFT) != (i >> BANK_SFT)) {
> -			offset = i;
> -			inreg = pca953x_recalc_addr(chip, chip->regs->input,
> -						    offset, true, false);
> -			mutex_lock(&chip->i2c_lock);
> -			ret = regmap_read(chip->regmap, inreg, &reg_val);
> -			mutex_unlock(&chip->i2c_lock);
> -			if (ret < 0)
> -				return ret;
> -		}
> -		/* reg_val is relative to the last read byte,
> -		 * so only shift the relative bits
> -		 */
> -		value = (reg_val >> (i % 8)) & 0x01;
> -		__assign_bit(i, bits, value);
> -	}
> -	return ret;
> +	mutex_lock(&chip->i2c_lock);
> +	ret = pca953x_read_regs(chip, chip->regs->input, reg_val);
> +	mutex_unlock(&chip->i2c_lock);
> +	if (ret)
> +		return ret;
> +
> +	bitmap_replace(bits, bits, reg_val, mask, gc->ngpio);
> +	return 0;
>  }
>  
>  static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko