[PATCH v2 0/3] MSM8916-mainline: improve gp2ap002 proximity sensor v2

[PATCH v2 0/3] MSM8916-mainline: improve gp2ap002 proximity sensor v2

[Christoph Rudorff]

Samsung J5 is equipped with GP2AP002 proximity sensor.
Unfortunally the driver wasn't working, no irq received.

According to datasheet page 8 "Recommended external circuit",
it can have tree supplies: vio, vdd and vled.

It missed to power up the LED.

I found this looking in downstream kernel, where
several #ifdefs handled the case of 2 or 3 supplies.

Now it works. Tested on 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>
---
Changes in v2:
- reorganized patch serie
- gp2ap002: use regulator bulk framework
- Link to v1: https://patch.msgid.link/20261016-iio-light-gp2ap002-v1-0-7c93312fa8d5@rudorff.com

To: Linus Walleij <linusw@kernel.org>
To: Jonathan Cameron <jic23@kernel.org>
To: David Lechner <dlechner@baylibre.com>
To: Nuno Sá <nuno.sa@analog.com>
To: Andy Shevchenko <andy@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Bjorn Andersson <andersson@kernel.org>
To: Konrad Dybcio <konradybcio@kernel.org>
Cc: linux-iio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-arm-msm@vger.kernel.org

---
Christoph Rudorff (3):
      dt-bindings: iio: light: gp2ap002: add vled
      iio: light: gp2ap002: add led power supply
      arm64: dts: qcom: msm8916-samsung-j5: Add proximity sensor

 .../bindings/iio/light/sharp,gp2ap002.yaml         |   4 +
 .../boot/dts/qcom/msm8916-samsung-j5-common.dtsi   |  15 +++
 arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dts    |  32 +++++++
 drivers/iio/light/gp2ap002.c                       | 101 +++++++++++----------
 4 files changed, 106 insertions(+), 46 deletions(-)
---
base-commit: a2de78ee6ed8bf8cd99d177c74865a6e8c3cb6ea
change-id: 20261016-iio-light-gp2ap002-6bd553aecb4b

Best regards,
--  
Christoph Rudorff <chris@rudorff.com>

[PATCH v2 1/3] dt-bindings: iio: light: gp2ap002: add vled

[Christoph Rudorff]

According to datasheet page 8 "Recommended external circuit",
it can have tree supplies: vio, vdd and vled. As vdd and vled wants the
same voltage, they may be connected to a single supply.

Signed-off-by: Christoph Rudorff <chris@rudorff.com>
---
 Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml b/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
index 99bddf31cbed..f5b2c6a4d7e3 100644
--- a/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
+++ b/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
@@ -33,6 +33,10 @@ properties:
   vio-supply:
     description: VIO power supply a phandle to a regulator
 
+  vled-supply:
+    description: LED power supply a phandle to a regulator
+      Optional. It may already be connected to VDD.
+
   io-channels:
     maxItems: 1
     description: ALSOUT ADC channel to read the ambient light

-- 
2.39.5

Re: [PATCH v2 1/3] dt-bindings: iio: light: gp2ap002: add vled

[Jonathan Cameron]

On Sat, 17 Oct 2026 19:27:41 +0200
Christoph Rudorff <chris@rudorff.com> wrote:

> According to datasheet page 8 "Recommended external circuit",
> it can have tree supplies: vio, vdd and vled. As vdd and vled wants the
> same voltage, they may be connected to a single supply.
> 
> Signed-off-by: Christoph Rudorff <chris@rudorff.com>
> ---
>  Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml b/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
> index 99bddf31cbed..f5b2c6a4d7e3 100644
> --- a/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
> @@ -33,6 +33,10 @@ properties:
>    vio-supply:
>      description: VIO power supply a phandle to a regulator
>  
> +  vled-supply:
> +    description: LED power supply a phandle to a regulator
> +      Optional. It may already be connected to VDD.
That's a board wiring thing - the DT really should have just listed
that supply twice.  So drop the optional bit.  We can leave
it as not being in required for historical reasons but no need
to call out that is is optional here!

Given confusion on v1, I'd mention it is the pin called LEDA even though
the supply is called vled on page 8.

Thanks,

Jonathan

> +
>    io-channels:
>      maxItems: 1
>      description: ALSOUT ADC channel to read the ambient light
> 

Re: [PATCH v2 1/3] dt-bindings: iio: light: gp2ap002: add vled

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

On Sat, Oct 17, 2026 at 07:27:41PM +0200, Christoph Rudorff wrote:
> According to datasheet page 8 "Recommended external circuit",
> it can have tree supplies: vio, vdd and vled. As vdd and vled wants the
> same voltage, they may be connected to a single supply.
> 
> Signed-off-by: Christoph Rudorff <chris@rudorff.com>

Valid comments from Jonathan here,
pw-bot: changes-requested

Thanks,
COnor.

> ---
>  Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml b/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
> index 99bddf31cbed..f5b2c6a4d7e3 100644
> --- a/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
> @@ -33,6 +33,10 @@ properties:
>    vio-supply:
>      description: VIO power supply a phandle to a regulator
>  
> +  vled-supply:
> +    description: LED power supply a phandle to a regulator
> +      Optional. It may already be connected to VDD.
> +
>    io-channels:
>      maxItems: 1
>      description: ALSOUT ADC channel to read the ambient light
> 
> -- 
> 2.39.5
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/3] dt-bindings: iio: light: gp2ap002: add vled

[Linus Walleij]

On Sat, Oct 17, 2026 at 7:28 PM Christoph Rudorff <chris@rudorff.com> wrote:

> According to datasheet page 8 "Recommended external circuit",
> it can have tree supplies: vio, vdd and vled. As vdd and vled wants the
> same voltage, they may be connected to a single supply.
>
> Signed-off-by: Christoph Rudorff <chris@rudorff.com>
> ---
>  Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml b/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
> index 99bddf31cbed..f5b2c6a4d7e3 100644
> --- a/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
> @@ -33,6 +33,10 @@ properties:
>    vio-supply:
>      description: VIO power supply a phandle to a regulator
>
> +  vled-supply:

So this should be named leda-supply like the pin on the component.

> +    description: LED power supply a phandle to a regulator
> +      Optional. It may already be connected to VDD.

Description should say "LED anode power supply" as that is what the
"A" means in leda.

I'm not sure it should be optional really as it technically *must*
be connected but the diplomatic solution is to not make it
required so we don't break a ton of device trees.

If we wanna be really detail oriented, add it optional, fix all
device trees, make it non-optional.

Yours,
Linus Walleij

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

[Christoph Rudorff]

The led power might be attached to an extra supply.

Signed-off-by: Christoph Rudorff <chris@rudorff.com>
---
 drivers/iio/light/gp2ap002.c | 101 +++++++++++++++++++++++--------------------
 1 file changed, 55 insertions(+), 46 deletions(-)

diff --git a/drivers/iio/light/gp2ap002.c b/drivers/iio/light/gp2ap002.c
index de29081fd3b2..5e4599e40376 100644
--- a/drivers/iio/light/gp2ap002.c
+++ b/drivers/iio/light/gp2ap002.c
@@ -123,12 +123,26 @@
 #define GP2AP002_CON_OCON_HIGH		(0x3 << GP2AP002_CON_OCON_SHIFT)
 #define GP2AP002_CON_OCON_MASK		(0x3 << GP2AP002_CON_OCON_SHIFT)
 
+/*
+ * @vio: regulator name controlling VIO
+ * @vdd: regulator name controlling VDD
+ * @vled: regulator name controlling VLED (optional)
+ */
+static const char *const gp2ap002_supply_names[] = {
+	"vio", "vdd", "vled"
+};
+enum {
+	SUPPLY_VIO,
+	SUPPLY_VDD,
+	SUPPLY_VLED,
+	NUM_SUPPLIES
+};
+
 /**
  * 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
  * @alsout: IIO ADC channel to convert the ALSOUT signal
  * @hys_far: hysteresis control from device tree
  * @hys_close: hysteresis control from device tree
@@ -140,8 +154,7 @@
 struct gp2ap002 {
 	struct regmap *map;
 	struct device *dev;
-	struct regulator *vdd;
-	struct regulator *vio;
+	struct regulator_bulk_data supplies[NUM_SUPPLIES];
 	struct iio_channel *alsout;
 	u8 hys_far;
 	u8 hys_close;
@@ -521,45 +534,52 @@ static int gp2ap002_probe(struct i2c_client *client)
 		}
 	}
 
-	gp2ap002->vdd = devm_regulator_get(dev, "vdd");
-	if (IS_ERR(gp2ap002->vdd))
-		return dev_err_probe(dev, PTR_ERR(gp2ap002->vdd),
-				     "failed to get VDD regulator\n");
+	regulator_bulk_set_supply_names(gp2ap002->supplies,
+		gp2ap002_supply_names,
+		NUM_SUPPLIES);
+
+	ret = devm_regulator_bulk_get(dev,
+		NUM_SUPPLIES,
+		gp2ap002->supplies);
 
-	gp2ap002->vio = devm_regulator_get(dev, "vio");
-	if (IS_ERR(gp2ap002->vio))
-		return dev_err_probe(dev, PTR_ERR(gp2ap002->vio),
-				     "failed to get VIO regulator\n");
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get regulators\n");
 
-	/* Operating voltage 2.4V .. 3.6V according to datasheet */
-	ret = regulator_set_voltage(gp2ap002->vdd, 2400000, 3600000);
+	/* vdd operating voltage 2.4V .. 3.6V according to datasheet */
+	ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VDD].consumer, 2400000, 3600000);
 	if (ret) {
-		dev_err(dev, "failed to sett VDD voltage\n");
+		dev_err(dev, "failed to set VDD voltage\n");
 		return ret;
 	}
-
 	/* VIO should be between 1.65V and VDD */
-	ret = regulator_get_voltage(gp2ap002->vdd);
+	ret = regulator_get_voltage(gp2ap002->supplies[SUPPLY_VDD].consumer);
 	if (ret < 0) {
 		dev_err(dev, "failed to get VDD voltage\n");
 		return ret;
 	}
-	ret = regulator_set_voltage(gp2ap002->vio, 1650000, ret);
+	ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VIO].consumer, 1650000, ret);
 	if (ret) {
 		dev_err(dev, "failed to set VIO voltage\n");
 		return ret;
 	}
+	/* LED voltage 2.4V .. 3.6V according to datasheet */
+	/* We need to check this, as dummy or fixed controllers trigger an error. */
+	ret = regulator_is_supported_voltage(gp2ap002->supplies[SUPPLY_VLED].consumer,
+		2400000, 3600000);
+	if (ret == 1) {
+		ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VLED].consumer,
+		2400000, 3600000);
+		if (ret) {
+			dev_err(dev, "failed to set VLED voltage\n");
+			return ret;
+		}
+	}
 
-	ret = regulator_enable(gp2ap002->vdd);
+	ret = regulator_bulk_enable(NUM_SUPPLIES, gp2ap002->supplies);
 	if (ret) {
-		dev_err(dev, "failed to enable VDD regulator\n");
+		dev_err(dev, "failed to enable regulators\n");
 		return ret;
 	}
-	ret = regulator_enable(gp2ap002->vio);
-	if (ret) {
-		dev_err(dev, "failed to enable VIO regulator\n");
-		goto out_disable_vdd;
-	}
 
 	msleep(20);
 
@@ -570,7 +590,7 @@ static int gp2ap002_probe(struct i2c_client *client)
 	ret = gp2ap002_init(gp2ap002);
 	if (ret) {
 		dev_err(dev, "initialization failed\n");
-		goto out_disable_vio;
+		goto out_disable_power;
 	}
 	pm_runtime_get_noresume(dev);
 	pm_runtime_set_active(dev);
@@ -616,10 +636,8 @@ static int gp2ap002_probe(struct i2c_client *client)
 	pm_runtime_put_noidle(dev);
 out_disable_pm:
 	pm_runtime_disable(dev);
-out_disable_vio:
-	regulator_disable(gp2ap002->vio);
-out_disable_vdd:
-	regulator_disable(gp2ap002->vdd);
+out_disable_power:
+	regulator_bulk_disable(NUM_SUPPLIES, gp2ap002->supplies);
 	return ret;
 }
 
@@ -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);
 }
 
 static int gp2ap002_runtime_suspend(struct device *dev)
@@ -657,8 +674,7 @@ static int gp2ap002_runtime_suspend(struct device *dev)
 	 * As these regulators may be shared, at least we are now in
 	 * sleep even if the regulators aren't really turned off.
 	 */
-	regulator_disable(gp2ap002->vio);
-	regulator_disable(gp2ap002->vdd);
+	regulator_bulk_disable(NUM_SUPPLIES, gp2ap002->supplies);
 
 	return 0;
 }
@@ -669,23 +685,18 @@ static int gp2ap002_runtime_resume(struct device *dev)
 	struct gp2ap002 *gp2ap002 = iio_priv(indio_dev);
 	int ret;
 
-	ret = regulator_enable(gp2ap002->vdd);
+	ret = regulator_bulk_enable(NUM_SUPPLIES, gp2ap002->supplies);
 	if (ret) {
-		dev_err(dev, "failed to enable VDD regulator in resume path\n");
+		dev_err(dev, "failed to enable regulators in resume path\n");
 		return ret;
 	}
-	ret = regulator_enable(gp2ap002->vio);
-	if (ret) {
-		dev_err(dev, "failed to enable VIO regulator in resume path\n");
-		goto out_disable_vdd;
-	}
 
 	msleep(20);
 
 	ret = gp2ap002_init(gp2ap002);
 	if (ret) {
 		dev_err(dev, "re-initialization failed\n");
-		goto out_disable_vio;
+		goto out_disable_power;
 	}
 
 	/* Re-activate the IRQ */
@@ -693,10 +704,8 @@ static int gp2ap002_runtime_resume(struct device *dev)
 
 	return 0;
 
-out_disable_vio:
-	regulator_disable(gp2ap002->vio);
-out_disable_vdd:
-	regulator_disable(gp2ap002->vdd);
+out_disable_power:
+	regulator_bulk_disable(NUM_SUPPLIES, gp2ap002->supplies);
 	return ret;
 }
 

-- 
2.39.5

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

[Jonathan Cameron]

On Sat, 17 Oct 2026 19:27:42 +0200
Christoph Rudorff <chris@rudorff.com> wrote:

> The led power might be attached to an extra supply.

I'd add a mention of LEDA.   + the fact it is always attached
to a supply, what is 'maybe' is whether we need to control it.

> 
> Signed-off-by: Christoph Rudorff <chris@rudorff.com>
> ---
>  drivers/iio/light/gp2ap002.c | 101 +++++++++++++++++++++++--------------------
>  1 file changed, 55 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/iio/light/gp2ap002.c b/drivers/iio/light/gp2ap002.c
> index de29081fd3b2..5e4599e40376 100644
> --- a/drivers/iio/light/gp2ap002.c
> +++ b/drivers/iio/light/gp2ap002.c
> @@ -123,12 +123,26 @@
>  #define GP2AP002_CON_OCON_HIGH		(0x3 << GP2AP002_CON_OCON_SHIFT)
>  #define GP2AP002_CON_OCON_MASK		(0x3 << GP2AP002_CON_OCON_SHIFT)
>  
> +/*
> + * @vio: regulator name controlling VIO
> + * @vdd: regulator name controlling VDD
> + * @vled: regulator name controlling VLED (optional)
> + */
> +static const char *const gp2ap002_supply_names[] = {
> +	"vio", "vdd", "vled"
> +};
blank line here.
> +enum {
> +	SUPPLY_VIO,
> +	SUPPLY_VDD,
> +	SUPPLY_VLED,
> +	NUM_SUPPLIES
> +};
Force these to match up via

static const char *const gp2ap002_supply_names[NUM_SUPPLIES] = {
	[SUPPLY_VIO] = "vio",
	[SUPPLY_VDD] = "vdd",
	[SUPPLY_VLED] = "vled",
};

You might as well also move this naming array local to where
it is used.

> +
>  /**
>   * 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
>   * @alsout: IIO ADC channel to convert the ALSOUT signal
>   * @hys_far: hysteresis control from device tree
>   * @hys_close: hysteresis control from device tree
> @@ -140,8 +154,7 @@
>  struct gp2ap002 {
>  	struct regmap *map;
>  	struct device *dev;
> -	struct regulator *vdd;
> -	struct regulator *vio;
> +	struct regulator_bulk_data supplies[NUM_SUPPLIES];
>  	struct iio_channel *alsout;
>  	u8 hys_far;
>  	u8 hys_close;
> @@ -521,45 +534,52 @@ static int gp2ap002_probe(struct i2c_client *client)
>  		}
>  	}
>  
> -	gp2ap002->vdd = devm_regulator_get(dev, "vdd");
> -	if (IS_ERR(gp2ap002->vdd))
> -		return dev_err_probe(dev, PTR_ERR(gp2ap002->vdd),
> -				     "failed to get VDD regulator\n");
> +	regulator_bulk_set_supply_names(gp2ap002->supplies,
> +		gp2ap002_supply_names,
> +		NUM_SUPPLIES);
I was kind of expecting this to be local style you were copying, but nope the
driver is at least fairly consistent in indenting to align parameters.
e.g.

	regulator_bulk_set_supply_names(gp2ap002->supplies,
					gp2ap002_supply_names,
					NUM_SUPPLIES);

	ret = devm_regulator_bulk_get(dev, NUM_SUPPLIES, gp2ap002->supplies);

> +
> +	ret = devm_regulator_bulk_get(dev,
> +		NUM_SUPPLIES,
> +		gp2ap002->supplies);


>  
> -	gp2ap002->vio = devm_regulator_get(dev, "vio");
> -	if (IS_ERR(gp2ap002->vio))
> -		return dev_err_probe(dev, PTR_ERR(gp2ap002->vio),
> -				     "failed to get VIO regulator\n");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get regulators\n");
>  
> -	/* Operating voltage 2.4V .. 3.6V according to datasheet */
> -	ret = regulator_set_voltage(gp2ap002->vdd, 2400000, 3600000);
> +	/* vdd operating voltage 2.4V .. 3.6V according to datasheet */
> +	ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VDD].consumer, 2400000, 3600000);
>  	if (ret) {
> -		dev_err(dev, "failed to sett VDD voltage\n");
> +		dev_err(dev, "failed to set VDD voltage\n");
>  		return ret;
>  	}
> -
>  	/* VIO should be between 1.65V and VDD */
> -	ret = regulator_get_voltage(gp2ap002->vdd);
> +	ret = regulator_get_voltage(gp2ap002->supplies[SUPPLY_VDD].consumer);

Ah. I'd failed to register the voltages were being manually set (generally
we leave that to firmware in more modern drivers).
That rather reduces the advantage in using bulk regulators.  Probably
still just about worth doing.


>  	if (ret < 0) {
>  		dev_err(dev, "failed to get VDD voltage\n");
>  		return ret;
>  	}
> -	ret = regulator_set_voltage(gp2ap002->vio, 1650000, ret);
> +	ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VIO].consumer, 1650000, ret);
>  	if (ret) {
>  		dev_err(dev, "failed to set VIO voltage\n");
>  		return ret;
>  	}
> +	/* LED voltage 2.4V .. 3.6V according to datasheet */
> +	/* We need to check this, as dummy or fixed controllers trigger an error. */
I'd make that a multiline comment and say what 'this' is.
	/*
	 * LED voltage 2.4V .. 3.6V according to datasheet
	 * We need to check control is supported, as controlling dummy or fixed
	 * controllers results in an error.
	 */

Do you actually need to control this for your board?  I'm not that
keen to replicate the controlling of voltages if we don't have
to.  We can't drop existing code as maybe a board is relying on it
but we can avoid adding new examples!

> +	ret = regulator_is_supported_voltage(gp2ap002->supplies[SUPPLY_VLED].consumer,
> +		2400000, 3600000);
> +	if (ret == 1) {
> +		ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VLED].consumer,
> +		2400000, 3600000);
> +		if (ret) {
> +			dev_err(dev, "failed to set VLED voltage\n");
> +			return ret;
> +		}
> +	}
>  
> -	ret = regulator_enable(gp2ap002->vdd);
> +	ret = regulator_bulk_enable(NUM_SUPPLIES, gp2ap002->supplies);
>  	if (ret) {
> -		dev_err(dev, "failed to enable VDD regulator\n");
> +		dev_err(dev, "failed to enable regulators\n");
>  		return ret;
>  	}
> -	ret = regulator_enable(gp2ap002->vio);
> -	if (ret) {
> -		dev_err(dev, "failed to enable VIO regulator\n");
> -		goto out_disable_vdd;
> -	}
>  
>  	msleep(20);
>  
> @@ -570,7 +590,7 @@ static int gp2ap002_probe(struct i2c_client *client)
>  	ret = gp2ap002_init(gp2ap002);
>  	if (ret) {
>  		dev_err(dev, "initialization failed\n");
> -		goto out_disable_vio;
> +		goto out_disable_power;
>  	}
>  	pm_runtime_get_noresume(dev);
>  	pm_runtime_set_active(dev);
> @@ -616,10 +636,8 @@ static int gp2ap002_probe(struct i2c_client *client)
>  	pm_runtime_put_noidle(dev);
>  out_disable_pm:
>  	pm_runtime_disable(dev);
> -out_disable_vio:
> -	regulator_disable(gp2ap002->vio);
> -out_disable_vdd:
> -	regulator_disable(gp2ap002->vdd);
> +out_disable_power:
> +	regulator_bulk_disable(NUM_SUPPLIES, gp2ap002->supplies);
>  	return ret;
>  }
>  
> @@ -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);

Why at that point in sequence?  You are turning the power off
before removing the userspace interfaces which seems unlikely to
be race free. More generally it is no longer reverse of probe
order. If that is necessary for some reason I'd expect a comment
to say why.

>  	iio_device_unregister(indio_dev);
> -	regulator_disable(gp2ap002->vio);
> -	regulator_disable(gp2ap002->vdd);
>  }

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

[chr[]]

Hi,

In preparation of v3:

Am 17.10.26 um 20:00 schrieb Jonathan Cameron:
> On Sat, 17 Oct 2026 19:27:42 +0200
> Christoph Rudorff <chris@rudorff.com> wrote:
> 
>>   	/* VIO should be between 1.65V and VDD */
>> -	ret = regulator_get_voltage(gp2ap002->vdd);
>> +	ret = regulator_get_voltage(gp2ap002->supplies[SUPPLY_VDD].consumer);
> 
> Ah. I'd failed to register the voltages were being manually set (generally
> we leave that to firmware in more modern drivers).
> That rather reduces the advantage in using bulk regulators.  Probably
> still just about worth doing.
>

Yes, this is why I didn't made it in v1.

> 
>>   	if (ret < 0) {
>>   		dev_err(dev, "failed to get VDD voltage\n");
>>   		return ret;
>>   	}
>> -	ret = regulator_set_voltage(gp2ap002->vio, 1650000, ret);
>> +	ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VIO].consumer, 1650000, ret);
>>   	if (ret) {
>>   		dev_err(dev, "failed to set VIO voltage\n");
>>   		return ret;
>>   	}
>> +	/* LED voltage 2.4V .. 3.6V according to datasheet */
>> +	/* We need to check this, as dummy or fixed controllers trigger an error. */
> I'd make that a multiline comment and say what 'this' is.
> 	/*
> 	 * LED voltage 2.4V .. 3.6V according to datasheet
> 	 * We need to check control is supported, as controlling dummy or fixed
> 	 * controllers results in an error.
> 	 */
> 
> Do you actually need to control this for your board?  I'm not that
> keen to replicate the controlling of voltages if we don't have
> to.  We can't drop existing code as maybe a board is relying on it
> but we can avoid adding new examples!
> 

So we don't want to drop all the set_voltage code in general.
My board does not need that at all, all voltages are already set
to valid values in DT, ... I hope ... I check downstream ...

So I skip the "example" for set_voltage(vled)?!

And thanks for the advice to pretty up the code!

chr[]

>> @@ -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);
> 
> Why at that point in sequence?  You are turning the power off
> before removing the userspace interfaces which seems unlikely to
> be race free. More generally it is no longer reverse of probe
> order. If that is necessary for some reason I'd expect a comment
> to say why.
>

Sorry, just a stupid mistake.

>>   	iio_device_unregister(indio_dev);
>> -	regulator_disable(gp2ap002->vio);
>> -	regulator_disable(gp2ap002->vdd);
>>   }
> 
> 

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

[Jonathan Cameron]

On Sun, 18 Oct 2026 16:41:48 +0200
"chr[]" <chris@rudorff.com> wrote:

> Hi,
> 
> In preparation of v3:
> 
> Am 17.10.26 um 20:00 schrieb Jonathan Cameron:
> > On Sat, 17 Oct 2026 19:27:42 +0200
> > Christoph Rudorff <chris@rudorff.com> wrote:
> >   
> >>   	/* VIO should be between 1.65V and VDD */
> >> -	ret = regulator_get_voltage(gp2ap002->vdd);
> >> +	ret = regulator_get_voltage(gp2ap002->supplies[SUPPLY_VDD].consumer);  
> > 
> > Ah. I'd failed to register the voltages were being manually set (generally
> > we leave that to firmware in more modern drivers).
> > That rather reduces the advantage in using bulk regulators.  Probably
> > still just about worth doing.
> >  
> 
> Yes, this is why I didn't made it in v1.
> 
> >   
> >>   	if (ret < 0) {
> >>   		dev_err(dev, "failed to get VDD voltage\n");
> >>   		return ret;
> >>   	}
> >> -	ret = regulator_set_voltage(gp2ap002->vio, 1650000, ret);
> >> +	ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VIO].consumer, 1650000, ret);
> >>   	if (ret) {
> >>   		dev_err(dev, "failed to set VIO voltage\n");
> >>   		return ret;
> >>   	}
> >> +	/* LED voltage 2.4V .. 3.6V according to datasheet */
> >> +	/* We need to check this, as dummy or fixed controllers trigger an error. */  
> > I'd make that a multiline comment and say what 'this' is.
> > 	/*
> > 	 * LED voltage 2.4V .. 3.6V according to datasheet
> > 	 * We need to check control is supported, as controlling dummy or fixed
> > 	 * controllers results in an error.
> > 	 */
> > 
> > Do you actually need to control this for your board?  I'm not that
> > keen to replicate the controlling of voltages if we don't have
> > to.  We can't drop existing code as maybe a board is relying on it
> > but we can avoid adding new examples!
> >   
> 
> So we don't want to drop all the set_voltage code in general.
> My board does not need that at all, all voltages are already set
> to valid values in DT, ... I hope ... I check downstream ...
Unfortunately it would be a bit of a gamble to remove the in
driver voltage control on existing supplies. Given it works
for yours though let us not do it for the new one! 
> 
> So I skip the "example" for set_voltage(vled)?!

Exactly.

Jonathan

> 
> And thanks for the advice to pretty up the code!
> 
> chr[]
> 
> >> @@ -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);  
> > 
> > Why at that point in sequence?  You are turning the power off
> > before removing the userspace interfaces which seems unlikely to
> > be race free. More generally it is no longer reverse of probe
> > order. If that is necessary for some reason I'd expect a comment
> > to say why.
> >  
> 
> Sorry, just a stupid mistake.
> 
> >>   	iio_device_unregister(indio_dev);
> >> -	regulator_disable(gp2ap002->vio);
> >> -	regulator_disable(gp2ap002->vdd);
> >>   }  
> > 
> >   
> 

[PATCH v2 3/3] arm64: dts: qcom: msm8916-samsung-j5: Add proximity sensor

[Christoph Rudorff]

J5 got a gp2ap002 sensor, add it and it's supplies to the device tree

Signed-off-by: Christoph Rudorff <chris@rudorff.com>
---
 .../boot/dts/qcom/msm8916-samsung-j5-common.dtsi   | 15 ++++++++++
 arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dts    | 32 ++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi b/arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi
index 697f25d51d9d..292e7d8b85a4 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi
@@ -149,6 +149,14 @@ &pm8916_resin {
 	linux,code = <KEY_VOLUMEDOWN>;
 };
 
+&pm8916_rpm_regulators {
+	pm8916_l17: l17 {
+		regulator-name = "proximity_vdd";
+		regulator-min-microvolt = <2850000>;
+		regulator-max-microvolt = <3100000>;
+	};
+};
+
 &sdhc_1 {
 	status = "okay";
 };
@@ -248,6 +256,13 @@ sdc2_cd_default: sdc2-cd-default-state {
 		bias-disable;
 	};
 
+	proximity_int_default: proximity-int-default-state {
+		pins = "gpio113";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-disable;
+	};
+
 	sensors_i2c_default: sensors-i2c-default-state {
 		pins = "gpio31", "gpio32";
 		function = "gpio";
diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dts b/arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dts
index ba8650971d6a..efd6c0357d53 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dts
+++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dts
@@ -8,6 +8,14 @@ / {
 	model = "Samsung Galaxy J5 (2015)";
 	compatible = "samsung,j5", "qcom,msm8916";
 	chassis-type = "handset";
+
+	reg_vled_proximity: regulator-vled-proximity {
+		compatible = "regulator-fixed";
+		regulator-name = "proximity_led";
+
+		gpio = <&tlmm 73 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
 };
 
 &accelerometer {
@@ -19,6 +27,21 @@ &blsp_i2c5 {
 	status = "disabled";
 };
 
+&i2c_sensors {
+	proximity-sensor@44 {
+		compatible = "sharp,gp2ap002s00f";
+		reg = <0x44>;
+		interrupt-parent = <&tlmm>;
+		interrupts = <113 IRQ_TYPE_EDGE_FALLING>;
+		vdd-supply = <&pm8916_l17>;
+		vled-supply = <&reg_vled_proximity>;
+		vio-supply = <&pm8916_l5>;
+
+		sharp,proximity-far-hysteresis = /bits/ 8 <0x2f>;
+		sharp,proximity-close-hysteresis = /bits/ 8 <0x0f>;
+	};
+};
+
 &pm8916_codec {
 	qcom,micbias1-ext-cap;
 };
@@ -30,3 +53,12 @@ &touchscreen {
 &usb_hs_phy {
 	qcom,init-seq = /bits/ 8 <0x1 0x19 0x2 0x0b>;
 };
+
+&tlmm {
+	proximity_ldo_en_default: proximity-ldo-en-default-state {
+		pins = "gpio73";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-disable;
+	};
+};

-- 
2.39.5

Re: [PATCH v2 0/3] MSM8916-mainline: improve gp2ap002 proximity sensor v2

[Jonathan Cameron]

On Sat, 17 Oct 2026 19:27:40 +0200
Christoph Rudorff <chris@rudorff.com> wrote:

> Samsung J5 is equipped with GP2AP002 proximity sensor.
> Unfortunally the driver wasn't working, no irq received.
> 
> According to datasheet page 8 "Recommended external circuit",
> it can have tree supplies: vio, vdd and vled.
> 
> It missed to power up the LED.
> 
> I found this looking in downstream kernel, where
> several #ifdefs handled the case of 2 or 3 supplies.
> 
> Now it works. Tested on 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>
> ---
> Changes in v2:
> - reorganized patch serie
> - gp2ap002: use regulator bulk framework
> - Link to v1: https://patch.msgid.link/20261016-iio-light-gp2ap002-v1-0-7c93312fa8d5@rudorff.com
> 
> To: Linus Walleij <linusw@kernel.org>
> To: Jonathan Cameron <jic23@kernel.org>
> To: David Lechner <dlechner@baylibre.com>
> To: Nuno Sá <nuno.sa@analog.com>
> To: Andy Shevchenko <andy@kernel.org>
> To: Rob Herring <robh@kernel.org>
> To: Krzysztof Kozlowski <krzk+dt@kernel.org>
> To: Conor Dooley <conor+dt@kernel.org>
> To: Bjorn Andersson <andersson@kernel.org>
> To: Konrad Dybcio <konradybcio@kernel.org>
> Cc: linux-iio@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-msm@vger.kernel.org
> 
https://sashiko.dev/#/patchset/20261017-iio-light-gp2ap002-v2-0-d6879767a446%40rudorff.com

Spotted stuff I missed.

J
> ---
> Christoph Rudorff (3):
>       dt-bindings: iio: light: gp2ap002: add vled
>       iio: light: gp2ap002: add led power supply
>       arm64: dts: qcom: msm8916-samsung-j5: Add proximity sensor
> 
>  .../bindings/iio/light/sharp,gp2ap002.yaml         |   4 +
>  .../boot/dts/qcom/msm8916-samsung-j5-common.dtsi   |  15 +++
>  arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dts    |  32 +++++++
>  drivers/iio/light/gp2ap002.c                       | 101 +++++++++++----------
>  4 files changed, 106 insertions(+), 46 deletions(-)
> ---
> base-commit: a2de78ee6ed8bf8cd99d177c74865a6e8c3cb6ea
> change-id: 20261016-iio-light-gp2ap002-6bd553aecb4b
> 
> Best regards,
> --  
> Christoph Rudorff <chris@rudorff.com>
>