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