Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 25 February 2016 15:04:52 Linus Walleij wrote:
> 
> I add support for doing this for the Integrator and RealView in
> the patch set, by grabbing a handle to the system controller
> where they have a few "misc registers". However if you look at
> it:
> 
> static void integrator_clcd_enable(struct clcd_fb *fb)
> {
>         struct fb_var_screeninfo *var = &fb->fb.var;
>         u32 val;
> 
>         dev_info(&fb->dev->dev, "enable Integrator CLCD connectors\n");
> 
>         val = INTEGRATOR_CLCD_LCD_STATIC1 | INTEGRATOR_CLCD_LCD_STATIC2 |
>                 INTEGRATOR_CLCD_LCD0_EN | INTEGRATOR_CLCD_LCD1_EN;
>         if (var->bits_per_pixel <= 8 ||
>             (var->bits_per_pixel = 16 && var->green.length = 5))
>                 /* Pseudocolor, RGB555, BGR555 */
>                 val |= INTEGRATOR_CLCD_LCDMUX_VGA555;
>         else if (fb->fb.var.bits_per_pixel <= 16)
>                 /* truecolor RGB565 */
>                 val |= INTEGRATOR_CLCD_LCDMUX_VGA565;
>         else
>                 val = 0; /* no idea for this, don't trust the docs */
> 
>         regmap_update_bits(versatile_syscon_map,
>                            INTEGRATOR_HDR_CTRL_OFFSET,
>                            0,
>                            INTEGRATOR_CLCD_MASK);
> }
> 
> This is stuff that is so closely tied in to the fbdev driver that even
> if it is SoC-specific (and reside in arch/arm/mach-integrator etc
> today) it would be hard to argument that it should not be part
> of the fbdev driver: what it does is connect the lines out of the
> CLCD block to the physical VGA encode chip in different ways
> depending on how the pixels were set up.

I think the nicest approach here would be to make this a layered
driver, where you have a separate platform_driver instance
that contains all the versatile specific add-ons, and this calls
into the common driver that handles everything that is not specific
to versatile.

It may not be worth investing much into a rework to get there
though, so simply putting it all into one module sounds like
a reasonable compromise.

	Arnd