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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux