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