[v1 0/3] media: ov8856: Add sensor modes & devicetree support

[v1 0/3] media: ov8856: Add sensor modes & 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 | 129 +++++++++++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/ov8856.c                    | 153 +++++++++++++++++-
 3 files changed, 281 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml

-- 
2.20.1

[v1 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 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 | 129 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 130 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..c8099ccab1d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
@@ -0,0 +1,129 @@
+# 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.
+
+  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>
+
+    ov8856: camera-sensor@10 {
+        compatible = "ovti,ov8856";
+        reg = <0x10>;
+        reset-gpios = <&pio 111 GPIO_ACTIVE_HIGH>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&clk_24m_cam>;
+
+        clocks = <&cru SCLK_TESTCLKOUT1>;
+        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>;
+                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

[v1 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>
---
 drivers/media/i2c/ov8856.c | 105 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 103 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 8655842af275..1769acdfaa44 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,14 @@
 
 #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 */
+};
+
+#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)
+
 enum {
 	OV8856_LINK_FREQ_720MBPS,
 	OV8856_LINK_FREQ_360MBPS,
@@ -566,6 +579,10 @@ struct ov8856 {
 	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
 
+	struct clk		*xvclk;
+	struct gpio_desc	*n_shutdn_gpio;
+	struct regulator_bulk_data supplies[OV8856_NUM_SUPPLIES];
+
 	/* V4L2 Controls */
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *pixel_rate;
@@ -908,6 +925,45 @@ 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->n_shutdn_gpio, GPIOD_OUT_LOW);
+
+	ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to enable regulators\n");
+		goto disable_clk;
+	}
+
+	gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
+
+	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->n_shutdn_gpio, GPIOD_OUT_LOW);
+	regulator_bulk_disable(OV8856_NUM_SUPPLIES, 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 +1231,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 +1245,45 @@ 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 (IS_ERR(ov8856->xvclk)) {
+		dev_err(&client->dev, "failed to get xvclk\n");
+		return -EINVAL;
+	}
+
+	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->n_shutdn_gpio = devm_gpiod_get(&client->dev, "reset",
+					       GPIOD_OUT_LOW);
+	if (IS_ERR(ov8856->n_shutdn_gpio)) {
+		dev_err(&client->dev, "failed to get reset-gpios\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < OV8856_NUM_SUPPLIES; i++)
+		ov8856->supplies[i].supply = ov8856_supply_names[i];
+
+	ret = devm_regulator_bulk_get(&client->dev, OV8856_NUM_SUPPLIES,
+				      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 +1329,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 +1348,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

[v1 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 | 48 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 1769acdfaa44..48e8f4b997d6 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
@@ -713,6 +725,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,
@@ -1145,6 +1176,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;
 }
 
-- 
2.20.1

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

[Fabio Estevam]

Hi Robert,

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

> +    ov8856: camera-sensor@10 {
> +        compatible = "ovti,ov8856";
> +        reg = <0x10>;
> +        reset-gpios = <&pio 111 GPIO_ACTIVE_HIGH>;

Could you double check this is correct? Other OmniVision sensors have
reset-gpios as active low.

I suspect that the driver has also an inverted logic, so that's why it works.

I don't have access to the datasheet though, so I am just guessing.

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

[Fabio Estevam]

On Tue, Mar 10, 2020 at 10:47 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->n_shutdn_gpio, GPIOD_OUT_LOW);
> +
> +       ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> +       if (ret < 0) {
> +               dev_err(&client->dev, "failed to enable regulators\n");
> +               goto disable_clk;
> +       }
> +
> +       gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);

To power it up you probably only need:

gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, 0);

And use reset-gpios as active low in your device tree. Assuming the
reset-gpios is active low like other OmniVision sensors.

> +
> +       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->n_shutdn_gpio, GPIOD_OUT_LOW);
> +       regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> +       clk_disable_unprepare(ov8856->xvclk);
> +}
> +
> +

Unneede extra blank line.

>         v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> +       ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> +       if (IS_ERR(ov8856->xvclk)) {
> +               dev_err(&client->dev, "failed to get xvclk\n");
> +               return -EINVAL;

You should better return the real error insteald
PTR_ERR(ov8856->xvclk). This way defer probe could work.

> +       }
> +
> +       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->n_shutdn_gpio = devm_gpiod_get(&client->dev, "reset",
> +                                              GPIOD_OUT_LOW);
> +       if (IS_ERR(ov8856->n_shutdn_gpio)) {
> +               dev_err(&client->dev, "failed to get reset-gpios\n");
> +               return -EINVAL;

Please return the real error.

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

[Andy Shevchenko]

On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote:
> Add devicetree match table, and enable ov8856_probe()
> to initialize power, clocks and reset pins.

...

> +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)

Use ARRAY_SIZE() directly.

Have you seen Sakari's comments?
Sakari, do I have déjà vu or you indeed commented this driver?

...

> +	gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);

