[PATCH v2 0/2] max9271: Fix pclk_detect silent failure

[PATCH v2 0/2] max9271: Fix pclk_detect silent failure

[Jacopo Mondi]

Repeatedly reading register 0x15 to validate the incoming pixel clock
causes sporadic read errors which went silently ignored, causing the camera
module to fail to start streaming.

Fix that by ignoring the read error and while at it rework the error message
handling in all functions.

v1->v2:
- Drop v1 [1/2]
- [2/2] new patch. Handle all bus access errors in the call sites

Jacopo Mondi (2):
  media: max9271: Ignore busy loop read errors
  media: max9271: Fail loud on bus errors in call sites

 drivers/media/i2c/max9271.c | 116 ++++++++++++++++++++++--------------
 1 file changed, 71 insertions(+), 45 deletions(-)

--
2.33.1

[PATCH v2 1/2] media: max9271: Ignore busy loop read errors

[Jacopo Mondi]

Valid pixel clock detection is performed by spinning on a register read
which if repeated too frequently might fail. As the error is not fatal
ignore it instead of bailing out to continue spinning until the timeout
completion.

Also relax the time between bus transactions and slightly increase the
wait interval to mitigate the failure risk.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---

v1->v2:
- Do not continue but jump to a label to respect the sleep timout after a
  failed read

Niklas I kept your tag anyway, hope it's ok.

Thanks
   j

---
 drivers/media/i2c/max9271.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index ff86c8c4ea61..aa4add473716 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -55,7 +55,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val)
 /*
  * max9271_pclk_detect() - Detect valid pixel clock from image sensor
  *
- * Wait up to 10ms for a valid pixel clock.
+ * Wait up to 15ms for a valid pixel clock.
  *
  * Returns 0 for success, < 0 for pixel clock not properly detected
  */
@@ -64,15 +64,16 @@ static int max9271_pclk_detect(struct max9271_device *dev)
 	unsigned int i;
 	int ret;

-	for (i = 0; i < 100; i++) {
+	for (i = 0; i < 10; i++) {
 		ret = max9271_read(dev, 0x15);
 		if (ret < 0)
-			return ret;
+			goto skip;

 		if (ret & MAX9271_PCLKDET)
 			return 0;

-		usleep_range(50, 100);
+skip:
+		usleep_range(1000, 1500);
 	}

 	dev_err(&dev->client->dev, "Unable to detect valid pixel clock\n");
--
2.33.1

[PATCH v2 2/2] media: max9271: Fail loud on bus errors in call sites

[Jacopo Mondi]

As not all bus access errors are fatal, as in example reads performed
in a busy loop, it's responsibility of the bus access function caller
to fail louder on fatal errors.

Instrument all functions in the max9271 library driver to fail on fatal
read/write errors and demote the max9271_write() error level to debug
to align it to the one in max9271_read().

While at it, align the style of the existing error messages by removing
"MAX9271" from the output string, as the device log helpers already
identify the driver emitting the message.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9271.c | 105 ++++++++++++++++++++++--------------
 1 file changed, 64 insertions(+), 41 deletions(-)

diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index aa4add473716..f5f354b8a43c 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -45,7 +45,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val)
 
 	ret = i2c_smbus_write_byte_data(dev->client, reg, val);
 	if (ret < 0)
-		dev_err(&dev->client->dev,
+		dev_dbg(&dev->client->dev,
 			"%s: register 0x%02x write failed (%d)\n",
 			__func__, reg, ret);
 
@@ -120,8 +120,11 @@ int max9271_set_serial_link(struct max9271_device *dev, bool enable)
 	 * Therefore a conservative delay seems best here.
 	 */
 	ret = max9271_write(dev, 0x04, val);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(&dev->client->dev, "Failed to set serial link (%d)\n",
+			ret);
 		return ret;
+	}
 
 	usleep_range(5000, 8000);
 
@@ -134,8 +137,11 @@ int max9271_configure_i2c(struct max9271_device *dev, u8 i2c_config)
 	int ret;
 
 	ret = max9271_write(dev, 0x0d, i2c_config);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(&dev->client->dev, "Failed to configure I2C (%d)\n",
+			ret);
 		return ret;
