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

2016-11-02 Thread 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 
> >>> ---
> >>> 
> >>>  .../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 000..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

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

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

2016-11-01 Thread Todor Tomov 
On 10/26/2016 09:53 PM, Rob Herring wrote:
> On Wed, Oct 19, 2016 at 4:21 AM, Laurent Pinchart
>  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 
>>> ---
>>>
>>>  .../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 000..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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-26 Thread Rob Herring 
On Wed, Oct 19, 2016 at 4:21 AM, Laurent Pinchart
 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 
>>  ---
>> 
>>   .../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 000..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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-19 Thread 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 
>  ---
>  
>   .../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 000..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:
>  +
>  + {
>  +...
>  +
>  +ov5645: ov5645@78 {
>  +compatible = "ovti,ov5645";
>  +reg = <0x78>;
>  +
>  +enable-gpios = < 6 GPIO_ACTIVE_HIGH>;
>  +reset-gpios = < 20 GPIO_ACTIVE_LOW>;
>  +pinctrl-names = "default";
>  +pinctrl-0 = <_rear_default>;
>  +
>  +clocks = < 200>;
>  +clock-names = "xclk";
>  +clock-frequency = <2388>;
>  +
>  +vdddo-supply = <_dovdd_1v8>;
>  +vdda-supply = <_avdd_2v8>;
>  +vddd-supply = <_dvdd_1v2>;
>  +
>  +port {
>  +ov5645_ep: endpoint {
>  +clock-lanes = <1>;
>  +data-lanes = <0 2>;
>  +remote-endpoint = <_ep>;
>  +};
>  +};
>  +};
>  +};

-- 
Regards,

Laurent Pinchart

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

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

2016-10-19 Thread 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 
> >> ---
> >> 
> >>  .../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 000..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:
> >> +
> >> +   {
> >> +  ...
> >> +
> >> +  ov5645: ov5645@78 {
> >> +  compatible = "ovti,ov5645";
> >> +  reg = <0x78>;
> >> +
> >> +  enable-gpios = < 6 GPIO_ACTIVE_HIGH>;
> >> +  reset-gpios = < 20 GPIO_ACTIVE_LOW>;
> >> +  pinctrl-names = "default";
> >> +  pinctrl-0 = <_rear_default>;
> >> +
> >> +  clocks = < 200>;
> >> +  clock-names = "xclk";
> >> +  clock-frequency = <2388>;
> >> +
> >> +  vdddo-supply = <_dovdd_1v8>;
> >> +  vdda-supply = <_avdd_2v8>;
> >> +  vddd-supply = <_dvdd_1v2>;
> >> +
> >> +  port {
> >> +  ov5645_ep: endpoint {
> >> +  clock-lanes = <1>;
> >> +  data-lanes = <0 2>;
> >> +  remote-endpoint = <_ep>;
> >> +  };
> >> +  };
> >> +  };
> >> +  };

-- 
Regards,

Laurent Pinchart

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

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

2016-10-19 Thread 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 
 ---

  .../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 000..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:
 +
 +   {
 +  ...
 +
 +  ov5645: ov5645@78 {
 +  compatible = "ovti,ov5645";
 +  reg = <0x78>;
 +
 +  enable-gpios = < 6 GPIO_ACTIVE_HIGH>;
 +  reset-gpios = < 20 GPIO_ACTIVE_LOW>;
 +  pinctrl-names = "default";
 +  pinctrl-0 = <_rear_default>;
 +
 +  clocks = < 200>;
 +  clock-names = "xclk";
 +  clock-frequency = <2388>;
 +
 +  vdddo-supply = <_dovdd_1v8>;
 +  vdda-supply = <_avdd_2v8>;
 +  vddd-supply = <_dvdd_1v2>;
 +
 +  port {
 +  ov5645_ep: endpoint {
 +  clock-lanes = <1>;
 +  data-lanes = <0 2>;
 +  remote-endpoint = <_ep>;
 +  };
 +  };
 +  };
 +  };
> 

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

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

2016-10-14 Thread 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 
>> ---
>>  .../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 000..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:
>> +
>> + {
>> +...
>> +
>> +ov5645: ov5645@78 {
>> +compatible = "ovti,ov5645";
>> +reg = <0x78>;
>> +
>> +enable-gpios = < 6 GPIO_ACTIVE_HIGH>;
>> +reset-gpios = < 20 GPIO_ACTIVE_LOW>;
>> +pinctrl-names = "default";
>> +pinctrl-0 = <_rear_default>;
>> +
>> +clocks = < 200>;
>> +clock-names = "xclk";
>> +clock-frequency = <2388>;
>> +
>> +vdddo-supply = <_dovdd_1v8>;
>> +vdda-supply = <_avdd_2v8>;
>> +vddd-supply = <_dvdd_1v2>;
>> +
>> +port {
>> +ov5645_ep: endpoint {
>> +clock-lanes = <1>;
>> +data-lanes = <0 2>;
>> +remote-endpoint = <_ep>;
>> +};
>> +};
>> +};
>> +};
> 

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

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

2016-09-08 Thread 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 
> ---
>  .../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 000..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:
> +
> +  {
> + ...
> +
> + ov5645: ov5645@78 {
> + compatible = "ovti,ov5645";
> + reg = <0x78>;
> +
> + enable-gpios = < 6 GPIO_ACTIVE_HIGH>;
> + reset-gpios = < 20 GPIO_ACTIVE_LOW>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_rear_default>;
> +
> + clocks = < 200>;
> + clock-names = "xclk";
> + clock-frequency = <2388>;
> +
> + vdddo-supply = <_dovdd_1v8>;
> + vdda-supply = <_avdd_2v8>;
> + vddd-supply = <_dvdd_1v2>;
> +
> + port {
> + ov5645_ep: endpoint {
> + clock-lanes = <1>;
> + data-lanes = <0 2>;
> + remote-endpoint = <_ep>;
> + };
> + };
> + };
> + };

-- 
Regards,

Laurent Pinchart

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

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

2016-09-08 Thread Todor Tomov 
Add the document for ov5645 device tree binding.

Signed-off-by: Todor Tomov 
---
 .../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 000..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:
+
+{
+   ...
+
+   ov5645: ov5645@78 {
+   compatible = "ovti,ov5645";
+   reg = <0x78>;
+
+   enable-gpios = < 6 GPIO_ACTIVE_HIGH>;
+   reset-gpios = < 20 GPIO_ACTIVE_LOW>;
+   pinctrl-names = "default";
+   pinctrl-0 = <_rear_default>;
+
+   clocks = < 200>;
+   clock-names = "xclk";
+   clock-frequency = <2388>;
+
+   vdddo-supply = <_dovdd_1v8>;
+   vdda-supply = <_avdd_2v8>;
+   vddd-supply = <_dvdd_1v2>;
+
+   port {
+   ov5645_ep: endpoint {
+   clock-lanes = <1>;
+   data-lanes = <0 2>;
+   remote-endpoint = <_ep>;
+   };
+   };
+   };
+   };
-- 
1.9.1

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