[PATCH v3 0/2] media: imx335: Fix reset-gpio handling

[PATCH v3 0/2] media: imx335: Fix reset-gpio handling

[Umang Jain]

These couple of patches intends to fix the reset-gpio handling
for imx335 driver.

Patch 1/2 mentions reset-gpio polarity in DT binding example.

Patch 2/2 fixes the logical value of reset-gpio during
power-on/power-off sequence.

--
Changes in v3:
- Rework 1/2 commit message
- Fix gpio include in DT example in 1/2
- Remove not-so-informative XCLR comment in 2/2

Changes in v2:
- Also include reset-gpio polarity, mention in DT binding
- Add Fixes tag in 2/2
- Set the reset line to high during init time in 2/2

Link to v2:
https://lore.kernel.org/linux-media/20240729110437.199428-1-umang.jain@ideasonboard.com/

Link to v1:
https://lore.kernel.org/linux-media/tyo5etjwsfznuk6vzwqmcphbu4pz4lskrg3fjieojq5qc3mg6s@6jbwavmapwmf/T/#m189ccfa77ddceda6c3b29be3306f1a27ed0934d6

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
Umang Jain (2):
      dt-bindings: media: imx335: Add reset-gpios to the DT example
      media: imx335: Fix reset-gpio handling

 Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml | 4 ++++
 drivers/media/i2c/imx335.c                                   | 9 ++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)
---
base-commit: f3d2b941adafcdfba9ef63d9ca5bb2d9b263e2af
change-id: 20240731-imx335-gpio-818d736f9295

Best regards,
-- 
Umang Jain <umang.jain@ideasonboard.com>

[PATCH v3 1/2] dt-bindings: media: imx335: Add reset-gpios to the DT example

[Umang Jain]

It's easy to get the polarity of GPIOs in the device tree wrong, as
shown by a recently fixed bug in the imx335 driver. To lower the chance
of future mistakes, especially in new bindings that would take the
imx335 binding as a starting point, add the reset-gpios property to the
DT example. This showcases the correct polarity of the XCLR signal for
Sony sensors in the most common case of the signal not being inverted on
the board.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
index 106c36ee966d..77bf3a4ee89d 100644
--- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
@@ -75,6 +75,8 @@ additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
+
     i2c {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -92,6 +94,8 @@ examples:
             ovdd-supply = <&camera_vddo_1v8>;
             dvdd-supply = <&camera_vddd_1v2>;
 
+            reset-gpios = <&gpio 50 GPIO_ACTIVE_LOW>;
+
             port {
                 imx335: endpoint {
                     remote-endpoint = <&cam>;

-- 
2.45.0

[PATCH v3 2/2] media: imx335: Fix reset-gpio handling

[Umang Jain]

Rectify the logical value of reset-gpio so that it is set to
0 (disabled) during power-on and to 1 (enabled) during power-off.

Meanwhile at it, set the reset-gpio to GPIO_OUT_HIGH at initialization
time to make sure it starts off in reset. Also drop the "Set XCLR"
comment which is not-so-informative.

Cc: stable@vger.kernel.org
Fixes: 45d19b5fb9ae ("media: i2c: Add imx335 camera sensor driver")
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index cd150606a8a9..79b6d60bf6af 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -1057,7 +1057,7 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
 
 	/* Request optional reset pin */
 	imx335->reset_gpio = devm_gpiod_get_optional(imx335->dev, "reset",
-						     GPIOD_OUT_LOW);
+						     GPIOD_OUT_HIGH);
 	if (IS_ERR(imx335->reset_gpio)) {
 		dev_err(imx335->dev, "failed to get reset gpio %ld\n",
 			PTR_ERR(imx335->reset_gpio));
@@ -1170,8 +1170,7 @@ static int imx335_power_on(struct device *dev)
 
 	usleep_range(500, 550); /* Tlow */
 
-	/* Set XCLR */
-	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
+	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
 
 	ret = clk_prepare_enable(imx335->inclk);
 	if (ret) {
@@ -1184,7 +1183,7 @@ static int imx335_power_on(struct device *dev)
 	return 0;
 
 error_reset:
-	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
+	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
 	regulator_bulk_disable(ARRAY_SIZE(imx335_supply_name), imx335->supplies);
 
 	return ret;
@@ -1201,7 +1200,7 @@ static int imx335_power_off(struct device *dev)
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct imx335 *imx335 = to_imx335(sd);
 
-	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
+	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
 	clk_disable_unprepare(imx335->inclk);
 	regulator_bulk_disable(ARRAY_SIZE(imx335_supply_name), imx335->supplies);
 

-- 
2.45.0

Re: [PATCH v3 2/2] media: imx335: Fix reset-gpio handling

[Krzysztof Kozlowski]

On 31/07/2024 09:02, Umang Jain wrote:
> Rectify the logical value of reset-gpio so that it is set to
> 0 (disabled) during power-on and to 1 (enabled) during power-off.
> 
> Meanwhile at it, set the reset-gpio to GPIO_OUT_HIGH at initialization
> time to make sure it starts off in reset. Also drop the "Set XCLR"
> comment which is not-so-informative.
> 

None of our conclusions are explained, which I requested.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: media: imx335: Add reset-gpios to the DT example

[Krzysztof Kozlowski]

On 31/07/2024 09:02, Umang Jain wrote:
> It's easy to get the polarity of GPIOs in the device tree wrong, as
> shown by a recently fixed bug in the imx335 driver. To lower the chance
> of future mistakes, especially in new bindings that would take the
> imx335 binding as a starting point, add the reset-gpios property to the
> DT example. This showcases the correct polarity of the XCLR signal for
> Sony sensors in the most common case of the signal not being inverted on
> the board.
> 

Just one sentence like - make the example complete by adding reset-gpios
with proper polarity - would be enough. Concise, yet still informative,
commit msgs are preferred, usually.

This device is not different than 1000 others which use GPIOs and for
every device you must use proper polarity. Commit msg suggests that here
we should explain something more.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof