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

[Hans de Goede]

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.

I think it would be best to instead use the same construct
as the simpledrm version of this which makes i signed
(and does not initialize it) and then does:


	for (i = sdev->pwr_dom_count - 1; i >= 0; i--) { .. }


> +}
> +
> +static int simplefb_attach_genpd(struct simplefb_par *par,
> +				 struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	unsigned int i;
> +	int err;
> +
> +	par->num_genpds = of_count_phandle_with_args(dev->of_node,
> +						     "power-domains",
> +						     "#power-domain-cells");
> +	/*
> +	 * Single power-domain devices are handled by the driver core, so
> +	 * nothing to do here.
> +	 */
> +	if (par->num_genpds <= 1)
> +		return 0;
> +
> +	par->genpds = devm_kcalloc(dev, par->num_genpds, sizeof(*par->genpds),
> +				   GFP_KERNEL);
> +	if (!par->genpds)
> +		return -ENOMEM;
> +
> +	par->genpd_links = devm_kcalloc(dev, par->num_genpds,
> +					sizeof(*par->genpd_links),
> +					GFP_KERNEL);
> +	if (!par->genpd_links)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < par->num_genpds; i++) {
> +		par->genpds[i] = dev_pm_domain_attach_by_id(dev, i);
> +		if (IS_ERR(par->genpds[i])) {
> +			err = PTR_ERR(par->genpds[i]);
> +			if (err == -EPROBE_DEFER) {
> +				simplefb_detach_genpds(par);
> +				return err;
> +			}
> +
> +			dev_warn(dev, "failed to attach domain %u: %d\n", i, err);
> +			continue;
> +		}
> +
> +		par->genpd_links[i] = device_link_add(dev, par->genpds[i],
> +						      DL_FLAG_STATELESS |
> +						      DL_FLAG_PM_RUNTIME |
> +						      DL_FLAG_RPM_ACTIVE);
> +		if (!par->genpd_links[i])
> +			dev_warn(dev, "failed to link power-domain %u\n", i);
> +	}
> +
> +	return devm_add_action_or_reset(dev, simplefb_detach_genpds, par);
> +}
> +#else
> +static int simplefb_attach_genpd(struct simplefb_par *par,
> +				 struct platform_device *pdev)
> +{
> +	return 0;
> +}
> +#endif
> +
>  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.     

>  	}
>  	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.

>  error_regulators:
>  	simplefb_regulators_destroy(par);
>  error_clocks:

Regards,

Hans