| Message ID | 20191030094902.32582-2-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding | expand |
Hi Sakari, On Wed, Oct 30, 2019 at 01:53:28PM +0200, Sakari Ailus wrote: > Hi Nabuvannan, > > On Wed, Oct 30, 2019 at 03:19:01PM +0530, Manivannan Sadhasivam wrote: > > Add YAML devicetree binding for IMX296 CMOS image sensor. Let's also > > add MAINTAINERS entry for the binding and driver. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > .../devicetree/bindings/media/i2c/imx296.yaml | 94 +++++++++++++++++++ > > MAINTAINERS | 8 ++ > > 2 files changed, 102 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx296.yaml b/Documentation/devicetree/bindings/media/i2c/imx296.yaml > > new file mode 100644 > > index 000000000000..c04ec2203268 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/imx296.yaml > > @@ -0,0 +1,94 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/imx296.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Sony IMX296 1/2.8-Inch CMOS Image Sensor > > + > > +maintainers: > > + - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > + > > +description: |- > > + The Sony IMX296 is a 1/2.9-Inch active pixel type CMOS Solid-state image > > + sensor with square pixel array and 1.58 M effective pixels. This chip > > + features a global shutter with variable charge-integration time. It is > > + programmable through I2C and 4-wire interfaces. The sensor output is > > + available via CSI-2 serial data output (1 Lane). > > + > > +properties: > > + compatible: > > + const: sony,imx296 > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > + clock-names: > > + description: > > + Input clock for the sensor. > > + items: > > + - const: mclk > > + > > + clock-frequency: > > + description: > > + Frequency of the mclk clock in Hertz. > > + > > + vddo-supply: > > + description: > > + Definition of the regulator used as interface power supply. > > + > > + vdda-supply: > > + description: > > + Definition of the regulator used as analog power supply. > > + > > + vddd-supply: > > + description: > > + Definition of the regulator used as digital power supply. > > + > > + reset-gpios: > > + description: > > + The phandle and specifier for the GPIO that controls sensor reset. > > + maxItems: 1 > > + > > + port: true > > You're missing "type: object" under port. > I did that intentionally, since there are other places where I can see the "type" field not specified. So, I was not sure about that. Most of the display bindings don't specify "type" and they are most available ones. I don't think the "port" property differs between cameras and displays. So I went with that. Thanks, Mani > -- > Regards, > > Sakari Ailus
Hi Laurent, On Thu, Oct 31, 2019 at 03:15:38PM +0200, Laurent Pinchart wrote: > Hi Mani, > > Thank you for the patch. > > On Wed, Oct 30, 2019 at 03:19:01PM +0530, Manivannan Sadhasivam wrote: > > Add YAML devicetree binding for IMX296 CMOS image sensor. Let's also > > add MAINTAINERS entry for the binding and driver. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > .../devicetree/bindings/media/i2c/imx296.yaml | 94 +++++++++++++++++++ > > MAINTAINERS | 8 ++ > > 2 files changed, 102 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx296.yaml b/Documentation/devicetree/bindings/media/i2c/imx296.yaml > > new file mode 100644 > > index 000000000000..c04ec2203268 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/imx296.yaml > > @@ -0,0 +1,94 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/imx296.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Sony IMX296 1/2.8-Inch CMOS Image Sensor > > + > > +maintainers: > > + - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > + > > +description: |- > > + The Sony IMX296 is a 1/2.9-Inch active pixel type CMOS Solid-state image > > + sensor with square pixel array and 1.58 M effective pixels. This chip > > + features a global shutter with variable charge-integration time. It is > > + programmable through I2C and 4-wire interfaces. The sensor output is > > + available via CSI-2 serial data output (1 Lane). > > + > > +properties: > > + compatible: > > + const: sony,imx296 > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > + clock-names: > > + description: > > + Input clock for the sensor. > > + items: > > + - const: mclk > > The pin is named INCK, let's name the clock accordingly. > Okay, I thought generic names are preferred here! > > + clock-frequency: > > + description: > > + Frequency of the mclk clock in Hertz. > > This shouldn't be needed, you can retrieve the clock frequency at > runtime from the clock source. > Unless the clock source is a fixed one! What if the clock source comes from SoC? We need to set the rate, right? > > + vddo-supply: > > + description: > > + Definition of the regulator used as interface power supply. > > + > > + vdda-supply: > > + description: > > + Definition of the regulator used as analog power supply. > > + > > + vddd-supply: > > + description: > > + Definition of the regulator used as digital power supply. > > Do we really need three regulators ? I agree that the sensor has three > power rails, but aren't they usually powered by regulators that are > tied together, without individual control ? The IMX926 specifications > require the three power supplies to raise within 200ms, which we should > be able to ensure in software. What does your board use, does it have > multiple GPIOs to control each power supply ? If not I wonder if we > could just define vddd-supply now, and add vdda-supply and vddo-supply > later if we need to support systems that can control the supplies > individually. > The whole power supply model is a bit rotten. In my case, there are 3 different regulators used with no software control. So, I can't control the rise time (I assume that they are handled by the external power regulator itself). So to be sane, I just documented with the assumption of fixed-regulators. Thanks, Mani > > + reset-gpios: > > + description: > > + The phandle and specifier for the GPIO that controls sensor reset. > > + maxItems: 1 > > + > > + port: true > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - clock-names > > + - clock-frequency > > + - vddo-supply > > + - vdda-supply > > + - vddd-supply > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + > > + imx296: camera-sensor@1a { > > + compatible = "sony,imx296"; > > + reg = <0x1a>; > > + reset-gpios = <&msmgpio 35 GPIO_ACTIVE_LOW>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&camera_rear_default>; > > + clocks = <&gcc 90>; > > + clock-names = "mclk"; > > + clock-frequency = <37125000>; > > + vddo-supply = <&camera_vddo_1v8>; > > + vdda-supply = <&camera_vdda_3v3>; > > + vddd-supply = <&camera_vddd_1v2>; > > + > > + port { > > + imx296_ep: endpoint { > > + remote-endpoint = <&csiphy0_ep>; > > + }; > > + }; > > + }; > > + > > +... > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 55199ef7fa74..51194bb2c392 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -15140,6 +15140,14 @@ S: Maintained > > F: drivers/media/i2c/imx274.c > > F: Documentation/devicetree/bindings/media/i2c/imx274.txt > > > > +SONY IMX296 SENSOR DRIVER > > +M: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > +L: linux-media@vger.kernel.org > > +T: git git://linuxtv.org/media_tree.git > > +S: Maintained > > +F: drivers/media/i2c/imx296.c > > +F: Documentation/devicetree/bindings/media/i2c/imx296.yaml > > + > > SONY IMX319 SENSOR DRIVER > > M: Bingbu Cao <bingbu.cao@intel.com> > > L: linux-media@vger.kernel.org > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On Thu, Oct 31, 2019 at 04:11:41PM +0200, Laurent Pinchart wrote: > Hi Mani, > > On Thu, Oct 31, 2019 at 07:15:12PM +0530, Manivannan Sadhasivam wrote: > > On Thu, Oct 31, 2019 at 03:15:38PM +0200, Laurent Pinchart wrote: > > > On Wed, Oct 30, 2019 at 03:19:01PM +0530, Manivannan Sadhasivam wrote: > > >> Add YAML devicetree binding for IMX296 CMOS image sensor. Let's also > > >> add MAINTAINERS entry for the binding and driver. > > >> > > >> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > >> --- > > >> .../devicetree/bindings/media/i2c/imx296.yaml | 94 +++++++++++++++++++ > > >> MAINTAINERS | 8 ++ > > >> 2 files changed, 102 insertions(+) > > >> create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.yaml > > >> > > >> diff --git a/Documentation/devicetree/bindings/media/i2c/imx296.yaml b/Documentation/devicetree/bindings/media/i2c/imx296.yaml > > >> new file mode 100644 > > >> index 000000000000..c04ec2203268 > > >> --- /dev/null > > >> +++ b/Documentation/devicetree/bindings/media/i2c/imx296.yaml > > >> @@ -0,0 +1,94 @@ > > >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > >> +%YAML 1.2 > > >> +--- > > >> +$id: http://devicetree.org/schemas/media/i2c/imx296.yaml# > > >> +$schema: http://devicetree.org/meta-schemas/core.yaml# > > >> + > > >> +title: Sony IMX296 1/2.8-Inch CMOS Image Sensor > > >> + > > >> +maintainers: > > >> + - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > >> + > > >> +description: |- > > >> + The Sony IMX296 is a 1/2.9-Inch active pixel type CMOS Solid-state image > > >> + sensor with square pixel array and 1.58 M effective pixels. This chip > > >> + features a global shutter with variable charge-integration time. It is > > >> + programmable through I2C and 4-wire interfaces. The sensor output is > > >> + available via CSI-2 serial data output (1 Lane). > > >> + > > >> +properties: > > >> + compatible: > > >> + const: sony,imx296 > > >> + > > >> + reg: > > >> + maxItems: 1 > > >> + > > >> + clocks: > > >> + maxItems: 1 > > >> + > > >> + clock-names: > > >> + description: > > >> + Input clock for the sensor. > > >> + items: > > >> + - const: mclk > > > > > > The pin is named INCK, let's name the clock accordingly. > > > > Okay, I thought generic names are preferred here! > > > > >> + clock-frequency: > > >> + description: > > >> + Frequency of the mclk clock in Hertz. > > > > > > This shouldn't be needed, you can retrieve the clock frequency at > > > runtime from the clock source. > > > > Unless the clock source is a fixed one! What if the clock source comes from > > SoC? We need to set the rate, right? > > In that case, if you want to hardcode the clock in DT, the preferred way > is to use the assigned-clock-rates property. Otherwise, if the driver > requires a specific clock frequency, it's better to hardcode it in the > driver itself. In this specific case, I think assigned-clock-rates is > best as the device can support three different clock frequencies. Just note that if ACPI support is added to the sensor driver, you'll need the clock-frequency property again, for that's the only way how the driver will get the clock frequency. This is certainly not something that has to be taken into account in DT bindings, but in any case it'll add some lines of code in the driver which are not very useful. -- Sakari Ailus
Hi Laurent, On Mon, Nov 04, 2019 at 09:02:01PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Thu, Oct 31, 2019 at 07:08:37PM +0200, Sakari Ailus wrote: > > On Thu, Oct 31, 2019 at 06:54:44PM +0200, Laurent Pinchart wrote: > > > On Thu, Oct 31, 2019 at 04:28:17PM +0200, Sakari Ailus wrote: > > >> On Thu, Oct 31, 2019 at 04:11:41PM +0200, Laurent Pinchart wrote: > > >>> On Thu, Oct 31, 2019 at 07:15:12PM +0530, Manivannan Sadhasivam wrote: > > >>>> On Thu, Oct 31, 2019 at 03:15:38PM +0200, Laurent Pinchart wrote: > > >>>>> On Wed, Oct 30, 2019 at 03:19:01PM +0530, Manivannan Sadhasivam wrote: > > >>>>>> Add YAML devicetree binding for IMX296 CMOS image sensor. Let's also > > >>>>>> add MAINTAINERS entry for the binding and driver. > > >>>>>> > > >>>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > >>>>>> --- > > >>>>>> .../devicetree/bindings/media/i2c/imx296.yaml | 94 +++++++++++++++++++ > > >>>>>> MAINTAINERS | 8 ++ > > >>>>>> 2 files changed, 102 insertions(+) > > >>>>>> create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.yaml > > >>>>>> > > >>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/imx296.yaml b/Documentation/devicetree/bindings/media/i2c/imx296.yaml > > >>>>>> new file mode 100644 > > >>>>>> index 000000000000..c04ec2203268 > > >>>>>> --- /dev/null > > >>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/imx296.yaml > > >>>>>> @@ -0,0 +1,94 @@ > > >>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > >>>>>> +%YAML 1.2 > > >>>>>> +--- > > >>>>>> +$id: http://devicetree.org/schemas/media/i2c/imx296.yaml# > > >>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > > >>>>>> + > > >>>>>> +title: Sony IMX296 1/2.8-Inch CMOS Image Sensor > > >>>>>> + > > >>>>>> +maintainers: > > >>>>>> + - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > >>>>>> + > > >>>>>> +description: |- > > >>>>>> + The Sony IMX296 is a 1/2.9-Inch active pixel type CMOS Solid-state image > > >>>>>> + sensor with square pixel array and 1.58 M effective pixels. This chip > > >>>>>> + features a global shutter with variable charge-integration time. It is > > >>>>>> + programmable through I2C and 4-wire interfaces. The sensor output is > > >>>>>> + available via CSI-2 serial data output (1 Lane). > > >>>>>> + > > >>>>>> +properties: > > >>>>>> + compatible: > > >>>>>> + const: sony,imx296 > > >>>>>> + > > >>>>>> + reg: > > >>>>>> + maxItems: 1 > > >>>>>> + > > >>>>>> + clocks: > > >>>>>> + maxItems: 1 > > >>>>>> + > > >>>>>> + clock-names: > > >>>>>> + description: > > >>>>>> + Input clock for the sensor. > > >>>>>> + items: > > >>>>>> + - const: mclk > > >>>>> > > >>>>> The pin is named INCK, let's name the clock accordingly. > > >>>> > > >>>> Okay, I thought generic names are preferred here! > > >>>> > > >>>>>> + clock-frequency: > > >>>>>> + description: > > >>>>>> + Frequency of the mclk clock in Hertz. > > >>>>> > > >>>>> This shouldn't be needed, you can retrieve the clock frequency at > > >>>>> runtime from the clock source. > > >>>> > > >>>> Unless the clock source is a fixed one! What if the clock source comes from > > >>>> SoC? We need to set the rate, right? > > >>> > > >>> In that case, if you want to hardcode the clock in DT, the preferred way > > >>> is to use the assigned-clock-rates property. Otherwise, if the driver > > >>> requires a specific clock frequency, it's better to hardcode it in the > > >>> driver itself. In this specific case, I think assigned-clock-rates is > > >>> best as the device can support three different clock frequencies. > > >> > > >> Just note that if ACPI support is added to the sensor driver, you'll need > > >> the clock-frequency property again, for that's the only way how the driver > > >> will get the clock frequency. > > > > > > Why is so ? Why can't we implement of assigned-clock-rates for ACPI ? > > > > ACPI doesn't deal with clocks as such. So there's also no ACPI defined way > > to access clocks specifically, including the frequency --- instead the > > clock is controlled by an AML methods which implement power on and off > > sequences for the device. > > It's a shortcoming of ACPI, which should be addressed at the ACPI level. > We shouldn't polute the DT bindings with a clock-frequency property for > this reason. It's really not a shortcoming but a design decision: what belongs to the scope of the firmware? And in this case system and device power management implementation is included. I do not believe this will be revisited in any foreseeable future, i.e. there will be no clock control interface for ACPI. Explicitly stating the frequency also has an added benefit: the driver can be certain that the given frequency is intended to be used on the board. Otherwise the frequency could have been changed by e.g. another driver. This does matter, as the frequency determines which link frequencies can be achieved, and as the two effectively have to be compliant, an unintended external clock frequency also means there will be no match between possible link frequencies and configured link frequencies. I.e. no images to capture either. That said, I don't know if this has been a practical issue in the past. -- Regards, Sakari Ailus
Hi Sakari, On Mon, Nov 04, 2019 at 11:30:32PM +0200, Sakari Ailus wrote: > On Mon, Nov 04, 2019 at 09:02:01PM +0200, Laurent Pinchart wrote: > > On Thu, Oct 31, 2019 at 07:08:37PM +0200, Sakari Ailus wrote: > >> On Thu, Oct 31, 2019 at 06:54:44PM +0200, Laurent Pinchart wrote: > >>> On Thu, Oct 31, 2019 at 04:28:17PM +0200, Sakari Ailus wrote: > >>>> On Thu, Oct 31, 2019 at 04:11:41PM +0200, Laurent Pinchart wrote: > >>>>> On Thu, Oct 31, 2019 at 07:15:12PM +0530, Manivannan Sadhasivam wrote: > >>>>>> On Thu, Oct 31, 2019 at 03:15:38PM +0200, Laurent Pinchart wrote: > >>>>>>> On Wed, Oct 30, 2019 at 03:19:01PM +0530, Manivannan Sadhasivam wrote: > >>>>>>>> Add YAML devicetree binding for IMX296 CMOS image sensor. Let's also > >>>>>>>> add MAINTAINERS entry for the binding and driver. > >>>>>>>> > >>>>>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > >>>>>>>> --- > >>>>>>>> .../devicetree/bindings/media/i2c/imx296.yaml | 94 +++++++++++++++++++ > >>>>>>>> MAINTAINERS | 8 ++ > >>>>>>>> 2 files changed, 102 insertions(+) > >>>>>>>> create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.yaml > >>>>>>>> > >>>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/imx296.yaml b/Documentation/devicetree/bindings/media/i2c/imx296.yaml > >>>>>>>> new file mode 100644 > >>>>>>>> index 000000000000..c04ec2203268 > >>>>>>>> --- /dev/null > >>>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/imx296.yaml > >>>>>>>> @@ -0,0 +1,94 @@ > >>>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > >>>>>>>> +%YAML 1.2 > >>>>>>>> +--- > >>>>>>>> +$id: http://devicetree.org/schemas/media/i2c/imx296.yaml# > >>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>>>>>>> + > >>>>>>>> +title: Sony IMX296 1/2.8-Inch CMOS Image Sensor > >>>>>>>> + > >>>>>>>> +maintainers: > >>>>>>>> + - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > >>>>>>>> + > >>>>>>>> +description: |- > >>>>>>>> + The Sony IMX296 is a 1/2.9-Inch active pixel type CMOS Solid-state image > >>>>>>>> + sensor with square pixel array and 1.58 M effective pixels. This chip > >>>>>>>> + features a global shutter with variable charge-integration time. It is > >>>>>>>> + programmable through I2C and 4-wire interfaces. The sensor output is > >>>>>>>> + available via CSI-2 serial data output (1 Lane). > >>>>>>>> + > >>>>>>>> +properties: > >>>>>>>> + compatible: > >>>>>>>> + const: sony,imx296 > >>>>>>>> + > >>>>>>>> + reg: > >>>>>>>> + maxItems: 1 > >>>>>>>> + > >>>>>>>> + clocks: > >>>>>>>> + maxItems: 1 > >>>>>>>> + > >>>>>>>> + clock-names: > >>>>>>>> + description: > >>>>>>>> + Input clock for the sensor. > >>>>>>>> + items: > >>>>>>>> + - const: mclk > >>>>>>> > >>>>>>> The pin is named INCK, let's name the clock accordingly. > >>>>>> > >>>>>> Okay, I thought generic names are preferred here! > >>>>>> > >>>>>>>> + clock-frequency: > >>>>>>>> + description: > >>>>>>>> + Frequency of the mclk clock in Hertz. > >>>>>>> > >>>>>>> This shouldn't be needed, you can retrieve the clock frequency at > >>>>>>> runtime from the clock source. > >>>>>> > >>>>>> Unless the clock source is a fixed one! What if the clock source comes from > >>>>>> SoC? We need to set the rate, right? > >>>>> > >>>>> In that case, if you want to hardcode the clock in DT, the preferred way > >>>>> is to use the assigned-clock-rates property. Otherwise, if the driver > >>>>> requires a specific clock frequency, it's better to hardcode it in the > >>>>> driver itself. In this specific case, I think assigned-clock-rates is > >>>>> best as the device can support three different clock frequencies. > >>>> > >>>> Just note that if ACPI support is added to the sensor driver, you'll need > >>>> the clock-frequency property again, for that's the only way how the driver > >>>> will get the clock frequency. > >>> > >>> Why is so ? Why can't we implement of assigned-clock-rates for ACPI ? > >> > >> ACPI doesn't deal with clocks as such. So there's also no ACPI defined way > >> to access clocks specifically, including the frequency --- instead the > >> clock is controlled by an AML methods which implement power on and off > >> sequences for the device. > > > > It's a shortcoming of ACPI, which should be addressed at the ACPI level. > > We shouldn't polute the DT bindings with a clock-frequency property for > > this reason. > > It's really not a shortcoming but a design decision: what belongs to the > scope of the firmware? And in this case system and device power management > implementation is included. I do not believe this will be revisited in any > foreseeable future, i.e. there will be no clock control interface for ACPI. I'm not saying there's a need to control clocks, but if the driver of the camera sensor needs to know the frequency of an externally supplied clock, the firmware should give a way for the operating system to retrieve the clock frequency. Doing so with a clock-frequency in the sensor node is a hack to work around a limitation of ACPI, as the Linux model to retrieve clock frequencies it to interogate the clock provider. > Explicitly stating the frequency also has an added benefit: the driver > can be certain that the given frequency is intended to be used on the > board. Otherwise the frequency could have been changed by e.g. another > driver. This does matter, as the frequency determines which link > frequencies can be achieved, and as the two effectively have to be > compliant, an unintended external clock frequency also means there will be > no match between possible link frequencies and configured link frequencies. This doesn't solve anything, quite the contrary. With assigned-clock-rates the frequency is set once in the firmware, the operating system configures the clock provider to set the frequency, and the sensor driver then queries the provider. There's a single source of clock frequency information. With clock-frequency, you have two sources of information, the value of the property and the value set when programming the clock provider in the firmware (either in the BIOS/UEFI, or in the ACPI DSDT AML). That's a chance to get it wrong, and we both know how reliable firmware is. Furthermore, the clock-frequency property requires drivers to be informed of firmware details. On DT-based systems they should use clk_get_rate(), while on ACPI-based systems they should read the clock-frequency property. If you want to support ACPI, this should be hidden by the firmware, with retrieval of clock frequency from the firmware handled in core code. One option would be to create fixed clocks automatically for ACPI devices that report clock frequency through an ACPI-specific (as in defined outside of the ACPI standard, through DSD for instance) mean. Drivers would then be able to call clk_get() and clk_get_rate(). > I.e. no images to capture either. > > That said, I don't know if this has been a practical issue in the past. -- Regards, Laurent Pinchart
On Wed, Oct 30, 2019 at 05:31:05PM +0530, Manivannan Sadhasivam wrote: > Hi Sakari, > > On Wed, Oct 30, 2019 at 01:53:28PM +0200, Sakari Ailus wrote: > > Hi Nabuvannan, > > > > On Wed, Oct 30, 2019 at 03:19:01PM +0530, Manivannan Sadhasivam wrote: > > > Add YAML devicetree binding for IMX296 CMOS image sensor. Let's also > > > add MAINTAINERS entry for the binding and driver. > > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > .../devicetree/bindings/media/i2c/imx296.yaml | 94 +++++++++++++++++++ > > > MAINTAINERS | 8 ++ > > > 2 files changed, 102 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx296.yaml b/Documentation/devicetree/bindings/media/i2c/imx296.yaml > > > new file mode 100644 > > > index 000000000000..c04ec2203268 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/i2c/imx296.yaml > > > @@ -0,0 +1,94 @@ > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/media/i2c/imx296.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Sony IMX296 1/2.8-Inch CMOS Image Sensor > > > + > > > +maintainers: > > > + - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > + > > > +description: |- > > > + The Sony IMX296 is a 1/2.9-Inch active pixel type CMOS Solid-state image > > > + sensor with square pixel array and 1.58 M effective pixels. This chip > > > + features a global shutter with variable charge-integration time. It is > > > + programmable through I2C and 4-wire interfaces. The sensor output is > > > + available via CSI-2 serial data output (1 Lane). > > > + > > > +properties: > > > + compatible: > > > + const: sony,imx296 > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + clocks: > > > + maxItems: 1 > > > + > > > + clock-names: > > > + description: > > > + Input clock for the sensor. > > > + items: > > > + - const: mclk > > > + > > > + clock-frequency: > > > + description: > > > + Frequency of the mclk clock in Hertz. > > > + > > > + vddo-supply: > > > + description: > > > + Definition of the regulator used as interface power supply. > > > + > > > + vdda-supply: > > > + description: > > > + Definition of the regulator used as analog power supply. > > > + > > > + vddd-supply: > > > + description: > > > + Definition of the regulator used as digital power supply. > > > + > > > + reset-gpios: > > > + description: > > > + The phandle and specifier for the GPIO that controls sensor reset. > > > + maxItems: 1 > > > + > > > + port: true > > > > You're missing "type: object" under port. > > > > I did that intentionally, since there are other places where I can see the > "type" field not specified. So, I was not sure about that. Most of the > display bindings don't specify "type" and they are most available ones. > I don't think the "port" property differs between cameras and displays. > So I went with that. The difference is the panel bindings have a common schema included which defines 'port' at least as a node. I don't think an include would help too much here, so probably best to add 'type: object' for now. Either way, this may change once video-interfaces.txt is converted if any of those properties apply here. Either way: Reviewed-by: Rob Herring <robh@kernel.org> Rob
diff --git a/Documentation/devicetree/bindings/media/i2c/imx296.yaml b/Documentation/devicetree/bindings/media/i2c/imx296.yaml new file mode 100644 index 000000000000..c04ec2203268 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/imx296.yaml @@ -0,0 +1,94 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/imx296.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Sony IMX296 1/2.8-Inch CMOS Image Sensor + +maintainers: + - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> + +description: |- + The Sony IMX296 is a 1/2.9-Inch active pixel type CMOS Solid-state image + sensor with square pixel array and 1.58 M effective pixels. This chip + features a global shutter with variable charge-integration time. It is + programmable through I2C and 4-wire interfaces. The sensor output is + available via CSI-2 serial data output (1 Lane). + +properties: + compatible: + const: sony,imx296 + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + description: + Input clock for the sensor. + items: + - const: mclk + + clock-frequency: + description: + Frequency of the mclk clock in Hertz. + + vddo-supply: + description: + Definition of the regulator used as interface power supply. + + vdda-supply: + description: + Definition of the regulator used as analog power supply. + + vddd-supply: + description: + Definition of the regulator used as digital power supply. + + reset-gpios: + description: + The phandle and specifier for the GPIO that controls sensor reset. + maxItems: 1 + + port: true + +required: + - compatible + - reg + - clocks + - clock-names + - clock-frequency + - vddo-supply + - vdda-supply + - vddd-supply + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + imx296: camera-sensor@1a { + compatible = "sony,imx296"; + reg = <0x1a>; + reset-gpios = <&msmgpio 35 GPIO_ACTIVE_LOW>; + pinctrl-names = "default"; + pinctrl-0 = <&camera_rear_default>; + clocks = <&gcc 90>; + clock-names = "mclk"; + clock-frequency = <37125000>; + vddo-supply = <&camera_vddo_1v8>; + vdda-supply = <&camera_vdda_3v3>; + vddd-supply = <&camera_vddd_1v2>; + + port { + imx296_ep: endpoint { + remote-endpoint = <&csiphy0_ep>; + }; + }; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index 55199ef7fa74..51194bb2c392 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -15140,6 +15140,14 @@ S: Maintained F: drivers/media/i2c/imx274.c F: Documentation/devicetree/bindings/media/i2c/imx274.txt +SONY IMX296 SENSOR DRIVER +M: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> +L: linux-media@vger.kernel.org +T: git git://linuxtv.org/media_tree.git +S: Maintained +F: drivers/media/i2c/imx296.c +F: Documentation/devicetree/bindings/media/i2c/imx296.yaml + SONY IMX319 SENSOR DRIVER M: Bingbu Cao <bingbu.cao@intel.com> L: linux-media@vger.kernel.org
Add YAML devicetree binding for IMX296 CMOS image sensor. Let's also add MAINTAINERS entry for the binding and driver. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- .../devicetree/bindings/media/i2c/imx296.yaml | 94 +++++++++++++++++++ MAINTAINERS | 8 ++ 2 files changed, 102 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.yaml -- 2.17.1