linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] media: max9271: Fix GPIO handling
@ 2020-11-20 16:15 Jacopo Mondi
  2020-11-20 16:15 ` [PATCH 1/2] media: max9271: Fix GPIO enable/disable Jacopo Mondi
  2020-11-20 16:15 ` [PATCH 2/2] media: rdacm20: Enable GPIO1 explicitly Jacopo Mondi
  0 siblings, 2 replies; 7+ messages in thread
From: Jacopo Mondi @ 2020-11-20 16:15 UTC (permalink / raw)
  To: kieran.bingham+renesas, laurent.pinchart+renesas,
	niklas.soderlund+renesas, geert
  Cc: Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel

While working on RDACM21 camera integration I found the GPIO handling
code in the max9271 library driver had a few bugs, that went un-noticed
when the library has been used to upstream RDACM20 as the GPIO lines are
not explicitly enabled.

Fix the GPIO handling code in the max9271 library driver and make rdacm20
a little more robust by enabling the GPIO1 line explicitly.

Tested on H3 Salvator-X with MAXIM GMLS expansion board.

Thanks
   j

Jacopo Mondi (2):
  media: max9271: Fix GPIO enable/disable
  media: rdacm20: Enable GPIO1 explicitly

 drivers/media/i2c/max9271.c |  8 ++++----
 drivers/media/i2c/rdacm20.c | 13 +++++++++++--
 2 files changed, 15 insertions(+), 6 deletions(-)

--
2.29.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] media: max9271: Fix GPIO enable/disable
  2020-11-20 16:15 [PATCH 0/2] media: max9271: Fix GPIO handling Jacopo Mondi
@ 2020-11-20 16:15 ` Jacopo Mondi
  2020-11-20 19:12   ` Geert Uytterhoeven
                     ` (2 more replies)
  2020-11-20 16:15 ` [PATCH 2/2] media: rdacm20: Enable GPIO1 explicitly Jacopo Mondi
  1 sibling, 3 replies; 7+ messages in thread
From: Jacopo Mondi @ 2020-11-20 16:15 UTC (permalink / raw)
  To: kieran.bingham+renesas, laurent.pinchart+renesas,
	niklas.soderlund+renesas, geert
  Cc: Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel

Fix GPIO enable/disable operations which wrongly read the 0x0f register
to obtain the current mask of the enabled lines instead of using
the correct 0x0e register.

Also fix access to bit 0 of the register which is marked as reserved.

Fixes: 34009bffc1c6 ("media: i2c: Add RDACM20 driver")
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9271.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index 0f6f7a092a46..c247db569bab 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -223,12 +223,12 @@ int max9271_enable_gpios(struct max9271_device *dev, u8 gpio_mask)
 {
 	int ret;
 
-	ret = max9271_read(dev, 0x0f);
+	ret = max9271_read(dev, 0x0e);
 	if (ret < 0)
 		return 0;
 
 	/* BIT(0) reserved: GPO is always enabled. */
-	ret |= gpio_mask | BIT(0);
+	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);
@@ -245,12 +245,12 @@ int max9271_disable_gpios(struct max9271_device *dev, u8 gpio_mask)
 {
 	int ret;
 
-	ret = max9271_read(dev, 0x0f);
+	ret = max9271_read(dev, 0x0e);
 	if (ret < 0)
 		return 0;
 
 	/* BIT(0) reserved: GPO cannot be disabled */
-	ret &= (~gpio_mask | BIT(0));
+	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);
-- 
2.29.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] media: rdacm20: Enable GPIO1 explicitly
  2020-11-20 16:15 [PATCH 0/2] media: max9271: Fix GPIO handling Jacopo Mondi
  2020-11-20 16:15 ` [PATCH 1/2] media: max9271: Fix GPIO enable/disable Jacopo Mondi
@ 2020-11-20 16:15 ` Jacopo Mondi
  2020-11-23 14:19   ` Kieran Bingham
  1 sibling, 1 reply; 7+ messages in thread
From: Jacopo Mondi @ 2020-11-20 16:15 UTC (permalink / raw)
  To: kieran.bingham+renesas, laurent.pinchart+renesas,
	niklas.soderlund+renesas, geert
  Cc: Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel

The MAX9271 GPIO1 line that controls the sensor reset is by default
enabled after a serializer chip reset.

As rdacm20 does not go through an explicit serializer reset, make sure
GPIO1 is enabled to make the camera module driver more robust.

Fixes: 34009bffc1c6 ("media: i2c: Add RDACM20 driver")
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 1ed928c4ca70..16bcb764b0e0 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -487,9 +487,18 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	 * Reset the sensor by cycling the OV10635 reset signal connected to the
 	 * MAX9271 GPIO1 and verify communication with the OV10635.
 	 */
-	max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
+	ret = max9271_enable_gpios(dev->serializer, MAX9271_GPIO1OUT);
+	if (ret)
+		return ret;
+
+	ret = max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
+	if (ret)
+		return ret;
 	usleep_range(10000, 15000);
-	max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
+
+	ret = max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
+	if (ret)
+		return ret;
 	usleep_range(10000, 15000);

 again:
--
2.29.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] media: max9271: Fix GPIO enable/disable
  2020-11-20 16:15 ` [PATCH 1/2] media: max9271: Fix GPIO enable/disable Jacopo Mondi
