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

[PATCH v4 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 v4:
- rework 2/2 commit message
- Explain conclusions for 2/2 patch, in the '---' section.

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: 393556c9f56ced8d9776c32ce99f34913cfd904e
change-id: 20240830-imx335-vflip-7f5d4b4d00fe

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

[PATCH v4 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.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
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 v4 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.

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.

The existing usage of imx335 had reset-gpios polarity inverted
(GPIO_ACTIVE_HIGH) in their device-tree sources. With this patch
included, those DTS will not be able to stream imx335 anymore. The
reset-gpio polarity will need to be rectified in the device-tree
sources as shown in [1] example, in order to get imx335 functional
again (as it remains in reset prior to this fix).

Cc: stable@vger.kernel.org
Fixes: 45d19b5fb9ae ("media: i2c: Add imx335 camera sensor driver")
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Link: https://lore.kernel.org/linux-media/20240729110437.199428-1-umang.jain@ideasonboard.com/
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
Following conclusions has been observed and discussed [2]:

- Original driver was reviewed [3] but, the improper handling of
  reset-gpios was missed in review.
- Commit fea91ee73b7c ("media: i2c: imx335: Enable regulator supplies")
  shows the driver didn't had regulator enablement support. The driver
  would have only worked for cases when the supplies were always-on.
- Commit 14a60786d72e ("media: imx335: Set reserved register to default value")
  reflects that the imx335 driver was un-usable due to a reserved
  register not been set to default.
- The original author is no longer using the driver nor it is used for its
  original purpose any more (confirmed by Sakari Ailus).
- It's extremely unlikely the driver has been or continues to be in
  use on ACPI based systems (comment by Sakari Ailus).

The above discussion points in a direction that driver does not cater
to a large user-base. Nonetheless, the breakage will be noticed by a few
users (if at all) hence, this explanation would help resolve the breakage
as soon as noticed (by using correct reset-gpio polarity as mentioned
in [1]).

[1]: Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
[2]: https://lore.kernel.org/linux-media/20240729110437.199428-1-umang.jain@ideasonboard.com/
[3]: https://lore.kernel.org/all/20210527142145.173-3-martinax.krasteva@linux.intel.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 990d74214cc2..54a1de53d497 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -997,7 +997,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));
@@ -1110,8 +1110,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) {
@@ -1124,7 +1123,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;
@@ -1141,7 +1140,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 v4 0/2] media: imx335: Fix reset-gpio handling

[Sakari Ailus]

On Fri, Aug 30, 2024 at 11:41:50AM +0530, Umang Jain wrote:
> 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 v4:
> - rework 2/2 commit message
> - Explain conclusions for 2/2 patch, in the '---' section.

Thanks, Umang!

I've applied these in my tree.

-- 
Sakari Ailus