Re: [PATCH v2 2/3] iio: light: gp2ap002: add led power supply

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] iio: light: gp2ap002: regulator_bulk_disable() before unregister in remove
- [Low] iio: light: gp2ap002: missing colon in struct gp2ap002 kernel-doc comment
--

commit 1b407693e15c0943285ade4ca753202aa66cebe1
Author: Christoph Rudorff <chris@rudorff.com>

iio: light: gp2ap002: add led power supply

This commit converts the gp2ap002 light and proximity sensor driver to use
the bulk regulator API and adds support for an optional vled power supply.

> diff --git a/drivers/iio/light/gp2ap002.c b/drivers/iio/light/gp2ap002.c
> index de29081fd3b20..5e4599e403761 100644
> --- a/drivers/iio/light/gp2ap002.c
> +++ b/drivers/iio/light/gp2ap002.c
> @@ -123,12 +123,26 @@

[ ... ]

>  /**
>   * struct gp2ap002 - GP2AP002 state
>   * @map: regmap pointer for the i2c regmap
>   * @dev: pointer to parent device
> - * @vdd: regulator controlling VDD
> - * @vio: regulator controlling VIO
> + * @supplies bulk regulator data

[Severity: Low]
Should @supplies have a colon after it so kernel-doc does not warn about an
undescribed struct member in struct gp2ap002?

>   * @alsout: IIO ADC channel to convert the ALSOUT signal
>   * @hys_far: hysteresis control from device tree
>   * @hys_close: hysteresis control from device tree

[ ... ]

> @@ -632,9 +650,8 @@ static void gp2ap002_remove(struct i2c_client *client)
>  	pm_runtime_get_sync(dev);
>  	pm_runtime_put_noidle(dev);
>  	pm_runtime_disable(dev);
> +	regulator_bulk_disable(NUM_SUPPLIES, gp2ap002->supplies);
>  	iio_device_unregister(indio_dev);
> -	regulator_disable(gp2ap002->vio);
> -	regulator_disable(gp2ap002->vdd);
>  }

[Severity: Medium]
Should regulator_bulk_disable() remain after iio_device_unregister() in
gp2ap002_remove()?

In gp2ap002_remove(), pm_runtime_get_sync() invokes
gp2ap002_runtime_resume(), which powers on the sensor, initializes it into
operating mode via gp2ap002_init(), and enables gp2ap002->irq.

Calling regulator_bulk_disable() before iio_device_unregister() cuts power
to the sensor while the IIO userspace interfaces and the threaded interrupt
handler gp2ap002_prox_irq() are still active.

Until iio_device_unregister() completes, userspace reads through
gp2ap002_read_raw() or event configuration changes through
gp2ap002_write_event_config() can run after pm_runtime_disable() and access
the unpowered sensor:

drivers/iio/light/gp2ap002.c:gp2ap002_read_raw() {
	...
	pm_runtime_get_sync(gp2ap002->dev);

	switch (mask) {
	case IIO_CHAN_INFO_RAW:
		switch (chan->type) {
		case IIO_LIGHT:
			ret = gp2ap002_get_lux(gp2ap002);
	...
}

Similarly, gp2ap002_prox_irq() remains enabled and can fire (including if
the VOUT line glitches when VIO and VDD power down), performing I2C register
reads and writes on the unpowered chip:

drivers/iio/light/gp2ap002.c:gp2ap002_prox_irq() {
	...
	ret = regmap_read(gp2ap002->map, GP2AP002_PROX, &val);
	if (ret) {
		dev_err(gp2ap002->dev, "error reading proximity\n");
		goto err_retrig;
	}
	...
}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017-iio-light-gp2ap002-v2-0-d6879767a446@rudorff.com?part=2