Re: [PATCH v4 3/3] iio: dac: mcp4821: add configurable gain and fix scale handling

[David Lechner <dlechner@baylibre.com>]

Subject sounds like two changes in one patch.

If we really are fixing the existing scale handling, it should be
a separate patch. Or if we are not, the subject line shouldn't 
say that.

On 4/13/26 4:44 AM, Nikhil Gautam wrote:
> Add support for configuring the DAC gain using the GA bit and
> update scale handling to follow the IIO ABI.
> 
> The MCP4821 supports two gain settings:
> - 1x gain → 2.048V full-scale
> - 2x gain → 4.096V full-scale
> 
> Scale is now exposed via IIO_CHAN_INFO_SCALE and reflects the

The driver already implemented the scale attribute, so this doesn't
sound right. Maybe you mean scale_available?

> selected gain. The driver returns scale using
> IIO_VAL_FRACTIONAL_LOG2 and provides scale_available with the
> corresponding values.

It already did this.

> 
> Writes to the scale attribute are validated and mapped to the

Writing wasn't supported before, so would be more clear to say
that we added support for writing to the scale attribute.

> appropriate gain setting. Only supported scale values are accepted,
> ensuring consistency between scale and scale_available.
> 
> Signed-off-by: Nikhil Gautam <nikhilgtr@gmail.com>
> ---

Code looks mostly good now, just one thing...

> @@ -140,34 +162,72 @@ 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;
>  
> -	write_val = MCP4821_ACTIVE_MODE | val << chan->scan_type.shift;
> -	if (chan->channel)
> -		write_val |= MCP4802_SECOND_CHAN;
> +		/* GA bit = 1 -> 1x gain */
> +		if (state->gain == 1)
> +			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_SCALE:
> +
> +		if (val == 0 && val2 == 500000)

This only works when realbits == 12. We need to make this more
general so it works for all supported chips in this driver. There
are also 8 and 10-bit chips.

> +			state->gain = 1;
> +		else if (val == 1 && val2 == 0)
> +			state->gain = 2;
> +		else
> +			return -EINVAL;
> +		return 0;
> +
> +	default:
> +		return -EINVAL;
>  	}
> +}
>