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
