Re: [PATCH] mach-at91: Support for gms board added.

From: Ryan Mallon
Date: Mon Dec 06 2010 - 15:10:47 EST

Next message: Jiri Slaby: "[PATCH v2 1/1] V4L: videobuf, don't use dma addr as physical"
Previous message: Alexey Dobriyan: "Re: [PATCH 26/45] kstrtox: convert drivers/isdn/"
In reply to: Igor Plyatov: "[PATCH] mach-at91: Support for gms board added."
Next in thread: Igor Plyatov: "Re: [PATCH] mach-at91: Support for gms board added."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 12/07/2010 08:32 AM, Igor Plyatov wrote:
> The gms is a board from GeoSIG Ltd. It is based on the Stamp9G20 module from Taskit.
>
> Signed-off-by: Igor Plyatov <plyatov@xxxxxxxxx>
> ---

Hi Igor,

A few comments below.

> arch/arm/configs/stamp9g20gms_defconfig | 1830 +++++++++++++++++++++++++++++++
> arch/arm/mach-at91/Kconfig | 7 +
> arch/arm/mach-at91/Makefile | 1 +
> arch/arm/mach-at91/board-stamp9g20gms.c | 720 ++++++++++++
> arch/arm/mach-at91/include/mach/gms.h | 27 +
> 5 files changed, 2585 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/configs/stamp9g20gms_defconfig
> create mode 100644 arch/arm/mach-at91/board-stamp9g20gms.c
> create mode 100644 arch/arm/mach-at91/include/mach/gms.h
>
> diff --git a/arch/arm/configs/stamp9g20gms_defconfig b/arch/arm/configs/stamp9g20gms_defconfig
> new file mode 100644
> index 0000000..430b57f
> --- /dev/null
> +++ b/arch/arm/configs/stamp9g20gms_defconfig
> @@ -0,0 +1,1830 @@

We are trying to avoid adding more large defconfigs. Can you roll this
into one of the existing defconfigs or at least run it through Uwe's
defconfig minimiser script.

<snip>

> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
> index c015b68..5f31223 100644
> --- a/arch/arm/mach-at91/Kconfig
> +++ b/arch/arm/mach-at91/Kconfig
> @@ -381,6 +381,13 @@ config MACH_PCONTROL_G20
> Select this if you are using taskit's Stamp9G20 CPU module on this
> carrier board, beeing the decentralized unit of a building automation
> system; featuring nvram, eth-switch, iso-rs485, display, io
> +
> +config MACH_STAMP9G20GMS
> + bool "GeoSIG Stamp9G20 GMS"
> + help
> + Select this if you are using taskit's Stamp9G20 with GeoSIG's GMS.
> + <http://www.geosig.com>
> +
> endif
>
> if (ARCH_AT91SAM9260 || ARCH_AT91SAM9G20)
> diff --git a/arch/arm/mach-at91/Makefile b/arch/arm/mach-at91/Makefile
> index 62d686f..6eeeba7 100644
> --- a/arch/arm/mach-at91/Makefile
> +++ b/arch/arm/mach-at91/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_MACH_AT91SAM9RLEK) += board-sam9rlek.o
> obj-$(CONFIG_MACH_AT91SAM9G20EK) += board-sam9g20ek.o
> obj-$(CONFIG_MACH_CPU9G20) += board-cpu9krea.o
> obj-$(CONFIG_MACH_STAMP9G20) += board-stamp9g20.o
> +obj-$(CONFIG_MACH_STAMP9G20GMS) += board-stamp9g20gms.o
> obj-$(CONFIG_MACH_PORTUXG20) += board-stamp9g20.o
> obj-$(CONFIG_MACH_PCONTROL_G20) += board-pcontrol-g20.o
>
> diff --git a/arch/arm/mach-at91/board-stamp9g20gms.c b/arch/arm/mach-at91/board-stamp9g20gms.c
> new file mode 100644
> index 0000000..d286c1f
> --- /dev/null
> +++ b/arch/arm/mach-at91/board-stamp9g20gms.c
> @@ -0,0 +1,720 @@
> +/*
> + * Copyright (C) 2010 Christian Glindkamp <christian.glindkamp@xxxxxxxxx>
> + * taskit GmbH
> + * 2010 Igor Plyatov <plyatov@xxxxxxxxx>
> + * GeoSIG Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#include <linux/mm.h>

What do you need this for?

> +#include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/w1-gpio.h>
> +#include <linux/i2c/pcf857x.h>
> +#include <linux/gpio_keys.h>
> +#include <linux/input.h>
> +
> +#include <asm/mach-types.h>
> +#include <asm/mach/arch.h>
> +
> +#include <mach/board.h>
> +#include <mach/at91sam9_smc.h>
> +#include <mach/gms.h>
> +
> +#include "sam9_smc.h"
> +#include "generic.h"
> +
> +static void __init stamp9g20gms_map_io(void)
> +{
> + /* Initialize processor: 18.432 MHz crystal */
> + at91sam9260_initialize(18432000);
> +
> + /* DGBU on ttyS0. (Rx & Tx only) */
> + at91_register_uart(0, 0, 0);
> +
> + /*
> + * USART0 on ttyS1 (Rx, Tx, CTS, RTS, DTR, DSR, DCD, RI).
> + * Used for Internal Analog Modem.
> + */
> + at91_register_uart(AT91SAM9260_ID_US0, 1,
> + ATMEL_UART_CTS | ATMEL_UART_RTS
> + | ATMEL_UART_DTR | ATMEL_UART_DSR
> + | ATMEL_UART_DCD | ATMEL_UART_RI);
> + /*
> + * USART1 on ttyS2 (Rx, Tx, CTS, RTS).
> + * Used for GPS or WiFi or Data stream.
> + */
> + at91_register_uart(AT91SAM9260_ID_US1, 2,
> + ATMEL_UART_CTS | ATMEL_UART_RTS);
> + /*
> + * USART2 on ttyS3 (Rx, Tx, CTS, RTS).
> + * Used for External Modem.
> + */
> + at91_register_uart(AT91SAM9260_ID_US2, 3,
> + ATMEL_UART_CTS | ATMEL_UART_RTS);
> + /*
> + * USART3 on ttyS4 (Rx, Tx, RTS).
> + * Used for RS-485.
> + */
> + at91_register_uart(AT91SAM9260_ID_US3, 4, ATMEL_UART_RTS);

Nitpick, blank line after the above line.

> + /*
> + * USART4 on ttyS5 (Rx, Tx).
> + * Used for TRX433 Radio Module.
> + */
> + at91_register_uart(AT91SAM9260_ID_US4, 5, 0);
> +
> + /* set serial console to ttyS0 (ie, DBGU) */
> + at91_set_serial_console(0);
> +}
> +
> +static void __init init_irq(void)
> +{
> + at91sam9260_init_interrupts(NULL);
> +}
> +
> +/******************************************************************************
> + * NAND flash
> + ******************************************************************************/
> +static struct atmel_nand_data __initdata nand_data = {
> + .ale = 21,
> + .cle = 22,
> + .rdy_pin = AT91_PIN_PC13,
> + .enable_pin = AT91_PIN_PC14,
> + .bus_width_16 = 0,
> +};
> +
> +static struct sam9_smc_config __initdata nand_smc_config = {
> + .ncs_read_setup = 0,
> + .nrd_setup = 2,
> + .ncs_write_setup = 0,
> + .nwe_setup = 2,
> +
> + .ncs_read_pulse = 4,
> + .nrd_pulse = 4,
> + .ncs_write_pulse = 4,
> + .nwe_pulse = 4,
> +
> + .read_cycle = 7,
> + .write_cycle = 7,
> +
> + .mode = AT91_SMC_READMODE | \

Nitpick, remove the tab on the right hand side of the equals here.

> + AT91_SMC_WRITEMODE | \
> + AT91_SMC_EXNWMODE_DISABLE | \
> + AT91_SMC_DBW_8,
> + .tdf_cycles = 3,
> +};
> +
> +static void __init add_device_nand(void)
> +{
> + /* configure chip-select 3 (NAND) */
> + sam9_smc_configure(3, &nand_smc_config);
> +
> + at91_add_device_nand(&nand_data);
> +}
> +
> +/******************************************************************************
> + * MCI (SD/MMC)
> + * det_pin, wp_pin and vcc_pin are not connected
> + ******************************************************************************/
> +#if defined(CONFIG_MMC_ATMELMCI) || defined(CONFIG_MMC_ATMELMCI_MODULE)
> +static struct mci_platform_data __initdata mmc_data = {
> + .slot[0] = {
> + .bus_width = 4,
> + },
> +};
> +#else
> +static struct at91_mmc_data __initdata mmc_data = {
> + .slot_b = 0,
> + .wire4 = 1,
> +};
> +#endif
> +
> +/******************************************************************************
> + * USB Host port
> + ******************************************************************************/
> +static struct at91_usbh_data __initdata usbh_data = {
> + .ports = 2,
> +};
> +
> +

