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
