Re: [PATCH v2 3/4] staging: iio: adt7316: add config names to registers and reorder

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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