[PATCH v6 0/2] OV5645 camera sensor driver

[PATCH v6 0/2] OV5645 camera sensor driver

[Todor Tomov]

This is the sixth version of the OV5645 camera sensor driver patchset.

Changes since version 5 include:
- external clock frequency set in DT;
- added v4l2_subdev_pad_ops.init_cfg function to initialize formats;
- current sensor mode not updated if set_fmt is TRY (not ACTIVE);
- other small changes - debug messages removed, register addresses defines
  renamed, redundant safety checks removed, unnecessary labels removed,
  mutex_destroy added.

Two one-line changes since version 4:
- return current format on set_format;
- return all frame sizes when enumerating them.

Only one change since version 3:
- build failure on kernel v4.7-rc1 fixed:
  s/media_entity_init/media_entity_pads_init/

Changes from version 2 include:
- external camera clock configuration is moved from DT to driver;
- pwdn-gpios renamed to enable-gpios;
- switched polarity of reset-gpios to the more intuitive active low;
- added Kconfig dependency to OF;
- return values checks;
- regulators and gpios are now required (not optional);
- regulators names renamed;
- power counter variable changed to a bool power state;
- ov5645_registered() is removed and sensor id reading moved to probe().

Changes from version 1 include:
- patch split to dt binding doc patch and driver patch;
- changes in power on/off logic - s_power is now not called on
  open/close;
- using assigned-clock-rates in dt for setting camera external
  clock rate;
- correct api for gpio handling;
- return values checks;
- style fixes.

Todor Tomov (2):
  media: i2c/ov5645: add the device tree binding document
  media: Add a driver for the ov5645 camera sensor.

 .../devicetree/bindings/media/i2c/ov5645.txt       |   52 +
 drivers/media/i2c/Kconfig                          |   12 +
 drivers/media/i2c/Makefile                         |    1 +
 drivers/media/i2c/ov5645.c                         | 1372 ++++++++++++++++++++
 4 files changed, 1437 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
 create mode 100644 drivers/media/i2c/ov5645.c

-- 
1.9.1

[PATCH v6 1/2] media: i2c/ov5645: add the device tree binding document

[Todor Tomov]

Add the document for ov5645 device tree binding.

Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
---
 .../devicetree/bindings/media/i2c/ov5645.txt       | 52 ++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
new file mode 100644
index 0000000..bcf6dba
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
@@ -0,0 +1,52 @@
+* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
+
+The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
+an active array size of 2592H x 1944V. It is programmable through a serial I2C
+interface.
+
+Required Properties:
+- compatible: Value should be "ovti,ov5645".
+- clocks: Reference to the xclk clock.
+- clock-names: Should be "xclk".
+- clock-frequency: Frequency of the xclk clock.
+- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH.
+- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW.
+- vdddo-supply: Chip digital IO regulator.
+- vdda-supply: Chip analog regulator.
+- vddd-supply: Chip digital core regulator.
+
+The device node must contain one 'port' child node for its digital output
+video port, in accordance with the video interface bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+
+	&i2c1 {
+		...
+
+		ov5645: ov5645@78 {
+			compatible = "ovti,ov5645";
+			reg = <0x78>;
+
+			enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
+			reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&camera_rear_default>;
+
+			clocks = <&clks 200>;
+			clock-names = "xclk";
+			clock-frequency = <23880000>;
+
+			vdddo-supply = <&camera_dovdd_1v8>;
+			vdda-supply = <&camera_avdd_2v8>;
+			vddd-supply = <&camera_dvdd_1v2>;
+
+			port {
+				ov5645_ep: endpoint {
+					clock-lanes = <1>;
+					data-lanes = <0 2>;
+					remote-endpoint = <&csi0_ep>;
+				};
+			};
+		};
+	};
-- 
1.9.1

Re: [PATCH v6 1/2] media: i2c/ov5645: add the device tree binding document

[Laurent Pinchart]

Hi Todor,

Thank you for the patch.

On Thursday 08 Sep 2016 12:13:54 Todor Tomov wrote:
> Add the document for ov5645 device tree binding.
> 
> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> ---
>  .../devicetree/bindings/media/i2c/ov5645.txt       | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt new file mode
> 100644
> index 0000000..bcf6dba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> @@ -0,0 +1,52 @@
> +* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> +
> +The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor
> with +an active array size of 2592H x 1944V. It is programmable through a
> serial I2C +interface.
> +
> +Required Properties:
> +- compatible: Value should be "ovti,ov5645".
> +- clocks: Reference to the xclk clock.
> +- clock-names: Should be "xclk".
> +- clock-frequency: Frequency of the xclk clock.
> +- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH.
> +- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW.

Shouldn't the enable and reset GPIOs be optional ?

> +- vdddo-supply: Chip digital IO regulator.
> +- vdda-supply: Chip analog regulator.
> +- vddd-supply: Chip digital core regulator.
> +
> +The device node must contain one 'port' child node for its digital output
> +video port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> +	&i2c1 {
> +		...
> +
> +		ov5645: ov5645@78 {
> +			compatible = "ovti,ov5645";
> +			reg = <0x78>;
> +
> +			enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> +			reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&camera_rear_default>;
> +
> +			clocks = <&clks 200>;
> +			clock-names = "xclk";
> +			clock-frequency = <23880000>;
> +
> +			vdddo-supply = <&camera_dovdd_1v8>;
> +			vdda-supply = <&camera_avdd_2v8>;
> +			vddd-supply = <&camera_dvdd_1v2>;
> +
> +			port {
> +				ov5645_ep: endpoint {
> +					clock-lanes = <1>;
> +					data-lanes = <0 2>;
> +					remote-endpoint = <&csi0_ep>;
> +				};
> +			};
> +		};
> +	};

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 1/2] media: i2c/ov5645: add the device tree binding document

[Todor Tomov]

Hi Laurent,

Thank you for the review.

On 09/08/2016 03:22 PM, Laurent Pinchart wrote:
> Hi Todor,
> 
> Thank you for the patch.
> 
> On Thursday 08 Sep 2016 12:13:54 Todor Tomov wrote:
>> Add the document for ov5645 device tree binding.
>>
>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
>> ---
>>  .../devicetree/bindings/media/i2c/ov5645.txt       | 52 +++++++++++++++++++
>>  1 file changed, 52 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
>> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt new file mode
>> 100644
>> index 0000000..bcf6dba
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
>> @@ -0,0 +1,52 @@
>> +* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
>> +
>> +The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor
>> with +an active array size of 2592H x 1944V. It is programmable through a
>> serial I2C +interface.
>> +
>> +Required Properties:
>> +- compatible: Value should be "ovti,ov5645".
>> +- clocks: Reference to the xclk clock.
>> +- clock-names: Should be "xclk".
>> +- clock-frequency: Frequency of the xclk clock.
>> +- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH.
>> +- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW.
> 
> Shouldn't the enable and reset GPIOs be optional ?
I don't think so. The operations on the GPIOs are part of the power up sequence
of the sensor so we must have control over them to execute the exact sequence.

> 
>> +- vdddo-supply: Chip digital IO regulator.
>> +- vdda-supply: Chip analog regulator.
>> +- vddd-supply: Chip digital core regulator.
>> +
>> +The device node must contain one 'port' child node for its digital output
>> +video port, in accordance with the video interface bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example:
>> +
>> +	&i2c1 {
>> +		...
>> +
>> +		ov5645: ov5645@78 {
>> +			compatible = "ovti,ov5645";
>> +			reg = <0x78>;
>> +
>> +			enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
>> +			reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
>> +			pinctrl-names = "default";
>> +			pinctrl-0 = <&camera_rear_default>;
>> +
>> +			clocks = <&clks 200>;
>> +			clock-names = "xclk";
>> +			clock-frequency = <23880000>;
>> +
>> +			vdddo-supply = <&camera_dovdd_1v8>;
>> +			vdda-supply = <&camera_avdd_2v8>;
>> +			vddd-supply = <&camera_dvdd_1v2>;
>> +
>> +			port {
>> +				ov5645_ep: endpoint {
>> +					clock-lanes = <1>;
>> +					data-lanes = <0 2>;
>> +					remote-endpoint = <&csi0_ep>;
>> +				};
>> +			};
>> +		};
>> +	};
> 

-- 
Best regards,
Todor Tomov

Re: [PATCH v6 1/2] media: i2c/ov5645: add the device tree binding document

[Laurent Pinchart]

Hi Todor,

On Friday 14 Oct 2016 15:01:08 Todor Tomov wrote:
> On 09/08/2016 03:22 PM, Laurent Pinchart wrote:
> > On Thursday 08 Sep 2016 12:13:54 Todor Tomov wrote:
> >> Add the document for ov5645 device tree binding.
> >> 
> >> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> >> ---
> >> 
> >>  .../devicetree/bindings/media/i2c/ov5645.txt       | 52 ++++++++++++++++
> >>  1 file changed, 52 insertions(+)
> >>  create mode 100644
> >>  Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >> 
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt new file mode
> >> 100644
> >> index 0000000..bcf6dba
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >> @@ -0,0 +1,52 @@
> >> +* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> >> +
> >> +The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image
> >> sensor with
> >> +an active array size of 2592H x 1944V. It is programmable through a
> >> serial I2C
> >> +interface.
> >> +
> >> +Required Properties:
> >> +- compatible: Value should be "ovti,ov5645".
> >> +- clocks: Reference to the xclk clock.
> >> +- clock-names: Should be "xclk".
> >> +- clock-frequency: Frequency of the xclk clock.
> >> +- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH.

By the way, isn't the pin called pwdnb and isn't it active low ?

> >> +- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW.
> > 
> > Shouldn't the enable and reset GPIOs be optional ?
> 
> I don't think so. The operations on the GPIOs are part of the power up
> sequence of the sensor so we must have control over them to execute the
> exact sequence.

Right, let's keep them mandatory. If we later have to make them optional for a 
board that pulls one of those signals up (assuming this can work at all) we'll 
revisit the bindings.

> >> +- vdddo-supply: Chip digital IO regulator.
> >> +- vdda-supply: Chip analog regulator.
> >> +- vddd-supply: Chip digital core regulator.
> >> +
> >> +The device node must contain one 'port' child node for its digital
> >> output
> >> +video port, in accordance with the video interface bindings defined in
> >> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> >> +
> >> +Example:
> >> +
> >> +	&i2c1 {
> >> +		...
> >> +
> >> +		ov5645: ov5645@78 {
> >> +			compatible = "ovti,ov5645";
> >> +			reg = <0x78>;
> >> +
> >> +			enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> >> +			reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
> >> +			pinctrl-names = "default";
> >> +			pinctrl-0 = <&camera_rear_default>;
> >> +
> >> +			clocks = <&clks 200>;
> >> +			clock-names = "xclk";
> >> +			clock-frequency = <23880000>;
> >> +
> >> +			vdddo-supply = <&camera_dovdd_1v8>;
> >> +			vdda-supply = <&camera_avdd_2v8>;
> >> +			vddd-supply = <&camera_dvdd_1v2>;
> >> +
> >> +			port {
> >> +				ov5645_ep: endpoint {
> >> +					clock-lanes = <1>;
> >> +					data-lanes = <0 2>;
> >> +					remote-endpoint = <&csi0_ep>;
> >> +				};
> >> +			};
> >> +		};
> >> +	};

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 1/2] media: i2c/ov5645: add the device tree binding document

[Todor Tomov]

Hi Laurent,

Thank you for the review.

On 10/19/2016 11:49 AM, Laurent Pinchart wrote:
> Hi Todor,
> 
> On Friday 14 Oct 2016 15:01:08 Todor Tomov wrote:
>> On 09/08/2016 03:22 PM, Laurent Pinchart wrote:
>>> On Thursday 08 Sep 2016 12:13:54 Todor Tomov wrote:
>>>> Add the document for ov5645 device tree binding.
>>>>
>>>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
>>>> ---
>>>>
>>>>  .../devicetree/bindings/media/i2c/ov5645.txt       | 52 ++++++++++++++++
>>>>  1 file changed, 52 insertions(+)
>>>>  create mode 100644
>>>>  Documentation/devicetree/bindings/media/i2c/ov5645.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
>>>> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt new file mode
>>>> 100644
>>>> index 0000000..bcf6dba
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
>>>> @@ -0,0 +1,52 @@
>>>> +* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
>>>> +
>>>> +The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image
>>>> sensor with
>>>> +an active array size of 2592H x 1944V. It is programmable through a
>>>> serial I2C
>>>> +interface.
>>>> +
>>>> +Required Properties:
>>>> +- compatible: Value should be "ovti,ov5645".
>>>> +- clocks: Reference to the xclk clock.
>>>> +- clock-names: Should be "xclk".
>>>> +- clock-frequency: Frequency of the xclk clock.
>>>> +- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH.
> 
> By the way, isn't the pin called pwdnb and isn't it active low ?

Yes, the pin is called "pwdnb" and is active low (must be up for power to be up).
I have changed the name to "enable" as it is more generally used - this change
was suggested by Rob Herring. As the logic switches with this change of the name
I have stated it is active high which ends up in the same condition (enable
must be up for the power to be up). I think this is correct, isn't it?

> 
>>>> +- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW.
>>>
>>> Shouldn't the enable and reset GPIOs be optional ?
>>
>> I don't think so. The operations on the GPIOs are part of the power up
>> sequence of the sensor so we must have control over them to execute the
>> exact sequence.
> 
> Right, let's keep them mandatory. If we later have to make them optional for a 
> board that pulls one of those signals up (assuming this can work at all) we'll 
> revisit the bindings.

Ok.

> 
>>>> +- vdddo-supply: Chip digital IO regulator.
>>>> +- vdda-supply: Chip analog regulator.
>>>> +- vddd-supply: Chip digital core regulator.
>>>> +
>>>> +The device node must contain one 'port' child node for its digital
>>>> output
>>>> +video port, in accordance with the video interface bindings defined in
>>>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>>>> +
>>>> +Example:
>>>> +
>>>> +	&i2c1 {
>>>> +		...
>>>> +
>>>> +		ov5645: ov5645@78 {
>>>> +			compatible = "ovti,ov5645";
>>>> +			reg = <0x78>;
>>>> +
>>>> +			enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
>>>> +			reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
>>>> +			pinctrl-names = "default";
>>>> +			pinctrl-0 = <&camera_rear_default>;
>>>> +
>>>> +			clocks = <&clks 200>;
>>>> +			clock-names = "xclk";
>>>> +			clock-frequency = <23880000>;
>>>> +
>>>> +			vdddo-supply = <&camera_dovdd_1v8>;
>>>> +			vdda-supply = <&camera_avdd_2v8>;
>>>> +			vddd-supply = <&camera_dvdd_1v2>;
>>>> +
>>>> +			port {
>>>> +				ov5645_ep: endpoint {
>>>> +					clock-lanes = <1>;
>>>> +					data-lanes = <0 2>;
>>>> +					remote-endpoint = <&csi0_ep>;
>>>> +				};
>>>> +			};
>>>> +		};
>>>> +	};
> 

-- 
Best regards,
Todor Tomov

Re: [PATCH v6 1/2] media: i2c/ov5645: add the device tree binding document

[Laurent Pinchart]

Hi Todor,

On Wednesday 19 Oct 2016 12:14:55 Todor Tomov wrote:
> On 10/19/2016 11:49 AM, Laurent Pinchart wrote:
> > On Friday 14 Oct 2016 15:01:08 Todor Tomov wrote:
> >> On 09/08/2016 03:22 PM, Laurent Pinchart wrote:
> >>> On Thursday 08 Sep 2016 12:13:54 Todor Tomov wrote:
> >>>> Add the document for ov5645 device tree binding.
> >>>> 
> >>>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> >>>> ---
> >>>> 
> >>>>  .../devicetree/bindings/media/i2c/ov5645.txt       | 52 ++++++++++++++
> >>>>  1 file changed, 52 insertions(+)
> >>>>  create mode 100644
> >>>>  Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>> 
> >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt new file mode
> >>>> 100644
> >>>> index 0000000..bcf6dba
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>> @@ -0,0 +1,52 @@
> >>>> +* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> >>>> +
> >>>> +The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image
> >>>> sensor with
> >>>> +an active array size of 2592H x 1944V. It is programmable through a
> >>>> serial I2C
> >>>> +interface.
> >>>> +
> >>>> +Required Properties:
> >>>> +- compatible: Value should be "ovti,ov5645".
> >>>> +- clocks: Reference to the xclk clock.
> >>>> +- clock-names: Should be "xclk".
> >>>> +- clock-frequency: Frequency of the xclk clock.
> >>>> +- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH.
> > 
> > By the way, isn't the pin called pwdnb and isn't it active low ?
> 
> Yes, the pin is called "pwdnb" and is active low (must be up for power to be
> up). I have changed the name to "enable" as it is more generally used -
> this change was suggested by Rob Herring. As the logic switches with this
> change of the name I have stated it is active high which ends up in the
> same condition (enable must be up for the power to be up). I think this is
> correct, isn't it?

I thought that the rule was to name the GPIO properties based on the name of 
the pin. I could be wrong though. Rob, what's your opinion ?

> >>>> +- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW.
> >>> 
> >>> Shouldn't the enable and reset GPIOs be optional ?
> >> 
> >> I don't think so. The operations on the GPIOs are part of the power up
> >> sequence of the sensor so we must have control over them to execute the
> >> exact sequence.
> > 
> > Right, let's keep them mandatory. If we later have to make them optional
> > for a board that pulls one of those signals up (assuming this can work at
> > all) we'll revisit the bindings.
> 
> Ok.
> 
> >>>> +- vdddo-supply: Chip digital IO regulator.
> >>>> +- vdda-supply: Chip analog regulator.
> >>>> +- vddd-supply: Chip digital core regulator.
> >>>> +
> >>>> +The device node must contain one 'port' child node for its digital
> >>>> output
> >>>> +video port, in accordance with the video interface bindings defined in
> >>>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> >>>> +
> >>>> +Example:
> >>>> +
> >>>> +	&i2c1 {
> >>>> +		...
> >>>> +
> >>>> +		ov5645: ov5645@78 {
> >>>> +			compatible = "ovti,ov5645";
> >>>> +			reg = <0x78>;
> >>>> +
> >>>> +			enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> >>>> +			reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
> >>>> +			pinctrl-names = "default";
> >>>> +			pinctrl-0 = <&camera_rear_default>;
> >>>> +
> >>>> +			clocks = <&clks 200>;
> >>>> +			clock-names = "xclk";
> >>>> +			clock-frequency = <23880000>;
> >>>> +
> >>>> +			vdddo-supply = <&camera_dovdd_1v8>;
> >>>> +			vdda-supply = <&camera_avdd_2v8>;
> >>>> +			vddd-supply = <&camera_dvdd_1v2>;
> >>>> +
> >>>> +			port {
> >>>> +				ov5645_ep: endpoint {
> >>>> +					clock-lanes = <1>;
> >>>> +					data-lanes = <0 2>;
> >>>> +					remote-endpoint = <&csi0_ep>;
> >>>> +				};
> >>>> +			};
> >>>> +		};
> >>>> +	};

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 1/2] media: i2c/ov5645: add the device tree binding document

[Rob Herring]

On Wed, Oct 19, 2016 at 4:21 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Todor,
>
> On Wednesday 19 Oct 2016 12:14:55 Todor Tomov wrote:
>> On 10/19/2016 11:49 AM, Laurent Pinchart wrote:
>> > On Friday 14 Oct 2016 15:01:08 Todor Tomov wrote:
>> >> On 09/08/2016 03:22 PM, Laurent Pinchart wrote:
>> >>> On Thursday 08 Sep 2016 12:13:54 Todor Tomov wrote:
>> >>>> Add the document for ov5645 device tree binding.
>> >>>>
>> >>>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
>> >>>> ---
>> >>>>
>> >>>>  .../devicetree/bindings/media/i2c/ov5645.txt       | 52 ++++++++++++++
>> >>>>  1 file changed, 52 insertions(+)
>> >>>>  create mode 100644
>> >>>>  Documentation/devicetree/bindings/media/i2c/ov5645.txt
>> >>>>
>> >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
>> >>>> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt new file mode
>> >>>> 100644
>> >>>> index 0000000..bcf6dba
>> >>>> --- /dev/null
>> >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
>> >>>> @@ -0,0 +1,52 @@
>> >>>> +* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
>> >>>> +
>> >>>> +The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image
>> >>>> sensor with
>> >>>> +an active array size of 2592H x 1944V. It is programmable through a
>> >>>> serial I2C
>> >>>> +interface.
>> >>>> +
>> >>>> +Required Properties:
>> >>>> +- compatible: Value should be "ovti,ov5645".
>> >>>> +- clocks: Reference to the xclk clock.
>> >>>> +- clock-names: Should be "xclk".
>> >>>> +- clock-frequency: Frequency of the xclk clock.
>> >>>> +- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH.
>> >
>> > By the way, isn't the pin called pwdnb and isn't it active low ?
>>
>> Yes, the pin is called "pwdnb" and is active low (must be up for power to be
>> up). I have changed the name to "enable" as it is more generally used -
>> this change was suggested by Rob Herring. As the logic switches with this
>> change of the name I have stated it is active high which ends up in the
>> same condition (enable must be up for the power to be up). I think this is
>> correct, isn't it?
>
> I thought that the rule was to name the GPIO properties based on the name of
> the pin. I could be wrong though. Rob, what's your opinion ?

Generally, yes that is the rule. However, an enable (or powerdown
being the inverse) pin is so common that I think it makes sense to use
a common name. Then generic power sequencing code can power up devices
(in the simple cases at least).

Rob

Re: [PATCH v6 1/2] media: i2c/ov5645: add the device tree binding document

[Todor Tomov]

On 10/26/2016 09:53 PM, Rob Herring wrote:
> On Wed, Oct 19, 2016 at 4:21 AM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hi Todor,
>>
>> On Wednesday 19 Oct 2016 12:14:55 Todor Tomov wrote:
>>> On 10/19/2016 11:49 AM, Laurent Pinchart wrote:
>>>> On Friday 14 Oct 2016 15:01:08 Todor Tomov wrote:
>>>>> On 09/08/2016 03:22 PM, Laurent Pinchart wrote:
>>>>>> On Thursday 08 Sep 2016 12:13:54 Todor Tomov wrote:
>>>>>>> Add the document for ov5645 device tree binding.
>>>>>>>
>>>>>>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
>>>>>>> ---
>>>>>>>
>>>>>>>  .../devicetree/bindings/media/i2c/ov5645.txt       | 52 ++++++++++++++
>>>>>>>  1 file changed, 52 insertions(+)
>>>>>>>  create mode 100644
>>>>>>>  Documentation/devicetree/bindings/media/i2c/ov5645.txt
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
>>>>>>> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt new file mode
>>>>>>> 100644
>>>>>>> index 0000000..bcf6dba
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
>>>>>>> @@ -0,0 +1,52 @@
>>>>>>> +* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
>>>>>>> +
>>>>>>> +The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image
>>>>>>> sensor with
>>>>>>> +an active array size of 2592H x 1944V. It is programmable through a
>>>>>>> serial I2C
>>>>>>> +interface.
>>>>>>> +
>>>>>>> +Required Properties:
>>>>>>> +- compatible: Value should be "ovti,ov5645".
>>>>>>> +- clocks: Reference to the xclk clock.
>>>>>>> +- clock-names: Should be "xclk".
>>>>>>> +- clock-frequency: Frequency of the xclk clock.
>>>>>>> +- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH.
>>>>
>>>> By the way, isn't the pin called pwdnb and isn't it active low ?
>>>
>>> Yes, the pin is called "pwdnb" and is active low (must be up for power to be
>>> up). I have changed the name to "enable" as it is more generally used -
>>> this change was suggested by Rob Herring. As the logic switches with this
>>> change of the name I have stated it is active high which ends up in the
>>> same condition (enable must be up for the power to be up). I think this is
>>> correct, isn't it?
>>
>> I thought that the rule was to name the GPIO properties based on the name of
>> the pin. I could be wrong though. Rob, what's your opinion ?
> 
> Generally, yes that is the rule. However, an enable (or powerdown
> being the inverse) pin is so common that I think it makes sense to use
> a common name. Then generic power sequencing code can power up devices
> (in the simple cases at least).

Ok, so what can we decide about this case? I personally have a slight preference
for the name same as documentation. But I think most important is to follow the
rule if we have such a rule. If we don't have a single rule to follow every time
then it is not really important which one we will choose.

> 
> Rob
> 

-- 
Best regards,
Todor Tomov

Re: [PATCH v6 1/2] media: i2c/ov5645: add the device tree binding document

[Laurent Pinchart]

Hi Todor,

On Tuesday 01 Nov 2016 10:24:26 Todor Tomov wrote:
> On 10/26/2016 09:53 PM, Rob Herring wrote:
> > On Wed, Oct 19, 2016 at 4:21 AM, Laurent Pinchart wrote:
> >> On Wednesday 19 Oct 2016 12:14:55 Todor Tomov wrote:
> >>> On 10/19/2016 11:49 AM, Laurent Pinchart wrote:
> >>>> On Friday 14 Oct 2016 15:01:08 Todor Tomov wrote:
> >>>>> On 09/08/2016 03:22 PM, Laurent Pinchart wrote:
> >>>>>> On Thursday 08 Sep 2016 12:13:54 Todor Tomov wrote:
> >>>>>>> Add the document for ov5645 device tree binding.
> >>>>>>> 
> >>>>>>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>>  .../devicetree/bindings/media/i2c/ov5645.txt       | 52 +++++++++++
> >>>>>>>  1 file changed, 52 insertions(+)
> >>>>>>>  create mode 100644
> >>>>>>>  Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>>>>> 
> >>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>>>>> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt new file
> >>>>>>> mode
> >>>>>>> 100644
> >>>>>>> index 0000000..bcf6dba
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>>>>> @@ -0,0 +1,52 @@
> >>>>>>> +* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> >>>>>>> +
> >>>>>>> +The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image
> >>>>>>> sensor with
> >>>>>>> +an active array size of 2592H x 1944V. It is programmable through a
> >>>>>>> serial I2C
> >>>>>>> +interface.
> >>>>>>> +
> >>>>>>> +Required Properties:
> >>>>>>> +- compatible: Value should be "ovti,ov5645".
> >>>>>>> +- clocks: Reference to the xclk clock.
> >>>>>>> +- clock-names: Should be "xclk".
> >>>>>>> +- clock-frequency: Frequency of the xclk clock.
> >>>>>>> +- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH.
> >>>> 
> >>>> By the way, isn't the pin called pwdnb and isn't it active low ?
> >>> 
> >>> Yes, the pin is called "pwdnb" and is active low (must be up for power
> >>> to be up). I have changed the name to "enable" as it is more generally
> >>> used - this change was suggested by Rob Herring. As the logic switches
> >>> with this change of the name I have stated it is active high which ends
> >>> up in the same condition (enable must be up for the power to be up). I
> >>> think this is correct, isn't it?
> >> 
> >> I thought that the rule was to name the GPIO properties based on the name
> >> of the pin. I could be wrong though. Rob, what's your opinion ?
> > 
> > Generally, yes that is the rule. However, an enable (or powerdown
> > being the inverse) pin is so common that I think it makes sense to use
> > a common name. Then generic power sequencing code can power up devices
> > (in the simple cases at least).
> 
> Ok, so what can we decide about this case? I personally have a slight
> preference for the name same as documentation. But I think most important
> is to follow the rule if we have such a rule. If we don't have a single
> rule to follow every time then it is not really important which one we will
> choose.

I'm fine with both solutions (and also have a slight preference for using the 
chip's pin name). If we decide to use "enable-gpios", the DT binding document 
should mention that the property corresponds to the chip's PWDNB pin.

-- 
Regards,

Laurent Pinchart

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

[Todor Tomov]

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 <todor.tomov@linaro.org>
---
 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

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index ce9006e..f934d1f 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -524,6 +524,18 @@ config VIDEO_OV2659
 	  To compile this driver as a module, choose M here: the
 	  module will be called ov2659.
 
+config VIDEO_OV5645
+	tristate "OmniVision OV5645 sensor support"
+	depends on OF
+	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+	depends on MEDIA_CAMERA_SUPPORT
+	---help---
+	  This is a Video4Linux2 sensor-level driver for the OmniVision
+	  OV5645 camera.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ov5645.
+
 config VIDEO_OV7640
 	tristate "OmniVision OV7640 sensor support"
 	depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 94f2c99..2485aed 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
 obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
 obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
 obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
+obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
 obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
 obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
 obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
new file mode 100644
index 0000000..5e5c37e
--- /dev/null
+++ b/drivers/media/i2c/ov5645.c
@@ -0,0 +1,1372 @@
+/*
+ * Driver for the OV5645 camera sensor.
+ *
+ * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2015 By Tech Design S.L. All Rights Reserved.
+ * Copyright (C) 2012-2013 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * Based on:
+ * - the OV5645 driver from QC msm-3.10 kernel on codeaurora.org:
+ *   https://us.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/
+ *       media/platform/msm/camera_v2/sensor/ov5645.c?h=LA.BR.1.2.4_rb1.41
+ * - the OV5640 driver posted on linux-media:
+ *   https://www.mail-archive.com/linux-media%40vger.kernel.org/msg92671.html
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-of.h>
+#include <media/v4l2-subdev.h>
+
+#define OV5645_VOLTAGE_ANALOG               2800000
+#define OV5645_VOLTAGE_DIGITAL_CORE         1500000
+#define OV5645_VOLTAGE_DIGITAL_IO           1800000
+
+#define OV5645_SYSTEM_CTRL0		0x3008
+#define		OV5645_SYSTEM_CTRL0_START	0x02
+#define		OV5645_SYSTEM_CTRL0_STOP	0x42
+#define OV5645_CHIP_ID_HIGH		0x300A
+#define		OV5645_CHIP_ID_HIGH_BYTE	0x56
+#define OV5645_CHIP_ID_LOW		0x300B
+#define		OV5645_CHIP_ID_LOW_BYTE		0x45
+#define OV5645_AWB_MANUAL_CONTROL	0x3406
+#define		OV5645_AWB_MANUAL_ENABLE	BIT(0)
+#define OV5645_AEC_PK_MANUAL		0x3503
+#define		OV5645_AEC_MANUAL_ENABLE	BIT(0)
+#define		OV5645_AGC_MANUAL_ENABLE	BIT(1)
+#define OV5645_TIMING_TC_REG20		0x3820
+#define		OV5645_SENSOR_VFLIP		BIT(1)
+#define		OV5645_ISP_VFLIP		BIT(2)
+#define OV5645_TIMING_TC_REG21		0x3821
+#define		OV5645_SENSOR_MIRROR		BIT(1)
+#define OV5645_PRE_ISP_TEST_SETTING_1	0x503d
+#define		OV5645_TEST_PATTERN_MASK	0x3
+#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_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;
+	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 23880000Hz */
+	u32 xclk_freq;
+
+	struct regulator *io_regulator;
+	struct regulator *core_regulator;
+	struct regulator *analog_regulator;
+
+	enum ov5645_mode current_mode;
+
+	/* 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[] = {
+	{ 0x3103, 0x11 },
+	{ 0x3008, 0x82 },
+	{ 0x3008, 0x42 },
+	{ 0x3103, 0x03 },
+	{ 0x3503, 0x07 },
+	{ 0x3002, 0x1c },
+	{ 0x3006, 0xc3 },
+	{ 0x300e, 0x45 },
+	{ 0x3017, 0x00 },
+	{ 0x3018, 0x00 },
+	{ 0x302e, 0x0b },
+	{ 0x3037, 0x13 },
+	{ 0x3108, 0x01 },
+	{ 0x3611, 0x06 },
+	{ 0x3500, 0x00 },
+	{ 0x3501, 0x01 },
+	{ 0x3502, 0x00 },
+	{ 0x350a, 0x00 },
+	{ 0x350b, 0x3f },
+	{ 0x3620, 0x33 },
+	{ 0x3621, 0xe0 },
+	{ 0x3622, 0x01 },
+	{ 0x3630, 0x2e },
+	{ 0x3631, 0x00 },
+	{ 0x3632, 0x32 },
+	{ 0x3633, 0x52 },
+	{ 0x3634, 0x70 },
+	{ 0x3635, 0x13 },
+	{ 0x3636, 0x03 },
+	{ 0x3703, 0x5a },
+	{ 0x3704, 0xa0 },
+	{ 0x3705, 0x1a },
+	{ 0x3709, 0x12 },
+	{ 0x370b, 0x61 },
+	{ 0x370f, 0x10 },
+	{ 0x3715, 0x78 },
+	{ 0x3717, 0x01 },
+	{ 0x371b, 0x20 },
+	{ 0x3731, 0x12 },
+	{ 0x3901, 0x0a },
+	{ 0x3905, 0x02 },
+	{ 0x3906, 0x10 },
+	{ 0x3719, 0x86 },
+	{ 0x3810, 0x00 },
+	{ 0x3811, 0x10 },
+	{ 0x3812, 0x00 },
+	{ 0x3821, 0x01 },
+	{ 0x3824, 0x01 },
+	{ 0x3826, 0x03 },
+	{ 0x3828, 0x08 },
+	{ 0x3a19, 0xf8 },
+	{ 0x3c01, 0x34 },
+	{ 0x3c04, 0x28 },
+	{ 0x3c05, 0x98 },
+	{ 0x3c07, 0x07 },
+	{ 0x3c09, 0xc2 },
+	{ 0x3c0a, 0x9c },
+	{ 0x3c0b, 0x40 },
+	{ 0x3c01, 0x34 },
+	{ 0x4001, 0x02 },
+	{ 0x4514, 0x00 },
+	{ 0x4520, 0xb0 },
+	{ 0x460b, 0x37 },
+	{ 0x460c, 0x20 },
+	{ 0x4818, 0x01 },
+	{ 0x481d, 0xf0 },
+	{ 0x481f, 0x50 },
+	{ 0x4823, 0x70 },
+	{ 0x4831, 0x14 },
+	{ 0x5000, 0xa7 },
+	{ 0x5001, 0x83 },
+	{ 0x501d, 0x00 },
+	{ 0x501f, 0x00 },
+	{ 0x503d, 0x00 },
+	{ 0x505c, 0x30 },
+	{ 0x5181, 0x59 },
+	{ 0x5183, 0x00 },
+	{ 0x5191, 0xf0 },
+	{ 0x5192, 0x03 },
+	{ 0x5684, 0x10 },
+	{ 0x5685, 0xa0 },
+	{ 0x5686, 0x0c },
+	{ 0x5687, 0x78 },
+	{ 0x5a00, 0x08 },
+	{ 0x5a21, 0x00 },
+	{ 0x5a24, 0x00 },
+	{ 0x3008, 0x02 },
+	{ 0x3503, 0x00 },
+	{ 0x5180, 0xff },
+	{ 0x5181, 0xf2 },
+	{ 0x5182, 0x00 },
+	{ 0x5183, 0x14 },
+	{ 0x5184, 0x25 },
+	{ 0x5185, 0x24 },
+	{ 0x5186, 0x09 },
+	{ 0x5187, 0x09 },
+	{ 0x5188, 0x0a },
+	{ 0x5189, 0x75 },
+	{ 0x518a, 0x52 },
+	{ 0x518b, 0xea },
+	{ 0x518c, 0xa8 },
+	{ 0x518d, 0x42 },
+	{ 0x518e, 0x38 },
+	{ 0x518f, 0x56 },
+	{ 0x5190, 0x42 },
+	{ 0x5191, 0xf8 },
+	{ 0x5192, 0x04 },
+	{ 0x5193, 0x70 },
+	{ 0x5194, 0xf0 },
+	{ 0x5195, 0xf0 },
+	{ 0x5196, 0x03 },
+	{ 0x5197, 0x01 },
+	{ 0x5198, 0x04 },
+	{ 0x5199, 0x12 },
+	{ 0x519a, 0x04 },
+	{ 0x519b, 0x00 },
+	{ 0x519c, 0x06 },
+	{ 0x519d, 0x82 },
+	{ 0x519e, 0x38 },
+	{ 0x5381, 0x1e },
+	{ 0x5382, 0x5b },
+	{ 0x5383, 0x08 },
+	{ 0x5384, 0x0a },
+	{ 0x5385, 0x7e },
+	{ 0x5386, 0x88 },
+	{ 0x5387, 0x7c },
+	{ 0x5388, 0x6c },
+	{ 0x5389, 0x10 },
+	{ 0x538a, 0x01 },
+	{ 0x538b, 0x98 },
+	{ 0x5300, 0x08 },
+	{ 0x5301, 0x30 },
+	{ 0x5302, 0x10 },
+	{ 0x5303, 0x00 },
+	{ 0x5304, 0x08 },
+	{ 0x5305, 0x30 },
+	{ 0x5306, 0x08 },
+	{ 0x5307, 0x16 },
+	{ 0x5309, 0x08 },
+	{ 0x530a, 0x30 },
+	{ 0x530b, 0x04 },
+	{ 0x530c, 0x06 },
+	{ 0x5480, 0x01 },
+	{ 0x5481, 0x08 },
+	{ 0x5482, 0x14 },
+	{ 0x5483, 0x28 },
+	{ 0x5484, 0x51 },
+	{ 0x5485, 0x65 },
+	{ 0x5486, 0x71 },
+	{ 0x5487, 0x7d },
+	{ 0x5488, 0x87 },
+	{ 0x5489, 0x91 },
+	{ 0x548a, 0x9a },
+	{ 0x548b, 0xaa },
+	{ 0x548c, 0xb8 },
+	{ 0x548d, 0xcd },
+	{ 0x548e, 0xdd },
+	{ 0x548f, 0xea },
+	{ 0x5490, 0x1d },
+	{ 0x5580, 0x02 },
+	{ 0x5583, 0x40 },
+	{ 0x5584, 0x10 },
+	{ 0x5589, 0x10 },
+	{ 0x558a, 0x00 },
+	{ 0x558b, 0xf8 },
+	{ 0x5800, 0x3f },
+	{ 0x5801, 0x16 },
+	{ 0x5802, 0x0e },
+	{ 0x5803, 0x0d },
+	{ 0x5804, 0x17 },
+	{ 0x5805, 0x3f },
+	{ 0x5806, 0x0b },
+	{ 0x5807, 0x06 },
+	{ 0x5808, 0x04 },
+	{ 0x5809, 0x04 },
+	{ 0x580a, 0x06 },
+	{ 0x580b, 0x0b },
+	{ 0x580c, 0x09 },
+	{ 0x580d, 0x03 },
+	{ 0x580e, 0x00 },
+	{ 0x580f, 0x00 },
+	{ 0x5810, 0x03 },
+	{ 0x5811, 0x08 },
+	{ 0x5812, 0x0a },
+	{ 0x5813, 0x03 },
+	{ 0x5814, 0x00 },
+	{ 0x5815, 0x00 },
+	{ 0x5816, 0x04 },
+	{ 0x5817, 0x09 },
+	{ 0x5818, 0x0f },
+	{ 0x5819, 0x08 },
+	{ 0x581a, 0x06 },
+	{ 0x581b, 0x06 },
+	{ 0x581c, 0x08 },
+	{ 0x581d, 0x0c },
+	{ 0x581e, 0x3f },
+	{ 0x581f, 0x1e },
+	{ 0x5820, 0x12 },
+	{ 0x5821, 0x13 },
+	{ 0x5822, 0x21 },
+	{ 0x5823, 0x3f },
+	{ 0x5824, 0x68 },
+	{ 0x5825, 0x28 },
+	{ 0x5826, 0x2c },
+	{ 0x5827, 0x28 },
+	{ 0x5828, 0x08 },
+	{ 0x5829, 0x48 },
+	{ 0x582a, 0x64 },
+	{ 0x582b, 0x62 },
+	{ 0x582c, 0x64 },
+	{ 0x582d, 0x28 },
+	{ 0x582e, 0x46 },
+	{ 0x582f, 0x62 },
+	{ 0x5830, 0x60 },
+	{ 0x5831, 0x62 },
+	{ 0x5832, 0x26 },
+	{ 0x5833, 0x48 },
+	{ 0x5834, 0x66 },
+	{ 0x5835, 0x44 },
+	{ 0x5836, 0x64 },
+	{ 0x5837, 0x28 },
+	{ 0x5838, 0x66 },
+	{ 0x5839, 0x48 },
+	{ 0x583a, 0x2c },
+	{ 0x583b, 0x28 },
+	{ 0x583c, 0x26 },
+	{ 0x583d, 0xae },
+	{ 0x5025, 0x00 },
+	{ 0x3a0f, 0x30 },
+	{ 0x3a10, 0x28 },
+	{ 0x3a1b, 0x30 },
+	{ 0x3a1e, 0x26 },
+	{ 0x3a11, 0x60 },
+	{ 0x3a1f, 0x14 },
+	{ 0x0601, 0x02 },
+	{ 0x3008, 0x42 },
+	{ 0x3008, 0x02 }
+};
+
+static struct reg_value ov5645_setting_sxga[] = {
+	{ 0x3612, 0xa9 },
+	{ 0x3614, 0x50 },
+	{ 0x3618, 0x00 },
+	{ 0x3034, 0x18 },
+	{ 0x3035, 0x21 },
+	{ 0x3036, 0x70 },
+	{ 0x3600, 0x09 },
+	{ 0x3601, 0x43 },
+	{ 0x3708, 0x66 },
+	{ 0x370c, 0xc3 },
+	{ 0x3800, 0x00 },
+	{ 0x3801, 0x00 },
+	{ 0x3802, 0x00 },
+	{ 0x3803, 0x06 },
+	{ 0x3804, 0x0a },
+	{ 0x3805, 0x3f },
+	{ 0x3806, 0x07 },
+	{ 0x3807, 0x9d },
+	{ 0x3808, 0x05 },
+	{ 0x3809, 0x00 },
+	{ 0x380a, 0x03 },
+	{ 0x380b, 0xc0 },
+	{ 0x380c, 0x07 },
+	{ 0x380d, 0x68 },
+	{ 0x380e, 0x03 },
+	{ 0x380f, 0xd8 },
+	{ 0x3813, 0x06 },
+	{ 0x3814, 0x31 },
+	{ 0x3815, 0x31 },
+	{ 0x3820, 0x47 },
+	{ 0x3a02, 0x03 },
+	{ 0x3a03, 0xd8 },
+	{ 0x3a08, 0x01 },
+	{ 0x3a09, 0xf8 },
+	{ 0x3a0a, 0x01 },
+	{ 0x3a0b, 0xa4 },
+	{ 0x3a0e, 0x02 },
+	{ 0x3a0d, 0x02 },
+	{ 0x3a14, 0x03 },
+	{ 0x3a15, 0xd8 },
+	{ 0x3a18, 0x00 },
+	{ 0x4004, 0x02 },
+	{ 0x4005, 0x18 },
+	{ 0x4300, 0x32 },
+	{ 0x4202, 0x00 }
+};
+
+static struct reg_value ov5645_setting_1080p[] = {
+	{ 0x3612, 0xab },
+	{ 0x3614, 0x50 },
+	{ 0x3618, 0x04 },
+	{ 0x3034, 0x18 },
+	{ 0x3035, 0x11 },
+	{ 0x3036, 0x54 },
+	{ 0x3600, 0x08 },
+	{ 0x3601, 0x33 },
+	{ 0x3708, 0x63 },
+	{ 0x370c, 0xc0 },
+	{ 0x3800, 0x01 },
+	{ 0x3801, 0x50 },
+	{ 0x3802, 0x01 },
+	{ 0x3803, 0xb2 },
+	{ 0x3804, 0x08 },
+	{ 0x3805, 0xef },
+	{ 0x3806, 0x05 },
+	{ 0x3807, 0xf1 },
+	{ 0x3808, 0x07 },
+	{ 0x3809, 0x80 },
+	{ 0x380a, 0x04 },
+	{ 0x380b, 0x38 },
+	{ 0x380c, 0x09 },
+	{ 0x380d, 0xc4 },
+	{ 0x380e, 0x04 },
+	{ 0x380f, 0x60 },
+	{ 0x3813, 0x04 },
+	{ 0x3814, 0x11 },
+	{ 0x3815, 0x11 },
+	{ 0x3820, 0x47 },
+	{ 0x4514, 0x88 },
+	{ 0x3a02, 0x04 },
+	{ 0x3a03, 0x60 },
+	{ 0x3a08, 0x01 },
+	{ 0x3a09, 0x50 },
+	{ 0x3a0a, 0x01 },
+	{ 0x3a0b, 0x18 },
+	{ 0x3a0e, 0x03 },
+	{ 0x3a0d, 0x04 },
+	{ 0x3a14, 0x04 },
+	{ 0x3a15, 0x60 },
+	{ 0x3a18, 0x00 },
+	{ 0x4004, 0x06 },
+	{ 0x4005, 0x18 },
+	{ 0x4300, 0x32 },
+	{ 0x4202, 0x00 },
+	{ 0x4837, 0x0b }
+};
+
+static struct reg_value ov5645_setting_full[] = {
+	{ 0x3612, 0xab },
+	{ 0x3614, 0x50 },
+	{ 0x3618, 0x04 },
+	{ 0x3034, 0x18 },
+	{ 0x3035, 0x11 },
+	{ 0x3036, 0x54 },
+	{ 0x3600, 0x08 },
+	{ 0x3601, 0x33 },
+	{ 0x3708, 0x63 },
+	{ 0x370c, 0xc0 },
+	{ 0x3800, 0x00 },
+	{ 0x3801, 0x00 },
+	{ 0x3802, 0x00 },
+	{ 0x3803, 0x00 },
+	{ 0x3804, 0x0a },
+	{ 0x3805, 0x3f },
+	{ 0x3806, 0x07 },
+	{ 0x3807, 0x9f },
+	{ 0x3808, 0x0a },
+	{ 0x3809, 0x20 },
+	{ 0x380a, 0x07 },
+	{ 0x380b, 0x98 },
+	{ 0x380c, 0x0b },
+	{ 0x380d, 0x1c },
+	{ 0x380e, 0x07 },
+	{ 0x380f, 0xb0 },
+	{ 0x3813, 0x06 },
+	{ 0x3814, 0x11 },
+	{ 0x3815, 0x11 },
+	{ 0x3820, 0x47 },
+	{ 0x4514, 0x88 },
+	{ 0x3a02, 0x07 },
+	{ 0x3a03, 0xb0 },
+	{ 0x3a08, 0x01 },
+	{ 0x3a09, 0x27 },
+	{ 0x3a0a, 0x00 },
+	{ 0x3a0b, 0xf6 },
+	{ 0x3a0e, 0x06 },
+	{ 0x3a0d, 0x08 },
+	{ 0x3a14, 0x07 },
+	{ 0x3a15, 0xb0 },
+	{ 0x3a18, 0x01 },
+	{ 0x4004, 0x06 },
+	{ 0x4005, 0x18 },
+	{ 0x4300, 0x32 },
+	{ 0x4837, 0x0b },
+	{ 0x4202, 0x00 }
+};
+
+static struct ov5645_mode_info ov5645_mode_info_data[OV5645_MODE_MAX + 1] = {
+	{
+		.mode = OV5645_MODE_SXGA,
+		.width = 1280,
+		.height = 960,
+		.data = ov5645_setting_sxga,
+		.data_size = ARRAY_SIZE(ov5645_setting_sxga)
+	},
+	{
+		.mode = OV5645_MODE_1080P,
+		.width = 1920,
+		.height = 1080,
+		.data = ov5645_setting_1080p,
+		.data_size = ARRAY_SIZE(ov5645_setting_1080p)
+	},
+	{
+		.mode = OV5645_MODE_FULL,
+		.width = 2592,
+		.height = 1944,
+		.data = ov5645_setting_full,
+		.data_size = ARRAY_SIZE(ov5645_setting_full)
+	},
+};
+
+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;
+	}
+
+	return 0;
+
+err_disable_core:
+	regulator_disable(ov5645->core_regulator);
+err_disable_io:
+	regulator_disable(ov5645->io_regulator);
+
+	return ret;
+}
+
+static void ov5645_regulators_disable(struct ov5645 *ov5645)
+{
+	int ret;
+
+	ret = regulator_disable(ov5645->analog_regulator);
+	if (ret < 0)
+		dev_err(ov5645->dev, "analog regulator disable failed\n");
+
+	ret = regulator_disable(ov5645->core_regulator);
+	if (ret < 0)
+		dev_err(ov5645->dev, "core regulator disable failed\n");
+
+	ret = regulator_disable(ov5645->io_regulator);
+	if (ret < 0)
+		dev_err(ov5645->dev, "io regulator disable failed\n");
+}
+
+static int ov5645_write_reg(struct ov5645 *ov5645, u16 reg, u8 val)
+{
+	u8 regbuf[3];
+	int ret;
+
+	regbuf[0] = reg >> 8;
+	regbuf[1] = reg & 0xff;
+	regbuf[2] = val;
+
+	ret = i2c_master_send(ov5645->i2c_client, regbuf, 3);
+	if (ret < 0)
+		dev_err(ov5645->dev, "%s: write reg error %d: reg=%x, val=%x\n",
+			__func__, ret, reg, val);
+
+	return ret;
+}
+
+static int ov5645_read_reg(struct ov5645 *ov5645, u16 reg, u8 *val)
+{
+	u8 regbuf[2];
+	u8 tmpval;
+	int ret;
+
+	regbuf[0] = reg >> 8;
+	regbuf[1] = reg & 0xff;
+
+	ret = i2c_master_send(ov5645->i2c_client, regbuf, 2);
+	if (ret < 0) {
+		dev_err(ov5645->dev, "%s: write reg error %d: reg=%x\n",
+			__func__, ret, reg);
+		return ret;
+	}
+
+	ret = i2c_master_recv(ov5645->i2c_client, &tmpval, 1);
+	if (ret < 0) {
+		dev_err(ov5645->dev, "%s: read reg error %d: reg=%x\n",
+			__func__, ret, reg);
+		return ret;
+	}
+
+	*val = tmpval;
+
+	return 0;
+}
+
+static int ov5645_set_aec_mode(struct ov5645 *ov5645, u32 mode)
+{
+	u8 val;
+	int ret;
+
+	ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL, &val);
+	if (ret < 0)
+		return ret;
+
+	if (mode == V4L2_EXPOSURE_AUTO)
+		val &= ~OV5645_AEC_MANUAL_ENABLE;
+	else /* V4L2_EXPOSURE_MANUAL */
+		val |= OV5645_AEC_MANUAL_ENABLE;
+
+	return ov5645_write_reg(ov5645, OV5645_AEC_PK_MANUAL, val);
+}
+
+static int ov5645_set_agc_mode(struct ov5645 *ov5645, u32 enable)
+{
+	u8 val;
+	int ret;
+
+	ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL, &val);
+	if (ret < 0)
+		return ret;
+
+	if (enable)
+		val &= ~OV5645_AGC_MANUAL_ENABLE;
+	else
+		val |= OV5645_AGC_MANUAL_ENABLE;
+
+	return ov5645_write_reg(ov5645, OV5645_AEC_PK_MANUAL, val);
+}
+
+static int ov5645_set_register_array(struct ov5645 *ov5645,
+				     struct reg_value *settings,
+				     u32 num_settings)
+{
+	u16 reg;
+	u8 val;
+	u32 i;
+	int ret;
+
+	for (i = 0; i < num_settings; ++i, ++settings) {
+		reg = settings->reg;
+		val = settings->val;
+
+		ret = ov5645_write_reg(ov5645, reg, val);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ov5645_init(struct ov5645 *ov5645)
+{
+	struct reg_value *settings;
+	u32 num_settings;
+
+	settings = ov5645_global_init_setting;
+	num_settings = ARRAY_SIZE(ov5645_global_init_setting);
+
+	return ov5645_set_register_array(ov5645, settings, num_settings);
+}
+
+static int ov5645_change_mode(struct ov5645 *ov5645, enum ov5645_mode mode)
+{
+	struct reg_value *settings;
+	u32 num_settings;
+
+	settings = ov5645_mode_info_data[mode].data;
+	num_settings = ov5645_mode_info_data[mode].data_size;
+
+	return ov5645_set_register_array(ov5645, settings, num_settings);
+}
+
+static int ov5645_set_power_on(struct ov5645 *ov5645)
+{
+	int ret;
+
+	clk_set_rate(ov5645->xclk, ov5645->xclk_freq);
+
+	ret = clk_prepare_enable(ov5645->xclk);
+	if (ret < 0) {
+		dev_err(ov5645->dev, "clk prepare enable failed\n");
+		return ret;
+	}
+
+	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;
+}
+
+static void ov5645_set_power_off(struct ov5645 *ov5645)
+{
+	gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
+	gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
+	ov5645_regulators_disable(ov5645);
+	clk_disable_unprepare(ov5645->xclk);
+}
+
+static int ov5645_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct ov5645 *ov5645 = to_ov5645(sd);
+	int ret = 0;
+
+	mutex_lock(&ov5645->power_lock);
+
+	if (ov5645->power == !on) {
+		/* Power state changes. */
+		if (on) {
+			ret = ov5645_set_power_on(ov5645);
+			if (ret < 0) {
+				dev_err(ov5645->dev, "could not set power %s\n",
+					on ? "on" : "off");
+				goto exit;
+			}
+
+			ret = ov5645_init(ov5645);
+			if (ret < 0) {
+				dev_err(ov5645->dev,
+					"could not set init registers\n");
+				ov5645_set_power_off(ov5645);
+				goto exit;
+			}
+
+			ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
+					       OV5645_SYSTEM_CTRL0_STOP);
+			if (ret < 0) {
+				ov5645_set_power_off(ov5645);
+				goto exit;
+			}
+		} else {
+			ov5645_set_power_off(ov5645);
+		}
+
+		/* Update the power state. */
+		ov5645->power = on ? true : false;
+	}
+
+exit:
+	mutex_unlock(&ov5645->power_lock);
+
+	return ret;
+}
+
+
+static int ov5645_set_saturation(struct ov5645 *ov5645, s32 value)
+{
+	u32 reg_value = (value * 0x10) + 0x40;
+	int ret;
+
+	ret = ov5645_write_reg(ov5645, OV5645_SDE_SAT_U, reg_value);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5645_write_reg(ov5645, OV5645_SDE_SAT_V, reg_value);
+
+	return ret;
+}
+
+static int ov5645_set_hflip(struct ov5645 *ov5645, s32 value)
+{
+	u8 val;
+	int ret;
+
+	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21, &val);
+	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);
+}
+
+static int ov5645_set_vflip(struct ov5645 *ov5645, s32 value)
+{
+	u8 val;
+	int ret;
+
+	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20, &val);
+	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);
+}
+
+static int ov5645_set_test_pattern(struct ov5645 *ov5645, s32 value)
+{
+	u8 val;
+	int ret;
+
+	ret = ov5645_read_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, &val);
+	if (ret < 0)
+		return ret;
+
+	if (value) {
+		val &= ~OV5645_SET_TEST_PATTERN(OV5645_TEST_PATTERN_MASK);
+		val |= OV5645_SET_TEST_PATTERN(value - 1);
+		val |= OV5645_TEST_PATTERN_ENABLE;
+	} else {
+		val &= ~OV5645_TEST_PATTERN_ENABLE;
+	}
+
+	return ov5645_write_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, val);
+}
+
+static const char * const ov5645_test_pattern_menu[] = {
+	"Disabled",
+	"Vertical Color Bars",
+	"Pseudo-Random Data",
+	"Color Square",
+	"Black Image",
+};
+
+static int ov5645_set_awb(struct ov5645 *ov5645, s32 enable_auto)
+{
+	u8 val;
+	int ret;
+
+	ret = ov5645_read_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, &val);
+	if (ret < 0)
+		return ret;
+
+	if (enable_auto)
+		val &= ~OV5645_AWB_MANUAL_ENABLE;
+	else
+		val |= OV5645_AWB_MANUAL_ENABLE;
+
+	return ov5645_write_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, val);
+}
+
+static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct ov5645 *ov5645 = container_of(ctrl->handler,
+					     struct ov5645, ctrls);
+	int ret = -EINVAL;
+
+	mutex_lock(&ov5645->power_lock);
+	if (ov5645->power == 0) {
+		mutex_unlock(&ov5645->power_lock);
+		return 0;
+	}
+
+	switch (ctrl->id) {
+	case V4L2_CID_SATURATION:
+		ret = ov5645_set_saturation(ov5645, ctrl->val);
+		break;
+	case V4L2_CID_AUTO_WHITE_BALANCE:
+		ret = ov5645_set_awb(ov5645, ctrl->val);
+		break;
+	case V4L2_CID_AUTOGAIN:
+		ret = ov5645_set_agc_mode(ov5645, ctrl->val);
+		break;
+	case V4L2_CID_EXPOSURE_AUTO:
+		ret = ov5645_set_aec_mode(ov5645, ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN:
+		ret = ov5645_set_test_pattern(ov5645, ctrl->val);
+		break;
+	case V4L2_CID_HFLIP:
+		ret = ov5645_set_hflip(ov5645, ctrl->val);
+		break;
+	case V4L2_CID_VFLIP:
+		ret = ov5645_set_vflip(ov5645, ctrl->val);
+		break;
+	}
+
+	mutex_unlock(&ov5645->power_lock);
+
+	return ret;
+}
+
+static struct v4l2_ctrl_ops ov5645_ctrl_ops = {
+	.s_ctrl = ov5645_s_ctrl,
+};
+
+static int ov5645_entity_init_cfg(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_pad_config *cfg)
+{
+	struct v4l2_subdev_format fmt = { 0 };
+
+	fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
+	fmt.format.width = 1920;
+	fmt.format.height = 1080;
+
+	v4l2_subdev_call(subdev, pad, set_fmt, cfg, &fmt);
+
+	return 0;
+}
+
+static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct ov5645 *ov5645 = to_ov5645(sd);
+
+	if (code->index > 0)
+		return -EINVAL;
+
+	code->code = ov5645->fmt.code;
+
+	return 0;
+}
+
+static int ov5645_enum_frame_size(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_pad_config *cfg,
+				  struct v4l2_subdev_frame_size_enum *fse)
+{
+	if (fse->index > OV5645_MODE_MAX)
+		return -EINVAL;
+
+	fse->min_width = ov5645_mode_info_data[fse->index].width;
+	fse->max_width = ov5645_mode_info_data[fse->index].width;
+	fse->min_height = ov5645_mode_info_data[fse->index].height;
+	fse->max_height = ov5645_mode_info_data[fse->index].height;
+
+	return 0;
+}
+
+static struct v4l2_mbus_framefmt *
+__ov5645_get_pad_format(struct ov5645 *ov5645,
+			struct v4l2_subdev_pad_config *cfg,
+			unsigned int pad,
+			enum v4l2_subdev_format_whence which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_format(&ov5645->sd, cfg, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &ov5645->fmt;
+	default:
+		return NULL;
+	}
+}
+
+static int ov5645_get_format(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_pad_config *cfg,
+			     struct v4l2_subdev_format *format)
+{
+	struct ov5645 *ov5645 = to_ov5645(sd);
+
+	format->format = *__ov5645_get_pad_format(ov5645, cfg, format->pad,
+						  format->which);
+	return 0;
+}
+
+static struct v4l2_rect *
+__ov5645_get_pad_crop(struct ov5645 *ov5645, struct v4l2_subdev_pad_config *cfg,
+		      unsigned int pad, enum v4l2_subdev_format_whence which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_crop(&ov5645->sd, cfg, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &ov5645->crop;
+	default:
+		return NULL;
+	}
+}
+
+static enum ov5645_mode ov5645_find_nearest_mode(struct ov5645 *ov5645,
+						 int width, int height)
+{
+	int i;
+
+	for (i = OV5645_MODE_MAX; i >= 0; i--) {
+		if (ov5645_mode_info_data[i].width <= width &&
+		    ov5645_mode_info_data[i].height <= height)
+			break;
+	}
+
+	if (i < 0)
+		i = 0;
+
+	return (enum ov5645_mode)i;
+}
+
+static int ov5645_set_format(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_pad_config *cfg,
+			     struct v4l2_subdev_format *format)
+{
+	struct ov5645 *ov5645 = to_ov5645(sd);
+	struct v4l2_mbus_framefmt *__format;
+	struct v4l2_rect *__crop;
+	enum ov5645_mode new_mode;
+
+	__crop = __ov5645_get_pad_crop(ov5645, cfg, format->pad,
+			format->which);
+
+	new_mode = ov5645_find_nearest_mode(ov5645,
+			format->format.width, format->format.height);
+	__crop->width = ov5645_mode_info_data[new_mode].width;
+	__crop->height = ov5645_mode_info_data[new_mode].height;
+
+	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		ov5645->current_mode = new_mode;
+
+	__format = __ov5645_get_pad_format(ov5645, cfg, format->pad,
+			format->which);
+	__format->width = __crop->width;
+	__format->height = __crop->height;
+	__format->code = MEDIA_BUS_FMT_UYVY8_2X8;
+	__format->field = V4L2_FIELD_NONE;
+	__format->colorspace = V4L2_COLORSPACE_SRGB;
+
+	format->format = *__format;
+
+	return 0;
+}
+
+static int ov5645_get_selection(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_pad_config *cfg,
+			   struct v4l2_subdev_selection *sel)
+{
+	struct ov5645 *ov5645 = to_ov5645(sd);
+
+	if (sel->target != V4L2_SEL_TGT_CROP)
+		return -EINVAL;
+
+	sel->r = *__ov5645_get_pad_crop(ov5645, cfg, sel->pad,
+					sel->which);
+	return 0;
+}
+
+static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
+{
+	struct ov5645 *ov5645 = to_ov5645(subdev);
+	int ret;
+
+	if (enable) {
+		ret = ov5645_change_mode(ov5645, ov5645->current_mode);
+		if (ret < 0) {
+			dev_err(ov5645->dev, "could not set mode %d\n",
+				ov5645->current_mode);
+			return ret;
+		}
+		ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
+		if (ret < 0) {
+			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
+			return ret;
+		}
+		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
+				       OV5645_SYSTEM_CTRL0_START);
+		if (ret < 0)
+			return ret;
+	} else {
+		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
+				       OV5645_SYSTEM_CTRL0_STOP);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static struct v4l2_subdev_core_ops ov5645_core_ops = {
+	.s_power = ov5645_s_power,
+};
+
+static struct v4l2_subdev_video_ops ov5645_video_ops = {
+	.s_stream = ov5645_s_stream,
+};
+
+static struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
+	.init_cfg = ov5645_entity_init_cfg,
+	.enum_mbus_code = ov5645_enum_mbus_code,
+	.enum_frame_size = ov5645_enum_frame_size,
+	.get_fmt = ov5645_get_format,
+	.set_fmt = ov5645_set_format,
+	.get_selection = ov5645_get_selection,
+};
+
+static struct v4l2_subdev_ops ov5645_subdev_ops = {
+	.core = &ov5645_core_ops,
+	.video = &ov5645_video_ops,
+	.pad = &ov5645_subdev_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops ov5645_subdev_internal_ops = {
+};
+
+static int ov5645_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct device_node *endpoint;
+	struct ov5645 *ov5645;
+	u8 chip_id_high, chip_id_low;
+	int ret;
+
+	ov5645 = devm_kzalloc(dev, sizeof(struct ov5645), GFP_KERNEL);
+	if (!ov5645)
+		return -ENOMEM;
+
+	ov5645->i2c_client = client;
+	ov5645->dev = dev;
+
+	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
+	if (!endpoint) {
+		dev_err(dev, "endpoint node not found\n");
+		return -EINVAL;
+	}
+
+	ret = v4l2_of_parse_endpoint(endpoint, &ov5645->ep);
+	if (ret < 0) {
+		dev_err(dev, "parsing endpoint node failed\n");
+		return ret;
+	}
+	if (ov5645->ep.bus_type != V4L2_MBUS_CSI2) {
+		dev_err(dev, "invalid bus type, must be CSI2\n");
+		of_node_put(endpoint);
+		return -EINVAL;
+	}
+	of_node_put(endpoint);
+
+	/* get system clock (xclk) */
+	ov5645->xclk = devm_clk_get(dev, "xclk");
+	if (IS_ERR(ov5645->xclk)) {
+		dev_err(dev, "could not get xclk");
+		return PTR_ERR(ov5645->xclk);
+	}
+
+	ret = of_property_read_u32(dev->of_node, "clock-frequency",
+				    &ov5645->xclk_freq);
+	if (ret) {
+		dev_err(dev, "could not get xclk frequency\n");
+		return ret;
+	}
+
+	ov5645->io_regulator = devm_regulator_get(dev, "vdddo");
+	if (IS_ERR(ov5645->io_regulator)) {
+		dev_err(dev, "cannot get io regulator\n");
+		return PTR_ERR(ov5645->io_regulator);
+	}
+
+	ret = regulator_set_voltage(ov5645->io_regulator,
+				    OV5645_VOLTAGE_DIGITAL_IO,
+				    OV5645_VOLTAGE_DIGITAL_IO);
+	if (ret < 0) {
+		dev_err(dev, "cannot set io voltage\n");
+		return ret;
+	}
+
+	ov5645->core_regulator = devm_regulator_get(dev, "vddd");
+	if (IS_ERR(ov5645->core_regulator)) {
+		dev_err(dev, "cannot get core regulator\n");
+		return PTR_ERR(ov5645->core_regulator);
+	}
+
+	ret = regulator_set_voltage(ov5645->core_regulator,
+				    OV5645_VOLTAGE_DIGITAL_CORE,
+				    OV5645_VOLTAGE_DIGITAL_CORE);
+	if (ret < 0) {
+		dev_err(dev, "cannot set core voltage\n");
+		return ret;
+	}
+
+	ov5645->analog_regulator = devm_regulator_get(dev, "vdda");
+	if (IS_ERR(ov5645->analog_regulator)) {
+		dev_err(dev, "cannot get analog regulator\n");
+		return PTR_ERR(ov5645->analog_regulator);
+	}
+
+	ret = regulator_set_voltage(ov5645->analog_regulator,
+				    OV5645_VOLTAGE_ANALOG,
+				    OV5645_VOLTAGE_ANALOG);
+	if (ret < 0) {
+		dev_err(dev, "cannot set analog voltage\n");
+		return ret;
+	}
+
+	ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
+	if (IS_ERR(ov5645->enable_gpio)) {
+		dev_err(dev, "cannot get enable gpio\n");
+		return PTR_ERR(ov5645->enable_gpio);
+	}
+
+	ov5645->rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ov5645->rst_gpio)) {
+		dev_err(dev, "cannot get reset gpio\n");
+		return PTR_ERR(ov5645->rst_gpio);
+	}
+
+	mutex_init(&ov5645->power_lock);
+
+	v4l2_ctrl_handler_init(&ov5645->ctrls, 7);
+	ov5645->saturation = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
+				V4L2_CID_SATURATION, -4, 4, 1, 0);
+	ov5645->hflip = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
+				V4L2_CID_HFLIP, 0, 1, 1, 0);
+	ov5645->vflip = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
+				V4L2_CID_VFLIP, 0, 1, 1, 0);
+	ov5645->autogain = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
+				V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
+	ov5645->autoexposure = v4l2_ctrl_new_std_menu(&ov5645->ctrls,
+				&ov5645_ctrl_ops, V4L2_CID_EXPOSURE_AUTO,
+				V4L2_EXPOSURE_MANUAL, 0, V4L2_EXPOSURE_AUTO);
+	ov5645->awb = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
+				V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
+	ov5645->pattern = v4l2_ctrl_new_std_menu_items(&ov5645->ctrls,
+				&ov5645_ctrl_ops, V4L2_CID_TEST_PATTERN,
+				ARRAY_SIZE(ov5645_test_pattern_menu) - 1, 0, 0,
+				ov5645_test_pattern_menu);
+
+	ov5645->sd.ctrl_handler = &ov5645->ctrls;
+
+	if (ov5645->ctrls.error) {
+		dev_err(dev, "%s: control initialization error %d\n",
+		       __func__, ov5645->ctrls.error);
+		ret = ov5645->ctrls.error;
+		goto free_ctrl;
+	}
+
+	v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
+	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ov5645->sd.internal_ops = &ov5645_subdev_internal_ops;
+
+	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
+	if (ret < 0) {
+		dev_err(dev, "could not register media entity\n");
+		goto free_ctrl;
+	}
+
+	ov5645->sd.dev = &client->dev;
+	ret = v4l2_async_register_subdev(&ov5645->sd);
+	if (ret < 0) {
+		dev_err(dev, "could not register v4l2 device\n");
+		goto free_entity;
+	}
+
+	ret = ov5645_s_power(&ov5645->sd, true);
+	if (ret < 0) {
+		dev_err(dev, "could not power up OV5645\n");
+		goto unregister_subdev;
+	}
+
+	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
+	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
+		dev_err(dev, "could not read ID high\n");
+		ret = -ENODEV;
+		goto power_down;
+	}
+	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low);
+	if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
+		dev_err(dev, "could not read ID low\n");
+		ret = -ENODEV;
+		goto power_down;
+	}
+
+	dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
+
+	ov5645_s_power(&ov5645->sd, false);
+
+	return 0;
+
+power_down:
+	ov5645_s_power(&ov5645->sd, false);
+unregister_subdev:
+	v4l2_async_unregister_subdev(&ov5645->sd);
+free_entity:
+	media_entity_cleanup(&ov5645->sd.entity);
+free_ctrl:
+	v4l2_ctrl_handler_free(&ov5645->ctrls);
+	mutex_destroy(&ov5645->power_lock);
+
+	return ret;
+}
+
+
+static int ov5645_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov5645 *ov5645 = to_ov5645(sd);
+
+	v4l2_async_unregister_subdev(&ov5645->sd);
+	media_entity_cleanup(&ov5645->sd.entity);
+	v4l2_ctrl_handler_free(&ov5645->ctrls);
+	mutex_destroy(&ov5645->power_lock);
+
+	return 0;
+}
+
+
+static const struct i2c_device_id ov5645_id[] = {
+	{ "ov5645", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ov5645_id);
+
+static const struct of_device_id ov5645_of_match[] = {
+	{ .compatible = "ovti,ov5645" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ov5645_of_match);
+
+static struct i2c_driver ov5645_i2c_driver = {
+	.driver = {
+		.of_match_table = of_match_ptr(ov5645_of_match),
+		.name  = "ov5645",
+	},
+	.probe  = ov5645_probe,
+	.remove = ov5645_remove,
+	.id_table = ov5645_id,
+};
+
+module_i2c_driver(ov5645_i2c_driver);
+
+MODULE_DESCRIPTION("Omnivision OV5645 Camera Driver");
+MODULE_AUTHOR("Todor Tomov <todor.tomov@linaro.org>");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

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 <todor.tomov@linaro.org>
> ---
>  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 0000000..5e5c37e
> --- /dev/null
> +++ b/drivers/media/i2c/ov5645.c
> @@ -0,0 +1,1372 @@

[snip]

> +#define OV5645_VOLTAGE_ANALOG               2800000
> +#define OV5645_VOLTAGE_DIGITAL_CORE         1500000
> +#define OV5645_VOLTAGE_DIGITAL_IO           1800000
> +
> +#define OV5645_SYSTEM_CTRL0		0x3008
> +#define		OV5645_SYSTEM_CTRL0_START	0x02
> +#define		OV5645_SYSTEM_CTRL0_STOP	0x42
> +#define OV5645_CHIP_ID_HIGH		0x300A
> +#define		OV5645_CHIP_ID_HIGH_BYTE	0x56
> +#define OV5645_CHIP_ID_LOW		0x300B
> +#define		OV5645_CHIP_ID_LOW_BYTE		0x45
> +#define OV5645_AWB_MANUAL_CONTROL	0x3406
> +#define		OV5645_AWB_MANUAL_ENABLE	BIT(0)
> +#define OV5645_AEC_PK_MANUAL		0x3503
> +#define		OV5645_AEC_MANUAL_ENABLE	BIT(0)
> +#define		OV5645_AGC_MANUAL_ENABLE	BIT(1)
> +#define OV5645_TIMING_TC_REG20		0x3820
> +#define		OV5645_SENSOR_VFLIP		BIT(1)
> +#define		OV5645_ISP_VFLIP		BIT(2)
> +#define OV5645_TIMING_TC_REG21		0x3821
> +#define		OV5645_SENSOR_MIRROR		BIT(1)
> +#define OV5645_PRE_ISP_TEST_SETTING_1	0x503d

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_MASK	0x3
> +#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 23880000Hz */
> +	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 completely and use an unsigned int array index instead.

> { +	{
> +		.mode = OV5645_MODE_SXGA,
> +		.width = 1280,
> +		.height = 960,
> +		.data = ov5645_setting_sxga,
> +		.data_size = ARRAY_SIZE(ov5645_setting_sxga)
> +	},
> +	{
> +		.mode = OV5645_MODE_1080P,
> +		.width = 1920,
> +		.height = 1080,
> +		.data = ov5645_setting_1080p,
> +		.data_size = ARRAY_SIZE(ov5645_setting_1080p)
> +	},
> +	{
> +		.mode = OV5645_MODE_FULL,
> +		.width = 2592,
> +		.height = 1944,
> +		.data = ov5645_setting_full,
> +		.data_size = ARRAY_SIZE(ov5645_setting_full)
> +	},
> +};
> +
> +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.

> +	return 0;
> +
> +err_disable_core:
> +	regulator_disable(ov5645->core_regulator);
> +err_disable_io:
> +	regulator_disable(ov5645->io_regulator);
> +
> +	return ret;
> +}
> +
> +static void ov5645_regulators_disable(struct ov5645 *ov5645)
> +{
> +	int ret;
> +
> +	ret = regulator_disable(ov5645->analog_regulator);
> +	if (ret < 0)
> +		dev_err(ov5645->dev, "analog regulator disable failed\n");
> +
> +	ret = regulator_disable(ov5645->core_regulator);
> +	if (ret < 0)
> +		dev_err(ov5645->dev, "core regulator disable failed\n");
> +
> +	ret = regulator_disable(ov5645->io_regulator);
> +	if (ret < 0)
> +		dev_err(ov5645->dev, "io regulator disable failed\n");
> +}
> +
> +static int ov5645_write_reg(struct ov5645 *ov5645, u16 reg, u8 val)
> +{
> +	u8 regbuf[3];
> +	int ret;
> +
> +	regbuf[0] = reg >> 8;
> +	regbuf[1] = reg & 0xff;
> +	regbuf[2] = val;
> +
> +	ret = i2c_master_send(ov5645->i2c_client, regbuf, 3);
> +	if (ret < 0)
> +		dev_err(ov5645->dev, "%s: write reg error %d: reg=%x, 
val=%x\n",
> +			__func__, ret, reg, val);
> +
> +	return ret;
> +}
> +
> +static int ov5645_read_reg(struct ov5645 *ov5645, u16 reg, u8 *val)
> +{
> +	u8 regbuf[2];
> +	u8 tmpval;
> +	int ret;
> +
> +	regbuf[0] = reg >> 8;
> +	regbuf[1] = reg & 0xff;
> +
> +	ret = i2c_master_send(ov5645->i2c_client, regbuf, 2);
> +	if (ret < 0) {
> +		dev_err(ov5645->dev, "%s: write reg error %d: reg=%x\n",
> +			__func__, ret, reg);
> +		return ret;
> +	}
> +
> +	ret = i2c_master_recv(ov5645->i2c_client, &tmpval, 1);

You can use val directly here and remove the tmpval local variable.

> +	if (ret < 0) {
> +		dev_err(ov5645->dev, "%s: read reg error %d: reg=%x\n",
> +			__func__, ret, reg);
> +		return ret;
> +	}
> +
> +	*val = tmpval;
> +
> +	return 0;
> +}
> +
> +static int ov5645_set_aec_mode(struct ov5645 *ov5645, u32 mode)
> +{
> +	u8 val;
> +	int ret;
> +
> +	ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (mode == V4L2_EXPOSURE_AUTO)
> +		val &= ~OV5645_AEC_MANUAL_ENABLE;
> +	else /* V4L2_EXPOSURE_MANUAL */
> +		val |= OV5645_AEC_MANUAL_ENABLE;
> +
> +	return ov5645_write_reg(ov5645, OV5645_AEC_PK_MANUAL, val);

I2C read operations being expensive, how about caching the 
OV5645_AEC_PK_MANUAL value in struct ov5645 to avoid reading it back from the 
hardware ?

> +}
> +
> +static int ov5645_set_agc_mode(struct ov5645 *ov5645, u32 enable)
> +{
> +	u8 val;
> +	int ret;
> +
> +	ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (enable)
> +		val &= ~OV5645_AGC_MANUAL_ENABLE;
> +	else
> +		val |= OV5645_AGC_MANUAL_ENABLE;
> +
> +	return ov5645_write_reg(ov5645, OV5645_AEC_PK_MANUAL, val);
> +}
> +
> +static int ov5645_set_register_array(struct ov5645 *ov5645,
> +				     struct reg_value *settings,

settings is never modified, you can make it const.

> +				     u32 num_settings)

I'd use unsigned int (same for the i variable below) as you don't really care 
about the exact number of bits.

> +{
> +	u16 reg;
> +	u8 val;
> +	u32 i;
> +	int ret;
> +
> +	for (i = 0; i < num_settings; ++i, ++settings) {
> +		reg = settings->reg;
> +		val = settings->val;
> +
> +		ret = ov5645_write_reg(ov5645, reg, val);

I'd write this

		ret = ov5645_write_reg(ov5645, settings->reg, settins->val);

and remove the reg and val local variables, they don't really improve 
readability in my opinion.

> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ov5645_init(struct ov5645 *ov5645)
> +{
> +	struct reg_value *settings;
> +	u32 num_settings;
> +
> +	settings = ov5645_global_init_setting;
> +	num_settings = ARRAY_SIZE(ov5645_global_init_setting);
> +
> +	return ov5645_set_register_array(ov5645, settings, num_settings);

I'd drop the local variables here too.

> +}
> +
> +static int ov5645_change_mode(struct ov5645 *ov5645, enum ov5645_mode mode)
> +{
> +	struct reg_value *settings;
> +	u32 num_settings;
> +
> +	settings = ov5645_mode_info_data[mode].data;
> +	num_settings = ov5645_mode_info_data[mode].data_size;
> +
> +	return ov5645_set_register_array(ov5645, settings, num_settings);

And here too.

> +}
> +
> +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 ?

> +	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.

> +	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.

> +}
> +
> +static void ov5645_set_power_off(struct ov5645 *ov5645)
> +{
> +	gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
> +	gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
> +	ov5645_regulators_disable(ov5645);
> +	clk_disable_unprepare(ov5645->xclk);
> +}
> +
> +static int ov5645_s_power(struct v4l2_subdev *sd, int on)
> +{
> +	struct ov5645 *ov5645 = to_ov5645(sd);
> +	int ret = 0;
> +
> +	mutex_lock(&ov5645->power_lock);
> +
> +	if (ov5645->power == !on) {

Power handling needs to be reference-counted, see the mt0p031 driver for an 
example.

> +		/* Power state changes. */
> +		if (on) {
> +			ret = ov5645_set_power_on(ov5645);
> +			if (ret < 0) {
> +				dev_err(ov5645->dev, "could not set power 
%s\n",
> +					on ? "on" : "off");

on is alway true here, you can hardcode the debug message.

Furthermore I think ov5645_set_power_on() or the functions it calls already 
print an error message on failure, so you could drop this one.

> +				goto exit;
> +			}
> +
> +			ret = ov5645_init(ov5645);
> +			if (ret < 0) {
> +				dev_err(ov5645->dev,
> +					"could not set init registers\n");
> +				ov5645_set_power_off(ov5645);
> +				goto exit;
> +			}
> +
> +			ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> +					       OV5645_SYSTEM_CTRL0_STOP);
> +			if (ret < 0) {
> +				ov5645_set_power_off(ov5645);
> +				goto exit;
> +			}
> +		} else {
> +			ov5645_set_power_off(ov5645);
> +		}
> +
> +		/* Update the power state. */
> +		ov5645->power = on ? true : false;
> +	}
> +
> +exit:
> +	mutex_unlock(&ov5645->power_lock);
> +
> +	return ret;
> +}
> +
> +

Extra blank line.

> +static int ov5645_set_saturation(struct ov5645 *ov5645, s32 value)
> +{
> +	u32 reg_value = (value * 0x10) + 0x40;
> +	int ret;
> +
> +	ret = ov5645_write_reg(ov5645, OV5645_SDE_SAT_U, reg_value);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ov5645_write_reg(ov5645, OV5645_SDE_SAT_V, reg_value);
> +
> +	return ret;

return ov5645_write_reg(ov5645, OV5645_SDE_SAT_V, reg_value);

> +}
> +
> +static int ov5645_set_hflip(struct ov5645 *ov5645, s32 value)
> +{
> +	u8 val;
> +	int ret;
> +
> +	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21, &val);
> +	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.

> +}
> +
> +static int ov5645_set_vflip(struct ov5645 *ov5645, s32 value)
> +{
> +	u8 val;
> +	int ret;
> +
> +	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20, &val);
> +	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.

How about using regmap by the way ?

> +}
> +
> +static int ov5645_set_test_pattern(struct ov5645 *ov5645, s32 value)
> +{
> +	u8 val;
> +	int ret;
> +
> +	ret = ov5645_read_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (value) {
> +		val &= ~OV5645_SET_TEST_PATTERN(OV5645_TEST_PATTERN_MASK);
> +		val |= OV5645_SET_TEST_PATTERN(value - 1);
> +		val |= OV5645_TEST_PATTERN_ENABLE;
> +	} else {
> +		val &= ~OV5645_TEST_PATTERN_ENABLE;
> +	}
> +
> +	return ov5645_write_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, val);

Are there other bits that need to be preserved in this register ?

> +}
> +
> +static const char * const ov5645_test_pattern_menu[] = {
> +	"Disabled",
> +	"Vertical Color Bars",
> +	"Pseudo-Random Data",
> +	"Color Square",
> +	"Black Image",
> +};
> +
> +static int ov5645_set_awb(struct ov5645 *ov5645, s32 enable_auto)
> +{
> +	u8 val;
> +	int ret;
> +
> +	ret = ov5645_read_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (enable_auto)
> +		val &= ~OV5645_AWB_MANUAL_ENABLE;
> +	else
> +		val |= OV5645_AWB_MANUAL_ENABLE;
> +
> +	return ov5645_write_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, val);

Same here, are there other bits that need to be preserved ?

> +}
> +
> +static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct ov5645 *ov5645 = container_of(ctrl->handler,
> +					     struct ov5645, ctrls);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&ov5645->power_lock);
> +	if (ov5645->power == 0) {

power is a bool, I'd thus write this

	if (!ov5645->power) {

> +		mutex_unlock(&ov5645->power_lock);
> +		return 0;
> +	}
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_SATURATION:
> +		ret = ov5645_set_saturation(ov5645, ctrl->val);
> +		break;
> +	case V4L2_CID_AUTO_WHITE_BALANCE:
> +		ret = ov5645_set_awb(ov5645, ctrl->val);
> +		break;
> +	case V4L2_CID_AUTOGAIN:
> +		ret = ov5645_set_agc_mode(ov5645, ctrl->val);
> +		break;
> +	case V4L2_CID_EXPOSURE_AUTO:
> +		ret = ov5645_set_aec_mode(ov5645, ctrl->val);
> +		break;
> +	case V4L2_CID_TEST_PATTERN:
> +		ret = ov5645_set_test_pattern(ov5645, ctrl->val);
> +		break;
> +	case V4L2_CID_HFLIP:
> +		ret = ov5645_set_hflip(ov5645, ctrl->val);
> +		break;
> +	case V4L2_CID_VFLIP:
> +		ret = ov5645_set_vflip(ov5645, ctrl->val);
> +		break;

Instead of initializing ret to -EINVAL you could add a default case here, it 
would avoid the unnecessary initialization in the common case where ctrl->id 
is valid.

> +	}
> +
> +	mutex_unlock(&ov5645->power_lock);
> +
> +	return ret;
> +}
> +
> +static struct v4l2_ctrl_ops ov5645_ctrl_ops = {
> +	.s_ctrl = ov5645_s_ctrl,
> +};
> +
> +static int ov5645_entity_init_cfg(struct v4l2_subdev *subdev,
> +				  struct v4l2_subdev_pad_config *cfg)
> +{
> +	struct v4l2_subdev_format fmt = { 0 };
> +
> +	fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;

The function will always be called with cfg != NULL.

> +	fmt.format.width = 1920;
> +	fmt.format.height = 1080;
> +
> +	v4l2_subdev_call(subdev, pad, set_fmt, cfg, &fmt);

You can call ov5645_set_format directly.

> +	return 0;
> +}
> +
> +static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_pad_config *cfg,
> +				 struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	struct ov5645 *ov5645 = to_ov5645(sd);
> +
> +	if (code->index > 0)
> +		return -EINVAL;
> +
> +	code->code = ov5645->fmt.code;

Given that ov5645->fmt.code is always equal to MEDIA_BUS_FMT_UYVY8_2X8, you 
can hardcode the value here. Returning the current code works when only a 
single one is supported, but is conceptually the wrong thing to do in the code 
enumeration handler.

> +
> +	return 0;
> +}
> +
> +static int ov5645_enum_frame_size(struct v4l2_subdev *subdev,
> +				  struct v4l2_subdev_pad_config *cfg,
> +				  struct v4l2_subdev_frame_size_enum *fse)
> +{

You should return -EINVAL if fse->code != MEDIA_BUS_FMT_UYVY8_2X8.

> +	if (fse->index > OV5645_MODE_MAX)
> +		return -EINVAL;
> +
> +	fse->min_width = ov5645_mode_info_data[fse->index].width;
> +	fse->max_width = ov5645_mode_info_data[fse->index].width;
> +	fse->min_height = ov5645_mode_info_data[fse->index].height;
> +	fse->max_height = ov5645_mode_info_data[fse->index].height;
> +
> +	return 0;
> +}
> +
> +static struct v4l2_mbus_framefmt *
> +__ov5645_get_pad_format(struct ov5645 *ov5645,
> +			struct v4l2_subdev_pad_config *cfg,
> +			unsigned int pad,
> +			enum v4l2_subdev_format_whence which)
> +{
> +	switch (which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		return v4l2_subdev_get_try_format(&ov5645->sd, cfg, pad);
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		return &ov5645->fmt;
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +static int ov5645_get_format(struct v4l2_subdev *sd,
> +			     struct v4l2_subdev_pad_config *cfg,
> +			     struct v4l2_subdev_format *format)
> +{
> +	struct ov5645 *ov5645 = to_ov5645(sd);
> +
> +	format->format = *__ov5645_get_pad_format(ov5645, cfg, format->pad,
> +						  format->which);
> +	return 0;
> +}
> +
> +static struct v4l2_rect *
> +__ov5645_get_pad_crop(struct ov5645 *ov5645, struct v4l2_subdev_pad_config
> *cfg,
> +		      unsigned int pad, enum v4l2_subdev_format_whence which)
> +{
> +	switch (which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		return v4l2_subdev_get_try_crop(&ov5645->sd, cfg, pad);
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		return &ov5645->crop;
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +static enum ov5645_mode ov5645_find_nearest_mode(struct ov5645 *ov5645,
> +						 int width, int height)
> +{

width and height only takes positive values, you can make them unsigned int.

> +	int i;
> +
> +	for (i = OV5645_MODE_MAX; i >= 0; i--) {
> +		if (ov5645_mode_info_data[i].width <= width &&
> +		    ov5645_mode_info_data[i].height <= height)
> +			break;
> +	}
> +
> +	if (i < 0)
> +		i = 0;
> +
> +	return (enum ov5645_mode)i;
> +}
> +
> +static int ov5645_set_format(struct v4l2_subdev *sd,
> +			     struct v4l2_subdev_pad_config *cfg,
> +			     struct v4l2_subdev_format *format)
> +{
> +	struct ov5645 *ov5645 = to_ov5645(sd);
> +	struct v4l2_mbus_framefmt *__format;
> +	struct v4l2_rect *__crop;
> +	enum ov5645_mode new_mode;
> +
> +	__crop = __ov5645_get_pad_crop(ov5645, cfg, format->pad,
> +			format->which);
> +
> +	new_mode = ov5645_find_nearest_mode(ov5645,
> +			format->format.width, format->format.height);
> +	__crop->width = ov5645_mode_info_data[new_mode].width;
> +	__crop->height = ov5645_mode_info_data[new_mode].height;
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +		ov5645->current_mode = new_mode;
> +
> +	__format = __ov5645_get_pad_format(ov5645, cfg, format->pad,
> +			format->which);
> +	__format->width = __crop->width;
> +	__format->height = __crop->height;
> +	__format->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +	__format->field = V4L2_FIELD_NONE;
> +	__format->colorspace = V4L2_COLORSPACE_SRGB;
> +
> +	format->format = *__format;
> +
> +	return 0;
> +}
> +
> +static int ov5645_get_selection(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_pad_config *cfg,
> +			   struct v4l2_subdev_selection *sel)
> +{
> +	struct ov5645 *ov5645 = to_ov5645(sd);
> +
> +	if (sel->target != V4L2_SEL_TGT_CROP)
> +		return -EINVAL;
> +
> +	sel->r = *__ov5645_get_pad_crop(ov5645, cfg, sel->pad,
> +					sel->which);
> +	return 0;
> +}
> +
> +static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> +{
> +	struct ov5645 *ov5645 = to_ov5645(subdev);
> +	int ret;
> +
> +	if (enable) {
> +		ret = ov5645_change_mode(ov5645, ov5645->current_mode);
> +		if (ret < 0) {
> +			dev_err(ov5645->dev, "could not set mode %d\n",
> +				ov5645->current_mode);
> +			return ret;
> +		}
> +		ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
> +		if (ret < 0) {
> +			dev_err(ov5645->dev, "could not sync v4l2 
controls\n");
> +			return ret;
> +		}
> +		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> +				       OV5645_SYSTEM_CTRL0_START);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> +				       OV5645_SYSTEM_CTRL0_STOP);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct v4l2_subdev_core_ops ov5645_core_ops = {
> +	.s_power = ov5645_s_power,
> +};
> +
> +static struct v4l2_subdev_video_ops ov5645_video_ops = {
> +	.s_stream = ov5645_s_stream,
> +};
> +
> +static struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
> +	.init_cfg = ov5645_entity_init_cfg,
> +	.enum_mbus_code = ov5645_enum_mbus_code,
> +	.enum_frame_size = ov5645_enum_frame_size,
> +	.get_fmt = ov5645_get_format,
> +	.set_fmt = ov5645_set_format,
> +	.get_selection = ov5645_get_selection,
> +};
> +
> +static struct v4l2_subdev_ops ov5645_subdev_ops = {
> +	.core = &ov5645_core_ops,
> +	.video = &ov5645_video_ops,
> +	.pad = &ov5645_subdev_pad_ops,
> +};

You can make all those structures static const.

> +
> +static const struct v4l2_subdev_internal_ops ov5645_subdev_internal_ops = {
> +};
> +
> +static int ov5645_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct device_node *endpoint;
> +	struct ov5645 *ov5645;
> +	u8 chip_id_high, chip_id_low;
> +	int ret;
> +
> +	ov5645 = devm_kzalloc(dev, sizeof(struct ov5645), GFP_KERNEL);
> +	if (!ov5645)
> +		return -ENOMEM;
> +
> +	ov5645->i2c_client = client;
> +	ov5645->dev = dev;
> +
> +	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> +	if (!endpoint) {
> +		dev_err(dev, "endpoint node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = v4l2_of_parse_endpoint(endpoint, &ov5645->ep);
> +	if (ret < 0) {
> +		dev_err(dev, "parsing endpoint node failed\n");
> +		return ret;
> +	}

You can call of_node_put(endpoint) here instead of twice below.

> +	if (ov5645->ep.bus_type != V4L2_MBUS_CSI2) {
> +		dev_err(dev, "invalid bus type, must be CSI2\n");
> +		of_node_put(endpoint);
> +		return -EINVAL;
> +	}
> +	of_node_put(endpoint);
> +
> +	/* get system clock (xclk) */
> +	ov5645->xclk = devm_clk_get(dev, "xclk");
> +	if (IS_ERR(ov5645->xclk)) {
> +		dev_err(dev, "could not get xclk");
> +		return PTR_ERR(ov5645->xclk);
> +	}
> +
> +	ret = of_property_read_u32(dev->of_node, "clock-frequency",
> +				    &ov5645->xclk_freq);
> +	if (ret) {
> +		dev_err(dev, "could not get xclk frequency\n");
> +		return ret;
> +	}

Shouldn't you return an error if the frequency is different than the only one 
supported by the driver (for which the register values have been computed and 
hardcoded) ?

> +	ov5645->io_regulator = devm_regulator_get(dev, "vdddo");
> +	if (IS_ERR(ov5645->io_regulator)) {
> +		dev_err(dev, "cannot get io regulator\n");
> +		return PTR_ERR(ov5645->io_regulator);
> +	}
> +
> +	ret = regulator_set_voltage(ov5645->io_regulator,
> +				    OV5645_VOLTAGE_DIGITAL_IO,
> +				    OV5645_VOLTAGE_DIGITAL_IO);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot set io voltage\n");
> +		return ret;
> +	}
> +
> +	ov5645->core_regulator = devm_regulator_get(dev, "vddd");
> +	if (IS_ERR(ov5645->core_regulator)) {
> +		dev_err(dev, "cannot get core regulator\n");
> +		return PTR_ERR(ov5645->core_regulator);
> +	}
> +
> +	ret = regulator_set_voltage(ov5645->core_regulator,
> +				    OV5645_VOLTAGE_DIGITAL_CORE,
> +				    OV5645_VOLTAGE_DIGITAL_CORE);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot set core voltage\n");
> +		return ret;
> +	}
> +
> +	ov5645->analog_regulator = devm_regulator_get(dev, "vdda");
> +	if (IS_ERR(ov5645->analog_regulator)) {
> +		dev_err(dev, "cannot get analog regulator\n");
> +		return PTR_ERR(ov5645->analog_regulator);
> +	}
> +
> +	ret = regulator_set_voltage(ov5645->analog_regulator,
> +				    OV5645_VOLTAGE_ANALOG,
> +				    OV5645_VOLTAGE_ANALOG);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot set analog voltage\n");
> +		return ret;
> +	}
> +
> +	ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
> +	if (IS_ERR(ov5645->enable_gpio)) {
> +		dev_err(dev, "cannot get enable gpio\n");
> +		return PTR_ERR(ov5645->enable_gpio);
> +	}
> +
> +	ov5645->rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(ov5645->rst_gpio)) {
> +		dev_err(dev, "cannot get reset gpio\n");
> +		return PTR_ERR(ov5645->rst_gpio);
> +	}
> +
> +	mutex_init(&ov5645->power_lock);
> +
> +	v4l2_ctrl_handler_init(&ov5645->ctrls, 7);
> +	ov5645->saturation = v4l2_ctrl_new_std(&ov5645->ctrls, 
&ov5645_ctrl_ops,
> +				V4L2_CID_SATURATION, -4, 4, 1, 0);
> +	ov5645->hflip = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
> +				V4L2_CID_HFLIP, 0, 1, 1, 0);
> +	ov5645->vflip = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
> +				V4L2_CID_VFLIP, 0, 1, 1, 0);
> +	ov5645->autogain = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
> +				V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
> +	ov5645->autoexposure = v4l2_ctrl_new_std_menu(&ov5645->ctrls,
> +				&ov5645_ctrl_ops, V4L2_CID_EXPOSURE_AUTO,
> +				V4L2_EXPOSURE_MANUAL, 0, V4L2_EXPOSURE_AUTO);
> +	ov5645->awb = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
> +				V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
> +	ov5645->pattern = v4l2_ctrl_new_std_menu_items(&ov5645->ctrls,
> +				&ov5645_ctrl_ops, V4L2_CID_TEST_PATTERN,
> +				ARRAY_SIZE(ov5645_test_pattern_menu) - 1, 0, 
0,
> +				ov5645_test_pattern_menu);

You don't need to store all these in the ov5645 structure as they are never 
used.

> +
> +	ov5645->sd.ctrl_handler = &ov5645->ctrls;
> +
> +	if (ov5645->ctrls.error) {
> +		dev_err(dev, "%s: control initialization error %d\n",
> +		       __func__, ov5645->ctrls.error);
> +		ret = ov5645->ctrls.error;
> +		goto free_ctrl;
> +	}
> +
> +	v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
> +	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ov5645->sd.internal_ops = &ov5645_subdev_internal_ops;
> +
> +	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
> +	if (ret < 0) {
> +		dev_err(dev, "could not register media entity\n");
> +		goto free_ctrl;
> +	}
> +
> +	ov5645->sd.dev = &client->dev;
> +	ret = v4l2_async_register_subdev(&ov5645->sd);
> +	if (ret < 0) {
> +		dev_err(dev, "could not register v4l2 device\n");
> +		goto free_entity;
> +	}
> +
> +	ret = ov5645_s_power(&ov5645->sd, true);
> +	if (ret < 0) {
> +		dev_err(dev, "could not power up OV5645\n");
> +		goto unregister_subdev;
> +	}
> +
> +	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
> +	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
> +		dev_err(dev, "could not read ID high\n");
> +		ret = -ENODEV;
> +		goto power_down;
> +	}
> +	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low);
> +	if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
> +		dev_err(dev, "could not read ID low\n");
> +		ret = -ENODEV;
> +		goto power_down;
> +	}

I would do this before registering the subdev, as you don't want to make it 
available to the system if the ID doesn't match.

> +
> +	dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
> +
> +	ov5645_s_power(&ov5645->sd, false);
> +
> +	return 0;
> +
> +power_down:
> +	ov5645_s_power(&ov5645->sd, false);
> +unregister_subdev:
> +	v4l2_async_unregister_subdev(&ov5645->sd);
> +free_entity:
> +	media_entity_cleanup(&ov5645->sd.entity);
> +free_ctrl:
> +	v4l2_ctrl_handler_free(&ov5645->ctrls);
> +	mutex_destroy(&ov5645->power_lock);
> +
> +	return ret;
> +}
> +
> +
> +static int ov5645_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov5645 *ov5645 = to_ov5645(sd);
> +
> +	v4l2_async_unregister_subdev(&ov5645->sd);
> +	media_entity_cleanup(&ov5645->sd.entity);
> +	v4l2_ctrl_handler_free(&ov5645->ctrls);
> +	mutex_destroy(&ov5645->power_lock);
> +
> +	return 0;
> +}
> +
> +
> +static const struct i2c_device_id ov5645_id[] = {
> +	{ "ov5645", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ov5645_id);
> +
> +static const struct of_device_id ov5645_of_match[] = {
> +	{ .compatible = "ovti,ov5645" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov5645_of_match);
> +
> +static struct i2c_driver ov5645_i2c_driver = {
> +	.driver = {
> +		.of_match_table = of_match_ptr(ov5645_of_match),
> +		.name  = "ov5645",
> +	},
> +	.probe  = ov5645_probe,
> +	.remove = ov5645_remove,
> +	.id_table = ov5645_id,
> +};
> +
> +module_i2c_driver(ov5645_i2c_driver);
> +
> +MODULE_DESCRIPTION("Omnivision OV5645 Camera Driver");
> +MODULE_AUTHOR("Todor Tomov <todor.tomov@linaro.org>");
> +MODULE_LICENSE("GPL v2");

-- 
Regards,

Laurent Pinchart

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 <todor.tomov@linaro.org>
>> ---
>>  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 0000000..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, &val);
>> +	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, &val);
>> +	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 ov5645_set_test_pattern(struct ov5645 *ov5645, s32 value)
>> +{
>> +	u8 val;
>> +	int ret;
>> +
>> +	ret = ov5645_read_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (value) {
>> +		val &= ~OV5645_SET_TEST_PATTERN(OV5645_TEST_PATTERN_MASK);
>> +		val |= OV5645_SET_TEST_PATTERN(value - 1);
>> +		val |= OV5645_TEST_PATTERN_ENABLE;
>> +	} else {
>> +		val &= ~OV5645_TEST_PATTERN_ENABLE;
>> +	}
>> +
>> +	return ov5645_write_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, val);
> 
> Are there other bits that need to be preserved in this register ?
This driver is based on the driver for OV5645 from QC and the driver for OV5640
that was sent to linux-media. I cannot add additional functionality so I preserve
the rest of the bits. But I'll add caching in a variable here too.

> 
>> +}
>> +
>> +static const char * const ov5645_test_pattern_menu[] = {
>> +	"Disabled",
>> +	"Vertical Color Bars",
>> +	"Pseudo-Random Data",
>> +	"Color Square",
>> +	"Black Image",
>> +};
>> +
>> +static int ov5645_set_awb(struct ov5645 *ov5645, s32 enable_auto)
>> +{
>> +	u8 val;
>> +	int ret;
>> +
>> +	ret = ov5645_read_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (enable_auto)
>> +		val &= ~OV5645_AWB_MANUAL_ENABLE;
>> +	else
>> +		val |= OV5645_AWB_MANUAL_ENABLE;
>> +
>> +	return ov5645_write_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, val);
> 
> Same here, are there other bits that need to be preserved ?
Same as above.

> 
>> +}
>> +
>> +static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +	struct ov5645 *ov5645 = container_of(ctrl->handler,
>> +					     struct ov5645, ctrls);
>> +	int ret = -EINVAL;
>> +
>> +	mutex_lock(&ov5645->power_lock);
>> +	if (ov5645->power == 0) {
> 
> power is a bool, I'd thus write this
power will be int now (for ref-counting) but I'll write it like this.

> 
> 	if (!ov5645->power) {
> 
>> +		mutex_unlock(&ov5645->power_lock);
>> +		return 0;
>> +	}
>> +
>> +	switch (ctrl->id) {
>> +	case V4L2_CID_SATURATION:
>> +		ret = ov5645_set_saturation(ov5645, ctrl->val);
>> +		break;
>> +	case V4L2_CID_AUTO_WHITE_BALANCE:
>> +		ret = ov5645_set_awb(ov5645, ctrl->val);
>> +		break;
>> +	case V4L2_CID_AUTOGAIN:
>> +		ret = ov5645_set_agc_mode(ov5645, ctrl->val);
>> +		break;
>> +	case V4L2_CID_EXPOSURE_AUTO:
>> +		ret = ov5645_set_aec_mode(ov5645, ctrl->val);
>> +		break;
>> +	case V4L2_CID_TEST_PATTERN:
>> +		ret = ov5645_set_test_pattern(ov5645, ctrl->val);
>> +		break;
>> +	case V4L2_CID_HFLIP:
>> +		ret = ov5645_set_hflip(ov5645, ctrl->val);
>> +		break;
>> +	case V4L2_CID_VFLIP:
>> +		ret = ov5645_set_vflip(ov5645, ctrl->val);
>> +		break;
> 
> Instead of initializing ret to -EINVAL you could add a default case here, it 
> would avoid the unnecessary initialization in the common case where ctrl->id 
> is valid.
Ok.

> 
>> +	}
>> +
>> +	mutex_unlock(&ov5645->power_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static struct v4l2_ctrl_ops ov5645_ctrl_ops = {
>> +	.s_ctrl = ov5645_s_ctrl,
>> +};
>> +
>> +static int ov5645_entity_init_cfg(struct v4l2_subdev *subdev,
>> +				  struct v4l2_subdev_pad_config *cfg)
>> +{
>> +	struct v4l2_subdev_format fmt = { 0 };
>> +
>> +	fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> 
> The function will always be called with cfg != NULL.
I intend to call this function from probe to init the active format. Will this be ok?

> 
>> +	fmt.format.width = 1920;
>> +	fmt.format.height = 1080;
>> +
>> +	v4l2_subdev_call(subdev, pad, set_fmt, cfg, &fmt);
> 
> You can call ov5645_set_format directly.
Ok.

> 
>> +	return 0;
>> +}
>> +
>> +static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
>> +				 struct v4l2_subdev_pad_config *cfg,
>> +				 struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> +	struct ov5645 *ov5645 = to_ov5645(sd);
>> +
>> +	if (code->index > 0)
>> +		return -EINVAL;
>> +
>> +	code->code = ov5645->fmt.code;
> 
> Given that ov5645->fmt.code is always equal to MEDIA_BUS_FMT_UYVY8_2X8, you 
> can hardcode the value here. Returning the current code works when only a 
> single one is supported, but is conceptually the wrong thing to do in the code 
> enumeration handler.
Agree.

> 
>> +
>> +	return 0;
>> +}
>> +

[snip]


>> +
>> +static struct v4l2_subdev_core_ops ov5645_core_ops = {
>> +	.s_power = ov5645_s_power,
>> +};
>> +
>> +static struct v4l2_subdev_video_ops ov5645_video_ops = {
>> +	.s_stream = ov5645_s_stream,
>> +};
>> +
>> +static struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
>> +	.init_cfg = ov5645_entity_init_cfg,
>> +	.enum_mbus_code = ov5645_enum_mbus_code,
>> +	.enum_frame_size = ov5645_enum_frame_size,
>> +	.get_fmt = ov5645_get_format,
>> +	.set_fmt = ov5645_set_format,
>> +	.get_selection = ov5645_get_selection,
>> +};
>> +
>> +static struct v4l2_subdev_ops ov5645_subdev_ops = {
>> +	.core = &ov5645_core_ops,
>> +	.video = &ov5645_video_ops,
>> +	.pad = &ov5645_subdev_pad_ops,
>> +};
> 
> You can make all those structures static const.
Ok.

> 
>> +
>> +static const struct v4l2_subdev_internal_ops ov5645_subdev_internal_ops = {
>> +};
>> +
>> +static int ov5645_probe(struct i2c_client *client,
>> +			const struct i2c_device_id *id)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct device_node *endpoint;
>> +	struct ov5645 *ov5645;
>> +	u8 chip_id_high, chip_id_low;
>> +	int ret;
>> +
>> +	ov5645 = devm_kzalloc(dev, sizeof(struct ov5645), GFP_KERNEL);
>> +	if (!ov5645)
>> +		return -ENOMEM;
>> +
>> +	ov5645->i2c_client = client;
>> +	ov5645->dev = dev;
>> +
>> +	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
>> +	if (!endpoint) {
>> +		dev_err(dev, "endpoint node not found\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = v4l2_of_parse_endpoint(endpoint, &ov5645->ep);
>> +	if (ret < 0) {
>> +		dev_err(dev, "parsing endpoint node failed\n");
>> +		return ret;
>> +	}
> 
> You can call of_node_put(endpoint) here instead of twice below.
Yes.

> 
>> +	if (ov5645->ep.bus_type != V4L2_MBUS_CSI2) {
>> +		dev_err(dev, "invalid bus type, must be CSI2\n");
>> +		of_node_put(endpoint);
>> +		return -EINVAL;
>> +	}
>> +	of_node_put(endpoint);
>> +
>> +	/* get system clock (xclk) */
>> +	ov5645->xclk = devm_clk_get(dev, "xclk");
>> +	if (IS_ERR(ov5645->xclk)) {
>> +		dev_err(dev, "could not get xclk");
>> +		return PTR_ERR(ov5645->xclk);
>> +	}
>> +
>> +	ret = of_property_read_u32(dev->of_node, "clock-frequency",
>> +				    &ov5645->xclk_freq);
>> +	if (ret) {
>> +		dev_err(dev, "could not get xclk frequency\n");
>> +		return ret;
>> +	}
> 
> Shouldn't you return an error if the frequency is different than the only one 
> supported by the driver (for which the register values have been computed and 
> hardcoded) ?
Yes, this will be useful.

> 
>> +	ov5645->io_regulator = devm_regulator_get(dev, "vdddo");
>> +	if (IS_ERR(ov5645->io_regulator)) {
>> +		dev_err(dev, "cannot get io regulator\n");
>> +		return PTR_ERR(ov5645->io_regulator);
>> +	}
>> +
>> +	ret = regulator_set_voltage(ov5645->io_regulator,
>> +				    OV5645_VOLTAGE_DIGITAL_IO,
>> +				    OV5645_VOLTAGE_DIGITAL_IO);
>> +	if (ret < 0) {
>> +		dev_err(dev, "cannot set io voltage\n");
>> +		return ret;
>> +	}
>> +
>> +	ov5645->core_regulator = devm_regulator_get(dev, "vddd");
>> +	if (IS_ERR(ov5645->core_regulator)) {
>> +		dev_err(dev, "cannot get core regulator\n");
>> +		return PTR_ERR(ov5645->core_regulator);
>> +	}
>> +
>> +	ret = regulator_set_voltage(ov5645->core_regulator,
>> +				    OV5645_VOLTAGE_DIGITAL_CORE,
>> +				    OV5645_VOLTAGE_DIGITAL_CORE);
>> +	if (ret < 0) {
>> +		dev_err(dev, "cannot set core voltage\n");
>> +		return ret;
>> +	}
>> +
>> +	ov5645->analog_regulator = devm_regulator_get(dev, "vdda");
>> +	if (IS_ERR(ov5645->analog_regulator)) {
>> +		dev_err(dev, "cannot get analog regulator\n");
>> +		return PTR_ERR(ov5645->analog_regulator);
>> +	}
>> +
>> +	ret = regulator_set_voltage(ov5645->analog_regulator,
>> +				    OV5645_VOLTAGE_ANALOG,
>> +				    OV5645_VOLTAGE_ANALOG);
>> +	if (ret < 0) {
>> +		dev_err(dev, "cannot set analog voltage\n");
>> +		return ret;
>> +	}
>> +
>> +	ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(ov5645->enable_gpio)) {
>> +		dev_err(dev, "cannot get enable gpio\n");
>> +		return PTR_ERR(ov5645->enable_gpio);
>> +	}
>> +
>> +	ov5645->rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(ov5645->rst_gpio)) {
>> +		dev_err(dev, "cannot get reset gpio\n");
>> +		return PTR_ERR(ov5645->rst_gpio);
>> +	}
>> +
>> +	mutex_init(&ov5645->power_lock);
>> +
>> +	v4l2_ctrl_handler_init(&ov5645->ctrls, 7);
>> +	ov5645->saturation = v4l2_ctrl_new_std(&ov5645->ctrls, 
> &ov5645_ctrl_ops,
>> +				V4L2_CID_SATURATION, -4, 4, 1, 0);
>> +	ov5645->hflip = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
>> +				V4L2_CID_HFLIP, 0, 1, 1, 0);
>> +	ov5645->vflip = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
>> +				V4L2_CID_VFLIP, 0, 1, 1, 0);
>> +	ov5645->autogain = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
>> +				V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
>> +	ov5645->autoexposure = v4l2_ctrl_new_std_menu(&ov5645->ctrls,
>> +				&ov5645_ctrl_ops, V4L2_CID_EXPOSURE_AUTO,
>> +				V4L2_EXPOSURE_MANUAL, 0, V4L2_EXPOSURE_AUTO);
>> +	ov5645->awb = v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
>> +				V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
>> +	ov5645->pattern = v4l2_ctrl_new_std_menu_items(&ov5645->ctrls,
>> +				&ov5645_ctrl_ops, V4L2_CID_TEST_PATTERN,
>> +				ARRAY_SIZE(ov5645_test_pattern_menu) - 1, 0, 
> 0,
>> +				ov5645_test_pattern_menu);
> 
> You don't need to store all these in the ov5645 structure as they are never 
> used.
Ok.

> 
>> +
>> +	ov5645->sd.ctrl_handler = &ov5645->ctrls;
>> +
>> +	if (ov5645->ctrls.error) {
>> +		dev_err(dev, "%s: control initialization error %d\n",
>> +		       __func__, ov5645->ctrls.error);
>> +		ret = ov5645->ctrls.error;
>> +		goto free_ctrl;
>> +	}
>> +
>> +	v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
>> +	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
>> +	ov5645->sd.internal_ops = &ov5645_subdev_internal_ops;
>> +
>> +	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
>> +	if (ret < 0) {
>> +		dev_err(dev, "could not register media entity\n");
>> +		goto free_ctrl;
>> +	}
>> +
>> +	ov5645->sd.dev = &client->dev;
>> +	ret = v4l2_async_register_subdev(&ov5645->sd);
>> +	if (ret < 0) {
>> +		dev_err(dev, "could not register v4l2 device\n");
>> +		goto free_entity;
>> +	}
>> +
>> +	ret = ov5645_s_power(&ov5645->sd, true);
>> +	if (ret < 0) {
>> +		dev_err(dev, "could not power up OV5645\n");
>> +		goto unregister_subdev;
>> +	}
>> +
>> +	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
>> +	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
>> +		dev_err(dev, "could not read ID high\n");
>> +		ret = -ENODEV;
>> +		goto power_down;
>> +	}
>> +	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low);
>> +	if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
>> +		dev_err(dev, "could not read ID low\n");
>> +		ret = -ENODEV;
>> +		goto power_down;
>> +	}
> 
> I would do this before registering the subdev, as you don't want to make it 
> available to the system if the ID doesn't match.
Agree, I'll move the registering.

> 
>> +
>> +	dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
>> +
>> +	ov5645_s_power(&ov5645->sd, false);
>> +
>> +	return 0;
>> +
>> +power_down:
>> +	ov5645_s_power(&ov5645->sd, false);
>> +unregister_subdev:
>> +	v4l2_async_unregister_subdev(&ov5645->sd);
>> +free_entity:
>> +	media_entity_cleanup(&ov5645->sd.entity);
>> +free_ctrl:
>> +	v4l2_ctrl_handler_free(&ov5645->ctrls);
>> +	mutex_destroy(&ov5645->power_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +
>> +static int ov5645_remove(struct i2c_client *client)
>> +{
>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +	struct ov5645 *ov5645 = to_ov5645(sd);
>> +
>> +	v4l2_async_unregister_subdev(&ov5645->sd);
>> +	media_entity_cleanup(&ov5645->sd.entity);
>> +	v4l2_ctrl_handler_free(&ov5645->ctrls);
>> +	mutex_destroy(&ov5645->power_lock);
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +static const struct i2c_device_id ov5645_id[] = {
>> +	{ "ov5645", 0 },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ov5645_id);
>> +
>> +static const struct of_device_id ov5645_of_match[] = {
>> +	{ .compatible = "ovti,ov5645" },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, ov5645_of_match);
>> +
>> +static struct i2c_driver ov5645_i2c_driver = {
>> +	.driver = {
>> +		.of_match_table = of_match_ptr(ov5645_of_match),
>> +		.name  = "ov5645",
>> +	},
>> +	.probe  = ov5645_probe,
>> +	.remove = ov5645_remove,
>> +	.id_table = ov5645_id,
>> +};
>> +
>> +module_i2c_driver(ov5645_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("Omnivision OV5645 Camera Driver");
>> +MODULE_AUTHOR("Todor Tomov <todor.tomov@linaro.org>");
>> +MODULE_LICENSE("GPL v2");
> 

-- 
Best regards,
Todor Tomov

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 <todor.tomov@linaro.org>
> >> ---
> >> 
> >>  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 0000000..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, &val);
> >> +	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, &val);
> >> +	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 ov5645_set_test_pattern(struct ov5645 *ov5645, s32 value)
> >> +{
> >> +	u8 val;
> >> +	int ret;
> >> +
> >> +	ret = ov5645_read_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, &val);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	if (value) {
> >> +		val &= ~OV5645_SET_TEST_PATTERN(OV5645_TEST_PATTERN_MASK);
> >> +		val |= OV5645_SET_TEST_PATTERN(value - 1);
> >> +		val |= OV5645_TEST_PATTERN_ENABLE;
> >> +	} else {
> >> +		val &= ~OV5645_TEST_PATTERN_ENABLE;
> >> +	}
> >> +
> >> +	return ov5645_write_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, val);
> > 
> > Are there other bits that need to be preserved in this register ?
> 
> This driver is based on the driver for OV5645 from QC and the driver for
> OV5640 that was sent to linux-media. I cannot add additional functionality
> so I preserve the rest of the bits. But I'll add caching in a variable here
> too.

As far as I know, based on the documentation I've seen, all bits in this 
register control the test pattern and none need to be preserved. The default 
reset value of the register is 0x00 and the initialization sequence sets it to 
0x00 as well, so it should be safe not caching it.

> >> +}
> >> +
> >> +static const char * const ov5645_test_pattern_menu[] = {
> >> +	"Disabled",
> >> +	"Vertical Color Bars",
> >> +	"Pseudo-Random Data",
> >> +	"Color Square",
> >> +	"Black Image",
> >> +};
> >> +
> >> +static int ov5645_set_awb(struct ov5645 *ov5645, s32 enable_auto)
> >> +{
> >> +	u8 val;
> >> +	int ret;
> >> +
> >> +	ret = ov5645_read_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, &val);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	if (enable_auto)
> >> +		val &= ~OV5645_AWB_MANUAL_ENABLE;
> >> +	else
> >> +		val |= OV5645_AWB_MANUAL_ENABLE;
> >> +
> >> +	return ov5645_write_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, val);
> > 
> > Same here, are there other bits that need to be preserved ?
> 
> Same as above.

Bits 7:1 are documented as "debug mode" and are set to 0 at reset time. It 
should be fine not caching this register.

> >> +}

[snip]

> >> +static int ov5645_entity_init_cfg(struct v4l2_subdev *subdev,
> >> +				  struct v4l2_subdev_pad_config *cfg)
> >> +{
> >> +	struct v4l2_subdev_format fmt = { 0 };
> >> +
> >> +	fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> > 
> > The function will always be called with cfg != NULL.
> 
> I intend to call this function from probe to init the active format. Will
> this be ok?

If you plan to call it with cfg == NULL then yes this has to be handled.

> >> +	fmt.format.width = 1920;
> >> +	fmt.format.height = 1080;
> >> +
> >> +	v4l2_subdev_call(subdev, pad, set_fmt, cfg, &fmt);
> > 
> > You can call ov5645_set_format directly.
> 
> Ok.
> 
> >> +	return 0;
> >> +}

[snip]

-- 
Regards,

Laurent Pinchart

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 <todor.tomov@linaro.org>
>>>> ---
>>>>
>>>>  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 0000000..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, &val);
>>>> +	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, &val);
>>>> +	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 ov5645_set_test_pattern(struct ov5645 *ov5645, s32 value)
>>>> +{
>>>> +	u8 val;
>>>> +	int ret;
>>>> +
>>>> +	ret = ov5645_read_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, &val);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	if (value) {
>>>> +		val &= ~OV5645_SET_TEST_PATTERN(OV5645_TEST_PATTERN_MASK);
>>>> +		val |= OV5645_SET_TEST_PATTERN(value - 1);
>>>> +		val |= OV5645_TEST_PATTERN_ENABLE;
>>>> +	} else {
>>>> +		val &= ~OV5645_TEST_PATTERN_ENABLE;
>>>> +	}
>>>> +
>>>> +	return ov5645_write_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, val);
>>>
>>> Are there other bits that need to be preserved in this register ?
>>
>> This driver is based on the driver for OV5645 from QC and the driver for
>> OV5640 that was sent to linux-media. I cannot add additional functionality
>> so I preserve the rest of the bits. But I'll add caching in a variable here
>> too.
> 
> As far as I know, based on the documentation I've seen, all bits in this 
> register control the test pattern and none need to be preserved. The default 
> reset value of the register is 0x00 and the initialization sequence sets it to 
> 0x00 as well, so it should be safe not caching it.
> 

Ok, then I'll remove any reading or caching for this one.

>>>> +}
>>>> +
>>>> +static const char * const ov5645_test_pattern_menu[] = {
>>>> +	"Disabled",
>>>> +	"Vertical Color Bars",
>>>> +	"Pseudo-Random Data",
>>>> +	"Color Square",
>>>> +	"Black Image",
>>>> +};
>>>> +
>>>> +static int ov5645_set_awb(struct ov5645 *ov5645, s32 enable_auto)
>>>> +{
>>>> +	u8 val;
>>>> +	int ret;
>>>> +
>>>> +	ret = ov5645_read_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, &val);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	if (enable_auto)
>>>> +		val &= ~OV5645_AWB_MANUAL_ENABLE;
>>>> +	else
>>>> +		val |= OV5645_AWB_MANUAL_ENABLE;
>>>> +
>>>> +	return ov5645_write_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, val);
>>>
>>> Same here, are there other bits that need to be preserved ?
>>
>> Same as above.
> 
> Bits 7:1 are documented as "debug mode" and are set to 0 at reset time. It 
> should be fine not caching this register.
> 

I will remove the reading and caching here too.

>>>> +}
> 
> [snip]
> 
>>>> +static int ov5645_entity_init_cfg(struct v4l2_subdev *subdev,
>>>> +				  struct v4l2_subdev_pad_config *cfg)
>>>> +{
>>>> +	struct v4l2_subdev_format fmt = { 0 };
>>>> +
>>>> +	fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
>>>
>>> The function will always be called with cfg != NULL.
>>
>> I intend to call this function from probe to init the active format. Will
>> this be ok?
> 
> If you plan to call it with cfg == NULL then yes this has to be handled.
> 
>>>> +	fmt.format.width = 1920;
>>>> +	fmt.format.height = 1080;
>>>> +
>>>> +	v4l2_subdev_call(subdev, pad, set_fmt, cfg, &fmt);
>>>
>>> You can call ov5645_set_format directly.
>>
>> Ok.
>>
>>>> +	return 0;
>>>> +}
> 
> [snip]
> 

-- 
Best regards,
Todor Tomov

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 <todor.tomov@linaro.org>
>>>>> ---
>>>>>
>>>>>  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 0000000..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, &val);
>>>>> +	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, &val);
>>>>> +	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 ov5645_set_test_pattern(struct ov5645 *ov5645, s32 value)
>>>>> +{
>>>>> +	u8 val;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = ov5645_read_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, &val);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (value) {
>>>>> +		val &= ~OV5645_SET_TEST_PATTERN(OV5645_TEST_PATTERN_MASK);
>>>>> +		val |= OV5645_SET_TEST_PATTERN(value - 1);
>>>>> +		val |= OV5645_TEST_PATTERN_ENABLE;
>>>>> +	} else {
>>>>> +		val &= ~OV5645_TEST_PATTERN_ENABLE;
>>>>> +	}
>>>>> +
>>>>> +	return ov5645_write_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, val);
>>>>
>>>> Are there other bits that need to be preserved in this register ?
>>>
>>> This driver is based on the driver for OV5645 from QC and the driver for
>>> OV5640 that was sent to linux-media. I cannot add additional functionality
>>> so I preserve the rest of the bits. But I'll add caching in a variable here
>>> too.
>>
>> As far as I know, based on the documentation I've seen, all bits in this 
>> register control the test pattern and none need to be preserved. The default 
>> reset value of the register is 0x00 and the initialization sequence sets it to 
>> 0x00 as well, so it should be safe not caching it.
>>
> 
> Ok, then I'll remove any reading or caching for this one.
> 
>>>>> +}
>>>>> +
>>>>> +static const char * const ov5645_test_pattern_menu[] = {
>>>>> +	"Disabled",
>>>>> +	"Vertical Color Bars",
>>>>> +	"Pseudo-Random Data",
>>>>> +	"Color Square",
>>>>> +	"Black Image",
>>>>> +};
>>>>> +
>>>>> +static int ov5645_set_awb(struct ov5645 *ov5645, s32 enable_auto)
>>>>> +{
>>>>> +	u8 val;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = ov5645_read_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, &val);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (enable_auto)
>>>>> +		val &= ~OV5645_AWB_MANUAL_ENABLE;
>>>>> +	else
>>>>> +		val |= OV5645_AWB_MANUAL_ENABLE;
>>>>> +
>>>>> +	return ov5645_write_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, val);
>>>>
>>>> Same here, are there other bits that need to be preserved ?
>>>
>>> Same as above.
>>
>> Bits 7:1 are documented as "debug mode" and are set to 0 at reset time. It 
>> should be fine not caching this register.
>>
> 
> I will remove the reading and caching here too.
> 
>>>>> +}
>>
>> [snip]
>>
>>>>> +static int ov5645_entity_init_cfg(struct v4l2_subdev *subdev,
>>>>> +				  struct v4l2_subdev_pad_config *cfg)
>>>>> +{
>>>>> +	struct v4l2_subdev_format fmt = { 0 };
>>>>> +
>>>>> +	fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
>>>>
>>>> The function will always be called with cfg != NULL.
>>>
>>> I intend to call this function from probe to init the active format. Will
>>> this be ok?
>>
>> If you plan to call it with cfg == NULL then yes this has to be handled.
>>
>>>>> +	fmt.format.width = 1920;
>>>>> +	fmt.format.height = 1080;
>>>>> +
>>>>> +	v4l2_subdev_call(subdev, pad, set_fmt, cfg, &fmt);
>>>>
>>>> You can call ov5645_set_format directly.
>>>
>>> Ok.
>>>
>>>>> +	return 0;
>>>>> +}
>>
>> [snip]
>>
> 

-- 
Best regards,
Todor Tomov

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

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

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
>

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