+	}
 
 	/* The delay required after an I2C bus configuration change is not
 	 * characterized in the serializer manual. Sleep up to 5msec to
@@ -153,7 +159,7 @@ int max9271_set_high_threshold(struct max9271_device *dev, bool enable)
 
 	ret = max9271_read(dev, 0x08);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	/*
 	 * Enable or disable reverse channel high threshold to increase
@@ -161,11 +167,15 @@ int max9271_set_high_threshold(struct max9271_device *dev, bool enable)
 	 */
 	ret = max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	usleep_range(2000, 2500);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev, "Failed to set high threshold (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_set_high_threshold);
 
@@ -186,7 +196,7 @@ int max9271_configure_gmsl_link(struct max9271_device *dev)
 	ret = max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
 			    MAX9271_EDC_1BIT_PARITY);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	usleep_range(5000, 8000);
 
@@ -199,11 +209,15 @@ int max9271_configure_gmsl_link(struct max9271_device *dev)
 			    MAX9271_PCLK_AUTODETECT |
 			    MAX9271_SERIAL_AUTODETECT);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	usleep_range(5000, 8000);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev, "Failed to configure GMSL link (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_configure_gmsl_link);
 
@@ -213,18 +227,20 @@ int max9271_set_gpios(struct max9271_device *dev, u8 gpio_mask)
 
 	ret = max9271_read(dev, 0x0f);
 	if (ret < 0)
-		return 0;
+		goto out;
 
 	ret |= gpio_mask;
 	ret = max9271_write(dev, 0x0f, ret);
-	if (ret < 0) {
-		dev_err(&dev->client->dev, "Failed to set gpio (%d)\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		goto out;
 
 	usleep_range(3500, 5000);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev, "Failed to set gpio (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_set_gpios);
 
@@ -234,18 +250,20 @@ int max9271_clear_gpios(struct max9271_device *dev, u8 gpio_mask)
 
 	ret = max9271_read(dev, 0x0f);
 	if (ret < 0)
-		return 0;
+		goto out;
 
 	ret &= ~gpio_mask;
 	ret = max9271_write(dev, 0x0f, ret);
-	if (ret < 0) {
-		dev_err(&dev->client->dev, "Failed to clear gpio (%d)\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		goto out;
 
 	usleep_range(3500, 5000);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev, "Failed to clear gpio (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_clear_gpios);
 
@@ -255,19 +273,21 @@ int max9271_enable_gpios(struct max9271_device *dev, u8 gpio_mask)
 
 	ret = max9271_read(dev, 0x0e);
 	if (ret < 0)
-		return 0;
+		goto out;
 
 	/* BIT(0) reserved: GPO is always enabled. */
 	ret |= (gpio_mask & ~BIT(0));
 	ret = max9271_write(dev, 0x0e, ret);
-	if (ret < 0) {
-		dev_err(&dev->client->dev, "Failed to enable gpio (%d)\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		goto out;
 
 	usleep_range(3500, 5000);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev, "Failed to enable gpio (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_enable_gpios);
 
@@ -277,19 +297,21 @@ int max9271_disable_gpios(struct max9271_device *dev, u8 gpio_mask)
 
 	ret = max9271_read(dev, 0x0e);
 	if (ret < 0)
-		return 0;
+		goto out;
 
 	/* BIT(0) reserved: GPO cannot be disabled */
 	ret &= ~(gpio_mask | BIT(0));
 	ret = max9271_write(dev, 0x0e, ret);
-	if (ret < 0) {
-		dev_err(&dev->client->dev, "Failed to disable gpio (%d)\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		goto out;
 
 	usleep_range(3500, 5000);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev, "Failed to disable gpio (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_disable_gpios);
 
@@ -299,13 +321,13 @@ int max9271_verify_id(struct max9271_device *dev)
 
 	ret = max9271_read(dev, 0x1e);
 	if (ret < 0) {
-		dev_err(&dev->client->dev, "MAX9271 ID read failed (%d)\n",
+		dev_err(&dev->client->dev, "Failed to read the chip ID (%d)\n",
 			ret);
 		return ret;
 	}
 
 	if (ret != MAX9271_ID) {
-		dev_err(&dev->client->dev, "MAX9271 ID mismatch (0x%02x)\n",
+		dev_err(&dev->client->dev, "Chip ID mismatch (0x%02x)\n",
 			ret);
 		return -ENXIO;
 	}
@@ -321,7 +343,7 @@ int max9271_set_address(struct max9271_device *dev, u8 addr)
 	ret = max9271_write(dev, 0x00, addr << 1);
 	if (ret < 0) {
 		dev_err(&dev->client->dev,
-			"MAX9271 I2C address change failed (%d)\n", ret);
+			"Failed to change I2C address (%d)\n", ret);
 		return ret;
 	}
 	usleep_range(3500, 5000);
@@ -337,7 +359,7 @@ int max9271_set_deserializer_address(struct max9271_device *dev, u8 addr)
 	ret = max9271_write(dev, 0x01, addr << 1);
 	if (ret < 0) {
 		dev_err(&dev->client->dev,
-			"MAX9271 deserializer address set failed (%d)\n", ret);
+			"Failed to set deser address (%d)\n", ret);
 		return ret;
 	}
 	usleep_range(3500, 5000);
@@ -351,22 +373,23 @@ int max9271_set_translation(struct max9271_device *dev, u8 source, u8 dest)
 	int ret;
 
 	ret = max9271_write(dev, 0x09, source << 1);
-	if (ret < 0) {
-		dev_err(&dev->client->dev,
-			"MAX9271 I2C translation setup failed (%d)\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		goto out;
+
 	usleep_range(3500, 5000);
 
 	ret = max9271_write(dev, 0x0a, dest << 1);
-	if (ret < 0) {
-		dev_err(&dev->client->dev,
-			"MAX9271 I2C translation setup failed (%d)\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		goto out;
+
 	usleep_range(3500, 5000);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev,
+		"Failed to set I2C addresses translation (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_set_translation);
 
-- 
2.33.1

Re: [PATCH v2 1/2] media: max9271: Ignore busy loop read errors

[Kieran Bingham]

Quoting Jacopo Mondi (2021-11-04 11:09:23)
> Valid pixel clock detection is performed by spinning on a register read
> which if repeated too frequently might fail. As the error is not fatal
> ignore it instead of bailing out to continue spinning until the timeout
> completion.
> 
> Also relax the time between bus transactions and slightly increase the
> wait interval to mitigate the failure risk.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

This seems good to me. In your testing did you identify how many
spins/how long it usually takes before it first detects the pixel clock?

I.e. - was it cutting it close at 10ms, and we should even still extend
this further? (as the usleep_range means we could still loop this 10 ms)

Anyway, this looks like a strong improvement.


Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
> 
> v1->v2:
> - Do not continue but jump to a label to respect the sleep timout after a
>   failed read
> 
> Niklas I kept your tag anyway, hope it's ok.
> 
> Thanks
>    j
> 
> ---
>  drivers/media/i2c/max9271.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
> index ff86c8c4ea61..aa4add473716 100644
> --- a/drivers/media/i2c/max9271.c
> +++ b/drivers/media/i2c/max9271.c
> @@ -55,7 +55,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val)
>  /*
>   * max9271_pclk_detect() - Detect valid pixel clock from image sensor
>   *
> - * Wait up to 10ms for a valid pixel clock.
> + * Wait up to 15ms for a valid pixel clock.
>   *
>   * Returns 0 for success, < 0 for pixel clock not properly detected
>   */
> @@ -64,15 +64,16 @@ static int max9271_pclk_detect(struct max9271_device *dev)
>         unsigned int i;
>         int ret;
> 
> -       for (i = 0; i < 100; i++) {
> +       for (i = 0; i < 10; i++) {
>                 ret = max9271_read(dev, 0x15);
>                 if (ret < 0)
> -                       return ret;
> +                       goto skip;
> 
>                 if (ret & MAX9271_PCLKDET)
>                         return 0;
> 
> -               usleep_range(50, 100);
> +skip:
> +               usleep_range(1000, 1500);
>         }
> 
>         dev_err(&dev->client->dev, "Unable to detect valid pixel clock\n");
> --
> 2.33.1
>

Re: [PATCH v2 2/2] media: max9271: Fail loud on bus errors in call sites

[Kieran Bingham]

Quoting Jacopo Mondi (2021-11-04 11:09:24)
> As not all bus access errors are fatal, as in example reads performed
> in a busy loop, it's responsibility of the bus access function caller
> to fail louder on fatal errors.
> 
> Instrument all functions in the max9271 library driver to fail on fatal
> read/write errors and demote the max9271_write() error level to debug
> to align it to the one in max9271_read().
> 
> While at it, align the style of the existing error messages by removing
> "MAX9271" from the output string, as the device log helpers already
> identify the driver emitting the message.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Seems good to me overall.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/max9271.c | 105 ++++++++++++++++++++++--------------
>  1 file changed, 64 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
> index aa4add473716..f5f354b8a43c 100644
> --- a/drivers/media/i2c/max9271.c
> +++ b/drivers/media/i2c/max9271.c
> @@ -45,7 +45,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val)
>  
>         ret = i2c_smbus_write_byte_data(dev->client, reg, val);
>         if (ret < 0)
> -               dev_err(&dev->client->dev,
> +               dev_dbg(&dev->client->dev,
>                         "%s: register 0x%02x write failed (%d)\n",
>                         __func__, reg, ret);
>  
> @@ -120,8 +120,11 @@ int max9271_set_serial_link(struct max9271_device *dev, bool enable)
>          * Therefore a conservative delay seems best here.
>          */
>         ret = max9271_write(dev, 0x04, val);
> -       if (ret < 0)
> +       if (ret < 0) {
> +               dev_err(&dev->client->dev, "Failed to set serial link (%d)\n",
> +                       ret);
>                 return ret;
> +       }
>  
>         usleep_range(5000, 8000);
>  
> @@ -134,8 +137,11 @@ int max9271_configure_i2c(struct max9271_device *dev, u8 i2c_config)
>         int ret;
>  
>         ret = max9271_write(dev, 0x0d, i2c_config);
> -       if (ret < 0)
> +       if (ret < 0) {
> +               dev_err(&dev->client->dev, "Failed to configure I2C (%d)\n",
> +                       ret);
>                 return ret;
> +       }
>  
>         /* The delay required after an I2C bus configuration change is not
>          * characterized in the serializer manual. Sleep up to 5msec to
> @@ -153,7 +159,7 @@ int max9271_set_high_threshold(struct max9271_device *dev, bool enable)
>  
>         ret = max9271_read(dev, 0x08);
>         if (ret < 0)
> -               return ret;
> +               goto out;
>  
>         /*
>          * Enable or disable reverse channel high threshold to increase
> @@ -161,11 +167,15 @@ int max9271_set_high_threshold(struct max9271_device *dev, bool enable)
>          */
>         ret = max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
>         if (ret < 0)
> -               return ret;
> +               goto out;
>  
>         usleep_range(2000, 2500);
>  
>         return 0;
> +
> +out:
> +       dev_err(&dev->client->dev, "Failed to set high threshold (%d)\n", ret);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(max9271_set_high_threshold);
>  
> @@ -186,7 +196,7 @@ int max9271_configure_gmsl_link(struct max9271_device *dev)
>         ret = max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
>                             MAX9271_EDC_1BIT_PARITY);
>         if (ret < 0)
> -               return ret;
> +               goto out;
>  
>         usleep_range(5000, 8000);
>  
> @@ -199,11 +209,15 @@ int max9271_configure_gmsl_link(struct max9271_device *dev)
>                             MAX9271_PCLK_AUTODETECT |
>                             MAX9271_SERIAL_AUTODETECT);
>         if (ret < 0)
> -               return ret;
> +               goto out;
>  
>         usleep_range(5000, 8000);
>  
>         return 0;
> +
> +out:
> +       dev_err(&dev->client->dev, "Failed to configure GMSL link (%d)\n", ret);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(max9271_configure_gmsl_link);
>  
> @@ -213,18 +227,20 @@ int max9271_set_gpios(struct max9271_device *dev, u8 gpio_mask)
>  
>         ret = max9271_read(dev, 0x0f);
>         if (ret < 0)
> -               return 0;
> +               goto out;
>  
>         ret |= gpio_mask;
>         ret = max9271_write(dev, 0x0f, ret);
> -       if (ret < 0) {
> -               dev_err(&dev->client->dev, "Failed to set gpio (%d)\n", ret);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               goto out;
>  
>         usleep_range(3500, 5000);
>  
>         return 0;
> +
> +out:
> +       dev_err(&dev->client->dev, "Failed to set gpio (%d)\n", ret);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(max9271_set_gpios);
>  
> @@ -234,18 +250,20 @@ int max9271_clear_gpios(struct max9271_device *dev, u8 gpio_mask)
>  
>         ret = max9271_read(dev, 0x0f);
>         if (ret < 0)
> -               return 0;
> +               goto out;
>  
>         ret &= ~gpio_mask;
>         ret = max9271_write(dev, 0x0f, ret);
> -       if (ret < 0) {
> -               dev_err(&dev->client->dev, "Failed to clear gpio (%d)\n", ret);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               goto out;
>  
>         usleep_range(3500, 5000);
>  
>         return 0;
> +
> +out:
> +       dev_err(&dev->client->dev, "Failed to clear gpio (%d)\n", ret);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(max9271_clear_gpios);
>  
> @@ -255,19 +273,21 @@ int max9271_enable_gpios(struct max9271_device *dev, u8 gpio_mask)
>  
>         ret = max9271_read(dev, 0x0e);
>         if (ret < 0)
> -               return 0;
> +               goto out;
>  
>         /* BIT(0) reserved: GPO is always enabled. */
>         ret |= (gpio_mask & ~BIT(0));
>         ret = max9271_write(dev, 0x0e, ret);
> -       if (ret < 0) {
> -               dev_err(&dev->client->dev, "Failed to enable gpio (%d)\n", ret);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               goto out;
>  
>         usleep_range(3500, 5000);
>  
>         return 0;
> +
> +out:
> +       dev_err(&dev->client->dev, "Failed to enable gpio (%d)\n", ret);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(max9271_enable_gpios);
>  
> @@ -277,19 +297,21 @@ int max9271_disable_gpios(struct max9271_device *dev, u8 gpio_mask)
>  
>         ret = max9271_read(dev, 0x0e);
>         if (ret < 0)
> -               return 0;
> +               goto out;
>  
>         /* BIT(0) reserved: GPO cannot be disabled */
>         ret &= ~(gpio_mask | BIT(0));
>         ret = max9271_write(dev, 0x0e, ret);
> -       if (ret < 0) {
> -               dev_err(&dev->client->dev, "Failed to disable gpio (%d)\n", ret);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               goto out;
>  
>         usleep_range(3500, 5000);
>  
>         return 0;
> +
> +out:
> +       dev_err(&dev->client->dev, "Failed to disable gpio (%d)\n", ret);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(max9271_disable_gpios);
>  
> @@ -299,13 +321,13 @@ int max9271_verify_id(struct max9271_device *dev)
>  
>         ret = max9271_read(dev, 0x1e);
>         if (ret < 0) {
> -               dev_err(&dev->client->dev, "MAX9271 ID read failed (%d)\n",
> +               dev_err(&dev->client->dev, "Failed to read the chip ID (%d)\n",
>                         ret);
>                 return ret;
>         }
>  
>         if (ret != MAX9271_ID) {
> -               dev_err(&dev->client->dev, "MAX9271 ID mismatch (0x%02x)\n",
> +               dev_err(&dev->client->dev, "Chip ID mismatch (0x%02x)\n",
>                         ret);
>                 return -ENXIO;
>         }
> @@ -321,7 +343,7 @@ int max9271_set_address(struct max9271_device *dev, u8 addr)
>         ret = max9271_write(dev, 0x00, addr << 1);
>         if (ret < 0) {
>                 dev_err(&dev->client->dev,
> -                       "MAX9271 I2C address change failed (%d)\n", ret);
> +                       "Failed to change I2C address (%d)\n", ret);
>                 return ret;
>         }
>         usleep_range(3500, 5000);
> @@ -337,7 +359,7 @@ int max9271_set_deserializer_address(struct max9271_device *dev, u8 addr)
>         ret = max9271_write(dev, 0x01, addr << 1);
>         if (ret < 0) {
>                 dev_err(&dev->client->dev,
> -                       "MAX9271 deserializer address set failed (%d)\n", ret);
> +                       "Failed to set deser address (%d)\n", ret);
>                 return ret;
>         }
>         usleep_range(3500, 5000);
> @@ -351,22 +373,23 @@ int max9271_set_translation(struct max9271_device *dev, u8 source, u8 dest)
>         int ret;
>  
>         ret = max9271_write(dev, 0x09, source << 1);
> -       if (ret < 0) {
> -               dev_err(&dev->client->dev,
> -                       "MAX9271 I2C translation setup failed (%d)\n", ret);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               goto out;
> +
>         usleep_range(3500, 5000);
>  
>         ret = max9271_write(dev, 0x0a, dest << 1);
> -       if (ret < 0) {
> -               dev_err(&dev->client->dev,
> -                       "MAX9271 I2C translation setup failed (%d)\n", ret);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               goto out;
> +
>         usleep_range(3500, 5000);
>  
>         return 0;
> +
> +out:
> +       dev_err(&dev->client->dev,
> +               "Failed to set I2C addresses translation (%d)\n", ret);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(max9271_set_translation);
>  
> -- 
> 2.33.1
>

Re: [PATCH v2 1/2] media: max9271: Ignore busy loop read errors

[Geert Uytterhoeven]

Hi Jacopo,

On Thu, Nov 4, 2021 at 12:10 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Valid pixel clock detection is performed by spinning on a register read
> which if repeated too frequently might fail. As the error is not fatal
> ignore it instead of bailing out to continue spinning until the timeout
> completion.
>
> Also relax the time between bus transactions and slightly increase the
> wait interval to mitigate the failure risk.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>
> v1->v2:
> - Do not continue but jump to a label to respect the sleep timout after a
>   failed read

Thanks for the update!

> --- a/drivers/media/i2c/max9271.c
> +++ b/drivers/media/i2c/max9271.c
> @@ -55,7 +55,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val)
>  /*
>   * max9271_pclk_detect() - Detect valid pixel clock from image sensor
>   *
> - * Wait up to 10ms for a valid pixel clock.
> + * Wait up to 15ms for a valid pixel clock.
>   *
>   * Returns 0 for success, < 0 for pixel clock not properly detected
>   */
> @@ -64,15 +64,16 @@ static int max9271_pclk_detect(struct max9271_device *dev)
>         unsigned int i;
>         int ret;
>
> -       for (i = 0; i < 100; i++) {
> +       for (i = 0; i < 10; i++) {
>                 ret = max9271_read(dev, 0x15);
>                 if (ret < 0)
> -                       return ret;
> +                       goto skip;

Edgar Dijkstra: Go To Statement Considered Harmful?

>
>                 if (ret & MAX9271_PCLKDET)

"if (ret > 0 && (ret & MAX9271_PCLKDET))"?

>                         return 0;
>
> -               usleep_range(50, 100);
> +skip:
> +               usleep_range(1000, 1500);
>         }
>
>         dev_err(&dev->client->dev, "Unable to detect valid pixel clock\n");

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 1/2] media: max9271: Ignore busy loop read errors

[Jacopo Mondi]

Hi Geert

On Mon, Nov 08, 2021 at 11:45:58AM +0100, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Thu, Nov 4, 2021 at 12:10 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> > Valid pixel clock detection is performed by spinning on a register read
> > which if repeated too frequently might fail. As the error is not fatal
> > ignore it instead of bailing out to continue spinning until the timeout
> > completion.
> >
> > Also relax the time between bus transactions and slightly increase the
> > wait interval to mitigate the failure risk.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >
> > v1->v2:
> > - Do not continue but jump to a label to respect the sleep timout after a
> >   failed read
>
> Thanks for the update!
>
> > --- a/drivers/media/i2c/max9271.c
> > +++ b/drivers/media/i2c/max9271.c
> > @@ -55,7 +55,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val)
> >  /*
> >   * max9271_pclk_detect() - Detect valid pixel clock from image sensor
> >   *
> > - * Wait up to 10ms for a valid pixel clock.
> > + * Wait up to 15ms for a valid pixel clock.
> >   *
> >   * Returns 0 for success, < 0 for pixel clock not properly detected
> >   */
> > @@ -64,15 +64,16 @@ static int max9271_pclk_detect(struct max9271_device *dev)
> >         unsigned int i;
> >         int ret;
> >
> > -       for (i = 0; i < 100; i++) {
> > +       for (i = 0; i < 10; i++) {
> >                 ret = max9271_read(dev, 0x15);
> >                 if (ret < 0)
> > -                       return ret;
> > +                       goto skip;
>
> Edgar Dijkstra: Go To Statement Considered Harmful?
>

Dijkstra would be very unpleased reading the kernel error path
handling implementations. But I got your point, we're not in a cleanup
path here =)

> >
> >                 if (ret & MAX9271_PCLKDET)
>
> "if (ret > 0 && (ret & MAX9271_PCLKDET))"?
>

Much better, thanks. I'll resend!

Thanks
   j

> >                         return 0;
> >
> > -               usleep_range(50, 100);
> > +skip:
> > +               usleep_range(1000, 1500);
> >         }
> >
> >         dev_err(&dev->client->dev, "Unable to detect valid pixel clock\n");
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v2 1/2] media: max9271: Ignore busy loop read errors

[Jacopo Mondi]

Hi Kieran

On Thu, Nov 04, 2021 at 11:04:57PM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2021-11-04 11:09:23)
> > Valid pixel clock detection is performed by spinning on a register read
> > which if repeated too frequently might fail. As the error is not fatal
> > ignore it instead of bailing out to continue spinning until the timeout
> > completion.
> >
> > Also relax the time between bus transactions and slightly increase the
> > wait interval to mitigate the failure risk.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> This seems good to me. In your testing did you identify how many
> spins/how long it usually takes before it first detects the pixel clock?
>
> I.e. - was it cutting it close at 10ms, and we should even still extend
> this further? (as the usleep_range means we could still loop this 10 ms)

Thanks for asking, this turned out to be much quicker than expected
with the pclk_clk detected at the first or second attempts all the
times. I would not bother reducing the sleep time though.

>
> Anyway, this looks like a strong improvement.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Thanks
  j

>
> > ---
> >
> > v1->v2:
> > - Do not continue but jump to a label to respect the sleep timout after a
> >   failed read
> >
> > Niklas I kept your tag anyway, hope it's ok.
> >
> > Thanks
> >    j
> >
> > ---
> >  drivers/media/i2c/max9271.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
> > index ff86c8c4ea61..aa4add473716 100644
> > --- a/drivers/media/i2c/max9271.c
> > +++ b/drivers/media/i2c/max9271.c
> > @@ -55,7 +55,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val)
> >  /*
> >   * max9271_pclk_detect() - Detect valid pixel clock from image sensor
> >   *
> > - * Wait up to 10ms for a valid pixel clock.
> > + * Wait up to 15ms for a valid pixel clock.
> >   *
> >   * Returns 0 for success, < 0 for pixel clock not properly detected
> >   */
> > @@ -64,15 +64,16 @@ static int max9271_pclk_detect(struct max9271_device *dev)
> >         unsigned int i;
> >         int ret;
> >
> > -       for (i = 0; i < 100; i++) {
> > +       for (i = 0; i < 10; i++) {
> >                 ret = max9271_read(dev, 0x15);
> >                 if (ret < 0)
> > -                       return ret;
> > +                       goto skip;
> >
> >                 if (ret & MAX9271_PCLKDET)
> >                         return 0;
> >
> > -               usleep_range(50, 100);
> > +skip:
> > +               usleep_range(1000, 1500);
> >         }
> >
> >         dev_err(&dev->client->dev, "Unable to detect valid pixel clock\n");
> > --
> > 2.33.1
> >