Re: [PATCH v3 3/6] iio: adc: ad7173: refactor ain and vref selection

[Jonathan Cameron]

On Fri, 31 May 2024 09:10:43 +0200
Nuno Sá <noname.nuno@xxxxxxxxx> wrote:

> On Thu, 2024-05-30 at 17:45 +0300, Ceclan, Dumitru wrote:
> > On 29/05/2024 15:49, Nuno Sá wrote:  
> > > On Wed, 2024-05-29 at 14:27 +0200, Nuno Sá wrote:  
> > > > On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:  
> > > > > From: Dumitru Ceclan <dumitru.ceclan@xxxxxxxxxx>  
> > 
> > ...
> >   
> > > > > +static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> > > > > +					      unsigned int ain[2])  
> > > 
> > > Pass the pointer and size of it... Also, it should be made 'const'
> > >   
> > 
> > I'm learning here: what is the purpose of passing the size of it?
> > This is a specific case where the size will always be 2
> >   
> 
> Basically readability... Yes, in this case it will be a stretch to assume we'll ever
> have anything bigger than 2 (so the scalability argument is not so applicable) so I'm
> ok if you don't pass the size. It's just I really dislike (as a practice) to have
> raw/magic numbers in the code. In here, it won't be that bad as by the context, one
> can easily understand the meaning of 2. Nevertheless, I would, still, at the very
> least consider to either use a #define or a better name for the iterator (anything
> more meaningful than 'i' so that it looks more understandable than 'i < 2')
> 

I'm late to the game, but I'd just split it into two parameters.
Code is shorter as well.

static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
					      unsigned int ain0, unsigned int ain1)
{
	if (ain0 >= st->info->num_inputs ||
	    ain1 >= st->info->num_inputs)
		return dev_err_probe(&st->sd.spi->dev, -EINVAL,
				     "Input pin number out of range for pair (%d %d).\n",
				     ain0, ain1);
	return 0;
}

> - Nuno Sá 
> >   
>