Re: [PATCH v2 1/3] staging: sm7xxfb: move sm712fb out of staging

[Tomi Valkeinen]

On 01/09/15 16:55, Sudip Mukherjee wrote:
> On Tue, Sep 01, 2015 at 04:27:24PM +0300, Tomi Valkeinen wrote:
>>
>>
>> On 18/07/15 07:08, Sudip Mukherjee wrote:
>>> Now since all cleanups are done and the code is ready to be merged lets
>>> move it out of staging into fbdev location.
>>
>> Have you considered writing a DRM driver for this? I'm not happy at all
>> adding new fbdev drivers, as the DRM framework is much better,
>> supported, and continuously improved. With fbdev you end up with things
>> like module parameters used to define video modes etc, which is just ugly.
> Yes, I am working on a DRM driver, but since these are all voluntary
> work it is taking time. And Greg has already merged it.
>>
>> Anyway, some comments below.
> Some replies inline and remaining I will fix and send patches to you.

Wouldn't the time be better spent on the DRM driver?

This driver will be obsolete immediately when there's a DRM driver for
this device, and then it'll be yet another obsoleted fbdev driver we
need to maintain.

>>> +static const struct vesa_mode vesa_mode_table[] = {
>>> +	{"0x301", 640,  480,  8},
>>> +	{"0x303", 800,  600,  8},
>>> +	{"0x305", 1024, 768,  8},
>>> +	{"0x307", 1280, 1024, 8},
>>> +
>>> +	{"0x311", 640,  480,  16},
>>> +	{"0x314", 800,  600,  16},
>>> +	{"0x317", 1024, 768,  16},
>>> +	{"0x31A", 1280, 1024, 16},
>>> +
>>> +	{"0x312", 640,  480,  24},
>>> +	{"0x315", 800,  600,  24},
>>> +	{"0x318", 1024, 768,  24},
>>> +	{"0x31B", 1280, 1024, 24},
>>> +};
>>
>> We have "vesa_modes" in include/linux/fb.h. What is the above table for?
> The resolutions that are supported along with the kernel boot parameter
> to point to the resolution to boot with.

Why does the user need to give such hex values? Why not modes according
to Documentation/fb/modedb.txt?

>>> +
>>> +/**********************************************************************
>>> +			 SM712 Mode table.
>>> + **********************************************************************/
>>> +static const struct modeinit vgamode[] = {
>>> +	{
> <snip>	
>>> +		{	/*  Init_CR90_CRA7 */
>>> +			0x55, 0xD9, 0x5D, 0xE1, 0x86, 0x1B, 0x8E, 0x26,
>>> +			0xDA, 0x8D, 0xDE, 0x94, 0x00, 0x00, 0x18, 0x00,
>>> +			0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x15, 0x03,
>>> +		},
>>> +	},
>>> +};
>>
>> What are these tables above for?
> Different register settings based on the display resolution. Do you want
> me to do anything with these vgamode table and the vesa_mode_table?

The vgamode table looks quite horrible. It's unmaintainable, and with a
quick look it seems to have lots of repetition. Large blocks of the data
for different modes are the same.

I don't know what the register there are, but I'd imagine you could
write generic functions like, say, "set_timings", which takes normal
linux videomode struct and writes those settings to the registers.

There should be no such bulk-write register tables in a proper driver,
except in some very special cases.

 Tomi

Attachment: signature.asc
Description: OpenPGP digital signature