| Message ID | 20220921081536.2551789-1-hidenorik@chromium.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] media: ov8856: Add runtime PM callbacks | expand |
Hi Hidenori, On Wed, Sep 21, 2022 at 05:15:35PM +0900, Hidenori Kobayashi wrote: > There were no runtime PM callbacks registered, leaving regulators being > enabled while the device is suspended on DT systems. Add callbacks that > call existing power controlling functions to turn them off/on. > > To simplify the code with this addition, change the argument of the > power controlling functions. > > Signed-off-by: Hidenori Kobayashi <hidenorik@chromium.org> > --- > V1 -> V2: Change argument of power controlling functions > --- > drivers/media/i2c/ov8856.c | 41 ++++++++++++++++++++++++++------------ > 1 file changed, 28 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c > index a9728afc81d4..188699455fdf 100644 > --- a/drivers/media/i2c/ov8856.c > +++ b/drivers/media/i2c/ov8856.c > @@ -2110,17 +2110,18 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable) > return ret; > } > > -static int __ov8856_power_on(struct ov8856 *ov8856) > +static int __ov8856_power_on(struct device *dev) > { > - struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > + struct ov8856 *ov8856 = to_ov8856(sd); > int ret; > > - if (is_acpi_node(dev_fwnode(&client->dev))) > + if (is_acpi_node(dev_fwnode(dev))) > return 0; > > ret = clk_prepare_enable(ov8856->xvclk); > if (ret < 0) { > - dev_err(&client->dev, "failed to enable xvclk\n"); > + dev_err(dev, "failed to enable xvclk\n"); > return ret; > } > > @@ -2132,7 +2133,7 @@ static int __ov8856_power_on(struct ov8856 *ov8856) > ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > ov8856->supplies); > if (ret < 0) { > - dev_err(&client->dev, "failed to enable regulators\n"); > + dev_err(dev, "failed to enable regulators\n"); > goto disable_clk; > } > > @@ -2148,11 +2149,12 @@ static int __ov8856_power_on(struct ov8856 *ov8856) > return ret; > } > > -static void __ov8856_power_off(struct ov8856 *ov8856) > +static void __ov8856_power_off(struct device *dev) > { > - struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > + struct ov8856 *ov8856 = to_ov8856(sd); > > - if (is_acpi_node(dev_fwnode(&client->dev))) > + if (is_acpi_node(dev_fwnode(dev))) > return; > > gpiod_set_value_cansleep(ov8856->reset_gpio, 1); > @@ -2170,7 +2172,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev) > if (ov8856->streaming) > ov8856_stop_streaming(ov8856); > > - __ov8856_power_off(ov8856); > + __ov8856_power_off(dev); > mutex_unlock(&ov8856->mutex); > > return 0; > @@ -2184,7 +2186,7 @@ static int __maybe_unused ov8856_resume(struct device *dev) > > mutex_lock(&ov8856->mutex); > > - __ov8856_power_on(ov8856); > + __ov8856_power_on(dev); > if (ov8856->streaming) { > ret = ov8856_start_streaming(ov8856); > if (ret) { > @@ -2200,6 +2202,18 @@ static int __maybe_unused ov8856_resume(struct device *dev) > return 0; > } > > +static int __maybe_unused ov8856_runtime_suspend(struct device *dev) > +{ > + __ov8856_power_off(dev); > + > + return 0; > +} > + > +static int __maybe_unused ov8856_runtime_resume(struct device *dev) > +{ > + return __ov8856_power_on(dev); > +} These two functions are redundant now, you can call __ov8856_power_on / __ov8856_power_off directly. The return type of __ov8856_power_off needs to be changed to int. You could also remove the underscores from the names at the same time. > + > static int ov8856_set_format(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_format *fmt) > @@ -2451,7 +2465,7 @@ static int ov8856_remove(struct i2c_client *client) > pm_runtime_disable(&client->dev); > mutex_destroy(&ov8856->mutex); > > - __ov8856_power_off(ov8856); > + __ov8856_power_off(&client->dev); > > return 0; > } > @@ -2477,7 +2491,7 @@ static int ov8856_probe(struct i2c_client *client) > > full_power = acpi_dev_state_d0(&client->dev); > if (full_power) { > - ret = __ov8856_power_on(ov8856); > + ret = __ov8856_power_on(&client->dev); > if (ret) { > dev_err(&client->dev, "failed to power on\n"); > return ret; > @@ -2533,13 +2547,14 @@ static int ov8856_probe(struct i2c_client *client) > mutex_destroy(&ov8856->mutex); > > probe_power_off: > - __ov8856_power_off(ov8856); > + __ov8856_power_off(&client->dev); > > return ret; > } > > static const struct dev_pm_ops ov8856_pm_ops = { > SET_SYSTEM_SLEEP_PM_OPS(ov8856_suspend, ov8856_resume) > + SET_RUNTIME_PM_OPS(ov8856_runtime_suspend, ov8856_runtime_resume, NULL) > }; > > #ifdef CONFIG_ACPI
Hi Sakari, On Wed, Sep 21, 2022 at 11:20:12AM +0300, Sakari Ailus wrote: > Hi Hidenori, > > On Wed, Sep 21, 2022 at 05:15:35PM +0900, Hidenori Kobayashi wrote: > > > > +static int __maybe_unused ov8856_runtime_suspend(struct device *dev) > > +{ > > + __ov8856_power_off(dev); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused ov8856_runtime_resume(struct device *dev) > > +{ > > + return __ov8856_power_on(dev); > > +} > > These two functions are redundant now, you can call __ov8856_power_on / > __ov8856_power_off directly. The return type of __ov8856_power_off needs to > be changed to int. You could also remove the underscores from the names at > the same time. Will fix it as above. I just wasn't sure about the policy. Thanks! Best regards, Hidenori
diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c index a9728afc81d4..188699455fdf 100644 --- a/drivers/media/i2c/ov8856.c +++ b/drivers/media/i2c/ov8856.c @@ -2110,17 +2110,18 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable) return ret; } -static int __ov8856_power_on(struct ov8856 *ov8856) +static int __ov8856_power_on(struct device *dev) { - struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct ov8856 *ov8856 = to_ov8856(sd); int ret; - if (is_acpi_node(dev_fwnode(&client->dev))) + if (is_acpi_node(dev_fwnode(dev))) return 0; ret = clk_prepare_enable(ov8856->xvclk); if (ret < 0) { - dev_err(&client->dev, "failed to enable xvclk\n"); + dev_err(dev, "failed to enable xvclk\n"); return ret; } @@ -2132,7 +2133,7 @@ static int __ov8856_power_on(struct ov8856 *ov8856) ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), ov8856->supplies); if (ret < 0) { - dev_err(&client->dev, "failed to enable regulators\n"); + dev_err(dev, "failed to enable regulators\n"); goto disable_clk; } @@ -2148,11 +2149,12 @@ static int __ov8856_power_on(struct ov8856 *ov8856) return ret; } -static void __ov8856_power_off(struct ov8856 *ov8856) +static void __ov8856_power_off(struct device *dev) { - struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct ov8856 *ov8856 = to_ov8856(sd); - if (is_acpi_node(dev_fwnode(&client->dev))) + if (is_acpi_node(dev_fwnode(dev))) return; gpiod_set_value_cansleep(ov8856->reset_gpio, 1); @@ -2170,7 +2172,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev) if (ov8856->streaming) ov8856_stop_streaming(ov8856); - __ov8856_power_off(ov8856); + __ov8856_power_off(dev); mutex_unlock(&ov8856->mutex); return 0; @@ -2184,7 +2186,7 @@ static int __maybe_unused ov8856_resume(struct device *dev) mutex_lock(&ov8856->mutex); - __ov8856_power_on(ov8856); + __ov8856_power_on(dev); if (ov8856->streaming) { ret = ov8856_start_streaming(ov8856); if (ret) { @@ -2200,6 +2202,18 @@ static int __maybe_unused ov8856_resume(struct device *dev) return 0; } +static int __maybe_unused ov8856_runtime_suspend(struct device *dev) +{ + __ov8856_power_off(dev); + + return 0; +} + +static int __maybe_unused ov8856_runtime_resume(struct device *dev) +{ + return __ov8856_power_on(dev); +} + static int ov8856_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state, struct v4l2_subdev_format *fmt) @@ -2451,7 +2465,7 @@ static int ov8856_remove(struct i2c_client *client) pm_runtime_disable(&client->dev); mutex_destroy(&ov8856->mutex); - __ov8856_power_off(ov8856); + __ov8856_power_off(&client->dev); return 0; } @@ -2477,7 +2491,7 @@ static int ov8856_probe(struct i2c_client *client) full_power = acpi_dev_state_d0(&client->dev); if (full_power) { - ret = __ov8856_power_on(ov8856); + ret = __ov8856_power_on(&client->dev); if (ret) { dev_err(&client->dev, "failed to power on\n"); return ret; @@ -2533,13 +2547,14 @@ static int ov8856_probe(struct i2c_client *client) mutex_destroy(&ov8856->mutex); probe_power_off: - __ov8856_power_off(ov8856); + __ov8856_power_off(&client->dev); return ret; } static const struct dev_pm_ops ov8856_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(ov8856_suspend, ov8856_resume) + SET_RUNTIME_PM_OPS(ov8856_runtime_suspend, ov8856_runtime_resume, NULL) }; #ifdef CONFIG_ACPI
There were no runtime PM callbacks registered, leaving regulators being enabled while the device is suspended on DT systems. Add callbacks that call existing power controlling functions to turn them off/on. To simplify the code with this addition, change the argument of the power controlling functions. Signed-off-by: Hidenori Kobayashi <hidenorik@chromium.org> --- V1 -> V2: Change argument of power controlling functions --- drivers/media/i2c/ov8856.c | 41 ++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 13 deletions(-)