Re: [PATCH] arm/pxa25x: HP iPAQ h5000

On Tue, Sep 16, 2008 at 10:45 PM, Milan Plzik <milan.plzik_at_gmail.com> wrote:
> From: Anton Vorontsov <cbouatmailru_at_gmail.com>
>

Ah, I almost ignore this one. Next time please include me in the CC list
so that mail gets into my concerned folder.

One big concern is about the naming of GPIO, can we just have
a consistent GPIO naming scheme across all platforms?
I'd suggest to use

<PLATFORM>_GPIO_<PURPOSE>

as all other PXA platforms are doing now.

> This patch adds HP iPAQ h5000's (h5400, h5500) basic definitions.
>
> Kernel will able to boot, work via serial console, mount filesystems
> placed on flashes and run USB gadgets (g_ether by default).
>
> Other device drivers (frame buffer, LCD, touchscreen, backlight,
> bluetooth, w1/battery, ...) are depend on SAMCOP and MediaQ
> SoCs/MFDs, drivers to which will be submitted too, after massive
> cleanups.
>
> This machine will be used as "real user" for these new drivers.
>
> Signed-off-by: Anton Vorontsov <cbouatmailru_at_gmail.com>
>
> Signed-off-by: Milan Plzik <milan.plzik_at_gmail.com>
> ---
>
> arch/arm/configs/h5000_defconfig | 996 ++++++++++++++++++++++++++++++++
> arch/arm/mach-pxa/Kconfig | 4
> arch/arm/mach-pxa/Makefile | 1
> arch/arm/mach-pxa/h5000.c | 258 ++++++++
> arch/arm/mach-pxa/include/mach/h5000.h | 111 ++++
> 5 files changed, 1370 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/configs/h5000_defconfig
> create mode 100644 arch/arm/mach-pxa/h5000.c
> create mode 100644 arch/arm/mach-pxa/include/mach/h5000.h
>

> +static unsigned long h5000_pin_config[] __initdata = {
> + /* Crystal and Clock Signals */
> + GPIO12_32KHz,
> +
> + /* SDRAM and Static Memory I/O Signals */
> + GPIO15_nCS_1,
> + GPIO78_nCS_2,
> + GPIO79_nCS_3,
> + GPIO80_nCS_4,
> +
> + /* FFUART */
> + GPIO34_FFUART_RXD,
> + GPIO35_FFUART_CTS,
> + GPIO36_FFUART_DCD,
> + GPIO37_FFUART_DSR,
> + GPIO38_FFUART_RI,
> + GPIO39_FFUART_TXD,
> + GPIO40_FFUART_DTR,
> + GPIO41_FFUART_RTS,
> +
> + /* BTUART */
> + GPIO42_BTUART_RXD,
> + GPIO43_BTUART_TXD,
> + GPIO44_BTUART_CTS,
> + GPIO45_BTUART_RTS,
> +
> + /* SSP1 */
> + GPIO23_SSP1_SCLK,
> + GPIO25_SSP1_TXD,
> + GPIO26_SSP1_RXD,
> + GPIO_NR_H5000_OPT_SPI_CS_N | MFP_DIR_OUT,
> +
> + /* H5000 GPIO */
> + GPIO_NR_H5000_POWER_BUTTON | MFP_DIR_IN,
> + GPIO_NR_H5000_RESET_BUTTON_N | MFP_DIR_IN,
> + GPIO_NR_H5000_OPT_INT | MFP_DIR_IN,
> + GPIO_NR_H5000_BACKUP_POWER | MFP_DIR_IN, /* XXX should be an output? */
> + GPIO_NR_H5000_ACTION_BUTTON | MFP_DIR_IN,
> + GPIO_NR_H5000_COM_DCD_SOMETHING | MFP_DIR_IN,
> + GPIO_NR_H5000_RESET_BUTTON_AGAIN_N | MFP_DIR_IN,
> + GPIO_NR_H5000_RSO_N | MFP_DIR_IN, /* BATT_FAULT */
> + GPIO_NR_H5000_ASIC_INT_N | MFP_DIR_IN,
> + GPIO_NR_H5000_BT_ENV_0 | MFP_DIR_OUT,
> + GPIO_NR_H5000_BT_ENV_1 | MFP_DIR_OUT,
> + GPIO_NR_H5000_BT_WU | MFP_DIR_IN,
> + GPIO_NR_H5000_IRDA_SD | MFP_DIR_OUT,
> + GPIO_NR_H5000_POWER_SD_N | MFP_DIR_OUT, /* XXX not really active low? */
> + GPIO_NR_H5000_POWER_RS232_N | MFP_DIR_OUT | MFP_LPM_DRIVE_HIGH,
> + GPIO_NR_H5000_POWER_ACCEL_N | MFP_DIR_OUT | MFP_LPM_DRIVE_HIGH,
> + GPIO_NR_H5000_OPT_NVRAM | MFP_DIR_OUT,
> + GPIO_NR_H5000_CHG_EN | MFP_DIR_OUT,
> + GPIO_NR_H5000_USB_PULLUP | MFP_DIR_OUT,
> + GPIO_NR_H5000_BT_2V8_N | MFP_DIR_OUT | MFP_LPM_DRIVE_HIGH,
> + GPIO_NR_H5000_EXT_CHG_RATE | MFP_DIR_OUT,
> + GPIO_NR_H5000_CIR_RESET | MFP_DIR_OUT,
> + GPIO_NR_H5000_POWER_LIGHT_SENSOR_N | MFP_DIR_OUT | MFP_LPM_DRIVE_HIGH,
> + GPIO_NR_H5000_BT_M_RESET | MFP_DIR_OUT,
> + GPIO_NR_H5000_STD_CHG_RATE | MFP_DIR_OUT,
> + GPIO_NR_H5000_SD_WP_N | MFP_DIR_IN, /* XXX docs say output */
> + GPIO_NR_H5000_MOTOR_ON_N | MFP_DIR_OUT | MFP_LPM_DRIVE_HIGH,
> + GPIO_NR_H5000_HEADPHONE_DETECT | MFP_DIR_IN,
> + GPIO_NR_H5000_USB_CHG_RATE | MFP_DIR_OUT,
> +

A big no no, please consider using GPIOxx_GPIO for those pins
configured to be GPIO, and leave the pin usage in the comment.

> + /* N/C */
> + 6 | MFP_DIR_IN,
> + 8 | MFP_DIR_OUT,
> + 16 | MFP_DIR_OUT,
> + 17 | MFP_DIR_OUT,
> + 22 | MFP_DIR_OUT,
> + 27 | MFP_DIR_OUT,
> + 59 | MFP_DIR_OUT, /* XXX docs say "usb charge on" input */
> + 63 | MFP_DIR_OUT,
> + 69 | MFP_DIR_OUT,
> +};