> +	gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);

Yes, seems this one is inverted.

...

> +{
> +	gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> +	regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> +	clk_disable_unprepare(ov8856->xvclk);
> +}
> +
> +

One blank line is enough.

...

> +	ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> +	if (IS_ERR(ov8856->xvclk)) {
> +		dev_err(&client->dev, "failed to get xvclk\n");
> +		return -EINVAL;
> +	}

Previously it worked without clock provider, now you make a dependency.

This won't work.


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Mar 10, 2020 at 02:46:03PM +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.

Are you sure you send latest version?

I have a déjà vu that I have seen this already and this one doesn't address any
comment given.

...

> +	dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n",
> +		val,

> +		val == OV8856_1B_MODULE ? "1B" : "unknown revision",

This is weird. Can you add a bit more general way of showing revision?

> +		client->addr);

-- 
With Best Regards,
Andy Shevchenko

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

[Robert Foss]

On Tue, 10 Mar 2020 at 15:03, Fabio Estevam <festevam@gmail.com> wrote:
>
> On Tue, Mar 10, 2020 at 10:47 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->n_shutdn_gpio, GPIOD_OUT_LOW);
> > +
> > +       ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > +       if (ret < 0) {
> > +               dev_err(&client->dev, "failed to enable regulators\n");
> > +               goto disable_clk;
> > +       }
> > +
> > +       gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
>
> To power it up you probably only need:
>
> gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, 0);
>
> And use reset-gpios as active low in your device tree. Assuming the
> reset-gpios is active low like other OmniVision sensors.

Ack.

>
> > +
> > +       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->n_shutdn_gpio, GPIOD_OUT_LOW);
> > +       regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > +       clk_disable_unprepare(ov8856->xvclk);
> > +}
> > +
> > +
>
> Unneede extra blank line.

Ack.

>
> >         v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> > +       ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> > +       if (IS_ERR(ov8856->xvclk)) {
> > +               dev_err(&client->dev, "failed to get xvclk\n");
> > +               return -EINVAL;
>
> You should better return the real error insteald
> PTR_ERR(ov8856->xvclk). This way defer probe could work.
>

Ack.

> > +       }
> > +
> > +       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->n_shutdn_gpio = devm_gpiod_get(&client->dev, "reset",
> > +                                              GPIOD_OUT_LOW);
> > +       if (IS_ERR(ov8856->n_shutdn_gpio)) {
> > +               dev_err(&client->dev, "failed to get reset-gpios\n");
> > +               return -EINVAL;
>
> Please return the real error.

Ack.

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

[Robert Foss]

Hey Fabio,

Thanks for having a look at this series so quickly.

On Tue, 10 Mar 2020 at 14:57, Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Robert,
>
> On Tue, Mar 10, 2020 at 10:46 AM Robert Foss <robert.foss@linaro.org> wrote:
>
> > +    ov8856: camera-sensor@10 {
> > +        compatible = "ovti,ov8856";
> > +        reg = <0x10>;
> > +        reset-gpios = <&pio 111 GPIO_ACTIVE_HIGH>;
>
> Could you double check this is correct? Other OmniVision sensors have
> reset-gpios as active low.

I have tested this, unfortunately I don't have access to a ov8856
datasheet that includes
this level of detail. But I have tested this.

>
> I suspect that the driver has also an inverted logic, so that's why it works.

That could explain it still working. Let me have a look into the
driver and see what it does.

>
> I don't have access to the datasheet though, so I am just guessing.

Me neither unfortunately, if anyone does have a link for it, I would
very much appreciate it.

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

[Robert Foss]

Hi Andy,

On Tue, 10 Mar 2020 at 15:26, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote:
> > Add devicetree match table, and enable ov8856_probe()
> > to initialize power, clocks and reset pins.
>
> ...
>
> > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)
>
> Use ARRAY_SIZE() directly.

Ack.

>
> Have you seen Sakari's comments?
> Sakari, do I have déją vu or you indeed commented this driver?

Yes, I may have missed some part of it, so please tell me if I have.

There is a patchset floating around that implements a larger chunk of
functionality,
including a couple of new modes. This is based on that series.

>
> ...
>
> > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
>
> > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
>
> Yes, seems this one is inverted.
>
> ...
>
> > +{
> > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> > +     regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > +     clk_disable_unprepare(ov8856->xvclk);
> > +}
> > +
> > +
>
> One blank line is enough.
>
> ...
>
> > +     ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> > +     if (IS_ERR(ov8856->xvclk)) {
> > +             dev_err(&client->dev, "failed to get xvclk\n");
> > +             return -EINVAL;
> > +     }
>
> Previously it worked without clock provider, now you make a dependency.
>
> This won't work.

So the ideal behavior would be to only use the xclk if it is provided?

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

[Rob Herring]

On Tue, 10 Mar 2020 14:46:01 +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 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 | 129 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 130 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> 

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

Error: Documentation/devicetree/bindings/media/i2c/ov8856.example.dts:26.28-29 syntax error
FATAL ERROR: Unable to parse input tree
scripts/Makefile.lib:311: recipe for target 'Documentation/devicetree/bindings/media/i2c/ov8856.example.dt.yaml' failed
make[1]: *** [Documentation/devicetree/bindings/media/i2c/ov8856.example.dt.yaml] Error 1
Makefile:1262: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

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

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

[Andy Shevchenko]

On Tue, Mar 10, 2020 at 04:55:20PM +0100, Robert Foss wrote:
> On Tue, 10 Mar 2020 at 15:26, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote:

...

> > > +     ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> > > +     if (IS_ERR(ov8856->xvclk)) {
> > > +             dev_err(&client->dev, "failed to get xvclk\n");
> > > +             return -EINVAL;
> > > +     }
> >
> > Previously it worked without clock provider, now you make a dependency.
> >
> > This won't work.
> 
> So the ideal behavior would be to only use the xclk if it is provided?

Yes, make it optional.

-- 
With Best Regards,
Andy Shevchenko

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

[Sakari Ailus]

Hi Robert,

On Tue, Mar 10, 2020 at 04:55:20PM +0100, Robert Foss wrote:
> Hi Andy,
> 
> On Tue, 10 Mar 2020 at 15:26, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote:
> > > Add devicetree match table, and enable ov8856_probe()
> > > to initialize power, clocks and reset pins.
> >
> > ...
> >
> > > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)
> >
> > Use ARRAY_SIZE() directly.
> 
> Ack.
> 
> >
> > Have you seen Sakari's comments?
> > Sakari, do I have déją vu or you indeed commented this driver?
> 
> Yes, I may have missed some part of it, so please tell me if I have.
> 
> There is a patchset floating around that implements a larger chunk of
> functionality,
> including a couple of new modes. This is based on that series.

Please see earlier comments given against an earlier variant of this set.
They're on LMML.

> 
> >
> > ...
> >
> > > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> >
> > > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
> >
> > Yes, seems this one is inverted.
> >
> > ...
> >
> > > +{
> > > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> > > +     regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > > +     clk_disable_unprepare(ov8856->xvclk);
> > > +}
> > > +
> > > +
> >
> > One blank line is enough.
> >
> > ...
> >
> > > +     ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> > > +     if (IS_ERR(ov8856->xvclk)) {
> > > +             dev_err(&client->dev, "failed to get xvclk\n");
> > > +             return -EINVAL;
> > > +     }
> >
> > Previously it worked without clock provider, now you make a dependency.
> >
> > This won't work.
> 
> So the ideal behavior would be to only use the xclk if it is provided?

See e.g. the smiapp driver on how to do this so it continues to work on
ACPI.

I think it'd be also appropriate to add the usleep() after lifting reset
only if the reset GPIO is defined for the device.

Also do consider dropping some people from the distribution. For many this
is just noise.

-- 
Kind regards,

Sakari Ailus

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

[Robert Foss]

Hey Sakari,

On Wed, 11 Mar 2020 at 12:49, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Robert,
>
> On Tue, Mar 10, 2020 at 04:55:20PM +0100, Robert Foss wrote:
> > Hi Andy,
> >
> > On Tue, 10 Mar 2020 at 15:26, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote:
> > > > Add devicetree match table, and enable ov8856_probe()
> > > > to initialize power, clocks and reset pins.
> > >
> > > ...
> > >
> > > > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)
> > >
> > > Use ARRAY_SIZE() directly.
> >
> > Ack.
> >
> > >
> > > Have you seen Sakari's comments?
> > > Sakari, do I have déją vu or you indeed commented this driver?
> >
> > Yes, I may have missed some part of it, so please tell me if I have.
> >
> > There is a patchset floating around that implements a larger chunk of
> > functionality,
> > including a couple of new modes. This is based on that series.
>
> Please see earlier comments given against an earlier variant of this set.
> They're on LMML.
>
> >
> > >
> > > ...
> > >
> > > > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> > >
> > > > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
> > >
> > > Yes, seems this one is inverted.
> > >
> > > ...
> > >
> > > > +{
> > > > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> > > > +     regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > > > +     clk_disable_unprepare(ov8856->xvclk);
> > > > +}
> > > > +
> > > > +
> > >
> > > One blank line is enough.
> > >
> > > ...
> > >
> > > > +     ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> > > > +     if (IS_ERR(ov8856->xvclk)) {
> > > > +             dev_err(&client->dev, "failed to get xvclk\n");
> > > > +             return -EINVAL;
> > > > +     }
> > >
> > > Previously it worked without clock provider, now you make a dependency.
> > >
> > > This won't work.
> >
> > So the ideal behavior would be to only use the xclk if it is provided?
>
> See e.g. the smiapp driver on how to do this so it continues to work on
> ACPI.

Thanks for the pointer!

>
> I think it'd be also appropriate to add the usleep() after lifting reset
> only if the reset GPIO is defined for the device.

Ack

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

[Robert Foss]

Hey Sakari,

On Wed, 11 Mar 2020 at 12:49, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Robert,
>
> On Tue, Mar 10, 2020 at 04:55:20PM +0100, Robert Foss wrote:
> > Hi Andy,
> >
> > On Tue, 10 Mar 2020 at 15:26, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote:
> > > > Add devicetree match table, and enable ov8856_probe()
> > > > to initialize power, clocks and reset pins.
> > >
> > > ...
> > >
> > > > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)
> > >
> > > Use ARRAY_SIZE() directly.
> >
> > Ack.
> >
> > >
> > > Have you seen Sakari's comments?
> > > Sakari, do I have déją vu or you indeed commented this driver?
> >
> > Yes, I may have missed some part of it, so please tell me if I have.
> >
> > There is a patchset floating around that implements a larger chunk of
> > functionality,
> > including a couple of new modes. This is based on that series.
>
> Please see earlier comments given against an earlier variant of this set.
> They're on LMML.
>
> >
> > >
> > > ...
> > >
> > > > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> > >
> > > > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
> > >
> > > Yes, seems this one is inverted.
> > >
> > > ...
> > >
> > > > +{
> > > > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> > > > +     regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > > > +     clk_disable_unprepare(ov8856->xvclk);
> > > > +}
> > > > +
> > > > +
> > >
> > > One blank line is enough.
> > >
> > > ...
> > >
> > > > +     ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> > > > +     if (IS_ERR(ov8856->xvclk)) {
> > > > +             dev_err(&client->dev, "failed to get xvclk\n");
> > > > +             return -EINVAL;
> > > > +     }
> > >
> > > Previously it worked without clock provider, now you make a dependency.
> > >
> > > This won't work.
> >
> > So the ideal behavior would be to only use the xclk if it is provided?
>
> See e.g. the smiapp driver on how to do this so it continues to work on
> ACPI.

Thanks for the pointer!

>
> I think it'd be also appropriate to add the usleep() after lifting reset
> only if the reset GPIO is defined for the device.

Ack

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

[Sakari Ailus]

On Wed, Mar 11, 2020 at 02:32:30PM +0100, Robert Foss wrote:
> Hey Sakari,
> 
> On Wed, 11 Mar 2020 at 12:49, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Hi Robert,
> >
> > On Tue, Mar 10, 2020 at 04:55:20PM +0100, Robert Foss wrote:
> > > Hi Andy,
> > >
> > > On Tue, 10 Mar 2020 at 15:26, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote:
> > > > > Add devicetree match table, and enable ov8856_probe()
> > > > > to initialize power, clocks and reset pins.
> > > >
> > > > ...
> > > >
> > > > > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)
> > > >
> > > > Use ARRAY_SIZE() directly.
> > >
> > > Ack.
> > >
> > > >
> > > > Have you seen Sakari's comments?
> > > > Sakari, do I have déją vu or you indeed commented this driver?
> > >
> > > Yes, I may have missed some part of it, so please tell me if I have.
> > >
> > > There is a patchset floating around that implements a larger chunk of
> > > functionality,
> > > including a couple of new modes. This is based on that series.
> >
> > Please see earlier comments given against an earlier variant of this set.
> > They're on LMML.
> >
> > >
> > > >
> > > > ...
> > > >
> > > > > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> > > >
> > > > > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
> > > >
> > > > Yes, seems this one is inverted.
> > > >
> > > > ...
> > > >
> > > > > +{
> > > > > +     gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> > > > > +     regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > > > > +     clk_disable_unprepare(ov8856->xvclk);
> > > > > +}
> > > > > +
> > > > > +
> > > >
> > > > One blank line is enough.
> > > >
> > > > ...
> > > >
> > > > > +     ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> > > > > +     if (IS_ERR(ov8856->xvclk)) {
> > > > > +             dev_err(&client->dev, "failed to get xvclk\n");
> > > > > +             return -EINVAL;
> > > > > +     }
> > > >
> > > > Previously it worked without clock provider, now you make a dependency.
> > > >
> > > > This won't work.
> > >
> > > So the ideal behavior would be to only use the xclk if it is provided?
> >
> > See e.g. the smiapp driver on how to do this so it continues to work on
> > ACPI.
> 
> Thanks for the pointer!
> 
> >
> > I think it'd be also appropriate to add the usleep() after lifting reset
> > only if the reset GPIO is defined for the device.
> 
> Ack

