Re: [PATCH v2 3/3] iio: Add driver for Nicera D3-323-AA PIR sensor

[Jonathan Cameron <jic23@kernel.org>]

> >> +static int d3323aa_set_lp_filter_freq(struct iio_dev *indio_dev, const int val,
> >> +				      int val2)
> >> +{
> >> +	struct d3323aa_data *data = iio_priv(indio_dev);
> >> +	size_t idx;
> >> +
> >> +	/* Truncate fractional part to one digit. */
> >> +	val2 /= 100000;
> >> +
> >> +	for (idx = 0; idx < ARRAY_SIZE(d3323aa_lp_filter_freq); ++idx) {
> >> +		int integer = d3323aa_lp_filter_freq[idx][0] /
> >> +			      d3323aa_lp_filter_freq[idx][1];
> >> +		int fract = d3323aa_lp_filter_freq[idx][0] %
> >> +			    d3323aa_lp_filter_freq[idx][1];
> >> +
> >> +		if (val == integer && val2 == fract)
> >> +			break;
> >> +	}
> >> +
> >> +	if (idx == ARRAY_SIZE(d3323aa_lp_filter_freq))
> >> +		return -ERANGE;  
> >
> > It's a patch not a range check, so -EINVAL may make more sense as
> > a return value.  
> 
> Hm, `ERANGE`s message does say "*Numerical result* out of range...",
> which I can see is not really applicable here (strictly speaking, we are
> really not "calculating" anything...). However, isn't `EDOM` actually
> the better alternative here? Consider the following
> 
>   echo a > in_proximity_hardwaregain
>   sh: write error: Invalid argument
> 
>   echo 1234 > in_proximity_hardwaregain
>   sh: write error: Numerical argument out of domain

I'd still stick to -EINVAL as correct if not that informative simply
because EDOM is very rarely used (wasn't one I even knew existed
until today ;)

> >> +				       data);
> >> +	if (ret)
> >> +		return dev_err_probe(
> >> +			data->dev, ret,
> >> +			"Could not add disable regulator action\n");  
> > Odd formatting.
> >
> > 		return dev_err_probe(dev, ret,
> > 				     "Could not add disable regulator action\n");
> >
> > It's fine to go a little over 80 chars if it helps readability and here I think
> > it does. However it is vanishingly unlikely this would fail (as it basically means
> > memory allocation is failing in which case not much is going to work) so
> > common practice is not to bother with prints for failed devm_add_action_or_reset().
> > Those prints do make sense for devm calls that are doing something more complex
> > though so keep the rest.
> >
> > 	if (ret)
> > 		return ret;
> >
> > is fine here.  
> 
> `clang-format` trying its best here. Let's just remove the print then.
> 
> There are a bunch drivers in iio that are printing in this
> devm_add_action_or_reset()-error-path (though it looks like the majority
> are not doing that). Probably not really worth changing those; in case
> someone would really "miss" the (very unlikely) prints.

If they are doing dev_err_probe() it won't print anyway as that only 
returns -ENOMEM which dev_err_probe() doesn't print on simply because
you get lots of info if a memory allocation fails anyway.

https://elixir.bootlin.com/linux/v6.15.3/source/drivers/base/core.c#L5017

So on that basis it would be a sensible I think to do a cleanup patch set
to drop that particular devm_add_action_or_reset() / dev_err_probe()
combination.  If it were just a case of unlikely (rather than impossible)
I'd agree that it wasn't worth the churn!

Jonathan