Hans,
Thanks for your review.
On 6/13/23 5:20 PM, Hans de Goede wrote:
> Hi,
>
> Thank you for the patch.
>
> On 6/13/23 07:05, 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. INT3472 will register one
>> regulator 'avdd', one reset gpio and clock source for ov13b10.
>> Add 2 power interfaces that are registered as runtime PM callbacks.
>>
>> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>> Signed-off-by: Hao Yao <hao.yao@xxxxxxxxx>
>> ---
>> drivers/media/i2c/ov13b10.c | 100 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 98 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
>> index 96d3bd6ab3bd..be2c7d8c83ac 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 *avdd;
>> + struct gpio_desc *reset;
>> +
>> /* V4L2 Controls */
>> struct v4l2_ctrl *link_freq;
>> struct v4l2_ctrl *pixel_rate;
>> @@ -1051,6 +1059,52 @@ 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->reset)
>> + gpiod_set_value_cansleep(ov13b10->reset, 1);
>
> Just like the clk APIs the gpiod APIs will happily take a NULL
> pointer, so the "if (ov13b10->reset)" can be dropped
> here and also in ov13b10_power_on().
Correct, I ignored the VALIDATE_DESC_VOID(desc) by mistake.
Will fix in v3.
>
>
>> + if (ov13b10->avdd)
>> + regulator_disable(ov13b10->avdd);
>> +
>> + 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;
>> +
>> + ret = clk_prepare_enable(ov13b10->img_clk);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to enable imaging clock: %d", ret);
>> + return ret;
>> + }
>> +
>> + if (ov13b10->avdd) {
>> + ret = regulator_enable(ov13b10->avdd);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to enable avdd: %d", ret);
>> + clk_disable_unprepare(ov13b10->img_clk);
>> + return ret;
>> + }
>> + }
>> +
>> + if (ov13b10->reset) {
>> + 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 +1371,34 @@ static void ov13b10_free_controls(struct ov13b10 *ov13b)
>> mutex_destroy(&ov13b->mutex);
>> }
>>
>> +static int ov13b10_get_pm_resources(struct device *dev)
>> +{
>> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> + struct ov13b10 *ov13b = to_ov13b10(sd);
>> + int ret;
>> +
>> + ov13b->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> + if (IS_ERR(ov13b->reset))
>> + return dev_err_probe(dev, PTR_ERR(ov13b->reset),
>> + "failed to get reset gpio\n");
>> +
>> + ov13b->img_clk = devm_clk_get_optional(dev, NULL);
>> + if (IS_ERR(ov13b->img_clk))
>> + return dev_err_probe(dev, PTR_ERR(ov13b->img_clk),
>> + "failed to get imaging clock\n");
>> +
>> + ov13b->avdd = devm_regulator_get_optional(dev, "avdd");
>> + if (IS_ERR(ov13b->avdd)) {
>> + ret = PTR_ERR(ov13b->avdd);
>> + ov13b->avdd = NULL;
>> + if (ret != -ENODEV)
>> + return dev_err_probe(dev, ret,
>> + "failed to get avdd regulator\n");
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int ov13b10_check_hwcfg(struct device *dev)
>> {
>> struct v4l2_fwnode_endpoint bus_cfg = {
>> @@ -1407,13 +1489,23 @@ static int ov13b10_probe(struct i2c_client *client)
>> /* Initialize subdev */
>> v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops);
>>
>> + ret = ov13b10_get_pm_resources(&client->dev);
>> + if (ret)
>> + return ret;
>> +
>> 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");
>> + return ret;
>> + }
>> +
>> /* 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 +1514,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;
>> @@ -1462,6 +1554,9 @@ static int ov13b10_probe(struct i2c_client *client)
>> ov13b10_free_controls(ov13b);
>> dev_err(&client->dev, "%s failed:%d\n", __func__, ret);
>>
>> +error_power_off:
>> + ov13b10_power_off(&client->dev);
>> +
>> return ret;
>> }
>>
>> @@ -1479,6 +1574,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)
>
> You also need to add ov13b10_power_off / ov13b10_power_on calls
> to ov13b10_suspend and ov13b10_resume so that the sensor gets
> properly powered-off if it was not already runtime-suspend during
> system suspend.
Ack.
>
> Regards,
>
> Hans
>
>
>
>
>> };
>>
>> #ifdef CONFIG_ACPI
>
--
Best regards,
Bingbu Cao
