Re: Patch adding support for FSC Loox N560

On Wed, Dec 12, 2007 at 10:18:41PM +0100, Marek Vašut wrote:
> Hi,

Hi!

> this patch adds support for FSC Loox N560 handheld. So far kernel boots and
> can read rootfs from memory card. Patch is mostly based on C550 port with
> some minor cleanups.
>
> Signed-off-by: Marek Vasut <marek.vasut_at_gmail.com>

Looks really good and mostly clean. Great work!

I see that that machine almost doesn't depend on -hh features,
i.e. -hh specific stuff: only touchscreen and adc (and mmc?). So, I'd
suggest you to prepare another version of that patch and push it to
mainline directly. If not you, none will do it, I'm afraid.

Soonish I'll post ADC class to LKML, with few drivers... so, if you'll
manage to include loox basic support, all you have to do later is
submit incremental patches against mainline, supporting ts/adc for loox.

Few comments below.

> Index: arch/arm/mach-pxa/looxn560/looxn560.c
> ===================================================================
> RCS file: arch/arm/mach-pxa/looxn560/looxn560.c
> diff -N arch/arm/mach-pxa/looxn560/looxn560.c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ arch/arm/mach-pxa/looxn560/looxn560.c 12 Dec 2007 20:51:12 -0000
> @@ -0,0 +1,246 @@
> +/*
[...]
> +/**************************** MCI **********************************/
> +

This is just my personal preference: don't use comments like that.

Better

/*
 * MCI
 */

Yes, it's not that noticeable, but that is the point indeed. ;-)

> +static int looxn560_mci_init(struct device *dev, irq_handler_t looxn560_detect_int, void *data)
> +{
> + int err;
> +
> + /*
> + * setup GPIO for PXA27x MMC controller
> + */
> +
> + pxa_gpio_mode(GPIO32_MMCCLK_MD);
> + pxa_gpio_mode(GPIO92_MMCDAT0_MD);
> + pxa_gpio_mode(GPIO109_MMCDAT1_MD);
> + pxa_gpio_mode(GPIO110_MMCDAT2_MD);
> + pxa_gpio_mode(GPIO111_MMCDAT3_MD);
> + pxa_gpio_mode(GPIO112_MMCCMD_MD);
> +
> + set_irq_type(gpio_to_irq(GPIO_NR_LOOXN560_SD_DETECT)
> + , IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING);
> + err = request_irq(gpio_to_irq(GPIO_NR_LOOXN560_SD_DETECT)
> + , looxn560_detect_int, IRQF_DISABLED
> + , "MMC card detect", data);
> + if (err) {
> + printk(KERN_ERR "looxn560_mci_init: MMC/SD: can't request MMC card detect IRQ\n");

long line

> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static void looxn560_mci_setpower(struct device *dev, unsigned int vdd)
> +{
> + struct pxamci_platform_data* p_d = dev->platform_data;
> +
> + gpio_set_value(GPIO_NR_LOOXN560_SD_POWER, ( 1 << vdd) & p_d->ocr_mask);
> +}
> +
> +static void looxn560_mci_exit(struct device *dev, void *data)
> +{
> + free_irq(gpio_to_irq(GPIO_NR_LOOXN560_SD_DETECT), data);
> +}
> +
> +static int looxn560_mci_get_ro(struct device* dev)
> +{
> + return gpio_get_value(GPIO_NR_LOOXN560_SD_RO);
> +}
> +
> +static struct pxamci_platform_data looxn560_mci_platform_data = {
> + .ocr_mask = MMC_VDD_32_33|MMC_VDD_33_34,

                ^^^^^^^ spaces for alignment
> + .init = looxn560_mci_init,
                ^^^^^^^ or tabs?
> + .setpower = looxn560_mci_setpower,
> + .exit = looxn560_mci_exit,
> + .get_ro = looxn560_mci_get_ro
> +};
> +
> +/**************************** UDC **************************/
> +
> +static struct pxa2xx_udc_mach_info looxn560_udc_info __initdata = {
> + .gpio_pullup = GPIO_NR_LOOXN560_USB_PULLUP
> +};
> +
> +/*************************** Power driver **************************/
> +
> +static int looxn560_is_ac_online(void)
> +{
> + return !gpio_get_value(GPIO_NR_LOOXN560_AC_IN_N);
> +}
> +
> +static int looxn560_is_usb_online(void)
> +{
> + return !gpio_get_value(GPIO_NR_LOOXN560_USB_DETECT_N);
> +}
> +
> +static void looxn560_set_charge(int flags)
> +{
> + gpio_set_value(GPIO_NR_LOOXN560_CHARGE_EN_N
> + , !(flags & PDA_POWER_CHARGE_AC));
> +
> + gpio_set_value(GPIO_NR_LOOXN560_USB_CHARGE_RATE_N
> + , !(flags & PDA_POWER_CHARGE_USB));
> +};
> +
> +static struct pda_power_pdata looxn560_power_data = {
> + .is_ac_online = looxn560_is_ac_online,
> + .is_usb_online = looxn560_is_usb_online,
> + .set_charge = looxn560_set_charge
> +};
> +
> +static struct resource looxn560_power_resources[] = {
> + [0] = {
> + .name = "ac",
> + .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE |
> + IORESOURCE_IRQ_LOWEDGE,
> + },
> + [1] = {
> + .name = "usb",
> + .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE |
> + IORESOURCE_IRQ_LOWEDGE,
> + },
> +};
> +
> +static struct platform_device looxn560_power = {
> + .name = "pda-power",
> + .id = -1,
> + .resource = looxn560_power_resources,
> + .num_resources = ARRAY_SIZE(looxn560_power_resources),
> + .dev = {
> + .platform_data = &looxn560_power_data
> + },
> +};
> +
> +/**************************** Entry point **************************/
> +struct ads7846_ssp_platform_data looxn560_ssp_params = {
> + .port = 1,
> + .pd_bits = 1,
> + .freq = 720000,
> +};
> +static struct platform_device ads7846_ssp = {

trailing whitespace

> + .name = "ads7846-ssp",

trailing whitespace

> + .id = -1,
> + .dev = {
> + .platform_data = &looxn560_ssp_params,
> + }
> +};
> +
> +struct tsadc_platform_data looxn560_ts_params = {
> + .pen_gpio = GPIO_NR_LOOXN560_TOUCHPANEL_IRQ_N,
> + .x_pin = "ads7846-ssp:x",
> + .y_pin = "ads7846-ssp:y",
> + .z1_pin = "ads7846-ssp:z1",
> + .z2_pin = "ads7846-ssp:z2",
> + .pressure_factor = 100000,
> + .min_pressure = 2,
> + .max_jitter = 8,
> +};
> +static struct resource looxn560_pen_irq = {

};
<newline here>
static struct...

> + .start = gpio_to_irq(GPIO_NR_LOOXN560_TOUCHPANEL_IRQ_N),
> + .end = gpio_to_irq(GPIO_NR_LOOXN560_TOUCHPANEL_IRQ_N),
> + .flags = IORESOURCE_IRQ,
> +};
> +static struct platform_device ads7846_ts = {

};
<newline here>
static struct...

> + .name = "ts-adc",
> + .id = -1,
> + .resource = &looxn560_pen_irq,
> + .num_resources = 1,
> + .dev = {
> + .platform_data = &looxn560_ts_params,
> + }

It's better to add comma after }

> +};
> +
> +static struct platform_device looxn560_keyboard = {
> + .name = "looxn560-buttons"
> +};
> +
> +static struct platform_device *devices[] __initdata = {
> + &ads7846_ssp,
> + &ads7846_ts,
> + &looxn560_power,
> + &looxn560_keyboard
> +};
> +
> +/*
> + * PXA27x framebuffer
> + */
> +
> +static struct pxafb_mode_info looxn560_pxafb_mode_info[] = {
> + {

^^ spaces.

> + .pixclock = 0,
> + .xres = 480,
> + .yres = 640,
> + .bpp = 16,
> + .hsync_len = 8,
> + .vsync_len = 2,
> +
> + .left_margin = 8,
> + .right_margin = 124,
> + .upper_margin = 3,
> + .lower_margin = 25,
> +
> + .sync = 0,
> + },

spaces

> +};
> +
> +static struct pxafb_mach_info looxn560_pxafb_info = {
> + .modes = looxn560_pxafb_mode_info,
> + .num_modes = ARRAY_SIZE(looxn560_pxafb_mode_info),
                  ^^^^^^ spaces

> + .lccr0 = LCCR0_Act | LCCR0_Sngl | LCCR0_Color,
> + .lccr3 = LCCR3_OutEnH | LCCR3_PixRsEdg,
> +};
> +
> +static void __init looxn560_init(void)
> +{
> + set_pxa_fb_info(&looxn560_pxafb_info);
> + pxa_set_mci_info(&looxn560_mci_platform_data);
> + pxa_set_udc_info(&looxn560_udc_info);
> + platform_add_devices(devices, ARRAY_SIZE(devices));
> +}
> +
> +MACHINE_START(LOOXN560, "FSC Loox N560")
> + .phys_io = 0x40000000,
> + .boot_params = 0xa0000100, /* BLOB boot parameter setting */
> + .io_pg_offst = (io_p2v(0x40000000) >> 18) & 0xfffc,
> + .map_io = pxa_map_io,
> + .init_irq = pxa_init_irq,
> + .timer = &pxa_timer,
> + .init_machine = looxn560_init,
> +MACHINE_END
> Index: arch/arm/mach-pxa/looxn560/looxn560_buttons.c
> ===================================================================
> RCS file: arch/arm/mach-pxa/looxn560/looxn560_buttons.c
> diff -N arch/arm/mach-pxa/looxn560/looxn560_buttons.c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ arch/arm/mach-pxa/looxn560/looxn560_buttons.c 12 Dec 2007 20:51:12 -0000
> @@ -0,0 +1,118 @@
> +/*
> + * linux/arch/arm/mach-pxa/looxn560/looxn560_buttons.c

no need to specify filename. better--copyright.

Also, you'd better merge that file into the main one.

> + *
> + * Keyboard definitions for FSC LOOX N560
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/input.h>
> +#include <linux/input_pda.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio_keys.h>
> +
> +#include <asm/mach-types.h>
> +#include <asm/arch/pxa27x_keyboard.h>
> +#include <asm/arch/looxn560.h>
> +
> +
> +/**************************** Keyboard **************************/
> +
> +static struct pxa27x_keyboard_platform_data looxn560_buttons = {
> + .nr_rows = 5,
> + .nr_cols = 2,
> + .keycodes = {

spaces

> + { _KEY_RECORD,
> + KEY_UP
> + },
> + { _KEY_CALENDAR,
> + KEY_DOWN
> + },

spaces

> + { KEY_RIGHT,
> + _KEY_MAIL
> + },
> + { _KEY_CONTACTS,
> + KEY_LEFT
> + },

spaces

> + { _KEY_HOMEPAGE,
> + KEY_ENTER
> + },
> + },
> + .gpio_modes = {

spaces

> + GPIO_NR_LOOXN560_KP_MKIN0_MD,
> + GPIO_NR_LOOXN560_KP_MKIN1_MD,
> + GPIO_NR_LOOXN560_KP_MKIN2_MD,
> + GPIO_NR_LOOXN560_KP_MKIN3_MD,
> + GPIO_NR_LOOXN560_KP_MKIN4_MD,
> + GPIO_NR_LOOXN560_KP_MKOUT0_MD,
> + GPIO_NR_LOOXN560_KP_MKOUT1_MD,
> + }

ditto

> +};
> +
> +static struct platform_device looxn560_pxa_keyboard = {
> + .name = "pxa27x-keyboard",
> + .id = -1,
> + .dev = {
> + .platform_data = &looxn560_buttons
> + }
> +};
> +
> +/*
> +Power button connected to GPIO 0, so we need use gpio-keys
> +*/

unorthodox comment

> +
> +static struct gpio_keys_button looxn560_gpio_buttons[] = {
> + { KEY_POWER, GPIO_NR_LOOXN560_KEYPWR, 0, "Power button"}
> +};
> +
> +static struct gpio_keys_platform_data looxn560_gpio_keys_data = {
> + .buttons = looxn560_gpio_buttons,
> + .nbuttons = ARRAY_SIZE(looxn560_gpio_buttons)
> +};
> +
> +static struct platform_device looxn560_gpio_keys = {
> + .name = "gpio-keys",
> + .dev = {
> + .platform_data = &looxn560_gpio_keys_data
> + }
> +};
> +
> +/*********************************************************************/
> +
> +static int __devinit looxn560_buttons_probe(struct platform_device *dev)
> +{
> + platform_device_register(&looxn560_pxa_keyboard);
> + platform_device_register(&looxn560_gpio_keys);
> + return 0;

spaces

> +}
> +
> +static struct platform_driver looxn560_buttons_driver = {
> + .driver = {
> + .name= "looxn560-buttons",

spaces

> + },
> + .probe = looxn560_buttons_probe

, missing at the end

> +};
> +
> +static int __init looxn560_buttons_init(void)
> +{
> + if (!machine_is_looxn560())
> + return -ENODEV;

spaces

> + return platform_driver_register(&looxn560_buttons_driver);
> +}
> +
> +static void __exit looxn560_buttons_exit(void)
> +{
> + platform_driver_unregister(&looxn560_buttons_driver);
> +}
> +
> +module_init(looxn560_buttons_init);
> +module_exit(looxn560_buttons_exit);
> +
> +MODULE_DESCRIPTION ("Buttons support for FSC Loox N560");
> +MODULE_LICENSE ("GPL");

MODULE_AUTHOR()

> Index: include/asm-arm/arch-pxa/looxn560.h
> ===================================================================
> RCS file: include/asm-arm/arch-pxa/looxn560.h
> diff -N include/asm-arm/arch-pxa/looxn560.h
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ include/asm-arm/arch-pxa/looxn560.h 12 Dec 2007 20:51:13 -0000
> @@ -0,0 +1,45 @@
> +/*
> + * FSC Loox N560 GPIO definitions
> + */
> +

Missing your copyright and GPL header?

> +#include <asm/arch/pxa-regs.h>
> +
> +#define GPIO_NR_LOOXN560_KEYPWR 0
> +
> +#define GPIO_NR_LOOXN560_BATTCOVER 4
> +
> +#define GPIO_NR_LOOXN560_AC_IN_N 9
> +
> +#define GPIO_NR_LOOXN560_USB_DETECT_N 13
> +
> +#define GPIO_NR_LOOXN560_SD_RO 15
> +
> +#define GPIO_NR_LOOXN560_USB_PULLUP 57
> +
> +#define GPIO_NR_LOOXN560_SD_DETECT 78
> +
> +#define GPIO_NR_LOOXN560_HP_JACK 93
> +#define GPIO_NR_LOOXN560_TOUCHPANEL_IRQ_N 94
> +
> +#define GPIO_NR_LOOXN560_KP_MKIN3 97
> +#define GPIO_NR_LOOXN560_KP_MKIN4 98
> +#define GPIO_NR_LOOXN560_CHARGE_EN_N 99
> +
> +
> +#define GPIO_NR_LOOXN560_KP_MKIN0 100
> +#define GPIO_NR_LOOXN560_KP_MKIN1 101
> +#define GPIO_NR_LOOXN560_KP_MKIN2 102
> +#define GPIO_NR_LOOXN560_KP_MKOUT0 103
> +#define GPIO_NR_LOOXN560_KP_MKOUT1 104
> +
> +#define GPIO_NR_LOOXN560_USB_CHARGE_RATE_N 107
> +#define GPIO_NR_LOOXN560_SD_POWER 108
> +
> +#define GPIO_NR_LOOXN560_KP_MKIN0_MD (GPIO_NR_LOOXN560_KP_MKIN0 | GPIO_ALT_FN_1_IN)
> +#define GPIO_NR_LOOXN560_KP_MKIN1_MD (GPIO_NR_LOOXN560_KP_MKIN1 | GPIO_ALT_FN_1_IN)
> +#define GPIO_NR_LOOXN560_KP_MKIN2_MD (GPIO_NR_LOOXN560_KP_MKIN2 | GPIO_ALT_FN_1_IN)
> +#define GPIO_NR_LOOXN560_KP_MKIN3_MD (GPIO_NR_LOOXN560_KP_MKIN3 | GPIO_ALT_FN_3_IN)
> +#define GPIO_NR_LOOXN560_KP_MKIN4_MD (GPIO_NR_LOOXN560_KP_MKIN4 | GPIO_ALT_FN_3_IN)
> +
> +#define GPIO_NR_LOOXN560_KP_MKOUT0_MD (GPIO_NR_LOOXN560_KP_MKOUT0 | GPIO_ALT_FN_2_OUT)
> +#define GPIO_NR_LOOXN560_KP_MKOUT1_MD (GPIO_NR_LOOXN560_KP_MKOUT1 | GPIO_ALT_FN_2_OUT)

If/when you'll want to submit that machine to the mainline,
try to pipe it through linux-2.6/scrips/checkpatch.pl. It's great
tool that finds hidden/non-obvious glitches. ;-)

p.s. Who is gonna to commit this? And where to? ;-) Paul seem to
busy nowadays.

-- 
Anton Vorontsov
email: cbou_at_mail.ru
backup email: ya-cbou_at_yandex.ru
irc://irc.freenode.net/bd2