Hi Bingbu,
On Fri, May 26, 2023 at 11:21:20AM +0000, Cao, Bingbu wrote:
> Sakari,
>
> Thanks for your review.
>
> ------------------------------------------------------------------------
> BRs,
> Bingbu Cao
>
> >-----Original Message-----
> >From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >Sent: Friday, May 26, 2023 18:02
> >To: Cao, Bingbu <bingbu.cao@xxxxxxxxx>
> >Cc: linux-media@xxxxxxxxxxxxxxx; Kao, Arec <arec.kao@xxxxxxxxx>; Yao, Hao
> ><hao.yao@xxxxxxxxx>; bingbu.cao@xxxxxxxxxxxxxxx
> >Subject: Re: [PATCH 3/4] media: ov13b10: add PM control support based on
> >power resources
> >
> >Hi Bingbu,
> >
> >Thanks for the patch.
> >
> >On Fri, May 26, 2023 at 05:58:39PM +0800, bingbu.cao@xxxxxxxxx wrote:
> >> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> >>
> >> On ACPI based platforms, the ov13b10 camera sensor need to be powered
> >> up by acquire the PM resource from INT3472. On such platform, 1 GPIO
> >> can be used to enable AVDD and DOVDD, 1 GPIO to reset, we just have
> >> one power supply 'vdd' for camera. Add 2 power interfaces and also
> >> registered as runtime PM callbacks to support that.
> >>
> >> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> >> Signed-off-by: Hao Yao <hao.yao@xxxxxxxxx>
> >> ---
> >> drivers/media/i2c/ov13b10.c | 98
> >> ++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 96 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
> >> index 2d48c94659a4..b1faa89a3571 100644
> >> --- a/drivers/media/i2c/ov13b10.c
> >> +++ b/drivers/media/i2c/ov13b10.c
> >> @@ -2,6 +2,9 @@
> >> // Copyright (c) 2021 Intel Corporation.
> >>
> >> #include <linux/acpi.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/gpio/consumer.h>
> >> #include <linux/i2c.h>
> >> #include <linux/module.h>
> >> #include <linux/pm_runtime.h>
> >> @@ -573,6 +576,11 @@ struct ov13b10 {
> >> struct media_pad pad;
> >>
> >> struct v4l2_ctrl_handler ctrl_handler;
> >> +
> >> + struct clk *img_clk;
> >> + struct regulator *vdd;
> >> + struct gpio_desc *reset;
> >> +
> >> /* V4L2 Controls */
> >> struct v4l2_ctrl *link_freq;
> >> struct v4l2_ctrl *pixel_rate;
> >> @@ -1051,6 +1059,50 @@ static int ov13b10_identify_module(struct ov13b10
> >*ov13b)
> >> return 0;
> >> }
> >>
> >> +static int ov13b10_power_off(struct device *dev) {
> >> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >> + struct ov13b10 *ov13b10 = to_ov13b10(sd);
> >> +
> >> + if (!ov13b10->vdd || !ov13b10->reset || !ov13b10->img_clk)
> >> + return 0;
> >
> >This can be dropped.
> >
> >> +
> >> + gpiod_set_value_cansleep(ov13b10->reset, 1);
> >> + regulator_disable(ov13b10->vdd);
> >> + clk_disable_unprepare(ov13b10->img_clk);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int ov13b10_power_on(struct device *dev) {
> >> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >> + struct ov13b10 *ov13b10 = to_ov13b10(sd);
> >> + int ret;
> >> +
> >> + if (!ov13b10->vdd || !ov13b10->reset || !ov13b10->img_clk)
> >> + return 0;
> >
> >Instead I'd execute the sleep below if any of these is non-NULL. No need to
> >return here.
> >
> >> +
> >> + ret = clk_prepare_enable(ov13b10->img_clk);
> >> + if (ret < 0) {
> >> + dev_err(dev, "failed to enable imaging clock: %d", ret);
> >> + return ret;
> >> + }
> >> +
> >> + ret = regulator_enable(ov13b10->vdd);
> >> + if (ret < 0) {
> >> + dev_err(dev, "failed to enable vdd: %d", ret);
> >> + return ret;
> >> + }
> >> +
> >> + gpiod_set_value_cansleep(ov13b10->reset, 0);
> >> +
> >> + /* 5ms to wait ready after XSHUTDN assert */
> >> + usleep_range(5000, 5500);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int ov13b10_start_streaming(struct ov13b10 *ov13b) {
> >> struct i2c_client *client = v4l2_get_subdevdata(&ov13b->sd); @@
> >> -1317,6 +1369,37 @@ static void ov13b10_free_controls(struct ov13b10
> >*ov13b)
> >> mutex_destroy(&ov13b->mutex);
> >> }
> >>
> >> +static void ov13b10_get_pm_resources(struct device *dev) {
> >> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >> + struct ov13b10 *ov13b = to_ov13b10(sd);
> >> +
> >> + if (!is_acpi_node(dev_fwnode(dev)))
> >> + return;
> >
> >I'd omit this check. This would work on DT although bindings should still
> >be written. That may be left for a future excercise I think.
> >
> >> +
> >> + ov13b->reset = devm_gpiod_get_optional(dev, "reset",
> >> + GPIOD_OUT_LOW);
> >> + if (IS_ERR(ov13b->reset)) {
> >> + dev_dbg(dev, "failed to get reset gpio: %ld",
> >> + PTR_ERR(ov13b->reset));
> >> + ov13b->reset = NULL;
> >> + }
> >> +
> >> + ov13b->img_clk = devm_clk_get_optional(dev, NULL);
> >> + if (IS_ERR(ov13b->img_clk)) {
> >> + dev_dbg(dev, "failed to get imaging clock: %ld",
> >> + PTR_ERR(ov13b->img_clk));
> >> + ov13b->img_clk = NULL;
> >> + }
> >> +
> >> + ov13b->vdd = devm_regulator_get_optional(dev, "vdd");
> >> + if (IS_ERR(ov13b->vdd)) {
> >> + dev_dbg(dev, "failed to get vdd regulator: %ld",
> >> + PTR_ERR(ov13b->vdd));
> >> + ov13b->vdd = NULL;
> >
> >You should return the error code instead, in all three cases.
>
> On some platforms such as Chromebook, there is no INT3472 requirement.
> Here it ignores that case and return without errors, thus the power on
> and off callback would be NULL functions then.
Don't the _optional() variants of these functions return NULL if there's no
such thing for the device?
>
>
> >
> >> + }
> >> +}
> >> +
> >> static int ov13b10_check_hwcfg(struct device *dev) {
> >> struct v4l2_fwnode_endpoint bus_cfg = { @@ -1407,13 +1490,21 @@
> >> static int ov13b10_probe(struct i2c_client *client)
> >> /* Initialize subdev */
> >> v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops);
> >>
> >> + ov13b10_get_pm_resources(&client->dev);
> >> +
> >> full_power = acpi_dev_state_d0(&client->dev);
> >> if (full_power) {
> >> + ov13b10_power_on(&client->dev);
> >> + if (ret) {
> >> + dev_err(&client->dev, "failed to power on\n");
> >> + goto error_power_off;
> >
> >return ret;
>
> After power on, the caller has no idea which PM resource was
> set, so here call power off as a rollback.
Sure, but it needs to be done in ov13b10_power_on() instead. If there's an
error, some of the resources weren't enabled so you can't undo that here.
>
> >
> >See also below...
> >
> >> + }
> >> +
> >> /* Check module identity */
> >> ret = ov13b10_identify_module(ov13b);
> >> if (ret) {
> >> dev_err(&client->dev, "failed to find sensor: %d\n",
> >ret);
> >> - return ret;
> >> + goto error_power_off;
> >> }
> >> }
> >>
> >> @@ -1422,7 +1513,7 @@ static int ov13b10_probe(struct i2c_client
> >> *client)
> >>
> >> ret = ov13b10_init_controls(ov13b);
> >> if (ret)
> >> - return ret;
> >> + goto error_power_off;
> >>
> >> /* Initialize subdev */
> >> ov13b->sd.internal_ops = &ov13b10_internal_ops; @@ -1461,6 +1552,8
> >> @@ static int ov13b10_probe(struct i2c_client *client)
> >> error_handler_free:
> >> ov13b10_free_controls(ov13b);
> >> dev_err(&client->dev, "%s failed:%d\n", __func__, ret);
> >> +error_power_off:
> >> + ov13b10_power_off(&client->dev);
> >
> >This may only be called if ov13b10_power_on() was successfully called first.
> >
> >>
> >> return ret;
> >> }
> >> @@ -1479,6 +1572,7 @@ static void ov13b10_remove(struct i2c_client
> >> *client)
> >>
> >> static const struct dev_pm_ops ov13b10_pm_ops = {
> >> SET_SYSTEM_SLEEP_PM_OPS(ov13b10_suspend, ov13b10_resume)
> >> + SET_RUNTIME_PM_OPS(ov13b10_power_off, ov13b10_power_on, NULL)
> >> };
> >>
> >> #ifdef CONFIG_ACPI
> >>
--
Kind regards,
Sakari Ailus
