[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

[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

[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 2/3] iio: light: gp2ap002: add led power supply

[sashiko-bot]

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: missing pinctrl properties on J5 proximity sensor nodes
- [Medium] iio: light: gp2ap002: premature regulator disable and reversed supply order
--

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

arm64: dts: qcom: msm8916-samsung-j5: Add proximity sensor

This commit adds the Sharp gp2ap002s00f proximity sensor, its regulator
supplies, and associated pinctrl states to the Samsung Galaxy J5 device
tree.

> 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 697f25d51d9d0..292e7d8b85a4b 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 ba8650971d6a9..efd6c0357d537 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>;
> +	};
> +};

[Severity: Medium]
Also, looking at the preceding commit in this series ("iio: light: gp2ap002:
add led power supply") which binds to this proximity-sensor@44 node and
manages vdd-supply, vled-supply, and vio-supply:

drivers/iio/light/gp2ap002.c:
static const char *const gp2ap002_supply_names[] = {
	"vio", "vdd", "vled"
};

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

In gp2ap002_remove(), pm_runtime_get_sync() resumes the device through
gp2ap002_runtime_resume(), which enables the regulators, configures the
sensor, and calls enable_irq(gp2ap002->irq). Calling regulator_bulk_disable()
before iio_device_unregister() powers off the sensor while its IIO sysfs
interface is still registered and its interrupt line is still active. Can
concurrent gp2ap002_read_raw() calls or a VOUT falling-edge interrupt into
gp2ap002_prox_irq() upon power collapse issue I2C transfers to the unpowered
sensor?

In addition, placing "vio" at index 0 and "vdd" at index 1 in
gp2ap002_supply_names[] causes regulator_bulk_disable() (which iterates in
reverse from NUM_SUPPLIES - 1 down to 0) to disable VDD before VIO during
gp2ap002_runtime_suspend(), probe error paths, and gp2ap002_remove(). Doesn't
this reverse the power-down sequence where VIO was previously disabled before
VDD to maintain VIO <= VDD?

> +
>  &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;
> +	};
> +};

[Severity: Medium]
Should reg_vled_proximity and proximity-sensor@44 reference these pinctrl
states via pinctrl-0 and pinctrl-names?

Both proximity_int_default (for gpio113 in msm8916-samsung-j5-common.dtsi)
and proximity_ldo_en_default (for gpio73 in msm8916-samsung-j5.dts) are
defined under &tlmm, but neither node references them with pinctrl-0 and
pinctrl-names = "default".

When drivers/regulator/fixed.c and drivers/iio/light/gp2ap002.c bind to
reg_vled_proximity and proximity-sensor@44, pinctrl_bind_pins() finds no
pinctrl properties on either device node, and drivers/pinctrl/qcom/pinctrl-msm.c
only sets pinmux direction and function on GPIO/IRQ request without applying
pinconf settings like drive-strength and bias-disable. Also, since
proximity-sensor@44 is only added in msm8916-samsung-j5.dts, should
proximity_int_default and pm8916_l17 be in msm8916-samsung-j5.dts as well,
rather than msm8916-samsung-j5-common.dtsi where they remain unused on
msm8916-samsung-j5x.dts?

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

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