[PATCH 0/2] fbdev/simplefb: Add missing simple-framebuffer features

[PATCH 0/2] fbdev/simplefb: Add missing simple-framebuffer features

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi,

This contains two patches that bring simplefb up to feature parity with
simpledrm. The patches add support for the "memory-region" property that
provides an alternative to the "reg" property to describe the memory
used for the framebuffer and allow attaching the simple-framebuffer
device to one or more generic power domains to make sure they aren't
turned off during the boot process and take down the display
configuration.

Thanks,
Thierry

Thierry Reding (2):
  fbdev/simplefb: Support memory-region property
  fbdev/simplefb: Add support for generic power-domains

 drivers/video/fbdev/simplefb.c | 128 +++++++++++++++++++++++++++++++--
 1 file changed, 121 insertions(+), 7 deletions(-)

-- 
2.42.0

[PATCH 1/2] fbdev/simplefb: Support memory-region property

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The simple-framebuffer bindings specify that the "memory-region"
property can be used as an alternative to the "reg" property to define
the framebuffer memory used by the display hardware. Implement support
for this in the simplefb driver.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/video/fbdev/simplefb.c | 35 +++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 62f99f6fccd3..18025f34fde7 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -21,6 +21,7 @@
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_clk.h>
 #include <linux/of_platform.h>
 #include <linux/parser.h>
@@ -121,12 +122,13 @@ struct simplefb_params {
 	u32 height;
 	u32 stride;
 	struct simplefb_format *format;
+	struct resource memory;
 };
 
 static int simplefb_parse_dt(struct platform_device *pdev,
 			   struct simplefb_params *params)
 {
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node, *mem;
 	int ret;
 	const char *format;
 	int i;
@@ -166,6 +168,23 @@ static int simplefb_parse_dt(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
+	mem = of_parse_phandle(np, "memory-region", 0);
+	if (mem) {
+		ret = of_address_to_resource(mem, 0, &params->memory);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to parse memory-region\n");
+			of_node_put(mem);
+			return ret;
+		}
+
+		if (of_property_present(np, "reg"))
+			dev_warn(&pdev->dev, "preferring \"memory-region\" over \"reg\" property\n");
+
+		of_node_put(mem);
+	} else {
+		memset(&params->memory, 0, sizeof(params->memory));
+	}
+
 	return 0;
 }
 
@@ -193,6 +212,8 @@ static int simplefb_parse_pd(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
+	memset(&params->memory, 0, sizeof(params->memory));
+
 	return 0;
 }
 
@@ -431,10 +452,14 @@ static int simplefb_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "No memory resource\n");
-		return -EINVAL;
+	if (params.memory.start == 0 && params.memory.end == 0) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!res) {
+			dev_err(&pdev->dev, "No memory resource\n");
+			return -EINVAL;
+		}
+	} else {
+		res = &params.memory;
 	}
 
 	mem = request_mem_region(res->start, resource_size(res), "simplefb");
-- 
2.42.0

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

[Thierry Reding]

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);
 error_clocks:
-- 
2.42.0

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

[kernel test robot]

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, &params);
   532		else if (pdev->dev.of_node)
   533			ret = simplefb_parse_dt(pdev, &params);
   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 = &params.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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Robert Foss]

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
>

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

[Robert Foss]

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.

Re: [PATCH 0/2] fbdev/simplefb: Add missing simple-framebuffer features

[Maxime Ripard]

Hi,

On Wed, Oct 11, 2023 at 04:38:07PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> This contains two patches that bring simplefb up to feature parity with
> simpledrm. The patches add support for the "memory-region" property that
> provides an alternative to the "reg" property to describe the memory
> used for the framebuffer and allow attaching the simple-framebuffer
> device to one or more generic power domains to make sure they aren't
> turned off during the boot process and take down the display
> configuration.

I just talked with Sima about it in the hallway at XDC. I'm fine with
those patches in principle, but it looks to me that simpledrm doesn't
have support for power domains and we'll want that :)

Thanks!
Maxime

Re: [PATCH 0/2] fbdev/simplefb: Add missing simple-framebuffer features

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

Hello Maxime,