1. Please avoid using MFP_DIR_* directly, they are not for external
 use.

2. Configuring pins in this way is not good. The MFP API is designed
for selecting pin function, and not for configuring GPIO direction and
level, use GPIO API for that purpose.

To avoid doing so, you have three options:

1) use GPIOxx_GPIO if the pin direction and level can be left unknown
and causes no side-effects (remember that GPIOxx_GPIO means
_only_ to configure GPIOxx as a GPIO, nothing else, one shall really
not assume the GPIO direction and level, but in fact, it is initialized to
input by default to minimize the potential impact to the external)

2) use GPIOxx_GPIO, and GPIO API to configure a correct pin direction
and level explicitly in the code.

3) use bootloader settings, remove these confusing configurations
and leave these pin unconfigured.

> +
> +/*
> + * Localbus setup:
> + * CS0: Flash;
> + * CS1: MediaQ chip, select 16-bit bus and vlio;
> + * CS5: SAMCOP.
> + */
> +
> +static void fix_msc(void)
> +{
> + MSC0 = 0x129c24f2;
> + (void)MSC0;
> + MSC1 = 0x7ff424fa;
> + (void)MSC1;
> + MSC2 = 0x7ff47ff4;
> + (void)MSC2;
> +
> + MDREFR |= 0x02080000;
> +}

I don't think the read-backs here are necessary from my understanding
of the PXA27x internal bus hierarchy. Try remove them and test.

> +
> +/*
> + * Platform devices
> + */
> +
> +static struct platform_device *devices[] __initdata = {
> + &h5000_flash[0],
> + &h5000_flash[1],
> +};
> +
> +static void __init h5000_init(void)
> +{
> + int i;
> +
> + fix_msc();
> +
> + pxa2xx_mfp_config(ARRAY_AND_SIZE(h5000_pin_config));
> +
> + PWER = PWER_GPIO0 | PWER_RTC;
> + PFER = PWER_GPIO0 | PWER_RTC;
> + PRER = 0;

Please don't. Use WAKEUP_ON_EDGE_* to configure the pin wakeup
capability. And use gpio-key to allow the configuration of enabling of
wakeup through sysfs attribute. This is also true for RTC. So please
just remove them, if GPIO0 is really used as wakeup pin, try OR
WAKEUP_ON_EDGE_* in the MFP table.

> + PCFR = PCFR_OPDE;
> + CKEN = (CKEN & 0xfffe0000) | 1 << CKEN_FFUART;
> +

Please remove this line. I understand the requirement of
initializing CKEN so to minimize the power consumption.
However, this is really a matter of system code. (I'd expect
CKEN being properly initialized by clock.c, to be honest)

> + pxa_set_udc_info(&h5000_udc_mach_info);
> + for (i = 0; i < ARRAY_SIZE(devices); i++)
> + platform_device_register(devices[i]);
> +}
> +

I'd prefer to use platform_add_devices().
Received on Fri Sep 26 2008 - 07:55:39 EDT