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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 19 Mar 2026 22:22:16 +0530
Nikhil Gautam <nikhilgtr@gmail.com> 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>
> ---
>  drivers/iio/dac/mcp4821.c | 140 ++++++++++++++++++++++++++++----------
>  1 file changed, 104 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iio/dac/mcp4821.c b/drivers/iio/dac/mcp4821.c
> index 748bdca9a964..f9f97917bc9c 100644
> --- a/drivers/iio/dac/mcp4821.c
> +++ b/drivers/iio/dac/mcp4821.c
> @@ -12,26 +12,43 @@
>   *	MCP48x2: https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf
>   *
>   * TODO:
> - *	- Configurable gain
>   *	- Regulator control
>   */
>  
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/spi/spi.h>
> -
> -#include <linux/iio/iio.h>
> -#include <linux/iio/types.h>
> -
Hi Nikhil,

Don't do code recorganization in a patch doing anything else.
Also, it's fairly common convention to have subsystem specific headers
in a block at the end for a driver in that subsystem.
>  #include <linux/unaligned.h>
This is the oddity.  Was a result of a mass change from
asm/unaligned.h to this that didn't include reordering headers.

Given you are touching the driver anyway, it's fine to move that up to
under spi.h, but should still be a seperate patch.


>  
>  #define MCP4821_ACTIVE_MODE BIT(12)
>  #define MCP4802_SECOND_CHAN BIT(15)
> +#define MCP4821_GAIN_ENABLE BIT(13)

Put this in order. So above MCP4802_SECOND_CHAN
>  
> -/* 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 {
>  	ID_MCP4801,
>  	ID_MCP4802,
>  	ID_MCP4811,
> @@ -43,6 +60,7 @@ enum mcp4821_supported_drvice_ids {
>  struct mcp4821_state {
>  	struct spi_device *spi;
>  	u16 dac_value[2];
> +	bool gain_1x;
>  };
>  
>  struct mcp4821_chip_info {
> @@ -51,16 +69,19 @@ struct mcp4821_chip_info {
>  	const struct iio_chan_spec channels[2];
>  };
>  
> -#define MCP4821_CHAN(channel_id, resolution)                          \
> -	{                                                             \
> -		.type = IIO_VOLTAGE, .output = 1, .indexed = 1,       \
> -		.channel = (channel_id),                              \
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),         \
> -		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> -		.scan_type = {                                        \
> -			.realbits = (resolution),                     \
> -			.shift = 12 - (resolution),                   \
> -		},                                                    \
> +#define MCP4821_CHAN(channel_id, resolution)					\
> +	{															\
> +		.type = IIO_VOLTAGE, .output = 1, .indexed = 1,			\
> +		.channel = (channel_id),								\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
> +			BIT(IIO_CHAN_INFO_CALIBSCALE),						\
> +	    .info_mask_shared_by_type_available =					\
> +			BIT(IIO_CHAN_INFO_CALIBSCALE),						\
> +		.scan_type = {											\
> +			.realbits = (resolution),							\
> +			.shift = 12 - (resolution),							\
> +		},														\
>  	}
No idea what went wrong formatting wise here, but that needs to go back to normal!

Why is it calibscale as opposed to scale?  A x2 multiplier presumably affects the
scale userspace should apply? calibbscale is for adjusting due to minor device differences
not this sort of major range adjustment.

>  
>  static const struct mcp4821_chip_info mcp4821_chip_info_table[6] = {
> @@ -122,13 +143,25 @@ 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);
> +		if (state->gain_1x)
> +			*val = MCP4821_VREF_MV;
> +		else
> +			*val = MCP4821_VREF_MV * 2;
>  		*val2 = chan->scan_type.realbits;
>  		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		state = iio_priv(indio_dev);
> +		*val = state->gain_1x ? 1 : 2;
> +		return IIO_VAL_INT;
> +
>  	default:
> -		return -EINVAL;
> +		break;
>  	}
> +	return -EINVAL;

Why?  Original code was at least as good if not better.

>  }
>  
>  static int mcp4821_write_raw(struct iio_dev *indio_dev,
> @@ -140,34 +173,68 @@ static int mcp4821_write_raw(struct iio_dev *indio_dev,
>  	__be16 write_buffer;
>  	int ret;
>  
> -	if (val2 != 0)
> -		return -EINVAL;
> +	switch (mask) {
>  
> -	if (val < 0 || val >= BIT(chan->scan_type.realbits))
> -		return -EINVAL;
> +	case IIO_CHAN_INFO_RAW:
>  
> -	if (mask != IIO_CHAN_INFO_RAW)
> -		return -EINVAL;
> +		if (val2 != 0)
> +			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;
> +		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_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_CALIBSCALE:
> +		if (val == 1)
> +			state->gain_1x = true;
> +		else if (val == 2)
> +			state->gain_1x = false;
> +		else
> +			return -EINVAL;
> +		return 0;
> +	default:
> +		break;
>  	}
> +	return -EINVAL;
Move that up into the default. 

> +}

Thanks,

Jonathan