| Message ID | 20241219134940.15472-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: ov08x40: Various improvements | expand |
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@redhat.com> > --- > 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); > + > #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;
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@redhat.com> >> --- >> 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; >
On 20/12/2024 13:47, Hans de Goede wrote: >>> } >>> +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 Heh. That'd do it, works now. --- bod
Hi, On 20-Dec-24 2:51 PM, Bryan O'Donoghue wrote: > On 20/12/2024 13:47, Hans de Goede wrote: >>>> } >>>> +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 > > Heh. > > That'd do it, works now. Great, thank you for testing! This probably also explains why this series did not help to get the sensor powered on on some x86 HP laptops... I guess you are going to test the rest of the series and then provided a Tested-by? If yes then I'll wait with posting v2 until you're done testing. Regards, Hans
On 20/12/2024 13:59, Hans de Goede wrote: > I guess you are going to test the rest of the series and then provided > a Tested-by? > > If yes then I'll wait with posting v2 until you're done testing. I can give you Tested-by: for the series now + the change we discussed. --- bod
Hi, On 20-Dec-24 3:27 PM, Bryan O'Donoghue wrote: > On 20/12/2024 13:59, Hans de Goede wrote: >> I guess you are going to test the rest of the series and then provided >> a Tested-by? >> >> If yes then I'll wait with posting v2 until you're done testing. > > I can give you Tested-by: for the series now + the change we discussed. Yes please, then I can include it in the v2 posting. Regards, Hans
On 20/12/2024 14:29, Hans de Goede wrote: > Hi, > > On 20-Dec-24 3:27 PM, Bryan O'Donoghue wrote: >> On 20/12/2024 13:59, Hans de Goede wrote: >>> I guess you are going to test the rest of the series and then provided >>> a Tested-by? >>> >>> If yes then I'll wait with posting v2 until you're done testing. >> >> I can give you Tested-by: for the series now + the change we discussed. > > Yes please, then I can include it in the v2 posting. > > Regards, > > Hans > With the change discussed Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
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); + #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,
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@redhat.com> --- drivers/media/i2c/ov08x40.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)