Message-ID: <20250706121117.75665bb0@jic23-huawei>
Date: Sun, 6 Jul 2025 12:11:17 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Waqar Hameed <waqar.hameed@...s.com>
Cc: David Lechner <dlechner@...libre.com>, Nuno Sá
 <nuno.sa@...log.com>, "Andy Shevchenko" <andy@...nel.org>,
 <kernel@...s.com>, <linux-kernel@...r.kernel.org>,
 <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v3 3/3] iio: Add driver for Nicera D3-323-AA PIR sensor

On Fri, 4 Jul 2025 18:14:38 +0200
Waqar Hameed <waqar.hameed@...s.com> wrote:

> 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.
> 
> 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@...s.com>
One suggestion inline on providing more information on the 'why' behind
the regulator handling.

I want to leave this on list anyway to give more time for other reviews,
but if nothing else comes up and you are happy with my description I can
tweak this whilst applying.

Jonathan

> ---
>  drivers/iio/proximity/Kconfig   |   9 +
>  drivers/iio/proximity/Makefile  |   1 +
>  drivers/iio/proximity/d3323aa.c | 814 ++++++++++++++++++++++++++++++++
>  3 files changed, 824 insertions(+)
>  create mode 100644 drivers/iio/proximity/d3323aa.c
> 
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index a562a78b7d0d..6070974c2c85 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -32,6 +32,15 @@ config CROS_EC_MKBP_PROXIMITY
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called cros_ec_mkbp_proximity.
>  
> +config D3323AA
> +	tristate "Nicera (Nippon Ceramic Co.) D3-323-AA PIR sensor"
> +	depends on GPIOLIB
> +	help
> +	  Say Y here to build a driver for the Nicera D3-323-AA PIR sensor.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called d3323aa.
> +
>  config HX9023S
>  	tristate "TYHX HX9023S SAR sensor"
>  	select IIO_BUFFER
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index c5e76995764a..152034d38c49 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -6,6 +6,7 @@
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AS3935)		+= as3935.o
>  obj-$(CONFIG_CROS_EC_MKBP_PROXIMITY) += cros_ec_mkbp_proximity.o
> +obj-$(CONFIG_D3323AA)		+= d3323aa.o
>  obj-$(CONFIG_HX9023S)		+= hx9023s.o
>  obj-$(CONFIG_IRSD200)		+= irsd200.o
>  obj-$(CONFIG_ISL29501)		+= isl29501.o
> diff --git a/drivers/iio/proximity/d3323aa.c b/drivers/iio/proximity/d3323aa.c
> new file mode 100644
> index 000000000000..b1bc3204c0c0
> --- /dev/null
> +++ b/drivers/iio/proximity/d3323aa.c
> @@ -0,0 +1,814 @@


> +static void d3323aa_disable_regulator(void *indata)
> +{
> +	struct d3323aa_data *data = indata;
> +	int ret;
> +
> +	/*
> +	 * During probe() the regulator may be disabled. It is enabled during
> +	 * device setup (in d3323aa_reset(), where it is also briefly disabled).
> +	 * The check is therefore needed in order to have balanced
> +	 * regulator_enable/disable() calls.
> +	 */
> +	if (!regulator_is_enabled(data->regulator_vdd))
> +		return;
> +
> +	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 device *dev = &pdev->dev;
> +	struct d3323aa_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "Could not allocate iio device\n");
> +
> +	data = iio_priv(indio_dev);
> +	data->dev = dev;
> +
> +	init_completion(&data->reset_completion);
> +
> +	ret = devm_mutex_init(dev, &data->statevar_lock);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Could not initialize mutex\n");
> +
> +	data->regulator_vdd = devm_regulator_get_exclusive(dev, "vdd");
> +	if (IS_ERR(data->regulator_vdd))
> +		return dev_err_probe(dev, PTR_ERR(data->regulator_vdd),
> +				     "Could not get regulator\n");
> +
> +	/*
> +	 * The regulator will be enabled during the device setup below (in
> +	 * d3323aa_reset()). Note that d3323aa_disable_regulator() also checks
> +	 * for the regulator state.

This comment doesn't explain why you do this here as opposed to after
reset.  Key is that there are complex paths in which the regulator is disabled
that are unrelated to probe()/remove()  Talk about those rather than why
this 'works'.  It's the why that matters in a comment more than the how.

If nothing else comes up in review, I can chagne this to something like

	* The regulator will be enabled for the first time during the
	* device setup below (in d3323aa_reset()). However parameter changes
	* from userspace can require a temporary disable of the regulator.
	* To avoid complex handling of state, use a callback that will disable
	* the regulator if it happens to be enabled at time of devm unwind.
	*/

> +	ret = d3323aa_setup(indio_dev, D3323AA_LP_FILTER_FREQ_DEFAULT_IDX,
> +			    D3323AA_FILTER_GAIN_DEFAULT_IDX,
> +			    D3323AA_THRESH_DEFAULT_VAL);
> +	if (ret)
> +		return ret;

Powered by blists - more mailing lists