Re: [PATCH v3 12/13] iio: adc: ad4134: Support high-speed data capture

[Marcelo Schmitt]

On 09/29, sashiko-bot@xxxxxxxxxx wrote:
...
> > +#define AD4134_OFFLOAD_CHANNEL(_index) {					\
> > +	.type = IIO_VOLTAGE,							\
> > +	.indexed = 1,								\
> > +	.channel = (_index),							\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),				\
> 
> [Severity: Low]
> Is it intentional to expose the raw voltage attribute for offload channels?
> 
> The IIO_CHAN_INFO_RAW bit is set in the info_mask_separate here, exposing
> the sysfs attribute. However, ad4134_read_raw() is modified later in this
> patch to unconditionally return -EPROTONOSUPPORT for raw reads when offload
> is enabled. Does this clutter the interface with an attribute that is
> guaranteed to always fail?

Oh, I should've done that. Will change accordingly.


> > +	if (!in_range(freq_Hz, AD4134_MIN_ODR_FREQ_HZ,
> > +		      AD4134_MAX_ODR_FREQ_HZ - AD4134_MIN_ODR_FREQ_HZ))
> > +		return -ERANGE;
> 
> [Severity: Medium]
> Does this validation check inadvertently reject the maximum supported
> frequency?
> 
> The in_range(val, start, len) macro strictly checks if val < start + len.
> If a user requests exactly AD4134_MAX_ODR_FREQ_HZ, the condition evaluates
> to MAX < MAX (which is false) and incorrectly returns -ERANGE.
> 
For cases where the range doesn't start with 0 or 1, in_range() becomes a
bit harder to read.

if (freq_Hz < AD4134_MIN_ODR_FREQ_HZ || freq_Hz > AD4134_MAX_ODR_FREQ_HZ)
	return -ERANGE;

would have been simpler, IMHO.
Anyways, will fix this and all other issues reported.