Re: [PATCH v9 4/4] gpio: xilinx: Utilize generic bitmap_get_value and _set_value.

[William Breathitt Gray]

On Sat, Jun 27, 2020 at 01:44:23PM +0530, Syed Nayyar Waris wrote:
> This patch reimplements the xgpio_set_multiple function in
> drivers/gpio/gpio-xilinx.c to use the new generic functions:
> bitmap_get_value and bitmap_set_value. The code is now simpler
> to read and understand. Moreover, instead of looping for each bit
> in xgpio_set_multiple function, now we can check each channel at
> a time and save cycles.
> ---
> Changes in v9:
>  - Remove looping of 'for_each_set_clump' and instead process two
>    halves of a 64-bit bitmap separately or individually. Use normal spin_lock 
>    call for second inner lock. And take the spin_lock_init call outside the 'if'
>    condition in the 'probe' function of driver.
> 
> Changes in v8:
>  - No change.
> 
> Changes in v7:
>  - No change.
> 
> Changes in v6:
>  - No change.
> 
> Changes in v5:
>  - Minor change: Inline values '32' and '64' in code for better
>    code readability.
> 
> Changes in v4:
>  - Minor change: Inline values '32' and '64' in code for better
>    code readability.
> 
> Changes in v3:
>  - No change.
> 
> Changes in v2:
>  - No change
> 
>  drivers/gpio/gpio-xilinx.c | 66 +++++++++++++++++++-------------------
>  1 file changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index 67f9f82e0db0..48393d06fb55 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -136,39 +136,39 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
>  static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
>  			       unsigned long *bits)
>  {
> -	unsigned long flags;
> +	unsigned long flag;

Hi Syed,

I have a couple minor suggestions in case you submit a version 10. Leave
this variable named "flags" because it refers to CPU flags which will
likely be more than one flag anyway.

> @@ -292,6 +292,7 @@ static int xgpio_probe(struct platform_device *pdev)
>  		chip->gpio_width[0] = 32;
>  
>  	spin_lock_init(&chip->gpio_lock[0]);
> +	spin_lock_init(&chip->gpio_lock[1]);
>  
>  	if (of_property_read_u32(np, "xlnx,is-dual", &is_dual))
>  		is_dual = 0;
> @@ -314,7 +315,6 @@ static int xgpio_probe(struct platform_device *pdev)
>  					 &chip->gpio_width[1]))
>  			chip->gpio_width[1] = 32;
>  

Remove this whitespace as well.

Thanks,

William Breathitt Gray

> -		spin_lock_init(&chip->gpio_lock[1]);
>  	}
>  
>  	chip->gc.base = -1;
> -- 
> 2.26.2
>

Attachment: signature.asc
Description: PGP signature