[1/3] media: ov5640: Handle delays when no reset_gpio set

  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(+)
  

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.

Cc the devicetree list and Krzysztof.

>  
>  	ret = ov5640_init_slave_id(sensor);
>  	if (ret)
  

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.