Re: [PATCH v3] iio: dac: mcp4821: add configurable gain support

[David Lechner <dlechner@baylibre.com>]

Please don't send new revisions in reply to previous revisions. It breaks
some tools. Always start a new thread for a new revision.

On 3/27/26 1:18 AM, Nikhil Gautam wrote:
> Add support for configuring the DAC gain using the GA bit.
> Expose gain control via IIO_CHAN_INFO_CALIBSCALE.
> 
> Scale is updated dynamically based on selected gain:
> - 1x gain → 2.048V full-scale
> - 2x gain → 4.096V full-scale
> 
> Signed-off-by: Nikhil Gautam <nikhilgtr@gmail.com>
> ---
> Changes in v2:
> - Drop header reordering
> - Move MCP4821_GAIN_ENABLE define before MCP4802_SECOND_CHAN
> - Use IIO_CHAN_INFO_SCALE instead of CALIBSCALE since full scale changes
> - Restore original -EINVAL handling in default cases
> 
> Changes in v3:
> - Restored NULL check in indio_dev alloc
> ---
>  drivers/iio/dac/mcp4821.c | 103 ++++++++++++++++++++++++++++++--------
>  1 file changed, 82 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/dac/mcp4821.c b/drivers/iio/dac/mcp4821.c
> index 748bdca9a964..9160812219f9 100644
> --- a/drivers/iio/dac/mcp4821.c
> +++ b/drivers/iio/dac/mcp4821.c
> @@ -12,7 +12,6 @@
>   *	MCP48x2: https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf
>   *
>   * TODO:
> - *	- Configurable gain
>   *	- Regulator control
>   */
>  
> @@ -26,12 +25,32 @@
>  #include <linux/unaligned.h>
>  
>  #define MCP4821_ACTIVE_MODE BIT(12)
> +#define MCP4821_GAIN_ENABLE BIT(13)
>  #define MCP4802_SECOND_CHAN BIT(15)
>  
> -/* DAC uses an internal Voltage reference of 4.096V at a gain of 2x */
> -#define MCP4821_2X_GAIN_VREF_MV 4096
> +/* DAC uses an internal Voltage reference of 2.048V */
> +#define MCP4821_VREF_MV 2048
>  
> -enum mcp4821_supported_drvice_ids {
> +/*
> + * MCP48xx DAC output:
> + *
> + * Vout = (Vref * D / 2^N) * G
> + *
> + * where:
> + *  - Vref = 2.048V (internal reference)
> + *  - N = DAC resolution (12 bits for MCP4821)
> + *  - G = gain selection:
> + *        1x when GA bit = 1
> + *        2x when GA bit = 0 (default)
> + *
> + * Therefore full-scale voltage is:
> + *  - 1x gain: 2.048V
> + *  - 2x gain: 4.096V
> + *
> + * Scale = Vfull-scale / 2^N
> + */
> +
> +enum mcp4821_supported_device_ids {

Unrelated change. Spelling error should be fixed in a separate patch.

>  	ID_MCP4801,
>  	ID_MCP4802,
>  	ID_MCP4811,
> @@ -43,6 +62,7 @@ enum mcp4821_supported_drvice_ids {
>  struct mcp4821_state {
>  	struct spi_device *spi;
>  	u16 dac_value[2];
> +	bool gain_1x;
>  };
>  
>  struct mcp4821_chip_info {
> @@ -57,6 +77,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),                   \
> @@ -122,8 +143,13 @@ static int mcp4821_read_raw(struct iio_dev *indio_dev,
>  		state = iio_priv(indio_dev);
>  		*val = state->dac_value[chan->channel];
>  		return IIO_VAL_INT;
> +
>  	case IIO_CHAN_INFO_SCALE:
> -		*val = MCP4821_2X_GAIN_VREF_MV;


> +		state = iio_priv(indio_dev);

I would start with a separate patch to move setting state to where it
is declared so that we don't have to duplicate this line in each case.


> +		if (state->gain_1x)
> +			*val = MCP4821_VREF_MV;
> +		else
> +			*val = MCP4821_VREF_MV * 2;

Why not make the variable store the gain value instead of being bool?

Then this can just be:

		*val = MCP4821_VREF_MV * state->gain;

>  		*val2 = chan->scan_type.realbits;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  	default:
> @@ -140,34 +166,66 @@ static int mcp4821_write_raw(struct iio_dev *indio_dev,
>  	__be16 write_buffer;
>  	int ret;
>  
> -	if (val2 != 0)
> -		return -EINVAL;
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
>  
> -	if (val < 0 || val >= BIT(chan->scan_type.realbits))
> -		return -EINVAL;
> +		if (val2 != 0)
> +			return -EINVAL;
>  
> -	if (mask != IIO_CHAN_INFO_RAW)
> -		return -EINVAL;
> +		if (val < 0 || val >= BIT(chan->scan_type.realbits))
> +			return -EINVAL;
> +
> +		write_val = MCP4821_ACTIVE_MODE | val << chan->scan_type.shift;
> +		if (chan->channel)
> +			write_val |= MCP4802_SECOND_CHAN;
> +		if (state->gain_1x)
> +			write_val |= MCP4821_GAIN_ENABLE;
>  
> -	write_val = MCP4821_ACTIVE_MODE | val << chan->scan_type.shift;
> -	if (chan->channel)
> -		write_val |= MCP4802_SECOND_CHAN;
> +		write_buffer = cpu_to_be16(write_val);
> +		ret = spi_write(state->spi, &write_buffer, sizeof(write_buffer));
> +		if (ret) {
> +			dev_err(&state->spi->dev, "Failed to write to device: %d", ret);
> +			return ret;
> +		}
>  
> -	write_buffer = cpu_to_be16(write_val);
> -	ret = spi_write(state->spi, &write_buffer, sizeof(write_buffer));
> -	if (ret) {
> -		dev_err(&state->spi->dev, "Failed to write to device: %d", ret);
> -		return ret;
> +		state->dac_value[chan->channel] = val;
> +		return 0;
> +
> +	case IIO_CHAN_INFO_SCALE:


> +		if (val == 1)
> +			state->gain_1x = true;
> +		else if (val == 2)
> +			state->gain_1x = false;
> +		else
> +			return -EINVAL;

And this can be:

		if (!in_range(val, 1, 2))
			return -EINVAL;

		state->gain = val;

> +		return 0;
> +	default:
> +		return -EINVAL;
>  	}
> +}
>  
> -	state->dac_value[chan->channel] = val;
> +static const int mcp4821_gain_avail[] = {1, 2};

The attribute is the scale attribute, not gain, so these need to
be: { MCP4821_VREF_MV, 2 * MCP4821_VREF_MV } combined with...

>  
> -	return 0;
> +static int mcp4821_read_avail(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      const int **vals, int *type, int *length,
> +			      long info)
> +{
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = mcp4821_gain_avail;
> +		*type = IIO_VAL_INT;

... IIO_VAL_FRACTIONAL_LOG2.

The idea is that the scale_available attribute should return the same
values as the scale attribute.

> +		*length = 2;
> +		return IIO_AVAIL_LIST;

> +	default:
> +		return -EINVAL;
> +	}
>  }
>  
>  static const struct iio_info mcp4821_info = {
>  	.read_raw = &mcp4821_read_raw,
>  	.write_raw = &mcp4821_write_raw,
> +	.read_avail = &mcp4821_read_avail,
>  };
>  
>  static int mcp4821_probe(struct spi_device *spi)
> @@ -183,6 +241,9 @@ 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*/
> +	state->gain_1x = false;
> +
>  	info = spi_get_device_match_data(spi);
>  	indio_dev->name = info->name;
>  	indio_dev->info = &mcp4821_info;