[PATCH 2/2] fbdev/simplefb: Add support for generic power-domains

[Hans de Goede]

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 at nvidia.com>
>>>
>>> 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 at nvidia.com>
>>> ---
>>>  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