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

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

czw., 9 kwi 2020 o 17:09 Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> napisał(a):
>
> 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.
>

Frankly this is just my personal preference. I think it's consistent
with the majority of codebase in the kernel but I won't block this
patch for that reason. Feel free to leave it like it is if you prefer
it.

Bart