* [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 related [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 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
* [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 related [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
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).