Re: [PATCH v8 6/6] iio: adc: ad7192: Add AD7194 support

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 14 May 2024 16:09:32 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Tue, May 14, 2024 at 03:02:22PM +0300, Alisa-Dariana Roman wrote:
> > Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> > dynamically configure them in the devicetree.
> > 
> > Add sigma_delta_info member to chip_info structure. Since AD7194 is the
> > only chip that has no channel sequencer, num_slots should remain
> > undefined.
> > 
> > Also modify config AD7192 description for better scaling.  
> 
> Some non-critical, mostly style related comments below.
> 
Tweaked a bit. And applied.  Please check the result in the testing branch
of iio.git.
> ...
> 
> This...
> 
> > +#define AD7194_CH(p)		(BIT(10) | AD7194_CH_POS(p))
> > +				  /* 10th bit corresponds to CON18(Pseudo) */  
> 
> ...should be (you have broken indentation on the comment, btw):
> 
> /* 10th bit corresponds to CON18(Pseudo) */
> #define AD7194_CH(p)		(BIT(10) | AD7194_CH_POS(p))
> 
> But no need to resend because of this, let's wait others to comment, and
> if everything fine I think Jonathan can massage this when applying.

Fixed.

> 
> ...
> 
> > +#define AD7194_CH_TEMP		0x100 /* Temp sensor */  
> 
> Not sure that the comment has any value here.
Dropped

> 
> ...
> 
> > +static int ad7194_validate_ain_channel(struct device *dev, u32 ain)
> > +{
> > +	if (!in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "Invalid AIN channel: %u\n", ain);
> > +
> > +	return 0;  
> 
> While this uses traditional pattern, it might be better looking in a form of
> 
> 	if (in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
> 		return 0;
> 
> 	return dev_err_probe(dev, -EINVAL, "Invalid AIN channel: %u\n", ain);
> 
> But at the same time I would rather expect this to be in the caller and here
> to have a boolean function
> 
Moved it.

> static bool ad7194_is_ain_channel_valid(struct device *dev, u32 ain)
> {
> 	return in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR);
> }
> 
> > +}  
> 
> ...
> 
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "Too many channels: %u\n", num_channels);  
> 
> 		return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n", num_channels);
> 
> ?
> 
> Or with limit
> 
> 		return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
> 				     num_channels);
> 
> 
This one.
> ...
> 
> > +	ad7194_channels = devm_kcalloc(dev, num_channels,
> > +				       sizeof(*ad7194_channels), GFP_KERNEL);  
> 
> 	ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels), GFP_KERNEL);
> 
> ?
> 
> Or
> 
> 	ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels),
> 				       GFP_KERNEL);
Nope. too long in either case.

> 
> ?
> 
> ...
> 
> > +	device_for_each_child_node_scoped(dev, child) {
> > +		ret = fwnode_property_read_u32_array(child, "diff-channels",
> > +						     ain, ARRAY_SIZE(ain));
> > +		if (ret == 0) {  
> 
> And here I would rather go for the traditional pattern, i.e.
> 
> 		if (ret) {
> 			...
> 		} else {
> 			...
> 		}
It's odd, as it's two good paths I've left this one alone.

> 
> > +			ret = ad7194_validate_ain_channel(dev, ain[0]);
> > +			if (ret)
> > +				return ret;
> > +
> > +			ret = ad7194_validate_ain_channel(dev, ain[1]);
> > +			if (ret)
> > +				return ret;
> > +
> > +			*ad7194_channels = ad7194_chan_diff;
> > +			ad7194_channels->scan_index = index++;
> > +			ad7194_channels->channel = ain[0];
> > +			ad7194_channels->channel2 = ain[1];
> > +			ad7194_channels->address = AD7194_DIFF_CH(ain[0], ain[1]);
> > +		} else {
> > +			ret = fwnode_property_read_u32(child, "single-channel",
> > +						       &ain[0]);
> > +			if (ret)
> > +				return dev_err_probe(dev, ret,
> > +						     "Missing channel property\n");
> > +
> > +			ret = ad7194_validate_ain_channel(dev, ain[0]);
> > +			if (ret)
> > +				return ret;
> > +
> > +			*ad7194_channels = ad7194_chan;
> > +			ad7194_channels->scan_index = index++;
> > +			ad7194_channels->channel = ain[0];
> > +			ad7194_channels->address = AD7194_CH(ain[0]);
> > +		}
> > +		ad7194_channels++;
> > +	}  
>