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

[Christophe JAILLET]

Le 09/05/2025 à 17:03, Waqar Hameed a écrit :
> Nicera D3-323-AA is a PIR sensor for human detection. It has support for
> raw data measurements and detection notification. The communication
> protocol is custom made and therefore needs to be GPIO bit banged.
>
> The device has two main settings that can be configured: a threshold
> value for detection and a band-pass filter. The configurable parameters
> for the band-pass filter are the high-pass and low-pass cutoff
> frequencies and its peak gain. Map these settings to the corresponding
> parameters in the `iio` framework.
>
> The low-pass and high-pass cutoff frequencies are pairwise for the
> different available filter types. Because of this, only allow to set the
> low-pass cutoff frequency from `sysfs` and use that to configure the
> corresponding high-pass cutoff frequency. This is sufficient to
> unambiguously choose a filter type.
>
> Raw data measurements can be obtained from the device. However, since we
> rely on bit banging, it will be rather cumbersome with buffer support.
> The main reason being that the data protocol has strict timing
> requirements (it's serial like UART), and it's mainly used during
> debugging since in real-world applications only the event notification
> is of importance. Therefore, only add support for events (for now).
>
> Signed-off-by: Waqar Hameed <waqar.hameed-VrBV9hrLPhE@xxxxxxxxxxxxxxxx>
> ---

Hi,

...

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

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

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