| Message ID | 20200331133346.372517-3-robert.foss@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: ov8856: Add devicetree support | expand |
On Tue, Mar 31, 2020 at 4:36 PM Robert Foss <robert.foss@linaro.org> wrote: > > Add devicetree match table, and enable ov8856_probe() > to initialize power, clocks and reset pins. ... > +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(ov8856->dev->fwnode)) Use dev_fwnode(). > + return 0; > + > + if (ov8856->reset_gpio) { > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); This is wrong. You have to fix it to use either 0 or 1. > + usleep_range(1000, 2000); > + } > + > + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > + ov8856->supplies); > + if (ret < 0) { Do you need all ' < 0' parts all over the series? > + dev_err(&client->dev, "failed to enable regulators\n"); > + goto disable_clk; > + } ... > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); Ditto. ... > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); Ditto. ... > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); Ditto. ... > - ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk); > - if (ret) > - return ret; Where is it gone? Why? > + ov8856->xvclk = devm_clk_get_optional(dev, "xvclk"); > + if (IS_ERR(ov8856->xvclk)) { > + dev_err(dev, "could not get xvclk clock (%ld)\n", > + PTR_ERR(ov8856->xvclk)); Also you may use %pe here and in similar cases. > + return PTR_ERR(ov8856->xvclk); > + } > + ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset", > + GPIOD_OUT_HIGH); Here parameter is correct. The question is, what the value should be HIGH or LOW? Basically HIGH means to assert the signal. > + if (IS_ERR(ov8856->reset_gpio)) { > + dev_dbg(dev, "failed to get reset-gpio\n"); Noise. Enable GPIO debug to see this kind of messages. > + return PTR_ERR(ov8856->reset_gpio); > + } ... > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names), > + ov8856->supplies); > + if (ret) { > + dev_warn(dev, "failed to get regulators\n"); If it's a warning, why we return from here? Same question to all other places with same issue. > + return ret; > }
Hi Robert, On Tue, Mar 31, 2020 at 03:33:45PM +0200, Robert Foss wrote: > Add devicetree match table, and enable ov8856_probe() > to initialize power, clocks and reset pins. > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > --- > > - 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 | 141 ++++++++++++++++++++++++++++++++----- > 1 file changed, 125 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c > index 8655842af275..260aaf332631 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(ov8856->dev->fwnode)) > + return 0; > + > + if (ov8856->reset_gpio) { > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > + 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, GPIOD_OUT_LOW); > + usleep_range(1500, 1800); > + > + return 0; > + > +disable_clk: > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > + 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); > +} > + > 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,27 +1155,52 @@ 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; > + unsigned long xvclk_rate; > int ret; > unsigned int i, j; > > if (!fwnode) > return -ENXIO; > > - ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk); > - if (ret) > - return ret; You'll still need to continue reading the clock-frequency property on ACPI systems, and assume that frequency (as you won't have the clock object). > + ov8856->xvclk = devm_clk_get_optional(dev, "xvclk"); > + if (IS_ERR(ov8856->xvclk)) { > + dev_err(dev, "could not get xvclk clock (%ld)\n", > + PTR_ERR(ov8856->xvclk)); > + return PTR_ERR(ov8856->xvclk); > + } > > - if (mclk != OV8856_MCLK) { > - dev_err(dev, "external clock %d is not supported", mclk); > - return -EINVAL; > + if (ov8856->xvclk) { > + xvclk_rate = clk_get_rate(ov8856->xvclk); > + if (xvclk_rate != OV8856_XVCLK_19_2) { > + dev_err(dev, "external clock %lu is not supported", > + xvclk_rate); > + return -EINVAL; > + } > + } > + > + ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(ov8856->reset_gpio)) { > + dev_dbg(dev, "failed to get reset-gpio\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(dev, ARRAY_SIZE(ov8856_supply_names), > + ov8856->supplies); > + if (ret) { > + dev_warn(dev, "failed to get regulators\n"); > + return ret; > } > > ep = fwnode_graph_get_next_endpoint(fwnode, NULL); > @@ -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; > + > + ov8856->dev = &client->dev; What happened here? :-) I think this patch is starting to look quite reasonable. > + 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_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 +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,
Hey Andy, Thanks for the review, it is much appreciated! On Tue, 31 Mar 2020 at 16:01, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Mar 31, 2020 at 4:36 PM Robert Foss <robert.foss@linaro.org> wrote: > > > > Add devicetree match table, and enable ov8856_probe() > > to initialize power, clocks and reset pins. > > ... > > > +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(ov8856->dev->fwnode)) > > Use dev_fwnode(). Ack. > > > + return 0; > > + > > + if (ov8856->reset_gpio) { > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > > This is wrong. You have to fix it to use either 0 or 1. I've changed all gpiod_set_value_cansleep() calls to use 0/1. > > > + usleep_range(1000, 2000); > > + } > > + > > + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > > + ov8856->supplies); > > > + if (ret < 0) { > > Do you need all ' < 0' parts all over the series? Some checks are needed due to ACPI and DT support co-existing. Maybe it would be better to just split the probing into an ACPI path and a DT path. I'll have a look through the series for redundant retval checks. > > > + dev_err(&client->dev, "failed to enable regulators\n"); > > + goto disable_clk; > > + } > > ... > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); > > Ditto. Ack. > > ... > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > > Ditto. Ack. > > ... > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > > Ditto. Ack. > > ... > > > - ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk); > > - if (ret) > > - return ret; > > Where is it gone? Why? It was replaced by a clk_get_rate call, which as Sakari pointed out, isn't correct. I'll rework the clock handling for v4. > > > + ov8856->xvclk = devm_clk_get_optional(dev, "xvclk"); > > + if (IS_ERR(ov8856->xvclk)) { > > > + dev_err(dev, "could not get xvclk clock (%ld)\n", > > + PTR_ERR(ov8856->xvclk)); > > Also you may use %pe here and in similar cases. Weirdly checkpatch complains about this. But it builds and runs cleanly, so I'll add it in v4. > > > + return PTR_ERR(ov8856->xvclk); > > + } > > > + ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset", > > + GPIOD_OUT_HIGH); > > Here parameter is correct. The question is, what the value should be > HIGH or LOW? > Basically HIGH means to assert the signal. Ack, I'll invert the logic. > > > + if (IS_ERR(ov8856->reset_gpio)) { > > > + dev_dbg(dev, "failed to get reset-gpio\n"); > > Noise. > Enable GPIO debug to see this kind of messages. Ack. > > > + return PTR_ERR(ov8856->reset_gpio); > > + } > > ... > > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names), > > + ov8856->supplies); > > + if (ret) { > > > + dev_warn(dev, "failed to get regulators\n"); > > If it's a warning, why we return from here? > Same question to all other places with same issue. The issue I was seeing was the driver having to return a EDEFER here, so this warning sheds some light on which exact component is returning an EDEFER. [ 15.962623] ov8856 16-0010: Dropping the link to regulator.29 [ 15.968464] ov8856 16-0010: failed to get regulators [ 15.973493] ov8856 16-0010: failed to get HW configuration: -517 [ 15.979591] ov8856 16-0010: removing from PM domain titan_top_gdsc [ 15.985855] ov8856 16-0010: genpd_remove_device() [ 15.990672] i2c 16-0010: Driver ov8856 requests probe deferral Personally I found it helpful to speed up debugging, but I'll happily remove it if you prefer no warning. > > > + return ret; > > } > > -- > With Best Regards, > Andy Shevchenko
On Mon, Apr 06, 2020 at 03:37:24PM +0200, Robert Foss wrote: > On Tue, 31 Mar 2020 at 16:01, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Mar 31, 2020 at 4:36 PM Robert Foss <robert.foss@linaro.org> wrote: ... > > > + if (ret < 0) { > > > > Do you need all ' < 0' parts all over the series? > > Some checks are needed due to ACPI and DT support co-existing. > Maybe it would be better to just split the probing into an ACPI path > and a DT path. > > I'll have a look through the series for redundant retval checks. Drop where it is redundant. ... > > > - ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk); > > > - if (ret) > > > - return ret; > > > > Where is it gone? Why? > > It was replaced by a clk_get_rate call, which as Sakari pointed out, > isn't correct. > I'll rework the clock handling for v4. If it was in the driver it should stay -- properties is an ABI (between firmware and kernel). > > > + ov8856->xvclk = devm_clk_get_optional(dev, "xvclk"); > > > + if (IS_ERR(ov8856->xvclk)) { > > > > > + dev_err(dev, "could not get xvclk clock (%ld)\n", > > > + PTR_ERR(ov8856->xvclk)); > > > > Also you may use %pe here and in similar cases. > > Weirdly checkpatch complains about this. > But it builds and runs cleanly, so I'll add it in v4. %pe requires pointer, PTR_ERR converts pointer to integer. ... > > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names), > > > + ov8856->supplies); > > > + if (ret) { > > > > > + dev_warn(dev, "failed to get regulators\n"); > > > > If it's a warning, why we return from here? > > Same question to all other places with same issue. > > The issue I was seeing was the driver having to return a EDEFER here, > so this warning sheds some light on which exact component is returning > an EDEFER. > > [ 15.962623] ov8856 16-0010: Dropping the link to regulator.29 > [ 15.968464] ov8856 16-0010: failed to get regulators > [ 15.973493] ov8856 16-0010: failed to get HW configuration: -517 > [ 15.979591] ov8856 16-0010: removing from PM domain titan_top_gdsc > [ 15.985855] ov8856 16-0010: genpd_remove_device() > [ 15.990672] i2c 16-0010: Driver ov8856 requests probe deferral > > Personally I found it helpful to speed up debugging, but I'll happily > remove it if you prefer no warning. My point is that you have it in align: - if it is an error, print as an error and bail out, otherwise - if it is a warning, print it and continue. > > > + return ret; > > > }
On Mon, 6 Apr 2020 at 17:06, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Apr 06, 2020 at 03:37:24PM +0200, Robert Foss wrote: > > On Tue, 31 Mar 2020 at 16:01, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Tue, Mar 31, 2020 at 4:36 PM Robert Foss <robert.foss@linaro.org> wrote: > > ... > > > > > + if (ret < 0) { > > > > > > Do you need all ' < 0' parts all over the series? > > > > Some checks are needed due to ACPI and DT support co-existing. > > Maybe it would be better to just split the probing into an ACPI path > > and a DT path. > > > > I'll have a look through the series for redundant retval checks. > > Drop where it is redundant. > > ... > > > > > - ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk); > > > > - if (ret) > > > > - return ret; > > > > > > Where is it gone? Why? > > > > It was replaced by a clk_get_rate call, which as Sakari pointed out, > > isn't correct. > > I'll rework the clock handling for v4. > > If it was in the driver it should stay -- properties is an ABI (between firmware and kernel). Ack. > > > > > + ov8856->xvclk = devm_clk_get_optional(dev, "xvclk"); > > > > + if (IS_ERR(ov8856->xvclk)) { > > > > > > > + dev_err(dev, "could not get xvclk clock (%ld)\n", > > > > + PTR_ERR(ov8856->xvclk)); > > > > > > Also you may use %pe here and in similar cases. > > > > Weirdly checkpatch complains about this. > > But it builds and runs cleanly, so I'll add it in v4. > > %pe requires pointer, PTR_ERR converts pointer to integer. Ack. > > ... > > > > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names), > > > > + ov8856->supplies); > > > > + if (ret) { > > > > > > > + dev_warn(dev, "failed to get regulators\n"); > > > > > > If it's a warning, why we return from here? > > > Same question to all other places with same issue. > > > > The issue I was seeing was the driver having to return a EDEFER here, > > so this warning sheds some light on which exact component is returning > > an EDEFER. > > > > [ 15.962623] ov8856 16-0010: Dropping the link to regulator.29 > > [ 15.968464] ov8856 16-0010: failed to get regulators > > [ 15.973493] ov8856 16-0010: failed to get HW configuration: -517 > > [ 15.979591] ov8856 16-0010: removing from PM domain titan_top_gdsc > > [ 15.985855] ov8856 16-0010: genpd_remove_device() > > [ 15.990672] i2c 16-0010: Driver ov8856 requests probe deferral > > > > Personally I found it helpful to speed up debugging, but I'll happily > > remove it if you prefer no warning. > > My point is that you have it in align: > - if it is an error, print as an error and bail out, otherwise > - if it is a warning, print it and continue. I see what you're saying now, let's remove it then :) I guess in the specific case of EDEFER, it doesn't fit neatly into either of those categories, in the sense that the way you continue is to return and then try to probe again later. There are some other locations where this is handled wrong, I'll align them properly for v4. > > > > > + return ret; > > > > } > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c index 8655842af275..260aaf332631 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(ov8856->dev->fwnode)) + return 0; + + if (ov8856->reset_gpio) { + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); + 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, GPIOD_OUT_LOW); + usleep_range(1500, 1800); + + return 0; + +disable_clk: + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); + 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); +} + 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,27 +1155,52 @@ 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; + unsigned long xvclk_rate; int ret; unsigned int i, j; if (!fwnode) return -ENXIO; - ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk); - if (ret) - return ret; + ov8856->xvclk = devm_clk_get_optional(dev, "xvclk"); + if (IS_ERR(ov8856->xvclk)) { + dev_err(dev, "could not get xvclk clock (%ld)\n", + PTR_ERR(ov8856->xvclk)); + return PTR_ERR(ov8856->xvclk); + } - if (mclk != OV8856_MCLK) { - dev_err(dev, "external clock %d is not supported", mclk); - return -EINVAL; + if (ov8856->xvclk) { + xvclk_rate = clk_get_rate(ov8856->xvclk); + if (xvclk_rate != OV8856_XVCLK_19_2) { + dev_err(dev, "external clock %lu is not supported", + xvclk_rate); + return -EINVAL; + } + } + + ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset", + GPIOD_OUT_HIGH); + if (IS_ERR(ov8856->reset_gpio)) { + dev_dbg(dev, "failed to get reset-gpio\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(dev, ARRAY_SIZE(ov8856_supply_names), + ov8856->supplies); + if (ret) { + dev_warn(dev, "failed to get regulators\n"); + return ret; } ep = fwnode_graph_get_next_endpoint(fwnode, NULL); @@ -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; + + 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_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 +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,
Add devicetree match table, and enable ov8856_probe() to initialize power, clocks and reset pins. Signed-off-by: Robert Foss <robert.foss@linaro.org> --- - 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 | 141 ++++++++++++++++++++++++++++++++----- 1 file changed, 125 insertions(+), 16 deletions(-)