@ 2020-11-20 19:12   ` Geert Uytterhoeven
  2020-11-23 14:14   ` Kieran Bingham
  2020-11-23 15:59   ` Sergei Shtylyov
  2 siblings, 0 replies; 7+ messages in thread
From: Geert Uytterhoeven @ 2020-11-20 19:12 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Kieran Bingham, Laurent Pinchart, Niklas Söderlund,
	Linux Media Mailing List, Linux-Renesas,
	Linux Kernel Mailing List

On Fri, Nov 20, 2020 at 5:15 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Fix GPIO enable/disable operations which wrongly read the 0x0f register
> to obtain the current mask of the enabled lines instead of using
> the correct 0x0e register.
>
> Also fix access to bit 0 of the register which is marked as reserved.
>
> Fixes: 34009bffc1c6 ("media: i2c: Add RDACM20 driver")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] media: max9271: Fix GPIO enable/disable
  2020-11-20 16:15 ` [PATCH 1/2] media: max9271: Fix GPIO enable/disable Jacopo Mondi
  2020-11-20 19:12   ` Geert Uytterhoeven
@ 2020-11-23 14:14   ` Kieran Bingham
  2020-11-23 15:59   ` Sergei Shtylyov
  2 siblings, 0 replies; 7+ messages in thread
From: Kieran Bingham @ 2020-11-23 14:14 UTC (permalink / raw)
  To: Jacopo Mondi, laurent.pinchart+renesas, niklas.soderlund+renesas, geert
  Cc: linux-media, linux-renesas-soc, linux-kernel

Hi Jacopo,

On 20/11/2020 16:15, Jacopo Mondi wrote:
> Fix GPIO enable/disable operations which wrongly read the 0x0f register
> to obtain the current mask of the enabled lines instead of using
> the correct 0x0e register.
> 
> Also fix access to bit 0 of the register which is marked as reserved.
> 
> Fixes: 34009bffc1c6 ("media: i2c: Add RDACM20 driver")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Took me a few reads of this and the datasheet to be sure :S

But now I am :-D

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

> ---
>  drivers/media/i2c/max9271.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
> index 0f6f7a092a46..c247db569bab 100644
> --- a/drivers/media/i2c/max9271.c
> +++ b/drivers/media/i2c/max9271.c
> @@ -223,12 +223,12 @@ int max9271_enable_gpios(struct max9271_device *dev, u8 gpio_mask)
>  {
>  	int ret;
>  
> -	ret = max9271_read(dev, 0x0f);
> +	ret = max9271_read(dev, 0x0e);
>  	if (ret < 0)
>  		return 0;
>  
>  	/* BIT(0) reserved: GPO is always enabled. */
> -	ret |= gpio_mask | BIT(0);
> +	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);
> @@ -245,12 +245,12 @@ int max9271_disable_gpios(struct max9271_device *dev, u8 gpio_mask)
>  {
>  	int ret;
>  
> -	ret = max9271_read(dev, 0x0f);
> +	ret = max9271_read(dev, 0x0e);
>  	if (ret < 0)
>  		return 0;
>  
>  	/* BIT(0) reserved: GPO cannot be disabled */
> -	ret &= (~gpio_mask | BIT(0));
> +	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);
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] media: rdacm20: Enable GPIO1 explicitly
  2020-11-20 16:15 ` [PATCH 2/2] media: rdacm20: Enable GPIO1 explicitly Jacopo Mondi
