Message-ID: <pndtt45aq6m.fsf@axis.com>
Date: Tue, 24 Jun 2025 21:35:13 +0200
From: Waqar Hameed <waqar.hameed@...s.com>
To: Jonathan Cameron <jic23@...nel.org>
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

On Sun, Jun 22, 2025 at 12:07 +0100 Jonathan Cameron <jic23@...nel.org> wrote:

> On Sun, 15 Jun 2025 00:14:05 +0200
> Waqar Hameed <waqar.hameed@...s.com> wrote:

[...]

>> +static irqreturn_t d3323aa_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct iio_dev *indio_dev = dev_id;
>> +	struct d3323aa_data *data = iio_priv(indio_dev);
>> +	enum iio_event_direction dir;
>> +	int val;
>> +
>> +	val = gpiod_get_value(data->gpiod_clkin_detectout);
>> +	if (val < 0) {
>> +		dev_err_ratelimited(data->dev,
>> +				    "Could not read from GPIO vout-clk (%d)\n",
>> +				    val);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	if (!data->detecting) {
>> +		/* Reset interrupt counting falling edges. */
>> +		if (!val && atomic_inc_return(&data->irq_reset_count) ==
>> +				    D3323AA_IRQ_RESET_COUNT)
> Odd line break
> 		if (!val &&
> 		    atomic_inc_return(&data->irq_reset_count) == D3323AA_IRQ_RESET_COUNT)
>
> Is a bit better though rather long line.  I'm not completely clear on why it needs
> to be an atomic counter though as the comment in reset() to to imply this
> can't race with the zeroing and no one else touches it.

Hm, now when we have unified the IRQ handlers, there is actually no need
for it to be atomic anymore. Since it is now "guarded" with an
if-statement on `data->detecting` and can't race. Good catch! Let's
convert `irq_reset_count` to an `u8`. This will also fix the formatting
when removing `atomic_inc_return()`.

>> +static int d3323aa_reset(struct iio_dev *indio_dev)
>> +{
>> +	struct d3323aa_data *data = iio_priv(indio_dev);
>> +	long time;
>> +	int ret;
>> +
>> +	if (regulator_is_enabled(data->regulator_vdd)) {
>
> Add a comment to say this check is only for the use in probe() where
> the power may not be on yet.  In other paths I think it will always
> be on.

Yeah, that might not be immediately obvious. Will add one.

>> +		ret = regulator_disable(data->regulator_vdd);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	/*
>> +	 * Datasheet says VDD needs to be low at least for 30 ms. Let's add a
>> +	 * couple more to allow VDD to completely discharge as well.
>> +	 */
>> +	msleep(30 + 5);
>
> Use fsleep here as well with the parameter value in usecs.  It will use msleep
> under the hood but we don't need to enforce that in the driver.  On some particular
> platform some other sleep call may be more suited.

I agree, that's a better approach. 

>> +
>> +	/*
>> +	 * After setting VDD to high, the device signals with
>
> This is a little odd.  vdd, as the regulator is currently low as you powered it down.
> This is referring I think to what happens later.  Edit the comment to make that
> clearer.  Someting like * When VDD is later enabled...

I can see how one can get a little confused by that. I'll rephrase it!

>> +static int d3323aa_setup(struct iio_dev *indio_dev, size_t lp_filter_freq_idx,
>> +			 size_t filter_gain_idx, u8 detect_thresh)
>> +{
>> +	DECLARE_BITMAP(write_regbitmap, D3323AA_REG_NR_BITS);
>> +	DECLARE_BITMAP(read_regbitmap, D3323AA_REG_NR_BITS);
>> +	struct d3323aa_data *data = iio_priv(indio_dev);
>> +	unsigned long start_time;
>> +	int ret;
>> +
>> +	ret = d3323aa_reset(indio_dev);
>> +	if (ret) {
>> +		if (ret != -ERESTARTSYS)
>> +			dev_err(data->dev, "Could not reset device (%d)\n",
>> +				ret);
>> +
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * Datasheet says to wait 10 us before setting the configuration.
>> +	 * Moreover, the total configuration should be done within
>> +	 * D3323AA_CONFIG_TIMEOUT ms. Clock it.
>> +	 */
>> +	usleep_range(10, 20);
>
> fsleep() preferred as that generalizes the tolerances fed to usleep_range()
> so we don't have to think if they are appropriate in each call.

As above, I agree.

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

>> +static void d3323aa_disable_regulator(void *indata)
>> +{
>> +	struct d3323aa_data *data = indata;
>> +	int ret;
>> +
>> +	if (!regulator_is_enabled(data->regulator_vdd))
>> +		return;
>> +
>
> This is unusual and I think an artefact of where you are registering
> the devm callback and the use of reset later.  Needs a comments
> at the very least to explain why the check is needed.

Correct! I'll add a comment clarifying this.

>
>> +	ret = regulator_disable(data->regulator_vdd);
>> +	if (ret)
>> +		dev_err(data->dev, "Could not disable regulator (%d)\n", ret);
>> +}
>> +
>> +static int d3323aa_probe(struct platform_device *pdev)
>> +{
>> +	struct d3323aa_data *data;
>> +	struct iio_dev *indio_dev;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>> +	if (!indio_dev)
>> +		return dev_err_probe(&pdev->dev, -ENOMEM,
>
> Given there is a lot of use of either pdev->dev or data->dev which is
> the same thing, I'd suggest a local declaration at the top and use that
> throughout.
>
> 	struct device *dev = &pdev->dev;

Alright, we can do that.

>> +				     "Could not allocate iio device\n");
>> +
>> +	data = iio_priv(indio_dev);
>> +	data->dev = &pdev->dev;
>> +
>> +	init_completion(&data->reset_completion);
>> +
>> +	ret = devm_mutex_init(data->dev, &data->statevar_lock);
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret,
>> +				     "Could not initialize mutex\n");
>> +
>> +	data->regulator_vdd = devm_regulator_get_exclusive(data->dev, "vdd");
>> +	if (IS_ERR(data->regulator_vdd))
>> +		return dev_err_probe(data->dev, PTR_ERR(data->regulator_vdd),
>> +				     "Could not get regulator\n");
>> +
>> +	ret = devm_add_action_or_reset(data->dev, d3323aa_disable_regulator,
>
> This is in a slightly odd place as you haven't turned it on yet. 
> At very least add a comment. 

Will do!

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

Powered by blists - more mailing lists