Re: [PATCH] fbdev: q40fb: request memory region

[David Hunter <david.hunter.linux@gmail.com>]

On 11/18/25 04:56, Sukrut Heroorkar wrote:
> The q40fb driver uses a fixed physical address but never reserves
> the corresponding I/O region. Reserve the range  as suggested in
> Documentation/gpu/todo.rst ("Request memory regions in all fbdev drivers").
> 
> No functional change beyond claming the resource. This change is compile
> tested only.

Reserving memory is a significant "functional" change, so you should not
put "No functional change...". I have noticed that in the mentorship
program, mentees might say this often times when they have not done
testing.

Thank you for describing that you did a compile test, but I believe that
more testing should be done before this patch is accepted.

As a result, if you are unable to test this device, I believe that an
RFT tag should be used. Also, the testing information goes below the
"---". This puts it in the change log and would make it so that if a
patch is accepted, everything below the change log is not put in the
commit message.

> 
> Signed-off-by: Sukrut Heroorkar <hsukrut3@gmail.com>
> ---
>  drivers/video/fbdev/q40fb.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/video/fbdev/q40fb.c b/drivers/video/fbdev/q40fb.c
> index 1ff8fa176124..935260326c6f 100644
> --- a/drivers/video/fbdev/q40fb.c
> +++ b/drivers/video/fbdev/q40fb.c
> @@ -101,6 +101,12 @@ static int q40fb_probe(struct platform_device *dev)
>  	info->par = NULL;
>  	info->screen_base = (char *) q40fb_fix.smem_start;
>  
> +	if (!request_mem_region(q40fb_fix.smem_start, q40fb_fix.smem_len,
> +				"q40fb")) {
> +		dev_err(&dev->dev, "cannot reserve video memory at 0x%lx\n",
> +			q40fb_fix.smem_start);
> +	}
> +

Is this correct? It seems to me that in the case of an error, all you
are doing is simply logging the error and proceeding. Would this cause
the device to continue to try to use space that it was not able to
reserve? I do not have experience with this device or the driver, but
that does not seem correct to me.

>  	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0) {
>  		framebuffer_release(info);
>  		return -ENOMEM;
> @@ -144,6 +150,7 @@ static int __init q40fb_init(void)
>  		if (ret)
>  			platform_driver_unregister(&q40fb_driver);
>  	}
> +
>  	return ret;
>  }
>