| Message ID | 1599012278-10203-1-git-send-email-skomatineni@nvidia.com |
|---|---|
| Headers | show |
| Series | IMX274 fixes and power on and off implementation | expand |
Hello, On Tue, Sep 01, 2020 at 07:04: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 | 134 ++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 131 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c > index a4b9dfd..79bfac3c6 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: List of analog and digital supply regulators > + * @inck: Pointer to sensor input clock > * @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 > + */ Can I say this does not bring much value ? :) Also the parameter name is wrong > +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); usleep_range() allows you to provide an interval in which your timeout can be coalesced with others. Giving a [1usec, 2usec] range kind of defeat the purpose. And most than everything, does sleeping for 2usec serve any real purpose ? > + imx274_reset(imx274, 1); > + > + return 0; > + > +fail_reg: > + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); regulator_bulk_enable() disables all the regulators that were enabled before the one that failed, so I don't think you need this one here > + clk_disable_unprepare(imx274->inck); > + return ret; > +} > + > +/* > + * imx274_power_off - Function called to power off the sensor > + * @imx274: Pointer to device structure > + */ Same as the above one > +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); > + Is reset before power-off necessary ? > + 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) For symmetry with the regulators API I would call this imx274_regulators_get(). Up to you :) > +{ > + unsigned 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); ^ not sure if it's my email client but you might have a wrong indent here Also, the regulators are optional in the bindings, how do the regulators API cope with that ? I had a look around and they seems to assume regulators are provided. I might be mistaken though > +} > + > /** > * 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; > + Right, but then you should call __v4l2_ctrl_handler_setup() in the s_stream(1) call path to have controls updated (after pm_runtime_get_sync() call for power on). I had a look at it seems only exposure is updated. > 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"); clk_get_optional() might return error. I would check this with IS_ERR > + 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; > + } Doesn't pm_runtime_get calls the poweron function for you ? But anyway, I don't see the device being probed for, in example, querying it's VID/PID for identification during the driver's probe routine. Do you need to power on ? > + > + 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: > @@ -1973,14 +2092,23 @@ static int imx274_remove(struct i2c_client *client) > > v4l2_async_unregister_subdev(sd); > v4l2_ctrl_handler_free(&imx274->ctrls.handler); > + > + pm_runtime_disable(&client->dev); > + pm_runtime_set_suspended(&client->dev); > + > media_entity_cleanup(&sd->entity); > mutex_destroy(&imx274->lock); > return 0; > } > > +static const struct dev_pm_ops imx274_pm_ops = { > + SET_RUNTIME_PM_OPS(imx274_power_off, imx274_power_on, 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, > -- > 2.7.4 >
On 9/3/20 8:03 AM, Jacopo Mondi wrote: > Hello, > > On Tue, Sep 01, 2020 at 07:04: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 | 134 ++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 131 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c >> index a4b9dfd..79bfac3c6 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: List of analog and digital supply regulators >> + * @inck: Pointer to sensor input clock >> * @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 >> + */ > Can I say this does not bring much value ? :) > Also the parameter name is wrong > >> +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); > usleep_range() allows you to provide an interval in which your timeout > can be coalesced with others. Giving a [1usec, 2usec] range kind of > defeat the purpose. And most than everything, does sleeping for 2usec > serve any real purpose ? Following delay recommendation from DS for power on sequence. > > >> + imx274_reset(imx274, 1); >> + >> + return 0; >> + >> +fail_reg: >> + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); > regulator_bulk_enable() disables all the regulators that were enabled > before the one that failed, so I don't think you need this one here > >> + clk_disable_unprepare(imx274->inck); >> + return ret; >> +} >> + >> +/* >> + * imx274_power_off - Function called to power off the sensor >> + * @imx274: Pointer to device structure >> + */ > Same as the above one > >> +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); >> + > Is reset before power-off necessary ? Its recommended power off sequence as per data sheet. Safe to keep sensor in reset before powering down one regulator at a time. > >> + 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) > For symmetry with the regulators API I would call this > imx274_regulators_get(). Up to you :) > >> +{ >> + unsigned 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); > ^ not sure if it's my email > client but you might have a > wrong indent here > > Also, the regulators are optional in the bindings, how do the > regulators API cope with that ? I had a look around and they seems to > assume regulators are provided. I might be mistaken though Yes these are optional regulators and based on feedback from sakari changed to use regulator_bulk_get() here. regulator_bulk_get() uses NORMAL_GET and in case if supplies is not found it will use dummy regulator. >> +} >> + >> /** >> * 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; >> + > Right, but then you should call __v4l2_ctrl_handler_setup() in the > s_stream(1) call path to have controls updated (after > pm_runtime_get_sync() call for power on). I had a look at it seems > only exposure is updated. Existing driver does v4l2_ctrl_handler_setup() in probe(). So, sensor power on happens prior to that in probe() and then powers down during idle. > >> 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"); > clk_get_optional() might return error. I would check this with IS_ERR > >> + 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; >> + } > Doesn't pm_runtime_get calls the poweron function for you ? > > But anyway, I don't see the device being probed for, in example, > querying it's VID/PID for identification during the driver's probe > routine. Do you need to power on ? existing driver does v4l2_ctrl handler setup and loads sensor default control values during probe. So doing sensor power_on here prior to setup. Power off happens during idle. > >> + >> + 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: >> @@ -1973,14 +2092,23 @@ static int imx274_remove(struct i2c_client *client) >> >> v4l2_async_unregister_subdev(sd); >> v4l2_ctrl_handler_free(&imx274->ctrls.handler); >> + >> + pm_runtime_disable(&client->dev); >> + pm_runtime_set_suspended(&client->dev); >> + >> media_entity_cleanup(&sd->entity); >> mutex_destroy(&imx274->lock); >> return 0; >> } >> >> +static const struct dev_pm_ops imx274_pm_ops = { >> + SET_RUNTIME_PM_OPS(imx274_power_off, imx274_power_on, 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, >> -- >> 2.7.4 >>
Hello Sowjanya, On Thu, Sep 03, 2020 at 09:25:44AM -0700, Sowjanya Komatineni wrote: > > On 9/3/20 8:03 AM, Jacopo Mondi wrote: > > Hello, > > > > On Tue, Sep 01, 2020 at 07:04: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 | 134 ++++++++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 131 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c > > > index a4b9dfd..79bfac3c6 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: List of analog and digital supply regulators > > > + * @inck: Pointer to sensor input clock > > > * @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 > > > + */ > > Can I say this does not bring much value ? :) > > Also the parameter name is wrong > > > > > +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); > > usleep_range() allows you to provide an interval in which your timeout > > can be coalesced with others. Giving a [1usec, 2usec] range kind of > > defeat the purpose. And most than everything, does sleeping for 2usec > > serve any real purpose ? > > Following delay recommendation from DS for power on sequence. > 2 useconds ? Seems very short :) > > > > > > > + imx274_reset(imx274, 1); > > > + > > > + return 0; > > > + > > > +fail_reg: > > > + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); > > regulator_bulk_enable() disables all the regulators that were enabled > > before the one that failed, so I don't think you need this one here > > > > > + clk_disable_unprepare(imx274->inck); > > > + return ret; > > > +} > > > + > > > +/* > > > + * imx274_power_off - Function called to power off the sensor > > > + * @imx274: Pointer to device structure > > > + */ > > Same as the above one > > > > > +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); > > > + > > Is reset before power-off necessary ? > > Its recommended power off sequence as per data sheet. > > Safe to keep sensor in reset before powering down one regulator at a time. > Fair enough then! > > > > > + 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) > > For symmetry with the regulators API I would call this > > imx274_regulators_get(). Up to you :) > > > > > +{ > > > + unsigned 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); > > ^ not sure if it's my email > > client but you might have a > > wrong indent here > > > > Also, the regulators are optional in the bindings, how do the > > regulators API cope with that ? I had a look around and they seems to > > assume regulators are provided. I might be mistaken though > > Yes these are optional regulators and based on feedback from sakari changed > to use regulator_bulk_get() here. > > regulator_bulk_get() uses NORMAL_GET and in case if supplies is not found it > will use dummy regulator. > Ah thanks, I had a look at the regulator_get() implementation and missed that. So we're safe here! > > > +} > > > + > > > /** > > > * 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; > > > + > > Right, but then you should call __v4l2_ctrl_handler_setup() in the > > s_stream(1) call path to have controls updated (after > > pm_runtime_get_sync() call for power on). I had a look at it seems > > only exposure is updated. > > Existing driver does v4l2_ctrl_handler_setup() in probe(). So, sensor power > on happens prior to that in probe() and then powers down during idle. > mmm, my point is that with this patch if a control is set while the sensor is powered off it does not get applied, as you return 0 here. This mean the newly set control values have to be applied as soon as it possible, or at least before starting to stream. If you have a look at the imx219 driver, in the s_stream() call path there's a call to v4l2_ctrl_handler_setup() (in imx219_start_streaming()). I think you should do the same unless I mis-interpreted your reply. > > > > > 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"); > > clk_get_optional() might return error. I would check this with IS_ERR > > > > > + 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; > > > + } > > Doesn't pm_runtime_get calls the poweron function for you ? > > > > But anyway, I don't see the device being probed for, in example, > > querying it's VID/PID for identification during the driver's probe > > routine. Do you need to power on ? > > existing driver does v4l2_ctrl handler setup and loads sensor default > control values during probe. Ouch, they're pretty evident and I've missed that. Although I think you can call pm_runtime_get_sync() to have resume executed, but this makes no difference in practice I guess. Thanks for the clarifications, there's just a few items left to address in my opinion. Thanks j > > So doing sensor power_on here prior to setup. Power off happens during idle. > > > > > > + > > > + 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: > > > @@ -1973,14 +2092,23 @@ static int imx274_remove(struct i2c_client *client) > > > > > > v4l2_async_unregister_subdev(sd); > > > v4l2_ctrl_handler_free(&imx274->ctrls.handler); > > > + > > > + pm_runtime_disable(&client->dev); > > > + pm_runtime_set_suspended(&client->dev); > > > + > > > media_entity_cleanup(&sd->entity); > > > mutex_destroy(&imx274->lock); > > > return 0; > > > } > > > > > > +static const struct dev_pm_ops imx274_pm_ops = { > > > + SET_RUNTIME_PM_OPS(imx274_power_off, imx274_power_on, 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, > > > -- > > > 2.7.4 > > >
Hello, On Fri, Sep 04, 2020 at 10:04:10AM -0700, Sowjanya Komatineni wrote: > > On 9/4/20 1:55 AM, Jacopo Mondi wrote: > > > > usleep_range() allows you to provide an interval in which your timeout > > > > can be coalesced with others. Giving a [1usec, 2usec] range kind of > > > > defeat the purpose. And most than everything, does sleeping for 2usec > > > > serve any real purpose ? > > > Following delay recommendation from DS for power on sequence. > > > > > 2 useconds ? Seems very short:) > > > As per IMX274 datasheet for power on sequence, 100ns is the min wait time > after the last power supply of 1v8/1v2/2v8 is ON before releasing RESET > high. ook.. well, it's actually reasonable, it's just the time for the regulators to ramp up, I initially thought it was the time for the chip to exit reset. Let me be a bit more picky and ask if you have considered busy waiting on such a small sleep interval by using udelay. Again, as this happens at chip power on only, the impact on the system of mis-using usleep_range() is negligible, but according to documentation: SLEEPING FOR "A FEW" USECS ( < ~10us? ): * Use udelay - Why not usleep? On slower systems, (embedded, OR perhaps a speed- stepped PC!) the overhead of setting up the hrtimers for usleep *may* not be worth it. Such an evaluation will obviously depend on your specific situation, but it is something to be aware of. Up to you, really! Thanks j
On 9/7/20 12:48 AM, Jacopo Mondi wrote: > Hello, > > On Fri, Sep 04, 2020 at 10:04:10AM -0700, Sowjanya Komatineni wrote: >> On 9/4/20 1:55 AM, Jacopo Mondi wrote: >>>>> usleep_range() allows you to provide an interval in which your timeout >>>>> can be coalesced with others. Giving a [1usec, 2usec] range kind of >>>>> defeat the purpose. And most than everything, does sleeping for 2usec >>>>> serve any real purpose ? >>>> Following delay recommendation from DS for power on sequence. >>>> >>> 2 useconds ? Seems very short:) >>> >> As per IMX274 datasheet for power on sequence, 100ns is the min wait time >> after the last power supply of 1v8/1v2/2v8 is ON before releasing RESET >> high. > ook.. well, it's actually reasonable, it's just the time for the > regulators to ramp up, I initially thought it was the time for the > chip to exit reset. > > Let me be a bit more picky and ask if you have considered busy waiting > on such a small sleep interval by using udelay. Again, as this happens > at chip power on only, the impact on the system of mis-using > usleep_range() is negligible, but according to documentation: > > SLEEPING FOR "A FEW" USECS ( < ~10us? ): > * Use udelay > > - Why not usleep? > On slower systems, (embedded, OR perhaps a speed- > stepped PC!) the overhead of setting up the hrtimers > for usleep *may* not be worth it. Such an evaluation > will obviously depend on your specific situation, but > it is something to be aware of. > > Up to you, really! > > Thanks > j Thanks Jacopo. Will update in v6 to use udelay.