| Message ID | 20200313110350.10864-1-robert.foss@linaro.org |
|---|---|
| Headers | show |
| Series | media: ov8856: Add devicetree support | expand |
Hi Robert, On Fri, Mar 13, 2020 at 12:03:49PM +0100, 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 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 > #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. > + > 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? > > 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? > + 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(). > 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,
On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote: > Add devicetree match table, and enable ov8856_probe() > to initialize power, clocks and reset pins. Thanks for an update. My comments below. ... > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); In many frameworks we have '_optional' variants of API. Please use it instead of open coded approach. > + if (PTR_ERR(ov8856->xvclk) == -ENOENT) { > + dev_info(&client->dev, "xvclk clock not defined, continuing...\n"); > + 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); > + 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); Same here. > + 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); Luckily regulator framework will create dummy ones if there is none found. > + if (ret) { > + dev_warn(&client->dev, "failed to get regulators\n"); > + return ret; > + }
Hi Robert, On Fri, Mar 13, 2020 at 12:03:50PM +0100, Robert Foss wrote: > Query the sensor for its module revision, and compare it > to known revisions. > Currently only the '1B' revision has been added. > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > --- > drivers/media/i2c/ov8856.c | 54 +++++++++++++++++++++++++++++++++----- > 1 file changed, 48 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c > index db61eed223e8..39662d3d86dd 100644 > --- a/drivers/media/i2c/ov8856.c > +++ b/drivers/media/i2c/ov8856.c > @@ -34,6 +34,18 @@ > #define OV8856_MODE_STANDBY 0x00 > #define OV8856_MODE_STREAMING 0x01 > > +/* define 1B module revision */ > +#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 > @@ -711,6 +723,25 @@ static int ov8856_test_pattern(struct ov8856 *ov8856, u32 pattern) > OV8856_REG_VALUE_08BIT, pattern); > } > > +static int ov8856_check_revision(struct ov8856 *ov8856) There are no version checks being done here, nor apparently the version is read by this function. > +{ > + int ret; > + > + 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) > + return ret; > + > + return ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL, > + OV8856_REG_VALUE_08BIT, > + OV8856_OTP_LOAD_CTRL_ENABLE); If streaming is started to read the EEPROM, shouldn't it be stopped after reading it as well? > +} > + > static int ov8856_set_ctrl(struct v4l2_ctrl *ctrl) > { > struct ov8856 *ov8856 = container_of(ctrl->handler, > @@ -1144,6 +1175,23 @@ static int ov8856_identify_module(struct ov8856 *ov8856) > return -ENXIO; > } > > + /* check sensor hardware revision */ > + ret = ov8856_check_revision(ov8856); > + if (ret) { > + dev_err(&client->dev, "failed to check sensor revision"); > + return ret; > + } > + > + ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION, > + OV8856_REG_VALUE_08BIT, &val); > + if (ret) > + return ret; How about moving this inside the check_revision function above? It looks as if it's dependent on that. > + > + dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n", > + val, > + val == OV8856_1B_MODULE ? "1B" : "unknown revision", > + client->addr); > + > return 0; > } > > @@ -1254,12 +1302,6 @@ static int ov8856_probe(struct i2c_client *client) > return PTR_ERR(ov8856->xvclk); > } > > - ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24); This seems like an unrelated change. > - 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); > if (IS_ERR(ov8856->reset_gpio)) {
Hi Robert, On Fri, Mar 13, 2020 at 8:04 AM Robert Foss <robert.foss@linaro.org> wrote: > +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); The parameter of gpiod_set_value_cansleep() is typically 0 (inactive state) or 1 (active state), so: gpiod_set_value_cansleep(ov8856->reset_gpio, 1); > + > + 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); and here it should be: gpiod_set_value_cansleep(ov8856->reset_gpio, 0); Also, don't you need a reset period between the two?
Hey Sakari, On Fri, 13 Mar 2020 at 13:18, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Robert, > > On Fri, Mar 13, 2020 at 12:03:49PM +0100, 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 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. Alright, I'll combine the two into a macro for both. > > > +#define OV8856_XVCLK_24 24000000 > > This doesn't seem to be needed As long as we don't set the clock to 24Mhz we don't. I'm assuming that you're saying that the 24Mhz clock rate isn't needed for the modes used currently. Removing this macro in v3. > > > #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. I'll add a check for reset_gpio being NULL, and skip the sleep for that case. > > > + > > + return 0; > > + > > +disable_clk: > > How about the GPIO here? Ack > > > + 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. Ack > > > + > > 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? Ack > > > > > 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()? Ack. > > > + 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. I'm a bit unsure about what this would look like. Are you thinking something like the way ext_clk is used in smiapp_core.c? I went ahead and implemented support for retrieving and storing 'clock-rates' during the ov8856_check_hwcfg() call, and then setting the rate to the configured rate during probing. > > > + 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. Ack. > > What if no gpio is defined? As per Andys comments, I'll switch to the optional version of devm_gpiod_get(). > > > + 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? Like Andy mentioned, we should be alright, since devm_regulator_bulk_get() creates dummy regulators if one isn't found. > > > + 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(). > Ack > > 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
Hi Robert, On Thu, Mar 26, 2020 at 12:56:37PM +0100, Robert Foss wrote: ... > > > +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. > > I'll add a check for reset_gpio being NULL, and skip the sleep for that case. There could also be a regulator but no GPIO. I think if you don't have either, then certainly there's no need for a delay. ... > > > + 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. > > I'm a bit unsure about what this would look like. > > Are you thinking something like the way ext_clk is used in smiapp_core.c? > I went ahead and implemented support for retrieving and storing > 'clock-rates' during the ov8856_check_hwcfg() call, and then setting > the rate to the configured rate during probing. With assigned-clock-rates, you can simply use clk_get_rate(). As you get the actual rate, it could be somewhat off of the intended one.
On Thu, 26 Mar 2020 at 15:47, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Robert, > > On Thu, Mar 26, 2020 at 12:56:37PM +0100, Robert Foss wrote: > ... > > > > +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. > > > > I'll add a check for reset_gpio being NULL, and skip the sleep for that case. > > There could also be a regulator but no GPIO. > > I think if you don't have either, then certainly there's no need for a > delay. Removing the delay if no action is taken makes sense, but I'm not sure how best to do it. If there are no regulators dummy ones are created automatically, which makes distinguishing between a little bit cumbersome. The regulator structs could of course all be inspected, and if all are dummy ones, the delay could be skipped. But is there a neater way of doing this? Manually inspecting the regs strikes me as a bit inelegant. > > ... > > > > > + 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. > > > > I'm a bit unsure about what this would look like. > > > > Are you thinking something like the way ext_clk is used in smiapp_core.c? > > I went ahead and implemented support for retrieving and storing > > 'clock-rates' during the ov8856_check_hwcfg() call, and then setting > > the rate to the configured rate during probing. > > With assigned-clock-rates, you can simply use clk_get_rate(). Ah, I see. I'll switch to that approach then. > > As you get the actual rate, it could be somewhat off of the intended one. > > -- > Kind regards, > > Sakari Ailus
Hi Robert, On Fri, Mar 27, 2020 at 11:32:29AM +0100, Robert Foss wrote: > On Thu, 26 Mar 2020 at 15:47, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > Hi Robert, > > > > On Thu, Mar 26, 2020 at 12:56:37PM +0100, Robert Foss wrote: > > ... > > > > > +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. > > > > > > I'll add a check for reset_gpio being NULL, and skip the sleep for that case. > > > > There could also be a regulator but no GPIO. > > > > I think if you don't have either, then certainly there's no need for a > > delay. > > Removing the delay if no action is taken makes sense, but I'm not sure > how best to do it. > If there are no regulators dummy ones are created automatically, which > makes distinguishing between a little bit cumbersome. The regulator > structs could of course all be inspected, and if all are dummy ones, > the delay could be skipped. But is there a neater way of doing this? > Manually inspecting the regs strikes me as a bit inelegant. I guess the cleanest, easy way to make this right, albeit slightly unoptimal in very rare cases where you have none of the above resources in a DT system, is to bail out if you're running on an ACPI based system. I.e. checking for e.g. is_acpi_node(dev->fwnode).
On Thu, 26 Mar 2020 at 13:18, Robert Foss <robert.foss@linaro.org> wrote: > > Hey Fabio, > > On Fri, 13 Mar 2020 at 14:15, Fabio Estevam <festevam@gmail.com> wrote: > > > > Hi Robert, > > > > On Fri, Mar 13, 2020 at 8:04 AM Robert Foss <robert.foss@linaro.org> wrote: > > > > > +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); > > > > The parameter of gpiod_set_value_cansleep() is typically 0 (inactive > > state) or 1 (active state), so: > > > > gpiod_set_value_cansleep(ov8856->reset_gpio, 1); > > Ack After testing this change, it breaks the driver during probing. I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they seem to never be 0 or 1. https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/consumer.h#L38 GPIOD_ASIS = 0, GPIOD_IN = 1, GPIOD_OUT_LOW = 3 GPIOD_OUT_HIGH = 7
Hi Robert, On Tue, Mar 31, 2020 at 10:37 AM Robert Foss <robert.foss@linaro.org> wrote: > After testing this change, it breaks the driver during probing. Why exactly does it break probing? Maybe the GPIO polarity defined in the device tree is wrong? > I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they > seem to never be 0 or 1. If you do a grep in all gpiod_set_value_cansleep() usages in the kernel tree, there is not a single case where GPIOD_OUT_HIGH or GPIOD_OUT_LOW is passed as argument of gpiod_set_value_cansleep().
On Tue, Mar 31, 2020 at 4:43 PM Fabio Estevam <festevam@gmail.com> wrote: > On Tue, Mar 31, 2020 at 10:37 AM Robert Foss <robert.foss@linaro.org> wrote: > > > After testing this change, it breaks the driver during probing. > > Why exactly does it break probing? Maybe the GPIO polarity defined in > the device tree is wrong? > > > I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they > > seem to never be 0 or 1. > > If you do a grep in all gpiod_set_value_cansleep() usages in the > kernel tree, there is not a single case where GPIOD_OUT_HIGH or > GPIOD_OUT_LOW is passed as argument of gpiod_set_value_cansleep(). +1. It simple reveals the problem that is somewhere else.