On 05/20/2016 03:16 AM, Rob Herring wrote: > 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. Ok, I'll remove these two and leave the external clock value in the driver. > >>>> + >>>> +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. Ok, I'll change it to enable-gpios. Thanks. > > Rob > -- Best regards, Todor Tomov -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
