Re: [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence
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.
Re: [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence
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
Re: [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence
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
> > > Signed-off-by: Sowjanya Komatineni
> > > ---
> > > 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
> > > #include
> > > #include
> > > +#include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -131,6 +133,15 @@
> > > #define IMX274_TABLE_WAIT_MS0
> > > #define IMX274_TABLE_END1
> > >
> > > +/* 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(>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(>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);
> > > +
> > > +
Re: [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence
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
Signed-off-by: Sowjanya Komatineni
---
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
#include
#include
+#include
#include
+#include
#include
#include
#include
@@ -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(>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(>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,
Re: [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence
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
> Signed-off-by: Sowjanya Komatineni
> ---
> 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
> #include
> #include
> +#include
> #include
> +#include
> #include
> #include
> #include
> @@ -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(>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(>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);
