Message ID | 1598903558-9691-5-git-send-email-skomatineni@nvidia.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | IMX274 fixes and power on and off implementation | expand |
Hi Sowjanya, Thanks for the update. On Mon, Aug 31, 2020 at 12:52:38PM -0700, Sowjanya Komatineni wrote: > IMX274 has VANA analog 2.8V supply, VDIG digital core 1.8V supply, > and VDDL digital io 1.2V supply which are optional based on camera > module design. > > IMX274 also need external 24Mhz clock and is optional based on > camera module design. > > This patch adds support for IMX274 power on and off to enable and > disable these supplies and external clock. > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > --- > drivers/media/i2c/imx274.c | 151 ++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 148 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c > index a4b9dfd..18a1e87 100644 > --- a/drivers/media/i2c/imx274.c > +++ b/drivers/media/i2c/imx274.c > @@ -18,7 +18,9 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of_gpio.h> > +#include <linux/pm_runtime.h> > #include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > #include <linux/slab.h> > #include <linux/v4l2-mediabus.h> > #include <linux/videodev2.h> > @@ -131,6 +133,15 @@ > #define IMX274_TABLE_WAIT_MS 0 > #define IMX274_TABLE_END 1 > > +/* regulator supplies */ > +static const char * const imx274_supply_names[] = { > + "vddl", /* IF (1.2V) supply */ > + "vdig", /* Digital Core (1.8V) supply */ > + "vana", /* Analog (2.8V) supply */ > +}; > + > +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names) > + > /* > * imx274 I2C operation related structure > */ > @@ -501,6 +512,8 @@ struct imx274_ctrls { > * @frame_rate: V4L2 frame rate structure > * @regmap: Pointer to regmap structure > * @reset_gpio: Pointer to reset gpio > + * @supplies: imx274 analog and digital supplies There's interface, too. You could also omit what there is as it's easily visible elsewhere. > + * @inck: input clock to imx274 > * @lock: Mutex structure > * @mode: Parameters for the selected readout mode > */ > @@ -514,6 +527,8 @@ struct stimx274 { > struct v4l2_fract frame_interval; > struct regmap *regmap; > struct gpio_desc *reset_gpio; > + struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES]; > + struct clk *inck; > struct mutex lock; /* mutex lock for operations */ > const struct imx274_mode *mode; > }; > @@ -767,6 +782,75 @@ static void imx274_reset(struct stimx274 *priv, int rst) > usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); > } > > +/* > + * imx274_power_on - Function called to power on the sensor > + * @imx274: Pointer to device structure > + */ > +static int imx274_power_on(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct stimx274 *imx274 = to_imx274(sd); > + int ret; > + > + /* keep sensor in reset before power on */ > + imx274_reset(imx274, 0); > + > + ret = clk_prepare_enable(imx274->inck); > + if (ret) { > + dev_err(&imx274->client->dev, > + "Failed to enable input clock: %d\n", ret); > + return ret; > + } > + > + ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies); > + if (ret) { > + dev_err(&imx274->client->dev, > + "Failed to enable regulators: %d\n", ret); > + goto fail_reg; > + } > + > + usleep_range(1, 2); > + imx274_reset(imx274, 1); > + > + return 0; > + > +fail_reg: > + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); > + clk_disable_unprepare(imx274->inck); > + return ret; > +} > + > +/* > + * imx274_power_off - Function called to power off the sensor > + * @imx274: Pointer to device structure > + */ > +static int imx274_power_off(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct stimx274 *imx274 = to_imx274(sd); > + > + imx274_reset(imx274, 0); > + > + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); > + > + clk_disable_unprepare(imx274->inck); > + > + return 0; > +} > + > +static int imx274_get_regulators(struct device *dev, struct stimx274 *imx274) > +{ > + int i; unsigned int > + > + for (i = 0; i < IMX274_NUM_SUPPLIES; i++) > + imx274->supplies[i].supply = imx274_supply_names[i]; > + > + return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES, > + imx274->supplies); > +} > + > /** > * imx274_s_ctrl - This is used to set the imx274 V4L2 controls > * @ctrl: V4L2 control to be set > @@ -781,6 +865,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) > struct stimx274 *imx274 = to_imx274(sd); > int ret = -EINVAL; > > + if (!pm_runtime_get_if_in_use(&imx274->client->dev)) > + return 0; > + > dev_dbg(&imx274->client->dev, > "%s : s_ctrl: %s, value: %d\n", __func__, > ctrl->name, ctrl->val); > @@ -811,6 +898,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) > break; > } > > + pm_runtime_put(&imx274->client->dev); > + > return ret; > } > > @@ -1327,6 +1416,13 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) > mutex_lock(&imx274->lock); > > if (on) { > + ret = pm_runtime_get_sync(&imx274->client->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&imx274->client->dev); > + mutex_unlock(&imx274->lock); > + return ret; > + } > + > /* load mode registers */ > ret = imx274_mode_regs(imx274); > if (ret) > @@ -1362,6 +1458,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) > ret = imx274_write_table(imx274, imx274_stop); > if (ret) > goto fail; > + pm_runtime_put(&imx274->client->dev); > } > > mutex_unlock(&imx274->lock); > @@ -1369,6 +1466,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) > return 0; > > fail: > + pm_runtime_put(&imx274->client->dev); > mutex_unlock(&imx274->lock); > dev_err(&imx274->client->dev, "s_stream failed\n"); > return ret; > @@ -1834,6 +1932,14 @@ static int imx274_probe(struct i2c_client *client) > > mutex_init(&imx274->lock); > > + imx274->inck = devm_clk_get_optional(&client->dev, "inck"); > + ret = imx274_get_regulators(&client->dev, imx274); > + if (ret) { > + dev_err(&client->dev, > + "Failed to get power regulators, err: %d\n", ret); > + return ret; > + } > + > /* initialize format */ > imx274->mode = &imx274_modes[IMX274_DEFAULT_BINNING]; > imx274->crop.width = IMX274_MAX_WIDTH; > @@ -1881,15 +1987,23 @@ static int imx274_probe(struct i2c_client *client) > goto err_me; > } > > - /* pull sensor out of reset */ > - imx274_reset(imx274, 1); > + /* power on the sensor */ > + ret = imx274_power_on(&client->dev); > + if (ret < 0) { > + dev_err(&client->dev, > + "%s : imx274 power on failed\n", __func__); > + goto err_me; > + } > + > + pm_runtime_set_active(&client->dev); > + pm_runtime_enable(&client->dev); > > /* initialize controls */ > ret = v4l2_ctrl_handler_init(&imx274->ctrls.handler, 4); > if (ret < 0) { > dev_err(&client->dev, > "%s : ctrl handler init Failed\n", __func__); > - goto err_me; > + goto err_disable_rpm; > } > > imx274->ctrls.handler.lock = &imx274->lock; > @@ -1951,11 +2065,16 @@ static int imx274_probe(struct i2c_client *client) > goto err_ctrls; > } > > + pm_runtime_idle(&client->dev); > + > dev_info(&client->dev, "imx274 : imx274 probe success !\n"); > return 0; > > err_ctrls: > v4l2_ctrl_handler_free(&imx274->ctrls.handler); > +err_disable_rpm: > + pm_runtime_disable(&client->dev); > + pm_runtime_set_suspended(&client->dev); > err_me: > media_entity_cleanup(&sd->entity); > err_regmap: > @@ -1968,19 +2087,45 @@ static int imx274_remove(struct i2c_client *client) > struct v4l2_subdev *sd = i2c_get_clientdata(client); > struct stimx274 *imx274 = to_imx274(sd); > > + pm_runtime_get_sync(&imx274->client->dev); > + > /* stop stream */ This really shouldn't happen and the driver isn't expected to handle it either. > imx274_write_table(imx274, imx274_stop); > > v4l2_async_unregister_subdev(sd); > v4l2_ctrl_handler_free(&imx274->ctrls.handler); > + > + pm_runtime_put(&client->dev); > + pm_runtime_disable(&client->dev); > + pm_runtime_set_suspended(&client->dev); > + > media_entity_cleanup(&sd->entity); > mutex_destroy(&imx274->lock); > return 0; > } > > +static int __maybe_unused imx274_runtime_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + > + return imx274_power_off(&client->dev); > +} > + > +static int __maybe_unused imx274_runtime_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + > + return imx274_power_on(&client->dev); > +} There's no need for the two wrappers, please remove them. > + > +static const struct dev_pm_ops imx274_pm_ops = { > + SET_RUNTIME_PM_OPS(imx274_runtime_suspend, imx274_runtime_resume, NULL) > +}; > + > static struct i2c_driver imx274_i2c_driver = { > .driver = { > .name = DRIVER_NAME, > + .pm = &imx274_pm_ops, > .of_match_table = imx274_of_id_table, > }, > .probe_new = imx274_probe,
On Mon, Aug 31, 2020 at 01:46:00PM -0700, Sowjanya Komatineni wrote: > > On 8/31/20 1:23 PM, Sakari Ailus wrote: > > > @@ -1968,19 +2087,45 @@ static int imx274_remove(struct i2c_client *client) > > > struct v4l2_subdev *sd = i2c_get_clientdata(client); > > > struct stimx274 *imx274 = to_imx274(sd); > > > + pm_runtime_get_sync(&imx274->client->dev); > > > + > > > /* stop stream */ > > This really shouldn't happen and the driver isn't expected to handle it > > either. > > Do you mean to remove stop stream during remove()? > > Stop stream is not part of this change and as writes to sensor can't happen > when power off, added pm_runtime_get_sync Indeed. But there certainly isn't a need to power the sensor on to stream off, is there? > > > > imx274_write_table(imx274, imx274_stop); > > > v4l2_async_unregister_subdev(sd); > > > v4l2_ctrl_handler_free(&imx274->ctrls.handler); > > > + > > > + pm_runtime_put(&client->dev); > > > + pm_runtime_disable(&client->dev); > > > + pm_runtime_set_suspended(&client->dev); > > > + > > > media_entity_cleanup(&sd->entity); > > > mutex_destroy(&imx274->lock); > > > return 0; > > > }
On 8/31/20 1:48 PM, Sakari Ailus wrote: > On Mon, Aug 31, 2020 at 01:46:00PM -0700, Sowjanya Komatineni wrote: >> On 8/31/20 1:23 PM, Sakari Ailus wrote: >>>> @@ -1968,19 +2087,45 @@ static int imx274_remove(struct i2c_client *client) >>>> struct v4l2_subdev *sd = i2c_get_clientdata(client); >>>> struct stimx274 *imx274 = to_imx274(sd); >>>> + pm_runtime_get_sync(&imx274->client->dev); >>>> + >>>> /* stop stream */ >>> This really shouldn't happen and the driver isn't expected to handle it >>> either. >> Do you mean to remove stop stream during remove()? >> >> Stop stream is not part of this change and as writes to sensor can't happen >> when power off, added pm_runtime_get_sync > Indeed. > > But there certainly isn't a need to power the sensor on to stream off, is > there? yeah not required. Will remove get_sync. Will remove imx274 stop stream during remove() as separate patch as it was not introduced in this. >>>> imx274_write_table(imx274, imx274_stop); >>>> v4l2_async_unregister_subdev(sd); >>>> v4l2_ctrl_handler_free(&imx274->ctrls.handler); >>>> + >>>> + pm_runtime_put(&client->dev); >>>> + pm_runtime_disable(&client->dev); >>>> + pm_runtime_set_suspended(&client->dev); >>>> + >>>> media_entity_cleanup(&sd->entity); >>>> mutex_destroy(&imx274->lock); >>>> return 0; >>>> }
diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c index a4b9dfd..18a1e87 100644 --- a/drivers/media/i2c/imx274.c +++ b/drivers/media/i2c/imx274.c @@ -18,7 +18,9 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/of_gpio.h> +#include <linux/pm_runtime.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include <linux/slab.h> #include <linux/v4l2-mediabus.h> #include <linux/videodev2.h> @@ -131,6 +133,15 @@ #define IMX274_TABLE_WAIT_MS 0 #define IMX274_TABLE_END 1 +/* regulator supplies */ +static const char * const imx274_supply_names[] = { + "vddl", /* IF (1.2V) supply */ + "vdig", /* Digital Core (1.8V) supply */ + "vana", /* Analog (2.8V) supply */ +}; + +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names) + /* * imx274 I2C operation related structure */ @@ -501,6 +512,8 @@ struct imx274_ctrls { * @frame_rate: V4L2 frame rate structure * @regmap: Pointer to regmap structure * @reset_gpio: Pointer to reset gpio + * @supplies: imx274 analog and digital supplies + * @inck: input clock to imx274 * @lock: Mutex structure * @mode: Parameters for the selected readout mode */ @@ -514,6 +527,8 @@ struct stimx274 { struct v4l2_fract frame_interval; struct regmap *regmap; struct gpio_desc *reset_gpio; + struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES]; + struct clk *inck; struct mutex lock; /* mutex lock for operations */ const struct imx274_mode *mode; }; @@ -767,6 +782,75 @@ static void imx274_reset(struct stimx274 *priv, int rst) usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); } +/* + * imx274_power_on - Function called to power on the sensor + * @imx274: Pointer to device structure + */ +static int imx274_power_on(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct stimx274 *imx274 = to_imx274(sd); + int ret; + + /* keep sensor in reset before power on */ + imx274_reset(imx274, 0); + + ret = clk_prepare_enable(imx274->inck); + if (ret) { + dev_err(&imx274->client->dev, + "Failed to enable input clock: %d\n", ret); + return ret; + } + + ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies); + if (ret) { + dev_err(&imx274->client->dev, + "Failed to enable regulators: %d\n", ret); + goto fail_reg; + } + + usleep_range(1, 2); + imx274_reset(imx274, 1); + + return 0; + +fail_reg: + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); + clk_disable_unprepare(imx274->inck); + return ret; +} + +/* + * imx274_power_off - Function called to power off the sensor + * @imx274: Pointer to device structure + */ +static int imx274_power_off(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct stimx274 *imx274 = to_imx274(sd); + + imx274_reset(imx274, 0); + + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); + + clk_disable_unprepare(imx274->inck); + + return 0; +} + +static int imx274_get_regulators(struct device *dev, struct stimx274 *imx274) +{ + int i; + + for (i = 0; i < IMX274_NUM_SUPPLIES; i++) + imx274->supplies[i].supply = imx274_supply_names[i]; + + return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES, + imx274->supplies); +} + /** * imx274_s_ctrl - This is used to set the imx274 V4L2 controls * @ctrl: V4L2 control to be set @@ -781,6 +865,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) struct stimx274 *imx274 = to_imx274(sd); int ret = -EINVAL; + if (!pm_runtime_get_if_in_use(&imx274->client->dev)) + return 0; + dev_dbg(&imx274->client->dev, "%s : s_ctrl: %s, value: %d\n", __func__, ctrl->name, ctrl->val); @@ -811,6 +898,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) break; } + pm_runtime_put(&imx274->client->dev); + return ret; } @@ -1327,6 +1416,13 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) mutex_lock(&imx274->lock); if (on) { + ret = pm_runtime_get_sync(&imx274->client->dev); + if (ret < 0) { + pm_runtime_put_noidle(&imx274->client->dev); + mutex_unlock(&imx274->lock); + return ret; + } + /* load mode registers */ ret = imx274_mode_regs(imx274); if (ret) @@ -1362,6 +1458,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) ret = imx274_write_table(imx274, imx274_stop); if (ret) goto fail; + pm_runtime_put(&imx274->client->dev); } mutex_unlock(&imx274->lock); @@ -1369,6 +1466,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) return 0; fail: + pm_runtime_put(&imx274->client->dev); mutex_unlock(&imx274->lock); dev_err(&imx274->client->dev, "s_stream failed\n"); return ret; @@ -1834,6 +1932,14 @@ static int imx274_probe(struct i2c_client *client) mutex_init(&imx274->lock); + imx274->inck = devm_clk_get_optional(&client->dev, "inck"); + ret = imx274_get_regulators(&client->dev, imx274); + if (ret) { + dev_err(&client->dev, + "Failed to get power regulators, err: %d\n", ret); + return ret; + } + /* initialize format */ imx274->mode = &imx274_modes[IMX274_DEFAULT_BINNING]; imx274->crop.width = IMX274_MAX_WIDTH; @@ -1881,15 +1987,23 @@ static int imx274_probe(struct i2c_client *client) goto err_me; } - /* pull sensor out of reset */ - imx274_reset(imx274, 1); + /* power on the sensor */ + ret = imx274_power_on(&client->dev); + if (ret < 0) { + dev_err(&client->dev, + "%s : imx274 power on failed\n", __func__); + goto err_me; + } + + pm_runtime_set_active(&client->dev); + pm_runtime_enable(&client->dev); /* initialize controls */ ret = v4l2_ctrl_handler_init(&imx274->ctrls.handler, 4); if (ret < 0) { dev_err(&client->dev, "%s : ctrl handler init Failed\n", __func__); - goto err_me; + goto err_disable_rpm; } imx274->ctrls.handler.lock = &imx274->lock; @@ -1951,11 +2065,16 @@ static int imx274_probe(struct i2c_client *client) goto err_ctrls; } + pm_runtime_idle(&client->dev); + dev_info(&client->dev, "imx274 : imx274 probe success !\n"); return 0; err_ctrls: v4l2_ctrl_handler_free(&imx274->ctrls.handler); +err_disable_rpm: + pm_runtime_disable(&client->dev); + pm_runtime_set_suspended(&client->dev); err_me: media_entity_cleanup(&sd->entity); err_regmap: @@ -1968,19 +2087,45 @@ static int imx274_remove(struct i2c_client *client) struct v4l2_subdev *sd = i2c_get_clientdata(client); struct stimx274 *imx274 = to_imx274(sd); + pm_runtime_get_sync(&imx274->client->dev); + /* stop stream */ imx274_write_table(imx274, imx274_stop); v4l2_async_unregister_subdev(sd); v4l2_ctrl_handler_free(&imx274->ctrls.handler); + + pm_runtime_put(&client->dev); + pm_runtime_disable(&client->dev); + pm_runtime_set_suspended(&client->dev); + media_entity_cleanup(&sd->entity); mutex_destroy(&imx274->lock); return 0; } +static int __maybe_unused imx274_runtime_suspend(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + + return imx274_power_off(&client->dev); +} + +static int __maybe_unused imx274_runtime_resume(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + + return imx274_power_on(&client->dev); +} + +static const struct dev_pm_ops imx274_pm_ops = { + SET_RUNTIME_PM_OPS(imx274_runtime_suspend, imx274_runtime_resume, NULL) +}; + static struct i2c_driver imx274_i2c_driver = { .driver = { .name = DRIVER_NAME, + .pm = &imx274_pm_ops, .of_match_table = imx274_of_id_table, }, .probe_new = imx274_probe,