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

[Waqar Hameed <waqar.hameed@axis.com>]

On Sat, Jun 28, 2025 at 18:11 +0100 Jonathan Cameron <jic23@kernel.org> wrote:

>> >> +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 ;)

Alright, I don't really have any strong opinions on this. Let's use
`-EINVAL`.

>> >> +				       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!

Right, I'll send a clean-up patch for those then.