You only need a single blank line between code blocks. Also, not sure if
you need the massive comment headers for structs which should be
relatively self explanatory.

> +/******************************************************************************
> + * USB Device port
> + ******************************************************************************/
> +static struct at91_udc_data __initdata stamp9g20gms_udc_data = {
> + .vbus_pin = AT91_PIN_PA22,
> + .pullup_pin = 0, /* pull-up driven by UDC */
> +};
> +
> +
> +/******************************************************************************
> + * MACB Ethernet device
> + ******************************************************************************/
> +static struct at91_eth_data __initdata macb_data = {
> + .phy_irq_pin = AT91_PIN_PA28,
> + .is_rmii = 1,
> +};
> +
> +
> +/******************************************************************************
> + * LEDs and GPOs
> + ******************************************************************************/
> +#if defined(CONFIG_LEDS_GPIO) || defined(CONFIG_LEDS_GPIO_MODULE)
> +static struct gpio_led stamp9g20gms_gpio_leds[] = {
> + {
> + .name = "gpo:spi1reset",
> + .gpio = AT91_PIN_PC1,
> + .active_low = 0,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + {
> + .name = "gpo:trig_net_out",
> + .gpio = AT91_PIN_PB20,
> + .active_low = 0,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + {
> + .name = "gpo:trig_net_dir",
> + .gpio = AT91_PIN_PB19,
> + .active_low = 0,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + {
> + .name = "gpo:charge_dis",
> + .gpio = AT91_PIN_PC2,
> + .active_low = 0,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + {
> + .name = "led:event",
> + .gpio = AT91_PIN_PB17,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + {
> + .name = "led:lan",
> + .gpio = AT91_PIN_PB18,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + {
> + .name = "led:error",
> + .gpio = AT91_PIN_PB16,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_ON,
> + }
> +};
> +
> +static struct gpio_led_platform_data stamp9g20gms_gpio_led_info = {
> + .leds = stamp9g20gms_gpio_leds,
> + .num_leds = ARRAY_SIZE(stamp9g20gms_gpio_leds),
> +};
> +
> +static struct platform_device stamp9g20gms_leds = {
> + .name = "leds-gpio",
> + .id = 0,
> + .dev = {
> + .platform_data = &stamp9g20gms_gpio_led_info,
> + }
> +};
> +
> +static void __init stamp9g20gms_leds_init(void)
> +{
> + platform_device_register(&stamp9g20gms_leds);
> +}
> +#else
> +static inline void stamp9g20gms_leds_init(void) {}
> +#endif
> +
> +/* PCF8574 0x20 GPIO - U1 on the GS_IA18-CB_V3 board */
> +#if (defined(CONFIG_LEDS_GPIO) || defined(CONFIG_LEDS_GPIO_MODULE)) && \
> + (defined(CONFIG_GPIO_PCF857X) || defined(CONFIG_GPIO_PCF857X_MODULE))
> +static struct gpio_led stamp9g20gms_pcf_gpio_leds1[] = {
> + { /* bit 0 */
> + .name = "gpo:hdc_power",
> + .gpio = PCF_GPIO_HDC_POWER,
> + .active_low = 0,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 1 */
> + .name = "gpo:wifi_setup",
> + .gpio = PCF_GPIO_WIFI_SETUP,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 2 */
> + .name = "gpo:wifi_enable",
> + .gpio = PCF_GPIO_WIFI_ENABLE,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 3 */
> + .name = "gpo:wifi_reset",
> + .gpio = PCF_GPIO_WIFI_RESET,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_ON,
> + },
> + /* bit 4 used as GPI */
> + { /* bit 5 */
> + .name = "gpo:gps_setup",
> + .gpio = PCF_GPIO_GPS_SETUP,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 6 */
> + .name = "gpo:gps_standby",
> + .gpio = PCF_GPIO_GPS_STANDBY,
> + .active_low = 0,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_ON,
> + },
> + { /* bit 7 */
> + .name = "gpo:gps_power",
> + .gpio = PCF_GPIO_GPS_POWER,
> + .active_low = 0,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + }
> +};
> +
> +static struct gpio_led_platform_data stamp9g20gms_pcf_gpio_led_info1 = {
> + .leds = stamp9g20gms_pcf_gpio_leds1,
> + .num_leds = ARRAY_SIZE(stamp9g20gms_pcf_gpio_leds1),
> +};
> +
> +static struct platform_device stamp9g20gms_pcf_leds1 = {
> + .name = "leds-gpio", /* GS_IA18-CB_board */
> + .id = 1,
> + .dev = {
> + .platform_data = &stamp9g20gms_pcf_gpio_led_info1,
> + }
> +};
> +
> +/* PCF8574 0x22 GPIO - U1 on the GS_2G_OPT1-A_V0 board (Alarm) */
> +static struct gpio_led stamp9g20gms_pcf_gpio_leds2[] = {
> + { /* bit 0 */
> + .name = "gpo:alarm_1",
> + .gpio = PCF_GPIO_ALARM1,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 1 */
> + .name = "gpo:alarm_2",
> + .gpio = PCF_GPIO_ALARM2,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 2 */
> + .name = "gpo:alarm_3",
> + .gpio = PCF_GPIO_ALARM3,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 3 */
> + .name = "gpo:alarm_4",
> + .gpio = PCF_GPIO_ALARM4,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + /* bits 4, 5, 6 not used */
> + { /* bit 7 */
> + .name = "gpo:alarm_v_relay_on",
> + .gpio = PCF_GPIO_ALARM_V_RELAY_ON,
> + .active_low = 0,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> +};
> +
> +static struct gpio_led_platform_data stamp9g20gms_pcf_gpio_led_info2 = {
> + .leds = stamp9g20gms_pcf_gpio_leds2,
> + .num_leds = ARRAY_SIZE(stamp9g20gms_pcf_gpio_leds2),
> +};
> +
> +static struct platform_device stamp9g20gms_pcf_leds2 = {
> + .name = "leds-gpio",
> + .id = 2,
> + .dev = {
> + .platform_data = &stamp9g20gms_pcf_gpio_led_info2,
> + }
> +};
> +
> +/* PCF8574 0x24 GPIO U1 on the GS_2G-OPT23-A_V0 board (Modem) */
> +static struct gpio_led stamp9g20gms_pcf_gpio_leds3[] = {
> + { /* bit 0 */
> + .name = "gpo:modem_power",
> + .gpio = PCF_GPIO_MODEM_POWER,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + /* bits 1 and 2 not used */
> + { /* bit 3 */
> + .name = "gpo:modem_reset",
> + .gpio = PCF_GPIO_MODEM_RESET,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_ON,
> + },
> + /* bits 4, 5 and 6 not used */
> + { /* bit 7 */
> + .name = "gpo:trx_reset",
> + .gpio = PCF_GPIO_TRX_RESET,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_ON,
> + }
> +};
> +
> +static struct gpio_led_platform_data stamp9g20gms_pcf_gpio_led_info3 = {
> + .leds = stamp9g20gms_pcf_gpio_leds3,
> + .num_leds = ARRAY_SIZE(stamp9g20gms_pcf_gpio_leds3),
> +};
> +
> +static struct platform_device stamp9g20gms_pcf_leds3 = {
> + .name = "leds-gpio",
> + .id = 3,
> + .dev = {
> + .platform_data = &stamp9g20gms_pcf_gpio_led_info3,
> + }
> +};
> +
> +static void __init stamp9g20gms_pcf_leds_init(void)
> +{
> + platform_device_register(&stamp9g20gms_pcf_leds1);
> + platform_device_register(&stamp9g20gms_pcf_leds2);
> + platform_device_register(&stamp9g20gms_pcf_leds3);
> +}
> +#else
> +static inline void stamp9g20gms_pcf_leds_init(void) {}
> +#endif /* CONFIG_LEDS_GPIO || CONFIG_GPIO_PCF857X */
> +
> +/******************************************************************************
> + * SPI busses.
> + ******************************************************************************/
> +static struct spi_board_info stamp9g20gms_spi_devices[] = {
> + { /* User accessible spi0, cs0 used for communication with MSP RTC */
> + .modalias = "spidev",
> + .bus_num = 0,
> + .chip_select = 0,
> + .max_speed_hz = 580000,
> + .mode = SPI_MODE_1,

Tab align these like the other structs in this file.

> + },
> + { /* User accessible spi1, cs0 used for communication with int. DSP */
> + .modalias = "spidev",
> + .bus_num = 1,
> + .chip_select = 0,
> + .max_speed_hz = 5600000,
> + .mode = SPI_MODE_0,
> + },
> + { /* User accessible spi1, cs1 used for communication with ext. DSP */
> + .modalias = "spidev",
> + .bus_num = 1,
> + .chip_select = 1,
> + .max_speed_hz = 5600000,
> + .mode = SPI_MODE_0,
> + },
> + { /* User accessible spi1, cs2 used for communication with ext. DSP */
> + .modalias = "spidev",
> + .bus_num = 1,
> + .chip_select = 2,
> + .max_speed_hz = 5600000,
> + .mode = SPI_MODE_0,
> + },
> + { /* User accessible spi1, cs3 used for communication with ext. DSP */
> + .modalias = "spidev",
> + .bus_num = 1,
> + .chip_select = 3,
> + .max_speed_hz = 5600000,
> + .mode = SPI_MODE_0,
> + }
> +};
> +
> +/******************************************************************************
> + * Dallas 1-Wire
> + ******************************************************************************/
> +static struct w1_gpio_platform_data w1_gpio_pdata = {
> + .pin = AT91_PIN_PA29,
> + .is_open_drain = 1,
> +};
> +
> +static struct platform_device w1_device = {
> + .name = "w1-gpio",
> + .id = -1,
> + .dev.platform_data = &w1_gpio_pdata,
> +};
> +
> +void add_w1(void)
> +{
> + at91_set_GPIO_periph(w1_gpio_pdata.pin, 1);
> + at91_set_multi_drive(w1_gpio_pdata.pin, 1);
> + platform_device_register(&w1_device);
> +}
> +
> +/******************************************************************************
> + * GPI Buttons
> + ******************************************************************************/
> +#if defined(CONFIG_KEYBOARD_GPIO) || defined(CONFIG_KEYBOARD_GPIO_MODULE)
> +static struct gpio_keys_button stamp9g20gms_buttons[] = {
> + {
> + .gpio = AT91_PIN_PB21,
> + .code = BTN_1,
> + .desc = "TRIG_NET_IN",
> + .type = EV_KEY,
> + .active_low = 0,
> + .wakeup = 1,
> + },
> + {
> + .gpio = AT91_PIN_PB13, /* SW80 */
> + .code = BTN_2,
> + .desc = "Card umount 0",
> + .type = EV_KEY,
> + .active_low = 1,
> + .wakeup = 1,
> + },
> + {
> + .gpio = AT91_PIN_PB12, /* SW79 */
> + .code = BTN_3,
> + .desc = "Card umount 1",
> + .type = EV_KEY,
> + .active_low = 1,
> + .wakeup = 1,
> + },
> + {
> + .gpio = AT91_PIN_PA25,
> + .code = KEY_POWER,
> + .desc = "Power Off Button",
> + .type = EV_KEY,
> + .active_low = 0,
> + .wakeup = 1,
> + }
> +};
> +
> +static struct gpio_keys_platform_data stamp9g20gms_button_data = {
> + .buttons = stamp9g20gms_buttons,
> + .nbuttons = ARRAY_SIZE(stamp9g20gms_buttons),
> +};
> +
> +static struct platform_device stamp9g20gms_button_device = {
> + .name = "gpio-keys",
> + .id = -1,
> + .num_resources = 0,
> + .dev = {
> + .platform_data = &stamp9g20gms_button_data,
> + }
> +};
> +
> +static void __init stamp9g20gms_add_device_buttons(void)
> +{
> + /* Configure "TRIG_NET_IN" input with pullup */
> + at91_set_gpio_input(AT91_PIN_PB21, 1); /* BTN_1 */
> + at91_set_deglitch(AT91_PIN_PB21, 1);
> + /* Configure "Card umount 0" input with pullup */
> + at91_set_gpio_input(AT91_PIN_PB12, 1); /* BTN_2 */
> + at91_set_deglitch(AT91_PIN_PB12, 1);
> + /* Configure "Card umount 1" input with pullup */
> + at91_set_gpio_input(AT91_PIN_PB13, 1); /* BTN_3 */
> + at91_set_deglitch(AT91_PIN_PB13, 1);
> + /* Configure "Power Off Button" input without pullup */
> + at91_set_gpio_input(AT91_PIN_PA25, 0); /* KEY_POWER */
> + at91_set_deglitch(AT91_PIN_PA25, 1);

I really dislike this style of commenting. You shouldn't need a comment
to explain the at91_set_gpio_input function. The comments for the button
functions should be replaced by using defines, ie:

#define BTN_1_TRIG_NET_IN AT91_PIN_PB21
#define BTN_2_CARD_UNMOUNT_0 AT91_PIN_PB12
#define BTN_3_CARD_UNMOUNT_1 AT91_PIN_PB13
#define BTN_POWER AT91_PIN_PA25

> + platform_device_register(&stamp9g20gms_button_device);
> +}
> +#else
> +static void __init stamp9g20gms_add_device_buttons(void) {}
> +#endif
> +
> +/******************************************************************************
> + * I2C
> + ******************************************************************************/
> +#if defined(CONFIG_GPIO_PCF857X) || defined(CONFIG_GPIO_PCF857X_MODULE)
> +static int pcf8574x_0x20_setup(struct i2c_client *client, int gpio,
> + unsigned int ngpio, void *context)
> +{
> + int status;
> +
> + status = gpio_request(gpio + 4, "eth_det");

Why gpio + 4? Either have the correct gpio passed in, or have a
define/comment explaining the magic number 4.

> + if (status < 0) {
> + pr_err("error: can't request GPIO%d\n", gpio + 4);
> + return -ENOSYS;

Why not return the error code which was returned by gpio_request?

> + }
> + status = gpio_direction_input(gpio + 4);
> + if (status < 0) {
> + pr_err("error: can't setup GPIO%d as input\n", gpio + 4);
> + return -ENOSYS;
> + }
> + status = gpio_export(gpio + 4, false);
> + if (status < 0) {
> + pr_err("error: can't export GPIO%d\n", gpio + 4);
> + return -ENOSYS;
> + }
> + status = gpio_sysfs_set_active_low(gpio + 4, 1);
> + if (status < 0) {
> + pr_err("error: gpio_sysfs_set active_low(GPIO%d, 1)\n", gpio+4);
> + return -ENOSYS;
> + }
> +
> + return 0;
> +}
> +
> +static int pcf8574x_0x20_teardown(struct i2c_client *client, int gpio,
> + unsigned ngpio, void *context)
> +{
> + gpio_free(gpio + 4);
> + return 0;
> +}
> +
> +static struct pcf857x_platform_data stamp9g20_pcf20_pdata = {
> + .gpio_base = GS_IA18_S_PCF_GPIO_BASE0,
> + .n_latch = (1 << 4),
> + .setup = pcf8574x_0x20_setup,
> + .teardown = pcf8574x_0x20_teardown,
> +};
> +
> +static struct pcf857x_platform_data stamp9g20_pcf22_pdata = {
> + .gpio_base = GS_IA18_S_PCF_GPIO_BASE1,
> +};
> +
> +static struct pcf857x_platform_data stamp9g20_pcf24_pdata = {
> + .gpio_base = GS_IA18_S_PCF_GPIO_BASE2,
> +};
> +#endif /* CONFIG_GPIO_PCF857X */
> +
> +static struct i2c_board_info __initdata stamp9g20gms_i2c_devices[] = {
> +#if defined(CONFIG_GPIO_PCF857X) || defined(CONFIG_GPIO_PCF857X_MODULE)
> + { /* U1 on the GS_IA18-CB_V3 board */
> + I2C_BOARD_INFO("pcf8574", 0x20),
> + .platform_data = &stamp9g20_pcf20_pdata,
> + },
> + { /* U1 on the GS_2G_OPT1-A_V0 board (Alarm) */
> + I2C_BOARD_INFO("pcf8574", 0x22),
> + .platform_data = &stamp9g20_pcf22_pdata,
> + },
> + { /* U1 on the GS_2G-OPT23-A_V0 board (Modem) */
> + I2C_BOARD_INFO("pcf8574", 0x24),
> + .platform_data = &stamp9g20_pcf24_pdata,
> + },
> +#endif /* CONFIG_GPIO_PCF857X */
> +#if defined(CONFIG_EEPROM_AT24) || defined(CONFIG_EEPROM_AT24_MODULE)
> + {
> + I2C_BOARD_INFO("24c1024", 0x50),
> + },
> +#else

Does this really need to be surrounded by an ifdef? There is no harm in
having a an I2C_BOARD_INFO entry for a device which is not present/not
probed. The code ugliness hardly seems worth the tiny saving this would
gain.

> + {
> + NULL,
> + },

If you are going to keep the ifdef, this entry is not needed.

> +#endif /* CONFIG_EEPROM_AT24 */
> +};
> +
> +/******************************************************************************
> + * Compact Flash
> + ******************************************************************************/
> +#if defined(CONFIG_PATA_AT91) || defined(CONFIG_PATA_AT91_MODULE)
> +static struct at91_cf_data __initdata stamp9g20gms_cf1_data = {
> + .irq_pin = AT91_PIN_PA27,
> + .det_pin = AT91_PIN_PB30,
> + .rst_pin = AT91_PIN_PB31,
> + .chipselect = 5,
> + .flags = AT91_CF_TRUE_IDE,
> +};
> +#endif /* CONFIG_PATA_AT91 */
> +
> +/* Power Off by RTC */
> +static void stamp9g20gms_power_off(void)
> +{
> + pr_notice("Power supply will be switched off automatically now or after"
> + " 60 seconds without ArmDAS.\n");

printk's should be on one line so they can be grepped for.

> + at91_set_gpio_output(AT91_PIN_PA25, 1);
> + /* Spin to death... */
> + while (1)
> + ;
> +}
> +
> +static int __init stamp9g20gms_power_off_init(void)
> +{
> + pm_power_off = stamp9g20gms_power_off;
> + return 0;
> +}
> +
> +/* ---------------------------------------------------------------------------*/
> +
> +static void __init generic_board_init(void)
> +{
> + /* Serial */
> + at91_add_device_serial();
> + /* NAND */
> + add_device_nand();
> + /* MMC */
> +#if defined(CONFIG_MMC_ATMELMCI) || defined(CONFIG_MMC_ATMELMCI_MODULE)
> + at91_add_device_mci(0, &mmc_data);
> +#else
> + at91_add_device_mmc(0, &mmc_data);
> +#endif /* CONFIG_MMC_ATMELMCI */
> + /* W1 */
> + add_w1();

Again the comments here are unhelpful since the function names reiterate
exactly what the comment says.

> +}
> +
> +static void __init stamp9g20gms_board_init(void)
> +{
> + generic_board_init();
> + /* USB Host */
> + at91_add_device_usbh(&usbh_data);
> + /* USB Device */
> + at91_add_device_udc(&stamp9g20gms_udc_data);
> + /* Ethernet */
> + at91_add_device_eth(&macb_data);
> + /* LEDs */
> + stamp9g20gms_leds_init();
> + stamp9g20gms_pcf_leds_init();
> + /* Push Buttons */
> + stamp9g20gms_add_device_buttons();
> + /* I2C */

Remove the comments here too.

> + at91_add_device_i2c(stamp9g20gms_i2c_devices,
> + ARRAY_SIZE(stamp9g20gms_i2c_devices));
> + /* Compact Flash */
> +#if defined(CONFIG_PATA_AT91) || defined(CONFIG_PATA_AT91_MODULE)
> + at91_add_device_cf(&stamp9g20gms_cf1_data);
> +#endif /* CONFIG_PATA_AT91 */

I think a lot of the ifdefs in this file could be removed. This is no
harm in having a device structure present for something that won't have
a driver attached later. The impact on code size is pretty minimal and
it makes the code easier to follow.

> + /* SPI */
> + at91_add_device_spi(stamp9g20gms_spi_devices,
> + ARRAY_SIZE(stamp9g20gms_spi_devices));
> + stamp9g20gms_power_off_init();
> +}
> +
> +MACHINE_START(STAMP9G20, "Stamp9G20 on the GeoSIG GS_IA18_S board")
> + /* Maintainer: GeoSIG Ltd */
> + .boot_params = AT91_SDRAM_BASE + 0x100,
> + .timer = &at91sam926x_timer,
> + .map_io = stamp9g20gms_map_io,
> + .init_irq = init_irq,
> + .init_machine = stamp9g20gms_board_init,
> +MACHINE_END
> diff --git a/arch/arm/mach-at91/include/mach/gms.h b/arch/arm/mach-at91/include/mach/gms.h
> new file mode 100644
> index 0000000..5d5b46f
> --- /dev/null
> +++ b/arch/arm/mach-at91/include/mach/gms.h
> @@ -0,0 +1,27 @@
> +/* PCF8574 0x20 GPIO - U1 on the GS_IA18-CB_V3 board */
> +#define GS_IA18_S_PCF_GPIO_BASE0 NR_BUILTIN_GPIO
> +#define PCF_GPIO_HDC_POWER (GS_IA18_S_PCF_GPIO_BASE0 + 0)
> +#define PCF_GPIO_WIFI_SETUP (GS_IA18_S_PCF_GPIO_BASE0 + 1)
> +#define PCF_GPIO_WIFI_ENABLE (GS_IA18_S_PCF_GPIO_BASE0 + 2)
> +#define PCF_GPIO_WIFI_RESET (GS_IA18_S_PCF_GPIO_BASE0 + 3)
> +#define PCF_GPIO_ETH_DETECT (GS_IA18_S_PCF_GPIO_BASE0 + 4)
> +#define PCF_GPIO_GPS_SETUP (GS_IA18_S_PCF_GPIO_BASE0 + 5)
> +#define PCF_GPIO_GPS_STANDBY (GS_IA18_S_PCF_GPIO_BASE0 + 6)
> +#define PCF_GPIO_GPS_POWER (GS_IA18_S_PCF_GPIO_BASE0 + 7)

Don't need a tab after the define.

> +/* PCF8574 0x22 GPIO - U1 on the GS_2G_OPT1-A_V0 board (Alarm) */
> +#define GS_IA18_S_PCF_GPIO_BASE1 (GS_IA18_S_PCF_GPIO_BASE0 + 8)
> +#define PCF_GPIO_ALARM1 (GS_IA18_S_PCF_GPIO_BASE1 + 0)
> +#define PCF_GPIO_ALARM2 (GS_IA18_S_PCF_GPIO_BASE1 + 1)
> +#define PCF_GPIO_ALARM3 (GS_IA18_S_PCF_GPIO_BASE1 + 2)
> +#define PCF_GPIO_ALARM4 (GS_IA18_S_PCF_GPIO_BASE1 + 3)
> +/* bits 4, 5, 6 not used */
> +#define PCF_GPIO_ALARM_V_RELAY_ON (GS_IA18_S_PCF_GPIO_BASE1 + 7)
> +
> +/* PCF8574 0x24 GPIO U1 on the GS_2G-OPT23-A_V0 board (Modem) */
> +#define GS_IA18_S_PCF_GPIO_BASE2 (GS_IA18_S_PCF_GPIO_BASE1 + 8)
> +#define PCF_GPIO_MODEM_POWER (GS_IA18_S_PCF_GPIO_BASE2 + 0)
> +#define PCF_GPIO_MODEM_RESET (GS_IA18_S_PCF_GPIO_BASE2 + 3)
> +/* bits 1, 2, 4, 5 not used */
> +#define PCF_GPIO_TRX_RESET (GS_IA18_S_PCF_GPIO_BASE2 + 6)
> +/* bit 7 not used */

~Ryan

--
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon 5 Amuri Park, 404 Barbadoes St
ryan@xxxxxxxxxxxxxxxx PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
--
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/

Next message: Jiri Slaby: "[PATCH v2 1/1] V4L: videobuf, don't use dma addr as physical"
Previous message: Alexey Dobriyan: "Re: [PATCH 26/45] kstrtox: convert drivers/isdn/"
In reply to: Igor Plyatov: "[PATCH] mach-at91: Support for gms board added."
Next in thread: Igor Plyatov: "Re: [PATCH] mach-at91: Support for gms board added."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]