Re: [PATCH] Xilinx framebuffer device driver - 2nd version

["Grant Likely" <grant.likely@secretlab.ca>]

On 4/26/07, Andrei Konovalov <akonovalov@ru.mvista.com> wrote:
> Add support for the video controller IP block included into Xilinx ML300 and
> ML403 reference designs.
>
> Signed-off-by: Andrei Konovalov <akonovalov@ru.mvista.com>
> ---
>
> This patch relies on the "Patchset to establish sanity in Xilinx Virtex support" by Gran Likely to have
> the frame buffer device registered on the platform bus. Without this patchset one needs to fill in
> the struct platform_device and make sure platform_device_register() is called elsewhere.
> The DCR access has been added but not tested - my targets are configured in the "memory mapped IO" way.
> I would appreciate if those having the video controller registers accessible as DCRs
> test the DCR mode.
>
> This is the 2nd version that addresses what was pointed out by Arnd and Grant.
> Please find the interdiff against the 1st version below, and the whole patch attached to this message
> Comments are welcome.

First off; I'm an idiot.  The ml403 ref design *does* use the opb2dcr
bridge; and my design *does not* use DCR instructions; so I haven't
been able to test direct DCR access.  :-)  It might just be better to
drop the DCR stuff for now until it's accepted into mainline; or
someone is able to test it.

I've got it running on my custom board.   Seems to work well and it's
more featureful than my driver, so I'll migrate over to using yours.
The design that I'm using has a different color map from the ml300 ref
design.  blue is at offset 0 and red offset 16 (vs. blue-16 and red-0
on the ml300 ref design).   It's probably worthwhile to add those
parameters to the xilinxfb_platform_data structure.

> Would be nice to get this driver into mainline for the 2.6.22.

I certainly support getting it submitted.  Have you emailed it to the
linux-fb-devel list?

Can you split the driver and the platform device registration up into
2 patches?  It will probably make submission less painful; the device
registration patch can go through paulus, and the driver itself
through the linux-fbdev-devel list.

> ===================================================================
> --- linux-2.6.20.orig/drivers/video/Kconfig
> +++ linux-2.6.20/drivers/video/Kconfig
> @@ -1648,6 +1648,16 @@ config FB_XILINX_ROTATE
>         bool "Rotate display"
>         depends on FB_XILINX
>
> +config FB_XILINX_SCR_HEIGHT
> +       int "Screen height in mm"
> +       depends on FB_XILINX
> +       default 99
> +
> +config FB_XILINX_SCR_WIDTH
> +       int "Screen width in mm"
> +       depends on FB_XILINX
> +       default 132
> +

I'm not so fond of doing this via KCONFIG options; at least not at the
driver level.  Also, the board I'm using will have 2 of these cores,
each with different display dimesions.  For these parameters, I think
it makes more sense for the board setup code to override
virtex_device_fixup() and insert the correct values into the pdata
structure before virtex_init() registers the device.  Individual board
ports can add Kconfig setting for the dimensions if appropriate for
the system.

Cheers,
g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195