@ 2020-11-23 14:19   ` Kieran Bingham
  0 siblings, 0 replies; 7+ messages in thread
From: Kieran Bingham @ 2020-11-23 14:19 UTC (permalink / raw)
  To: Jacopo Mondi, laurent.pinchart+renesas, niklas.soderlund+renesas, geert
  Cc: linux-media, linux-renesas-soc, linux-kernel

Hi Jacopo,

On 20/11/2020 16:15, Jacopo Mondi wrote:
> The MAX9271 GPIO1 line that controls the sensor reset is by default
> enabled after a serializer chip reset.
> 
> As rdacm20 does not go through an explicit serializer reset, make sure
> GPIO1 is enabled to make the camera module driver more robust.
> 
> Fixes: 34009bffc1c6 ("media: i2c: Add RDACM20 driver")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Looks helpful to ensure a GPIO is enabled before toggling ;-)

Reading the datasheet, GPIO1EN defaults to enabled, so I assume this was
already working - however I think being explicit is a good thing anyway.

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


> ---
>  drivers/media/i2c/rdacm20.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 1ed928c4ca70..16bcb764b0e0 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -487,9 +487,18 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	 * Reset the sensor by cycling the OV10635 reset signal connected to the
>  	 * MAX9271 GPIO1 and verify communication with the OV10635.
>  	 */
> -	max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
> +	ret = max9271_enable_gpios(dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
> +
> +	ret = max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
>  	usleep_range(10000, 15000);
> -	max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
> +
> +	ret = max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
>  	usleep_range(10000, 15000);
> 
>  again:
> --
> 2.29.1
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] media: max9271: Fix GPIO enable/disable
  2020-11-20 16:15 ` [PATCH 1/2] media: max9271: Fix GPIO enable/disable Jacopo Mondi
  2020-11-20 19:12   ` Geert Uytterhoeven
  2020-11-23 14:14   ` Kieran Bingham
@ 2020-11-23 15:59   ` Sergei Shtylyov
  2 siblings, 0 replies; 7+ messages in thread
From: Sergei Shtylyov @ 2020-11-23 15:59 UTC (permalink / raw)
  To: Jacopo Mondi, kieran.bingham+renesas, laurent.pinchart+renesas,
	niklas.soderlund+renesas, geert
  Cc: linux-media, linux-renesas-soc, linux-kernel

On 11/20/20 7:15 PM, Jacopo Mondi wrote:

> Fix GPIO enable/disable operations which wrongly read the 0x0f register
> to obtain the current mask of the enabled lines instead of using
> the correct 0x0e register.
> 
> Also fix access to bit 0 of the register which is marked as reserved.
> 
> Fixes: 34009bffc1c6 ("media: i2c: Add RDACM20 driver")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9271.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
> index 0f6f7a092a46..c247db569bab 100644
> --- a/drivers/media/i2c/max9271.c
> +++ b/drivers/media/i2c/max9271.c
> @@ -223,12 +223,12 @@ int max9271_enable_gpios(struct max9271_device *dev, u8 gpio_mask)
>  {
>  	int ret;
>  
> -	ret = max9271_read(dev, 0x0f);
> +	ret = max9271_read(dev, 0x0e);
>  	if (ret < 0)
>  		return 0;
>  
>  	/* BIT(0) reserved: GPO is always enabled. */
> -	ret |= gpio_mask | BIT(0);
> +	ret |= (gpio_mask & ~BIT(0));

   () hardly needed here, = and <op>= have very low prio...

[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-11-23 16:00 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 16:15 [PATCH 0/2] media: max9271: Fix GPIO handling Jacopo Mondi
2020-11-20 16:15 ` [PATCH 1/2] media: max9271: Fix GPIO enable/disable Jacopo Mondi
2020-11-20 19:12   ` Geert Uytterhoeven
2020-11-23 14:14   ` Kieran Bingham
2020-11-23 15:59   ` Sergei Shtylyov
2020-11-20 16:15 ` [PATCH 2/2] media: rdacm20: Enable GPIO1 explicitly Jacopo Mondi
2020-11-23 14:19   ` Kieran Bingham

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).