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

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

On Sun, May 11, 2025 at 09:57 +0200 Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

[...]

> Hi,

Hi Christophe! Thank you for your review!

>
> ...
>
>> +static int d3323aa_set_lp_filter_freq(unsigned long *regmap, const int val,
>> +				      int val2)
>> +{
>> +	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];
>
> Missing newline.

Sure, I'll add one.

>
>> +		if (val == integer && val2 == fract)
>> +			break;
>> +	}
>> +
>> +	if (idx == ARRAY_SIZE(d3323aa_lp_filter_freq))
>> +		return -ERANGE;
>> +
>> +	bitmap_write(regmap, d3323aa_lp_filter_regval[idx],
>> +		     D3323AA_REG_BIT_FILSEL0, D3323AA_FILTER_TYPE_NR_BITS);
>> +
>> +	return 0;
>> +}
>
> ...
>
>> +static int d3323aa_probe(struct platform_device *pdev)
>> +{
>> +	DECLARE_BITMAP(default_regmap, D3323AA_REG_NR_BITS);
>> +	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,
>> +				     "Could not allocate iio device\n");
>> +
>> +	data = iio_priv(indio_dev);
>> +	data->dev = &pdev->dev;
>> +	platform_set_drvdata(pdev, indio_dev);
>> +
>> +	init_completion(&data->reset_completion);
>> +
>> +	ret = devm_mutex_init(data->dev, &data->regmap_lock);
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret,
>> +				     "Could not initialize mutex (%d)\n", ret);
>
> No need to repeat the error code in the message, when using dev_err_probe().
>
> Same for all calls below.

Ah, of course! I'll remove it. (There is actually another driver that is
doing this, I'll update that one as well...)

>
>> +
>> +	/* Request GPIOs. */
>> +	data->gpiod_vdd = devm_gpiod_get(data->dev, "vdd", GPIOD_OUT_LOW);
>> +	if (IS_ERR(data->gpiod_vdd))
>> +		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_vdd),
>> +				     "Could not get GPIO vdd (%ld)\n",
>> +				     PTR_ERR(data->gpiod_vdd));
>> +
>> +	data->gpiod_clk_vout =
>> +		devm_gpiod_get(data->dev, "clk-vout", GPIOD_OUT_LOW);
>> +	if (IS_ERR(data->gpiod_clk_vout))
>> +		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_clk_vout),
>> +				     "Could not get GPIO clk-vout (%ld)\n",
>> +				     PTR_ERR(data->gpiod_clk_vout));
>> +
>> +	data->gpiod_data = devm_gpiod_get(data->dev, "data", GPIOD_OUT_LOW);
>> +	if (IS_ERR(data->gpiod_data))
>> +		return dev_err_probe(data->dev, PTR_ERR(data->gpiod_data),
>> +				     "Could not get GPIO data (%ld)\n",
>> +				     PTR_ERR(data->gpiod_data));
>> +
>> +	ret = gpiod_to_irq(data->gpiod_clk_vout);
>> +	if (ret < 0)
>> +		return dev_err_probe(data->dev, ret, "Could not get IRQ (%d)\n",
>> +				     ret);
>> +
>> +	data->irq = ret;
>> +
>> +	/* Do one setup with the default values. */
>> +	bitmap_zero(default_regmap, D3323AA_REG_NR_BITS);
>> +	d3323aa_set_threshold(default_regmap, D3323AA_THRESH_DEFAULT_VAL);
>> +	d3323aa_set_filter_gain(default_regmap,
>> +				D3323AA_FILTER_GAIN_DEFAULT_VAL);
>> +	ret = d3323aa_setup(indio_dev, default_regmap);
>> +	if (ret)
>> +		return ret;
>> +
>> +	indio_dev->info = &d3323aa_info;
>> +	indio_dev->name = D3323AA_DRV_NAME;
>> +	indio_dev->channels = d3323aa_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(d3323aa_channels);
>> +
>> +	ret = devm_iio_device_register(data->dev, indio_dev);
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret,
>> +				     "Could not register iio device (%d)\n",
>> +				     ret);
>> +
>> +	return 0;
>> +}
>
> ...
>
> CJ