Re: [PATCH v1 1/2] gpio: pch: Use BIT() and GENMASK() where it's appropriate

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Apr 06, 2020 at 09:23:30AM +0200, Bartosz Golaszewski wrote:
> czw., 2 kwi 2020 o 22:19 Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> napisał(a):
> >
> > Use BIT() and GENMASK() where it's appropriate.
> > At the same time drop it where it's not appropriate.

Thanks for review, my comments below.

...

> >  #define PCH_EDGE_FALLING       0
> > -#define PCH_EDGE_RISING                BIT(0)
> > -#define PCH_LEVEL_L            BIT(1)
> > -#define PCH_LEVEL_H            (BIT(0) | BIT(1))
> > +#define PCH_EDGE_RISING                1
> > +#define PCH_LEVEL_L            2
> > +#define PCH_LEVEL_H            3
> 
> If these define bitmask values for some fields in registers, then I'd
> suggest to write it as hex numbers. I find it much more readable this
> way.

You meant
 0x0
 0x1
 0x2
 0x3
?

But what the benefit comes out of it? There are sliding 3 bits (3 bits
per each GPIO line), so this numbers in hex, in my opinion, will add
a confusion: "Are they always in position 2..0 or not?"

That said, I'm not against the change, but I would like to be sure
what is the benefit.

-- 
With Best Regards,
Andy Shevchenko