Hi Thierry
On Tue, Sep 22, 2020 at 09:55:01AM +0200, Thierry Reding wrote:
> On Mon, Sep 21, 2020 at 02:39:39PM -0700, Sowjanya Komatineni wrote:
> > IMX274 has analog 2.8V supply, 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 <l...@lucaceresoli.net>
> > Signed-off-by: Sowjanya Komatineni <skomatin...@nvidia.com>
> > ---
> > drivers/media/i2c/imx274.c | 184
> > +++++++++++++++++++++++++++++++++------------
> > 1 file changed, 134 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> > index 5e515f0..b3057a5 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 */
>
> According to the device tree bindings these should be uppercase. Did I
> miss a patch that updates the bindings?Yes! Sorry, there's been some chrun around these bindings https://patchwork.linuxtv.org/project/linux-media/patch/20200917144416.gn...@valkosipuli.retiisi.org.uk/ It should get in for 5.9 as a late fix > > I think the preference is for supply names to be lowercase and given > that there are no users of this binding yet we could update it without > breaking any existing device trees. > > > +}; > > + > > +#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; > > }; > > @@ -726,6 +741,12 @@ static int imx274_start_stream(struct stimx274 *priv) > > { > > int err = 0; > > > > + err = __v4l2_ctrl_handler_setup(&priv->ctrls.handler); > > + if (err) { > > + dev_err(&priv->client->dev, "Error %d setup controls\n", err); > > + return err; > > + } > > + > > /* > > * Refer to "Standby Cancel Sequence when using CSI-2" in > > * imx274 datasheet, it should wait 10ms or more here. > > @@ -767,6 +788,66 @@ static void imx274_reset(struct stimx274 *priv, int > > rst) > > usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); > > } > > > > +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; > > + } > > + > > + udelay(2); > > This looks like some sort of extra delay to make sure all the supply > voltages have settled. Should this perhaps be encoded as part of the > regulator ramp-up times? Or is this really an IC-specific delay that > is needed for some internal timing? > > > + imx274_reset(imx274, 1); > > + > > + return 0; > > + > > +fail_reg: > > + clk_disable_unprepare(imx274->inck); > > + return ret; > > +} > > + > > +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_regulators_get(struct device *dev, struct stimx274 > > *imx274) > > +{ > > + 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); > > +} > > + > > /** > > * imx274_s_ctrl - This is used to set the imx274 V4L2 controls > > * @ctrl: V4L2 control to be set > > @@ -781,6 +862,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; > > I'm not sure I understand this, and sorry if this has been discussed > earlier. Aren't there any other mechanisms in place to ensure that a > control can only be configured when in use? If so, then is this even > necessary? > > If not, silently ignoring at this point seems like it could cause subtle > failures by ignoring some control settings and applying others if the > timing is right. > > > + > > dev_dbg(&imx274->client->dev, > > "%s : s_ctrl: %s, value: %d\n", __func__, > > ctrl->name, ctrl->val); > > @@ -811,6 +895,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) > > break; > > } > > > > + pm_runtime_put(&imx274->client->dev); > > + > > return ret; > > } > > > > @@ -1269,10 +1355,8 @@ static int imx274_s_frame_interval(struct > > v4l2_subdev *sd, > > * > > * Return: 0 on success, errors otherwise > > */ > > -static int imx274_load_default(struct stimx274 *priv) > > +static void imx274_load_default(struct stimx274 *priv) > > { > > - int ret; > > - > > /* load default control values */ > > priv->frame_interval.numerator = 1; > > priv->frame_interval.denominator = IMX274_DEF_FRAME_RATE; > > @@ -1280,29 +1364,6 @@ static int imx274_load_default(struct stimx274 *priv) > > priv->ctrls.gain->val = IMX274_DEF_GAIN; > > priv->ctrls.vflip->val = 0; > > priv->ctrls.test_pattern->val = TEST_PATTERN_DISABLED; > > - > > - /* update frame rate */ > > - ret = imx274_set_frame_interval(priv, > > - priv->frame_interval); > > - if (ret) > > - return ret; > > - > > - /* update exposure time */ > > - ret = v4l2_ctrl_s_ctrl(priv->ctrls.exposure, priv->ctrls.exposure->val); > > - if (ret) > > - return ret; > > - > > - /* update gain */ > > - ret = v4l2_ctrl_s_ctrl(priv->ctrls.gain, priv->ctrls.gain->val); > > - if (ret) > > - return ret; > > - > > - /* update vflip */ > > - ret = v4l2_ctrl_s_ctrl(priv->ctrls.vflip, priv->ctrls.vflip->val); > > - if (ret) > > - return ret; > > This is not moved to somewhere else, so I assume the equivalent will > happen somewhere higher up in the stack? Might be worth mentioning in > the commit message why this can be dropped. > > Thierry
signature.asc
Description: PGP signature
