| Message ID | 20231011143809.1108034-3-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fbdev/simplefb: Add missing simple-framebuffer features | expand |
Hi Thierry, kernel test robot noticed the following build errors: [auto build test ERROR on drm-misc/drm-misc-next] [also build test ERROR on drm-tip/drm-tip linus/master v6.6-rc5 next-20231012] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Thierry-Reding/fbdev-simplefb-Support-memory-region-property/20231011-223908 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20231011143809.1108034-3-thierry.reding%40gmail.com patch subject: [PATCH 2/2] fbdev/simplefb: Add support for generic power-domains config: sparc64-randconfig-001-20231012 (https://download.01.org/0day-ci/archive/20231012/202310121911.nusbPhr5-lkp@intel.com/config) compiler: sparc64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231012/202310121911.nusbPhr5-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202310121911.nusbPhr5-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/video/fbdev/simplefb.c: In function 'simplefb_probe': >> drivers/video/fbdev/simplefb.c:637:9: error: implicit declaration of function 'simplefb_detach_genpds'; did you mean 'simplefb_attach_genpd'? [-Werror=implicit-function-declaration] 637 | simplefb_detach_genpds(par); | ^~~~~~~~~~~~~~~~~~~~~~ | simplefb_attach_genpd cc1: some warnings being treated as errors vim +637 drivers/video/fbdev/simplefb.c 517 518 static int simplefb_probe(struct platform_device *pdev) 519 { 520 int ret; 521 struct simplefb_params params; 522 struct fb_info *info; 523 struct simplefb_par *par; 524 struct resource *res, *mem; 525 526 if (fb_get_options("simplefb", NULL)) 527 return -ENODEV; 528 529 ret = -ENODEV; 530 if (dev_get_platdata(&pdev->dev)) 531 ret = simplefb_parse_pd(pdev, ¶ms); 532 else if (pdev->dev.of_node) 533 ret = simplefb_parse_dt(pdev, ¶ms); 534 535 if (ret) 536 return ret; 537 538 if (params.memory.start == 0 && params.memory.end == 0) { 539 res = platform_get_resource(pdev, IORESOURCE_MEM, 0); 540 if (!res) { 541 dev_err(&pdev->dev, "No memory resource\n"); 542 return -EINVAL; 543 } 544 } else { 545 res = ¶ms.memory; 546 } 547 548 mem = request_mem_region(res->start, resource_size(res), "simplefb"); 549 if (!mem) { 550 /* 551 * We cannot make this fatal. Sometimes this comes from magic 552 * spaces our resource handlers simply don't know about. Use 553 * the I/O-memory resource as-is and try to map that instead. 554 */ 555 dev_warn(&pdev->dev, "simplefb: cannot reserve video memory at %pR\n", res); 556 mem = res; 557 } 558 559 info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev); 560 if (!info) { 561 ret = -ENOMEM; 562 goto error_release_mem_region; 563 } 564 platform_set_drvdata(pdev, info); 565 566 par = info->par; 567 568 info->fix = simplefb_fix; 569 info->fix.smem_start = mem->start; 570 info->fix.smem_len = resource_size(mem); 571 info->fix.line_length = params.stride; 572 573 info->var = simplefb_var; 574 info->var.xres = params.width; 575 info->var.yres = params.height; 576 info->var.xres_virtual = params.width; 577 info->var.yres_virtual = params.height; 578 info->var.bits_per_pixel = params.format->bits_per_pixel; 579 info->var.red = params.format->red; 580 info->var.green = params.format->green; 581 info->var.blue = params.format->blue; 582 info->var.transp = params.format->transp; 583 584 par->base = info->fix.smem_start; 585 par->size = info->fix.smem_len; 586 587 info->fbops = &simplefb_ops; 588 info->screen_base = ioremap_wc(info->fix.smem_start, 589 info->fix.smem_len); 590 if (!info->screen_base) { 591 ret = -ENOMEM; 592 goto error_fb_release; 593 } 594 info->pseudo_palette = par->palette; 595 596 ret = simplefb_clocks_get(par, pdev); 597 if (ret < 0) 598 goto error_unmap; 599 600 ret = simplefb_regulators_get(par, pdev); 601 if (ret < 0) 602 goto error_clocks; 603 604 ret = simplefb_attach_genpd(par, pdev); 605 if (ret < 0) 606 goto error_regulators; 607 608 simplefb_clocks_enable(par, pdev); 609 simplefb_regulators_enable(par, pdev); 610 611 dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes\n", 612 info->fix.smem_start, info->fix.smem_len); 613 dev_info(&pdev->dev, "format=%s, mode=%dx%dx%d, linelength=%d\n", 614 params.format->name, 615 info->var.xres, info->var.yres, 616 info->var.bits_per_pixel, info->fix.line_length); 617 618 if (mem != res) 619 par->mem = mem; /* release in clean-up handler */ 620 621 ret = devm_aperture_acquire_for_platform_device(pdev, par->base, par->size); 622 if (ret) { 623 dev_err(&pdev->dev, "Unable to acquire aperture: %d\n", ret); 624 goto error_genpds; 625 } 626 ret = register_framebuffer(info); 627 if (ret < 0) { 628 dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret); 629 goto error_genpds; 630 } 631 632 dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node); 633 634 return 0; 635 636 error_genpds: > 637 simplefb_detach_genpds(par); 638 error_regulators: 639 simplefb_regulators_destroy(par); 640 error_clocks: 641 simplefb_clocks_destroy(par); 642 error_unmap: 643 iounmap(info->screen_base); 644 error_fb_release: 645 framebuffer_release(info); 646 error_release_mem_region: 647 if (mem != res) 648 release_mem_region(mem->start, resource_size(mem)); 649 return ret; 650 } 651
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 >
On Wed, Oct 18, 2023 at 12:35 PM Robert Foss <rfoss@kernel.org> wrote: > > 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: > | ^~~~~~~~~~~~~~~~ > > Scratch that. After applying on an upstream build, it builds cleanly.
Hi, Thank you for your patches. On 10/11/23 16:38, Thierry Reding 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); > + } 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
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@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); > > + } > > 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. > > 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. Thanks, Thierry
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@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); >>> + } >> >> 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
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); error_clocks: