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

[sashiko-bot@kernel.org]

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