> Hi,
>
> On Wed, Oct 11, 2023 at 04:38:07PM +0200, Thierry Reding wrote:
>> From: Thierry Reding <treding@nvidia.com>
>> This contains two patches that bring simplefb up to feature parity with
>> simpledrm. The patches add support for the "memory-region" property that
>> provides an alternative to the "reg" property to describe the memory
>> used for the framebuffer and allow attaching the simple-framebuffer
>> device to one or more generic power domains to make sure they aren't
>> turned off during the boot process and take down the display
>> configuration.
>
> I just talked with Sima about it in the hallway at XDC. I'm fine with
> those patches in principle, but it looks to me that simpledrm doesn't
> have support for power domains and we'll want that :)
>

It has support since commit 61df9ca23107 ("drm/simpledrm: Add support for
multiple "power-domains") AFAIK.

> Thanks!
> Maxime
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 0/2] fbdev/simplefb: Add missing simple-framebuffer features

[Maxime Ripard]

Hi,

On Thu, Oct 19, 2023 at 10:38:31AM +0200, Javier Martinez Canillas wrote:
> Maxime Ripard <mripard@kernel.org> writes:
> > On Wed, Oct 11, 2023 at 04:38:07PM +0200, Thierry Reding wrote:
> >> From: Thierry Reding <treding@nvidia.com>
> >> This contains two patches that bring simplefb up to feature parity with
> >> simpledrm. The patches add support for the "memory-region" property that
> >> provides an alternative to the "reg" property to describe the memory
> >> used for the framebuffer and allow attaching the simple-framebuffer
> >> device to one or more generic power domains to make sure they aren't
> >> turned off during the boot process and take down the display
> >> configuration.
> >
> > I just talked with Sima about it in the hallway at XDC. I'm fine with
> > those patches in principle, but it looks to me that simpledrm doesn't
> > have support for power domains and we'll want that :)
> >
> 
> It has support since commit 61df9ca23107 ("drm/simpledrm: Add support for
> multiple "power-domains") AFAIK.

Ah, my bad I didn't look in drm-misc-next

All good then, thanks!
Maxime

Re: [PATCH 1/2] fbdev/simplefb: Support memory-region property

[Hans de Goede]

Hi,

On 10/11/23 16:38, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The simple-framebuffer bindings specify that the "memory-region"
> property can be used as an alternative to the "reg" property to define
> the framebuffer memory used by the display hardware. Implement support
> for this in the simplefb driver.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/video/fbdev/simplefb.c | 35 +++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 62f99f6fccd3..18025f34fde7 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -21,6 +21,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/clk.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/of_clk.h>
>  #include <linux/of_platform.h>
>  #include <linux/parser.h>
> @@ -121,12 +122,13 @@ struct simplefb_params {
>  	u32 height;
>  	u32 stride;
>  	struct simplefb_format *format;
> +	struct resource memory;
>  };
>  
>  static int simplefb_parse_dt(struct platform_device *pdev,
>  			   struct simplefb_params *params)
>  {
> -	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *np = pdev->dev.of_node, *mem;
>  	int ret;
>  	const char *format;
>  	int i;
> @@ -166,6 +168,23 @@ static int simplefb_parse_dt(struct platform_device *pdev,
>  		return -EINVAL;
>  	}
>  
> +	mem = of_parse_phandle(np, "memory-region", 0);
> +	if (mem) {
> +		ret = of_address_to_resource(mem, 0, &params->memory);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "failed to parse memory-region\n");
> +			of_node_put(mem);
> +			return ret;
> +		}
> +
> +		if (of_property_present(np, "reg"))
> +			dev_warn(&pdev->dev, "preferring \"memory-region\" over \"reg\" property\n");
> +
> +		of_node_put(mem);
> +	} else {
> +		memset(&params->memory, 0, sizeof(params->memory));
> +	}
> +
>  	return 0;
>  }
>  
> @@ -193,6 +212,8 @@ static int simplefb_parse_pd(struct platform_device *pdev,
>  		return -EINVAL;
>  	}
>  
> +	memset(&params->memory, 0, sizeof(params->memory));
> +
>  	return 0;
>  }
>  
> @@ -431,10 +452,14 @@ static int simplefb_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(&pdev->dev, "No memory resource\n");
> -		return -EINVAL;
> +	if (params.memory.start == 0 && params.memory.end == 0) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!res) {
> +			dev_err(&pdev->dev, "No memory resource\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		res = &params.memory;
>  	}
>  
>  	mem = request_mem_region(res->start, resource_size(res), "simplefb");

Re: [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@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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4716 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [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@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