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