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@xxxxxx> > >> --- > >> 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. -- Kind regards, Sakari Ailus
