Re: [v2 2/3] media: ov8856: Add devicetree support
From: Sakari Ailus <sakari.ailus@iki.fi>
Date: 2020-03-13 12:18:50
Also in:
linux-arm-kernel, linux-media, lkml
Hi Robert, On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote:
quoted hunk ↗ jump to hunk
Add devicetree match table, and enable ov8856_probe() to initialize power, clocks and reset pins. Signed-off-by: Robert Foss <redacted> --- - Changes since v1: * Fabio: Change n_shutdown_gpio name to reset_gpio * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change * Fabio: Remove empty line * Fabio: Remove real error from devm_gpiod_get() failures * Andy & Sakari: Make XVCLK optional since to not break ACPI * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES * Sakari: Use XVCLK rate as provided by DT drivers/media/i2c/ov8856.c | 109 ++++++++++++++++++++++++++++++++++++- 1 file changed, 107 insertions(+), 2 deletions(-)diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c index 8655842af275..db61eed223e8 100644 --- a/drivers/media/i2c/ov8856.c +++ b/drivers/media/i2c/ov8856.c@@ -3,10 +3,13 @@ #include <asm/unaligned.h> #include <linux/acpi.h> +#include <linux/clk.h> #include <linux/delay.h> +#include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/module.h> #include <linux/pm_runtime.h> +#include <linux/regulator/consumer.h> #include <media/v4l2-ctrls.h> #include <media/v4l2-device.h> #include <media/v4l2-fwnode.h>@@ -19,6 +22,8 @@ #define OV8856_LINK_FREQ_180MHZ 180000000ULL #define OV8856_SCLK 144000000ULL #define OV8856_MCLK 19200000 +#define OV8856_XVCLK_19_2 19200000
Please use a single macro to refer to 19,2 MHz clock.
+#define OV8856_XVCLK_24 24000000
This doesn't seem to be needed
quoted hunk ↗ jump to hunk
#define OV8856_DATA_LANES 4 #define OV8856_RGB_DEPTH 10@@ -64,6 +69,12 @@ #define to_ov8856(_sd) container_of(_sd, struct ov8856, sd) +static const char * const ov8856_supply_names[] = { + "dovdd", /* Digital I/O power */ + "avdd", /* Analog power */ + "dvdd", /* Digital core power */ +}; + enum { OV8856_LINK_FREQ_720MBPS, OV8856_LINK_FREQ_360MBPS,@@ -566,6 +577,10 @@ struct ov8856 { struct media_pad pad; struct v4l2_ctrl_handler ctrl_handler; + struct clk *xvclk; + struct gpio_desc *reset_gpio; + struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)]; + /* V4L2 Controls */ struct v4l2_ctrl *link_freq; struct v4l2_ctrl *pixel_rate;@@ -908,6 +923,46 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable) return ret; } +static int __ov8856_power_on(struct ov8856 *ov8856) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); + int ret; + + ret = clk_prepare_enable(ov8856->xvclk); + if (ret < 0) { + dev_err(&client->dev, "failed to enable xvclk\n"); + return ret; + } + + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); + + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), + ov8856->supplies); + if (ret < 0) { + dev_err(&client->dev, "failed to enable regulators\n"); + goto disable_clk; + } + + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); + + usleep_range(1500, 1800);
I think you could omit the delay on ACPI based systems. Or just bail out early in that case.
+ + return 0; + +disable_clk:
How about the GPIO here?
+ clk_disable_unprepare(ov8856->xvclk);
+
+ return ret;
+}
+
+static void __ov8856_power_off(struct ov8856 *ov8856)
+{
+ gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
+ regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
+ ov8856->supplies);
+ clk_disable_unprepare(ov8856->xvclk);
+}You'll need to call the two in the driver's suspend and resume functions.
quoted hunk ↗ jump to hunk
+ static int __maybe_unused ov8856_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev);@@ -1175,7 +1230,7 @@ static int ov8856_remove(struct i2c_client *client) static int ov8856_probe(struct i2c_client *client) { struct ov8856 *ov8856; - int ret; + int i, ret;
unsigned int?
quoted hunk ↗ jump to hunk
ret = ov8856_check_hwcfg(&client->dev); if (ret) {@@ -1189,10 +1244,50 @@ static int ov8856_probe(struct i2c_client *client) return -ENOMEM; v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops); + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); + if (PTR_ERR(ov8856->xvclk) == -ENOENT) { + dev_info(&client->dev, "xvclk clock not defined, continuing...\n");
How about dev_dbg()?
+ ov8856->xvclk = NULL;
+ } else if (IS_ERR(ov8856->xvclk)) {
+ dev_err(&client->dev, "could not get xvclk clock (%ld)\n",
+ PTR_ERR(ov8856->xvclk));
+ return PTR_ERR(ov8856->xvclk);
+ }
+
+ ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24);This should either come from platform data, or perhaps it'd be even better to get the clock rate and use assigned-clock-rates. I guess that's preferred nowadays.
+ if (ret < 0) {
+ dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
+ return ret;
+ }
+
+ ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
+ GPIOD_OUT_HIGH);Indentation. What if no gpio is defined?
+ if (IS_ERR(ov8856->reset_gpio)) {
+ dev_err(&client->dev, "failed to get reset-gpios\n");
+ return PTR_ERR(ov8856->reset_gpio);
+ }
+
+ for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
+ ov8856->supplies[i].supply = ov8856_supply_names[i];
+
+ ret = devm_regulator_bulk_get(&client->dev,
+ ARRAY_SIZE(ov8856_supply_names),
+ ov8856->supplies);What happens if there are no regulators?
quoted hunk ↗ jump to hunk
+ if (ret) { + dev_warn(&client->dev, "failed to get regulators\n"); + return ret; + } + + ret = __ov8856_power_on(ov8856); + if (ret) { + dev_warn(&client->dev, "failed to power on\n"); + return ret; + } + ret = ov8856_identify_module(ov8856); if (ret) { dev_err(&client->dev, "failed to find sensor: %d", ret); - return ret; + goto probe_power_off; } mutex_init(&ov8856->mutex);@@ -1238,6 +1333,9 @@ static int ov8856_probe(struct i2c_client *client) v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler); mutex_destroy(&ov8856->mutex); +probe_power_off: + __ov8856_power_off(ov8856); +
Also remember to power off the device in remove().
quoted hunk ↗ jump to hunk
return ret; }@@ -1254,11 +1352,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = { MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids); #endif +static const struct of_device_id ov8856_of_match[] = { + { .compatible = "ovti,ov8856" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, ov8856_of_match); + static struct i2c_driver ov8856_i2c_driver = { .driver = { .name = "ov8856", .pm = &ov8856_pm_ops, .acpi_match_table = ACPI_PTR(ov8856_acpi_ids), + .of_match_table = ov8856_of_match, }, .probe_new = ov8856_probe, .remove = ov8856_remove,
-- Regards, Sakari Ailus