Message-Id: <201010211401.53001.arnd@arndb.de>
Date:	Thu, 21 Oct 2010 14:01:52 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Alexey Charkov <alchark@...il.com>
Cc:	vt8500-wm8505-linux-kernel@...glegroups.com,
	linux-arm-kernel@...ts.infradead.org,
	Russell King <linux@....linux.org.uk>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's

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?
 
> 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.
 
> >> +#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.

> >> +#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.

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

Powered by blists - more mailing lists