[PATCH v6 v6 0/3] media: ov8856: Add devicetree support

[PATCH v6 v6 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 | 140 +++++++++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/ov8856.c                    | 190 ++++++++++++++++--
 3 files changed, 317 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml

-- 
2.25.1

[PATCH v6 v6 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 | 140 +++++++++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/ov8856.c                    | 190 ++++++++++++++++--
 3 files changed, 317 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml

-- 
2.25.1


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

[PATCH v9 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>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
---

- Changes since v8:
  * Maxime: Added r-b

- Changes since v7:
  * Marco: Make 'port' property optional
  * Maxime & Sakari: Add 'link-frequencies' property to dt binding
  * robher: Improve description for 'port' property

- Changes since v6:
  * Marco: remove qcom specifics from DT example
   
- Changes since v5:
  * Add assigned-clocks and assigned-clock-rates
  * robher: dt-schema errors

- 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 | 140 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 141 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..f78d3eae81cb
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
@@ -0,0 +1,140 @@
+# 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 an output port node with an endpoint definition
+      as documented in
+      Documentation/devicetree/bindings/media/video-interfaces.txt
+
+    properties:
+      endpoint:
+        type: object
+
+        properties:
+          clock-lanes:
+            maxItems: 1
+
+          data-lanes:
+            maxItems: 1
+
+          link-frequencies:
+            maxItems: 1
+
+          remote-endpoint: true
+
+        required:
+          - clock-lanes
+          - data-lanes
+          - remote-endpoint
+          - link-frequencies
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - clock-frequency
+  - dovdd-supply
+  - avdd-supply
+  - dvdd-supply
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov8856: camera@10 {
+            compatible = "ovti,ov8856";
+            reg = <0x10>;
+
+            reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&clk_24m_cam>;
+
+            clocks = <&cam_osc>;
+            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 26f281d9f32a..84b262afd13d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12489,6 +12489,7 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
 F:	drivers/media/i2c/ov8856.c
+F:	Documentation/devicetree/bindings/media/i2c/ov8856.yaml
 
 OMNIVISION OV9640 SENSOR DRIVER
 M:	Petr Cvek <petrcvekcz@gmail.com>
-- 
2.25.1

[PATCH v9 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>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
---

- Changes since v8:
  * Maxime: Added r-b

- Changes since v7:
  * Marco: Make 'port' property optional
  * Maxime & Sakari: Add 'link-frequencies' property to dt binding
  * robher: Improve description for 'port' property

- Changes since v6:
  * Marco: remove qcom specifics from DT example
   
- Changes since v5:
  * Add assigned-clocks and assigned-clock-rates
  * robher: dt-schema errors

- 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 | 140 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 141 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..f78d3eae81cb
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
@@ -0,0 +1,140 @@
+# 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 an output port node with an endpoint definition
+      as documented in
+      Documentation/devicetree/bindings/media/video-interfaces.txt
+
+    properties:
+      endpoint:
+        type: object
+
+        properties:
+          clock-lanes:
+            maxItems: 1
+
+          data-lanes:
+            maxItems: 1
+
+          link-frequencies:
+            maxItems: 1
+
+          remote-endpoint: true
+
+        required:
+          - clock-lanes
+          - data-lanes
+          - remote-endpoint
+          - link-frequencies
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - clock-frequency
+  - dovdd-supply
+  - avdd-supply
+  - dvdd-supply
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov8856: camera@10 {
+            compatible = "ovti,ov8856";
+            reg = <0x10>;
+
+            reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&clk_24m_cam>;
+
+            clocks = <&cam_osc>;
+            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 26f281d9f32a..84b262afd13d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12489,6 +12489,7 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
 F:	drivers/media/i2c/ov8856.c
+F:	Documentation/devicetree/bindings/media/i2c/ov8856.yaml
 
 OMNIVISION OV9640 SENSOR DRIVER
 M:	Petr Cvek <petrcvekcz@gmail.com>
-- 
2.25.1


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

[PATCH v6 2/3] media: ov8856: Add devicetree support

[Robert Foss]

Add match table, enable ov8856_probe() to support
both ACPI and DT modes.

ACPI and DT modes are primarily distinguished from
each other by relying on devm_XXX_get_optional()
will return NULL instead of a reference for the
desired managed resource.

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

- Changes since v5:
  * Maxime & Sakari: Replaced clock tolerance check with warning

- Changes since v4:
  * Maxime & Sakari: Switch to clock-frequency

- Changes since v3:
  * Remove redundant {}-brackets
  * Compare xvclk_rate to 5% tolerance
  * Andy: Use dev_fwnode()
  * Andy: Use %pe instead of %ld + PTR_ERR()
  * Andy: Invert reset_gpio logic
  * Andy: Remove dev_dbg() from failing reset_gpio setup
  * Andy: Use dev_err for logging for failures
  * Andy: Remove dev_warn from EDEFER/regulator error path
  * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
  * Maxime & Sakari: Verify clock frequency from DT
  * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
  * Sakari: Remove duplicate ov8856->dev assignment

- Changes since v2:
  * Added "struct device *dev" member to struct ov8856
  * Andy: Switch to optional version of devm_gpiod_get
  * Andy: Switch to optional version of devm_clk_get
  * Fabio: Add reset sleep period
  * Sakari: Unify defines for 19.2Mhz
  * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
  * Sakari: Replace dev_info() with dev_dbg()
  * Sakari: Switch induction variable type to unsigned
  * Sakari: Don't wait for reset_gpio when in ACPI mode
  * Sakari: Pull reset GPIO high on power on failure
  * Sakari: Add power on/off to resume/suspend
  * Sakari: Fix indentation
  * Sakari: Power off during ov8856_remove()
  * Sakari: Don't sleep during power-on in ACPI mode
  * Sakari: Switch to getting xvclk from clk_get_rate

- Changes since v1:
  * Andy & Sakari: Make XVCLK optional since to not break ACPI
  * 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
  * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
  * Sakari: Use XVCLK rate as provided by DT

 drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
 1 file changed, 123 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 8655842af275..4749dc74d5ad 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>
@@ -18,7 +21,7 @@
 #define OV8856_LINK_FREQ_360MHZ		360000000ULL
 #define OV8856_LINK_FREQ_180MHZ		180000000ULL
 #define OV8856_SCLK			144000000ULL
-#define OV8856_MCLK			19200000
+#define OV8856_XVCLK_19_2		19200000
 #define OV8856_DATA_LANES		4
 #define OV8856_RGB_DEPTH		10
 
@@ -64,6 +67,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 +575,11 @@ struct ov8856 {
 	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
 
+	struct device		*dev;
+	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 +922,52 @@ 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;
+	}
+
+	if (is_acpi_node(dev_fwnode(ov8856->dev)))
+		return 0;
+
+	if (ov8856->reset_gpio) {
+		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
+		usleep_range(1000, 2000);
+	}
+
+	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, 0);
+	usleep_range(1500, 1800);
+
+	return 0;
+
+disable_clk:
+	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
+	clk_disable_unprepare(ov8856->xvclk);
+
+	return ret;
+}
+
+static void __ov8856_power_off(struct ov8856 *ov8856)
+{
+	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
+	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);
@@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
 	if (ov8856->streaming)
 		ov8856_stop_streaming(ov8856);
 
+	__ov8856_power_off(ov8856);
 	mutex_unlock(&ov8856->mutex);
 
 	return 0;
@@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
 	int ret;
 
 	mutex_lock(&ov8856->mutex);
+
+	__ov8856_power_on(ov8856);
 	if (ov8856->streaming) {
 		ret = ov8856_start_streaming(ov8856);
 		if (ret) {
@@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
 	return 0;
 }
 
-static int ov8856_check_hwcfg(struct device *dev)
+static int ov8856_get_hwcfg(struct ov8856 *ov8856)
 {
+	struct device *dev = ov8856->dev;
 	struct fwnode_handle *ep;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct v4l2_fwnode_endpoint bus_cfg = {
 		.bus_type = V4L2_MBUS_CSI2_DPHY
 	};
-	u32 mclk;
+	u32 xvclk_rate;
 	int ret;
 	unsigned int i, j;
 
 	if (!fwnode)
 		return -ENXIO;
 
-	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
+	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
+		&xvclk_rate);
 	if (ret)
 		return ret;
 
-	if (mclk != OV8856_MCLK) {
-		dev_err(dev, "external clock %d is not supported", mclk);
-		return -EINVAL;
+	if (!is_acpi_node(fwnode)) {
+		ov8856->xvclk = devm_clk_get(dev, "xvclk");
+		if (IS_ERR(ov8856->xvclk)) {
+			dev_err(dev, "could not get xvclk clock (%pe)\n",
+					ov8856->xvclk);
+			return PTR_ERR(ov8856->xvclk);
+		}
+
+		clk_set_rate(ov8856->xvclk, xvclk_rate);
+		xvclk_rate = clk_get_rate(ov8856->xvclk);
 	}
 
+	if (xvclk_rate != OV8856_XVCLK_19_2)
+		dev_warn(dev, "external clock rate %d is unsupported", xvclk_rate);
+
+	ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+		GPIOD_OUT_LOW);
+	if (IS_ERR(ov8856->reset_gpio))
+		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(dev, ARRAY_SIZE(ov8856_supply_names),
+				      ov8856->supplies);
+	if (ret)
+		return ret;
+
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
 	if (!ep)
 		return -ENXIO;
@@ -1169,6 +1257,8 @@ static int ov8856_remove(struct i2c_client *client)
 	pm_runtime_disable(&client->dev);
 	mutex_destroy(&ov8856->mutex);
 
+	__ov8856_power_off(ov8856);
+
 	return 0;
 }
 
@@ -1177,22 +1267,31 @@ static int ov8856_probe(struct i2c_client *client)
 	struct ov8856 *ov8856;
 	int ret;
 
-	ret = ov8856_check_hwcfg(&client->dev);
+	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
+	if (!ov8856)
+		return -ENOMEM;
+
+	ov8856->dev = &client->dev;
+
+	ret = ov8856_get_hwcfg(ov8856);
 	if (ret) {
-		dev_err(&client->dev, "failed to check HW configuration: %d",
+		dev_err(&client->dev, "failed to get HW configuration: %d",
 			ret);
 		return ret;
 	}
 
-	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
-	if (!ov8856)
-		return -ENOMEM;
-
 	v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
+
+	ret = __ov8856_power_on(ov8856);
+	if (ret) {
+		dev_err(&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 +1337,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 +1356,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.25.1

[PATCH v6 2/3] media: ov8856: Add devicetree support

[Robert Foss]

Add match table, enable ov8856_probe() to support
both ACPI and DT modes.

ACPI and DT modes are primarily distinguished from
each other by relying on devm_XXX_get_optional()
will return NULL instead of a reference for the
desired managed resource.

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

- Changes since v5:
  * Maxime & Sakari: Replaced clock tolerance check with warning

- Changes since v4:
  * Maxime & Sakari: Switch to clock-frequency

- Changes since v3:
  * Remove redundant {}-brackets
  * Compare xvclk_rate to 5% tolerance
  * Andy: Use dev_fwnode()
  * Andy: Use %pe instead of %ld + PTR_ERR()
  * Andy: Invert reset_gpio logic
  * Andy: Remove dev_dbg() from failing reset_gpio setup
  * Andy: Use dev_err for logging for failures
  * Andy: Remove dev_warn from EDEFER/regulator error path
  * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
  * Maxime & Sakari: Verify clock frequency from DT
  * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
  * Sakari: Remove duplicate ov8856->dev assignment

- Changes since v2:
  * Added "struct device *dev" member to struct ov8856
  * Andy: Switch to optional version of devm_gpiod_get
  * Andy: Switch to optional version of devm_clk_get
  * Fabio: Add reset sleep period
  * Sakari: Unify defines for 19.2Mhz
  * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
  * Sakari: Replace dev_info() with dev_dbg()
  * Sakari: Switch induction variable type to unsigned
  * Sakari: Don't wait for reset_gpio when in ACPI mode
  * Sakari: Pull reset GPIO high on power on failure
  * Sakari: Add power on/off to resume/suspend
  * Sakari: Fix indentation
  * Sakari: Power off during ov8856_remove()
  * Sakari: Don't sleep during power-on in ACPI mode
  * Sakari: Switch to getting xvclk from clk_get_rate

- Changes since v1:
  * Andy & Sakari: Make XVCLK optional since to not break ACPI
  * 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
  * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
  * Sakari: Use XVCLK rate as provided by DT

 drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
 1 file changed, 123 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 8655842af275..4749dc74d5ad 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>
@@ -18,7 +21,7 @@
 #define OV8856_LINK_FREQ_360MHZ		360000000ULL
 #define OV8856_LINK_FREQ_180MHZ		180000000ULL
 #define OV8856_SCLK			144000000ULL
-#define OV8856_MCLK			19200000
+#define OV8856_XVCLK_19_2		19200000
 #define OV8856_DATA_LANES		4
 #define OV8856_RGB_DEPTH		10
 
@@ -64,6 +67,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 +575,11 @@ struct ov8856 {
 	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
 
+	struct device		*dev;
+	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 +922,52 @@ 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;
+	}
+
+	if (is_acpi_node(dev_fwnode(ov8856->dev)))
+		return 0;
+
+	if (ov8856->reset_gpio) {
+		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
+		usleep_range(1000, 2000);
+	}
+
+	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, 0);
+	usleep_range(1500, 1800);
+
+	return 0;
+
+disable_clk:
+	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
+	clk_disable_unprepare(ov8856->xvclk);
+
+	return ret;
+}
+
+static void __ov8856_power_off(struct ov8856 *ov8856)
+{
+	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
+	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);
@@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
 	if (ov8856->streaming)
 		ov8856_stop_streaming(ov8856);
 
+	__ov8856_power_off(ov8856);
 	mutex_unlock(&ov8856->mutex);
 
 	return 0;
@@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
 	int ret;
 
 	mutex_lock(&ov8856->mutex);
+
+	__ov8856_power_on(ov8856);
 	if (ov8856->streaming) {
 		ret = ov8856_start_streaming(ov8856);
 		if (ret) {
@@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
 	return 0;
 }
 
-static int ov8856_check_hwcfg(struct device *dev)
+static int ov8856_get_hwcfg(struct ov8856 *ov8856)
 {
+	struct device *dev = ov8856->dev;
 	struct fwnode_handle *ep;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct v4l2_fwnode_endpoint bus_cfg = {
 		.bus_type = V4L2_MBUS_CSI2_DPHY
 	};
-	u32 mclk;
+	u32 xvclk_rate;
 	int ret;
 	unsigned int i, j;
 
 	if (!fwnode)
 		return -ENXIO;
 
-	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
+	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
+		&xvclk_rate);
 	if (ret)
 		return ret;
 
-	if (mclk != OV8856_MCLK) {
-		dev_err(dev, "external clock %d is not supported", mclk);
-		return -EINVAL;
+	if (!is_acpi_node(fwnode)) {
+		ov8856->xvclk = devm_clk_get(dev, "xvclk");
+		if (IS_ERR(ov8856->xvclk)) {
+			dev_err(dev, "could not get xvclk clock (%pe)\n",
+					ov8856->xvclk);
+			return PTR_ERR(ov8856->xvclk);
+		}
+
+		clk_set_rate(ov8856->xvclk, xvclk_rate);
+		xvclk_rate = clk_get_rate(ov8856->xvclk);
 	}
 
+	if (xvclk_rate != OV8856_XVCLK_19_2)
+		dev_warn(dev, "external clock rate %d is unsupported", xvclk_rate);
+
+	ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+		GPIOD_OUT_LOW);
+	if (IS_ERR(ov8856->reset_gpio))
+		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(dev, ARRAY_SIZE(ov8856_supply_names),
+				      ov8856->supplies);
+	if (ret)
+		return ret;
+
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
 	if (!ep)
 		return -ENXIO;
@@ -1169,6 +1257,8 @@ static int ov8856_remove(struct i2c_client *client)
 	pm_runtime_disable(&client->dev);
 	mutex_destroy(&ov8856->mutex);
 
+	__ov8856_power_off(ov8856);
+
 	return 0;
 }
 
@@ -1177,22 +1267,31 @@ static int ov8856_probe(struct i2c_client *client)
 	struct ov8856 *ov8856;
 	int ret;
 
-	ret = ov8856_check_hwcfg(&client->dev);
+	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
+	if (!ov8856)
+		return -ENOMEM;
+
+	ov8856->dev = &client->dev;
+
+	ret = ov8856_get_hwcfg(ov8856);
 	if (ret) {
-		dev_err(&client->dev, "failed to check HW configuration: %d",
+		dev_err(&client->dev, "failed to get HW configuration: %d",
 			ret);
 		return ret;
 	}
 
-	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
-	if (!ov8856)
-		return -ENOMEM;
-
 	v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
+
+	ret = __ov8856_power_on(ov8856);
+	if (ret) {
+		dev_err(&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 +1337,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 +1356,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.25.1


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

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

[Marco Felsch]

Hi Robert,

thnakf for the patch but pls keep in mind to do one thing per patch.
IMHO this patch do a lot more. Anyway below are my comment :)

On 20-04-29 18:24, Robert Foss wrote:
> Add match table, enable ov8856_probe() to support
> both ACPI and DT modes.

You are also adding the support for reset-gpios and regualtors. IMHO the
commit message don't belong to the changes you made anymore.

> ACPI and DT modes are primarily distinguished from
> each other by relying on devm_XXX_get_optional()
> will return NULL instead of a reference for the
> desired managed resource.
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
> 
> - Changes since v5:
>   * Maxime & Sakari: Replaced clock tolerance check with warning
> 
> - Changes since v4:
>   * Maxime & Sakari: Switch to clock-frequency
> 
> - Changes since v3:
>   * Remove redundant {}-brackets
>   * Compare xvclk_rate to 5% tolerance
>   * Andy: Use dev_fwnode()
>   * Andy: Use %pe instead of %ld + PTR_ERR()
>   * Andy: Invert reset_gpio logic
>   * Andy: Remove dev_dbg() from failing reset_gpio setup
>   * Andy: Use dev_err for logging for failures
>   * Andy: Remove dev_warn from EDEFER/regulator error path
>   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
>   * Maxime & Sakari: Verify clock frequency from DT
>   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
>   * Sakari: Remove duplicate ov8856->dev assignment
> 
> - Changes since v2:
>   * Added "struct device *dev" member to struct ov8856
>   * Andy: Switch to optional version of devm_gpiod_get
>   * Andy: Switch to optional version of devm_clk_get
>   * Fabio: Add reset sleep period
>   * Sakari: Unify defines for 19.2Mhz
>   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
>   * Sakari: Replace dev_info() with dev_dbg()
>   * Sakari: Switch induction variable type to unsigned
>   * Sakari: Don't wait for reset_gpio when in ACPI mode
>   * Sakari: Pull reset GPIO high on power on failure
>   * Sakari: Add power on/off to resume/suspend
>   * Sakari: Fix indentation
>   * Sakari: Power off during ov8856_remove()
>   * Sakari: Don't sleep during power-on in ACPI mode
>   * Sakari: Switch to getting xvclk from clk_get_rate
> 
> - Changes since v1:
>   * Andy & Sakari: Make XVCLK optional since to not break ACPI
>   * 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
>   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
>   * Sakari: Use XVCLK rate as provided by DT
> 
>  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
>  1 file changed, 123 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index 8655842af275..4749dc74d5ad 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>
> @@ -18,7 +21,7 @@
>  #define OV8856_LINK_FREQ_360MHZ		360000000ULL
>  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
>  #define OV8856_SCLK			144000000ULL
> -#define OV8856_MCLK			19200000
> +#define OV8856_XVCLK_19_2		19200000
>  #define OV8856_DATA_LANES		4
>  #define OV8856_RGB_DEPTH		10
>  
> @@ -64,6 +67,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 +575,11 @@ struct ov8856 {
>  	struct media_pad pad;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  
> +	struct device		*dev;
> +	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 +922,52 @@ 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);

You're request the clk only in DT case or do I miss something? If so you
have to check if the clk is available.

> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to enable xvclk\n");
> +		return ret;
> +	}
> +
> +	if (is_acpi_node(dev_fwnode(ov8856->dev)))
> +		return 0;
> +
> +	if (ov8856->reset_gpio) {
> +		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> +		usleep_range(1000, 2000);
> +	}
> +
> +	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, 0);

You need to check the existance of the gpio here too.

> +	usleep_range(1500, 1800);
> +
> +	return 0;
> +
> +disable_clk:
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);

And here.. pls check the whole patch.

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

Clk is only availabel in DT use-case.

> +}
> +
>  static int __maybe_unused ov8856_suspend(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> @@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
>  	if (ov8856->streaming)
>  		ov8856_stop_streaming(ov8856);
>  
> +	__ov8856_power_off(ov8856);
>  	mutex_unlock(&ov8856->mutex);
>  
>  	return 0;
> @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
>  	int ret;
>  
>  	mutex_lock(&ov8856->mutex);
> +
> +	__ov8856_power_on(ov8856);
>  	if (ov8856->streaming) {
>  		ret = ov8856_start_streaming(ov8856);
>  		if (ret) {
> @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
>  	return 0;
>  }
>  
> -static int ov8856_check_hwcfg(struct device *dev)
> +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
>  {
> +	struct device *dev = ov8856->dev;
>  	struct fwnode_handle *ep;
>  	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct v4l2_fwnode_endpoint bus_cfg = {
>  		.bus_type = V4L2_MBUS_CSI2_DPHY
>  	};
> -	u32 mclk;
> +	u32 xvclk_rate;
>  	int ret;
>  	unsigned int i, j;
>  
>  	if (!fwnode)
>  		return -ENXIO;
>  
> -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> +	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> +		&xvclk_rate);
>  	if (ret)
>  		return ret;
>  
> -	if (mclk != OV8856_MCLK) {
> -		dev_err(dev, "external clock %d is not supported", mclk);
> -		return -EINVAL;
> +	if (!is_acpi_node(fwnode)) {
> +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> +		if (IS_ERR(ov8856->xvclk)) {
> +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> +					ov8856->xvclk);
> +			return PTR_ERR(ov8856->xvclk);
> +		}
> +
> +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> +		xvclk_rate = clk_get_rate(ov8856->xvclk);
>  	}

Why do we handle the clock only in DT case? Is there a problem with the
clock handling and ACPI?

> +	if (xvclk_rate != OV8856_XVCLK_19_2)
> +		dev_warn(dev, "external clock rate %d is unsupported", xvclk_rate);
> +
> +	ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +		GPIOD_OUT_LOW);

Pls align this.

Regards,
  Marco

> +	if (IS_ERR(ov8856->reset_gpio))
> +		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(dev, ARRAY_SIZE(ov8856_supply_names),
> +				      ov8856->supplies);
> +	if (ret)
> +		return ret;
> +
>  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>  	if (!ep)
>  		return -ENXIO;
> @@ -1169,6 +1257,8 @@ static int ov8856_remove(struct i2c_client *client)
>  	pm_runtime_disable(&client->dev);
>  	mutex_destroy(&ov8856->mutex);
>  
> +	__ov8856_power_off(ov8856);
> +
>  	return 0;
>  }
>  
> @@ -1177,22 +1267,31 @@ static int ov8856_probe(struct i2c_client *client)
>  	struct ov8856 *ov8856;
>  	int ret;
>  
> -	ret = ov8856_check_hwcfg(&client->dev);
> +	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> +	if (!ov8856)
> +		return -ENOMEM;
> +
> +	ov8856->dev = &client->dev;
> +
> +	ret = ov8856_get_hwcfg(ov8856);
>  	if (ret) {
> -		dev_err(&client->dev, "failed to check HW configuration: %d",
> +		dev_err(&client->dev, "failed to get HW configuration: %d",
>  			ret);
>  		return ret;
>  	}
>  
> -	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> -	if (!ov8856)
> -		return -ENOMEM;
> -
>  	v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> +
> +	ret = __ov8856_power_on(ov8856);
> +	if (ret) {
> +		dev_err(&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 +1337,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 +1356,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.25.1
> 

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

[Marco Felsch]

Hi Robert,

thnakf for the patch but pls keep in mind to do one thing per patch.
IMHO this patch do a lot more. Anyway below are my comment :)

On 20-04-29 18:24, Robert Foss wrote:
> Add match table, enable ov8856_probe() to support
> both ACPI and DT modes.

You are also adding the support for reset-gpios and regualtors. IMHO the
commit message don't belong to the changes you made anymore.

> ACPI and DT modes are primarily distinguished from
> each other by relying on devm_XXX_get_optional()
> will return NULL instead of a reference for the
> desired managed resource.
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
> 
> - Changes since v5:
>   * Maxime & Sakari: Replaced clock tolerance check with warning
> 
> - Changes since v4:
>   * Maxime & Sakari: Switch to clock-frequency
> 
> - Changes since v3:
>   * Remove redundant {}-brackets
>   * Compare xvclk_rate to 5% tolerance
>   * Andy: Use dev_fwnode()
>   * Andy: Use %pe instead of %ld + PTR_ERR()
>   * Andy: Invert reset_gpio logic
>   * Andy: Remove dev_dbg() from failing reset_gpio setup
>   * Andy: Use dev_err for logging for failures
>   * Andy: Remove dev_warn from EDEFER/regulator error path
>   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
>   * Maxime & Sakari: Verify clock frequency from DT
>   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
>   * Sakari: Remove duplicate ov8856->dev assignment
> 
> - Changes since v2:
>   * Added "struct device *dev" member to struct ov8856
>   * Andy: Switch to optional version of devm_gpiod_get
>   * Andy: Switch to optional version of devm_clk_get
>   * Fabio: Add reset sleep period
>   * Sakari: Unify defines for 19.2Mhz
>   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
>   * Sakari: Replace dev_info() with dev_dbg()
>   * Sakari: Switch induction variable type to unsigned
>   * Sakari: Don't wait for reset_gpio when in ACPI mode
>   * Sakari: Pull reset GPIO high on power on failure
>   * Sakari: Add power on/off to resume/suspend
>   * Sakari: Fix indentation
>   * Sakari: Power off during ov8856_remove()
>   * Sakari: Don't sleep during power-on in ACPI mode
>   * Sakari: Switch to getting xvclk from clk_get_rate
> 
> - Changes since v1:
>   * Andy & Sakari: Make XVCLK optional since to not break ACPI
>   * 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
>   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
>   * Sakari: Use XVCLK rate as provided by DT
> 
>  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
>  1 file changed, 123 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index 8655842af275..4749dc74d5ad 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>
> @@ -18,7 +21,7 @@
>  #define OV8856_LINK_FREQ_360MHZ		360000000ULL
>  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
>  #define OV8856_SCLK			144000000ULL
> -#define OV8856_MCLK			19200000
> +#define OV8856_XVCLK_19_2		19200000
>  #define OV8856_DATA_LANES		4
>  #define OV8856_RGB_DEPTH		10
>  
> @@ -64,6 +67,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 +575,11 @@ struct ov8856 {
>  	struct media_pad pad;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  
> +	struct device		*dev;
> +	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 +922,52 @@ 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);

You're request the clk only in DT case or do I miss something? If so you
have to check if the clk is available.

> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to enable xvclk\n");
> +		return ret;
> +	}
> +
> +	if (is_acpi_node(dev_fwnode(ov8856->dev)))
> +		return 0;
> +
> +	if (ov8856->reset_gpio) {
> +		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> +		usleep_range(1000, 2000);
> +	}
> +
> +	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, 0);

You need to check the existance of the gpio here too.

> +	usleep_range(1500, 1800);
> +
> +	return 0;
> +
> +disable_clk:
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);

And here.. pls check the whole patch.

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

Clk is only availabel in DT use-case.

> +}
> +
>  static int __maybe_unused ov8856_suspend(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> @@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
>  	if (ov8856->streaming)
>  		ov8856_stop_streaming(ov8856);
>  
> +	__ov8856_power_off(ov8856);
>  	mutex_unlock(&ov8856->mutex);
>  
>  	return 0;
> @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
>  	int ret;
>  
>  	mutex_lock(&ov8856->mutex);
> +
> +	__ov8856_power_on(ov8856);
>  	if (ov8856->streaming) {
>  		ret = ov8856_start_streaming(ov8856);
>  		if (ret) {
> @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
>  	return 0;
>  }
>  
> -static int ov8856_check_hwcfg(struct device *dev)
> +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
>  {
> +	struct device *dev = ov8856->dev;
>  	struct fwnode_handle *ep;
>  	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct v4l2_fwnode_endpoint bus_cfg = {
>  		.bus_type = V4L2_MBUS_CSI2_DPHY
>  	};
> -	u32 mclk;
> +	u32 xvclk_rate;
>  	int ret;
>  	unsigned int i, j;
>  
>  	if (!fwnode)
>  		return -ENXIO;
>  
> -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> +	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> +		&xvclk_rate);
>  	if (ret)
>  		return ret;
>  
> -	if (mclk != OV8856_MCLK) {
> -		dev_err(dev, "external clock %d is not supported", mclk);
> -		return -EINVAL;
> +	if (!is_acpi_node(fwnode)) {
> +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> +		if (IS_ERR(ov8856->xvclk)) {
> +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> +					ov8856->xvclk);
> +			return PTR_ERR(ov8856->xvclk);
> +		}
> +
> +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> +		xvclk_rate = clk_get_rate(ov8856->xvclk);
>  	}

Why do we handle the clock only in DT case? Is there a problem with the
clock handling and ACPI?

> +	if (xvclk_rate != OV8856_XVCLK_19_2)
> +		dev_warn(dev, "external clock rate %d is unsupported", xvclk_rate);
> +
> +	ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +		GPIOD_OUT_LOW);

Pls align this.

Regards,
  Marco

> +	if (IS_ERR(ov8856->reset_gpio))
> +		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(dev, ARRAY_SIZE(ov8856_supply_names),
> +				      ov8856->supplies);
> +	if (ret)
> +		return ret;
> +
>  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>  	if (!ep)
>  		return -ENXIO;
> @@ -1169,6 +1257,8 @@ static int ov8856_remove(struct i2c_client *client)
>  	pm_runtime_disable(&client->dev);
>  	mutex_destroy(&ov8856->mutex);
>  
> +	__ov8856_power_off(ov8856);
> +
>  	return 0;
>  }
>  
> @@ -1177,22 +1267,31 @@ static int ov8856_probe(struct i2c_client *client)
>  	struct ov8856 *ov8856;
>  	int ret;
>  
> -	ret = ov8856_check_hwcfg(&client->dev);
> +	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> +	if (!ov8856)
> +		return -ENOMEM;
> +
> +	ov8856->dev = &client->dev;
> +
> +	ret = ov8856_get_hwcfg(ov8856);
>  	if (ret) {
> -		dev_err(&client->dev, "failed to check HW configuration: %d",
> +		dev_err(&client->dev, "failed to get HW configuration: %d",
>  			ret);
>  		return ret;
>  	}
>  
> -	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> -	if (!ov8856)
> -		return -ENOMEM;
> -
>  	v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> +
> +	ret = __ov8856_power_on(ov8856);
> +	if (ret) {
> +		dev_err(&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 +1337,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 +1356,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.25.1
> 

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

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

[Sakari Ailus]

Hi Marco,

On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> Hi Robert,
> 
> thnakf for the patch but pls keep in mind to do one thing per patch.
> IMHO this patch do a lot more. Anyway below are my comment :)
> 
> On 20-04-29 18:24, Robert Foss wrote:
> > Add match table, enable ov8856_probe() to support
> > both ACPI and DT modes.
> 
> You are also adding the support for reset-gpios and regualtors. IMHO the
> commit message don't belong to the changes you made anymore.
> 
> > ACPI and DT modes are primarily distinguished from
> > each other by relying on devm_XXX_get_optional()
> > will return NULL instead of a reference for the
> > desired managed resource.
> > 
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > ---
> > 
> > - Changes since v5:
> >   * Maxime & Sakari: Replaced clock tolerance check with warning
> > 
> > - Changes since v4:
> >   * Maxime & Sakari: Switch to clock-frequency
> > 
> > - Changes since v3:
> >   * Remove redundant {}-brackets
> >   * Compare xvclk_rate to 5% tolerance
> >   * Andy: Use dev_fwnode()
> >   * Andy: Use %pe instead of %ld + PTR_ERR()
> >   * Andy: Invert reset_gpio logic
> >   * Andy: Remove dev_dbg() from failing reset_gpio setup
> >   * Andy: Use dev_err for logging for failures
> >   * Andy: Remove dev_warn from EDEFER/regulator error path
> >   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
> >   * Maxime & Sakari: Verify clock frequency from DT
> >   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
> >   * Sakari: Remove duplicate ov8856->dev assignment
> > 
> > - Changes since v2:
> >   * Added "struct device *dev" member to struct ov8856
> >   * Andy: Switch to optional version of devm_gpiod_get
> >   * Andy: Switch to optional version of devm_clk_get
> >   * Fabio: Add reset sleep period
> >   * Sakari: Unify defines for 19.2Mhz
> >   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
> >   * Sakari: Replace dev_info() with dev_dbg()
> >   * Sakari: Switch induction variable type to unsigned
> >   * Sakari: Don't wait for reset_gpio when in ACPI mode
> >   * Sakari: Pull reset GPIO high on power on failure
> >   * Sakari: Add power on/off to resume/suspend
> >   * Sakari: Fix indentation
> >   * Sakari: Power off during ov8856_remove()
> >   * Sakari: Don't sleep during power-on in ACPI mode
> >   * Sakari: Switch to getting xvclk from clk_get_rate
> > 
> > - Changes since v1:
> >   * Andy & Sakari: Make XVCLK optional since to not break ACPI
> >   * 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
> >   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> >   * Sakari: Use XVCLK rate as provided by DT
> > 
> >  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 123 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > index 8655842af275..4749dc74d5ad 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>
> > @@ -18,7 +21,7 @@
> >  #define OV8856_LINK_FREQ_360MHZ		360000000ULL
> >  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
> >  #define OV8856_SCLK			144000000ULL
> > -#define OV8856_MCLK			19200000
> > +#define OV8856_XVCLK_19_2		19200000
> >  #define OV8856_DATA_LANES		4
> >  #define OV8856_RGB_DEPTH		10
> >  
> > @@ -64,6 +67,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 +575,11 @@ struct ov8856 {
> >  	struct media_pad pad;
> >  	struct v4l2_ctrl_handler ctrl_handler;
> >  
> > +	struct device		*dev;
> > +	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 +922,52 @@ 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);
> 
> You're request the clk only in DT case or do I miss something? If so you
> have to check if the clk is available.
> 
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed to enable xvclk\n");
> > +		return ret;
> > +	}
> > +
> > +	if (is_acpi_node(dev_fwnode(ov8856->dev)))
> > +		return 0;
> > +
> > +	if (ov8856->reset_gpio) {
> > +		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +		usleep_range(1000, 2000);
> > +	}
> > +
> > +	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, 0);
> 
> You need to check the existance of the gpio here too.

No need to; the GPIO framework can handle this internally.

> 
> > +	usleep_range(1500, 1800);
> > +
> > +	return 0;
> > +
> > +disable_clk:
> > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> 
> And here.. pls check the whole patch.
> 
> > +	clk_disable_unprepare(ov8856->xvclk);
> > +
> > +	return ret;
> > +}
> > +
> > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > +{
> > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > +			       ov8856->supplies);
> > +	clk_disable_unprepare(ov8856->xvclk);
> 
> Clk is only availabel in DT use-case.
> 
> > +}
> > +
> >  static int __maybe_unused ov8856_suspend(struct device *dev)
> >  {
> >  	struct i2c_client *client = to_i2c_client(dev);
> > @@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
> >  	if (ov8856->streaming)
> >  		ov8856_stop_streaming(ov8856);
> >  
> > +	__ov8856_power_off(ov8856);
> >  	mutex_unlock(&ov8856->mutex);
> >  
> >  	return 0;
> > @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
> >  	int ret;
> >  
> >  	mutex_lock(&ov8856->mutex);
> > +
> > +	__ov8856_power_on(ov8856);
> >  	if (ov8856->streaming) {
> >  		ret = ov8856_start_streaming(ov8856);
> >  		if (ret) {
> > @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> >  	return 0;
> >  }
> >  
> > -static int ov8856_check_hwcfg(struct device *dev)
> > +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
> >  {
> > +	struct device *dev = ov8856->dev;
> >  	struct fwnode_handle *ep;
> >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> >  	struct v4l2_fwnode_endpoint bus_cfg = {
> >  		.bus_type = V4L2_MBUS_CSI2_DPHY
> >  	};
> > -	u32 mclk;
> > +	u32 xvclk_rate;
> >  	int ret;
> >  	unsigned int i, j;
> >  
> >  	if (!fwnode)
> >  		return -ENXIO;
> >  
> > -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > +	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > +		&xvclk_rate);
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (mclk != OV8856_MCLK) {
> > -		dev_err(dev, "external clock %d is not supported", mclk);
> > -		return -EINVAL;
> > +	if (!is_acpi_node(fwnode)) {
> > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > +		if (IS_ERR(ov8856->xvclk)) {
> > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > +					ov8856->xvclk);
> > +			return PTR_ERR(ov8856->xvclk);
> > +		}
> > +
> > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> >  	}
> 
> Why do we handle the clock only in DT case? Is there a problem with the
> clock handling and ACPI?

Not really, it's just that ACPI does not provide an interface to the clocks
as such.

-- 
Regards,

Sakari Ailus

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

[Sakari Ailus]

Hi Marco,

On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> Hi Robert,
> 
> thnakf for the patch but pls keep in mind to do one thing per patch.
> IMHO this patch do a lot more. Anyway below are my comment :)
> 
> On 20-04-29 18:24, Robert Foss wrote:
> > Add match table, enable ov8856_probe() to support
> > both ACPI and DT modes.
> 
> You are also adding the support for reset-gpios and regualtors. IMHO the
> commit message don't belong to the changes you made anymore.
> 
> > ACPI and DT modes are primarily distinguished from
> > each other by relying on devm_XXX_get_optional()
> > will return NULL instead of a reference for the
> > desired managed resource.
> > 
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > ---
> > 
> > - Changes since v5:
> >   * Maxime & Sakari: Replaced clock tolerance check with warning
> > 
> > - Changes since v4:
> >   * Maxime & Sakari: Switch to clock-frequency
> > 
> > - Changes since v3:
> >   * Remove redundant {}-brackets
> >   * Compare xvclk_rate to 5% tolerance
> >   * Andy: Use dev_fwnode()
> >   * Andy: Use %pe instead of %ld + PTR_ERR()
> >   * Andy: Invert reset_gpio logic
> >   * Andy: Remove dev_dbg() from failing reset_gpio setup
> >   * Andy: Use dev_err for logging for failures
> >   * Andy: Remove dev_warn from EDEFER/regulator error path
> >   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
> >   * Maxime & Sakari: Verify clock frequency from DT
> >   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
> >   * Sakari: Remove duplicate ov8856->dev assignment
> > 
> > - Changes since v2:
> >   * Added "struct device *dev" member to struct ov8856
> >   * Andy: Switch to optional version of devm_gpiod_get
> >   * Andy: Switch to optional version of devm_clk_get
> >   * Fabio: Add reset sleep period
> >   * Sakari: Unify defines for 19.2Mhz
> >   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
> >   * Sakari: Replace dev_info() with dev_dbg()
> >   * Sakari: Switch induction variable type to unsigned
> >   * Sakari: Don't wait for reset_gpio when in ACPI mode
> >   * Sakari: Pull reset GPIO high on power on failure
> >   * Sakari: Add power on/off to resume/suspend
> >   * Sakari: Fix indentation
> >   * Sakari: Power off during ov8856_remove()
> >   * Sakari: Don't sleep during power-on in ACPI mode
> >   * Sakari: Switch to getting xvclk from clk_get_rate
> > 
> > - Changes since v1:
> >   * Andy & Sakari: Make XVCLK optional since to not break ACPI
> >   * 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
> >   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> >   * Sakari: Use XVCLK rate as provided by DT
> > 
> >  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 123 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > index 8655842af275..4749dc74d5ad 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>
> > @@ -18,7 +21,7 @@
> >  #define OV8856_LINK_FREQ_360MHZ		360000000ULL
> >  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
> >  #define OV8856_SCLK			144000000ULL
> > -#define OV8856_MCLK			19200000
> > +#define OV8856_XVCLK_19_2		19200000
> >  #define OV8856_DATA_LANES		4
> >  #define OV8856_RGB_DEPTH		10
> >  
> > @@ -64,6 +67,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 +575,11 @@ struct ov8856 {
> >  	struct media_pad pad;
> >  	struct v4l2_ctrl_handler ctrl_handler;
> >  
> > +	struct device		*dev;
> > +	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 +922,52 @@ 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);
> 
> You're request the clk only in DT case or do I miss something? If so you
> have to check if the clk is available.
> 
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed to enable xvclk\n");
> > +		return ret;
> > +	}
> > +
> > +	if (is_acpi_node(dev_fwnode(ov8856->dev)))
> > +		return 0;
> > +
> > +	if (ov8856->reset_gpio) {
> > +		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +		usleep_range(1000, 2000);
> > +	}
> > +
> > +	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, 0);
> 
> You need to check the existance of the gpio here too.

