diff mbox series

[v2,09/18] media: i2c: max9271: Introduce wake_up() function

Message ID 20210315131512.133720-10-jacopo+renesas@jmondi.org (mailing list archive)
State New, archived
Headers show
Series media: gmsl: Reliability improvement | expand

Commit Message

Jacopo Mondi March 15, 2021, 1:15 p.m. UTC
The MAX9271 chip manual prescribes a delay of 5 milliseconds
after the chip exists from low power state.

Add a new function to the max9271 library driver that wakes up the chip
with a dummy i2c transaction and implements the correct delay of 5
milliseconds after the chip exits from low power state.

Use the newly introduced function in the rdacm20 and rdacm21 camera
drivers. The former was not respecting the required delay while the
latter was waiting for a too-short timeout.

Do not handle the initial i2c address configuration in the library
driver function as the camera module drivers control address
reprogramming.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9271.c | 7 +++++++
 drivers/media/i2c/max9271.h | 9 +++++++++
 drivers/media/i2c/rdacm20.c | 2 +-
 drivers/media/i2c/rdacm21.c | 3 +--
 4 files changed, 18 insertions(+), 3 deletions(-)

Comments

Kieran Bingham March 15, 2021, 5:14 p.m. UTC | #1
On 15/03/2021 13:15, Jacopo Mondi wrote:
> The MAX9271 chip manual prescribes a delay of 5 milliseconds
> after the chip exists from low power state.
> 
> Add a new function to the max9271 library driver that wakes up the chip
> with a dummy i2c transaction and implements the correct delay of 5
> milliseconds after the chip exits from low power state.
> 
> Use the newly introduced function in the rdacm20 and rdacm21 camera
> drivers. The former was not respecting the required delay while the
> latter was waiting for a too-short timeout.
> 
> Do not handle the initial i2c address configuration in the library
> driver function as the camera module drivers control address
> reprogramming.

I still think that the MAX9271_DEFAULT_ADDR belongs in the max9271
library, but this patch is functionally good to me, and correcting those
delays is certainly a good thing to fix here.

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


> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9271.c | 7 +++++++
>  drivers/media/i2c/max9271.h | 9 +++++++++
>  drivers/media/i2c/rdacm20.c | 2 +-
>  drivers/media/i2c/rdacm21.c | 3 +--
>  4 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
> index 2c7dc7fb9846..f7bfe7266763 100644
> --- a/drivers/media/i2c/max9271.c
> +++ b/drivers/media/i2c/max9271.c
> @@ -80,6 +80,13 @@ static int max9271_pclk_detect(struct max9271_device *dev)
>  	return -EIO;
>  }
>  
> +void max9271_wake_up(struct max9271_device *dev)
> +{
> +	i2c_smbus_read_byte(dev->client);
> +	usleep_range(5000, 8000);
> +}
> +EXPORT_SYMBOL_GPL(max9271_wake_up);
> +
>  int max9271_set_serial_link(struct max9271_device *dev, bool enable)
>  {
>  	int ret;
> diff --git a/drivers/media/i2c/max9271.h b/drivers/media/i2c/max9271.h
> index d78fb21441e9..dc5e4e70ba6f 100644
> --- a/drivers/media/i2c/max9271.h
> +++ b/drivers/media/i2c/max9271.h
> @@ -85,6 +85,15 @@ struct max9271_device {
>  	struct i2c_client *client;
>  };
>  
> +/**
> + * max9271_wake_up() - Wake up the serializer by issuing an i2c transaction
> + * @dev: The max9271 device
> + *
> + * This function shall be called before any other interaction with the
> + * serializer.
> + */
> +void max9271_wake_up(struct max9271_device *dev);
> +
>  /**
>   * max9271_set_serial_link() - Enable/disable serial link
>   * @dev: The max9271 device
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index b9aaa0f7db42..2265ef7c65d4 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -459,7 +459,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  
>  	/* Verify communication with the MAX9271: ping to wakeup. */
>  	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> -	i2c_smbus_read_byte(dev->serializer.client);
> +	max9271_wake_up(&dev->serializer);
>  
>  	/* Serial link disabled during config as it needs a valid pixel clock. */
>  	ret = max9271_set_serial_link(&dev->serializer, false);
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 179d107f494c..7bce55adfd7c 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -452,8 +452,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>  
>  	/* Verify communication with the MAX9271: ping to wakeup. */
>  	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> -	i2c_smbus_read_byte(dev->serializer.client);
> -	usleep_range(3000, 5000);
> +	max9271_wake_up(&dev->serializer);
>  
>  	/* Enable reverse channel and disable the serial link. */
>  	ret = max9271_set_serial_link(&dev->serializer, false);
>
Laurent Pinchart March 15, 2021, 9:43 p.m. UTC | #2
Hi Jacopo,

Thank you for the patch.

On Mon, Mar 15, 2021 at 02:15:03PM +0100, Jacopo Mondi wrote:
> The MAX9271 chip manual prescribes a delay of 5 milliseconds
> after the chip exists from low power state.

I don't think we'll ever try to access the chip within 5ms of the
beginning of its existence.

s/exists/exits/

:-)

> Add a new function to the max9271 library driver that wakes up the chip
> with a dummy i2c transaction and implements the correct delay of 5
> milliseconds after the chip exits from low power state.
> 
> Use the newly introduced function in the rdacm20 and rdacm21 camera
> drivers. The former was not respecting the required delay while the
> latter was waiting for a too-short timeout.
> 
> Do not handle the initial i2c address configuration in the library
> driver function as the camera module drivers control address
> reprogramming.

Isn't the initial address fixed though ? Nonetheless, this can be
addressed separately, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9271.c | 7 +++++++
>  drivers/media/i2c/max9271.h | 9 +++++++++
>  drivers/media/i2c/rdacm20.c | 2 +-
>  drivers/media/i2c/rdacm21.c | 3 +--
>  4 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
> index 2c7dc7fb9846..f7bfe7266763 100644
> --- a/drivers/media/i2c/max9271.c
> +++ b/drivers/media/i2c/max9271.c
> @@ -80,6 +80,13 @@ static int max9271_pclk_detect(struct max9271_device *dev)
>  	return -EIO;
>  }
>  
> +void max9271_wake_up(struct max9271_device *dev)
> +{
> +	i2c_smbus_read_byte(dev->client);
> +	usleep_range(5000, 8000);
> +}
> +EXPORT_SYMBOL_GPL(max9271_wake_up);
> +
>  int max9271_set_serial_link(struct max9271_device *dev, bool enable)
>  {
>  	int ret;
> diff --git a/drivers/media/i2c/max9271.h b/drivers/media/i2c/max9271.h
> index d78fb21441e9..dc5e4e70ba6f 100644
> --- a/drivers/media/i2c/max9271.h
> +++ b/drivers/media/i2c/max9271.h
> @@ -85,6 +85,15 @@ struct max9271_device {
>  	struct i2c_client *client;
>  };
>  
> +/**
> + * max9271_wake_up() - Wake up the serializer by issuing an i2c transaction
> + * @dev: The max9271 device
> + *
> + * This function shall be called before any other interaction with the
> + * serializer.
> + */
> +void max9271_wake_up(struct max9271_device *dev);
> +
>  /**
>   * max9271_set_serial_link() - Enable/disable serial link
>   * @dev: The max9271 device
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index b9aaa0f7db42..2265ef7c65d4 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -459,7 +459,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  
>  	/* Verify communication with the MAX9271: ping to wakeup. */
>  	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> -	i2c_smbus_read_byte(dev->serializer.client);
> +	max9271_wake_up(&dev->serializer);
>  
>  	/* Serial link disabled during config as it needs a valid pixel clock. */
>  	ret = max9271_set_serial_link(&dev->serializer, false);
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 179d107f494c..7bce55adfd7c 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -452,8 +452,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>  
>  	/* Verify communication with the MAX9271: ping to wakeup. */
>  	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> -	i2c_smbus_read_byte(dev->serializer.client);
> -	usleep_range(3000, 5000);
> +	max9271_wake_up(&dev->serializer);
>  
>  	/* Enable reverse channel and disable the serial link. */
>  	ret = max9271_set_serial_link(&dev->serializer, false);
diff mbox series

Patch

diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index 2c7dc7fb9846..f7bfe7266763 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -80,6 +80,13 @@  static int max9271_pclk_detect(struct max9271_device *dev)
 	return -EIO;
 }
 
+void max9271_wake_up(struct max9271_device *dev)
+{
+	i2c_smbus_read_byte(dev->client);
+	usleep_range(5000, 8000);
+}
+EXPORT_SYMBOL_GPL(max9271_wake_up);
+
 int max9271_set_serial_link(struct max9271_device *dev, bool enable)
 {
 	int ret;
diff --git a/drivers/media/i2c/max9271.h b/drivers/media/i2c/max9271.h
index d78fb21441e9..dc5e4e70ba6f 100644
--- a/drivers/media/i2c/max9271.h
+++ b/drivers/media/i2c/max9271.h
@@ -85,6 +85,15 @@  struct max9271_device {
 	struct i2c_client *client;
 };
 
+/**
+ * max9271_wake_up() - Wake up the serializer by issuing an i2c transaction
+ * @dev: The max9271 device
+ *
+ * This function shall be called before any other interaction with the
+ * serializer.
+ */
+void max9271_wake_up(struct max9271_device *dev);
+
 /**
  * max9271_set_serial_link() - Enable/disable serial link
  * @dev: The max9271 device
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index b9aaa0f7db42..2265ef7c65d4 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -459,7 +459,7 @@  static int rdacm20_initialize(struct rdacm20_device *dev)
 
 	/* Verify communication with the MAX9271: ping to wakeup. */
 	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
-	i2c_smbus_read_byte(dev->serializer.client);
+	max9271_wake_up(&dev->serializer);
 
 	/* Serial link disabled during config as it needs a valid pixel clock. */
 	ret = max9271_set_serial_link(&dev->serializer, false);
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 179d107f494c..7bce55adfd7c 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -452,8 +452,7 @@  static int rdacm21_initialize(struct rdacm21_device *dev)
 
 	/* Verify communication with the MAX9271: ping to wakeup. */
 	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
-	i2c_smbus_read_byte(dev->serializer.client);
-	usleep_range(3000, 5000);
+	max9271_wake_up(&dev->serializer);
 
 	/* Enable reverse channel and disable the serial link. */
 	ret = max9271_set_serial_link(&dev->serializer, false);