[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250628181129.08f55227@jic23-huawei>
Date: Sat, 28 Jun 2025 18:11:29 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Waqar Hameed <waqar.hameed@...s.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, <kernel@...s.com>,
<linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] iio: Add driver for Nicera D3-323-AA PIR sensor
> >> +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
Powered by blists - more mailing lists