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

[Alexey Charkov]

2010/10/21 Arnd Bergmann <arnd@xxxxxxxx>:
> On Wednesday 20 October 2010 22:55:33 Alexey Charkov wrote:
>
>> Please review and state whether this could be acceptable for a merge
>> to mainline in the coming 2.6.37 window. If possible, I would deeply
>> appreciate a merge to a relevant git tree for integration prior to
>> asking Linus to pull the changes. I could rebase the code if needed,
>> currently this is against Linus' master branch.
>
> As Greg mentioned for his review of the serial drivers, it's too late
> for 2.6.37. Fortunately that means you're really early for 2.6.38
> and getting it in there should be easy.
>
> The code does look very good though overall.
>

Good to hear that, thanks!

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

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).

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

>> +#ifdef CONFIG_VTWM_VERSION_WM8505
>> +extern struct platform_device vt8500_device_uart4;
>> +extern struct platform_device vt8500_device_uart5;
>> +#endif
>> +
>> +#ifdef CONFIG_FB_VT8500
>> +extern struct platform_device vt8500_device_lcdc;
>> +#endif
>> +#ifdef CONFIG_FB_WM8505
>> +extern struct platform_device vt8500_device_wm8505_fb;
>> +#endif
>> +
>> +#ifdef CONFIG_USB_EHCI_HCD
>> +extern struct platform_device vt8500_device_ehci;
>> +#endif
>> +
>> +#ifdef CONFIG_FB_WMT_GE_ROPS
>> +extern struct platform_device vt8500_device_ge_rops;
>> +#endif
>> +#ifdef CONFIG_HAVE_PWM
>> +extern struct platform_device vt8500_device_pwm;
>> +#endif
>> +#ifdef CONFIG_BACKLIGHT_PWM
>> +extern struct platform_device vt8500_device_pwmbl;
>> +#endif
>> +#ifdef CONFIG_RTC_DRV_VT8500
>> +extern struct platform_device vt8500_device_rtc;
>> +#endif
>
> The declarations never belong in an #ifdef, just make them
> unconditional.
>

Ok, got it.

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

> Â Â Â Â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/