Re: [PATCH v3 19/19] media: i2c: rdacm20: Re-work ov10635 reset
Hi Laurent, On Sat, Mar 20, 2021 at 06:24:09PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, Mar 19, 2021 at 05:41:48PM +0100, Jacopo Mondi wrote: > > The OV10635 image sensor embedded in the camera module is currently > > reset after the MAX9271 initialization with two long delays that were > > most probably not correctly characterized. > > > > Re-work the image sensor reset procedure by holding the chip in reset > > during the MAX9271 configuration, removing the long sleep delays and > > only wait after the chip exits from reset for 350-500 microseconds > > interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout > > characterized in the chip manual. > > > > Reviewed-by: Kieran Bingham > > Reviewed-by: Laurent Pinchart > > Signed-off-by: Jacopo Mondi > > --- > > drivers/media/i2c/rdacm20.c | 29 + > > 1 file changed, 17 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > > index 7ed3866b5335..7ba2d0249da8 100644 > > --- a/drivers/media/i2c/rdacm20.c > > +++ b/drivers/media/i2c/rdacm20.c > > @@ -454,6 +454,19 @@ static int rdacm20_init(struct v4l2_subdev *sd, > > unsigned int val) > > if (ret) > > return ret; > > > > + /* > > +* Hold OV10635 in reset during max9271 configuration. The reset signal > > +* has to be asserted for at least 200 microseconds. > > +*/ > > + ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); > > + if (ret) > > + return ret; > > + > > + ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); > > enable and clear is very cnonfusing. How about mimicking the GPIO API, > with direction_input(), direction_output() and set_value() functions ? > It would also be nice if the polarity could be handled in a nicer way. > There's no GPIO request API here, but maybe a max9271_gpio_set_flags() ? > Probably, that's an unrelated rework that could go on top unrelated to this patch > > + if (ret) > > + return ret; > > + usleep_range(200, 500); > > + > > ret = max9271_configure_gmsl_link(>serializer); > > if (ret) > > return ret; > > @@ -468,22 +481,14 @@ static int rdacm20_init(struct v4l2_subdev *sd, > > unsigned int val) > > dev->serializer.client->addr = dev->addrs[0]; > > > > /* > > -* Reset the sensor by cycling the OV10635 reset signal connected to the > > -* MAX9271 GPIO1 and verify communication with the OV10635. > > +* Release ov10635 from reset and initialize it. The image sensor > > +* requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz) > > +* before being available. Stay safe and wait up to 500 micro-seconds. > > */ > > - ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); > > - if (ret) > > - return ret; > > - > > - ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); > > - if (ret) > > - return ret; > > - usleep_range(1, 15000); > > - > > ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT); > > if (ret) > > return ret; > > - usleep_range(1, 15000); > > + usleep_range(100, 500); > > > > for (i = 0; i < OV10635_PID_TIMEOUT; ++i) { > > ret = ov10635_read16(dev, OV10635_PID); > > -- > Regards, > > Laurent Pinchart
Re: [PATCH v3 19/19] media: i2c: rdacm20: Re-work ov10635 reset
Hi Jacopo, Thank you for the patch. On Fri, Mar 19, 2021 at 05:41:48PM +0100, Jacopo Mondi wrote: > The OV10635 image sensor embedded in the camera module is currently > reset after the MAX9271 initialization with two long delays that were > most probably not correctly characterized. > > Re-work the image sensor reset procedure by holding the chip in reset > during the MAX9271 configuration, removing the long sleep delays and > only wait after the chip exits from reset for 350-500 microseconds > interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout > characterized in the chip manual. > > Reviewed-by: Kieran Bingham > Reviewed-by: Laurent Pinchart > Signed-off-by: Jacopo Mondi > --- > drivers/media/i2c/rdacm20.c | 29 + > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > index 7ed3866b5335..7ba2d0249da8 100644 > --- a/drivers/media/i2c/rdacm20.c > +++ b/drivers/media/i2c/rdacm20.c > @@ -454,6 +454,19 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned > int val) > if (ret) > return ret; > > + /* > + * Hold OV10635 in reset during max9271 configuration. The reset signal > + * has to be asserted for at least 200 microseconds. > + */ > + ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); > + if (ret) > + return ret; > + > + ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); enable and clear is very cnonfusing. How about mimicking the GPIO API, with direction_input(), direction_output() and set_value() functions ? It would also be nice if the polarity could be handled in a nicer way. There's no GPIO request API here, but maybe a max9271_gpio_set_flags() ? > + if (ret) > + return ret; > + usleep_range(200, 500); > + > ret = max9271_configure_gmsl_link(>serializer); > if (ret) > return ret; > @@ -468,22 +481,14 @@ static int rdacm20_init(struct v4l2_subdev *sd, > unsigned int val) > dev->serializer.client->addr = dev->addrs[0]; > > /* > - * Reset the sensor by cycling the OV10635 reset signal connected to the > - * MAX9271 GPIO1 and verify communication with the OV10635. > + * Release ov10635 from reset and initialize it. The image sensor > + * requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz) > + * before being available. Stay safe and wait up to 500 micro-seconds. >*/ > - ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); > - if (ret) > - return ret; > - > - ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); > - if (ret) > - return ret; > - usleep_range(1, 15000); > - > ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT); > if (ret) > return ret; > - usleep_range(1, 15000); > + usleep_range(100, 500); > > for (i = 0; i < OV10635_PID_TIMEOUT; ++i) { > ret = ov10635_read16(dev, OV10635_PID); -- Regards, Laurent Pinchart
[PATCH v3 19/19] media: i2c: rdacm20: Re-work ov10635 reset
The OV10635 image sensor embedded in the camera module is currently reset after the MAX9271 initialization with two long delays that were most probably not correctly characterized. Re-work the image sensor reset procedure by holding the chip in reset during the MAX9271 configuration, removing the long sleep delays and only wait after the chip exits from reset for 350-500 microseconds interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout characterized in the chip manual. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/rdacm20.c | 29 + 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c index 7ed3866b5335..7ba2d0249da8 100644 --- a/drivers/media/i2c/rdacm20.c +++ b/drivers/media/i2c/rdacm20.c @@ -454,6 +454,19 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val) if (ret) return ret; + /* +* Hold OV10635 in reset during max9271 configuration. The reset signal +* has to be asserted for at least 200 microseconds. +*/ + ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); + if (ret) + return ret; + + ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); + if (ret) + return ret; + usleep_range(200, 500); + ret = max9271_configure_gmsl_link(>serializer); if (ret) return ret; @@ -468,22 +481,14 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val) dev->serializer.client->addr = dev->addrs[0]; /* -* Reset the sensor by cycling the OV10635 reset signal connected to the -* MAX9271 GPIO1 and verify communication with the OV10635. +* Release ov10635 from reset and initialize it. The image sensor +* requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz) +* before being available. Stay safe and wait up to 500 micro-seconds. */ - ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); - if (ret) - return ret; - - ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); - if (ret) - return ret; - usleep_range(1, 15000); - ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT); if (ret) return ret; - usleep_range(1, 15000); + usleep_range(100, 500); for (i = 0; i < OV10635_PID_TIMEOUT; ++i) { ret = ov10635_read16(dev, OV10635_PID); -- 2.30.0