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

[Nikhil Gautam <nikhilgtr@gmail.com>]

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;