| Message ID | 20210315131512.133720-12-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: gmsl: Reliability improvement | expand |
Hi Jacopo, On 15/03/2021 13:15, Jacopo Mondi wrote: > The OV10640 image sensor powerdown signal is controlled by the first > line of the OV490 GPIO pad #1, but the pad #0 identifier > OV490_GPIO_OUTPUT_VALUE0 was erroneously used. As a result the image > sensor powerdown signal was never asserted but was kept high by an > internal pull-up resistor, causing sporadic failures during the > image sensor startup phase. > > Fix this by using the correct GPIO pad identifier. > > While at it also fix the GPIO signal handling sequence, as the reset > line was released before the powerdown one, and introduce the correct > delays in between the two operations. > > Wait the mandatory 1 millisecond delay after the powerup lane is Technically you wait 1.5 milliseconds below - but I think that's fine here ;-) > asserted. The reset delay is not characterized in the chip manual if > not as "255 XVCLK + initialization". Wait for at least 3 milliseconds > to guarantee the SCCB bus is available. > > This commit fixes a sporadic start-up error triggered by a failure to > read the OV10640 chip ID: > rdacm21 8-0054: OV10640 ID mismatch: (0x01) > > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module") > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Sometime it might be nice to see this look more like the GPIO interfaces, but I think this is fine for now. Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/media/i2c/rdacm21.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index 7bce55adfd7c..50a9b0d8255d 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev) > { > u8 val; > > - /* Power-up OV10640 by setting RESETB and PWDNB pins high. */ > + /* Power-up OV10640 by setting PWDNB and RESETB pins high. */ > ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0); > ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0); > ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0); > ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0); > + > + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE1, OV490_SPWDN0); > + usleep_range(1500, 3000); > ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); > - ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0); > usleep_range(3000, 5000); > > /* Read OV10640 ID to test communications. */ >
Hi Jacopo, Thank you for the patch. On Mon, Mar 15, 2021 at 02:15:05PM +0100, Jacopo Mondi wrote: > The OV10640 image sensor powerdown signal is controlled by the first > line of the OV490 GPIO pad #1, but the pad #0 identifier > OV490_GPIO_OUTPUT_VALUE0 was erroneously used. As a result the image > sensor powerdown signal was never asserted but was kept high by an > internal pull-up resistor, causing sporadic failures during the > image sensor startup phase. > > Fix this by using the correct GPIO pad identifier. > > While at it also fix the GPIO signal handling sequence, as the reset > line was released before the powerdown one, and introduce the correct > delays in between the two operations. > > Wait the mandatory 1 millisecond delay after the powerup lane is > asserted. The reset delay is not characterized in the chip manual if > not as "255 XVCLK + initialization". Wait for at least 3 milliseconds > to guarantee the SCCB bus is available. > > This commit fixes a sporadic start-up error triggered by a failure to > read the OV10640 chip ID: > rdacm21 8-0054: OV10640 ID mismatch: (0x01) > > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module") > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/rdacm21.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index 7bce55adfd7c..50a9b0d8255d 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev) > { > u8 val; > > - /* Power-up OV10640 by setting RESETB and PWDNB pins high. */ > + /* Power-up OV10640 by setting PWDNB and RESETB pins high. */ > ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0); > ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0); > ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0); > ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0); > + > + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE1, OV490_SPWDN0); > + usleep_range(1500, 3000); > ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); > - ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0); > usleep_range(3000, 5000); > > /* Read OV10640 ID to test communications. */
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index 7bce55adfd7c..50a9b0d8255d 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev) { u8 val; - /* Power-up OV10640 by setting RESETB and PWDNB pins high. */ + /* Power-up OV10640 by setting PWDNB and RESETB pins high. */ ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0); ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0); ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0); ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0); + + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE1, OV490_SPWDN0); + usleep_range(1500, 3000); ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); - ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0); usleep_range(3000, 5000); /* Read OV10640 ID to test communications. */
The OV10640 image sensor powerdown signal is controlled by the first line of the OV490 GPIO pad #1, but the pad #0 identifier OV490_GPIO_OUTPUT_VALUE0 was erroneously used. As a result the image sensor powerdown signal was never asserted but was kept high by an internal pull-up resistor, causing sporadic failures during the image sensor startup phase. Fix this by using the correct GPIO pad identifier. While at it also fix the GPIO signal handling sequence, as the reset line was released before the powerdown one, and introduce the correct delays in between the two operations. Wait the mandatory 1 millisecond delay after the powerup lane is asserted. The reset delay is not characterized in the chip manual if not as "255 XVCLK + initialization". Wait for at least 3 milliseconds to guarantee the SCCB bus is available. This commit fixes a sporadic start-up error triggered by a failure to read the OV10640 chip ID: rdacm21 8-0054: OV10640 ID mismatch: (0x01) Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module") Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/i2c/rdacm21.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)