LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] media: i2c: dw9714: add optional regulator support
@ 2021-11-26  9:01 Martin Kepplinger
  2021-11-26 10:44 ` Sakari Ailus
  2021-11-26 13:05 ` Mark Brown
  0 siblings, 2 replies; 5+ messages in thread
From: Martin Kepplinger @ 2021-11-26  9:01 UTC (permalink / raw)
  To: martin.kepplinger, mchehab, broonie, sakari.ailus
  Cc: kernel, linux-kernel, linux-media, linux-pm, Angus Ainslie

From: Angus Ainslie <angus@akkea.ca>

Allow the dw9714 to control a regulator and adjust suspend() and resume()
to support both runtime and system pm.

Signed-off-by: Angus Ainslie <angus@akkea.ca>
Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---

revision history
----------------

v2: (thank you Mark)
 * simplify the regulator_get_optional() error path
 * fix regulator usage during probe()

v1:
https://lore.kernel.org/linux-media/20211125080922.978583-1-martin.kepplinger@puri.sm/



 drivers/media/i2c/dw9714.c | 39 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
index 3863dfeb8293..e8cc19b89861 100644
--- a/drivers/media/i2c/dw9714.c
+++ b/drivers/media/i2c/dw9714.c
@@ -5,6 +5,7 @@
 #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-event.h>
@@ -36,6 +37,7 @@ struct dw9714_device {
 	struct v4l2_ctrl_handler ctrls_vcm;
 	struct v4l2_subdev sd;
 	u16 current_val;
+	struct regulator *vcc;
 };
 
 static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl *ctrl)
@@ -145,6 +147,21 @@ static int dw9714_probe(struct i2c_client *client)
 	if (dw9714_dev == NULL)
 		return -ENOMEM;
 
+	dw9714_dev->vcc = devm_regulator_get_optional(&client->dev, "vcc");
+	if (IS_ERR(dw9714_dev->vcc)) {
+		dev_dbg(&client->dev, "No vcc regulator found: %ld\n",
+			PTR_ERR(dw9714_dev->vcc));
+		dw9714_dev->vcc = NULL;
+	}
+
+	if (dw9714_dev->vcc) {
+		rval = regulator_enable(dw9714_dev->vcc);
+		if (rval < 0) {
+			dev_err(&client->dev, "failed to enable vcc: %d\n", rval);
+			return rval;
+		}
+	}
+
 	v4l2_i2c_subdev_init(&dw9714_dev->sd, client, &dw9714_ops);
 	dw9714_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
 				V4L2_SUBDEV_FL_HAS_EVENTS;
@@ -200,6 +217,9 @@ static int __maybe_unused dw9714_vcm_suspend(struct device *dev)
 	struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
 	int ret, val;
 
+	if (pm_runtime_suspended(&client->dev))
+		return 0;
+
 	for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
 	     val >= 0; val -= DW9714_CTRL_STEPS) {
 		ret = dw9714_i2c_write(client,
@@ -208,6 +228,13 @@ static int __maybe_unused dw9714_vcm_suspend(struct device *dev)
 			dev_err_once(dev, "%s I2C failure: %d", __func__, ret);
 		usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
 	}
+
+	if (dw9714_dev->vcc) {
+		ret = regulator_disable(dw9714_dev->vcc);
+		if (ret)
+			dev_err(dev, "Failed to disable vcc: %d\n", ret);
+	}
+
 	return 0;
 }
 
@@ -224,6 +251,18 @@ static int  __maybe_unused dw9714_vcm_resume(struct device *dev)
 	struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
 	int ret, val;
 
+	if (pm_runtime_suspended(&client->dev))
+		return 0;
+
+	if (dw9714_dev->vcc) {
+		ret = regulator_enable(dw9714_dev->vcc);
+		if (ret) {
+			dev_err(dev, "Failed to enable vcc: %d\n", ret);
+			return ret;
+		}
+		usleep_range(1000, 2000);
+	}
+
 	for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
 	     val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
 	     val += DW9714_CTRL_STEPS) {
-- 
2.30.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] media: i2c: dw9714: add optional regulator support
  2021-11-26  9:01 [PATCH v2] media: i2c: dw9714: add optional regulator support Martin Kepplinger
@ 2021-11-26 10:44 ` Sakari Ailus
  2021-11-26 11:06   ` Martin Kepplinger
  2021-11-26 13:05 ` Mark Brown
  1 sibling, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2021-11-26 10:44 UTC (permalink / raw)
  To: Martin Kepplinger
  Cc: mchehab, broonie, kernel, linux-kernel, linux-media, linux-pm,
	Angus Ainslie

Hi Martin,

On Fri, Nov 26, 2021 at 10:01:07AM +0100, Martin Kepplinger wrote:
> From: Angus Ainslie <angus@akkea.ca>
> 
> Allow the dw9714 to control a regulator and adjust suspend() and resume()
> to support both runtime and system pm.
> 
> Signed-off-by: Angus Ainslie <angus@akkea.ca>
> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> ---
> 
> revision history
> ----------------
> 
> v2: (thank you Mark)
>  * simplify the regulator_get_optional() error path
>  * fix regulator usage during probe()
> 
> v1:
> https://lore.kernel.org/linux-media/20211125080922.978583-1-martin.kepplinger@puri.sm/
> 
> 
> 
>  drivers/media/i2c/dw9714.c | 39 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
> index 3863dfeb8293..e8cc19b89861 100644
> --- a/drivers/media/i2c/dw9714.c
> +++ b/drivers/media/i2c/dw9714.c
> @@ -5,6 +5,7 @@
>  #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-event.h>
> @@ -36,6 +37,7 @@ struct dw9714_device {
>  	struct v4l2_ctrl_handler ctrls_vcm;
>  	struct v4l2_subdev sd;
>  	u16 current_val;
> +	struct regulator *vcc;
>  };
>  
>  static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl *ctrl)
> @@ -145,6 +147,21 @@ static int dw9714_probe(struct i2c_client *client)
>  	if (dw9714_dev == NULL)
>  		return -ENOMEM;
>  
> +	dw9714_dev->vcc = devm_regulator_get_optional(&client->dev, "vcc");

You you used regular devm_regulator_get(), you could remove the error
handling below. If there's no regulator, you'll simply get a dummy one.

> +	if (IS_ERR(dw9714_dev->vcc)) {
> +		dev_dbg(&client->dev, "No vcc regulator found: %ld\n",
> +			PTR_ERR(dw9714_dev->vcc));
> +		dw9714_dev->vcc = NULL;
> +	}
> +
> +	if (dw9714_dev->vcc) {

With (dummy) regulators, these checks become unnecessary.

> +		rval = regulator_enable(dw9714_dev->vcc);
> +		if (rval < 0) {
> +			dev_err(&client->dev, "failed to enable vcc: %d\n", rval);
> +			return rval;
> +		}
> +	}
> +
>  	v4l2_i2c_subdev_init(&dw9714_dev->sd, client, &dw9714_ops);
>  	dw9714_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>  				V4L2_SUBDEV_FL_HAS_EVENTS;
> @@ -200,6 +217,9 @@ static int __maybe_unused dw9714_vcm_suspend(struct device *dev)
>  	struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
>  	int ret, val;
>  
> +	if (pm_runtime_suspended(&client->dev))
> +		return 0;

This can't take place in a runtime PM suspend callback. You'll need to add
system suspend callback for this.

> +
>  	for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1);
>  	     val >= 0; val -= DW9714_CTRL_STEPS) {
>  		ret = dw9714_i2c_write(client,
> @@ -208,6 +228,13 @@ static int __maybe_unused dw9714_vcm_suspend(struct device *dev)
>  			dev_err_once(dev, "%s I2C failure: %d", __func__, ret);
>  		usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
>  	}
> +
> +	if (dw9714_dev->vcc) {
> +		ret = regulator_disable(dw9714_dev->vcc);
> +		if (ret)
> +			dev_err(dev, "Failed to disable vcc: %d\n", ret);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -224,6 +251,18 @@ static int  __maybe_unused dw9714_vcm_resume(struct device *dev)
>  	struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
>  	int ret, val;
>  
> +	if (pm_runtime_suspended(&client->dev))

Same for this one.

> +		return 0;
> +
> +	if (dw9714_dev->vcc) {
> +		ret = regulator_enable(dw9714_dev->vcc);
> +		if (ret) {
> +			dev_err(dev, "Failed to enable vcc: %d\n", ret);
> +			return ret;
> +		}
> +		usleep_range(1000, 2000);
> +	}
> +
>  	for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
>  	     val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
>  	     val += DW9714_CTRL_STEPS) {

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] media: i2c: dw9714: add optional regulator support
  2021-11-26 10:44 ` Sakari Ailus
