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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 16 Oct 2026 16:43:06 +0200
Christoph Rudorff <chris@rudorff.com> wrote:

> According to data sheet "Recommended external circuit"
> this got up to three supplies, we missed VLED
> 
> Now it works (tested on msm8916-samsung-j5):
> 
> $ cd /sys/bus/iio/devices/iio\:device1
> $ echo 1 > events/in_proximity_thresh_either_en
> 
> $ iio_event_monitor /dev/iio\:device1
> 
> Event: time: 1791460068846871297, type: proximity, channel: 0,
> evtype: thresh, direction: rising
> 
> Event: time: 1791460068876669724, type: proximity, channel: 0,
> evtype: thresh, direction: falling
> 
> ...
> 
> Signed-off-by: Christoph Rudorff <chris@rudorff.com>

A few comments inline if you resolve the question Linus raised.

> ---
>  drivers/iio/light/gp2ap002.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/gp2ap002.c b/drivers/iio/light/gp2ap002.c
> index de29081fd3b2..c03eb0f1988b 100644
> --- a/drivers/iio/light/gp2ap002.c
> +++ b/drivers/iio/light/gp2ap002.c
> @@ -129,6 +129,7 @@
>   * @dev: pointer to parent device
>   * @vdd: regulator controlling VDD
>   * @vio: regulator controlling VIO
> + * @vled: regulator controlling VLED
>   * @alsout: IIO ADC channel to convert the ALSOUT signal
>   * @hys_far: hysteresis control from device tree
>   * @hys_close: hysteresis control from device tree
> @@ -142,6 +143,7 @@ struct gp2ap002 {
>  	struct device *dev;
>  	struct regulator *vdd;
>  	struct regulator *vio;
> +	struct regulator *vled;
>  	struct iio_channel *alsout;
>  	u8 hys_far;
>  	u8 hys_close;
> @@ -169,7 +171,7 @@ static irqreturn_t gp2ap002_prox_irq(int irq, void *d)
>  
>  	if (val & GP2AP002_PROX_VO_DETECT) {
>  		/* Close */
> -		dev_dbg(gp2ap002->dev, "close\n");
> +		dev_dbg(gp2ap002->dev, "close %i\n", val);

Why?

>  		ret = regmap_write(gp2ap002->map, GP2AP002_HYS,
>  				   gp2ap002->hys_far);
>  		if (ret)
> @@ -179,7 +181,7 @@ static irqreturn_t gp2ap002_prox_irq(int irq, void *d)
>  					IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING);
>  	} else {
>  		/* Far */
> -		dev_dbg(gp2ap002->dev, "far\n");
> +		dev_dbg(gp2ap002->dev, "far %i\n", val);

Why?

>  		ret = regmap_write(gp2ap002->map, GP2AP002_HYS,
>  				   gp2ap002->hys_close);
>  		if (ret)
> @@ -530,14 +532,22 @@ static int gp2ap002_probe(struct i2c_client *client)
>  	if (IS_ERR(gp2ap002->vio))
>  		return dev_err_probe(dev, PTR_ERR(gp2ap002->vio),
>  				     "failed to get VIO regulator\n");
> +	// returns a dummy if not declared:

See comment syntax elsewhere and match it.

> +	gp2ap002->vled = devm_regulator_get(dev, "vled");
> +	if (IS_ERR(gp2ap002->vled))
> +		return dev_err_probe(dev, PTR_ERR(gp2ap002->vled),
> +				     "failed to get VLED regulator\n");
>  
>  	/* Operating voltage 2.4V .. 3.6V according to datasheet */
>  	ret = regulator_set_voltage(gp2ap002->vdd, 2400000, 3600000);
>  	if (ret) {
> -		dev_err(dev, "failed to sett VDD voltage\n");
> +		dev_err(dev, "failed to set VDD voltage\n");

Good to fix, but unrelated so separate patch.

>  		return ret;
>  	}
>  
> +	/* LED voltage 2.4V .. 3.6V according to datasheet */
> +	/* nothing to set: it's a simple "regulator-fixed" gpio pin */
Use multiline comment syntax.
> +
>  	/* VIO should be between 1.65V and VDD */
>  	ret = regulator_get_voltage(gp2ap002->vdd);
>  	if (ret < 0) {
> @@ -560,6 +570,11 @@ static int gp2ap002_probe(struct i2c_client *client)
>  		dev_err(dev, "failed to enable VIO regulator\n");
>  		goto out_disable_vdd;
>  	}
> +	ret = regulator_enable(gp2ap002->vled);
> +	if (ret) {
> +		dev_err(dev, "failed to enable VLED regulator\n");
> +		goto out_disable_vio;
> +	}
>  
>  	msleep(20);
>  
> @@ -635,6 +650,7 @@ static void gp2ap002_remove(struct i2c_client *client)
>  	iio_device_unregister(indio_dev);
>  	regulator_disable(gp2ap002->vio);
>  	regulator_disable(gp2ap002->vdd);
> +	regulator_disable(gp2ap002->vled);

There are enough of them that if this goes forward consider using
the bulk handling.

>  }
>  
>  static int gp2ap002_runtime_suspend(struct device *dev)
> @@ -659,6 +675,7 @@ static int gp2ap002_runtime_suspend(struct device *dev)
>  	 */
>  	regulator_disable(gp2ap002->vio);
>  	regulator_disable(gp2ap002->vdd);
> +	regulator_disable(gp2ap002->vled);
>  
>  	return 0;
>  }
> @@ -679,6 +696,11 @@ static int gp2ap002_runtime_resume(struct device *dev)
>  		dev_err(dev, "failed to enable VIO regulator in resume path\n");
>  		goto out_disable_vdd;
>  	}
> +	ret = regulator_enable(gp2ap002->vled);
> +	if (ret) {
> +		dev_err(dev, "failed to enable VLED regulator in resume path\n");
> +		goto out_disable_vio;
> +	}
>  
>  	msleep(20);
>  
>