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

From: Robert Foss <rfoss@kernel.org>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Hans de Goede <hdegoede@redhat.com>, Helge Deller <deller@gmx.de>,
	linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Jon Hunter <jonathanh@nvidia.com>
Subject: Re: [PATCH 2/2] fbdev/simplefb: Add support for generic power-domains
Date: Wed, 18 Oct 2023 12:35:06 +0200	[thread overview]
Message-ID: <CAN6tsi462nsJ1x_Z-Mcy+MuyaWhwvyFSuG0Ey=Uvqy_Vd1L0xA@mail.gmail.com> (raw)
In-Reply-To: <20231011143809.1108034-3-thierry.reding@gmail.com>

On Wed, Oct 11, 2023 at 4:38 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@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@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);
> +       }
> +}
> +
> +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;
>         }
>         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);
>  error_regulators:
>         simplefb_regulators_destroy(par);

I saw an error on a rhel9.3 kernel build, it may or may not be hit on
an upstream build.

drivers/video/fbdev/simplefb.c: In function 'simplefb_probe':
drivers/video/fbdev/simplefb.c:650:1: warning: label
'error_regulators' defined but not used [-Wunused-label]
  650 | error_regulators:
      | ^~~~~~~~~~~~~~~~

>  error_clocks:
> --
> 2.42.0
>