[v2 0/3] media: ov8856: Add devicetree support

[v2 0/3] media: ov8856: Add devicetree support

[Robert Foss]

This adds devicetree support to the ov8856 driver.
In order to to aid debugging and enable future sensor
modes to be supported, module revision detection is also added.

Dongchun Zhu (1):
  media: dt-bindings: ov8856: Document YAML bindings

Robert Foss (2):
  media: ov8856: Add devicetree support
  media: ov8856: Implement sensor module revision identification

 .../devicetree/bindings/media/i2c/ov8856.yaml | 133 +++++++++++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/ov8856.c                    | 151 +++++++++++++++++-
 3 files changed, 283 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[v2 1/3] media: dt-bindings: ov8856: Document YAML bindings

[Robert Foss]

From: Dongchun Zhu <dongchun.zhu@mediatek.com>

This patch adds documentation of device tree in YAML schema for the
OV8856 CMOS image sensor.

Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
Signed-off-by: Robert Foss <robert.foss@linaro.org>
---

- Changes since v4:
  * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description
  * Add clock-lanes property to example
  * robher: Fix syntax error in devicetree example

- Changes since v3:
  * robher: Fix syntax error
  * robher: Removed maxItems
  * Fixes yaml 'make dt-binding-check' errors

- Changes since v2:
  Fixes comments from from Andy, Tomasz, Sakari, Rob.
  * Convert text documentation to YAML schema.

- Changes since v1:
  Fixes comments from Sakari, Tomasz
  * Add clock-frequency and link-frequencies in DT

 .../devicetree/bindings/media/i2c/ov8856.yaml | 133 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 134 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
new file mode 100644
index 000000000000..f5cb9add9277
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
@@ -0,0 +1,133 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (c) 2019 MediaTek Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV8856 CMOS Sensor Device Tree Bindings
+
+maintainers:
+  - Ben Kao <ben.kao@intel.com>
+  - Dongchun Zhu <dongchun.zhu@mediatek.com>
+
+description: |-
+  The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS
+  image sensor that delivers 3264x2448 at 30fps. It provides full-frame,
+  sub-sampled, and windowed 10-bit MIPI images in various formats via the
+  Serial Camera Control Bus (SCCB) interface. This chip is programmable
+  through I2C and two-wire SCCB. The sensor output is available via CSI-2
+  serial data output (up to 4-lane).
+
+properties:
+  compatible:
+    const: ovti,ov8856
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    description:
+      Input clock for the sensor.
+    items:
+      - const: xvclk
+
+  clock-frequency:
+    description:
+      Frequency of the xvclk clock in Hertz.
+
+  dovdd-supply:
+    description:
+      Definition of the regulator used as interface power supply.
+
+  avdd-supply:
+    description:
+      Definition of the regulator used as analog power supply.
+
+  dvdd-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.
+      This corresponds to the hardware pin XSHUTDOWN which is physically
+      active low.
+
+  port:
+    type: object
+    additionalProperties: false
+    description:
+      A node containing input and output port nodes with endpoint definitions
+      as documented in
+      Documentation/devicetree/bindings/media/video-interfaces.txt
+
+    properties:
+      endpoint:
+        type: object
+
+        properties:
+          clock-lanes:
+            maxItems: 1
+
+          data-lanes:
+            maxItems: 1
+
+          remote-endpoint: true
+
+        required:
+          - clock-lanes
+          - data-lanes
+          - remote-endpoint
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - clock-frequency
+  - dovdd-supply
+  - avdd-supply
+  - dvdd-supply
+  - reset-gpios
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/clock/qcom,camcc-sdm845.h>
+
+    ov8856: camera-sensor@10 {
+        compatible = "ovti,ov8856";
+        reg = <0x10>;
+        reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&clk_24m_cam>;
+
+        clocks = <&clock_camcc CAM_CC_MCLK0_CLK>;
+        clock-names = "xvclk";
+        clock-frequency = <19200000>;
+
+        avdd-supply = <&mt6358_vcama2_reg>;
+        dvdd-supply = <&mt6358_vcamd_reg>;
+        dovdd-supply = <&mt6358_vcamio_reg>;
+
+        port {
+            wcam_out: endpoint {
+                remote-endpoint = <&mipi_in_wcam>;
+                clock-lanes = <0>;
+                data-lanes = <1 2 3 4>;
+                link-frequencies = /bits/ 64 <360000000 180000000>;
+            };
+        };
+    };
+
+...
\ No newline at end of file
diff --git a/MAINTAINERS b/MAINTAINERS
index a6fbdf354d34..0f99e863978a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12355,6 +12355,7 @@ L:	linux-media@vger.kernel.org
 T:	git git://linuxtv.org/media_tree.git
 S:	Maintained
 F:	drivers/media/i2c/ov8856.c
+F:	Documentation/devicetree/bindings/media/i2c/ov8856.yaml
 
 OMNIVISION OV9650 SENSOR DRIVER
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[v2 2/3] media: ov8856: Add devicetree support

[Robert Foss]

Add devicetree match table, and enable ov8856_probe()
to initialize power, clocks and reset pins.

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---

- Changes since v1:
  * Fabio: Change n_shutdown_gpio name to reset_gpio
  * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change
  * Fabio: Remove empty line
  * Fabio: Remove real error from devm_gpiod_get() failures
  * Andy & Sakari: Make XVCLK optional since to not break ACPI
  * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
  * Sakari: Use XVCLK rate as provided by DT

 drivers/media/i2c/ov8856.c | 109 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 107 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 8655842af275..db61eed223e8 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -3,10 +3,13 @@
 
 #include <asm/unaligned.h>
 #include <linux/acpi.h>
+#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/regulator/consumer.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -19,6 +22,8 @@
 #define OV8856_LINK_FREQ_180MHZ		180000000ULL
 #define OV8856_SCLK			144000000ULL
 #define OV8856_MCLK			19200000
+#define OV8856_XVCLK_19_2		19200000
+#define OV8856_XVCLK_24			24000000
 #define OV8856_DATA_LANES		4
 #define OV8856_RGB_DEPTH		10
 
@@ -64,6 +69,12 @@
 
 #define to_ov8856(_sd)			container_of(_sd, struct ov8856, sd)
 
+static const char * const ov8856_supply_names[] = {
+	"dovdd",	/* Digital I/O power */
+	"avdd",		/* Analog power */
+	"dvdd",		/* Digital core power */
+};
+
 enum {
 	OV8856_LINK_FREQ_720MBPS,
 	OV8856_LINK_FREQ_360MBPS,
@@ -566,6 +577,10 @@ struct ov8856 {
 	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
 
+	struct clk		*xvclk;
+	struct gpio_desc	*reset_gpio;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)];
+
 	/* V4L2 Controls */
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *pixel_rate;
@@ -908,6 +923,46 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
+static int __ov8856_power_on(struct ov8856 *ov8856)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
+	int ret;
+
+	ret = clk_prepare_enable(ov8856->xvclk);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to enable xvclk\n");
+		return ret;
+	}
+
+	gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
+				    ov8856->supplies);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to enable regulators\n");
+		goto disable_clk;
+	}
+
+	gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW);
+
+	usleep_range(1500, 1800);
+
+	return 0;
+
+disable_clk:
+	clk_disable_unprepare(ov8856->xvclk);
+
+	return ret;
+}
+
+static void __ov8856_power_off(struct ov8856 *ov8856)
+{
+	gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
+	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
+			       ov8856->supplies);
+	clk_disable_unprepare(ov8856->xvclk);
+}
+
 static int __maybe_unused ov8856_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -1175,7 +1230,7 @@ static int ov8856_remove(struct i2c_client *client)
 static int ov8856_probe(struct i2c_client *client)
 {
 	struct ov8856 *ov8856;
-	int ret;
+	int i, ret;
 
 	ret = ov8856_check_hwcfg(&client->dev);
 	if (ret) {
@@ -1189,10 +1244,50 @@ static int ov8856_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
+	ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
+	if (PTR_ERR(ov8856->xvclk) == -ENOENT) {
+		dev_info(&client->dev, "xvclk clock not defined, continuing...\n");
+		ov8856->xvclk = NULL;
+	} else if (IS_ERR(ov8856->xvclk)) {
+		dev_err(&client->dev, "could not get xvclk clock (%ld)\n",
+			PTR_ERR(ov8856->xvclk));
+		return PTR_ERR(ov8856->xvclk);
+	}
+
+	ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
+		return ret;
+	}
+
+	ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
+					       GPIOD_OUT_HIGH);
+	if (IS_ERR(ov8856->reset_gpio)) {
+		dev_err(&client->dev, "failed to get reset-gpios\n");
+		return PTR_ERR(ov8856->reset_gpio);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
+		ov8856->supplies[i].supply = ov8856_supply_names[i];
+
+	ret = devm_regulator_bulk_get(&client->dev,
+				      ARRAY_SIZE(ov8856_supply_names),
+				      ov8856->supplies);
+	if (ret) {
+		dev_warn(&client->dev, "failed to get regulators\n");
+		return ret;
+	}
+
+	ret = __ov8856_power_on(ov8856);
+	if (ret) {
+		dev_warn(&client->dev, "failed to power on\n");
+		return ret;
+	}
+
 	ret = ov8856_identify_module(ov8856);
 	if (ret) {
 		dev_err(&client->dev, "failed to find sensor: %d", ret);
-		return ret;
+		goto probe_power_off;
 	}
 
 	mutex_init(&ov8856->mutex);
@@ -1238,6 +1333,9 @@ static int ov8856_probe(struct i2c_client *client)
 	v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
 	mutex_destroy(&ov8856->mutex);
 
+probe_power_off:
+	__ov8856_power_off(ov8856);
+
 	return ret;
 }
 
@@ -1254,11 +1352,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
 #endif
 
+static const struct of_device_id ov8856_of_match[] = {
+	{ .compatible = "ovti,ov8856" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ov8856_of_match);
+
 static struct i2c_driver ov8856_i2c_driver = {
 	.driver = {
 		.name = "ov8856",
 		.pm = &ov8856_pm_ops,
 		.acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
+		.of_match_table = ov8856_of_match,
 	},
 	.probe_new = ov8856_probe,
 	.remove = ov8856_remove,
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[v2 3/3] media: ov8856: Implement sensor module revision identification

[Robert Foss]

Query the sensor for its module revision, and compare it
to known revisions.
Currently only the '1B' revision has been added.

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
 drivers/media/i2c/ov8856.c | 54 +++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index db61eed223e8..39662d3d86dd 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -34,6 +34,18 @@
 #define OV8856_MODE_STANDBY		0x00
 #define OV8856_MODE_STREAMING		0x01
 
+/* define 1B module revision */
+#define OV8856_1B_MODULE		0x02
+
+/* the OTP read-out buffer is at 0x7000 and 0xf is the offset
+ * of the byte in the OTP that means the module revision
+ */
+#define OV8856_MODULE_REVISION		0x700f
+#define OV8856_OTP_MODE_CTRL		0x3d84
+#define OV8856_OTP_LOAD_CTRL		0x3d81
+#define OV8856_OTP_MODE_AUTO		0x00
+#define OV8856_OTP_LOAD_CTRL_ENABLE	BIT(0)
+
 /* vertical-timings from sensor */
 #define OV8856_REG_VTS			0x380e
 #define OV8856_VTS_MAX			0x7fff
@@ -711,6 +723,25 @@ static int ov8856_test_pattern(struct ov8856 *ov8856, u32 pattern)
 				OV8856_REG_VALUE_08BIT, pattern);
 }
 
+static int ov8856_check_revision(struct ov8856 *ov8856)
+{
+	int ret;
+
+	ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
+			       OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING);
+	if (ret)
+		return ret;
+
+	ret = ov8856_write_reg(ov8856, OV8856_OTP_MODE_CTRL,
+			       OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO);
+	if (ret)
+		return ret;
+
+	return ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
+				OV8856_REG_VALUE_08BIT,
+				OV8856_OTP_LOAD_CTRL_ENABLE);
+}
+
 static int ov8856_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct ov8856 *ov8856 = container_of(ctrl->handler,
@@ -1144,6 +1175,23 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
 		return -ENXIO;
 	}
 
+	/* check sensor hardware revision */
+	ret = ov8856_check_revision(ov8856);
+	if (ret) {
+		dev_err(&client->dev, "failed to check sensor revision");
+		return ret;
+	}
+
+	ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
+			      OV8856_REG_VALUE_08BIT, &val);
+	if (ret)
+		return ret;
+
+	dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n",
+		val,
+		val == OV8856_1B_MODULE ? "1B" : "unknown revision",
+		client->addr);
+
 	return 0;
 }
 
@@ -1254,12 +1302,6 @@ static int ov8856_probe(struct i2c_client *client)
 		return PTR_ERR(ov8856->xvclk);
 	}
 
-	ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24);
-	if (ret < 0) {
-		dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
-		return ret;
-	}
-
 	ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
 					       GPIOD_OUT_HIGH);
 	if (IS_ERR(ov8856->reset_gpio)) {
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 2/3] media: ov8856: Add devicetree support

[Sakari Ailus]

Hi Robert,

On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote:
> Add devicetree match table, and enable ov8856_probe()
> to initialize power, clocks and reset pins.
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
> 
> - Changes since v1:
>   * Fabio: Change n_shutdown_gpio name to reset_gpio
>   * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change
>   * Fabio: Remove empty line
>   * Fabio: Remove real error from devm_gpiod_get() failures
>   * Andy & Sakari: Make XVCLK optional since to not break ACPI
>   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
>   * Sakari: Use XVCLK rate as provided by DT
> 
>  drivers/media/i2c/ov8856.c | 109 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 107 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index 8655842af275..db61eed223e8 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -3,10 +3,13 @@
>  
>  #include <asm/unaligned.h>
>  #include <linux/acpi.h>
> +#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/regulator/consumer.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
> @@ -19,6 +22,8 @@
>  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
>  #define OV8856_SCLK			144000000ULL
>  #define OV8856_MCLK			19200000
> +#define OV8856_XVCLK_19_2		19200000

Please use a single macro to refer to 19,2 MHz clock.

> +#define OV8856_XVCLK_24			24000000

This doesn't seem to be needed 

>  #define OV8856_DATA_LANES		4
>  #define OV8856_RGB_DEPTH		10
>  
> @@ -64,6 +69,12 @@
>  
>  #define to_ov8856(_sd)			container_of(_sd, struct ov8856, sd)
>  
> +static const char * const ov8856_supply_names[] = {
> +	"dovdd",	/* Digital I/O power */
> +	"avdd",		/* Analog power */
> +	"dvdd",		/* Digital core power */
> +};
> +
>  enum {
>  	OV8856_LINK_FREQ_720MBPS,
>  	OV8856_LINK_FREQ_360MBPS,
> @@ -566,6 +577,10 @@ struct ov8856 {
>  	struct media_pad pad;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  
> +	struct clk		*xvclk;
> +	struct gpio_desc	*reset_gpio;
> +	struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)];
> +
>  	/* V4L2 Controls */
>  	struct v4l2_ctrl *link_freq;
>  	struct v4l2_ctrl *pixel_rate;
> @@ -908,6 +923,46 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
>  	return ret;
>  }
>  
> +static int __ov8856_power_on(struct ov8856 *ov8856)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> +	int ret;
> +
> +	ret = clk_prepare_enable(ov8856->xvclk);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to enable xvclk\n");
> +		return ret;
> +	}
> +
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> +				    ov8856->supplies);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to enable regulators\n");
> +		goto disable_clk;
> +	}
> +
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW);
> +
> +	usleep_range(1500, 1800);

I think you could omit the delay on ACPI based systems. Or just bail out
early in that case.

> +
> +	return 0;
> +
> +disable_clk:

How about the GPIO here?

> +	clk_disable_unprepare(ov8856->xvclk);
> +
> +	return ret;
> +}
> +
> +static void __ov8856_power_off(struct ov8856 *ov8856)
> +{
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> +	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> +			       ov8856->supplies);
> +	clk_disable_unprepare(ov8856->xvclk);
> +}

You'll need to call the two in the driver's suspend and resume functions.

> +
>  static int __maybe_unused ov8856_suspend(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> @@ -1175,7 +1230,7 @@ static int ov8856_remove(struct i2c_client *client)
>  static int ov8856_probe(struct i2c_client *client)
>  {
>  	struct ov8856 *ov8856;
> -	int ret;
> +	int i, ret;

unsigned int?

>  
>  	ret = ov8856_check_hwcfg(&client->dev);
>  	if (ret) {
> @@ -1189,10 +1244,50 @@ static int ov8856_probe(struct i2c_client *client)
>  		return -ENOMEM;
>  
>  	v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> +	ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> +	if (PTR_ERR(ov8856->xvclk) == -ENOENT) {
> +		dev_info(&client->dev, "xvclk clock not defined, continuing...\n");

How about dev_dbg()?

> +		ov8856->xvclk = NULL;
> +	} else if (IS_ERR(ov8856->xvclk)) {
> +		dev_err(&client->dev, "could not get xvclk clock (%ld)\n",
> +			PTR_ERR(ov8856->xvclk));
> +		return PTR_ERR(ov8856->xvclk);
> +	}
> +
> +	ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24);

This should either come from platform data, or perhaps it'd be even better
to get the clock rate and use assigned-clock-rates. I guess that's
preferred nowadays.

> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> +		return ret;
> +	}
> +
> +	ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
> +					       GPIOD_OUT_HIGH);

Indentation.

What if no gpio is defined?

> +	if (IS_ERR(ov8856->reset_gpio)) {
> +		dev_err(&client->dev, "failed to get reset-gpios\n");
> +		return PTR_ERR(ov8856->reset_gpio);
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> +		ov8856->supplies[i].supply = ov8856_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(&client->dev,
> +				      ARRAY_SIZE(ov8856_supply_names),
> +				      ov8856->supplies);

What happens if there are no regulators?

> +	if (ret) {
> +		dev_warn(&client->dev, "failed to get regulators\n");
> +		return ret;
> +	}
> +
> +	ret = __ov8856_power_on(ov8856);
> +	if (ret) {
> +		dev_warn(&client->dev, "failed to power on\n");
> +		return ret;
> +	}
> +
>  	ret = ov8856_identify_module(ov8856);
>  	if (ret) {
>  		dev_err(&client->dev, "failed to find sensor: %d", ret);
> -		return ret;
> +		goto probe_power_off;
>  	}
>  
>  	mutex_init(&ov8856->mutex);
> @@ -1238,6 +1333,9 @@ static int ov8856_probe(struct i2c_client *client)
>  	v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
>  	mutex_destroy(&ov8856->mutex);
>  
> +probe_power_off:
> +	__ov8856_power_off(ov8856);
> +

Also remember to power off the device in remove().

>  	return ret;
>  }
>  
> @@ -1254,11 +1352,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
>  MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
>  #endif
>  
> +static const struct of_device_id ov8856_of_match[] = {
> +	{ .compatible = "ovti,ov8856" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> +
>  static struct i2c_driver ov8856_i2c_driver = {
>  	.driver = {
>  		.name = "ov8856",
>  		.pm = &ov8856_pm_ops,
>  		.acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> +		.of_match_table = ov8856_of_match,
>  	},
>  	.probe_new = ov8856_probe,
>  	.remove = ov8856_remove,

-- 
Regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 1/3] media: dt-bindings: ov8856: Document YAML bindings

[Sakari Ailus]

Hi Robert,

On Fri, Mar 13, 2020 at 12:03:48PM +0100, Robert Foss wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> 
> This patch adds documentation of device tree in YAML schema for the
> OV8856 CMOS image sensor.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
> 
> - Changes since v4:
>   * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description
>   * Add clock-lanes property to example
>   * robher: Fix syntax error in devicetree example
> 
> - Changes since v3:
>   * robher: Fix syntax error
>   * robher: Removed maxItems
>   * Fixes yaml 'make dt-binding-check' errors
> 
> - Changes since v2:
>   Fixes comments from from Andy, Tomasz, Sakari, Rob.
>   * Convert text documentation to YAML schema.
> 
> - Changes since v1:
>   Fixes comments from Sakari, Tomasz
>   * Add clock-frequency and link-frequencies in DT
> 
>  .../devicetree/bindings/media/i2c/ov8856.yaml | 133 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 134 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> new file mode 100644
> index 000000000000..f5cb9add9277
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> @@ -0,0 +1,133 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (c) 2019 MediaTek Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings
> +
> +maintainers:
> +  - Ben Kao <ben.kao@intel.com>

Is Ben aware of this?

> +  - Dongchun Zhu <dongchun.zhu@mediatek.com>
> +
> +description: |-
> +  The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS
> +  image sensor that delivers 3264x2448 at 30fps. It provides full-frame,
> +  sub-sampled, and windowed 10-bit MIPI images in various formats via the
> +  Serial Camera Control Bus (SCCB) interface. This chip is programmable
> +  through I2C and two-wire SCCB. The sensor output is available via CSI-2
> +  serial data output (up to 4-lane).
> +
> +properties:
> +  compatible:
> +    const: ovti,ov8856
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description:
> +      Input clock for the sensor.
> +    items:
> +      - const: xvclk
> +
> +  clock-frequency:
> +    description:
> +      Frequency of the xvclk clock in Hertz.
> +
> +  dovdd-supply:
> +    description:
> +      Definition of the regulator used as interface power supply.
> +
> +  avdd-supply:
> +    description:
> +      Definition of the regulator used as analog power supply.
> +
> +  dvdd-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.
> +      This corresponds to the hardware pin XSHUTDOWN which is physically
> +      active low.
> +
> +  port:
> +    type: object
> +    additionalProperties: false
> +    description:
> +      A node containing input and output port nodes with endpoint definitions
> +      as documented in
> +      Documentation/devicetree/bindings/media/video-interfaces.txt
> +
> +    properties:
> +      endpoint:
> +        type: object
> +
> +        properties:
> +          clock-lanes:
> +            maxItems: 1
> +
> +          data-lanes:
> +            maxItems: 1
> +
> +          remote-endpoint: true
> +
> +        required:
> +          - clock-lanes

Do you need the clock-lanes property, i.e. does the device support lane
reordering? If not, it should be removed.

> +          - data-lanes
> +          - remote-endpoint
> +
> +    required:
> +      - endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - clock-frequency
> +  - dovdd-supply
> +  - avdd-supply
> +  - dvdd-supply
> +  - reset-gpios
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/clock/qcom,camcc-sdm845.h>
> +
> +    ov8856: camera-sensor@10 {
> +        compatible = "ovti,ov8856";
> +        reg = <0x10>;
> +        reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&clk_24m_cam>;
> +
> +        clocks = <&clock_camcc CAM_CC_MCLK0_CLK>;
> +        clock-names = "xvclk";
> +        clock-frequency = <19200000>;
> +
> +        avdd-supply = <&mt6358_vcama2_reg>;
> +        dvdd-supply = <&mt6358_vcamd_reg>;
> +        dovdd-supply = <&mt6358_vcamio_reg>;
> +
> +        port {
> +            wcam_out: endpoint {
> +                remote-endpoint = <&mipi_in_wcam>;
> +                clock-lanes = <0>;
> +                data-lanes = <1 2 3 4>;
> +                link-frequencies = /bits/ 64 <360000000 180000000>;
> +            };
> +        };
> +    };
> +
> +...
> \ No newline at end of file
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a6fbdf354d34..0f99e863978a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12355,6 +12355,7 @@ L:	linux-media@vger.kernel.org
>  T:	git git://linuxtv.org/media_tree.git
>  S:	Maintained
>  F:	drivers/media/i2c/ov8856.c
> +F:	Documentation/devicetree/bindings/media/i2c/ov8856.yaml
>  
>  OMNIVISION OV9650 SENSOR DRIVER
>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 2/3] media: ov8856: Add devicetree support

[Andy Shevchenko]

On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote:
> Add devicetree match table, and enable ov8856_probe()
> to initialize power, clocks and reset pins.

Thanks for an update.
My comments below.

...

> +	ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");

In many frameworks we have '_optional' variants of API. Please use it instead
of open coded approach.

> +	if (PTR_ERR(ov8856->xvclk) == -ENOENT) {
> +		dev_info(&client->dev, "xvclk clock not defined, continuing...\n");
> +		ov8856->xvclk = NULL;
> +	} else if (IS_ERR(ov8856->xvclk)) {
> +		dev_err(&client->dev, "could not get xvclk clock (%ld)\n",
> +			PTR_ERR(ov8856->xvclk));
> +		return PTR_ERR(ov8856->xvclk);
> +	}
> +
> +	ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> +		return ret;
> +	}
> +

