Re: [PATCH v2 3/4] staging: iio: adt7316: add config names to registers and reorder
From: Andy Shevchenko
Date: Fri Mar 06 2026 - 10:07:30 EST
On Thu, Mar 05, 2026 at 11:17:00PM -0800, Michael Harris wrote:
> Add config names to macros to make it more clear which register they're
> affecting. Also renamed ADT7316_EN to further clarify what
> it's enabling.
>
> Some macros were reordered, so that mask values were below the actual
> mask.
...
> +#define ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK 0x6
GENMASK*()
...
> +#define ADT7516_CONFIG1_SEL_EX_TEMP 0x4
> +#define ADT7516_CONFIG1_SEL_AIN3 0x8
Bits? Values?
...
> +#define ADT7316_CONFIG1_INT_EN 0x20
> +#define ADT7316_CONFIG1_INT_POLARITY 0x40
These looks like bits, why not BIT*()?
> +#define ADT7316_CONFIG1_PD 0x80
A bit?
...
> +#define ADT7316_CONFIG2_AD_SINGLE_CH_MASK 0x3
> +#define ADT7516_CONFIG2_AD_SINGLE_CH_MASK 0x7
GENMASK()
> +#define ADT7316_CONFIG2_AD_SINGLE_CH_VDD 0
> +#define ADT7316_CONFIG2_AD_SINGLE_CH_IN 1
> +#define ADT7316_CONFIG2_AD_SINGLE_CH_EX 2
> +#define ADT7516_CONFIG2_AD_SINGLE_CH_AIN1 2
> +#define ADT7516_CONFIG2_AD_SINGLE_CH_AIN2 3
> +#define ADT7516_CONFIG2_AD_SINGLE_CH_AIN3 4
> +#define ADT7516_CONFIG2_AD_SINGLE_CH_AIN4 5
> +#define ADT7316_CONFIG2_AD_SINGLE_CH_MODE 0x10
...
> +#define ADT7316_CONFIG2_DISABLE_AVERAGING 0x20
> +#define ADT7316_CONFIG2_EN_SMBUS_TIMEOUT 0x40
> +#define ADT7316_CONFIG2_RESET 0x80
Bits?
And so on...
...
Are you going to have this conversion in a separate patch?
...
> + switch (chip->config2 & ADT7516_CONFIG2_AD_SINGLE_CH_MASK) {
> + case ADT7316_CONFIG2_AD_SINGLE_CH_VDD:
> return sysfs_emit(buf, "0 - VDD\n");
> + case ADT7316_CONFIG2_AD_SINGLE_CH_IN:
> return sysfs_emit(buf, "1 - Internal Temperature\n");
> + case ADT7316_CONFIG2_AD_SINGLE_CH_EX:
> if (((chip->id & ID_FAMILY_MASK) == ID_ADT75XX) &&
> + (chip->config1 & ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK) == 0)
> return sysfs_emit(buf, "2 - AIN1\n");
>
> return sysfs_emit(buf, "2 - External Temperature\n");
> + case ADT7516_CONFIG2_AD_SINGLE_CH_AIN2:
> + if ((chip->config1 & ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK) == 0)
> return sysfs_emit(buf, "3 - AIN2\n");
>
> return sysfs_emit(buf, "N/A\n");
> + case ADT7516_CONFIG2_AD_SINGLE_CH_AIN3:
> + if (chip->config1 & ADT7516_CONFIG1_SEL_AIN3)
> return sysfs_emit(buf, "4 - AIN3\n");
>
> return sysfs_emit(buf, "N/A\n");
> + case ADT7516_CONFIG2_AD_SINGLE_CH_AIN4:
> return sysfs_emit(buf, "5 - AIN4\n");
> default:
> return sysfs_emit(buf, "N/A\n");
Side note:
Instead of this long switch I would rather introduce a clear string array per
each of the possible variant. Note, that linker will eliminate string duplication,
so it won't be a problem.
...
> + switch (chip->dac_config & ADT7316_DAC_CONFIG_EN_MODE_MASK) {
> + case ADT7316_DAC_CONFIG_EN_MODE_SINGLE:
> return sysfs_emit(buf,
> "0 - auto at any MSB DAC writing\n");
> + case ADT7316_DAC_CONFIG_EN_MODE_AB_CD:
> return sysfs_emit(buf,
> "1 - auto at MSB DAC AB and CD writing\n");
> + case ADT7316_DAC_CONFIG_EN_MODE_ABCD:
> return sysfs_emit(buf,
> "2 - auto at MSB DAC ABCD writing\n");
> + default: /* ADT7316_DAC_CONFIG_EN_MODE_LDAC */
> return sysfs_emit(buf, "3 - manual\n");
> }
Also can be just a string array with indexed access and a single call to
sysfs_emit(buf, "%s\n");
...
> static IIO_DEVICE_ATTR(ex_temp_AIN1, 0444, adt7316_show_ex_temp_AIN1,
Side note:
Can IIO_DEVICE_ATTR_RO() be used here (and in other similar cases)?
--
With Best Regards,
Andy Shevchenko