On second thought, that probably applies if any of the resources needed for
powering the device on are defined. It could be that there's no reset GPIO
but a regulator is still there, in which case a delay is needed.

-- 
Sakari Ailus

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

[Robert Foss]

On Tue, 10 Mar 2020 at 16:51, Robert Foss <robert.foss@linaro.org> wrote:
>
> Hey Fabio,
>
> Thanks for having a look at this series so quickly.
>
> On Tue, 10 Mar 2020 at 14:57, Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Robert,
> >
> > On Tue, Mar 10, 2020 at 10:46 AM Robert Foss <robert.foss@linaro.org> wrote:
> >
> > > +    ov8856: camera-sensor@10 {
> > > +        compatible = "ovti,ov8856";
> > > +        reg = <0x10>;
> > > +        reset-gpios = <&pio 111 GPIO_ACTIVE_HIGH>;
> >
> > Could you double check this is correct? Other OmniVision sensors have
> > reset-gpios as active low.
>
> I have tested this, unfortunately I don't have access to a ov8856
> datasheet that includes
> this level of detail. But I have tested this.
>
> >
> > I suspect that the driver has also an inverted logic, so that's why it works.
>
> That could explain it still working. Let me have a look into the
> driver and see what it does.
>

I had a look at some of OmniVision drivers, and there does seem to be
a logical inversion in some of them,
but not all of them.

ov7251:
- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
  to the hardware pin XSHUTDOWN which is physically active low.

ov5640:
- reset-gpios: reference to the GPIO connected to the reset pin, if any.
           This is an active low signal to the OV5640.

I think the confusion stems from the XSHUTDOWN pin being mapped to a
GPIO called reset, and the two being logically inverted. Currently
this series does several mappings.

XSHUTDOWN -> reset-gpio -> n_shutdn_gpio
       ^                           ^                      ^
Physical Pin               DT                Driver

I think changing to what ov5640 does makes the most sense.
XSHUTDOWN -> reset-gpio -> reset_gpio

> >
> > I don't have access to the datasheet though, so I am just guessing.
>
> Me neither unfortunately, if anyone does have a link for it, I would
> very much appreciate it.

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

[Robert Foss]

Hey Andy,

On Tue, 10 Mar 2020 at 15:30, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Mar 10, 2020 at 02:46:03PM +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.
>
> Are you sure you send latest version?
>
> I have a déją vu that I have seen this already and this one doesn't address any
> comment given.

I think pulled a series Dongchun Zhus earlier series apart and used some of it,
I may have missed some of the feedback given to his v3. Sorry about that.

>
> ...
>
> > +     dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n",
> > +             val,
>
> > +             val == OV8856_1B_MODULE ? "1B" : "unknown revision",
>
> This is weird. Can you add a bit more general way of showing revision?

This is modeled after the ov7251 driver, since that output came in
handy during bringup.

    dev_info(dev, "OV7251 revision %x (%s) detected at address 0x%02x\n",
         chip_rev,
         chip_rev == 0x4 ? "1A / 1B" :
         chip_rev == 0x5 ? "1C / 1D" :
         chip_rev == 0x6 ? "1E" :
         chip_rev == 0x7 ? "1F" : "unknown",
         client->addr);

To me this is pretty general approach, at least until this revision
information is used in other places.
I'm not quite sure what you had in mind. Maybe the current
implementation is a little bit clunky in the case of ov8856 since
there's only one revision number known currently.

Either way, I'll happily change it. But I don't quite know what you
have in mind.

>
> > +             client->addr);
>
> --
> With Best Regards,
> Andy Shevchenko
>
>