Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Mark Brown]

On Mon, Nov 14, 2016 at 02:18:49PM +0200, Laurent Pinchart wrote:
> On Wednesday 26 Oct 2016 12:51:49 Mark Brown wrote:

> > Why would this be guaranteed by the API given that it's not documented
> > and why would many drivers break?  It's fairly rare for devices other
> > than SoCs to have strict power on sequencing requirements as it is hard
> > to achieve in practical systems.

> Is there a reason why the API shouldn't guarantee that regulators are powered 
> on in the order listed, and powered off in the reverse order ? Looking at the 

If it ever even did that through implementation it's not been true for a
very long time - it does the regulator enables in parallel in order to
reduce the overall time to power things up.  I keep wanting to come up
with code to figure out if we're using multiple enable bits in a single
register and hit them all at once though it's likely to be more trouble
than it's worth.

> implementation that's already the case for regulator_bulk_disable(), but 
> regulator_bulk_enable() uses async scheduling so doesn't guarantee ordering. 
> I 
> wonder whether a synchronous version of regulator_bulk_enable() would be 
> useful.

*Possibly* but I'd be surprised to learn that there's a substantial
amount of hardware out there that cares given that a power on reset
circuit isn't exactly complex to implement.  You do sometimes see a
global rail that should come up first (especially if there is an
integrated regulator) but I've not seen many cases where the hardware
cared outside of SoCs.


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Laurent Pinchart]

Hi Mark,

On Wednesday 26 Oct 2016 12:51:49 Mark Brown wrote:
> On Wed, Oct 26, 2016 at 02:27:23PM +0300, Todor Tomov wrote:
> > And using Mark Brown's correct address...
> 
> This is an *enormous* e-mail quoted to multiple levels with top posting
> and very little editing which makes it incredibly hard to find any
> relevant content.
> 
> > >> I believe it should be an API guarantee, otherwise many drivers using
> > >> the bulk API would break. Mark, could you please comment on that ?
> > > 
> > > Ok, let's wait for a response from Mark.
> 
> Why would this be guaranteed by the API given that it's not documented
> and why would many drivers break?  It's fairly rare for devices other
> than SoCs to have strict power on sequencing requirements as it is hard
> to achieve in practical systems.

I'm surprised, I've always considered the bulk regulator API as guaranteeing 
sequencing, and have written quite a few drivers with that assumption. If 
that's not correct then I'll have to switch them back to manual regulator 
handling.

Is there a reason why the API shouldn't guarantee that regulators are powered 
on in the order listed, and powered off in the reverse order ? Looking at the 
implementation that's already the case for regulator_bulk_disable(), but 
regulator_bulk_enable() uses async scheduling so doesn't guarantee ordering. I 
wonder whether a synchronous version of regulator_bulk_enable() would be 
useful.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Todor Tomov]

Hi,

On 10/26/2016 07:48 PM, Ian Arkver wrote:
> On 26/10/16 15:07, Todor Tomov wrote:
>> Hi,
>>
>> On 10/26/2016 03:48 PM, Ian Arkver wrote:
>>> [snip]
> +static int ov5645_regulators_enable(struct ov5645 *ov5645)
> +{
> +int ret;
> +
> +ret = regulator_enable(ov5645->io_regulator);
> +if (ret < 0) {
> +dev_err(ov5645->dev, "set io voltage failed\n");
> +return ret;
> +}
> +
> +ret = regulator_enable(ov5645->core_regulator);
> +if (ret) {
> +dev_err(ov5645->dev, "set core voltage failed\n");
> +goto err_disable_io;
> +}
> +
> +ret = regulator_enable(ov5645->analog_regulator);
> +if (ret) {
> +dev_err(ov5645->dev, "set analog voltage failed\n");
> +goto err_disable_core;
> +}
 How about using the regulator bulk API ? This would simplify the enable
 and disable functions.
>>> The driver must enable the regulators in this order. I can see in the
>>> implementation of the bulk api that they are enabled again in order
>>> but I don't see stated anywhere that it is guaranteed to follow the
>>> same order in future. I'd prefer to keep it explicit as it is now.
>> I believe it should be an API guarantee, otherwise many drivers using 
>> the bulk
>> API would break. Mark, could you please comment on that ?
> Ok, let's wait for a response from Mark.
>>> I don't have the OV5645 datasheet, but I do have the OV5640 and OV5647 
>>> datasheets. Both of these show that AVDD should come up before DVDD where 
>>> DVDD is externally supplied, although the minimum delay between them is 
>>> 0ms. Both datasheets also imply that this requirement is only to allow host 
>>> SCCB access on a shared I2C bus while the device is powering up. They imply 
>>> that if one waits 20ms after power on then SCCB will be fine without this 
>>> sequencing. Your code includes an msleep(20);
>> There is also requirement that DOVDD should become stable before AVDD (in 
>> both cases -
>> external or internal DVDD).
>>
>> Aren't these requirements needed to allow I2C access to another device on 
>> the same I2C bus even during these 20ms?
> Well, it's a really obscure corner case where these rails are actually 
> switched and the rise times are all available to the regulator framework (so 
> that there's a difference between three distinct calls to regulator_enable 
> and one call to the ASYNC_DOMAIN driven bulk enable) and the I2C bus is 
> shared with another device that is being accessed at the same time as the 
> sensor is enabled and the sensor breaks that access.
> 
> Having said that, really obscure corner cases are what lurk around and catch 
> you unawares years in the future, so maybe three calls is necessary here. 
> However, I think analog should be enabled before core - check with your 
> datasheet if you have the correct one.

Yes, analog (AVDD) should be enabled before core (DVDD);
and I/O (DOVDD) should be enabled before analog.

I also don't think that the benefit of using the bulk API (like shorter and
simpler code) is worth against the possibility of introducing a potential
problem (even if it is that rare).

But in any case, thank you for reviewing my code!

> 
> Regards,
> Ian
> 
>>
>>> Further, the reference schematic for the OV5647 shows three separate LDOs 
>>> with no sequencing between them.
>>>
>>> I've no comment on whether the bulk regulator API needs a "keep sequencing" 
>>> flag or somesuch, but I don't think this device will drive that requirement.
>>>
>>> Regards,
>>> Ian
>>>
>> Best regards,
>> Todor
>>
> 

-- 
Best regards,
Todor Tomov
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Ian Arkver]

On 26/10/16 15:07, Todor Tomov wrote:

Hi,

On 10/26/2016 03:48 PM, Ian Arkver wrote:

[snip]

+static int ov5645_regulators_enable(struct ov5645 *ov5645)
+{
+int ret;
+
+ret = regulator_enable(ov5645->io_regulator);
+if (ret < 0) {
+dev_err(ov5645->dev, "set io voltage failed\n");
+return ret;
+}
+
+ret = regulator_enable(ov5645->core_regulator);
+if (ret) {
+dev_err(ov5645->dev, "set core voltage failed\n");
+goto err_disable_io;
+}
+
+ret = regulator_enable(ov5645->analog_regulator);
+if (ret) {
+dev_err(ov5645->dev, "set analog voltage failed\n");
+goto err_disable_core;
+}

How about using the regulator bulk API ? This would simplify the enable
and disable functions.

The driver must enable the regulators in this order. I can see in the
implementation of the bulk api that they are enabled again in order
but I don't see stated anywhere that it is guaranteed to follow the
same order in future. I'd prefer to keep it explicit as it is now.

I believe it should be an API guarantee, otherwise many drivers using the bulk
API would break. Mark, could you please comment on that ?

Ok, let's wait for a response from Mark.

I don't have the OV5645 datasheet, but I do have the OV5640 and OV5647 
datasheets. Both of these show that AVDD should come up before DVDD where DVDD 
is externally supplied, although the minimum delay between them is 0ms. Both 
datasheets also imply that this requirement is only to allow host SCCB access 
on a shared I2C bus while the device is powering up. They imply that if one 
waits 20ms after power on then SCCB will be fine without this sequencing. Your 
code includes an msleep(20);

There is also requirement that DOVDD should become stable before AVDD (in both 
cases -
external or internal DVDD).

Aren't these requirements needed to allow I2C access to another device on the 
same I2C bus even during these 20ms?
Well, it's a really obscure corner case where these rails are actually 
switched and the rise times are all available to the regulator framework 
(so that there's a difference between three distinct calls to 
regulator_enable and one call to the ASYNC_DOMAIN driven bulk enable) 
and the I2C bus is shared with another device that is being accessed at 
the same time as the sensor is enabled and the sensor breaks that access.


Having said that, really obscure corner cases are what lurk around and 
catch you unawares years in the future, so maybe three calls is 
necessary here. However, I think analog should be enabled before core - 
check with your datasheet if you have the correct one.


Regards,
Ian




Further, the reference schematic for the OV5647 shows three separate LDOs with 
no sequencing between them.

I've no comment on whether the bulk regulator API needs a "keep sequencing" 
flag or somesuch, but I don't think this device will drive that requirement.

Regards,
Ian


Best regards,
Todor



--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Todor Tomov]

Hi,

On 10/26/2016 03:48 PM, Ian Arkver wrote:
> [snip]
>>> +static int ov5645_regulators_enable(struct ov5645 *ov5645)
>>> +{
>>> +int ret;
>>> +
>>> +ret = regulator_enable(ov5645->io_regulator);
>>> +if (ret < 0) {
>>> +dev_err(ov5645->dev, "set io voltage failed\n");
>>> +return ret;
>>> +}
>>> +
>>> +ret = regulator_enable(ov5645->core_regulator);
>>> +if (ret) {
>>> +dev_err(ov5645->dev, "set core voltage failed\n");
>>> +goto err_disable_io;
>>> +}
>>> +
>>> +ret = regulator_enable(ov5645->analog_regulator);
>>> +if (ret) {
>>> +dev_err(ov5645->dev, "set analog voltage failed\n");
>>> +goto err_disable_core;
>>> +}
>> How about using the regulator bulk API ? This would simplify the enable
>> and disable functions.
> The driver must enable the regulators in this order. I can see in the
> implementation of the bulk api that they are enabled again in order
> but I don't see stated anywhere that it is guaranteed to follow the
> same order in future. I'd prefer to keep it explicit as it is now.
 I believe it should be an API guarantee, otherwise many drivers using the 
 bulk
 API would break. Mark, could you please comment on that ?
>>> Ok, let's wait for a response from Mark.
> I don't have the OV5645 datasheet, but I do have the OV5640 and OV5647 
> datasheets. Both of these show that AVDD should come up before DVDD where 
> DVDD is externally supplied, although the minimum delay between them is 0ms. 
> Both datasheets also imply that this requirement is only to allow host SCCB 
> access on a shared I2C bus while the device is powering up. They imply that 
> if one waits 20ms after power on then SCCB will be fine without this 
> sequencing. Your code includes an msleep(20);

There is also requirement that DOVDD should become stable before AVDD (in both 
cases -
external or internal DVDD).

Aren't these requirements needed to allow I2C access to another device on the 
same I2C bus even during these 20ms?

> 
> Further, the reference schematic for the OV5647 shows three separate LDOs 
> with no sequencing between them.
> 
> I've no comment on whether the bulk regulator API needs a "keep sequencing" 
> flag or somesuch, but I don't think this device will drive that requirement.
> 
> Regards,
> Ian
> 

Best regards,
Todor

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Ian Arkver]

[snip]

+static int ov5645_regulators_enable(struct ov5645 *ov5645)
+{
+   int ret;
+
+   ret = regulator_enable(ov5645->io_regulator);
+   if (ret < 0) {
+   dev_err(ov5645->dev, "set io voltage failed\n");
+   return ret;
+   }
+
+   ret = regulator_enable(ov5645->core_regulator);
+   if (ret) {
+   dev_err(ov5645->dev, "set core voltage failed\n");
+   goto err_disable_io;
+   }
+
+   ret = regulator_enable(ov5645->analog_regulator);
+   if (ret) {
+   dev_err(ov5645->dev, "set analog voltage failed\n");
+   goto err_disable_core;
+   }

How about using the regulator bulk API ? This would simplify the enable
and disable functions.

The driver must enable the regulators in this order. I can see in the
implementation of the bulk api that they are enabled again in order
but I don't see stated anywhere that it is guaranteed to follow the
same order in future. I'd prefer to keep it explicit as it is now.

I believe it should be an API guarantee, otherwise many drivers using the bulk
API would break. Mark, could you please comment on that ?

Ok, let's wait for a response from Mark.
I don't have the OV5645 datasheet, but I do have the OV5640 and OV5647 
datasheets. Both of these show that AVDD should come up before DVDD 
where DVDD is externally supplied, although the minimum delay between 
them is 0ms. Both datasheets also imply that this requirement is only to 
allow host SCCB access on a shared I2C bus while the device is powering 
up. They imply that if one waits 20ms after power on then SCCB will be 
fine without this sequencing. Your code includes an msleep(20);


Further, the reference schematic for the OV5647 shows three separate 
LDOs with no sequencing between them.


I've no comment on whether the bulk regulator API needs a "keep 
sequencing" flag or somesuch, but I don't think this device will drive 
that requirement.


Regards,
Ian

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Mark Brown]

On Wed, Oct 26, 2016 at 02:27:23PM +0300, Todor Tomov wrote:

> And using Mark Brown's correct address...

This is an *enormous* e-mail quoted to multiple levels with top posting
and very little editing which makes it incredibly hard to find any
relevant content.

> >> I believe it should be an API guarantee, otherwise many drivers using the 
> >> bulk 
> >> API would break. Mark, could you please comment on that ?

> > Ok, let's wait for a response from Mark.

Why would this be guaranteed by the API given that it's not documented
and why would many drivers break?  It's fairly rare for devices other
than SoCs to have strict power on sequencing requirements as it is hard
to achieve in practical systems.


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Todor Tomov]

And using Mark Brown's correct address...

On 10/26/2016 02:15 PM, Todor Tomov wrote:
> Hi,
> 
> Adding Mark Brown in --to list.
> 
> My reply on comments below.
> The question on regulator bulk API to Mark Brown still holds.
> 
> 
> On 10/19/2016 11:44 AM, Laurent Pinchart wrote:
>> Hi Todor,
>>
>> (CC'ing Mark Brown for a question on regulators)
>>
>> On Friday 14 Oct 2016 14:57:01 Todor Tomov wrote:
>>> Hi Laurent,
>>>
>>> Thank you for the time spent to do this thorough review of the patch!
>>>
>>> Below I have removed some of the comments where I agree and I'll fix.
>>> I have left the places where I have something relevant to say or ask.
>>>
>>> On 09/08/2016 03:22 PM, Laurent Pinchart wrote:
 On Thursday 08 Sep 2016 12:13:55 Todor Tomov wrote:
> The ov5645 sensor from Omnivision supports up to 2592x1944
> and CSI2 interface.
>
> The driver adds support for the following modes:
> - 1280x960
> - 1920x1080
> - 2592x1944
>
> Output format is packed 8bit UYVY.
>
> Signed-off-by: Todor Tomov 
> ---
>
>  drivers/media/i2c/Kconfig  |   12 +
>  drivers/media/i2c/Makefile |1 +
>  drivers/media/i2c/ov5645.c | 1372 ++
>  3 files changed, 1385 insertions(+)
>  create mode 100644 drivers/media/i2c/ov5645.c

 [snip]

> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> new file mode 100644
> index 000..5e5c37e
> --- /dev/null
> +++ b/drivers/media/i2c/ov5645.c
> @@ -0,0 +1,1372 @@
>>
>> [snip]
>>
> + { 0x3103, 0x11 },
> + { 0x3008, 0x82 },
> + { 0x3008, 0x42 },
> + { 0x3103, 0x03 },
> + { 0x3503, 0x07 },

 [snip]

> + { 0x3503, 0x00 },

 Can't you get rid of the first write to 0x3503 ?
>>>
>>> No, this is a startup sequence from the vendor so I'm following it as it is.
>>
>> 0x3503 controls the AEC/AGC mode, I wonder if that's really needed. I'm OK 
>> keeping it as-is for now.
> 
> I agree that there is a reason to wonder if it is really needed, but I'd still
> prefer not to touch it.
> 
>>
>>> [snip]
>>>
> +static int ov5645_regulators_enable(struct ov5645 *ov5645)
> +{
> + int ret;
> +
> + ret = regulator_enable(ov5645->io_regulator);
> + if (ret < 0) {
> + dev_err(ov5645->dev, "set io voltage failed\n");
> + return ret;
> + }
> +
> + ret = regulator_enable(ov5645->core_regulator);
> + if (ret) {
> + dev_err(ov5645->dev, "set core voltage failed\n");
> + goto err_disable_io;
> + }
> +
> + ret = regulator_enable(ov5645->analog_regulator);
> + if (ret) {
> + dev_err(ov5645->dev, "set analog voltage failed\n");
> + goto err_disable_core;
> + }

 How about using the regulator bulk API ? This would simplify the enable
 and disable functions.
>>>
>>> The driver must enable the regulators in this order. I can see in the
>>> implementation of the bulk api that they are enabled again in order
>>> but I don't see stated anywhere that it is guaranteed to follow the
>>> same order in future. I'd prefer to keep it explicit as it is now.
>>
>> I believe it should be an API guarantee, otherwise many drivers using the 
>> bulk 
>> API would break. Mark, could you please comment on that ?
> 
> Ok, let's wait for a response from Mark.
> 
>>
>>> [snip]
>>>
> +static int ov5645_set_power_on(struct ov5645 *ov5645)
> +{
> + int ret;
> +
> + clk_set_rate(ov5645->xclk, ov5645->xclk_freq);

 Is this needed every time you power the sensor on or could you do it just
 once at probe time ?
>>>
>>> I'll move it at probe time.
>>>
> + ret = clk_prepare_enable(ov5645->xclk);
> + if (ret < 0) {
> + dev_err(ov5645->dev, "clk prepare enable failed\n");
> + return ret;
> + }

 Is it safe to start the clock before the regulators ? Driving an input of
 an unpowered chip can lead to latch-up issues.
>>>
>>> Correct, power should be enabled first. I'll fix this.
>>>
> + ret = ov5645_regulators_enable(ov5645);
> + if (ret < 0) {
> + clk_disable_unprepare(ov5645->xclk);
> + return ret;
> + }
> +
> + usleep_range(5000, 15000);
> + gpiod_set_value_cansleep(ov5645->enable_gpio, 1);
> +
> + usleep_range(1000, 2000);
> + gpiod_set_value_cansleep(ov5645->rst_gpio, 0);
> +
> + msleep(20);
> +
> + return ret;

 You can return 0.
>>>
>>> Ok.
>>>
>>> [snip]
>>>
> +static int ov5645_set_hflip(struct ov5645 *ov5645, s32 value)
> +{
> + u8 val;
> + int ret;
> +
> + ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21, );
> + if (ret < 0)
> + return ret;
> +
> + if (value == 0)
> + val &= ~(OV5645_SENSOR_MIRROR);
> + else
> + val |= 

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Todor Tomov]

Hi,

Adding Mark Brown in --to list.

My reply on comments below.
The question on regulator bulk API to Mark Brown still holds.


On 10/19/2016 11:44 AM, Laurent Pinchart wrote:
> Hi Todor,
> 
> (CC'ing Mark Brown for a question on regulators)
> 
> On Friday 14 Oct 2016 14:57:01 Todor Tomov wrote:
>> Hi Laurent,
>>
>> Thank you for the time spent to do this thorough review of the patch!
>>
>> Below I have removed some of the comments where I agree and I'll fix.
>> I have left the places where I have something relevant to say or ask.
>>
>> On 09/08/2016 03:22 PM, Laurent Pinchart wrote:
>>> On Thursday 08 Sep 2016 12:13:55 Todor Tomov wrote:
 The ov5645 sensor from Omnivision supports up to 2592x1944
 and CSI2 interface.

 The driver adds support for the following modes:
 - 1280x960
 - 1920x1080
 - 2592x1944

 Output format is packed 8bit UYVY.

 Signed-off-by: Todor Tomov 
 ---

  drivers/media/i2c/Kconfig  |   12 +
  drivers/media/i2c/Makefile |1 +
  drivers/media/i2c/ov5645.c | 1372 ++
  3 files changed, 1385 insertions(+)
  create mode 100644 drivers/media/i2c/ov5645.c
>>>
>>> [snip]
>>>
 diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
 new file mode 100644
 index 000..5e5c37e
 --- /dev/null
 +++ b/drivers/media/i2c/ov5645.c
 @@ -0,0 +1,1372 @@
> 
> [snip]
> 
 +  { 0x3103, 0x11 },
 +  { 0x3008, 0x82 },
 +  { 0x3008, 0x42 },
 +  { 0x3103, 0x03 },
 +  { 0x3503, 0x07 },
>>>
>>> [snip]
>>>
 +  { 0x3503, 0x00 },
>>>
>>> Can't you get rid of the first write to 0x3503 ?
>>
>> No, this is a startup sequence from the vendor so I'm following it as it is.
> 
> 0x3503 controls the AEC/AGC mode, I wonder if that's really needed. I'm OK 
> keeping it as-is for now.

I agree that there is a reason to wonder if it is really needed, but I'd still
prefer not to touch it.

> 
>> [snip]
>>
 +static int ov5645_regulators_enable(struct ov5645 *ov5645)
 +{
 +  int ret;
 +
 +  ret = regulator_enable(ov5645->io_regulator);
 +  if (ret < 0) {
 +  dev_err(ov5645->dev, "set io voltage failed\n");
 +  return ret;
 +  }
 +
 +  ret = regulator_enable(ov5645->core_regulator);
 +  if (ret) {
 +  dev_err(ov5645->dev, "set core voltage failed\n");
 +  goto err_disable_io;
 +  }
 +
 +  ret = regulator_enable(ov5645->analog_regulator);
 +  if (ret) {
 +  dev_err(ov5645->dev, "set analog voltage failed\n");
 +  goto err_disable_core;
 +  }
>>>
>>> How about using the regulator bulk API ? This would simplify the enable
>>> and disable functions.
>>
>> The driver must enable the regulators in this order. I can see in the
>> implementation of the bulk api that they are enabled again in order
>> but I don't see stated anywhere that it is guaranteed to follow the
>> same order in future. I'd prefer to keep it explicit as it is now.
> 
> I believe it should be an API guarantee, otherwise many drivers using the 
> bulk 
> API would break. Mark, could you please comment on that ?

Ok, let's wait for a response from Mark.

> 
>> [snip]
>>
 +static int ov5645_set_power_on(struct ov5645 *ov5645)
 +{
 +  int ret;
 +
 +  clk_set_rate(ov5645->xclk, ov5645->xclk_freq);
>>>
>>> Is this needed every time you power the sensor on or could you do it just
>>> once at probe time ?
>>
>> I'll move it at probe time.
>>
 +  ret = clk_prepare_enable(ov5645->xclk);
 +  if (ret < 0) {
 +  dev_err(ov5645->dev, "clk prepare enable failed\n");
 +  return ret;
 +  }
>>>
>>> Is it safe to start the clock before the regulators ? Driving an input of
>>> an unpowered chip can lead to latch-up issues.
>>
>> Correct, power should be enabled first. I'll fix this.
>>
 +  ret = ov5645_regulators_enable(ov5645);
 +  if (ret < 0) {
 +  clk_disable_unprepare(ov5645->xclk);
 +  return ret;
 +  }
 +
 +  usleep_range(5000, 15000);
 +  gpiod_set_value_cansleep(ov5645->enable_gpio, 1);
 +
 +  usleep_range(1000, 2000);
 +  gpiod_set_value_cansleep(ov5645->rst_gpio, 0);
 +
 +  msleep(20);
 +
 +  return ret;
>>>
>>> You can return 0.
>>
>> Ok.
>>
>> [snip]
>>
 +static int ov5645_set_hflip(struct ov5645 *ov5645, s32 value)
 +{
 +  u8 val;
 +  int ret;
 +
 +  ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21, );
 +  if (ret < 0)
 +  return ret;
 +
 +  if (value == 0)
 +  val &= ~(OV5645_SENSOR_MIRROR);
 +  else
 +  val |= (OV5645_SENSOR_MIRROR);
 +
 +  return ov5645_write_reg(ov5645, OV5645_TIMING_TC_REG21, val);
>>>
>>> You could cache this register too.
>>
>> Ok.
>>
 +}
 +
 +static int ov5645_set_vflip(struct ov5645 *ov5645, s32 

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Laurent Pinchart]

Hi Todor,

(CC'ing Mark Brown for a question on regulators)

On Friday 14 Oct 2016 14:57:01 Todor Tomov wrote:
> Hi Laurent,
> 
> Thank you for the time spent to do this thorough review of the patch!
> 
> Below I have removed some of the comments where I agree and I'll fix.
> I have left the places where I have something relevant to say or ask.
> 
> On 09/08/2016 03:22 PM, Laurent Pinchart wrote:
> > On Thursday 08 Sep 2016 12:13:55 Todor Tomov wrote:
> >> The ov5645 sensor from Omnivision supports up to 2592x1944
> >> and CSI2 interface.
> >> 
> >> The driver adds support for the following modes:
> >> - 1280x960
> >> - 1920x1080
> >> - 2592x1944
> >> 
> >> Output format is packed 8bit UYVY.
> >> 
> >> Signed-off-by: Todor Tomov 
> >> ---
> >> 
> >>  drivers/media/i2c/Kconfig  |   12 +
> >>  drivers/media/i2c/Makefile |1 +
> >>  drivers/media/i2c/ov5645.c | 1372 ++
> >>  3 files changed, 1385 insertions(+)
> >>  create mode 100644 drivers/media/i2c/ov5645.c
> > 
> > [snip]
> > 
> >> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> >> new file mode 100644
> >> index 000..5e5c37e
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/ov5645.c
> >> @@ -0,0 +1,1372 @@

[snip]

> >> +  { 0x3103, 0x11 },
> >> +  { 0x3008, 0x82 },
> >> +  { 0x3008, 0x42 },
> >> +  { 0x3103, 0x03 },
> >> +  { 0x3503, 0x07 },
> > 
> > [snip]
> > 
> >> +  { 0x3503, 0x00 },
> > 
> > Can't you get rid of the first write to 0x3503 ?
> 
> No, this is a startup sequence from the vendor so I'm following it as it is.

0x3503 controls the AEC/AGC mode, I wonder if that's really needed. I'm OK 
keeping it as-is for now.

> [snip]
> 
> >> +static int ov5645_regulators_enable(struct ov5645 *ov5645)
> >> +{
> >> +  int ret;
> >> +
> >> +  ret = regulator_enable(ov5645->io_regulator);
> >> +  if (ret < 0) {
> >> +  dev_err(ov5645->dev, "set io voltage failed\n");
> >> +  return ret;
> >> +  }
> >> +
> >> +  ret = regulator_enable(ov5645->core_regulator);
> >> +  if (ret) {
> >> +  dev_err(ov5645->dev, "set core voltage failed\n");
> >> +  goto err_disable_io;
> >> +  }
> >> +
> >> +  ret = regulator_enable(ov5645->analog_regulator);
> >> +  if (ret) {
> >> +  dev_err(ov5645->dev, "set analog voltage failed\n");
> >> +  goto err_disable_core;
> >> +  }
> > 
> > How about using the regulator bulk API ? This would simplify the enable
> > and disable functions.
> 
> The driver must enable the regulators in this order. I can see in the
> implementation of the bulk api that they are enabled again in order
> but I don't see stated anywhere that it is guaranteed to follow the
> same order in future. I'd prefer to keep it explicit as it is now.

I believe it should be an API guarantee, otherwise many drivers using the bulk 
API would break. Mark, could you please comment on that ?

> [snip]
> 
> >> +static int ov5645_set_power_on(struct ov5645 *ov5645)
> >> +{
> >> +  int ret;
> >> +
> >> +  clk_set_rate(ov5645->xclk, ov5645->xclk_freq);
> > 
> > Is this needed every time you power the sensor on or could you do it just
> > once at probe time ?
> 
> I'll move it at probe time.
> 
> >> +  ret = clk_prepare_enable(ov5645->xclk);
> >> +  if (ret < 0) {
> >> +  dev_err(ov5645->dev, "clk prepare enable failed\n");
> >> +  return ret;
> >> +  }
> > 
> > Is it safe to start the clock before the regulators ? Driving an input of
> > an unpowered chip can lead to latch-up issues.
> 
> Correct, power should be enabled first. I'll fix this.
> 
> >> +  ret = ov5645_regulators_enable(ov5645);
> >> +  if (ret < 0) {
> >> +  clk_disable_unprepare(ov5645->xclk);
> >> +  return ret;
> >> +  }
> >> +
> >> +  usleep_range(5000, 15000);
> >> +  gpiod_set_value_cansleep(ov5645->enable_gpio, 1);
> >> +
> >> +  usleep_range(1000, 2000);
> >> +  gpiod_set_value_cansleep(ov5645->rst_gpio, 0);
> >> +
> >> +  msleep(20);
> >> +
> >> +  return ret;
> > 
> > You can return 0.
> 
> Ok.
> 
> [snip]
> 
> >> +static int ov5645_set_hflip(struct ov5645 *ov5645, s32 value)
> >> +{
> >> +  u8 val;
> >> +  int ret;
> >> +
> >> +  ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21, );
> >> +  if (ret < 0)
> >> +  return ret;
> >> +
> >> +  if (value == 0)
> >> +  val &= ~(OV5645_SENSOR_MIRROR);
> >> +  else
> >> +  val |= (OV5645_SENSOR_MIRROR);
> >> +
> >> +  return ov5645_write_reg(ov5645, OV5645_TIMING_TC_REG21, val);
> > 
> > You could cache this register too.
> 
> Ok.
> 
> >> +}
> >> +
> >> +static int ov5645_set_vflip(struct ov5645 *ov5645, s32 value)
> >> +{
> >> +  u8 val;
> >> +  int ret;
> >> +
> >> +  ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20, );
> >> +  if (ret < 0)
> >> +  return ret;
> >> +
> >> +  if (value == 0)
> >> +  val |= (OV5645_SENSOR_VFLIP | OV5645_ISP_VFLIP);
> >> +  else
> >> +  val &= ~(OV5645_SENSOR_VFLIP | OV5645_ISP_VFLIP);
> >> +
> >> +  

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Todor Tomov]

Hi Laurent,

Thank you for the time spent to do this thorough review of the patch!

Below I have removed some of the comments where I agree and I'll fix.
I have left the places where I have something relevant to say or ask.


On 09/08/2016 03:22 PM, Laurent Pinchart wrote:
> Hi Todor,
> 
> Thank you for the patch.
> 
> On Thursday 08 Sep 2016 12:13:55 Todor Tomov wrote:
>> The ov5645 sensor from Omnivision supports up to 2592x1944
>> and CSI2 interface.
>>
>> The driver adds support for the following modes:
>> - 1280x960
>> - 1920x1080
>> - 2592x1944
>>
>> Output format is packed 8bit UYVY.
>>
>> Signed-off-by: Todor Tomov 
>> ---
>>  drivers/media/i2c/Kconfig  |   12 +
>>  drivers/media/i2c/Makefile |1 +
>>  drivers/media/i2c/ov5645.c | 1372 +
>>  3 files changed, 1385 insertions(+)
>>  create mode 100644 drivers/media/i2c/ov5645.c
> 
> [snip]
> 
>> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
>> new file mode 100644
>> index 000..5e5c37e
>> --- /dev/null
>> +++ b/drivers/media/i2c/ov5645.c
>> @@ -0,0 +1,1372 @@
> 
> [snip]
> 

[snip]

>> +static inline struct ov5645 *to_ov5645(struct v4l2_subdev *sd)
>> +{
>> +return container_of(sd, struct ov5645, sd);
>> +}
>> +
>> +static struct reg_value ov5645_global_init_setting[] = {
> 
> You can make this static const. Same comment for the other register arrays.
Ok.

> 
>> +{ 0x3103, 0x11 },
>> +{ 0x3008, 0x82 },
>> +{ 0x3008, 0x42 },
>> +{ 0x3103, 0x03 },
>> +{ 0x3503, 0x07 },
> 
> [snip]
> 
>> +{ 0x3503, 0x00 },
> 
> Can't you get rid of the first write to 0x3503 ?
No, this is a startup sequence from the vendor so I'm following it as it is.

[snip]

>> +static int ov5645_regulators_enable(struct ov5645 *ov5645)
>> +{
>> +int ret;
>> +
>> +ret = regulator_enable(ov5645->io_regulator);
>> +if (ret < 0) {
>> +dev_err(ov5645->dev, "set io voltage failed\n");
>> +return ret;
>> +}
>> +
>> +ret = regulator_enable(ov5645->core_regulator);
>> +if (ret) {
>> +dev_err(ov5645->dev, "set core voltage failed\n");
>> +goto err_disable_io;
>> +}
>> +
>> +ret = regulator_enable(ov5645->analog_regulator);
>> +if (ret) {
>> +dev_err(ov5645->dev, "set analog voltage failed\n");
>> +goto err_disable_core;
>> +}
> 
> How about using the regulator bulk API ? This would simplify the enable and 
> disable functions.
The driver must enable the regulators in this order. I can see in the
implementation of the bulk api that they are enabled again in order
but I don't see stated anywhere that it is guaranteed to follow the
same order in future. I'd prefer to keep it explicit as it is now. 

[snip]

>> +static int ov5645_set_power_on(struct ov5645 *ov5645)
>> +{
>> +int ret;
>> +
>> +clk_set_rate(ov5645->xclk, ov5645->xclk_freq);
> 
> Is this needed every time you power the sensor on or could you do it just 
> once 
> at probe time ?
I'll move it at probe time.

> 
>> +ret = clk_prepare_enable(ov5645->xclk);
>> +if (ret < 0) {
>> +dev_err(ov5645->dev, "clk prepare enable failed\n");
>> +return ret;
>> +}
> 
> Is it safe to start the clock before the regulators ? Driving an input of an 
> unpowered chip can lead to latch-up issues.
Correct, power should be enabled first. I'll fix this.

> 
>> +ret = ov5645_regulators_enable(ov5645);
>> +if (ret < 0) {
>> +clk_disable_unprepare(ov5645->xclk);
>> +return ret;
>> +}
>> +
>> +usleep_range(5000, 15000);
>> +gpiod_set_value_cansleep(ov5645->enable_gpio, 1);
>> +
>> +usleep_range(1000, 2000);
>> +gpiod_set_value_cansleep(ov5645->rst_gpio, 0);
>> +
>> +msleep(20);
>> +
>> +return ret;
> 
> You can return 0.
Ok.

[snip]

>> +static int ov5645_set_hflip(struct ov5645 *ov5645, s32 value)
>> +{
>> +u8 val;
>> +int ret;
>> +
>> +ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21, );
>> +if (ret < 0)
>> +return ret;
>> +
>> +if (value == 0)
>> +val &= ~(OV5645_SENSOR_MIRROR);
>> +else
>> +val |= (OV5645_SENSOR_MIRROR);
>> +
>> +return ov5645_write_reg(ov5645, OV5645_TIMING_TC_REG21, val);
> 
> You could cache this register too.
Ok.

> 
>> +}
>> +
>> +static int ov5645_set_vflip(struct ov5645 *ov5645, s32 value)
>> +{
>> +u8 val;
>> +int ret;
>> +
>> +ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20, );
>> +if (ret < 0)
>> +return ret;
>> +
>> +if (value == 0)
>> +val |= (OV5645_SENSOR_VFLIP | OV5645_ISP_VFLIP);
>> +else
>> +val &= ~(OV5645_SENSOR_VFLIP | OV5645_ISP_VFLIP);
>> +
>> +return ov5645_write_reg(ov5645, OV5645_TIMING_TC_REG20, val);
> 
> And this one as well.
Yes.

> 
> How about using regmap by the way ?
I'd prefer to keep it as is for now.

> 
>> +}
>> +
>> +static int 

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Laurent Pinchart]

Hi Todor,

Thank you for the patch.

On Thursday 08 Sep 2016 12:13:55 Todor Tomov wrote:
> The ov5645 sensor from Omnivision supports up to 2592x1944
> and CSI2 interface.
> 
> The driver adds support for the following modes:
> - 1280x960
> - 1920x1080
> - 2592x1944
> 
> Output format is packed 8bit UYVY.
> 
> Signed-off-by: Todor Tomov 
> ---
>  drivers/media/i2c/Kconfig  |   12 +
>  drivers/media/i2c/Makefile |1 +
>  drivers/media/i2c/ov5645.c | 1372 +
>  3 files changed, 1385 insertions(+)
>  create mode 100644 drivers/media/i2c/ov5645.c

[snip]

> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> new file mode 100644
> index 000..5e5c37e
> --- /dev/null
> +++ b/drivers/media/i2c/ov5645.c
> @@ -0,0 +1,1372 @@

[snip]

> +#define OV5645_VOLTAGE_ANALOG   280
> +#define OV5645_VOLTAGE_DIGITAL_CORE 150
> +#define OV5645_VOLTAGE_DIGITAL_IO   180
> +
> +#define OV5645_SYSTEM_CTRL0  0x3008
> +#define  OV5645_SYSTEM_CTRL0_START   0x02
> +#define  OV5645_SYSTEM_CTRL0_STOP0x42
> +#define OV5645_CHIP_ID_HIGH  0x300A
> +#define  OV5645_CHIP_ID_HIGH_BYTE0x56
> +#define OV5645_CHIP_ID_LOW   0x300B
> +#define  OV5645_CHIP_ID_LOW_BYTE 0x45
> +#define OV5645_AWB_MANUAL_CONTROL0x3406
> +#define  OV5645_AWB_MANUAL_ENABLEBIT(0)
> +#define OV5645_AEC_PK_MANUAL 0x3503
> +#define  OV5645_AEC_MANUAL_ENABLEBIT(0)
> +#define  OV5645_AGC_MANUAL_ENABLEBIT(1)
> +#define OV5645_TIMING_TC_REG20   0x3820
> +#define  OV5645_SENSOR_VFLIP BIT(1)
> +#define  OV5645_ISP_VFLIPBIT(2)
> +#define OV5645_TIMING_TC_REG21   0x3821
> +#define  OV5645_SENSOR_MIRRORBIT(1)
> +#define OV5645_PRE_ISP_TEST_SETTING_10x503d

You're using a mix of upper and lower case hex values in the driver. I would 
standardize on lower case as that's what the majority of the kernel code uses.

> +#define  OV5645_TEST_PATTERN_MASK0x3
> +#define  OV5645_SET_TEST_PATTERN(x)  ((x) & 
OV5645_TEST_PATTERN_MASK)
> +#define  OV5645_TEST_PATTERN_ENABLE  BIT(7)
> +#define OV5645_SDE_SAT_U 0x5583
> +#define OV5645_SDE_SAT_V 0x5584
> +
> +enum ov5645_mode {
> + OV5645_MODE_MIN = 0,

OV5645_MODE_MIN isn't used.

> + OV5645_MODE_SXGA = 0,
> + OV5645_MODE_1080P = 1,
> + OV5645_MODE_FULL = 2,
> + OV5645_MODE_MAX = 2
> +};
> +
> +struct reg_value {
> + u16 reg;
> + u8 val;
> +};
> +
> +struct ov5645_mode_info {
> + enum ov5645_mode mode;

This field is never used, you can remove it.

> + u32 width;
> + u32 height;
> + struct reg_value *data;
> + u32 data_size;
> +};
> +
> +struct ov5645 {
> + struct i2c_client *i2c_client;
> + struct device *dev;
> + struct v4l2_subdev sd;
> + struct media_pad pad;
> + struct v4l2_of_endpoint ep;
> + struct v4l2_mbus_framefmt fmt;
> + struct v4l2_rect crop;
> + struct clk *xclk;
> + /* External clock frequency currently supported is 2388Hz */
> + u32 xclk_freq;
> +
> + struct regulator *io_regulator;
> + struct regulator *core_regulator;
> + struct regulator *analog_regulator;
> +
> + enum ov5645_mode current_mode;

You could store a pointer to struct ov5645_mode_info instead, it would save 
the array lookup when using it.

> + /* Cached control values */
> + struct v4l2_ctrl_handler ctrls;
> + struct v4l2_ctrl *saturation;
> + struct v4l2_ctrl *hflip;
> + struct v4l2_ctrl *vflip;
> + struct v4l2_ctrl *autogain;
> + struct v4l2_ctrl *autoexposure;
> + struct v4l2_ctrl *awb;
> + struct v4l2_ctrl *pattern;
> +
> + struct mutex power_lock; /* lock to protect power state */
> + bool power;
> +
> + struct gpio_desc *enable_gpio;
> + struct gpio_desc *rst_gpio;
> +};
> +
> +static inline struct ov5645 *to_ov5645(struct v4l2_subdev *sd)
> +{
> + return container_of(sd, struct ov5645, sd);
> +}
> +
> +static struct reg_value ov5645_global_init_setting[] = {

You can make this static const. Same comment for the other register arrays.

> + { 0x3103, 0x11 },
> + { 0x3008, 0x82 },
> + { 0x3008, 0x42 },
> + { 0x3103, 0x03 },
> + { 0x3503, 0x07 },

[snip]

> + { 0x3503, 0x00 },

Can't you get rid of the first write to 0x3503 ?

[snip]

> +};

[snip]

> +static struct ov5645_mode_info ov5645_mode_info_data[OV5645_MODE_MAX + 1] =

static const here too.

You can leave the array size out here (ov5645_mode_info_data[]), use 
ARRAY_SIZE(ov5645_mode_info_data) instead of OV5645_MODE_MAX below, and drop 
the OV5645_MODE_MAX enum value completely. I might even go as far as dropping 
enum ov5645_mode