Re: [PATCH v3 11/11] platform/x86: int3472/discrete: Get the polarity from the _DSM entry

Andy Shevchenko
2022-12-16T17:20:12Z

On Fri, Dec 16, 2022 at 05:42:08PM +0100, Hans de Goede wrote:
> On 12/16/22 15:57, Andy Shevchenko wrote:
> > On Fri, Dec 16, 2022 at 12:30:13PM +0100, Hans de Goede wrote:

...

> >> + /* If bits 31-24 of the _DSM entry are all 0 then the signal is inverted */
> >
> >> + active_value = obj->integer.value >> 24;
> >> + if (!active_value)
> >
> > Not sure why you need a temporary variable for this. Just use
> > GENMASK()/GENMASK_ULL()?
> >
> > if (obj->integer.value & GENMASK(31, 24));
> >
> > In this case you even don't need to repeat bit numbers in the comment.
>
> These bits contain the value to which the pin should be set when the
> sensor is active (on), the active_value helper variable IMHO makes this
> a lot more clear then directly checking the mask.

Mask makes much more clear to understand what bits you are really use without
looking at the type of a temporary variable. (IIUC the value is u64.)

Maybe

active_value = (obj->integer.value & GENMASK(31, 24)) >> 24;

But yes, this is on the edge of bikeshedding.

> >> + polarity ^= GPIO_ACTIVE_LOW;

--
With Best Regards,
Andy Shevchenko