@ 2021-11-26 11:06   ` Martin Kepplinger
  2021-11-26 12:28     ` Sakari Ailus
  0 siblings, 1 reply; 5+ messages in thread
From: Martin Kepplinger @ 2021-11-26 11:06 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: mchehab, broonie, kernel, linux-kernel, linux-media, linux-pm,
	Angus Ainslie

Am Freitag, dem 26.11.2021 um 12:44 +0200 schrieb Sakari Ailus:
> Hi Martin,
> 
> On Fri, Nov 26, 2021 at 10:01:07AM +0100, Martin Kepplinger wrote:
> > From: Angus Ainslie <angus@akkea.ca>
> > 
> > Allow the dw9714 to control a regulator and adjust suspend() and
> > resume()
> > to support both runtime and system pm.
> > 
> > Signed-off-by: Angus Ainslie <angus@akkea.ca>
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > ---
> > 
> > revision history
> > ----------------
> > 
> > v2: (thank you Mark)
> >  * simplify the regulator_get_optional() error path
> >  * fix regulator usage during probe()
> > 
> > v1:
> > https://lore.kernel.org/linux-media/20211125080922.978583-1-martin.kepplinger@puri.sm/
> > 
> > 
> > 
> >  drivers/media/i2c/dw9714.c | 39
> > ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/dw9714.c
> > b/drivers/media/i2c/dw9714.c
> > index 3863dfeb8293..e8cc19b89861 100644
> > --- a/drivers/media/i2c/dw9714.c
> > +++ b/drivers/media/i2c/dw9714.c
> > @@ -5,6 +5,7 @@
> >  #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-event.h>
> > @@ -36,6 +37,7 @@ struct dw9714_device {
> >         struct v4l2_ctrl_handler ctrls_vcm;
> >         struct v4l2_subdev sd;
> >         u16 current_val;
> > +       struct regulator *vcc;
> >  };
> >  
> >  static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl
> > *ctrl)
> > @@ -145,6 +147,21 @@ static int dw9714_probe(struct i2c_client
> > *client)
> >         if (dw9714_dev == NULL)
> >                 return -ENOMEM;
> >  
> > +       dw9714_dev->vcc = devm_regulator_get_optional(&client->dev,
> > "vcc");
> 
> You you used regular devm_regulator_get(), you could remove the error
> handling below. If there's no regulator, you'll simply get a dummy
> one.

ok thanks


> 
> > +       if (IS_ERR(dw9714_dev->vcc)) {
> > +               dev_dbg(&client->dev, "No vcc regulator found:
> > %ld\n",
> > +                       PTR_ERR(dw9714_dev->vcc));
> > +               dw9714_dev->vcc = NULL;
> > +       }
> > +
> > +       if (dw9714_dev->vcc) {
> 
> With (dummy) regulators, these checks become unnecessary.
> 
> > +               rval = regulator_enable(dw9714_dev->vcc);
> > +               if (rval < 0) {
> > +                       dev_err(&client->dev, "failed to enable
> > vcc: %d\n", rval);
> > +                       return rval;
> > +               }
> > +       }
> > +
> >         v4l2_i2c_subdev_init(&dw9714_dev->sd, client, &dw9714_ops);
> >         dw9714_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> >                                 V4L2_SUBDEV_FL_HAS_EVENTS;
> > @@ -200,6 +217,9 @@ static int __maybe_unused
> > dw9714_vcm_suspend(struct device *dev)
> >         struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
> >         int ret, val;
> >  
> > +       if (pm_runtime_suspended(&client->dev))
> > +               return 0;
> 
> This can't take place in a runtime PM suspend callback. You'll need
> to add
> system suspend callback for this.

but this function is both the system and runtime suspend callback.
doesn't splitting up the callbacks just add lines of code
unnecessarily?

> 
> > +
> >         for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS -
> > 1);
> >              val >= 0; val -= DW9714_CTRL_STEPS) {
> >                 ret = dw9714_i2c_write(client,
> > @@ -208,6 +228,13 @@ static int __maybe_unused
> > dw9714_vcm_suspend(struct device *dev)
> >                         dev_err_once(dev, "%s I2C failure: %d",
> > __func__, ret);
> >                 usleep_range(DW9714_CTRL_DELAY_US,
> > DW9714_CTRL_DELAY_US + 10);
> >         }
> > +
> > +       if (dw9714_dev->vcc) {
> > +               ret = regulator_disable(dw9714_dev->vcc);
> > +               if (ret)
> > +                       dev_err(dev, "Failed to disable vcc: %d\n",
> > ret);
> > +       }
> > +
> >         return 0;
> >  }
> >  
> > @@ -224,6 +251,18 @@ static int  __maybe_unused
> > dw9714_vcm_resume(struct device *dev)
> >         struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
> >         int ret, val;
> >  
> > +       if (pm_runtime_suspended(&client->dev))
> 
> Same for this one.
> 
> > +               return 0;
> > +
> > +       if (dw9714_dev->vcc) {
> > +               ret = regulator_enable(dw9714_dev->vcc);
> > +               if (ret) {
> > +                       dev_err(dev, "Failed to enable vcc: %d\n",
> > ret);
> > +                       return ret;
> > +               }
> > +               usleep_range(1000, 2000);
> > +       }
> > +
> >         for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
> >              val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
> >              val += DW9714_CTRL_STEPS) {
> 



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] media: i2c: dw9714: add optional regulator support
  2021-11-26 11:06   ` Martin Kepplinger
@ 2021-11-26 12:28     ` Sakari Ailus
  0 siblings, 0 replies; 5+ messages in thread
From: Sakari Ailus @ 2021-11-26 12:28 UTC (permalink / raw)
  To: Martin Kepplinger
  Cc: mchehab, broonie, kernel, linux-kernel, linux-media, linux-pm,
	Angus Ainslie

On Fri, Nov 26, 2021 at 12:06:03PM +0100, Martin Kepplinger wrote:
> Am Freitag, dem 26.11.2021 um 12:44 +0200 schrieb Sakari Ailus:
> > Hi Martin,
> > 
> > On Fri, Nov 26, 2021 at 10:01:07AM +0100, Martin Kepplinger wrote:
> > > From: Angus Ainslie <angus@akkea.ca>
> > > 
> > > Allow the dw9714 to control a regulator and adjust suspend() and
> > > resume()
> > > to support both runtime and system pm.
> > > 
> > > Signed-off-by: Angus Ainslie <angus@akkea.ca>
> > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > > ---
> > > 
> > > revision history
> > > ----------------
> > > 
> > > v2: (thank you Mark)
> > >  * simplify the regulator_get_optional() error path
> > >  * fix regulator usage during probe()
> > > 
> > > v1:
> > > https://lore.kernel.org/linux-media/20211125080922.978583-1-martin.kepplinger@puri.sm/
> > > 
> > > 
> > > 
> > >  drivers/media/i2c/dw9714.c | 39
> > > ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 39 insertions(+)
> > > 
> > > diff --git a/drivers/media/i2c/dw9714.c
> > > b/drivers/media/i2c/dw9714.c
> > > index 3863dfeb8293..e8cc19b89861 100644
> > > --- a/drivers/media/i2c/dw9714.c
> > > +++ b/drivers/media/i2c/dw9714.c
> > > @@ -5,6 +5,7 @@
> > >  #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-event.h>
> > > @@ -36,6 +37,7 @@ struct dw9714_device {
> > >         struct v4l2_ctrl_handler ctrls_vcm;
> > >         struct v4l2_subdev sd;
> > >         u16 current_val;
> > > +       struct regulator *vcc;
> > >  };
> > >  
> > >  static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl
> > > *ctrl)
> > > @@ -145,6 +147,21 @@ static int dw9714_probe(struct i2c_client
> > > *client)
> > >         if (dw9714_dev == NULL)
> > >                 return -ENOMEM;
> > >  
> > > +       dw9714_dev->vcc = devm_regulator_get_optional(&client->dev,
> > > "vcc");
> > 
> > You you used regular devm_regulator_get(), you could remove the error
> > handling below. If there's no regulator, you'll simply get a dummy
> > one.
> 
> ok thanks
> 
> 
> > 
> > > +       if (IS_ERR(dw9714_dev->vcc)) {
> > > +               dev_dbg(&client->dev, "No vcc regulator found:
> > > %ld\n",
> > > +                       PTR_ERR(dw9714_dev->vcc));
> > > +               dw9714_dev->vcc = NULL;
> > > +       }
> > > +
> > > +       if (dw9714_dev->vcc) {
> > 
> > With (dummy) regulators, these checks become unnecessary.
> > 
> > > +               rval = regulator_enable(dw9714_dev->vcc);
> > > +               if (rval < 0) {
> > > +                       dev_err(&client->dev, "failed to enable
> > > vcc: %d\n", rval);
> > > +                       return rval;
> > > +               }
> > > +       }
> > > +
> > >         v4l2_i2c_subdev_init(&dw9714_dev->sd, client, &dw9714_ops);
> > >         dw9714_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > >                                 V4L2_SUBDEV_FL_HAS_EVENTS;
> > > @@ -200,6 +217,9 @@ static int __maybe_unused
> > > dw9714_vcm_suspend(struct device *dev)
> > >         struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
> > >         int ret, val;
> > >  
> > > +       if (pm_runtime_suspended(&client->dev))
> > > +               return 0;
> > 
> > This can't take place in a runtime PM suspend callback. You'll need
> > to add
> > system suspend callback for this.
> 
> but this function is both the system and runtime suspend callback.
> doesn't splitting up the callbacks just add lines of code
> unnecessarily?

Hmm. After thinking about this a little, I think this could indeed work.

Yeah, please leave it as-is.

> 
> > 
> > > +
> > >         for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS -
> > > 1);
> > >              val >= 0; val -= DW9714_CTRL_STEPS) {
> > >                 ret = dw9714_i2c_write(client,
> > > @@ -208,6 +228,13 @@ static int __maybe_unused
> > > dw9714_vcm_suspend(struct device *dev)
> > >                         dev_err_once(dev, "%s I2C failure: %d",
> > > __func__, ret);
> > >                 usleep_range(DW9714_CTRL_DELAY_US,
> > > DW9714_CTRL_DELAY_US + 10);
> > >         }
> > > +
> > > +       if (dw9714_dev->vcc) {
> > > +               ret = regulator_disable(dw9714_dev->vcc);
> > > +               if (ret)
> > > +                       dev_err(dev, "Failed to disable vcc: %d\n",
> > > ret);
> > > +       }
> > > +
> > >         return 0;
> > >  }
> > >  
> > > @@ -224,6 +251,18 @@ static int  __maybe_unused
> > > dw9714_vcm_resume(struct device *dev)
> > >         struct dw9714_device *dw9714_dev = sd_to_dw9714_vcm(sd);
> > >         int ret, val;
> > >  
> > > +       if (pm_runtime_suspended(&client->dev))
> > 
> > Same for this one.
> > 
> > > +               return 0;
> > > +
> > > +       if (dw9714_dev->vcc) {
> > > +               ret = regulator_enable(dw9714_dev->vcc);
> > > +               if (ret) {
> > > +                       dev_err(dev, "Failed to enable vcc: %d\n",
> > > ret);
> > > +                       return ret;
> > > +               }
> > > +               usleep_range(1000, 2000);
> > > +       }
> > > +
> > >         for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
> > >              val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1;
> > >              val += DW9714_CTRL_STEPS) {
> > 
> 
> 

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] media: i2c: dw9714: add optional regulator support
  2021-11-26  9:01 [PATCH v2] media: i2c: dw9714: add optional regulator support Martin Kepplinger
  2021-11-26 10:44 ` Sakari Ailus
@ 2021-11-26 13:05 ` Mark Brown
  1 sibling, 0 replies; 5+ messages in thread
From: Mark Brown @ 2021-11-26 13:05 UTC (permalink / raw)
  To: Martin Kepplinger
  Cc: mchehab, sakari.ailus, kernel, linux-kernel, linux-media,
	linux-pm, Angus Ainslie

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

On Fri, Nov 26, 2021 at 10:01:07AM +0100, Martin Kepplinger wrote:

> +	dw9714_dev->vcc = devm_regulator_get_optional(&client->dev, "vcc");
> +	if (IS_ERR(dw9714_dev->vcc)) {
> +		dev_dbg(&client->dev, "No vcc regulator found: %ld\n",
> +			PTR_ERR(dw9714_dev->vcc));
> +		dw9714_dev->vcc = NULL;
> +	}

To repeat my original feedback: unless the supply can be physically
absent you should use regulator_get().   The _optional variants are only
for cases where the supply might be physically absent.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-11-26 13:07 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-26  9:01 [PATCH v2] media: i2c: dw9714: add optional regulator support Martin Kepplinger
2021-11-26 10:44 ` Sakari Ailus
2021-11-26 11:06   ` Martin Kepplinger
2021-11-26 12:28     ` Sakari Ailus
2021-11-26 13:05 ` Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).