* [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings
@ 2020-04-28 18:07 Robert Foss
2020-04-28 18:07 ` [PATCH v5 v5 2/3] media: ov8856: Add devicetree support Robert Foss
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Robert Foss @ 2020-04-28 18:07 UTC (permalink / raw)
To: Andy Shevchenko, Sakari Ailus, Marco Felsch, Maxime Ripard,
linux-media, devicetree, linux-kernel, linux-arm-kernel
Cc: Dongchun Zhu, Fabio Estevam, Tomasz Figa, Robert Foss
From: Dongchun Zhu <dongchun.zhu@mediatek.com>
This patch adds documentation of device tree in YAML schema for the
OV8856 CMOS image sensor.
Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
- Changes since 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v5 v5 2/3] media: ov8856: Add devicetree support
2020-04-28 18:07 [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss
@ 2020-04-28 18:07 ` Robert Foss
2020-04-29 9:00 ` Maxime Ripard
2020-04-28 18:07 ` [PATCH v5 v5 3/3] media: ov8856: Implement sensor module revision identification Robert Foss
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Robert Foss @ 2020-04-28 18:07 UTC (permalink / raw)
To: Andy Shevchenko, Sakari Ailus, Marco Felsch, Maxime Ripard,
linux-media, devicetree, linux-kernel, linux-arm-kernel
Cc: Dongchun Zhu, Fabio Estevam, Tomasz Figa, 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 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 | 139 +++++++++++++++++++++++++++++++++----
1 file changed, 126 insertions(+), 13 deletions(-)
diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 8655842af275..48b02b8d205f 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,58 @@ 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);
+ 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);
+ }
+
+ /* external clock must be 19.2MHz, allow 5% tolerance */
+ if (xvclk_rate < OV8856_XVCLK_19_2 * 0.95 ||
+ xvclk_rate > OV8856_XVCLK_19_2 * 1.05) {
+ dev_err(dev, "external clock %d is not supported", xvclk_rate);
return -EINVAL;
}
+ 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 +1261,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 +1271,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 +1341,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 +1360,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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v5 v5 3/3] media: ov8856: Implement sensor module revision identification
2020-04-28 18:07 [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss
2020-04-28 18:07 ` [PATCH v5 v5 2/3] media: ov8856: Add devicetree support Robert Foss
@ 2020-04-28 18:07 ` Robert Foss
2020-04-28 22:05 ` [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings Sakari Ailus
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Robert Foss @ 2020-04-28 18:07 UTC (permalink / raw)
To: Andy Shevchenko, Sakari Ailus, Marco Felsch, Maxime Ripard,
linux-media, devicetree, linux-kernel, linux-arm-kernel
Cc: Dongchun Zhu, Fabio Estevam, Tomasz Figa, Robert Foss
Query the sensor for its module revision, and compare it
to known revisions.
Currently only the '1B' revision has been added.
Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
- 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 48b02b8d205f..a0ba0a5d5aad 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings
2020-04-28 18:07 [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss
2020-04-28 18:07 ` [PATCH v5 v5 2/3] media: ov8856: Add devicetree support Robert Foss
2020-04-28 18:07 ` [PATCH v5 v5 3/3] media: ov8856: Implement sensor module revision identification Robert Foss
@ 2020-04-28 22:05 ` Sakari Ailus
2020-04-29 5:55 ` Marco Felsch
2020-04-29 8:48 ` Maxime Ripard
4 siblings, 0 replies; 11+ messages in thread
From: Sakari Ailus @ 2020-04-28 22:05 UTC (permalink / raw)
To: Robert Foss
Cc: Andy Shevchenko, Marco Felsch, Maxime Ripard, linux-media,
devicetree, linux-kernel, linux-arm-kernel, Dongchun Zhu,
Fabio Estevam, Tomasz Figa
Hi Robert,
Thanks for the update. Some small matters below.
On Tue, Apr 28, 2020 at 08:07:16PM +0200, Robert Foss wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
>
> This patch adds documentation of device tree in YAML schema for the
> OV8856 CMOS image sensor.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>
> - Changes since 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>
I guess Dongchun would be the maintainer for these bindings, not Ben.
Please also cc Ben in the next version.
> +
> +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:
Does the sensor support lane reordering? If not, please omit this.
> + maxItems: 1
> +
> + data-lanes:
> + maxItems: 1
Hmm. The example has four lanes. Do I miss something?
> +
> + link-frequencies:
> + maxItems: 1
The number of items in link-frequencies should not be limited to one
either.
> +
> + 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>
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings
2020-04-28 18:07 [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss
` (2 preceding siblings ...)
2020-04-28 22:05 ` [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings Sakari Ailus
@ 2020-04-29 5:55 ` Marco Felsch
2020-04-29 8:48 ` Maxime Ripard
4 siblings, 0 replies; 11+ messages in thread
From: Marco Felsch @ 2020-04-29 5:55 UTC (permalink / raw)
To: Robert Foss
Cc: Andy Shevchenko, Sakari Ailus, Maxime Ripard, linux-media,
devicetree, linux-kernel, linux-arm-kernel, Dongchun Zhu,
Fabio Estevam, Tomasz Figa
Hi Robert,
On 20-04-28 20:07, Robert Foss wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
>
> This patch adds documentation of device tree in YAML schema for the
> OV8856 CMOS image sensor.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>
> - Changes since v7:
> * Marco: Make 'port' property optional
Sorry if my comment was not precise enough but this wasn't my intention.
The port property must be required.
> * 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
This was my intention :)
Regards,
Marco
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings
2020-04-28 18:07 [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss
` (3 preceding siblings ...)
2020-04-29 5:55 ` Marco Felsch
@ 2020-04-29 8:48 ` Maxime Ripard
4 siblings, 0 replies; 11+ messages in thread
From: Maxime Ripard @ 2020-04-29 8:48 UTC (permalink / raw)
To: Robert Foss
Cc: Andy Shevchenko, Sakari Ailus, Marco Felsch, linux-media,
devicetree, linux-kernel, linux-arm-kernel, Dongchun Zhu,
Fabio Estevam, Tomasz Figa
[-- Attachment #1: Type: text/plain, Size: 404 bytes --]
On Tue, Apr 28, 2020 at 08:07:16PM +0200, Robert Foss wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
>
> This patch adds documentation of device tree in YAML schema for the
> OV8856 CMOS image sensor.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Thanks!
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 v5 2/3] media: ov8856: Add devicetree support
2020-04-28 18:07 ` [PATCH v5 v5 2/3] media: ov8856: Add devicetree support Robert Foss
@ 2020-04-29 9:00 ` Maxime Ripard
2020-04-29 10:19 ` Robert Foss
0 siblings, 1 reply; 11+ messages in thread
From: Maxime Ripard @ 2020-04-29 9:00 UTC (permalink / raw)
To: Robert Foss
Cc: Andy Shevchenko, Sakari Ailus, Marco Felsch, linux-media,
devicetree, linux-kernel, linux-arm-kernel, Dongchun Zhu,
Fabio Estevam, Tomasz Figa
[-- Attachment #1: Type: text/plain, Size: 7568 bytes --]
Hi,
On Tue, Apr 28, 2020 at 08:07:17PM +0200, Robert Foss wrote:
> 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 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 | 139 +++++++++++++++++++++++++++++++++----
> 1 file changed, 126 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index 8655842af275..48b02b8d205f 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,58 @@ 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);
> + 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);
> + }
> +
> + /* external clock must be 19.2MHz, allow 5% tolerance */
Where is that 5% tolerance coming from? Experimentations, datasheets, something
that looks good enough? Either way, this should be in the comment.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 v5 2/3] media: ov8856: Add devicetree support
2020-04-29 9:00 ` Maxime Ripard
@ 2020-04-29 10:19 ` Robert Foss
2020-04-29 11:13 ` Sakari Ailus
0 siblings, 1 reply; 11+ messages in thread
From: Robert Foss @ 2020-04-29 10:19 UTC (permalink / raw)
To: Maxime Ripard
Cc: Andy Shevchenko, Sakari Ailus, Marco Felsch, linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Dongchun Zhu, Fabio Estevam, Tomasz Figa
On Wed, 29 Apr 2020 at 11:00, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> On Tue, Apr 28, 2020 at 08:07:17PM +0200, Robert Foss wrote:
> > 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 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 | 139 +++++++++++++++++++++++++++++++++----
> > 1 file changed, 126 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > index 8655842af275..48b02b8d205f 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,58 @@ 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);
> > + 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);
> > + }
> > +
> > + /* external clock must be 19.2MHz, allow 5% tolerance */
>
> Where is that 5% tolerance coming from? Experimentations, datasheets, something
> that looks good enough? Either way, this should be in the comment.
I don't have access to the full datasheet unfortunately. A 24Mhz rate
is as far as I understand it supported and required for higher
bandwidth count modes.
It was suggested to me that adding a tolerance is the best practice,
the ov5645 driver uses a 1% tolerance, which may be more appropriate.
The closest frequency to 19.2Mhz * 1.05 I'm able to generate is 24Mhz,
which works. But unfortunately my testing platform sdm845-db845c is
not able to generate lower frequencies than 19.2Mhz.
This all adds up to a pretty unclear picture of what is supported, if
anyone has access to the full documentation I would like to make the
tolerances and comments reflect that.
>
> Maxime
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 v5 2/3] media: ov8856: Add devicetree support
2020-04-29 10:19 ` Robert Foss
@ 2020-04-29 11:13 ` Sakari Ailus
2020-04-29 11:39 ` Robert Foss
0 siblings, 1 reply; 11+ messages in thread
From: Sakari Ailus @ 2020-04-29 11:13 UTC (permalink / raw)
To: Robert Foss
Cc: Maxime Ripard, Andy Shevchenko, Marco Felsch, linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Dongchun Zhu, Fabio Estevam, Tomasz Figa
Hi Robert, Maxime,
On Wed, Apr 29, 2020 at 12:19:38PM +0200, Robert Foss wrote:
> On Wed, 29 Apr 2020 at 11:00, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi,
> >
> > On Tue, Apr 28, 2020 at 08:07:17PM +0200, Robert Foss wrote:
> > > 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 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 | 139 +++++++++++++++++++++++++++++++++----
> > > 1 file changed, 126 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > > index 8655842af275..48b02b8d205f 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,58 @@ 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);
> > > + 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);
> > > + }
> > > +
> > > + /* external clock must be 19.2MHz, allow 5% tolerance */
> >
> > Where is that 5% tolerance coming from? Experimentations, datasheets, something
> > that looks good enough? Either way, this should be in the comment.
>
> I don't have access to the full datasheet unfortunately. A 24Mhz rate
> is as far as I understand it supported and required for higher
> bandwidth count modes.
> It was suggested to me that adding a tolerance is the best practice,
> the ov5645 driver uses a 1% tolerance, which may be more appropriate.
The frequency should really be exact. Sometimes what happens is, however,
that a register list based driver does not have the register lists for a
frequency that is available on a given system. That's why some drivers have
allowed some difference to the intended frequency.
That 5 % seems like a random value, just like any other number that differs
from the exact frequency would be.
I'd issue a warning if the frequency differs from what was intended, but
still proceed with probe. This way we can make sure the difference is noted
while boards that cannot provide the exact frequency supported by the
driver can still function.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 v5 2/3] media: ov8856: Add devicetree support
2020-04-29 11:13 ` Sakari Ailus
@ 2020-04-29 11:39 ` Robert Foss
2020-04-29 11:46 ` Maxime Ripard
0 siblings, 1 reply; 11+ messages in thread
From: Robert Foss @ 2020-04-29 11:39 UTC (permalink / raw)
To: Sakari Ailus
Cc: Maxime Ripard, Andy Shevchenko, Marco Felsch, linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Dongchun Zhu, Fabio Estevam, Tomasz Figa
On Wed, 29 Apr 2020 at 13:13, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Robert, Maxime,
>
> On Wed, Apr 29, 2020 at 12:19:38PM +0200, Robert Foss wrote:
> > On Wed, 29 Apr 2020 at 11:00, Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Apr 28, 2020 at 08:07:17PM +0200, Robert Foss wrote:
> > > > 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 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 | 139 +++++++++++++++++++++++++++++++++----
> > > > 1 file changed, 126 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > > > index 8655842af275..48b02b8d205f 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,58 @@ 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);
> > > > + 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);
> > > > + }
> > > > +
> > > > + /* external clock must be 19.2MHz, allow 5% tolerance */
> > >
> > > Where is that 5% tolerance coming from? Experimentations, datasheets, something
> > > that looks good enough? Either way, this should be in the comment.
> >
> > I don't have access to the full datasheet unfortunately. A 24Mhz rate
> > is as far as I understand it supported and required for higher
> > bandwidth count modes.
> > It was suggested to me that adding a tolerance is the best practice,
> > the ov5645 driver uses a 1% tolerance, which may be more appropriate.
>
> The frequency should really be exact. Sometimes what happens is, however,
> that a register list based driver does not have the register lists for a
> frequency that is available on a given system. That's why some drivers have
> allowed some difference to the intended frequency.
>
> That 5 % seems like a random value, just like any other number that differs
> from the exact frequency would be.
>
> I'd issue a warning if the frequency differs from what was intended, but
> still proceed with probe. This way we can make sure the difference is noted
> while boards that cannot provide the exact frequency supported by the
> driver can still function.
Issuing a warning sounds like a good solution to me. What do you think Maxime?
>
> --
> Regards,
>
> Sakari Ailus
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 v5 2/3] media: ov8856: Add devicetree support
2020-04-29 11:39 ` Robert Foss
@ 2020-04-29 11:46 ` Maxime Ripard
0 siblings, 0 replies; 11+ messages in thread
From: Maxime Ripard @ 2020-04-29 11:46 UTC (permalink / raw)
To: Robert Foss
Cc: Sakari Ailus, Andy Shevchenko, Marco Felsch, linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Dongchun Zhu, Fabio Estevam, Tomasz Figa
[-- Attachment #1: Type: text/plain, Size: 11550 bytes --]
On Wed, Apr 29, 2020 at 01:39:12PM +0200, Robert Foss wrote:
> On Wed, 29 Apr 2020 at 13:13, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Hi Robert, Maxime,
> >
> > On Wed, Apr 29, 2020 at 12:19:38PM +0200, Robert Foss wrote:
> > > On Wed, 29 Apr 2020 at 11:00, Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Apr 28, 2020 at 08:07:17PM +0200, Robert Foss wrote:
> > > > > 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 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 | 139 +++++++++++++++++++++++++++++++++----
> > > > > 1 file changed, 126 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > > > > index 8655842af275..48b02b8d205f 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,58 @@ 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);
> > > > > + 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);
> > > > > + }
> > > > > +
> > > > > + /* external clock must be 19.2MHz, allow 5% tolerance */
> > > >
> > > > Where is that 5% tolerance coming from? Experimentations, datasheets, something
> > > > that looks good enough? Either way, this should be in the comment.
> > >
> > > I don't have access to the full datasheet unfortunately. A 24Mhz rate
> > > is as far as I understand it supported and required for higher
> > > bandwidth count modes.
> > > It was suggested to me that adding a tolerance is the best practice,
> > > the ov5645 driver uses a 1% tolerance, which may be more appropriate.
> >
> > The frequency should really be exact. Sometimes what happens is, however,
> > that a register list based driver does not have the register lists for a
> > frequency that is available on a given system. That's why some drivers have
> > allowed some difference to the intended frequency.
> >
> > That 5 % seems like a random value, just like any other number that differs
> > from the exact frequency would be.
> >
> > I'd issue a warning if the frequency differs from what was intended, but
> > still proceed with probe. This way we can make sure the difference is noted
> > while boards that cannot provide the exact frequency supported by the
> > driver can still function.
>
> Issuing a warning sounds like a good solution to me. What do you think Maxime?
Sounds good to me too :)
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-29 11:46 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-28 18:07 [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss
2020-04-28 18:07 ` [PATCH v5 v5 2/3] media: ov8856: Add devicetree support Robert Foss
2020-04-29 9:00 ` Maxime Ripard
2020-04-29 10:19 ` Robert Foss
2020-04-29 11:13 ` Sakari Ailus
2020-04-29 11:39 ` Robert Foss
2020-04-29 11:46 ` Maxime Ripard
2020-04-28 18:07 ` [PATCH v5 v5 3/3] media: ov8856: Implement sensor module revision identification Robert Foss
2020-04-28 22:05 ` [PATCH v8 v5 1/3] media: dt-bindings: ov8856: Document YAML bindings Sakari Ailus
2020-04-29 5:55 ` Marco Felsch
2020-04-29 8:48 ` Maxime Ripard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).