| Message ID | 20221216134409.6868-2-j-luthra@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: ov5640: Fix power up sequence delays | expand |
On 16/12/2022 14:57, Sakari Ailus wrote: > Hi Jai, > > Thanks for the set. > > On Fri, Dec 16, 2022 at 07:14:07PM +0530, Jai Luthra wrote: >> Some module manufacturers [1][2] don't expose the RESETB gpio of the >> sensor directly through the 15-pin FFC connector. Instead wiring ~PWDN >> as a proxy reset with appropriate delays. >> >> In such cases, reset_gpio will not be available to the driver, but it >> will still be toggled when the sensor is powered on, and thus we should >> still honor the wait time of >= 5ms + 1ms + 20ms (see figure 2-3 in [3]) >> before attempting any i/o operations over SCCB. >> >> [1] https://digilent.com/reference/_media/reference/add-ons/pcam-5c/pcam_5c_sch.pdf >> [2] https://www.alinx.com/public/upload/file/AN5641_User_Manual.pdf >> [3] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf >> >> Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") >> Signed-off-by: Jai Luthra <j-luthra@ti.com> >> --- >> drivers/media/i2c/ov5640.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >> index e0f908af581b..4bb7bf557cfa 100644 >> --- a/drivers/media/i2c/ov5640.c >> +++ b/drivers/media/i2c/ov5640.c >> @@ -2466,6 +2466,7 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor) >> >> ov5640_reset(sensor); >> ov5640_power(sensor, true); >> + usleep_range(26000, 30000); > > I think you should only do this if you don't have RESETB pin. > > I'm not sure how to best describe this in DT. It's not the same as if you > didn't have RESETB GPIO. Why it's not the same? I understand the RESETB pin is always there just sometimes going to GPIO and sometimes to some other line. Best regards, Krzysztof
On Fri, Dec 16, 2022 at 8:09 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Jai, > > Thanks for the set. > > On Fri, Dec 16, 2022 at 07:14:07PM +0530, Jai Luthra wrote: > > Some module manufacturers [1][2] don't expose the RESETB gpio of the > > sensor directly through the 15-pin FFC connector. Instead wiring ~PWDN > > as a proxy reset with appropriate delays. > > > > In such cases, reset_gpio will not be available to the driver, but it > > will still be toggled when the sensor is powered on, and thus we should > > still honor the wait time of >= 5ms + 1ms + 20ms (see figure 2-3 in [3]) > > before attempting any i/o operations over SCCB. > > > > [1] https://digilent.com/reference/_media/reference/add-ons/pcam-5c/pcam_5c_sch.pdf > > [2] https://www.alinx.com/public/upload/file/AN5641_User_Manual.pdf > > [3] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf > > > > Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") > > Signed-off-by: Jai Luthra <j-luthra@ti.com> > > --- > > drivers/media/i2c/ov5640.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > index e0f908af581b..4bb7bf557cfa 100644 > > --- a/drivers/media/i2c/ov5640.c > > +++ b/drivers/media/i2c/ov5640.c > > @@ -2466,6 +2466,7 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor) > > > > ov5640_reset(sensor); > > ov5640_power(sensor, true); > > + usleep_range(26000, 30000); > > I think you should only do this if you don't have RESETB pin. > > I'm not sure how to best describe this in DT. It's not the same as if you > didn't have RESETB GPIO. Can that function check if (!sensor->reset_gpio) and do the delay only if reset_gpio is not present? adam > > Cc the devicetree list and Krzysztof. > > > > > ret = ov5640_init_slave_id(sensor); > > if (ret) > > -- > Kind regards, > > Sakari Ailus
Hi Sakari, Krzysztof, On 16/12/22 19:40, Krzysztof Kozlowski wrote: > On 16/12/2022 14:57, Sakari Ailus wrote: >> Hi Jai, >> >> Thanks for the set. >> >> On Fri, Dec 16, 2022 at 07:14:07PM +0530, Jai Luthra wrote: >>> Some module manufacturers [1][2] don't expose the RESETB gpio of the >>> sensor directly through the 15-pin FFC connector. Instead wiring ~PWDN >>> as a proxy reset with appropriate delays. >>> >>> In such cases, reset_gpio will not be available to the driver, but it >>> will still be toggled when the sensor is powered on, and thus we should >>> still honor the wait time of >= 5ms + 1ms + 20ms (see figure 2-3 in [3]) >>> before attempting any i/o operations over SCCB. >>> >>> [1] https://digilent.com/reference/_media/reference/add-ons/pcam-5c/pcam_5c_sch.pdf >>> [2] https://www.alinx.com/public/upload/file/AN5641_User_Manual.pdf >>> [3] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf >>> >>> Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") >>> Signed-off-by: Jai Luthra <j-luthra@ti.com> >>> --- >>> drivers/media/i2c/ov5640.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >>> index e0f908af581b..4bb7bf557cfa 100644 >>> --- a/drivers/media/i2c/ov5640.c >>> +++ b/drivers/media/i2c/ov5640.c >>> @@ -2466,6 +2466,7 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor) >>> >>> ov5640_reset(sensor); >>> ov5640_power(sensor, true); >>> + usleep_range(26000, 30000); >> >> I think you should only do this if you don't have RESETB pin. >> >> I'm not sure how to best describe this in DT. It's not the same as if you >> didn't have RESETB GPIO. > > Why it's not the same? I understand the RESETB pin is always there just > sometimes going to GPIO and sometimes to some other line. Thanks for the quick review. I agree, will send v2 where we check if reset-gpio is missing in the DT and in that case just toggle powerdown-gpio and wait for 26ms. > > Best regards, > Krzysztof > Thanks, Jai
Hi Krzysztof, On Fri, Dec 16, 2022 at 03:10:55PM +0100, Krzysztof Kozlowski wrote: > On 16/12/2022 14:57, Sakari Ailus wrote: > > Hi Jai, > > > > Thanks for the set. > > > > On Fri, Dec 16, 2022 at 07:14:07PM +0530, Jai Luthra wrote: > >> Some module manufacturers [1][2] don't expose the RESETB gpio of the > >> sensor directly through the 15-pin FFC connector. Instead wiring ~PWDN > >> as a proxy reset with appropriate delays. > >> > >> In such cases, reset_gpio will not be available to the driver, but it > >> will still be toggled when the sensor is powered on, and thus we should > >> still honor the wait time of >= 5ms + 1ms + 20ms (see figure 2-3 in [3]) > >> before attempting any i/o operations over SCCB. > >> > >> [1] https://digilent.com/reference/_media/reference/add-ons/pcam-5c/pcam_5c_sch.pdf > >> [2] https://www.alinx.com/public/upload/file/AN5641_User_Manual.pdf > >> [3] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf > >> > >> Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") > >> Signed-off-by: Jai Luthra <j-luthra@ti.com> > >> --- > >> drivers/media/i2c/ov5640.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > >> index e0f908af581b..4bb7bf557cfa 100644 > >> --- a/drivers/media/i2c/ov5640.c > >> +++ b/drivers/media/i2c/ov5640.c > >> @@ -2466,6 +2466,7 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor) > >> > >> ov5640_reset(sensor); > >> ov5640_power(sensor, true); > >> + usleep_range(26000, 30000); > > > > I think you should only do this if you don't have RESETB pin. > > > > I'm not sure how to best describe this in DT. It's not the same as if you > > didn't have RESETB GPIO. > > Why it's not the same? I understand the RESETB pin is always there just > sometimes going to GPIO and sometimes to some other line. The end result is the same but it's only the PWDN GPIO that the driver controls, and through some logic, that also controls the RESETB GPIO. The end result may be the same but the driver isn't in control of this, so at the very least a comment in the driver needs to be added to document this. I'd just drop the check in the beginning of ov5640_reset(). Speaking of which --- it's unlikely the device needs to be power cycled to reset it as it has a reset pin. Also ov5640_set_power_on() has an extra call to ov5640_power(). These are not problems in this patch though.
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index e0f908af581b..4bb7bf557cfa 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -2466,6 +2466,7 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor) ov5640_reset(sensor); ov5640_power(sensor, true); + usleep_range(26000, 30000); ret = ov5640_init_slave_id(sensor); if (ret)
Some module manufacturers [1][2] don't expose the RESETB gpio of the sensor directly through the 15-pin FFC connector. Instead wiring ~PWDN as a proxy reset with appropriate delays. In such cases, reset_gpio will not be available to the driver, but it will still be toggled when the sensor is powered on, and thus we should still honor the wait time of >= 5ms + 1ms + 20ms (see figure 2-3 in [3]) before attempting any i/o operations over SCCB. [1] https://digilent.com/reference/_media/reference/add-ons/pcam-5c/pcam_5c_sch.pdf [2] https://www.alinx.com/public/upload/file/AN5641_User_Manual.pdf [3] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") Signed-off-by: Jai Luthra <j-luthra@ti.com> --- drivers/media/i2c/ov5640.c | 1 + 1 file changed, 1 insertion(+)