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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 7 Jul 2025 10:46:09 +0200
Waqar Hameed <waqar.hameed@axis.com> wrote:

> On Sun, Jul 06, 2025 at 12:11 +0100 Jonathan Cameron <jic23@kernel.org> wrote:
> 
> [...]
> 
> > 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.  
> 
> Sure, we can let it breathe for a bit. I'm fine with you editing it
> while applying it (maybe also the minor format comment in the
> dt-bindings patch then?). Either way, if there is anything else you want
> me to do do, just tell! Thanks again Jonathan!
> 
I decided to be lazy and only tidied up the comment. The slightly odd formatting
in the dt binding can stay.

Applied to the togreg branch of iio.git and pushed out as testing for 0-day
to look at them.

J
> >
> > 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.
> > 	*/  
> 
> Ah, I see that I misunderstood you the first time! The comment looks
> fine to me.
> 
> >  
> >> +	ret = d3323aa_setup(indio_dev, D3323AA_LP_FILTER_FREQ_DEFAULT_IDX,
> >> +			    D3323AA_FILTER_GAIN_DEFAULT_IDX,
> >> +			    D3323AA_THRESH_DEFAULT_VAL);
> >> +	if (ret)
> >> +		return ret;