Re: [PATCH 4/5] fbdev/simplefb: Request memory region in driver

[Javier Martinez Canillas <javierm@redhat.com>]

On 1/24/22 13:36, Thomas Zimmermann wrote:
> Requesting the framebuffer memory in simpledrm marks the memory
> range as busy. This used to be done by the firmware sysfb code,
> but the driver is the correct place.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/video/fbdev/simplefb.c | 59 ++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 57541887188b..84452028ecc9 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -66,16 +66,36 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
>  	return 0;
>  }
>  
> -struct simplefb_par;
> +struct simplefb_par {
> +	u32 palette[PSEUDO_PALETTE_SIZE];
> +#if defined CONFIG_OF && defined CONFIG_COMMON_CLK
> +	bool clks_enabled;
> +	unsigned int clk_count;
> +	struct clk **clks;
> +#endif
> +#if defined CONFIG_OF && defined CONFIG_REGULATOR
> +	bool regulators_enabled;
> +	u32 regulator_count;
> +	struct regulator **regulators;
> +#endif
> +	bool release_mem_region;

Maybe instead you could have:

	struct resource *mem; 

that would be more consistent with the fields for the other
resources like clocks, regulators, etc.

> +};
> +
>  static void simplefb_clocks_destroy(struct simplefb_par *par);
>  static void simplefb_regulators_destroy(struct simplefb_par *par);
>  
>  static void simplefb_destroy(struct fb_info *info)
>  {
> +	struct simplefb_par *par = info->par;
> +
>  	simplefb_regulators_destroy(info->par);
>  	simplefb_clocks_destroy(info->par);
>  	if (info->screen_base)
>  		iounmap(info->screen_base);
> +
> +	if (par->release_mem_region)
> +		release_mem_region(info->apertures->ranges[0].base,
> +				   info->apertures->ranges[0].size);

and here you could instead use the pointer to the resource:

	if (par->mem)
		release_mem_region(par->mem->start, resource_size(par->mem));

[snip]

>  static int simplefb_probe(struct platform_device *pdev)
>  {
> +	bool request_mem_succeeded = false;
>  	int ret;
>  	struct simplefb_params params;
>  	struct fb_info *info;
> @@ -436,9 +443,22 @@ static int simplefb_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	if (request_mem_region(mem->start, resource_size(mem), "simplefb")) {
> +		request_mem_succeeded = true;

and if you do the request_mem_region() after the struct fb_info allocation then
this could just be:

		par->mem = mem;

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat