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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Thu, May 19, 2016 at 3:14 AM, Todor Tomov <todor.tomov@xxxxxxxxxx> wrote:
> Hi Rob,
>
> Thank you for your time to review. My responses are below:
>
> On 05/19/2016 02:16 AM, Rob Herring wrote:
>> On Wed, May 18, 2016 at 02:50:07PM +0300, Todor Tomov wrote:
>>> Add the document for ov5645 device tree binding.
>>>
>>> Signed-off-by: Todor Tomov <todor.tomov@xxxxxxxxxx>
>>> ---
>>>  .../devicetree/bindings/media/i2c/ov5645.txt       | 56 ++++++++++++++++++++++
>>>  1 file changed, 56 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..8799000
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
>>> @@ -0,0 +1,56 @@
>>> +* 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 SCCB
>>
>> s/SCCB/I2C/ because that is the more common name.
> Ok.
>
>>
>>> +interface.
>>> +
>>> +Required Properties:
>>> +- compatible: value should be "ovti,ov5645"
>>> +- clocks: reference to the xclk clock
>>> +- clock-names: should be "xclk"
>>> +- assigned-clocks: reference to the xclk clock
>>
>> This should be optional as it only makes sense if there is more than one
>> option.
> I have only used assigned-clocks to specify for which clock the
> assigned-clock-rates property is. This is the way I understood it.
> Isn't this correct? (Also please see below.)

AIUI, assigned-clocks is which parent you want to assign for the clock
specified in "clocks". Whether you have a parent option or not is
board/chip dependent.

>>> +- assigned-clock-rates: should be "23880000"
>>
>> Doesn't this depend on the board? Most parts take a range of
>> frequencies. The driver should know what the range is and request a rate
>> within this range.
> This is the sensor external clock. Actually the driver depends on this value -
> the sensor mode settings which the driver configures are calculated based on
> this value. If you change this clock rate you need to change the sensor mode
> settings. However they usually come from the vendor of the sensor so they
> usually never change. So this clock rate for this driver is fixed to 23.88MHz
> and is not expected to change.

If fixed in the driver, then it doesn't need to be in DT.

>>> +
>>> +Optional Properties:
>>> +- reset-gpios: Chip reset GPIO
>>> +- pwdn-gpios: Chip power down GPIO
>>
>> Use enable-gpios as it is more common and would just be the inverse of
>> this.
> pwdn is the notation which OV use for this gpio, so I'd personally prefer
> to keep the name. Do you think it is still better to change it?

Yes.

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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux