| Message ID | 1463572208-8826-2-git-send-email-todor.tomov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linaro.org> > --- > .../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. > +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. > +- 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. > + > +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. Both need to specify the polarity. > +- dovdd-supply: Chip IO regulator > +- dvdd-supply: Chip core regulator > +- avdd-supply: Chip analog 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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@linaro.org> >> --- >> .../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.) > >> +- 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. > >> + >> +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? > > Both need to specify the polarity. Ok, I'll add a note for the polarity. > >> +- dovdd-supply: Chip IO regulator >> +- dvdd-supply: Chip core regulator >> +- avdd-supply: Chip analog 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.
On Thu, May 19, 2016 at 3:14 AM, Todor Tomov <todor.tomov@linaro.org> 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@linaro.org> >>> --- >>> .../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 linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/20/2016 03:16 AM, Rob Herring wrote: > On Thu, May 19, 2016 at 3:14 AM, Todor Tomov <todor.tomov@linaro.org> 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@linaro.org> >>>> --- >>>> .../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 >
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 +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 +- assigned-clock-rates: should be "23880000" + +Optional Properties: +- reset-gpios: Chip reset GPIO +- pwdn-gpios: Chip power down GPIO +- dovdd-supply: Chip IO regulator +- dvdd-supply: Chip core regulator +- avdd-supply: Chip analog 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>; + + pwdn-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>; + reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>; + pinctrl-names = "default"; + pinctrl-0 = <&camera_rear_default>; + + clocks = <&clks 200>; + clock-names = "xclk"; + assigned-clocks = <&clks 200>; + assigned-clock-rates = <23880000>; + + dovdd-supply = <&camera_dovdd_1v8>; + avdd-supply = <&camera_avdd_2v8>; + dvdd-supply = <&camera_dvdd_1v2>; + + port { + ov5645_ep: endpoint { + clock-lanes = <1>; + data-lanes = <0 2>; + remote-endpoint = <&csi0_ep>; + }; + }; + }; + };
Add the document for ov5645 device tree binding. Signed-off-by: Todor Tomov <todor.tomov@linaro.org> --- .../devicetree/bindings/media/i2c/ov5645.txt | 56 ++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt