Hi,
On 20-Dec-24 1:32 PM, Bryan O'Donoghue wrote:
> On 19/12/2024 13:49, Hans de Goede wrote:
>> Commit df1ae2251a50 ("media: ov08x40: Add OF probe support") added support
>> for a reset GPIO, regulators and a clk provider controlled through new
>> ov08x40_power_off() and ov08x40_power_on() functions.
>>
>> But it missed adding a pm ops structure to call these functions on
>> runtime suspend/resume. Add the missing pm ops and only call
>> ov08x40_power_off() on remove() when not already runtime-suspended
>> to avoid unbalanced regulator / clock disable calls.
>>
>> Fixes: df1ae2251a50 ("media: ov08x40: Add OF probe support")
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> drivers/media/i2c/ov08x40.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov08x40.c b/drivers/media/i2c/ov08x40.c
>> index b9682264e2f5..aae923da1e86 100644
>> --- a/drivers/media/i2c/ov08x40.c
>> +++ b/drivers/media/i2c/ov08x40.c
>> @@ -2324,11 +2324,14 @@ static void ov08x40_remove(struct i2c_client *client)
>> ov08x40_free_controls(ov08x);
>> pm_runtime_disable(&client->dev);
>> + if (!pm_runtime_status_suspended(&client->dev))
>> + ov08x40_power_off(&client->dev);
>> pm_runtime_set_suspended(&client->dev);
>> -
>> - ov08x40_power_off(&client->dev);
>> }
>> +static DEFINE_RUNTIME_DEV_PM_OPS(ov08x40_pm_ops, ov08x40_power_on,
>> + ov08x40_power_off, NULL);
>> +
Ugh I have on/off swapped here, second argument of the macro is the suspend
function so that should be ov08x40_power_off. IOW this should be:
static DEFINE_RUNTIME_DEV_PM_OPS(ov08x40_pm_ops, ov08x40_power_off,
ov08x40_power_on, NULL);
Can you give this a try with that change?
Regards,
Hans
>> #ifdef CONFIG_ACPI
>> static const struct acpi_device_id ov08x40_acpi_ids[] = {
>> {"OVTI08F4"},
>> @@ -2349,6 +2352,7 @@ static struct i2c_driver ov08x40_i2c_driver = {
>> .name = "ov08x40",
>> .acpi_match_table = ACPI_PTR(ov08x40_acpi_ids),
>> .of_match_table = ov08x40_of_match,
>> + .pm = pm_sleep_ptr(&ov08x40_pm_ops),
>> },
>> .probe = ov08x40_probe,
>> .remove = ov08x40_remove,
>
> Here's a trace of what I'm finding on the qcom crd with this patch.
>
> [ 4.383434] ov08x40 1-0036: ov08x40_power_on
> [ 4.393299] ov08x40 1-0036: ov08x40_power_on
> [ 10.119144] ov08x40 1-0036: ov08x40_set_stream
> [ 10.127842] ov08x40 1-0036: ov08x40_set_stream do pm_runtime_resume_and_get
> [ 10.135067] ov08x40 1-0036: ov08x40_power_off
> [ 10.139608] ov08x40 1-0036: ov08x40_start_streaming
> [ 10.150832] ov08x40 1-0036: ov08x40_start_streaming failed to set powerup registers
> [ 10.165625] ov08x40 1-0036: ov08x40_power_on
>
>
> diff --git a/drivers/media/i2c/ov08x40.c b/drivers/media/i2c/ov08x40.c
> index 579b4aa5f041d..c1dead0ca0756 100644
> --- a/drivers/media/i2c/ov08x40.c
> +++ b/drivers/media/i2c/ov08x40.c
> @@ -1321,7 +1321,7 @@ static int ov08x40_power_on(struct device *dev)
> struct v4l2_subdev *sd = dev_get_drvdata(dev);
> struct ov08x40 *ov08x = to_ov08x40(sd);
> int ret;
> -
> +dev_info(dev, "%s\n", __func__);
> ret = clk_prepare_enable(ov08x->xvclk);
> if (ret < 0) {
> dev_err(dev, "failed to enable xvclk\n");
> @@ -1356,6 +1356,8 @@ static int ov08x40_power_off(struct device *dev)
> struct v4l2_subdev *sd = dev_get_drvdata(dev);
> struct ov08x40 *ov08x = to_ov08x40(sd);
>
> +dev_info(dev, "%s\n", __func__);
> +
> if (ov08x->reset_gpio)
> gpiod_set_value_cansleep(ov08x->reset_gpio, 1);
>
> @@ -1874,6 +1876,8 @@ static int ov08x40_start_streaming(struct ov08x40 *ov08x)
> const struct ov08x40_reg_list *reg_list;
> int ret, link_freq_index;
>
> +dev_info(&client->dev, "%s\n", __func__);
> +
> /* Get out of from software reset */
> ret = ov08x40_write_reg(ov08x, OV08X40_REG_SOFTWARE_RST,
> OV08X40_REG_VALUE_08BIT, OV08X40_SOFTWARE_RST);
> @@ -1967,9 +1971,11 @@ static int ov08x40_set_stream(struct v4l2_subdev *sd, int enable)
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> int ret = 0;
>
> +dev_info(&client->dev, "%s\n", __func__);
> mutex_lock(&ov08x->mutex);
>
> if (enable) {
> + dev_info(&client->dev, "%s do pm_runtime_resume_and_get\n", __func__);
> ret = pm_runtime_resume_and_get(&client->dev);
> if (ret < 0)
> goto err_unlock;
>
