LinuxLists.cc - [PATCH v6 v6 0/3] media: ov8856: Add devicetree support

[permalink] [raw]

Subject: [PATCH v9 1/3] media: dt-bindings: ov8856: Document YAML bindings

From: Dongchun Zhu <[email protected]>

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

Signed-off-by: Dongchun Zhu <[email protected]>
Signed-off-by: Robert Foss <[email protected]>
Reviewed-by: Maxime Ripard <[email protected]>
---

- 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 <[email protected]>
+ - Dongchun Zhu <[email protected]>
+
+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: [email protected]
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 <[email protected]>
--
2.25.1

2020-04-29 16:27:19

[permalink] [raw]

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

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 <[email protected]>
---

- 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

2020-04-29 16:29:18

[permalink] [raw]

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

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

Signed-off-by: Robert Foss <[email protected]>
---

- 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

2020-04-30 09:37:51

[permalink] [raw]

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

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 <[email protected]>
> ---
>
> - 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
>

2020-04-30 09:47:42

[permalink] [raw]

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

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 <[email protected]>
> > ---
> >
> > - 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

2020-04-30 09:55:57

[permalink] [raw]

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

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 <[email protected]>
> > > ---
> > >
> > > - 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
>

2020-04-30 10:01:02

[permalink] [raw]

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

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 <[email protected]>
> > > > ---
> > > >
> > > > - 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

2020-04-30 10:14:53

[permalink] [raw]

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

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

2020-04-30 10:22:31

[permalink] [raw]

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

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

2020-04-30 12:10:34

[permalink] [raw]

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

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

2020-04-30 13:35:29

[permalink] [raw]

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

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

2020-04-30 16:13:16

[permalink] [raw]

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

Hey Marco,

On Thu, 30 Apr 2020 at 14:07, Marco Felsch <[email protected]> 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

2020-05-04 06:28:59