No need to; the GPIO framework can handle this internally.

> 
> > +	usleep_range(1500, 1800);
> > +
> > +	return 0;
> > +
> > +disable_clk:
> > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> 
> And here.. pls check the whole patch.
> 
> > +	clk_disable_unprepare(ov8856->xvclk);
> > +
> > +	return ret;
> > +}
> > +
> > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > +{
> > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > +			       ov8856->supplies);
> > +	clk_disable_unprepare(ov8856->xvclk);
> 
> Clk is only availabel in DT use-case.
> 
> > +}
> > +
> >  static int __maybe_unused ov8856_suspend(struct device *dev)
> >  {
> >  	struct i2c_client *client = to_i2c_client(dev);
> > @@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
> >  	if (ov8856->streaming)
> >  		ov8856_stop_streaming(ov8856);
> >  
> > +	__ov8856_power_off(ov8856);
> >  	mutex_unlock(&ov8856->mutex);
> >  
> >  	return 0;
> > @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
> >  	int ret;
> >  
> >  	mutex_lock(&ov8856->mutex);
> > +
> > +	__ov8856_power_on(ov8856);
> >  	if (ov8856->streaming) {
> >  		ret = ov8856_start_streaming(ov8856);
> >  		if (ret) {
> > @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> >  	return 0;
> >  }
> >  
> > -static int ov8856_check_hwcfg(struct device *dev)
> > +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
> >  {
> > +	struct device *dev = ov8856->dev;
> >  	struct fwnode_handle *ep;
> >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> >  	struct v4l2_fwnode_endpoint bus_cfg = {
> >  		.bus_type = V4L2_MBUS_CSI2_DPHY
> >  	};
> > -	u32 mclk;
> > +	u32 xvclk_rate;
> >  	int ret;
> >  	unsigned int i, j;
> >  
> >  	if (!fwnode)
> >  		return -ENXIO;
> >  
> > -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > +	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > +		&xvclk_rate);
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (mclk != OV8856_MCLK) {
> > -		dev_err(dev, "external clock %d is not supported", mclk);
> > -		return -EINVAL;
> > +	if (!is_acpi_node(fwnode)) {
> > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > +		if (IS_ERR(ov8856->xvclk)) {
> > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > +					ov8856->xvclk);
> > +			return PTR_ERR(ov8856->xvclk);
> > +		}
> > +
> > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> >  	}
> 
> Why do we handle the clock only in DT case? Is there a problem with the
> clock handling and ACPI?

Not really, it's just that ACPI does not provide an interface to the clocks
as such.

-- 
Regards,

Sakari Ailus

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

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

[Marco Felsch]

Hi Sakari,

On 20-04-30 12:45, Sakari Ailus wrote:
> Hi Marco,
> 
> On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> > Hi Robert,
> > 
> > thnakf for the patch but pls keep in mind to do one thing per patch.
> > IMHO this patch do a lot more. Anyway below are my comment :)
> > 
> > On 20-04-29 18:24, Robert Foss wrote:
> > > Add match table, enable ov8856_probe() to support
> > > both ACPI and DT modes.
> > 
> > You are also adding the support for reset-gpios and regualtors. IMHO the
> > commit message don't belong to the changes you made anymore.
> > 
> > > ACPI and DT modes are primarily distinguished from
> > > each other by relying on devm_XXX_get_optional()
> > > will return NULL instead of a reference for the
> > > desired managed resource.
> > > 
> > > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > > ---
> > > 
> > > - Changes since v5:
> > >   * Maxime & Sakari: Replaced clock tolerance check with warning
> > > 
> > > - Changes since v4:
> > >   * Maxime & Sakari: Switch to clock-frequency
> > > 
> > > - Changes since v3:
> > >   * Remove redundant {}-brackets
> > >   * Compare xvclk_rate to 5% tolerance
> > >   * Andy: Use dev_fwnode()
> > >   * Andy: Use %pe instead of %ld + PTR_ERR()
> > >   * Andy: Invert reset_gpio logic
> > >   * Andy: Remove dev_dbg() from failing reset_gpio setup
> > >   * Andy: Use dev_err for logging for failures
> > >   * Andy: Remove dev_warn from EDEFER/regulator error path
> > >   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
> > >   * Maxime & Sakari: Verify clock frequency from DT
> > >   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
> > >   * Sakari: Remove duplicate ov8856->dev assignment
> > > 
> > > - Changes since v2:
> > >   * Added "struct device *dev" member to struct ov8856
> > >   * Andy: Switch to optional version of devm_gpiod_get
> > >   * Andy: Switch to optional version of devm_clk_get
> > >   * Fabio: Add reset sleep period
> > >   * Sakari: Unify defines for 19.2Mhz
> > >   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
> > >   * Sakari: Replace dev_info() with dev_dbg()
> > >   * Sakari: Switch induction variable type to unsigned
> > >   * Sakari: Don't wait for reset_gpio when in ACPI mode
> > >   * Sakari: Pull reset GPIO high on power on failure
> > >   * Sakari: Add power on/off to resume/suspend
> > >   * Sakari: Fix indentation
> > >   * Sakari: Power off during ov8856_remove()
> > >   * Sakari: Don't sleep during power-on in ACPI mode
> > >   * Sakari: Switch to getting xvclk from clk_get_rate
> > > 
> > > - Changes since v1:
> > >   * Andy & Sakari: Make XVCLK optional since to not break ACPI
> > >   * 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
> > >   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> > >   * Sakari: Use XVCLK rate as provided by DT
> > > 
> > >  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
> > >  1 file changed, 123 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > > index 8655842af275..4749dc74d5ad 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>
> > > @@ -18,7 +21,7 @@
> > >  #define OV8856_LINK_FREQ_360MHZ		360000000ULL
> > >  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
> > >  #define OV8856_SCLK			144000000ULL
> > > -#define OV8856_MCLK			19200000
> > > +#define OV8856_XVCLK_19_2		19200000
> > >  #define OV8856_DATA_LANES		4
> > >  #define OV8856_RGB_DEPTH		10
> > >  
> > > @@ -64,6 +67,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 +575,11 @@ struct ov8856 {
> > >  	struct media_pad pad;
> > >  	struct v4l2_ctrl_handler ctrl_handler;
> > >  
> > > +	struct device		*dev;
> > > +	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 +922,52 @@ 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);
> > 
> > You're request the clk only in DT case or do I miss something? If so you
> > have to check if the clk is available.
> > 
> > > +	if (ret < 0) {
> > > +		dev_err(&client->dev, "failed to enable xvclk\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (is_acpi_node(dev_fwnode(ov8856->dev)))
> > > +		return 0;
> > > +
> > > +	if (ov8856->reset_gpio) {
> > > +		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > +		usleep_range(1000, 2000);
> > > +	}
> > > +
> > > +	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, 0);
> > 
> > You need to check the existance of the gpio here too.
> 
> No need to; the GPIO framework can handle this internally.

Ahh, I said nothing :) so all my comments about that can be dropped. 

> > > +	usleep_range(1500, 1800);
> > > +
> > > +	return 0;
> > > +
> > > +disable_clk:
> > > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > 
> > And here.. pls check the whole patch.
> > 
> > > +	clk_disable_unprepare(ov8856->xvclk);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > > +{
> > > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > +	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > > +			       ov8856->supplies);
> > > +	clk_disable_unprepare(ov8856->xvclk);
> > 
> > Clk is only availabel in DT use-case.
> > 
> > > +}
> > > +
> > >  static int __maybe_unused ov8856_suspend(struct device *dev)
> > >  {
> > >  	struct i2c_client *client = to_i2c_client(dev);
> > > @@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
> > >  	if (ov8856->streaming)
> > >  		ov8856_stop_streaming(ov8856);
> > >  
> > > +	__ov8856_power_off(ov8856);
> > >  	mutex_unlock(&ov8856->mutex);
> > >  
> > >  	return 0;
> > > @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
> > >  	int ret;
> > >  
> > >  	mutex_lock(&ov8856->mutex);
> > > +
> > > +	__ov8856_power_on(ov8856);
> > >  	if (ov8856->streaming) {
> > >  		ret = ov8856_start_streaming(ov8856);
> > >  		if (ret) {
> > > @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> > >  	return 0;
> > >  }
> > >  
> > > -static int ov8856_check_hwcfg(struct device *dev)
> > > +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
> > >  {
> > > +	struct device *dev = ov8856->dev;
> > >  	struct fwnode_handle *ep;
> > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > >  	struct v4l2_fwnode_endpoint bus_cfg = {
> > >  		.bus_type = V4L2_MBUS_CSI2_DPHY
> > >  	};
> > > -	u32 mclk;
> > > +	u32 xvclk_rate;
> > >  	int ret;
> > >  	unsigned int i, j;
> > >  
> > >  	if (!fwnode)
> > >  		return -ENXIO;
> > >  
> > > -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > +	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > > +		&xvclk_rate);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	if (mclk != OV8856_MCLK) {
> > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > -		return -EINVAL;
> > > +	if (!is_acpi_node(fwnode)) {
> > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > +		if (IS_ERR(ov8856->xvclk)) {
> > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > +					ov8856->xvclk);
> > > +			return PTR_ERR(ov8856->xvclk);
> > > +		}
> > > +
> > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > >  	}
> > 
> > Why do we handle the clock only in DT case? Is there a problem with the
> > clock handling and ACPI?
> 
> Not really, it's just that ACPI does not provide an interface to the clocks
> as such.

But you will get a clk by devm_clk_get()?

> 
> -- 
> Regards,
> 
> Sakari Ailus
> 

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

[Marco Felsch]

Hi Sakari,

On 20-04-30 12:45, Sakari Ailus wrote:
> Hi Marco,
> 
> On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> > Hi Robert,
> > 
> > thnakf for the patch but pls keep in mind to do one thing per patch.
> > IMHO this patch do a lot more. Anyway below are my comment :)
> > 
> > On 20-04-29 18:24, Robert Foss wrote:
> > > Add match table, enable ov8856_probe() to support
> > > both ACPI and DT modes.
> > 
> > You are also adding the support for reset-gpios and regualtors. IMHO the
> > commit message don't belong to the changes you made anymore.
> > 
> > > ACPI and DT modes are primarily distinguished from
> > > each other by relying on devm_XXX_get_optional()
> > > will return NULL instead of a reference for the
> > > desired managed resource.
> > > 
> > > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > > ---
> > > 
> > > - Changes since v5:
> > >   * Maxime & Sakari: Replaced clock tolerance check with warning
> > > 
> > > - Changes since v4:
> > >   * Maxime & Sakari: Switch to clock-frequency
> > > 
> > > - Changes since v3:
> > >   * Remove redundant {}-brackets
> > >   * Compare xvclk_rate to 5% tolerance
> > >   * Andy: Use dev_fwnode()
> > >   * Andy: Use %pe instead of %ld + PTR_ERR()
> > >   * Andy: Invert reset_gpio logic
> > >   * Andy: Remove dev_dbg() from failing reset_gpio setup
> > >   * Andy: Use dev_err for logging for failures
> > >   * Andy: Remove dev_warn from EDEFER/regulator error path
> > >   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
> > >   * Maxime & Sakari: Verify clock frequency from DT
> > >   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
> > >   * Sakari: Remove duplicate ov8856->dev assignment
> > > 
> > > - Changes since v2:
> > >   * Added "struct device *dev" member to struct ov8856
> > >   * Andy: Switch to optional version of devm_gpiod_get
> > >   * Andy: Switch to optional version of devm_clk_get
> > >   * Fabio: Add reset sleep period
> > >   * Sakari: Unify defines for 19.2Mhz
> > >   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
> > >   * Sakari: Replace dev_info() with dev_dbg()
> > >   * Sakari: Switch induction variable type to unsigned
> > >   * Sakari: Don't wait for reset_gpio when in ACPI mode
> > >   * Sakari: Pull reset GPIO high on power on failure
> > >   * Sakari: Add power on/off to resume/suspend
> > >   * Sakari: Fix indentation
> > >   * Sakari: Power off during ov8856_remove()
> > >   * Sakari: Don't sleep during power-on in ACPI mode
> > >   * Sakari: Switch to getting xvclk from clk_get_rate
> > > 
> > > - Changes since v1:
> > >   * Andy & Sakari: Make XVCLK optional since to not break ACPI
> > >   * 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
> > >   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> > >   * Sakari: Use XVCLK rate as provided by DT
> > > 
> > >  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
> > >  1 file changed, 123 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > > index 8655842af275..4749dc74d5ad 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>
> > > @@ -18,7 +21,7 @@
> > >  #define OV8856_LINK_FREQ_360MHZ		360000000ULL
> > >  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
> > >  #define OV8856_SCLK			144000000ULL
> > > -#define OV8856_MCLK			19200000
> > > +#define OV8856_XVCLK_19_2		19200000
> > >  #define OV8856_DATA_LANES		4
> > >  #define OV8856_RGB_DEPTH		10
> > >  
> > > @@ -64,6 +67,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 +575,11 @@ struct ov8856 {
> > >  	struct media_pad pad;
> > >  	struct v4l2_ctrl_handler ctrl_handler;
> > >  
> > > +	struct device		*dev;
> > > +	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 +922,52 @@ 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);
> > 
> > You're request the clk only in DT case or do I miss something? If so you
> > have to check if the clk is available.
> > 
> > > +	if (ret < 0) {
> > > +		dev_err(&client->dev, "failed to enable xvclk\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (is_acpi_node(dev_fwnode(ov8856->dev)))
> > > +		return 0;
> > > +
> > > +	if (ov8856->reset_gpio) {
> > > +		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > +		usleep_range(1000, 2000);
> > > +	}
> > > +
> > > +	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, 0);
> > 
> > You need to check the existance of the gpio here too.
> 
> No need to; the GPIO framework can handle this internally.

Ahh, I said nothing :) so all my comments about that can be dropped. 

> > > +	usleep_range(1500, 1800);
> > > +
> > > +	return 0;
> > > +
> > > +disable_clk:
> > > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > 
> > And here.. pls check the whole patch.
> > 
> > > +	clk_disable_unprepare(ov8856->xvclk);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > > +{
> > > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > +	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > > +			       ov8856->supplies);
> > > +	clk_disable_unprepare(ov8856->xvclk);
> > 
> > Clk is only availabel in DT use-case.
> > 
> > > +}
> > > +
> > >  static int __maybe_unused ov8856_suspend(struct device *dev)
> > >  {
> > >  	struct i2c_client *client = to_i2c_client(dev);
> > > @@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
> > >  	if (ov8856->streaming)
> > >  		ov8856_stop_streaming(ov8856);
> > >  
> > > +	__ov8856_power_off(ov8856);
> > >  	mutex_unlock(&ov8856->mutex);
> > >  
> > >  	return 0;
> > > @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
> > >  	int ret;
> > >  
> > >  	mutex_lock(&ov8856->mutex);
> > > +
> > > +	__ov8856_power_on(ov8856);
> > >  	if (ov8856->streaming) {
> > >  		ret = ov8856_start_streaming(ov8856);
> > >  		if (ret) {
> > > @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> > >  	return 0;
> > >  }
> > >  
> > > -static int ov8856_check_hwcfg(struct device *dev)
> > > +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
> > >  {
> > > +	struct device *dev = ov8856->dev;
> > >  	struct fwnode_handle *ep;
> > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > >  	struct v4l2_fwnode_endpoint bus_cfg = {
> > >  		.bus_type = V4L2_MBUS_CSI2_DPHY
> > >  	};
> > > -	u32 mclk;
> > > +	u32 xvclk_rate;
> > >  	int ret;
> > >  	unsigned int i, j;
> > >  
> > >  	if (!fwnode)
> > >  		return -ENXIO;
> > >  
> > > -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > +	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > > +		&xvclk_rate);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	if (mclk != OV8856_MCLK) {
> > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > -		return -EINVAL;
> > > +	if (!is_acpi_node(fwnode)) {
> > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > +		if (IS_ERR(ov8856->xvclk)) {
> > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > +					ov8856->xvclk);
> > > +			return PTR_ERR(ov8856->xvclk);
> > > +		}
> > > +
> > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > >  	}
> > 
> > Why do we handle the clock only in DT case? Is there a problem with the
> > clock handling and ACPI?
> 
> Not really, it's just that ACPI does not provide an interface to the clocks
> as such.

But you will get a clk by devm_clk_get()?

> 
> -- 
> Regards,
> 
> Sakari Ailus
> 

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

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

[Sakari Ailus]

Hi Marco,

On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> Hi Sakari,
> 
> On 20-04-30 12:45, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> > > Hi Robert,
> > > 
> > > thnakf for the patch but pls keep in mind to do one thing per patch.
> > > IMHO this patch do a lot more. Anyway below are my comment :)
> > > 
> > > On 20-04-29 18:24, Robert Foss wrote:
> > > > Add match table, enable ov8856_probe() to support
> > > > both ACPI and DT modes.
> > > 
> > > You are also adding the support for reset-gpios and regualtors. IMHO the
> > > commit message don't belong to the changes you made anymore.
> > > 
> > > > ACPI and DT modes are primarily distinguished from
> > > > each other by relying on devm_XXX_get_optional()
> > > > will return NULL instead of a reference for the
> > > > desired managed resource.
> > > > 
> > > > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > > > ---
> > > > 
> > > > - Changes since v5:
> > > >   * Maxime & Sakari: Replaced clock tolerance check with warning
> > > > 
> > > > - Changes since v4:
> > > >   * Maxime & Sakari: Switch to clock-frequency
> > > > 
> > > > - Changes since v3:
> > > >   * Remove redundant {}-brackets
> > > >   * Compare xvclk_rate to 5% tolerance
> > > >   * Andy: Use dev_fwnode()
> > > >   * Andy: Use %pe instead of %ld + PTR_ERR()
> > > >   * Andy: Invert reset_gpio logic
> > > >   * Andy: Remove dev_dbg() from failing reset_gpio setup
> > > >   * Andy: Use dev_err for logging for failures
> > > >   * Andy: Remove dev_warn from EDEFER/regulator error path
> > > >   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
> > > >   * Maxime & Sakari: Verify clock frequency from DT
> > > >   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
> > > >   * Sakari: Remove duplicate ov8856->dev assignment
> > > > 
> > > > - Changes since v2:
> > > >   * Added "struct device *dev" member to struct ov8856
> > > >   * Andy: Switch to optional version of devm_gpiod_get
> > > >   * Andy: Switch to optional version of devm_clk_get
> > > >   * Fabio: Add reset sleep period
> > > >   * Sakari: Unify defines for 19.2Mhz
> > > >   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
> > > >   * Sakari: Replace dev_info() with dev_dbg()
> > > >   * Sakari: Switch induction variable type to unsigned
> > > >   * Sakari: Don't wait for reset_gpio when in ACPI mode
> > > >   * Sakari: Pull reset GPIO high on power on failure
> > > >   * Sakari: Add power on/off to resume/suspend
> > > >   * Sakari: Fix indentation
> > > >   * Sakari: Power off during ov8856_remove()
> > > >   * Sakari: Don't sleep during power-on in ACPI mode
> > > >   * Sakari: Switch to getting xvclk from clk_get_rate
> > > > 
> > > > - Changes since v1:
> > > >   * Andy & Sakari: Make XVCLK optional since to not break ACPI
> > > >   * 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
> > > >   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> > > >   * Sakari: Use XVCLK rate as provided by DT
> > > > 
> > > >  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
> > > >  1 file changed, 123 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > > > index 8655842af275..4749dc74d5ad 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>
> > > > @@ -18,7 +21,7 @@
> > > >  #define OV8856_LINK_FREQ_360MHZ		360000000ULL
> > > >  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
> > > >  #define OV8856_SCLK			144000000ULL
> > > > -#define OV8856_MCLK			19200000
> > > > +#define OV8856_XVCLK_19_2		19200000
> > > >  #define OV8856_DATA_LANES		4
> > > >  #define OV8856_RGB_DEPTH		10
> > > >  
> > > > @@ -64,6 +67,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 +575,11 @@ struct ov8856 {
> > > >  	struct media_pad pad;
> > > >  	struct v4l2_ctrl_handler ctrl_handler;
> > > >  
> > > > +	struct device		*dev;
> > > > +	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 +922,52 @@ 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);
> > > 
> > > You're request the clk only in DT case or do I miss something? If so you
> > > have to check if the clk is available.
> > > 
> > > > +	if (ret < 0) {
> > > > +		dev_err(&client->dev, "failed to enable xvclk\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	if (is_acpi_node(dev_fwnode(ov8856->dev)))
> > > > +		return 0;
> > > > +
> > > > +	if (ov8856->reset_gpio) {
> > > > +		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > > +		usleep_range(1000, 2000);
> > > > +	}
> > > > +
> > > > +	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, 0);
> > > 
> > > You need to check the existance of the gpio here too.
> > 
> > No need to; the GPIO framework can handle this internally.
> 
> Ahh, I said nothing :) so all my comments about that can be dropped. 
> 
> > > > +	usleep_range(1500, 1800);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +disable_clk:
> > > > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > 
> > > And here.. pls check the whole patch.
> > > 
> > > > +	clk_disable_unprepare(ov8856->xvclk);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > > > +{
> > > > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > > +	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > > > +			       ov8856->supplies);
> > > > +	clk_disable_unprepare(ov8856->xvclk);
> > > 
> > > Clk is only availabel in DT use-case.
> > > 
> > > > +}
> > > > +
> > > >  static int __maybe_unused ov8856_suspend(struct device *dev)
> > > >  {
> > > >  	struct i2c_client *client = to_i2c_client(dev);
> > > > @@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
> > > >  	if (ov8856->streaming)
> > > >  		ov8856_stop_streaming(ov8856);
> > > >  
> > > > +	__ov8856_power_off(ov8856);
> > > >  	mutex_unlock(&ov8856->mutex);
> > > >  
> > > >  	return 0;
> > > > @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
> > > >  	int ret;
> > > >  
> > > >  	mutex_lock(&ov8856->mutex);
> > > > +
> > > > +	__ov8856_power_on(ov8856);
> > > >  	if (ov8856->streaming) {
> > > >  		ret = ov8856_start_streaming(ov8856);
> > > >  		if (ret) {
> > > > @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static int ov8856_check_hwcfg(struct device *dev)
> > > > +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
> > > >  {
> > > > +	struct device *dev = ov8856->dev;
> > > >  	struct fwnode_handle *ep;
> > > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > >  	struct v4l2_fwnode_endpoint bus_cfg = {
> > > >  		.bus_type = V4L2_MBUS_CSI2_DPHY
> > > >  	};
> > > > -	u32 mclk;
> > > > +	u32 xvclk_rate;
> > > >  	int ret;
> > > >  	unsigned int i, j;
> > > >  
> > > >  	if (!fwnode)
> > > >  		return -ENXIO;
> > > >  
> > > > -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > > +	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > > > +		&xvclk_rate);
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	if (mclk != OV8856_MCLK) {
> > > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > > -		return -EINVAL;
> > > > +	if (!is_acpi_node(fwnode)) {
> > > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > +		if (IS_ERR(ov8856->xvclk)) {
> > > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > +					ov8856->xvclk);
> > > > +			return PTR_ERR(ov8856->xvclk);
> > > > +		}
> > > > +
> > > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > >  	}
> > > 
> > > Why do we handle the clock only in DT case? Is there a problem with the
> > > clock handling and ACPI?
> > 
> > Not really, it's just that ACPI does not provide an interface to the clocks
> > as such.
> 
> But you will get a clk by devm_clk_get()?

No, because ACPI does not expose one to drivers. Effectively the entire
power sequences are implemented in ACPI, not in the driver.

-- 
Regards,

Sakari Ailus

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

[Sakari Ailus]

Hi Marco,

On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> Hi Sakari,
> 
> On 20-04-30 12:45, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> > > Hi Robert,
> > > 
> > > thnakf for the patch but pls keep in mind to do one thing per patch.
> > > IMHO this patch do a lot more. Anyway below are my comment :)
> > > 
> > > On 20-04-29 18:24, Robert Foss wrote:
> > > > Add match table, enable ov8856_probe() to support
> > > > both ACPI and DT modes.
> > > 
> > > You are also adding the support for reset-gpios and regualtors. IMHO the
> > > commit message don't belong to the changes you made anymore.
> > > 
> > > > ACPI and DT modes are primarily distinguished from
> > > > each other by relying on devm_XXX_get_optional()
> > > > will return NULL instead of a reference for the
> > > > desired managed resource.
> > > > 
> > > > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > > > ---
> > > > 
> > > > - Changes since v5:
> > > >   * Maxime & Sakari: Replaced clock tolerance check with warning
> > > > 
> > > > - Changes since v4:
> > > >   * Maxime & Sakari: Switch to clock-frequency
> > > > 
> > > > - Changes since v3:
> > > >   * Remove redundant {}-brackets
> > > >   * Compare xvclk_rate to 5% tolerance
> > > >   * Andy: Use dev_fwnode()
> > > >   * Andy: Use %pe instead of %ld + PTR_ERR()
> > > >   * Andy: Invert reset_gpio logic
> > > >   * Andy: Remove dev_dbg() from failing reset_gpio setup
> > > >   * Andy: Use dev_err for logging for failures
> > > >   * Andy: Remove dev_warn from EDEFER/regulator error path
> > > >   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
> > > >   * Maxime & Sakari: Verify clock frequency from DT
> > > >   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
> > > >   * Sakari: Remove duplicate ov8856->dev assignment
> > > > 
> > > > - Changes since v2:
> > > >   * Added "struct device *dev" member to struct ov8856
> > > >   * Andy: Switch to optional version of devm_gpiod_get
> > > >   * Andy: Switch to optional version of devm_clk_get
> > > >   * Fabio: Add reset sleep period
> > > >   * Sakari: Unify defines for 19.2Mhz
> > > >   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
> > > >   * Sakari: Replace dev_info() with dev_dbg()
> > > >   * Sakari: Switch induction variable type to unsigned
> > > >   * Sakari: Don't wait for reset_gpio when in ACPI mode
> > > >   * Sakari: Pull reset GPIO high on power on failure
> > > >   * Sakari: Add power on/off to resume/suspend
> > > >   * Sakari: Fix indentation
> > > >   * Sakari: Power off during ov8856_remove()
> > > >   * Sakari: Don't sleep during power-on in ACPI mode
> > > >   * Sakari: Switch to getting xvclk from clk_get_rate
> > > > 
> > > > - Changes since v1:
> > > >   * Andy & Sakari: Make XVCLK optional since to not break ACPI
> > > >   * 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
> > > >   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> > > >   * Sakari: Use XVCLK rate as provided by DT
> > > > 
> > > >  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
> > > >  1 file changed, 123 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > > > index 8655842af275..4749dc74d5ad 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>
> > > > @@ -18,7 +21,7 @@
> > > >  #define OV8856_LINK_FREQ_360MHZ		360000000ULL
> > > >  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
> > > >  #define OV8856_SCLK			144000000ULL
> > > > -#define OV8856_MCLK			19200000
> > > > +#define OV8856_XVCLK_19_2		19200000
> > > >  #define OV8856_DATA_LANES		4
> > > >  #define OV8856_RGB_DEPTH		10
> > > >  
> > > > @@ -64,6 +67,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 +575,11 @@ struct ov8856 {
> > > >  	struct media_pad pad;
> > > >  	struct v4l2_ctrl_handler ctrl_handler;
> > > >  
> > > > +	struct device		*dev;
> > > > +	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 +922,52 @@ 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);
> > > 
> > > You're request the clk only in DT case or do I miss something? If so you
> > > have to check if the clk is available.
> > > 
> > > > +	if (ret < 0) {
> > > > +		dev_err(&client->dev, "failed to enable xvclk\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	if (is_acpi_node(dev_fwnode(ov8856->dev)))
> > > > +		return 0;
> > > > +
> > > > +	if (ov8856->reset_gpio) {
> > > > +		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > > +		usleep_range(1000, 2000);
> > > > +	}
> > > > +
> > > > +	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, 0);
> > > 
> > > You need to check the existance of the gpio here too.
> > 
> > No need to; the GPIO framework can handle this internally.
> 
> Ahh, I said nothing :) so all my comments about that can be dropped. 
> 
> > > > +	usleep_range(1500, 1800);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +disable_clk:
> > > > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > 
> > > And here.. pls check the whole patch.
> > > 
> > > > +	clk_disable_unprepare(ov8856->xvclk);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > > > +{
> > > > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > > +	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > > > +			       ov8856->supplies);
> > > > +	clk_disable_unprepare(ov8856->xvclk);
> > > 
> > > Clk is only availabel in DT use-case.
> > > 
> > > > +}
> > > > +
> > > >  static int __maybe_unused ov8856_suspend(struct device *dev)
> > > >  {
> > > >  	struct i2c_client *client = to_i2c_client(dev);
> > > > @@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
> > > >  	if (ov8856->streaming)
> > > >  		ov8856_stop_streaming(ov8856);
> > > >  
> > > > +	__ov8856_power_off(ov8856);
> > > >  	mutex_unlock(&ov8856->mutex);
> > > >  
> > > >  	return 0;
> > > > @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
> > > >  	int ret;
> > > >  
> > > >  	mutex_lock(&ov8856->mutex);
> > > > +
> > > > +	__ov8856_power_on(ov8856);
> > > >  	if (ov8856->streaming) {
> > > >  		ret = ov8856_start_streaming(ov8856);
> > > >  		if (ret) {
> > > > @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static int ov8856_check_hwcfg(struct device *dev)
> > > > +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
> > > >  {
> > > > +	struct device *dev = ov8856->dev;
> > > >  	struct fwnode_handle *ep;
> > > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > >  	struct v4l2_fwnode_endpoint bus_cfg = {
> > > >  		.bus_type = V4L2_MBUS_CSI2_DPHY
> > > >  	};
> > > > -	u32 mclk;
> > > > +	u32 xvclk_rate;
> > > >  	int ret;
> > > >  	unsigned int i, j;
> > > >  
> > > >  	if (!fwnode)
> > > >  		return -ENXIO;
> > > >  
> > > > -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > > +	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > > > +		&xvclk_rate);
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	if (mclk != OV8856_MCLK) {
> > > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > > -		return -EINVAL;
> > > > +	if (!is_acpi_node(fwnode)) {
> > > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > +		if (IS_ERR(ov8856->xvclk)) {
> > > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > +					ov8856->xvclk);
> > > > +			return PTR_ERR(ov8856->xvclk);
> > > > +		}
> > > > +
> > > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > >  	}
> > > 
> > > Why do we handle the clock only in DT case? Is there a problem with the
> > > clock handling and ACPI?
> > 
> > Not really, it's just that ACPI does not provide an interface to the clocks
> > as such.
> 
> But you will get a clk by devm_clk_get()?

No, because ACPI does not expose one to drivers. Effectively the entire
power sequences are implemented in ACPI, not in the driver.

-- 
Regards,

Sakari Ailus

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

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

[Marco Felsch]

On 20-04-30 12:59, Sakari Ailus wrote:
> Hi Marco,
> 
> On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> > Hi Sakari,
> > 
> > On 20-04-30 12:45, Sakari Ailus wrote:
> > > Hi Marco,
> > > 
> > > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:

...

> > > > > -	if (mclk != OV8856_MCLK) {
> > > > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > > > -		return -EINVAL;
> > > > > +	if (!is_acpi_node(fwnode)) {
> > > > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > > +		if (IS_ERR(ov8856->xvclk)) {
> > > > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > > +					ov8856->xvclk);
> > > > > +			return PTR_ERR(ov8856->xvclk);
> > > > > +		}
> > > > > +
> > > > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > > >  	}
> > > > 
> > > > Why do we handle the clock only in DT case? Is there a problem with the
> > > > clock handling and ACPI?
> > > 
> > > Not really, it's just that ACPI does not provide an interface to the clocks
> > > as such.
> > 
> > But you will get a clk by devm_clk_get()?
> 
> No, because ACPI does not expose one to drivers. Effectively the entire
> power sequences are implemented in ACPI, not in the driver.
> 

Ah okay, thanks for the explanation. I'm really not into the ACPI
stuff.. So this means the __power_off / power_on should only be done if
we are using DT's?

Regards,
  Marco

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

[Marco Felsch]

On 20-04-30 12:59, Sakari Ailus wrote:
> Hi Marco,
> 
> On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> > Hi Sakari,
> > 
> > On 20-04-30 12:45, Sakari Ailus wrote:
> > > Hi Marco,
> > > 
> > > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:

...

> > > > > -	if (mclk != OV8856_MCLK) {
> > > > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > > > -		return -EINVAL;
> > > > > +	if (!is_acpi_node(fwnode)) {
> > > > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > > +		if (IS_ERR(ov8856->xvclk)) {
> > > > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > > +					ov8856->xvclk);
> > > > > +			return PTR_ERR(ov8856->xvclk);
> > > > > +		}
> > > > > +
> > > > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > > >  	}
> > > > 
> > > > Why do we handle the clock only in DT case? Is there a problem with the
> > > > clock handling and ACPI?
> > > 
> > > Not really, it's just that ACPI does not provide an interface to the clocks
> > > as such.
> > 
> > But you will get a clk by devm_clk_get()?
> 
> No, because ACPI does not expose one to drivers. Effectively the entire
> power sequences are implemented in ACPI, not in the driver.
> 

Ah okay, thanks for the explanation. I'm really not into the ACPI
stuff.. So this means the __power_off / power_on should only be done if
we are using DT's?

Regards,
  Marco

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

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

[Sakari Ailus]

On Thu, Apr 30, 2020 at 12:11:57PM +0200, Marco Felsch wrote:
> On 20-04-30 12:59, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> > > Hi Sakari,
> > > 
> > > On 20-04-30 12:45, Sakari Ailus wrote:
> > > > Hi Marco,
> > > > 
> > > > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> 
> ...
> 
> > > > > > -	if (mclk != OV8856_MCLK) {
> > > > > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > > > > -		return -EINVAL;
> > > > > > +	if (!is_acpi_node(fwnode)) {
> > > > > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > > > +		if (IS_ERR(ov8856->xvclk)) {
> > > > > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > > > +					ov8856->xvclk);
> > > > > > +			return PTR_ERR(ov8856->xvclk);
> > > > > > +		}
> > > > > > +
> > > > > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > > > >  	}
> > > > > 
> > > > > Why do we handle the clock only in DT case? Is there a problem with the
> > > > > clock handling and ACPI?
> > > > 
> > > > Not really, it's just that ACPI does not provide an interface to the clocks
> > > > as such.
> > > 
> > > But you will get a clk by devm_clk_get()?
> > 
> > No, because ACPI does not expose one to drivers. Effectively the entire
> > power sequences are implemented in ACPI, not in the driver.
> > 
> 
> Ah okay, thanks for the explanation. I'm really not into the ACPI
> stuff.. So this means the __power_off / power_on should only be done if
> we are using DT's?

Correct. That's why it bails out early. It could be yet earlier though,
without doing anything.

-- 
Sakari Ailus

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

[Sakari Ailus]

On Thu, Apr 30, 2020 at 12:11:57PM +0200, Marco Felsch wrote:
> On 20-04-30 12:59, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> > > Hi Sakari,
> > > 
> > > On 20-04-30 12:45, Sakari Ailus wrote:
> > > > Hi Marco,
> > > > 
> > > > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> 
> ...
> 
> > > > > > -	if (mclk != OV8856_MCLK) {
> > > > > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > > > > -		return -EINVAL;
> > > > > > +	if (!is_acpi_node(fwnode)) {
> > > > > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > > > +		if (IS_ERR(ov8856->xvclk)) {
> > > > > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > > > +					ov8856->xvclk);
> > > > > > +			return PTR_ERR(ov8856->xvclk);
> > > > > > +		}
> > > > > > +
> > > > > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > > > >  	}
> > > > > 
> > > > > Why do we handle the clock only in DT case? Is there a problem with the
> > > > > clock handling and ACPI?
> > > > 
> > > > Not really, it's just that ACPI does not provide an interface to the clocks
> > > > as such.
> > > 
> > > But you will get a clk by devm_clk_get()?
> > 
> > No, because ACPI does not expose one to drivers. Effectively the entire
> > power sequences are implemented in ACPI, not in the driver.
> > 
> 
> Ah okay, thanks for the explanation. I'm really not into the ACPI
> stuff.. So this means the __power_off / power_on should only be done if
> we are using DT's?

Correct. That's why it bails out early. It could be yet earlier though,
without doing anything.

-- 
Sakari Ailus

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

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

[Marco Felsch]

On 20-04-30 13:20, Sakari Ailus wrote:
> On Thu, Apr 30, 2020 at 12:11:57PM +0200, Marco Felsch wrote:
> > On 20-04-30 12:59, Sakari Ailus wrote:
> > > Hi Marco,
> > > 
> > > On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> > > > Hi Sakari,
> > > > 
> > > > On 20-04-30 12:45, Sakari Ailus wrote:
> > > > > Hi Marco,
> > > > > 
> > > > > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> > 
> > ...
> > 
> > > > > > > -	if (mclk != OV8856_MCLK) {
> > > > > > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > > > > > -		return -EINVAL;
> > > > > > > +	if (!is_acpi_node(fwnode)) {
> > > > > > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > > > > +		if (IS_ERR(ov8856->xvclk)) {
> > > > > > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > > > > +					ov8856->xvclk);
> > > > > > > +			return PTR_ERR(ov8856->xvclk);
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > > > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > > > > >  	}
> > > > > > 
> > > > > > Why do we handle the clock only in DT case? Is there a problem with the
> > > > > > clock handling and ACPI?
> > > > > 
> > > > > Not really, it's just that ACPI does not provide an interface to the clocks
> > > > > as such.
> > > > 
> > > > But you will get a clk by devm_clk_get()?
> > > 
> > > No, because ACPI does not expose one to drivers. Effectively the entire
> > > power sequences are implemented in ACPI, not in the driver.
> > > 
> > 
> > Ah okay, thanks for the explanation. I'm really not into the ACPI
> > stuff.. So this means the __power_off / power_on should only be done if
> > we are using DT's?
> 
> Correct. That's why it bails out early. It could be yet earlier though,
> without doing anything.

Yes I see. For easier and error less prone handling I would prefer:

if (is_acpi_node())
	return 0;

as first instruction for __power_off/on().

Also I would refator the ov8856_check_hwcfg() so the common part can be
used by this function and by a ov8856_parse_of() function. But thats
only my opinion.

Regards,
  Marco

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

[Marco Felsch]

On 20-04-30 13:20, Sakari Ailus wrote:
> On Thu, Apr 30, 2020 at 12:11:57PM +0200, Marco Felsch wrote:
> > On 20-04-30 12:59, Sakari Ailus wrote:
> > > Hi Marco,
> > > 
> > > On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> > > > Hi Sakari,
> > > > 
> > > > On 20-04-30 12:45, Sakari Ailus wrote:
> > > > > Hi Marco,
> > > > > 
> > > > > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> > 
> > ...
> > 
> > > > > > > -	if (mclk != OV8856_MCLK) {
> > > > > > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > > > > > -		return -EINVAL;
> > > > > > > +	if (!is_acpi_node(fwnode)) {
> > > > > > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > > > > +		if (IS_ERR(ov8856->xvclk)) {
> > > > > > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > > > > +					ov8856->xvclk);
> > > > > > > +			return PTR_ERR(ov8856->xvclk);
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > > > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > > > > >  	}
> > > > > > 
> > > > > > Why do we handle the clock only in DT case? Is there a problem with the
> > > > > > clock handling and ACPI?
> > > > > 
> > > > > Not really, it's just that ACPI does not provide an interface to the clocks
> > > > > as such.
> > > > 
> > > > But you will get a clk by devm_clk_get()?
> > > 
> > > No, because ACPI does not expose one to drivers. Effectively the entire
> > > power sequences are implemented in ACPI, not in the driver.
> > > 
> > 
> > Ah okay, thanks for the explanation. I'm really not into the ACPI
> > stuff.. So this means the __power_off / power_on should only be done if
> > we are using DT's?
> 
> Correct. That's why it bails out early. It could be yet earlier though,
> without doing anything.

Yes I see. For easier and error less prone handling I would prefer:

if (is_acpi_node())
	return 0;

as first instruction for __power_off/on().

Also I would refator the ov8856_check_hwcfg() so the common part can be
used by this function and by a ov8856_parse_of() function. But thats
only my opinion.

Regards,
  Marco

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

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

[Sakari Ailus]

On Thu, Apr 30, 2020 at 02:07:41PM +0200, Marco Felsch wrote:
> On 20-04-30 13:20, Sakari Ailus wrote:
> > On Thu, Apr 30, 2020 at 12:11:57PM +0200, Marco Felsch wrote:
> > > On 20-04-30 12:59, Sakari Ailus wrote:
> > > > Hi Marco,
> > > > 
> > > > On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> > > > > Hi Sakari,
> > > > > 
> > > > > On 20-04-30 12:45, Sakari Ailus wrote:
> > > > > > Hi Marco,
> > > > > > 
> > > > > > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> > > 
> > > ...
> > > 
> > > > > > > > -	if (mclk != OV8856_MCLK) {
> > > > > > > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > > > > > > -		return -EINVAL;
> > > > > > > > +	if (!is_acpi_node(fwnode)) {
> > > > > > > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > > > > > +		if (IS_ERR(ov8856->xvclk)) {
> > > > > > > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > > > > > +					ov8856->xvclk);
> > > > > > > > +			return PTR_ERR(ov8856->xvclk);
> > > > > > > > +		}
> > > > > > > > +
> > > > > > > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > > > > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > > > > > >  	}
> > > > > > > 
> > > > > > > Why do we handle the clock only in DT case? Is there a problem with the
> > > > > > > clock handling and ACPI?
> > > > > > 
> > > > > > Not really, it's just that ACPI does not provide an interface to the clocks
> > > > > > as such.
> > > > > 
> > > > > But you will get a clk by devm_clk_get()?
> > > > 
> > > > No, because ACPI does not expose one to drivers. Effectively the entire
> > > > power sequences are implemented in ACPI, not in the driver.
> > > > 
> > > 
> > > Ah okay, thanks for the explanation. I'm really not into the ACPI
> > > stuff.. So this means the __power_off / power_on should only be done if
> > > we are using DT's?
> > 
> > Correct. That's why it bails out early. It could be yet earlier though,
> > without doing anything.
> 
> Yes I see. For easier and error less prone handling I would prefer:
> 
> if (is_acpi_node())
> 	return 0;
> 
> as first instruction for __power_off/on().
> 
> Also I would refator the ov8856_check_hwcfg() so the common part can be
> used by this function and by a ov8856_parse_of() function. But thats
> only my opinion.

I guess it could be the same function, up to you.

-- 
Sakari Ailus

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

[Sakari Ailus]

On Thu, Apr 30, 2020 at 02:07:41PM +0200, Marco Felsch wrote:
> On 20-04-30 13:20, Sakari Ailus wrote:
> > On Thu, Apr 30, 2020 at 12:11:57PM +0200, Marco Felsch wrote:
> > > On 20-04-30 12:59, Sakari Ailus wrote:
> > > > Hi Marco,
> > > > 
> > > > On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> > > > > Hi Sakari,
> > > > > 
> > > > > On 20-04-30 12:45, Sakari Ailus wrote:
> > > > > > Hi Marco,
> > > > > > 
> > > > > > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> > > 
> > > ...
> > > 
> > > > > > > > -	if (mclk != OV8856_MCLK) {
> > > > > > > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > > > > > > -		return -EINVAL;
> > > > > > > > +	if (!is_acpi_node(fwnode)) {
> > > > > > > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > > > > > +		if (IS_ERR(ov8856->xvclk)) {
> > > > > > > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > > > > > +					ov8856->xvclk);
> > > > > > > > +			return PTR_ERR(ov8856->xvclk);
> > > > > > > > +		}
> > > > > > > > +
> > > > > > > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > > > > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > > > > > >  	}
> > > > > > > 
> > > > > > > Why do we handle the clock only in DT case? Is there a problem with the
> > > > > > > clock handling and ACPI?
> > > > > > 
> > > > > > Not really, it's just that ACPI does not provide an interface to the clocks
> > > > > > as such.
> > > > > 
> > > > > But you will get a clk by devm_clk_get()?
> > > > 
> > > > No, because ACPI does not expose one to drivers. Effectively the entire
> > > > power sequences are implemented in ACPI, not in the driver.
> > > > 
> > > 
> > > Ah okay, thanks for the explanation. I'm really not into the ACPI
> > > stuff.. So this means the __power_off / power_on should only be done if
> > > we are using DT's?
> > 
> > Correct. That's why it bails out early. It could be yet earlier though,
> > without doing anything.
> 
> Yes I see. For easier and error less prone handling I would prefer:
> 
> if (is_acpi_node())
> 	return 0;
> 
> as first instruction for __power_off/on().
> 
> Also I would refator the ov8856_check_hwcfg() so the common part can be
> used by this function and by a ov8856_parse_of() function. But thats
> only my opinion.

I guess it could be the same function, up to you.

-- 
Sakari Ailus

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

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

[Robert Foss]

Hey Marco,

On Thu, 30 Apr 2020 at 14:07, Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> On 20-04-30 13:20, Sakari Ailus wrote:
> > On Thu, Apr 30, 2020 at 12:11:57PM +0200, Marco Felsch wrote:
> > > On 20-04-30 12:59, Sakari Ailus wrote:
> > > > Hi Marco,
> > > >
> > > > On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> > > > > Hi Sakari,
> > > > >
> > > > > On 20-04-30 12:45, Sakari Ailus wrote:
> > > > > > Hi Marco,
> > > > > >
> > > > > > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> > >
> > > ...
> > >
> > > > > > > > - if (mclk != OV8856_MCLK) {
> > > > > > > > -         dev_err(dev, "external clock %d is not supported", mclk);
> > > > > > > > -         return -EINVAL;
> > > > > > > > + if (!is_acpi_node(fwnode)) {
> > > > > > > > +         ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > > > > > +         if (IS_ERR(ov8856->xvclk)) {
> > > > > > > > +                 dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > > > > > +                                 ov8856->xvclk);
> > > > > > > > +                 return PTR_ERR(ov8856->xvclk);
> > > > > > > > +         }
> > > > > > > > +
> > > > > > > > +         clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > > > > > +         xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > > > > > >   }
> > > > > > >
> > > > > > > Why do we handle the clock only in DT case? Is there a problem with the
> > > > > > > clock handling and ACPI?
> > > > > >
> > > > > > Not really, it's just that ACPI does not provide an interface to the clocks
> > > > > > as such.
> > > > >
> > > > > But you will get a clk by devm_clk_get()?
> > > >
> > > > No, because ACPI does not expose one to drivers. Effectively the entire
> > > > power sequences are implemented in ACPI, not in the driver.
> > > >
> > >
> > > Ah okay, thanks for the explanation. I'm really not into the ACPI
> > > stuff.. So this means the __power_off / power_on should only be done if
> > > we are using DT's?
> >
> > Correct. That's why it bails out early. It could be yet earlier though,
> > without doing anything.
>
> Yes I see. For easier and error less prone handling I would prefer:
>
> if (is_acpi_node())
>         return 0;
>
> as first instruction for __power_off/on().

__ov8856_power_on() does make a check like that, albeit only after
having run clk_prepare_enable() which won't do anything due to
ov8856->xvclk==NULL. So this should be fixed and be moved to after the
ACPI check.

__ov8856_power_off() has no ACPI check, but all of the calls it makes
are going to do nothing due to v8856->reset_gpio / v8856->reset_gpio /
ov8856->xvclk all being NULL or dummies. For the sake of clarity an
early ACPI check+return could be added, but if clarity is the goal a
comment would work too.

>
> Also I would refactor the ov8856_check_hwcfg() so the common part can be
> used by this function and by a ov8856_parse_of() function. But thats
> only my opinion.

I'm trying to grok the above paragraph. You'd like to see something in
the style of tc358743_probe_of()?

>
> Regards,
>   Marco

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

[Robert Foss]

Hey Marco,

On Thu, 30 Apr 2020 at 14:07, Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> On 20-04-30 13:20, Sakari Ailus wrote:
> > On Thu, Apr 30, 2020 at 12:11:57PM +0200, Marco Felsch wrote:
> > > On 20-04-30 12:59, Sakari Ailus wrote:
> > > > Hi Marco,
> > > >
> > > > On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> > > > > Hi Sakari,
> > > > >
> > > > > On 20-04-30 12:45, Sakari Ailus wrote:
> > > > > > Hi Marco,
> > > > > >
> > > > > > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> > >
> > > ...
> > >
> > > > > > > > - if (mclk != OV8856_MCLK) {
> > > > > > > > -         dev_err(dev, "external clock %d is not supported", mclk);
> > > > > > > > -         return -EINVAL;
> > > > > > > > + if (!is_acpi_node(fwnode)) {
> > > > > > > > +         ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > > > > > +         if (IS_ERR(ov8856->xvclk)) {
> > > > > > > > +                 dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > > > > > +                                 ov8856->xvclk);
> > > > > > > > +                 return PTR_ERR(ov8856->xvclk);
> > > > > > > > +         }
> > > > > > > > +
> > > > > > > > +         clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > > > > > +         xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > > > > > >   }
> > > > > > >
> > > > > > > Why do we handle the clock only in DT case? Is there a problem with the
> > > > > > > clock handling and ACPI?
> > > > > >
> > > > > > Not really, it's just that ACPI does not provide an interface to the clocks
> > > > > > as such.
> > > > >
> > > > > But you will get a clk by devm_clk_get()?
> > > >
> > > > No, because ACPI does not expose one to drivers. Effectively the entire
> > > > power sequences are implemented in ACPI, not in the driver.
> > > >
> > >
> > > Ah okay, thanks for the explanation. I'm really not into the ACPI
> > > stuff.. So this means the __power_off / power_on should only be done if
> > > we are using DT's?
> >
> > Correct. That's why it bails out early. It could be yet earlier though,
> > without doing anything.
>
> Yes I see. For easier and error less prone handling I would prefer:
>
> if (is_acpi_node())
>         return 0;
>
> as first instruction for __power_off/on().

__ov8856_power_on() does make a check like that, albeit only after
having run clk_prepare_enable() which won't do anything due to
ov8856->xvclk==NULL. So this should be fixed and be moved to after the
ACPI check.

__ov8856_power_off() has no ACPI check, but all of the calls it makes
are going to do nothing due to v8856->reset_gpio / v8856->reset_gpio /
ov8856->xvclk all being NULL or dummies. For the sake of clarity an
early ACPI check+return could be added, but if clarity is the goal a
comment would work too.

>
> Also I would refactor the ov8856_check_hwcfg() so the common part can be
> used by this function and by a ov8856_parse_of() function. But thats
> only my opinion.

I'm trying to grok the above paragraph. You'd like to see something in
the style of tc358743_probe_of()?

>
> Regards,
>   Marco

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

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

[Marco Felsch]

Hi Robert,

On 20-04-30 18:11, Robert Foss wrote:
> Hey Marco,
> 
> On Thu, 30 Apr 2020 at 14:07, Marco Felsch <m.felsch@pengutronix.de> wrote:
> >
> > On 20-04-30 13:20, Sakari Ailus wrote:
> > > On Thu, Apr 30, 2020 at 12:11:57PM +0200, Marco Felsch wrote:
> > > > On 20-04-30 12:59, Sakari Ailus wrote:
> > > > > Hi Marco,
> > > > >
> > > > > On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> > > > > > Hi Sakari,
> > > > > >
> > > > > > On 20-04-30 12:45, Sakari Ailus wrote:
> > > > > > > Hi Marco,
> > > > > > >
> > > > > > > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > > - if (mclk != OV8856_MCLK) {
> > > > > > > > > -         dev_err(dev, "external clock %d is not supported", mclk);
> > > > > > > > > -         return -EINVAL;
> > > > > > > > > + if (!is_acpi_node(fwnode)) {
> > > > > > > > > +         ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > > > > > > +         if (IS_ERR(ov8856->xvclk)) {
> > > > > > > > > +                 dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > > > > > > +                                 ov8856->xvclk);
> > > > > > > > > +                 return PTR_ERR(ov8856->xvclk);
> > > > > > > > > +         }
> > > > > > > > > +
> > > > > > > > > +         clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > > > > > > +         xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > > > > > > >   }
> > > > > > > >
> > > > > > > > Why do we handle the clock only in DT case? Is there a problem with the
> > > > > > > > clock handling and ACPI?
> > > > > > >
> > > > > > > Not really, it's just that ACPI does not provide an interface to the clocks
> > > > > > > as such.
> > > > > >
> > > > > > But you will get a clk by devm_clk_get()?
> > > > >
> > > > > No, because ACPI does not expose one to drivers. Effectively the entire
> > > > > power sequences are implemented in ACPI, not in the driver.
> > > > >
> > > >
> > > > Ah okay, thanks for the explanation. I'm really not into the ACPI
> > > > stuff.. So this means the __power_off / power_on should only be done if
> > > > we are using DT's?
> > >
> > > Correct. That's why it bails out early. It could be yet earlier though,
> > > without doing anything.
> >
> > Yes I see. For easier and error less prone handling I would prefer:
> >
> > if (is_acpi_node())
> >         return 0;
> >
> > as first instruction for __power_off/on().
> 
> __ov8856_power_on() does make a check like that, albeit only after
> having run clk_prepare_enable() which won't do anything due to
> ov8856->xvclk==NULL. So this should be fixed and be moved to after the
> ACPI check.

Yep, I saw that. I didn't checked the clk_prepare_enable() function and
just saw that we don't request the clk for the acpi case and enable it
in both cases. This doesn't sound right to me.

> __ov8856_power_off() has no ACPI check, but all of the calls it makes
> are going to do nothing due to v8856->reset_gpio / v8856->reset_gpio /
> ov8856->xvclk all being NULL or dummies. For the sake of clarity an
> early ACPI check+return could be added, but if clarity is the goal a
> comment would work too.

Thanks god that most of the library functions taking NULL into account
=) But I think we have to take the regulator count into account. Again I
don't know how the ACPI part is working. What happens if we request
regulators which aren't listed within the ACPI table? In case of DT
there will be added dummy-regulator. If this is the case for ACPI too we
are ending in an unbalanced regulator enable/disable count since you
enable it for the DT case and disable it in both cases.

> >
> > Also I would refactor the ov8856_check_hwcfg() so the common part can be
> > used by this function and by a ov8856_parse_of() function. But thats
> > only my opinion.
> 
> I'm trying to grok the above paragraph. You'd like to see something in
> the style of tc358743_probe_of()?

You don't have to if Saki is fine with the current patch. Just saying
that it would be a bit easier for the patch review.

Regards,
  Marco

> >
> > Regards,
> >   Marco
> 

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

[Marco Felsch]

Hi Robert,

On 20-04-30 18:11, Robert Foss wrote:
> Hey Marco,
> 
> On Thu, 30 Apr 2020 at 14:07, Marco Felsch <m.felsch@pengutronix.de> wrote:
> >
> > On 20-04-30 13:20, Sakari Ailus wrote:
> > > On Thu, Apr 30, 2020 at 12:11:57PM +0200, Marco Felsch wrote:
> > > > On 20-04-30 12:59, Sakari Ailus wrote:
> > > > > Hi Marco,
> > > > >
> > > > > On Thu, Apr 30, 2020 at 11:53:32AM +0200, Marco Felsch wrote:
> > > > > > Hi Sakari,
> > > > > >
> > > > > > On 20-04-30 12:45, Sakari Ailus wrote:
> > > > > > > Hi Marco,
> > > > > > >
> > > > > > > On Thu, Apr 30, 2020 at 11:35:24AM +0200, Marco Felsch wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > > - if (mclk != OV8856_MCLK) {
> > > > > > > > > -         dev_err(dev, "external clock %d is not supported", mclk);
> > > > > > > > > -         return -EINVAL;
> > > > > > > > > + if (!is_acpi_node(fwnode)) {
> > > > > > > > > +         ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > > > > > > > +         if (IS_ERR(ov8856->xvclk)) {
> > > > > > > > > +                 dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > > > > > > > +                                 ov8856->xvclk);
> > > > > > > > > +                 return PTR_ERR(ov8856->xvclk);
> > > > > > > > > +         }
> > > > > > > > > +
> > > > > > > > > +         clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > > > > > > > +         xvclk_rate = clk_get_rate(ov8856->xvclk);
> > > > > > > > >   }
> > > > > > > >
> > > > > > > > Why do we handle the clock only in DT case? Is there a problem with the
> > > > > > > > clock handling and ACPI?
> > > > > > >
> > > > > > > Not really, it's just that ACPI does not provide an interface to the clocks
> > > > > > > as such.
> > > > > >
> > > > > > But you will get a clk by devm_clk_get()?
> > > > >
> > > > > No, because ACPI does not expose one to drivers. Effectively the entire
> > > > > power sequences are implemented in ACPI, not in the driver.
> > > > >
> > > >
> > > > Ah okay, thanks for the explanation. I'm really not into the ACPI
> > > > stuff.. So this means the __power_off / power_on should only be done if
> > > > we are using DT's?
> > >
> > > Correct. That's why it bails out early. It could be yet earlier though,
> > > without doing anything.
> >
> > Yes I see. For easier and error less prone handling I would prefer:
> >
> > if (is_acpi_node())
> >         return 0;
> >
> > as first instruction for __power_off/on().
> 
> __ov8856_power_on() does make a check like that, albeit only after
> having run clk_prepare_enable() which won't do anything due to
> ov8856->xvclk==NULL. So this should be fixed and be moved to after the
> ACPI check.

Yep, I saw that. I didn't checked the clk_prepare_enable() function and
just saw that we don't request the clk for the acpi case and enable it
in both cases. This doesn't sound right to me.

> __ov8856_power_off() has no ACPI check, but all of the calls it makes
> are going to do nothing due to v8856->reset_gpio / v8856->reset_gpio /
> ov8856->xvclk all being NULL or dummies. For the sake of clarity an
> early ACPI check+return could be added, but if clarity is the goal a
> comment would work too.

Thanks god that most of the library functions taking NULL into account
=) But I think we have to take the regulator count into account. Again I
don't know how the ACPI part is working. What happens if we request
regulators which aren't listed within the ACPI table? In case of DT
there will be added dummy-regulator. If this is the case for ACPI too we
are ending in an unbalanced regulator enable/disable count since you
enable it for the DT case and disable it in both cases.

> >
> > Also I would refactor the ov8856_check_hwcfg() so the common part can be
> > used by this function and by a ov8856_parse_of() function. But thats
> > only my opinion.
> 
> I'm trying to grok the above paragraph. You'd like to see something in
> the style of tc358743_probe_of()?

You don't have to if Saki is fine with the current patch. Just saying
that it would be a bit easier for the patch review.

Regards,
  Marco

> >
> > Regards,
> >   Marco
> 

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

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

[Robert Foss]

Query the sensor for its module revision, and compare it
to known revisions.

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

- Changes since v3:
  * Actually add module revision 2A

- Changes since v2:
  * Add module revision 2A
  * Sakari: Remove ov8856_check_revision()
  * Sakari: Stop EEPROM streaming mode

 drivers/media/i2c/ov8856.c | 53 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 4749dc74d5ad..3f4000aef2ab 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -32,6 +32,19 @@
 #define OV8856_MODE_STANDBY		0x00
 #define OV8856_MODE_STREAMING		0x01
 
+/* module revisions */
+#define OV8856_2A_MODULE		0x01
+#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
@@ -1152,6 +1165,46 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
 		return -ENXIO;
 	}
 
+	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) {
+		dev_err(&client->dev, "failed to set otp mode");
+		return ret;
+	}
+
+	ret = ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
+			       OV8856_REG_VALUE_08BIT,
+			       OV8856_OTP_LOAD_CTRL_ENABLE);
+	if (ret) {
+		dev_err(&client->dev, "failed to enable load control");
+		return ret;
+	}
+
+	ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
+			      OV8856_REG_VALUE_08BIT, &val);
+	if (ret) {
+		dev_err(&client->dev, "failed to read module revision");
+		return ret;
+	}
+
+	dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n",
+		val,
+		val == OV8856_2A_MODULE ? "2A" :
+		val == OV8856_1B_MODULE ? "1B" : "unknown revision",
+		client->addr);
+
+	ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
+			       OV8856_REG_VALUE_08BIT, OV8856_MODE_STANDBY);
+	if (ret) {
+		dev_err(&client->dev, "failed to exit streaming mode");
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.25.1

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

[Robert Foss]

Query the sensor for its module revision, and compare it
to known revisions.

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

- Changes since v3:
  * Actually add module revision 2A

- Changes since v2:
  * Add module revision 2A
  * Sakari: Remove ov8856_check_revision()
  * Sakari: Stop EEPROM streaming mode

 drivers/media/i2c/ov8856.c | 53 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 4749dc74d5ad..3f4000aef2ab 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -32,6 +32,19 @@
 #define OV8856_MODE_STANDBY		0x00
 #define OV8856_MODE_STREAMING		0x01
 
+/* module revisions */
+#define OV8856_2A_MODULE		0x01
+#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
@@ -1152,6 +1165,46 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
 		return -ENXIO;
 	}
 
+	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) {
+		dev_err(&client->dev, "failed to set otp mode");
+		return ret;
+	}
+
+	ret = ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
+			       OV8856_REG_VALUE_08BIT,
+			       OV8856_OTP_LOAD_CTRL_ENABLE);
+	if (ret) {
+		dev_err(&client->dev, "failed to enable load control");
+		return ret;
+	}
+
+	ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
+			      OV8856_REG_VALUE_08BIT, &val);
+	if (ret) {
+		dev_err(&client->dev, "failed to read module revision");
+		return ret;
+	}
+
+	dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n",
+		val,
+		val == OV8856_2A_MODULE ? "2A" :
+		val == OV8856_1B_MODULE ? "1B" : "unknown revision",
+		client->addr);
+
+	ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
+			       OV8856_REG_VALUE_08BIT, OV8856_MODE_STANDBY);
+	if (ret) {
+		dev_err(&client->dev, "failed to exit streaming mode");
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.25.1


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