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

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

[Nikhil Gautam]

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>
-
 #include <linux/unaligned.h>
 
 #define MCP4821_ACTIVE_MODE BIT(12)
 #define MCP4802_SECOND_CHAN BIT(15)
+#define MCP4821_GAIN_ENABLE BIT(13)
 
-/* 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),							\
+		},														\
 	}
 
 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;
 }
 
 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;
+}
 
-	state->dac_value[chan->channel] = val;
+static const int mcp4821_gain_avail[] = {1, 2};
 
-	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_CALIBSCALE:
+		*vals = mcp4821_gain_avail;
+		*type = IIO_VAL_INT;
+		*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)
@@ -182,6 +249,7 @@ static int mcp4821_probe(struct spi_device *spi)
 
 	state = iio_priv(indio_dev);
 	state->spi = spi;
+	state->gain_1x = false; /* default 2x */
 
 	info = spi_get_device_match_data(spi);
 	indio_dev->name = info->name;
-- 
2.43.0

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

[Jonathan Cameron]

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

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

[Nikhil Gautam]

On Sat, Mar 21, 2026 at 06:07:07PM +0000, Jonathan Cameron wrote:
> On Thu, 19 Mar 2026 22:22:16 +0530
> Nikhil Gautam <nikhilgtr@gmail.com> wrote:
> 

Hi Jonathan,

Thank you for the detailed review, that’s very helpful.

> > +#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.
> 
Understood. I agree this wasn’t appropriate to mix with functional changes.
I’ll drop the header reordering from this patch, and if needed,
will send a separate cleanup patch that moves linux/unaligned.h to a more sensible location.

> 
> >  
> >  #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
> >  
Agreed. I’ll reorder the defines so the gain bit is placed logically alongside the other command bits,
before MCP4802_SECOND_CHAN. 

> > -#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!
>
Yes, that was accidental and slipped through during editing. I’ll restore the original formatting style

> 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.
>
Thanks for the clarification. You’re right exposing the gain selection via calibscale was incorrect.
The GA bit changes the voltage‑per‑LSB and therefore the scale userspace should apply.
I will switch this to use selectable IIO_CHAN_INFO_SCALE values with scale_available, and remove all use of calibscale.

> > +
> >  	default:
> > -		return -EINVAL;
> > +		break;
> >  	}
> > +	return -EINVAL;
> 
> Why?  Original code was at least as good if not better.
>
Agreed. The original structure was clearer.
I’ll move the EINVAL returns back into the default case and avoid the extra break.

> >  }
> >  
> > +	default:
> > +		break;
> >  	}
> > +	return -EINVAL;
> Move that up into the default. 
> 
Sure, will move that up.

I’ll post a revised version addressing all of the above.

Thanks again for the review.

Best regards,
Nikhil

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

[Nikhil Gautam]

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
---
 drivers/iio/dac/mcp4821.c | 105 ++++++++++++++++++++++++++++++--------
 1 file changed, 83 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/dac/mcp4821.c b/drivers/iio/dac/mcp4821.c
index 748bdca9a964..362ca12988dc 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 {
 	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);
+		if (state->gain_1x)
+			*val = MCP4821_VREF_MV;
+		else
+			*val = MCP4821_VREF_MV * 2;
 		*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;
+		return 0;
+	default:
+		return -EINVAL;
 	}
+}
 
-	state->dac_value[chan->channel] = val;
+static const int mcp4821_gain_avail[] = {1, 2};
 
-	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;
+		*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)
@@ -177,12 +235,15 @@ static int mcp4821_probe(struct spi_device *spi)
 	const struct mcp4821_chip_info *info;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
-	if (indio_dev == NULL)
+	if (!indio_dev)
 		return -ENOMEM;
 
 	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;
-- 
2.43.0

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

[Jonathan Cameron]

On Thu, 26 Mar 2026 12:42:42 +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>
Just one small thing from me.

>  static int mcp4821_probe(struct spi_device *spi)
> @@ -177,12 +235,15 @@ static int mcp4821_probe(struct spi_device *spi)
>  	const struct mcp4821_chip_info *info;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> -	if (indio_dev == NULL)
> +	if (!indio_dev)

Not related to rest of the patch. So doesn't belong in here.
Also generally not worth the noise of changing these.

>  		return -ENOMEM;
>  
>  	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;

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

[Nikhil Gautam]

On Thu, Mar 26, 2026 at 08:38:24PM +0000, Jonathan Cameron wrote:
> On Thu, 26 Mar 2026 12:42:42 +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>
> Just one small thing from me.
> 
> >  static int mcp4821_probe(struct spi_device *spi)
> > @@ -177,12 +235,15 @@ static int mcp4821_probe(struct spi_device *spi)
> >  	const struct mcp4821_chip_info *info;
> >  
> >  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> > -	if (indio_dev == NULL)
> > +	if (!indio_dev)
> 
> Not related to rest of the patch. So doesn't belong in here.
> Also generally not worth the noise of changing these.

Sure will remove this change in v3,
will avoid these kind of changes in my future patches
> 
> >  		return -ENOMEM;
> >  
> >  	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;
> 

Thanks,
Nikhil

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

[Nikhil Gautam]

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 {
 	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);
+		if (state->gain_1x)
+			*val = MCP4821_VREF_MV;
+		else
+			*val = MCP4821_VREF_MV * 2;
 		*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;
+		return 0;
+	default:
+		return -EINVAL;
 	}
+}
 
-	state->dac_value[chan->channel] = val;
+static const int mcp4821_gain_avail[] = {1, 2};
 
-	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;
+		*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;
-- 
2.43.0

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

[David Lechner]

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;

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

[Nikhil Gautam]

Hi David,

Thanks a lot for the detailed review and suggestions.

I understand the issues you pointed out. I will resend the next revision
as a new thread instead of replying, to avoid breaking tooling.

I will also split the unrelated changes into separate patches, starting
with moving the state initialization, and handle spelling fixes
independently.

Regarding the gain handling, your suggestion to use an integer value
instead of a boolean makes sense. I will update the implementation to
store the gain directly and simplify the logic accordingly.

For the scale handling, I see that the attribute should reflect the
actual output scale rather than the internal gain. I will update
scale_available to return the correct values based on Vref and use
IIO_VAL_FRACTIONAL_LOG2 as suggested, ensuring consistency between
scale and scale_available. I am thinking to return (2048/4096) Values 
based on gain selected
instead (1/2) Values in scale_available, so that user will have clear 
picture which gain
to be selected, else for (1/2) Values he always has to refer the driver.

Let me know you inputs.

I will incorporate all of these changes and send a revised version shortly.

Thanks again for your guidance.

Best regards,

Nikhil


On 12-04-2026 01:18 am, David Lechner wrote:
> 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;

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

[David Lechner]

On 4/12/26 10:23 AM, Nikhil Gautam wrote:
> Hi David,
> 
> Thanks a lot for the detailed review and suggestions.
> 
> I understand the issues you pointed out. I will resend the next revision
> as a new thread instead of replying, to avoid breaking tooling.
> 
> I will also split the unrelated changes into separate patches, starting
> with moving the state initialization, and handle spelling fixes
> independently.
> 
> Regarding the gain handling, your suggestion to use an integer value
> instead of a boolean makes sense. I will update the implementation to
> store the gain directly and simplify the logic accordingly.
> 

To save time, you don't need to comment on things you agree with. It
makes it hard to find any important parts where you might be asking
for additional feedback.

And trim out irrelevant quoted bits when replying. It saves time too.

And please don't top post.

>>> +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.

Instead, reply inline where the context makes sense like this:

> I am thinking to return (2048/4096) Values based on gain selected
> instead (1/2) Values in scale_available, so that user will have clear picture which gain
> to be selected, else for (1/2) Values he always has to refer the driver.
> 

We have to follow the IIO ABI of what these attribute mean so that all drivers
work the same. We can't make up new rules for this driver.

For a 12-bit chip, `cat iio\:device0/out_voltage_scale_available` for this
driver should return `0.5 1` since the scale is either 2048 / (1 << 12)
or 4096 / (1 << 12).

These are exactly the values that must be written to
`iio\:device0/out_voltage_scale_available` to change the scale. And then
`cat iio\:device0/out_voltage_scale_available` must read back exactly what
was written.

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

[Jonathan Cameron]

On Sun, 12 Apr 2026 13:35:45 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 4/12/26 10:23 AM, Nikhil Gautam wrote:
> > Hi David,
> > 
> > Thanks a lot for the detailed review and suggestions.
> > 
> > I understand the issues you pointed out. I will resend the next revision
> > as a new thread instead of replying, to avoid breaking tooling.
> > 
> > I will also split the unrelated changes into separate patches, starting
> > with moving the state initialization, and handle spelling fixes
> > independently.
> > 
> > Regarding the gain handling, your suggestion to use an integer value
> > instead of a boolean makes sense. I will update the implementation to
> > store the gain directly and simplify the logic accordingly.
> >   
> 
> To save time, you don't need to comment on things you agree with. It
> makes it hard to find any important parts where you might be asking
> for additional feedback.
> 
> And trim out irrelevant quoted bits when replying. It saves time too.
> 
> And please don't top post.
> 
> >>> +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.  
> 
> Instead, reply inline where the context makes sense like this:
> 
> > I am thinking to return (2048/4096) Values based on gain selected
> > instead (1/2) Values in scale_available, so that user will have clear picture which gain
> > to be selected, else for (1/2) Values he always has to refer the driver.
> >   
> 
> We have to follow the IIO ABI of what these attribute mean so that all drivers
> work the same. We can't make up new rules for this driver.
> 
> For a 12-bit chip, `cat iio\:device0/out_voltage_scale_available` for this
> driver should return `0.5 1` since the scale is either 2048 / (1 << 12)
> or 4096 / (1 << 12).
> 
> These are exactly the values that must be written to
> `iio\:device0/out_voltage_scale_available` to change the scale. And then
> `cat iio\:device0/out_voltage_scale_available` must read back exactly what
> was written.

out_voltage_scale was intended here I think as _available attributes
are read only.


> 
>