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:

quoted

Hi Martin,

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

quoted

From: Angus Ainslie <redacted>

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

Signed-off-by: Angus Ainslie <redacted>
Signed-off-by: Martin Kepplinger <redacted>
---

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/ (local)



 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

quoted

quoted

+       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.

quoted

+               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.

quoted

quoted

+
        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.

quoted

+               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