Re: [PATCH] fb: split out framebuffer initialization from allocation

[Timur Tabi <timur@freescale.com>]

Bruno Prémont wrote:

> Exactly, thats why I would prefer to see all that initialization moved
> out of registrer_framebuffer() into a init_framebuffer().

I'm okay with that, but I'm not crazy about it.

> 
> For simplicity any driver that uses framebuffer_alloc() should not need
> to additionally call init_framebuffer() - framebuffer_alloc() should
> call it just before returning.

My patch does that.

> All those drivers that don't call framebuffer_alloc() would then have to
> call init_framebuffer().

Well, that would then mean that it's easier to move initialization *into* register_framebuffer().

> I think register_framebuffer() is a bit too late to do the initialisation
> of mutexes and other class state because driver cannot use the same code
> for HW setup before registration and after registration as at least the lock
> is in undefined state.

How many drivers actually do that?  The only thing that framebuffer_alloc() initializes in fb_info is the 'device' field, which is just a parameter passed into framebuffer_alloc(), so the driver already knows what that value is.  I can't find any examples of a driver doing what you're talking about.

It should be easy to modify any driver to stop using the fields of fb_info before register_framebuffer() is called.  In fact, I would say it's bad programming to use the fb_info object before the framebuffer is registered.

> In pseudo-code my though would be:
> 
> 
> // driver using their own memory allocation
> 
> struct driver_data {
>    ...
>    struct fb_info fb;
>    ...
> }
> 
> int driver_init() {
>    struct driver_data *data;
>    ...
>    memset(&data->fb, 0, sizeof(data->fb));
>    init_framebuffer(&data->fb);
>    // fill in driver's settings for fb
>    ...
>    register_framebuffer(&data->fb);
>    ...
> }
> 
> // driver using alloc_framebuffer()
> 
> struct driver_data {
>    ...
>    struct fb_info *fb;
>    ...
> }
> 
> int driver_init() {
>    struct driver_data *data;
>    ...
>    data->fb = alloc_framebuffer(0); // this implicitly calls init_framebuffer();
>    // fill in driver's settings for fb
>    ...
>    register_framebuffer(&data->fb);
>    ...
> }

This is pretty much what my patch does.  But the more I think about it, the more I'm in favor of moving all initialization into register_framebuffer().

-- 
Timur Tabi
Linux kernel developer at Freescale