> +	ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
> +					       GPIOD_OUT_HIGH);

Same here.

> +	if (IS_ERR(ov8856->reset_gpio)) {
> +		dev_err(&client->dev, "failed to get reset-gpios\n");
> +		return PTR_ERR(ov8856->reset_gpio);
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> +		ov8856->supplies[i].supply = ov8856_supply_names[i];
> +

> +	ret = devm_regulator_bulk_get(&client->dev,
> +				      ARRAY_SIZE(ov8856_supply_names),
> +				      ov8856->supplies);

Luckily regulator framework will create dummy ones if there is none found.

> +	if (ret) {
> +		dev_warn(&client->dev, "failed to get regulators\n");
> +		return ret;
> +	}

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 3/3] media: ov8856: Implement sensor module revision identification

[Sakari Ailus]

Hi Robert,

On Fri, Mar 13, 2020 at 12:03:50PM +0100, Robert Foss wrote:
> Query the sensor for its module revision, and compare it
> to known revisions.
> Currently only the '1B' revision has been added.
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>  drivers/media/i2c/ov8856.c | 54 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index db61eed223e8..39662d3d86dd 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -34,6 +34,18 @@
>  #define OV8856_MODE_STANDBY		0x00
>  #define OV8856_MODE_STREAMING		0x01
>  
> +/* define 1B module revision */
> +#define OV8856_1B_MODULE		0x02
> +
> +/* the OTP read-out buffer is at 0x7000 and 0xf is the offset
> + * of the byte in the OTP that means the module revision
> + */
> +#define OV8856_MODULE_REVISION		0x700f
> +#define OV8856_OTP_MODE_CTRL		0x3d84
> +#define OV8856_OTP_LOAD_CTRL		0x3d81
> +#define OV8856_OTP_MODE_AUTO		0x00
> +#define OV8856_OTP_LOAD_CTRL_ENABLE	BIT(0)
> +
>  /* vertical-timings from sensor */
>  #define OV8856_REG_VTS			0x380e
>  #define OV8856_VTS_MAX			0x7fff
> @@ -711,6 +723,25 @@ static int ov8856_test_pattern(struct ov8856 *ov8856, u32 pattern)
>  				OV8856_REG_VALUE_08BIT, pattern);
>  }
>  
> +static int ov8856_check_revision(struct ov8856 *ov8856)

There are no version checks being done here, nor apparently the version is
read by this function. 

> +{
> +	int ret;
> +
> +	ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
> +			       OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov8856_write_reg(ov8856, OV8856_OTP_MODE_CTRL,
> +			       OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO);
> +	if (ret)
> +		return ret;
> +
> +	return ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
> +				OV8856_REG_VALUE_08BIT,
> +				OV8856_OTP_LOAD_CTRL_ENABLE);

If streaming is started to read the EEPROM, shouldn't it be stopped after
reading it as well?

> +}
> +
>  static int ov8856_set_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct ov8856 *ov8856 = container_of(ctrl->handler,
> @@ -1144,6 +1175,23 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
>  		return -ENXIO;
>  	}
>  
> +	/* check sensor hardware revision */
> +	ret = ov8856_check_revision(ov8856);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to check sensor revision");
> +		return ret;
> +	}
> +
> +	ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
> +			      OV8856_REG_VALUE_08BIT, &val);
> +	if (ret)
> +		return ret;

How about moving this inside the check_revision function above? It looks as
if it's dependent on that.

> +
> +	dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n",
> +		val,
> +		val == OV8856_1B_MODULE ? "1B" : "unknown revision",
> +		client->addr);
> +
>  	return 0;
>  }
>  
> @@ -1254,12 +1302,6 @@ static int ov8856_probe(struct i2c_client *client)
>  		return PTR_ERR(ov8856->xvclk);
>  	}
>  
> -	ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24);

This seems like an unrelated change.

> -	if (ret < 0) {
> -		dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> -		return ret;
> -	}
> -
>  	ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
>  					       GPIOD_OUT_HIGH);
>  	if (IS_ERR(ov8856->reset_gpio)) {

-- 
Regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 2/3] media: ov8856: Add devicetree support

[Fabio Estevam]

Hi Robert,

On Fri, Mar 13, 2020 at 8:04 AM Robert Foss <robert.foss@linaro.org> wrote:

> +static int __ov8856_power_on(struct ov8856 *ov8856)
> +{
> +       struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> +       int ret;
> +
> +       ret = clk_prepare_enable(ov8856->xvclk);
> +       if (ret < 0) {
> +               dev_err(&client->dev, "failed to enable xvclk\n");
> +               return ret;
> +       }
> +
> +       gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);

The parameter of gpiod_set_value_cansleep() is typically 0 (inactive
state) or 1 (active state), so:

 gpiod_set_value_cansleep(ov8856->reset_gpio, 1);

> +
> +       ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> +                                   ov8856->supplies);
> +       if (ret < 0) {
> +               dev_err(&client->dev, "failed to enable regulators\n");
> +               goto disable_clk;
> +       }
> +
> +       gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW);

and here it should be:

gpiod_set_value_cansleep(ov8856->reset_gpio, 0);

Also, don't you need a reset period between the two?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 1/3] media: dt-bindings: ov8856: Document YAML bindings

[Rob Herring]

On Fri, 13 Mar 2020 12:03:48 +0100, Robert Foss wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> 
> This patch adds documentation of device tree in YAML schema for the
> OV8856 CMOS image sensor.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
> 
> - Changes since v4:
>   * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description
>   * Add clock-lanes property to example
>   * robher: Fix syntax error in devicetree example
> 
> - Changes since v3:
>   * robher: Fix syntax error
>   * robher: Removed maxItems
>   * Fixes yaml 'make dt-binding-check' errors
> 
> - Changes since v2:
>   Fixes comments from from Andy, Tomasz, Sakari, Rob.
>   * Convert text documentation to YAML schema.
> 
> - Changes since v1:
>   Fixes comments from Sakari, Tomasz
>   * Add clock-frequency and link-frequencies in DT
> 
>  .../devicetree/bindings/media/i2c/ov8856.yaml | 133 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 134 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

Documentation/devicetree/bindings/media/i2c/ov8856.example.dts:22.13-26: Warning (reg_format): /example-0/camera-sensor@10:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1)
Documentation/devicetree/bindings/media/i2c/ov8856.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/media/i2c/ov8856.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/media/i2c/ov8856.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format'

See https://patchwork.ozlabs.org/patch/1254346
Please check and re-submit.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 2/3] media: ov8856: Add devicetree support

[Robert Foss]

Hey Sakari,

On Fri, 13 Mar 2020 at 13:18, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Robert,
>
> On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote:
> > Add devicetree match table, and enable ov8856_probe()
> > to initialize power, clocks and reset pins.
> >
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > ---
> >
> > - Changes since v1:
> >   * Fabio: Change n_shutdown_gpio name to reset_gpio
> >   * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change
> >   * Fabio: Remove empty line
> >   * Fabio: Remove real error from devm_gpiod_get() failures
> >   * Andy & Sakari: Make XVCLK optional since to not break ACPI
> >   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> >   * Sakari: Use XVCLK rate as provided by DT
> >
> >  drivers/media/i2c/ov8856.c | 109 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 107 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > index 8655842af275..db61eed223e8 100644
> > --- a/drivers/media/i2c/ov8856.c
> > +++ b/drivers/media/i2c/ov8856.c
> > @@ -3,10 +3,13 @@
> >
> >  #include <asm/unaligned.h>
> >  #include <linux/acpi.h>
> > +#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/regulator/consumer.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-fwnode.h>
> > @@ -19,6 +22,8 @@
> >  #define OV8856_LINK_FREQ_180MHZ              180000000ULL
> >  #define OV8856_SCLK                  144000000ULL
> >  #define OV8856_MCLK                  19200000
> > +#define OV8856_XVCLK_19_2            19200000
>
> Please use a single macro to refer to 19,2 MHz clock.

Alright, I'll combine the two into a macro for both.

>
> > +#define OV8856_XVCLK_24                      24000000
>
> This doesn't seem to be needed

As long as we don't set the clock to 24Mhz we don't.
I'm assuming that you're saying that the 24Mhz clock rate isn't needed
for the modes used currently.

Removing this macro in v3.

>
> >  #define OV8856_DATA_LANES            4
> >  #define OV8856_RGB_DEPTH             10
> >
> > @@ -64,6 +69,12 @@
> >
> >  #define to_ov8856(_sd)                       container_of(_sd, struct ov8856, sd)
> >
> > +static const char * const ov8856_supply_names[] = {
> > +     "dovdd",        /* Digital I/O power */
> > +     "avdd",         /* Analog power */
> > +     "dvdd",         /* Digital core power */
> > +};
> > +
> >  enum {
> >       OV8856_LINK_FREQ_720MBPS,
> >       OV8856_LINK_FREQ_360MBPS,
> > @@ -566,6 +577,10 @@ struct ov8856 {
> >       struct media_pad pad;
> >       struct v4l2_ctrl_handler ctrl_handler;
> >
> > +     struct clk              *xvclk;
> > +     struct gpio_desc        *reset_gpio;
> > +     struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)];
> > +
> >       /* V4L2 Controls */
> >       struct v4l2_ctrl *link_freq;
> >       struct v4l2_ctrl *pixel_rate;
> > @@ -908,6 +923,46 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> >       return ret;
> >  }
> >
> > +static int __ov8856_power_on(struct ov8856 *ov8856)
> > +{
> > +     struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > +     int ret;
> > +
> > +     ret = clk_prepare_enable(ov8856->xvclk);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to enable xvclk\n");
> > +             return ret;
> > +     }
> > +
> > +     gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> > +
> > +     ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> > +                                 ov8856->supplies);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to enable regulators\n");
> > +             goto disable_clk;
> > +     }
> > +
> > +     gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW);
> > +
> > +     usleep_range(1500, 1800);
>
> I think you could omit the delay on ACPI based systems. Or just bail out
> early in that case.

I'll add a check for reset_gpio being NULL, and skip the sleep for that case.

>
> > +
> > +     return 0;
> > +
> > +disable_clk:
>
> How about the GPIO here?

Ack

>
> > +     clk_disable_unprepare(ov8856->xvclk);
> > +
> > +     return ret;
> > +}
> > +
> > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > +{
> > +     gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> > +     regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > +                            ov8856->supplies);
> > +     clk_disable_unprepare(ov8856->xvclk);
> > +}
>
> You'll need to call the two in the driver's suspend and resume functions.

Ack

>
> > +
> >  static int __maybe_unused ov8856_suspend(struct device *dev)
> >  {
> >       struct i2c_client *client = to_i2c_client(dev);
> > @@ -1175,7 +1230,7 @@ static int ov8856_remove(struct i2c_client *client)
> >  static int ov8856_probe(struct i2c_client *client)
> >  {
> >       struct ov8856 *ov8856;
> > -     int ret;
> > +     int i, ret;
>
> unsigned int?

Ack

>
> >
> >       ret = ov8856_check_hwcfg(&client->dev);
> >       if (ret) {
> > @@ -1189,10 +1244,50 @@ static int ov8856_probe(struct i2c_client *client)
> >               return -ENOMEM;
> >
> >       v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> > +     ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> > +     if (PTR_ERR(ov8856->xvclk) == -ENOENT) {
> > +             dev_info(&client->dev, "xvclk clock not defined, continuing...\n");
>
> How about dev_dbg()?

Ack.

>
> > +             ov8856->xvclk = NULL;
> > +     } else if (IS_ERR(ov8856->xvclk)) {
> > +             dev_err(&client->dev, "could not get xvclk clock (%ld)\n",
> > +                     PTR_ERR(ov8856->xvclk));
> > +             return PTR_ERR(ov8856->xvclk);
> > +     }
> > +
> > +     ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24);
>
> This should either come from platform data, or perhaps it'd be even better
> to get the clock rate and use assigned-clock-rates. I guess that's
> preferred nowadays.

I'm a bit unsure about what this would look like.

Are you thinking something like the way ext_clk is used in smiapp_core.c?
I went ahead and implemented support for retrieving and storing
'clock-rates' during the ov8856_check_hwcfg() call, and then setting
the rate to the configured rate during probing.

>
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> > +             return ret;
> > +     }
> > +
> > +     ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
> > +                                            GPIOD_OUT_HIGH);
>
> Indentation.

Ack.

>
> What if no gpio is defined?

As per Andys comments, I'll switch to the optional version of devm_gpiod_get().

>
> > +     if (IS_ERR(ov8856->reset_gpio)) {
> > +             dev_err(&client->dev, "failed to get reset-gpios\n");
> > +             return PTR_ERR(ov8856->reset_gpio);
> > +     }
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> > +             ov8856->supplies[i].supply = ov8856_supply_names[i];
> > +
> > +     ret = devm_regulator_bulk_get(&client->dev,
> > +                                   ARRAY_SIZE(ov8856_supply_names),
> > +                                   ov8856->supplies);
>
> What happens if there are no regulators?

Like Andy mentioned, we should be alright, since
devm_regulator_bulk_get() creates dummy regulators if one isn't found.

>
> > +     if (ret) {
> > +             dev_warn(&client->dev, "failed to get regulators\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = __ov8856_power_on(ov8856);
> > +     if (ret) {
> > +             dev_warn(&client->dev, "failed to power on\n");
> > +             return ret;
> > +     }
> > +
> >       ret = ov8856_identify_module(ov8856);
> >       if (ret) {
> >               dev_err(&client->dev, "failed to find sensor: %d", ret);
> > -             return ret;
> > +             goto probe_power_off;
> >       }
> >
> >       mutex_init(&ov8856->mutex);
> > @@ -1238,6 +1333,9 @@ static int ov8856_probe(struct i2c_client *client)
> >       v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
> >       mutex_destroy(&ov8856->mutex);
> >
> > +probe_power_off:
> > +     __ov8856_power_off(ov8856);
> > +
>
> Also remember to power off the device in remove().
>

Ack

> >       return ret;
> >  }
> >
> > @@ -1254,11 +1352,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
> >  MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
> >  #endif
> >
> > +static const struct of_device_id ov8856_of_match[] = {
> > +     { .compatible = "ovti,ov8856" },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> > +
> >  static struct i2c_driver ov8856_i2c_driver = {
> >       .driver = {
> >               .name = "ov8856",
> >               .pm = &ov8856_pm_ops,
> >               .acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> > +             .of_match_table = ov8856_of_match,
> >       },
> >       .probe_new = ov8856_probe,
> >       .remove = ov8856_remove,
>
> --
> Regards,
>
> Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 2/3] media: ov8856: Add devicetree support

[Sakari Ailus]

Hi Robert,

On Thu, Mar 26, 2020 at 12:56:37PM +0100, Robert Foss wrote:
...
> > > +static int __ov8856_power_on(struct ov8856 *ov8856)
> > > +{
> > > +     struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > > +     int ret;
> > > +
> > > +     ret = clk_prepare_enable(ov8856->xvclk);
> > > +     if (ret < 0) {
> > > +             dev_err(&client->dev, "failed to enable xvclk\n");
> > > +             return ret;
> > > +     }
> > > +
> > > +     gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> > > +
> > > +     ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> > > +                                 ov8856->supplies);
> > > +     if (ret < 0) {
> > > +             dev_err(&client->dev, "failed to enable regulators\n");
> > > +             goto disable_clk;
> > > +     }
> > > +
> > > +     gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW);
> > > +
> > > +     usleep_range(1500, 1800);
> >
> > I think you could omit the delay on ACPI based systems. Or just bail out
> > early in that case.
> 
> I'll add a check for reset_gpio being NULL, and skip the sleep for that case.

There could also be a regulator but no GPIO.

I think if you don't have either, then certainly there's no need for a
delay.

...

> > > +             ov8856->xvclk = NULL;
> > > +     } else if (IS_ERR(ov8856->xvclk)) {
> > > +             dev_err(&client->dev, "could not get xvclk clock (%ld)\n",
> > > +                     PTR_ERR(ov8856->xvclk));
> > > +             return PTR_ERR(ov8856->xvclk);
> > > +     }
> > > +
> > > +     ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24);
> >
> > This should either come from platform data, or perhaps it'd be even better
> > to get the clock rate and use assigned-clock-rates. I guess that's
> > preferred nowadays.
> 
> I'm a bit unsure about what this would look like.
> 
> Are you thinking something like the way ext_clk is used in smiapp_core.c?
> I went ahead and implemented support for retrieving and storing
> 'clock-rates' during the ov8856_check_hwcfg() call, and then setting
> the rate to the configured rate during probing.

With assigned-clock-rates, you can simply use clk_get_rate().

As you get the actual rate, it could be somewhat off of the intended one.

-- 
Kind regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 2/3] media: ov8856: Add devicetree support

[Robert Foss]

On Thu, 26 Mar 2020 at 15:47, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Robert,
>
> On Thu, Mar 26, 2020 at 12:56:37PM +0100, Robert Foss wrote:
> ...
> > > > +static int __ov8856_power_on(struct ov8856 *ov8856)
> > > > +{
> > > > +     struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > > > +     int ret;
> > > > +
> > > > +     ret = clk_prepare_enable(ov8856->xvclk);
> > > > +     if (ret < 0) {
> > > > +             dev_err(&client->dev, "failed to enable xvclk\n");
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> > > > +
> > > > +     ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> > > > +                                 ov8856->supplies);
> > > > +     if (ret < 0) {
> > > > +             dev_err(&client->dev, "failed to enable regulators\n");
> > > > +             goto disable_clk;
> > > > +     }
> > > > +
> > > > +     gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW);
> > > > +
> > > > +     usleep_range(1500, 1800);
> > >
> > > I think you could omit the delay on ACPI based systems. Or just bail out
> > > early in that case.
> >
> > I'll add a check for reset_gpio being NULL, and skip the sleep for that case.
>
> There could also be a regulator but no GPIO.
>
> I think if you don't have either, then certainly there's no need for a
> delay.

Removing the delay if no action is taken makes sense, but I'm not sure
how best to do it.
If there are no regulators dummy ones are created automatically, which
makes distinguishing between a little bit cumbersome. The regulator
structs could of course all be inspected, and if all are dummy ones,
the delay could be skipped. But is there a neater way of doing this?
Manually inspecting the regs strikes me as a bit inelegant.

>
> ...
>
> > > > +             ov8856->xvclk = NULL;
> > > > +     } else if (IS_ERR(ov8856->xvclk)) {
> > > > +             dev_err(&client->dev, "could not get xvclk clock (%ld)\n",
> > > > +                     PTR_ERR(ov8856->xvclk));
> > > > +             return PTR_ERR(ov8856->xvclk);
> > > > +     }
> > > > +
> > > > +     ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24);
> > >
> > > This should either come from platform data, or perhaps it'd be even better
> > > to get the clock rate and use assigned-clock-rates. I guess that's
> > > preferred nowadays.
> >
> > I'm a bit unsure about what this would look like.
> >
> > Are you thinking something like the way ext_clk is used in smiapp_core.c?
> > I went ahead and implemented support for retrieving and storing
> > 'clock-rates' during the ov8856_check_hwcfg() call, and then setting
> > the rate to the configured rate during probing.
>
> With assigned-clock-rates, you can simply use clk_get_rate().

Ah, I see.

I'll switch to that approach then.

>
> As you get the actual rate, it could be somewhat off of the intended one.
>
> --
> Kind regards,
>
> Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 2/3] media: ov8856: Add devicetree support

[Sakari Ailus]

Hi Robert,

On Fri, Mar 27, 2020 at 11:32:29AM +0100, Robert Foss wrote:
> On Thu, 26 Mar 2020 at 15:47, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Hi Robert,
> >
> > On Thu, Mar 26, 2020 at 12:56:37PM +0100, Robert Foss wrote:
> > ...
> > > > > +static int __ov8856_power_on(struct ov8856 *ov8856)
> > > > > +{
> > > > > +     struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > > > > +     int ret;
> > > > > +
> > > > > +     ret = clk_prepare_enable(ov8856->xvclk);
> > > > > +     if (ret < 0) {
> > > > > +             dev_err(&client->dev, "failed to enable xvclk\n");
> > > > > +             return ret;
> > > > > +     }
> > > > > +
> > > > > +     gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> > > > > +
> > > > > +     ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> > > > > +                                 ov8856->supplies);
> > > > > +     if (ret < 0) {
> > > > > +             dev_err(&client->dev, "failed to enable regulators\n");
> > > > > +             goto disable_clk;
> > > > > +     }
> > > > > +
> > > > > +     gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW);
> > > > > +
> > > > > +     usleep_range(1500, 1800);
> > > >
> > > > I think you could omit the delay on ACPI based systems. Or just bail out
> > > > early in that case.
> > >
> > > I'll add a check for reset_gpio being NULL, and skip the sleep for that case.
> >
> > There could also be a regulator but no GPIO.
> >
> > I think if you don't have either, then certainly there's no need for a
> > delay.
> 
> Removing the delay if no action is taken makes sense, but I'm not sure
> how best to do it.
> If there are no regulators dummy ones are created automatically, which
> makes distinguishing between a little bit cumbersome. The regulator
> structs could of course all be inspected, and if all are dummy ones,
> the delay could be skipped. But is there a neater way of doing this?
> Manually inspecting the regs strikes me as a bit inelegant.

I guess the cleanest, easy way to make this right, albeit slightly
unoptimal in very rare cases where you have none of the above resources in
a DT system, is to bail out if you're running on an ACPI based system.

I.e. checking for e.g. is_acpi_node(dev->fwnode).

-- 
Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 2/3] media: ov8856: Add devicetree support

[Robert Foss]

On Thu, 26 Mar 2020 at 13:18, Robert Foss <robert.foss@linaro.org> wrote:
>
> Hey Fabio,
>
> On Fri, 13 Mar 2020 at 14:15, Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Robert,
> >
> > On Fri, Mar 13, 2020 at 8:04 AM Robert Foss <robert.foss@linaro.org> wrote:
> >
> > > +static int __ov8856_power_on(struct ov8856 *ov8856)
> > > +{
> > > +       struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > > +       int ret;
> > > +
> > > +       ret = clk_prepare_enable(ov8856->xvclk);
> > > +       if (ret < 0) {
> > > +               dev_err(&client->dev, "failed to enable xvclk\n");
> > > +               return ret;
> > > +       }
> > > +
> > > +       gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> >
> > The parameter of gpiod_set_value_cansleep() is typically 0 (inactive
> > state) or 1 (active state), so:
> >
> >  gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
>
> Ack

After testing this change, it breaks the driver during probing.
I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they
seem to never be 0 or 1.

https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/consumer.h#L38

GPIOD_ASIS = 0,
GPIOD_IN = 1,
GPIOD_OUT_LOW = 3
GPIOD_OUT_HIGH = 7

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 2/3] media: ov8856: Add devicetree support

[Fabio Estevam]

Hi Robert,

On Tue, Mar 31, 2020 at 10:37 AM Robert Foss <robert.foss@linaro.org> wrote:

> After testing this change, it breaks the driver during probing.

Why exactly does it break probing? Maybe the GPIO polarity defined in
the device tree is wrong?

> I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they
> seem to never be 0 or 1.

If you do a grep in all gpiod_set_value_cansleep() usages in the
kernel tree, there is not a single case where  GPIOD_OUT_HIGH or
GPIOD_OUT_LOW is passed as argument of gpiod_set_value_cansleep().

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2 2/3] media: ov8856: Add devicetree support

[Andy Shevchenko]

On Tue, Mar 31, 2020 at 4:43 PM Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Mar 31, 2020 at 10:37 AM Robert Foss <robert.foss@linaro.org> wrote:
>
> > After testing this change, it breaks the driver during probing.
>
> Why exactly does it break probing? Maybe the GPIO polarity defined in
> the device tree is wrong?
>
> > I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they
> > seem to never be 0 or 1.
>
> If you do a grep in all gpiod_set_value_cansleep() usages in the
> kernel tree, there is not a single case where  GPIOD_OUT_HIGH or
> GPIOD_OUT_LOW is passed as argument of gpiod_set_value_cansleep().

+1. It simple reveals the problem that is somewhere else.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel