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
