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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55E6E45B.5030500@ti.com>
Date:	Wed, 2 Sep 2015 14:58:19 +0300
From:	Tomi Valkeinen <tomi.valkeinen@...com>
To:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
CC:	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Jonathan Corbet <corbet@....net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	<linux-kernel@...r.kernel.org>, <linux-fbdev@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <devel@...verdev.osuosl.org>
Subject: Re: [PATCH v2 1/3] staging: sm7xxfb: move sm712fb out of staging



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


Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)