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

[Nikhil Gautam <nikhilgtr@gmail.com>]

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