[PATCH v4 0/2] Add IMX296 CMOS image sensor support

[PATCH v4 0/2] Add IMX296 CMOS image sensor support

[Manivannan Sadhasivam]

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.

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

-- 
2.17.1

[PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Manivannan Sadhasivam]

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
+
+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
-- 
2.17.1

[PATCH v4 2/2] media: i2c: Add IMX296 CMOS image sensor driver

[Manivannan Sadhasivam]

Add driver for Sony IMX296 CMOS image sensor driver. The driver only
supports I2C interface for programming and MIPI CSI-2 for sensor output.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/media/i2c/Kconfig  |  11 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/imx296.c | 715 +++++++++++++++++++++++++++++++++++++
 3 files changed, 727 insertions(+)
 create mode 100644 drivers/media/i2c/imx296.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 7eee1812bba3..930db46fa2b8 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -598,6 +598,17 @@ config VIDEO_IMX274
 	  This is a V4L2 sensor driver for the Sony IMX274
 	  CMOS image sensor.
 
+config VIDEO_IMX296
+	tristate "Sony IMX296 sensor support"
+	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+	select V4L2_FWNODE
+	help
+	  This is a Video4Linux2 sensor driver for the Sony
+	  IMX296 camera sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called imx296.
+
 config VIDEO_IMX319
 	tristate "Sony IMX319 sensor support"
 	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index beb170b002dc..ed0c02bda450 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -112,6 +112,7 @@ obj-$(CONFIG_VIDEO_TC358743)	+= tc358743.o
 obj-$(CONFIG_VIDEO_IMX214)	+= imx214.o
 obj-$(CONFIG_VIDEO_IMX258)	+= imx258.o
 obj-$(CONFIG_VIDEO_IMX274)	+= imx274.o
+obj-$(CONFIG_VIDEO_IMX296)	+= imx296.o
 obj-$(CONFIG_VIDEO_IMX319)	+= imx319.o
 obj-$(CONFIG_VIDEO_IMX355)	+= imx355.o
 obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
diff --git a/drivers/media/i2c/imx296.c b/drivers/media/i2c/imx296.c
new file mode 100644
index 000000000000..1e510c42c117
--- /dev/null
+++ b/drivers/media/i2c/imx296.c
@@ -0,0 +1,715 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Sony IMX296 CMOS Image Sensor Driver
+ *
+ * Copyright (C) 2019 FRAMOS GmbH.
+ *
+ * Copyright (C) 2019 Linaro Ltd.
+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <media/media-entity.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+#define IMX296_STANDBY 0x3000
+#define IMX296_REGHOLD 0x3008
+#define IMX296_XMSTA 0x300a
+#define IMX296_GAIN_LOW 0x3204
+#define IMX296_GAIN_HIGH 0x3205
+
+#define IMX296_DEFAULT_FORMAT MEDIA_BUS_FMT_SRGGB10_1X10
+
+static const char * const imx296_supply_name[] = {
+	"vdda",
+	"vddd",
+	"vddo",
+};
+
+#define IMX296_NUM_SUPPLIES ARRAY_SIZE(imx296_supply_name)
+
+struct imx296_regval {
+	u16 reg;
+	u8 val;
+};
+
+struct imx296_mode {
+	u32 width;
+	u32 height;
+	u32 pixel_rate;
+
+	const struct imx296_regval *data;
+	u32 data_size;
+};
+
+struct imx296 {
+	struct device *dev;
+	struct clk *mclk;
+	struct regmap *regmap;
+
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+	struct v4l2_mbus_framefmt current_format;
+	const struct imx296_mode *current_mode;
+
+	struct regulator_bulk_data supplies[IMX296_NUM_SUPPLIES];
+	struct gpio_desc *rst_gpio;
+
+	struct v4l2_ctrl_handler ctrls;
+	struct v4l2_ctrl *pixel_rate;
+
+	struct mutex lock;
+};
+
+static const struct regmap_config imx296_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static const struct imx296_regval imx296_global_init_settings[] = {
+	{ 0x3a00, 0x80 },
+	{ 0x3005, 0xf0 },
+	{ 0x350b, 0x0f },
+	{ 0x300d, 0x00 },
+	{ 0x400e, 0x58 },
+	{ 0x3010, 0x5e },
+	{ 0x3011, 0x04 },
+	{ 0x3014, 0x4c },
+	{ 0x4014, 0x1c },
+	{ 0x3015, 0x04 },
+	{ 0x3516, 0x77 },
+	{ 0x321a, 0x00 },
+	{ 0x3226, 0x02 },
+	{ 0x3832, 0xf5 },
+	{ 0x3833, 0x00 },
+	{ 0x3541, 0x72 },
+	{ 0x4041, 0x2a },
+	{ 0x3d48, 0xa3 },
+	{ 0x3d49, 0x00 },
+	{ 0x3d4a, 0x85 },
+	{ 0x3d4b, 0x00 },
+	{ 0x3256, 0x01 },
+	{ 0x3758, 0xa3 },
+	{ 0x3759, 0x00 },
+	{ 0x375a, 0x85 },
+	{ 0x375b, 0x00 },
+	{ 0x3165, 0x00 },
+	{ 0x3169, 0x10 },
+	{ 0x316a, 0x02 },
+	{ 0x4174, 0x00 },
+	{ 0x3079, 0x08 },
+	{ 0x3090, 0x04 },
+	{ 0x3094, 0x04 },
+	{ 0x3098, 0x04 },
+	{ 0x309e, 0x04 },
+	{ 0x30a0, 0x04 },
+	{ 0x30a1, 0x3c },
+	{ 0x38a2, 0xf6 },
+	{ 0x40a2, 0x06 },
+	{ 0x38a3, 0x00 },
+	{ 0x30a4, 0x5f },
+	{ 0x30a8, 0x91 },
+	{ 0x30ac, 0x28 },
+	{ 0x30af, 0x09 },
+	{ 0x40c1, 0xf6 },
+	{ 0x40c7, 0x0f },
+	{ 0x31c8, 0xf3 },
+	{ 0x40c8, 0x00 },
+	{ 0x31d0, 0xf4 },
+	{ 0x30df, 0x00 },
+};
+
+static const struct imx296_regval imx296_1440_1088_settings[] = {
+	{ 0x300d, 0x00 },
+	{ 0x3010, 0xcb },
+	{ 0x3011, 0x08 },
+	{ 0x3014, 0x4c },
+	{ 0x3015, 0x04 },
+	{ 0x3300, 0x03 },
+	{ 0x3310, 0x08 },
+	{ 0x3311, 0x00 },
+	{ 0x3312, 0x00 },
+	{ 0x3313, 0x00 },
+	{ 0x3314, 0xa0 },
+	{ 0x3315, 0x05 },
+	{ 0x3316, 0x40 },
+	{ 0x3317, 0x04 },
+
+	{ 0x3204, 0x00 },
+	{ 0x3205, 0x00 },
+	{ 0x3212, 0x08 },
+	{ 0x3254, 0x3c },
+	{ 0x3255, 0x00 },
+	{ 0x3089, 0x80 },
+	{ 0x308a, 0x0b },
+	{ 0x308b, 0x80 },
+	{ 0x308c, 0x08 },
+	{ 0x418c, 0x74 },
+	{ 0x308d, 0x0e },
+	{ 0x308e, 0x00 },
+	{ 0x308f, 0x00 },
+};
+
+/* Mode configs */
+static const struct imx296_mode imx296_modes[] = {
+	{
+		.width = 1440,
+		.height = 1088,
+		.data = imx296_1440_1088_settings,
+		.data_size = ARRAY_SIZE(imx296_1440_1088_settings),
+		.pixel_rate = 80000000,
+	},
+};
+
+static inline struct imx296 *to_imx296(struct v4l2_subdev *_sd)
+{
+	return container_of(_sd, struct imx296, sd);
+}
+
+static inline int imx296_read_reg(struct imx296 *imx296, u16 addr, u8 *value)
+{
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read(imx296->regmap, addr, &regval);
+	if (ret) {
+		dev_err(imx296->dev, "I2C read failed for addr: %x\n", addr);
+		return ret;
+	}
+
+	*value = regval & 0xff;
+
+	return 0;
+}
+
+static int imx296_write_reg(struct imx296 *imx296, u16 addr, u8 value)
+{
+	int ret;
+
+	ret = regmap_write(imx296->regmap, addr, value);
+	if (ret) {
+		dev_err(imx296->dev, "I2C write failed for addr: %x\n", addr);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int imx296_set_register_array(struct imx296 *imx296,
+				     const struct imx296_regval *settings,
+				     unsigned int num_settings)
+{
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < num_settings; ++i, ++settings) {
+		ret = imx296_write_reg(imx296, settings->reg, settings->val);
+		if (ret < 0)
+			return ret;
+	}
+
+	msleep(10);
+
+	return 0;
+}
+
+static int imx296_write_buffered_reg(struct imx296 *imx296, u16 address_low,
+				     u8 nr_regs, u32 value)
+{
+	unsigned int i;
+	int ret;
+
+	ret = imx296_write_reg(imx296, IMX296_REGHOLD, 0x01);
+	if (ret) {
+		dev_err(imx296->dev, "Error setting hold register\n");
+		return ret;
+	}
+
+	for (i = 0; i < nr_regs; i++) {
+		ret = imx296_write_reg(imx296, address_low + i,
+				       (u8)(value >> (i * 8)));
+		if (ret) {
+			dev_err(imx296->dev, "Error writing buffered registers\n");
+			return ret;
+		}
+	}
+
+	ret = imx296_write_reg(imx296, IMX296_REGHOLD, 0x00);
+	if (ret) {
+		dev_err(imx296->dev, "Error setting hold register\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+static inline int imx296_set_gain(struct imx296 *imx296, u32 value)
+{
+	return imx296_write_buffered_reg(imx296, IMX296_GAIN_LOW, 2, value);
+}
+
+/* Stop streaming */
+static int imx296_stop_streaming(struct imx296 *imx296)
+{
+	int ret;
+
+	ret = imx296_write_reg(imx296, IMX296_STANDBY, 0x01);
+	if (ret < 0)
+		return ret;
+
+	msleep(30);
+
+	return imx296_write_reg(imx296, IMX296_XMSTA, 0x01);
+}
+
+static int imx296_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct imx296 *imx296 = container_of(ctrl->handler,
+					     struct imx296, ctrls);
+	int ret = 0;
+
+	/* V4L2 controls values will be applied only when power is already up */
+	if (!pm_runtime_get_if_in_use(imx296->dev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_GAIN:
+		ret = imx296_set_gain(imx296, ctrl->val);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	pm_runtime_put(imx296->dev);
+
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops imx296_ctrl_ops = {
+	.s_ctrl = imx296_set_ctrl,
+};
+
+static int imx296_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	code->code = IMX296_DEFAULT_FORMAT;
+
+	return 0;
+}
+
+static int imx296_get_fmt(struct v4l2_subdev *sd,
+			  struct v4l2_subdev_pad_config *cfg,
+			  struct v4l2_subdev_format *fmt)
+{
+	struct imx296 *imx296 = to_imx296(sd);
+	struct v4l2_mbus_framefmt *framefmt;
+
+	mutex_lock(&imx296->lock);
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
+		framefmt = v4l2_subdev_get_try_format(&imx296->sd, cfg,
+						      fmt->pad);
+	else
+		framefmt = &imx296->current_format;
+
+	fmt->format = *framefmt;
+
+	mutex_unlock(&imx296->lock);
+
+	return 0;
+}
+
+static int imx296_set_fmt(struct v4l2_subdev *sd,
+			  struct v4l2_subdev_pad_config *cfg,
+			  struct v4l2_subdev_format *fmt)
+{
+	struct imx296 *imx296 = to_imx296(sd);
+	const struct imx296_mode *mode;
+	struct v4l2_mbus_framefmt *format;
+	int ret = 0;
+
+	mutex_lock(&imx296->lock);
+
+	mode = v4l2_find_nearest_size(imx296_modes,
+				      ARRAY_SIZE(imx296_modes),
+				      width, height,
+				      fmt->format.width, fmt->format.height);
+
+	fmt->format.width = mode->width;
+	fmt->format.height = mode->height;
+
+	fmt->format.code = IMX296_DEFAULT_FORMAT;
+	fmt->format.field = V4L2_FIELD_NONE;
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		format = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
+	} else {
+		format = &imx296->current_format;
+		__v4l2_ctrl_s_ctrl_int64(imx296->pixel_rate, mode->pixel_rate);
+
+		imx296->current_mode = mode;
+	}
+
+	*format = fmt->format;
+
+	mutex_unlock(&imx296->lock);
+
+	return ret;
+}
+
+static int imx296_entity_init_cfg(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_pad_config *cfg)
+{
+	struct v4l2_subdev_format fmt = { 0 };
+
+	fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
+	fmt.format.width = 1440;
+	fmt.format.height = 1088;
+
+	imx296_set_fmt(subdev, cfg, &fmt);
+
+	return 0;
+}
+
+/* Start streaming */
+static int imx296_start_streaming(struct imx296 *imx296)
+{
+	int ret;
+
+	/* Set init register settings */
+	ret = imx296_set_register_array(imx296, imx296_global_init_settings,
+				ARRAY_SIZE(imx296_global_init_settings));
+	if (ret < 0) {
+		dev_err(imx296->dev, "Could not set init registers\n");
+		return ret;
+	}
+
+	/* Apply default values of current mode */
+	ret = imx296_set_register_array(imx296, imx296->current_mode->data,
+					imx296->current_mode->data_size);
+	if (ret < 0) {
+		dev_err(imx296->dev, "Could not set current mode\n");
+		return ret;
+	}
+
+	/* Apply customized values from user */
+	ret = v4l2_ctrl_handler_setup(imx296->sd.ctrl_handler);
+	if (ret) {
+		dev_err(imx296->dev, "Could not sync v4l2 controls\n");
+		return ret;
+	}
+
+	ret = imx296_write_reg(imx296, IMX296_STANDBY, 0x00);
+	if (ret < 0)
+		return ret;
+
+	msleep(30);
+
+	/* Start streaming */
+	return imx296_write_reg(imx296, IMX296_XMSTA, 0x00);
+}
+
+static int imx296_set_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct imx296 *imx296 = to_imx296(sd);
+	int ret;
+
+	if (enable) {
+		ret = pm_runtime_get_sync(imx296->dev);
+		if (ret < 0) {
+			pm_runtime_put_noidle(imx296->dev);
+			return ret;
+		}
+
+		ret = imx296_start_streaming(imx296);
+		if (ret) {
+			dev_err(imx296->dev, "Start stream failed\n");
+			pm_runtime_put(imx296->dev);
+			return ret;
+		}
+	} else {
+		imx296_stop_streaming(imx296);
+		pm_runtime_put(imx296->dev);
+	}
+
+	return 0;
+}
+
+static int imx296_get_regulators(struct device *dev, struct imx296 *imx296)
+{
+	unsigned int i;
+
+	for (i = 0; i < IMX296_NUM_SUPPLIES; i++)
+		imx296->supplies[i].supply = imx296_supply_name[i];
+
+	return devm_regulator_bulk_get(dev, IMX296_NUM_SUPPLIES,
+				       imx296->supplies);
+}
+
+static int imx296_power_on(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx296 *imx296 = to_imx296(sd);
+	int ret;
+
+	ret = regulator_bulk_enable(IMX296_NUM_SUPPLIES, imx296->supplies);
+	if (ret) {
+		dev_err(imx296->dev, "Failed to enable regulators\n");
+		return ret;
+	}
+
+	usleep_range(1, 2);
+
+	gpiod_set_value_cansleep(imx296->rst_gpio, 1);
+
+	usleep_range(1, 2);
+
+	ret = clk_prepare_enable(imx296->mclk);
+	if (ret) {
+		dev_err(imx296->dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	usleep_range(30000, 31000);
+
+	return 0;
+}
+
+static int imx296_power_off(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx296 *imx296 = to_imx296(sd);
+
+	clk_disable_unprepare(imx296->mclk);
+	gpiod_set_value_cansleep(imx296->rst_gpio, 0);
+	regulator_bulk_disable(IMX296_NUM_SUPPLIES, imx296->supplies);
+
+	return 0;
+}
+
+static const struct dev_pm_ops imx296_pm_ops = {
+	SET_RUNTIME_PM_OPS(imx296_power_on, imx296_power_off, NULL)
+};
+
+static const struct v4l2_subdev_video_ops imx296_video_ops = {
+	.s_stream = imx296_set_stream,
+};
+
+static const struct v4l2_subdev_pad_ops imx296_pad_ops = {
+	.init_cfg = imx296_entity_init_cfg,
+	.enum_mbus_code = imx296_enum_mbus_code,
+	.get_fmt = imx296_get_fmt,
+	.set_fmt = imx296_set_fmt,
+};
+
+static const struct v4l2_subdev_ops imx296_subdev_ops = {
+	.video = &imx296_video_ops,
+	.pad = &imx296_pad_ops,
+};
+
+static const struct media_entity_operations imx296_subdev_entity_ops = {
+	.link_validate = v4l2_subdev_link_validate,
+};
+
+static int imx296_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct fwnode_handle *endpoint;
+	struct imx296 *imx296;
+	struct v4l2_fwnode_endpoint ep = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY
+	};
+	u32 mclk_freq;
+	int ret;
+
+	imx296 = devm_kzalloc(dev, sizeof(*imx296), GFP_KERNEL);
+	if (!imx296)
+		return -ENOMEM;
+
+	imx296->dev = dev;
+	imx296->regmap = devm_regmap_init_i2c(client, &imx296_regmap_config);
+	if (IS_ERR(imx296->regmap)) {
+		dev_err(dev, "Unable to initialize I2C\n");
+		return -ENODEV;
+	}
+
+	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
+	if (!endpoint) {
+		dev_err(dev, "Endpoint node not found\n");
+		return -EINVAL;
+	}
+
+	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
+	fwnode_handle_put(endpoint);
+	if (ret) {
+		dev_err(dev, "Parsing endpoint node failed\n");
+		goto free_err;
+	}
+
+	/* Set default mode to max resolution */
+	imx296->current_mode = &imx296_modes[0];
+
+	/* get system clock (mclk) */
+	imx296->mclk = devm_clk_get(dev, "mclk");
+	if (IS_ERR(imx296->mclk)) {
+		dev_err(dev, "Could not get mclk");
+		ret = PTR_ERR(imx296->mclk);
+		goto free_err;
+	}
+
+	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
+				       &mclk_freq);
+	if (ret) {
+		dev_err(dev, "Could not get mclk frequency\n");
+		goto free_err;
+	}
+
+	/* external clock must be 37.125 MHz */
+	if (mclk_freq != 37125000) {
+		dev_err(dev, "External clock frequency %u is not supported\n",
+			mclk_freq);
+		ret = -EINVAL;
+		goto free_err;
+	}
+
+	ret = clk_set_rate(imx296->mclk, mclk_freq);
+	if (ret) {
+		dev_err(dev, "Could not set mclk frequency\n");
+		goto free_err;
+	}
+
+	ret = imx296_get_regulators(dev, imx296);
+	if (ret < 0) {
+		dev_err(dev, "Cannot get regulators\n");
+		goto free_err;
+	}
+
+	imx296->rst_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS);
+	if (IS_ERR(imx296->rst_gpio)) {
+		dev_err(dev, "Cannot get reset gpio\n");
+		ret = PTR_ERR(imx296->rst_gpio);
+		goto free_err;
+	}
+
+	mutex_init(&imx296->lock);
+
+	v4l2_ctrl_handler_init(&imx296->ctrls, 2);
+
+	v4l2_ctrl_new_std(&imx296->ctrls, &imx296_ctrl_ops,
+			  V4L2_CID_GAIN, 0, 48, 1, 0);
+
+	imx296->pixel_rate = v4l2_ctrl_new_std(&imx296->ctrls, &imx296_ctrl_ops,
+					       V4L2_CID_PIXEL_RATE, 1,
+					       INT_MAX, 1,
+					       imx296_modes[0].pixel_rate);
+
+	imx296->sd.ctrl_handler = &imx296->ctrls;
+
+	if (imx296->ctrls.error) {
+		dev_err(dev, "Control initialization error %d\n",
+			imx296->ctrls.error);
+		ret = imx296->ctrls.error;
+		goto free_ctrl;
+	}
+
+	v4l2_i2c_subdev_init(&imx296->sd, client, &imx296_subdev_ops);
+	imx296->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	imx296->sd.dev = &client->dev;
+	imx296->sd.entity.ops = &imx296_subdev_entity_ops;
+	imx296->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
+	imx296->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ret = media_entity_pads_init(&imx296->sd.entity, 1, &imx296->pad);
+	if (ret < 0) {
+		dev_err(dev, "Could not register media entity\n");
+		goto free_ctrl;
+	}
+
+	ret = v4l2_async_register_subdev(&imx296->sd);
+	if (ret < 0) {
+		dev_err(dev, "Could not register v4l2 device\n");
+		goto free_entity;
+	}
+
+	/* Power on the device to match runtime PM state below */
+	ret = imx296_power_on(dev);
+	if (ret < 0) {
+		dev_err(dev, "Could not power on the device\n");
+		goto free_entity;
+	}
+
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_idle(dev);
+
+	v4l2_fwnode_endpoint_free(&ep);
+
+	return 0;
+
+free_entity:
+	media_entity_cleanup(&imx296->sd.entity);
+free_ctrl:
+	v4l2_ctrl_handler_free(&imx296->ctrls);
+	mutex_destroy(&imx296->lock);
+free_err:
+	v4l2_fwnode_endpoint_free(&ep);
+
+	return ret;
+}
+
+static int imx296_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx296 *imx296 = to_imx296(sd);
+
+	v4l2_async_unregister_subdev(sd);
+	media_entity_cleanup(&sd->entity);
+	v4l2_ctrl_handler_free(sd->ctrl_handler);
+
+	mutex_destroy(&imx296->lock);
+
+	pm_runtime_disable(imx296->dev);
+	if (!pm_runtime_status_suspended(imx296->dev))
+		imx296_power_off(imx296->dev);
+	pm_runtime_set_suspended(imx296->dev);
+
+	return 0;
+}
+
+static const struct of_device_id imx296_of_match[] = {
+	{ .compatible = "sony,imx296" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx296_of_match);
+
+static struct i2c_driver imx296_i2c_driver = {
+	.probe_new  = imx296_probe,
+	.remove = imx296_remove,
+	.driver = {
+		.name  = "imx296",
+		.pm = &imx296_pm_ops,
+		.of_match_table = imx296_of_match,
+	},
+};
+
+module_i2c_driver(imx296_i2c_driver);
+
+MODULE_DESCRIPTION("Sony IMX296 CMOS Image Sensor Driver");
+MODULE_AUTHOR("FRAMOS GmbH");
+MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Sakari Ailus]

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

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Manivannan Sadhasivam]

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

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Laurent Pinchart]

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

Re: [PATCH v4 0/2] Add IMX296 CMOS image sensor support

[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

Re: [PATCH v4 0/2] Add IMX296 CMOS image sensor support

[Manivannan Sadhasivam]

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

Re: [PATCH v4 0/2] Add IMX296 CMOS image sensor support

[Laurent Pinchart]

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).

> > > 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

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Manivannan Sadhasivam]

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

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[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

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Sakari Ailus]

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

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Manivannan Sadhasivam]

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

Re: [PATCH v4 0/2] Add IMX296 CMOS image sensor support

[Sakari Ailus]

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

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Laurent Pinchart]

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

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Sakari Ailus]

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

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Laurent Pinchart]

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

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[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

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Laurent Pinchart]

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

Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Rob Herring]

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