Hi,
On 11/1/23 17:50, Thierry Reding wrote:
> On Thu, Oct 26, 2023 at 02:50:27PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> Thank you for your patches.
>>
>> On 10/11/23 16:38, Thierry Reding wrote:
>>> From: Thierry Reding <treding@xxxxxxxxxx>
>>>
>>> The simple-framebuffer device tree bindings document the power-domains
>>> property, so make sure that simplefb supports it. This ensures that the
>>> power domains remain enabled as long as simplefb is active.
>>>
>>> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
>>> ---
>>> drivers/video/fbdev/simplefb.c | 93 +++++++++++++++++++++++++++++++++-
>>> 1 file changed, 91 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>> index 18025f34fde7..e69fb0ad2d54 100644
>>> --- a/drivers/video/fbdev/simplefb.c
>>> +++ b/drivers/video/fbdev/simplefb.c
>>> @@ -25,6 +25,7 @@
>>> #include <linux/of_clk.h>
>>> #include <linux/of_platform.h>
>>> #include <linux/parser.h>
>>> +#include <linux/pm_domain.h>
>>> #include <linux/regulator/consumer.h>
>>>
>>> static const struct fb_fix_screeninfo simplefb_fix = {
>>> @@ -78,6 +79,11 @@ struct simplefb_par {
>>> unsigned int clk_count;
>>> struct clk **clks;
>>> #endif
>>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>>> + unsigned int num_genpds;
>>> + struct device **genpds;
>>> + struct device_link **genpd_links;
>>> +#endif
>>> #if defined CONFIG_OF && defined CONFIG_REGULATOR
>>> bool regulators_enabled;
>>> u32 regulator_count;
>>> @@ -432,6 +438,83 @@ static void simplefb_regulators_enable(struct simplefb_par *par,
>>> static void simplefb_regulators_destroy(struct simplefb_par *par) { }
>>> #endif
>>>
>>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>>> +static void simplefb_detach_genpds(void *res)
>>> +{
>>> + struct simplefb_par *par = res;
>>> + unsigned int i = par->num_genpds;
>>> +
>>> + if (par->num_genpds <= 1)
>>> + return;
>>> +
>>> + while (i--) {
>>> + if (par->genpd_links[i])
>>> + device_link_del(par->genpd_links[i]);
>>> +
>>> + if (!IS_ERR_OR_NULL(par->genpds[i]))
>>> + dev_pm_domain_detach(par->genpds[i], true);
>>> + }
>>
>> Using this i-- construct means that genpd at index 0 will
>> not be cleaned up.
>
> This is actually a common variant to clean up in reverse order. You'll
> find this used a lot in core code and so on. It has the advantage that
> you can use it to unwind (not the case here) because i will already be
> set to the right value, typically. It's also nice because it works for
> unsigned integers.
>
> Note that this uses the postfix decrement, so evaluation happens before
> the decrement and therefore the last iteration of the loop will run with
> i == 0. For unsigned integers this also means that after the loop the
> variable will actually have wrapped around, but that's usually not a
> problem since it isn't used after this point anymore.
Ah yes you are right, I messed the post-decrement part.
I got confused when I compaired this to the simpledrm code
which uses the other construct.
>
>>> static int simplefb_probe(struct platform_device *pdev)
>>> {
>>> int ret;
>>> @@ -518,6 +601,10 @@ static int simplefb_probe(struct platform_device *pdev)
>>> if (ret < 0)
>>> goto error_clocks;
>>>
>>> + ret = simplefb_attach_genpd(par, pdev);
>>> + if (ret < 0)
>>> + goto error_regulators;
>>> +
>>> simplefb_clocks_enable(par, pdev);
>>> simplefb_regulators_enable(par, pdev);
>>>
>>> @@ -534,18 +621,20 @@ static int simplefb_probe(struct platform_device *pdev)
>>> ret = devm_aperture_acquire_for_platform_device(pdev, par->base, par->size);
>>> if (ret) {
>>> dev_err(&pdev->dev, "Unable to acquire aperture: %d\n", ret);
>>> - goto error_regulators;
>>> + goto error_genpds;
>>
>> This is not necessary since simplefb_attach_genpd() ends with:
>>
>> devm_add_action_or_reset(dev, simplefb_detach_genpds, par)
>>
>> Which causes simplefb_detach_genpds() to run when probe() fails.
>
> Yes, you're right. I've removed all these explicit cleanup paths since
> they are not needed.
>
>>
>>> }
>>> ret = register_framebuffer(info);
>>> if (ret < 0) {
>>> dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
>>> - goto error_regulators;
>>> + goto error_genpds;
>>> }
>>>
>>> dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
>>>
>>> return 0;
>>>
>>> +error_genpds:
>>> + simplefb_detach_genpds(par);
>>
>> As the kernel test robot (LKP) already pointed out this is causing
>> compile errors when #if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>> evaluates as false.
>>
>> Since there is no simplefb_detach_genpds() stub in the #else, but as
>> mentioned above this is not necessary so just remove it.
>
> Yep, done.
Great, thank you.
Regards,
Hans
