Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756361AbYFWKN2 (ORCPT ); Mon, 23 Jun 2008 06:13:28 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754044AbYFWKNU (ORCPT ); Mon, 23 Jun 2008 06:13:20 -0400 Received: from 3a.49.1343.static.theplanet.com ([67.19.73.58]:60730 "EHLO pug.o-hand.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753608AbYFWKNT (ORCPT ); Mon, 23 Jun 2008 06:13:19 -0400 Date: Mon, 23 Jun 2008 12:13:12 +0200 From: Samuel Ortiz To: pHilipp Zabel Cc: Andrew Morton , Linux Kernel ML Subject: Re: [PATCH -mm 3/5] asic3: New gpio configuration code Message-ID: <20080623101311.GE7008@caravaggio> Reply-To: Samuel Ortiz References: <20080511173653.869192035@sortiz.org> <20080511174444.455389162@sortiz.org> <74d0deb30806210743x44f9d68elf2a394902c3fb8ce@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <74d0deb30806210743x44f9d68elf2a394902c3fb8ce@mail.gmail.com> User-Agent: Mutt/1.5.13 (2006-08-11) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12633 Lines: 284 On Sat, Jun 21, 2008 at 04:43:08PM +0200, pHilipp Zabel wrote: > On Sun, May 11, 2008 at 7:36 PM, Samuel Ortiz wrote: > > The ASIC3 GPIO configuration code is a bit obscure and hardly readable. > > This patch changes it so that it is now more readable and understandable, > > by being more explicit. > > > > Signed-off-by: Samuel Ortiz > > --- > > drivers/mfd/asic3.c | 99 +++++++++++++++++++--------------------------- > > include/linux/mfd/asic3.h | 34 +++++++++++---- > > 2 files changed, 67 insertions(+), 66 deletions(-) > > > > Index: linux-2.6-htc-asic3/drivers/mfd/asic3.c > > =================================================================== > > --- linux-2.6-htc-asic3.orig/drivers/mfd/asic3.c 2008-05-11 17:47:15.000000000 +0200 > > +++ linux-2.6-htc-asic3/drivers/mfd/asic3.c 2008-05-11 18:20:35.000000000 +0200 > > @@ -465,69 +465,54 @@ static void asic3_gpio_set(struct gpio_c > > return; > > } > > > > -static inline u32 asic3_get_gpio(struct asic3 *asic, unsigned int base, > > - unsigned int function) > > +static int asic3_gpio_probe(struct platform_device *pdev, > > + u16 *gpio_config, int num) > > { > > - return asic3_read_register(asic, base + function); > > -} > > - > > -static void asic3_set_gpio(struct asic3 *asic, unsigned int base, > > - unsigned int function, u32 bits, u32 val) > > -{ > > - unsigned long flags; > > - > > - spin_lock_irqsave(&asic->lock, flags); > > - val |= (asic3_read_register(asic, base + function) & ~bits); > > - > > - asic3_write_register(asic, base + function, val); > > - spin_unlock_irqrestore(&asic->lock, flags); > > -} > > - > > -#define asic3_set_gpio_a(asic, fn, bits, val) \ > > - asic3_set_gpio(asic, ASIC3_GPIO_A_Base, ASIC3_GPIO_##fn, bits, val) > > -#define asic3_set_gpio_b(asic, fn, bits, val) \ > > - asic3_set_gpio(asic, ASIC3_GPIO_B_Base, ASIC3_GPIO_##fn, bits, val) > > -#define asic3_set_gpio_c(asic, fn, bits, val) \ > > - asic3_set_gpio(asic, ASIC3_GPIO_C_Base, ASIC3_GPIO_##fn, bits, val) > > -#define asic3_set_gpio_d(asic, fn, bits, val) \ > > - asic3_set_gpio(asic, ASIC3_GPIO_D_Base, ASIC3_GPIO_##fn, bits, val) > > - > > -#define asic3_set_gpio_banks(asic, fn, bits, pdata, field) \ > > - do { \ > > - asic3_set_gpio_a((asic), fn, (bits), (pdata)->gpio_a.field); \ > > - asic3_set_gpio_b((asic), fn, (bits), (pdata)->gpio_b.field); \ > > - asic3_set_gpio_c((asic), fn, (bits), (pdata)->gpio_c.field); \ > > - asic3_set_gpio_d((asic), fn, (bits), (pdata)->gpio_d.field); \ > > - } while (0) > > - > > - > > -static int asic3_gpio_probe(struct platform_device *pdev) > > -{ > > - struct asic3_platform_data *pdata = pdev->dev.platform_data; > > struct asic3 *asic = platform_get_drvdata(pdev); > > + u16 alt_reg[ASIC3_NUM_GPIO_BANKS]; > > + u16 out_reg[ASIC3_NUM_GPIO_BANKS]; > > + u16 dir_reg[ASIC3_NUM_GPIO_BANKS]; > > + int i; > > + > > + memset(alt_reg, 0, ASIC3_NUM_GPIO_BANKS); > > + memset(out_reg, 0, ASIC3_NUM_GPIO_BANKS); > > + memset(dir_reg, 0, ASIC3_NUM_GPIO_BANKS); > > Those should be > memset(alt_reg, 0, ASIC3_NUM_GPIO_BANKS*sizeof(u16)); > memset(out_reg, 0, ASIC3_NUM_GPIO_BANKS*sizeof(u16)); > memset(dir_reg, 0, ASIC3_NUM_GPIO_BANKS*sizeof(u16)); > > With that change, > Tested-by: Philipp Zabel Ah, right, thanks a lot. I'll forward it to Andrew, whenever I'm done with setting my MFD git tree. > Do you think it would make sense to include constants for the > alternate functions like > #define ASIC3_GPIO32_LED0 ASIC3_CONFIG_GPIO(32,1,1,0) > #define ASIC3_GPIO43_PSKTSEL ASIC3_CONFIG_GPIO(43,1,0,0) > etc. in asic3.h? Hmm, not sure you want to hard code the alt function here. On the universal, most of them are set to 0 it seems. I think it would make sense to have something like that though: #define ASIC3_GPIO15_LCD_POWER5(alt) ASIC3_CONFIG_GPIO(15, (alt), 1, 0) > How does the array look like that you use for Universal? This is my (probably incomplete still) asic3 config for the universal: static u16 asic3_gpio_config[] = { /* Bank A */ ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_LCD_PWR5_ON, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_FLASHLIGHT, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_UNKNOWNA9, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_SPK_PWR2_ON, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_UNKNOWNA4, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_EARPHONE_PWR_ON, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_AUDIO_PWR_ON, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_SPK_PWR1_ON, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_I2C_EN, 1), /* Bank B */ ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_CODEC_PDN, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_LCD_PWR3_ON, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_BB_RESET2, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_CHARGE_EN, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_USB_PUEN, 1), /* Bank C */ ASIC3_CONFIG_GPIO(GPIO_LED_BTWIFI, 1, 1, 0), ASIC3_CONFIG_GPIO(GPIO_LED_RED, 1, 1, 0), ASIC3_CONFIG_GPIO(GPIO_LED_GREEN, 1, 1, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_WIFI_RESET, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_WIFI_PWR1_ON, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_BT_RESET, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_UNKNOWNC8, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_LCD_PWR1_ON, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_LCD_PWR2_ON, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_BT_PWR_ON, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_CHARGE_ON, 1), /* Bank D */ ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_WIFI_PWR2_ON, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_HW_REBOOT, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_BB_RESET1, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_UNKNOWND9, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_VIBRA_PWR_ON, 0), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_WIFI_PWR3_ON, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_FL_PWR_ON, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_LCD_PWR4_ON, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_BL_KEYP_PWR_ON, 1), ASIC3_CONFIG_GPIO_DEFAULT_OUT(GPIO_BL_KEYB_PWR_ON, 1), }; > > > + /* Enable all GPIOs */ > > asic3_write_register(asic, ASIC3_GPIO_OFFSET(A, Mask), 0xffff); > > asic3_write_register(asic, ASIC3_GPIO_OFFSET(B, Mask), 0xffff); > > asic3_write_register(asic, ASIC3_GPIO_OFFSET(C, Mask), 0xffff); > > asic3_write_register(asic, ASIC3_GPIO_OFFSET(D, Mask), 0xffff); > > > > - asic3_set_gpio_a(asic, SleepMask, 0xffff, 0xffff); > > - asic3_set_gpio_b(asic, SleepMask, 0xffff, 0xffff); > > - asic3_set_gpio_c(asic, SleepMask, 0xffff, 0xffff); > > - asic3_set_gpio_d(asic, SleepMask, 0xffff, 0xffff); > > - > > - if (pdata) { > > - asic3_set_gpio_banks(asic, Out, 0xffff, pdata, init); > > - asic3_set_gpio_banks(asic, Direction, 0xffff, pdata, dir); > > - asic3_set_gpio_banks(asic, SleepMask, 0xffff, pdata, > > - sleep_mask); > > - asic3_set_gpio_banks(asic, SleepOut, 0xffff, pdata, sleep_out); > > - asic3_set_gpio_banks(asic, BattFaultOut, 0xffff, pdata, > > - batt_fault_out); > > - asic3_set_gpio_banks(asic, SleepConf, 0xffff, pdata, > > - sleep_conf); > > - asic3_set_gpio_banks(asic, AltFunction, 0xffff, pdata, > > - alt_function); > > + for (i = 0; i < num; i++) { > > + u8 alt, pin, dir, init, bank_num, bit_num; > > + u16 config = gpio_config[i]; > > + > > + pin = ASIC3_CONFIG_GPIO_PIN(config); > > + alt = ASIC3_CONFIG_GPIO_ALT(config); > > + dir = ASIC3_CONFIG_GPIO_DIR(config); > > + init = ASIC3_CONFIG_GPIO_INIT(config); > > + > > + bank_num = ASIC3_GPIO_TO_BANK(pin); > > + bit_num = ASIC3_GPIO_TO_BIT(pin); > > + > > + alt_reg[bank_num] |= (alt << bit_num); > > + out_reg[bank_num] |= (init << bit_num); > > + dir_reg[bank_num] |= (dir << bit_num); > > + } > > + > > + for (i = 0; i < ASIC3_NUM_GPIO_BANKS; i++) { > > + asic3_write_register(asic, > > + ASIC3_BANK_TO_BASE(i) + > > + ASIC3_GPIO_Direction, > > + dir_reg[i]); > > + asic3_write_register(asic, > > + ASIC3_BANK_TO_BASE(i) + ASIC3_GPIO_Out, > > + out_reg[i]); > > + asic3_write_register(asic, > > + ASIC3_BANK_TO_BASE(i) + > > + ASIC3_GPIO_AltFunction, > > + alt_reg[i]); > > } > > > > return gpiochip_add(&asic->gpio); > > @@ -598,7 +583,9 @@ static int asic3_probe(struct platform_d > > asic->gpio.direction_input = asic3_gpio_direction_input; > > asic->gpio.direction_output = asic3_gpio_direction_output; > > > > - ret = asic3_gpio_probe(pdev); > > + ret = asic3_gpio_probe(pdev, > > + pdata->gpio_config, > > + pdata->gpio_config_num); > > if (ret < 0) { > > printk(KERN_ERR "GPIO probe failed\n"); > > goto out_irq; > > Index: linux-2.6-htc-asic3/include/linux/mfd/asic3.h > > =================================================================== > > --- linux-2.6-htc-asic3.orig/include/linux/mfd/asic3.h 2008-05-11 17:47:15.000000000 +0200 > > +++ linux-2.6-htc-asic3/include/linux/mfd/asic3.h 2008-05-11 18:12:07.000000000 +0200 > > @@ -8,7 +8,7 @@ > > * published by the Free Software Foundation. > > * > > * Copyright 2001 Compaq Computer Corporation. > > - * Copyright 2007 OpendHand. > > + * Copyright 2007-2008 OpenedHand Ltd. > > */ > > > > #ifndef __ASIC3_H__ > > @@ -17,15 +17,8 @@ > > #include > > > > struct asic3_platform_data { > > - struct { > > - u32 dir; > > - u32 init; > > - u32 sleep_mask; > > - u32 sleep_out; > > - u32 batt_fault_out; > > - u32 sleep_conf; > > - u32 alt_function; > > - } gpio_a, gpio_b, gpio_c, gpio_d; > > + u16 *gpio_config; > > + unsigned int gpio_config_num; > > > > unsigned int bus_shift; > > > > @@ -86,6 +79,27 @@ struct asic3_platform_data { > > */ > > #define ASIC3_GPIO_Status 0x30 /* R Pin status */ > > > > +/* > > + * ASIC3 GPIO config > > + * > > + * Bits 0..6 gpio number > > + * Bits 7..13 Alternate function > > + * Bit 14 Direction > > + * Bit 15 Initial value > > + * > > + */ > > +#define ASIC3_CONFIG_GPIO_PIN(config) ((config) & 0x7f) > > +#define ASIC3_CONFIG_GPIO_ALT(config) (((config) & (0x7f << 7)) >> 7) > > +#define ASIC3_CONFIG_GPIO_DIR(config) ((config & (1 << 14)) >> 14) > > +#define ASIC3_CONFIG_GPIO_INIT(config) ((config & (1 << 15)) >> 15) > > +#define ASIC3_CONFIG_GPIO(gpio, alt, dir, init) (((gpio) & 0x7f) \ > > + | (((alt) & 0x7f) << 7) | (((dir) & 0x1) << 14) \ > > + | (((init) & 0x1) << 15)) > > +#define ASIC3_CONFIG_GPIO_DEFAULT(gpio, dir, init) \ > > + ASIC3_CONFIG_GPIO((gpio), 0, (dir), (init)) > > +#define ASIC3_CONFIG_GPIO_DEFAULT_OUT(gpio, init) \ > > + ASIC3_CONFIG_GPIO((gpio), 0, 1, (init)) > > + > > #define ASIC3_SPI_Base 0x0400 > > #define ASIC3_SPI_Control 0x0000 > > #define ASIC3_SPI_TxData 0x0004 > > > > -- > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > > > regards > Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/