Re: [PATCH v5 3/3] iio: dac: mcp4821: add configurable gain support

[David Lechner <dlechner@baylibre.com>]

On 4/14/26 4:22 AM, Nikhil Gautam wrote:
> Add support for configuring the DAC gain using the GA bit and
> update scale handling
> 

...

> +enum gain_modes {
> +	MCP4821_GAIN_X1 = 1,
> +	MCP4821_GAIN_X2 = 2,

We try to avoid making enums that just map the number 1 to 1
and so on.

> +};
> +
>  struct mcp4821_state {
>  	struct spi_device *spi;
>  	u16 dac_value[2];
> +	int gain;
> +	int avail_gain[4];

Would make more sense to call this scale_avail.

>  };
>  
>  struct mcp4821_chip_info {
> @@ -57,6 +83,7 @@ struct mcp4821_chip_info {
>  		.channel = (channel_id),                              \
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),         \
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +		.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), \
>  		.scan_type = {                                        \
>  			.realbits = (resolution),                     \
>  			.shift = 12 - (resolution),                   \
> @@ -121,8 +148,9 @@ static int mcp4821_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_RAW:
>  		*val = state->dac_value[chan->channel];
>  		return IIO_VAL_INT;
> +
>  	case IIO_CHAN_INFO_SCALE:
> -		*val = MCP4821_2X_GAIN_VREF_MV;
> +		*val = MCP4821_VREF_MV * state->gain;
>  		*val2 = chan->scan_type.realbits;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  	default:
> @@ -130,6 +158,17 @@ static int mcp4821_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static void mcp4821_calc_scale(int vref_mv, int resolution,
> +				int *val, int *val2)
> +{
> +	s64 tmp;
> +	int micro;
> +
> +	tmp = (s64)vref_mv * 1000000LL >> resolution;

Use MICRO instead of writing lots of 0s.

> +	*val = div_s64_rem(tmp, 1000000LL, &micro);
> +	*val2 = micro;

Technically, IIO_VAL_FRACTIONAL_LOG2 can print up to 9 digits after
the decimal, so this would be safer to do nano instead of micro.

For that, we need to also implement a write_raw_get_fmt() callback.

> +}
> +
>  static int mcp4821_write_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan, int val,
>  			     int val2, long mask)

...

> @@ -182,12 +271,15 @@ static int mcp4821_probe(struct spi_device *spi)
>  	state = iio_priv(indio_dev);
>  	state->spi = spi;
>  
> +	/* default gain is 2x as GA bit is active low*/

Needs space before ending */

Saying "active low" is a bit odd. I think the main point of selecting
this value is so we don't break userspace that may already be depending
on it being the default.

> +	state->gain = MCP4821_GAIN_X2;
>  	info = spi_get_device_match_data(spi);
>  	indio_dev->name = info->name;
>  	indio_dev->info = &mcp4821_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = info->channels;
>  	indio_dev->num_channels = info->num_channels;
> +	mcp4821_init_avail_gain(state, info->channels[0].scan_type.realbits);
>  
>  	return devm_iio_device_register(&spi->dev, indio_dev);
>  }