Re: [PATCH v3 4/4] gpio: xilinx: Utilize for_each_set_clump macro

[William Breathitt Gray]

On Thu, Apr 30, 2020 at 07:38:55PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 30, 2020 at 09:45:14PM +0530, Syed Nayyar Waris wrote:
> > On Wed, Apr 29, 2020 at 01:21:14PM +0300, Andy Shevchenko wrote:
> > > On Wed, Apr 29, 2020 at 04:39:47AM +0530, Syed Nayyar Waris wrote:
> 
> ...
> 
> > > > +	const unsigned long state_size = BITS_PER_TYPE(*state);
> > > 
> > > This '*state' is unneeded complication, use BITS_PER_U32.
> > > 
> > > > +#define TOTAL_BITS BITS_PER_TYPE(chip->gpio_state)
> > > 
> > > This macro makes code uglier, besides the fact of absence of #undef.
> > > And also see above.
> > 
> > Thank you for your review comments. Just want to clarify, you want
> >  a new macro to be created - 'BITS_PER_U32' ?
> 
> It's already there (read bits.h).

I'm having trouble finding the BITS_PER_U32 macro; are you thinking of
BITS_PER_LONG? I don't think there are any cases where u32 is not 32
bits wide, so perhaps it'll be better to just hardcode 32 directly in
the code here to make it easier to read.

William Breathitt Gray

> 
> > Also, don't you think that with BITS_PER_TYPE(), in case later the type
> > of 'state' changes, it will be reflected in this code without any code
> > change?
> 
> If it changes the bits per type will be least issues there. The rationale
> behind is to have code readable. In proposed change it is not.
> 
> > Let me know if I have misunderstood something.
> > 
> > > 
> > > > +	DECLARE_BITMAP(old, TOTAL_BITS);
> > > > +	DECLARE_BITMAP(new, TOTAL_BITS);
> > > > +	DECLARE_BITMAP(changed, TOTAL_BITS);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Attachment: signature.asc
Description: PGP signature