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

[Waqar Hameed]

On Sun, Jul 06, 2025 at 12:11 +0100 Jonathan Cameron <jic23@xxxxxxxxxx> 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!

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