Re: [PATCH 1/6] ARM: Add basic architecture support forVIA/WonderMedia 85xx SoC's

[Alexey Charkov]

2010/10/21 Arnd Bergmann <arnd@xxxxxxxx>:
> On Thursday 21 October 2010, Alexey Charkov wrote:
>
>> >> +
>> >> +choice
>> >> + Â Â prompt "LCD panel size"
>> >> + Â Â depends on (FB_VT8500 || FB_WM8505)
>> >> + Â Â default WMT_PANEL_800X480
>> >> +
>> >> +config WMT_PANEL_800X480
>> >> + Â Â bool "7-inch with 800x480 resolution"
>> >> + Â Â help
>> >> + Â Â Â These are found in most of the netbooks in generic cases, as
>> >> + Â Â Â well as in Eken M001 tablets and possibly elsewhere.
>> >> +
>> >> +config WMT_PANEL_800X600
>> >> + Â Â bool "8-inch with 800x600 resolution"
>> >> + Â Â help
>> >> + Â Â Â These are found in Eken M003 tablets and possibly elsewhere.
>> >> +
>> >> +config WMT_PANEL_1024X600
>> >> + Â Â bool "10-inch with 1024x600 resolution"
>> >> + Â Â help
>> >> + Â Â Â These are found in Eken M006 tablets and possibly elsewhere.
>> >> +
>> >> +endchoice
>> >
>> > This should really be a run-time or at least boot-time option. If you
>> > set the frame buffer size at compile time, I guess you can no longer
>> > boot on a machine that uses a different size.
>> >
>>
>> It could be, but then I'd have to parse kernel command line at the
>> map_io stage. Is that fine? If yes, I could rework it to e.g. accept a
>> default value via Kconfig and allow it to be overriden via a boot
>> argument.
>
> Parsing complex options in general is not ok, but something simpler
> probably is.
>
> Having a Kconfig selected default is probably a good idea. The most
> simple way to select this at boot time would be to have a list of
> possible resolutions and pass a table index.
>
> Would a __setup() call work for you?
>

Should probably be fine. Will it be allowable to accept something like
"panel=800x480" and strncmp it against a list of recognized values,
fall back to a Kconfig default on failure and printk the
possibilities? Just expecting an obscure table index would not be too
user-friendly, imho.

>> And due to the fact that the framebuffer size calculation is tied to
>> panel specification, it will boot in any case. The only problem that
>> one could encounter would be suboptimal display (for example,
>> offscreen pixmaps to become actually visible on screen if the panel is
>> taller
>> than expected, or some corruption in case it is wider).
>
> Another option might be to have a submenu with the possible resolutions
> you want to allow and size the frame buffer for the largest of those,
> but allow overriding the actual one at boot time.
>

In this case display parameters could be parsed in the driver itself,
but quite some memory will be over-allocated in extreme cases without
any way to claim it back after boot. Having only 128MB of RAM, is it
really better?

>> >> +#include <mach/vt8500.h>
>> >> +#include "devices.h"
>> >> +
>> >> +static struct platform_device *devices[] __initdata = {
>> >> + Â Â &vt8500_device_uart0,
>> >> +#ifdef CONFIG_FB_VT8500
>> >> + Â Â &vt8500_device_lcdc,
>> >> +#endif
>> >> +#ifdef CONFIG_USB_EHCI_HCD
>> >> + Â Â &vt8500_device_ehci,
>> >> +#endif
>> >> +#ifdef CONFIG_FB_WMT_GE_ROPS
>> >> + Â Â &vt8500_device_ge_rops,
>> >> +#endif
>> >> +#ifdef CONFIG_HAVE_PWM
>> >> + Â Â &vt8500_device_pwm,
>> >> +#endif
>> >> +#ifdef CONFIG_BACKLIGHT_PWM
>> >> + Â Â &vt8500_device_pwmbl,
>> >> +#endif
>> >> +#ifdef CONFIG_RTC_DRV_VT8500
>> >> + Â Â &vt8500_device_rtc,
>> >> +#endif
>> >> +};
>> >
>> > This doesn't work if the drivers are built as loadable modules, right?
>> > I wouldn't even make the definitions of the devices configuration dependent.
>> > The idea of the device model is that you describe what you have in one
>> > place and use that information to load the drivers for it.
>> >
>>
>> But with loadable modules those symbols should still be defined as 'm'
>> or something, shouldn't they? Anyway, I'll drop those conditions,
>> thanks for pointing out.
>
> If you configure an option as a module you get e.g. CONFIG_USB_EHCI_HCD_MODULE=1,
> but CONFIG_USB_EHCI_HCD remains unset. It would be possible to check for
> both of them, but IMHO it's cleaner to just leave the code in unconditionally.
>

Cleaned this up in the development repo, thanks. Only left #ifdef's
for the sections where respective register/interrupt definitions would
be unavailable due to compiling for a different SoC version, and
adjusted the conditions accordingly.

>> >> +#ifndef __ASM_ARM_ARCH_IO_H
>> >> +#define __ASM_ARM_ARCH_IO_H
>> >> +
>> >> +#define IO_SPACE_LIMIT 0xffffffff
>> >> +
>> >> +#define __io(a) Â Â Â Â Â Â Â__typesafe_io(a)
>> >> +#define __mem_pci(a) (a)
>> >> +
>> >> +#endif
>> >
>> >
>> > This won't work if you ever want to use the PCI on vt8505 with devices
>> > that have I/O space mapping.
>> >
>> > You need to define IO_SPACE_LIMIT to the size of your I/O space and
>> > make the __io macro offset the address with the start of that window.
>> >
>>
>> The problem is that there is no documentation available for the PCI
>> bus in these systems (if it is even implemented there).
>> Vendor-provided sources do not really clarify it either, which you
>> have commented about at:
>> http://groups.google.com/group/vt8500-wm8505-linux-kernel/msg/97bf44bc5ea5d46a?
>>
>> With no possibilities to test this (as I don't know any of these
>> devices to really use PCI with enumeration rather than just static
>> platform-like definitions as in the vendor's kernel), I just can't
>> imagine how this could be done any better.
>
> I can have a look again. It shouldn't be hard to do an almost correct
> implementation based on the source code we have, but I only own a wm8505
> based device, so I also have no way of testing and it would very likely
> have some subtle bugs.
>

Ok, figuring out better values for the macros would be great!

> Â Â Â ÂArnd
>

Thanks,
Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/