Re: [PATCH v2 2/4] staging: iio: adt7316: remove shift/offset macros

[Michael Harris]

On 3/6/26 6:53 AM, Andy Shevchenko wrote:

Hi Andy, thanks for reviewing my patch series.

>>  #include <linux/i2c.h>
>>  #include <linux/rtc.h>
>>  #include <linux/module.h>
>> +#include <linux/bitfield.h>
>
> Try to squeeze it to make a longest (but sparse AFAICS) ordered list of
> inclusions. With given context
>
>   #include <linux/bitfield.h>
>   #include <linux/i2c.h>
>   #include <linux/rtc.h>
>   #include <linux/module.h>
>
> gives 3 out of 4 in order.

Placing it there would put it right below slab.h, sysfs.h, and list.h.
The includes as a whole are fairly messy and unorganized and I don't
think there's a clean area where I could insert it. Fully reordering it
would be out of scope for this patch series. If you prefer, I can put it
at the top of the includes since it would be the highest alphabetically.

>>      dac_config = chip->dac_config & (~ADT7316_DA_EN_MODE_MASK);
>> -    dac_config |= data << ADT7316_DA_EN_MODE_SHIFT;
>> +    dac_config |= FIELD_PREP(ADT7316_DA_EN_MODE_MASK, data);
>
>FIELD_MODIFY() ?
>

I will apply this.

>
>> -            data = msb << ADT7316_T_VALUE_FLOAT_OFFSET;
>> +            data = FIELD_PREP(ADT7316_AD_MSB_MASK, msb);
>>              data |= lsb & ADT7316_LSB_IN_TEMP_MASK;
>
> Ditto and so on...
>

The main purpose of this patch was to delete the offset/shift macros,
so I only applied the bitfield macros to areas that were using those
offsets or shifts. I left that existing bitwise operation there because
it wasn't affected by the offsets or shifts. If you'd prefer, I can
update it in this case for the sake of symmetry.

Thanks,
Michael Harris