| Message ID | 20191030094902.32582-1-manivannan.sadhasivam@linaro.org |
|---|---|
| Headers | show |
| Series | Add IMX296 CMOS image sensor support | expand |
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. -- Regards, Sakari Ailus
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. > + 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. > + 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. > + 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 Mani, Thank you for the patches. On Wed, Oct 30, 2019 at 03:19:00PM +0530, Manivannan Sadhasivam wrote: > Hello, > > This patchset adds support for IMX296 CMOS image sensor from Sony. > Sensor can be programmed through I2C and 4-wire interface but the > current driver only supports I2C interface. The sensor is > capable of outputting frames in CSI2 format (1 Lane). In the case > of sensor resolution, driver only supports 1440x1088 at 30 FPS. > > The driver has been validated using Framos IMX296 module interfaced to > 96Boards Dragonboard410c. I've just been made aware of your work. I also worked on an IMX296 sensor driver in parallel, which I will post to the list. My driver doesn't hardcode the resolution but computes register values at runtime, so I wonder if it could be a better option. I'll post it now. > Changes in v4: > > * Fixed issues related to gain settings and few misc cleanups in driver > * Documented port node and removed maxItems, default prop from dt binding > as per the review > > Changes in v3: > > * Fixed the reference to video-interfaces.txt in binding. > > Changes in v2: > > * Switched to YAML binding > > Manivannan Sadhasivam (2): > dt-bindings: media: i2c: Add IMX296 CMOS sensor binding > media: i2c: Add IMX296 CMOS image sensor driver > > .../devicetree/bindings/media/i2c/imx296.yaml | 94 +++ > MAINTAINERS | 8 + > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/imx296.c | 715 ++++++++++++++++++ > 5 files changed, 829 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.yaml > create mode 100644 drivers/media/i2c/imx296.c -- Regards, Laurent Pinchart
Hi Laurent, On Thu, Oct 31, 2019 at 03:16:44PM +0200, Laurent Pinchart wrote: > Hi Mani, > > Thank you for the patches. > > On Wed, Oct 30, 2019 at 03:19:00PM +0530, Manivannan Sadhasivam wrote: > > Hello, > > > > This patchset adds support for IMX296 CMOS image sensor from Sony. > > Sensor can be programmed through I2C and 4-wire interface but the > > current driver only supports I2C interface. The sensor is > > capable of outputting frames in CSI2 format (1 Lane). In the case > > of sensor resolution, driver only supports 1440x1088 at 30 FPS. > > > > The driver has been validated using Framos IMX296 module interfaced to > > 96Boards Dragonboard410c. > > I've just been made aware of your work. I also worked on an IMX296 > sensor driver in parallel, which I will post to the list. My driver > doesn't hardcode the resolution but computes register values at runtime, > so I wonder if it could be a better option. I'll post it now. > I'm fine with it. The reason the driver is simple in the first place is, that's how my usual workflow is. Start small and build it big ;-) Anyway, I'm happy if your driver gets in. Thanks, Mani > > Changes in v4: > > > > * Fixed issues related to gain settings and few misc cleanups in driver > > * Documented port node and removed maxItems, default prop from dt binding > > as per the review > > > > Changes in v3: > > > > * Fixed the reference to video-interfaces.txt in binding. > > > > Changes in v2: > > > > * Switched to YAML binding > > > > Manivannan Sadhasivam (2): > > dt-bindings: media: i2c: Add IMX296 CMOS sensor binding > > media: i2c: Add IMX296 CMOS image sensor driver > > > > .../devicetree/bindings/media/i2c/imx296.yaml | 94 +++ > > MAINTAINERS | 8 + > > drivers/media/i2c/Kconfig | 11 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/imx296.c | 715 ++++++++++++++++++ > > 5 files changed, 829 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.yaml > > create mode 100644 drivers/media/i2c/imx296.c > > -- > Regards, > > Laurent Pinchart
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. > >> + 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. Should we then go for one supply, and add the other two when (and if) needed ? > >> + 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. > Agree. assigned-clock* properties makes sense for multiple frequencies. In my driver, I only used one frequency so I was happy with clock-frequency :) > > >> + 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. > > Should we then go for one supply, and add the other two when (and if) > needed ? > I'm not really sure if we should use one power supply here. The single power supply configuration is not true for all cases. And following what other sensors are using, I'd prefer to have 3 individual power supplies. 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 Mani, Laurent, On Thu, Oct 31, 2019 at 03:28:01PM +0200, Laurent Pinchart wrote: > Hi Mani, > > On Thu, Oct 31, 2019 at 06:53:52PM +0530, Manivannan Sadhasivam wrote: > > On Thu, Oct 31, 2019 at 03:16:44PM +0200, Laurent Pinchart wrote: > > > On Wed, Oct 30, 2019 at 03:19:00PM +0530, Manivannan Sadhasivam wrote: > > > > Hello, > > > > > > > > This patchset adds support for IMX296 CMOS image sensor from Sony. > > > > Sensor can be programmed through I2C and 4-wire interface but the > > > > current driver only supports I2C interface. The sensor is > > > > capable of outputting frames in CSI2 format (1 Lane). In the case > > > > of sensor resolution, driver only supports 1440x1088 at 30 FPS. > > > > > > > > The driver has been validated using Framos IMX296 module interfaced to > > > > 96Boards Dragonboard410c. > > > > > > I've just been made aware of your work. I also worked on an IMX296 > > > sensor driver in parallel, which I will post to the list. My driver > > > doesn't hardcode the resolution but computes register values at runtime, > > > so I wonder if it could be a better option. I'll post it now. > > > > I'm fine with it. The reason the driver is simple in the first place is, that's > > how my usual workflow is. Start small and build it big ;-) > > > > Anyway, I'm happy if your driver gets in. > > My driver sometimes has trouble finding the sensor at probe time, so > I'll study and try your code too. It could be a problem specific to my > platform (I'm testing on a custom i.MX7 board). Based on this discussion I'll mark the second patch of the set obsolete in Patchwork. Laurent: please see my comments on the driver as well. -- Kind regards, Sakari Ailus
Hi Sakari, 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 ? > 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. -- Regards, Laurent Pinchart
Hi Laurent, On Thu, Oct 31, 2019 at 06:54:44PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > 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. -- Sakari Ailus
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. -- Regards, Laurent Pinchart