From: Samuel Ortiz <sameo@openedhand.com>
To: pHilipp Zabel <philipp.zabel@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Linux Kernel ML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH -mm 3/5] asic3: New gpio configuration code
Date: Mon, 23 Jun 2008 12:13:12 +0200 [thread overview]
Message-ID: <20080623101311.GE7008@caravaggio> (raw)
In-Reply-To: <74d0deb30806210743x44f9d68elf2a394902c3fb8ce@mail.gmail.com>
On Sat, Jun 21, 2008 at 04:43:08PM +0200, pHilipp Zabel wrote:
> On Sun, May 11, 2008 at 7:36 PM, Samuel Ortiz <sameo@openedhand.com> 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 <sameo@openedhand.com>
> > ---
> > 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 <philipp.zabel@gmail.com>
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 <linux/types.h>
> >
> > 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
next prev parent reply other threads:[~2008-06-23 10:13 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-05-11 17:36 [PATCH -mm 0/5] ASIC3 updates Samuel Ortiz
2008-05-11 17:36 ` [PATCH -mm 1/5] asic3: gpiolib support Samuel Ortiz
2008-05-11 17:36 ` [PATCH -mm 2/5] asic3: Remove children platform data Samuel Ortiz
2008-05-11 17:36 ` [PATCH -mm 3/5] asic3: New gpio configuration code Samuel Ortiz
2008-06-21 14:43 ` pHilipp Zabel
2008-06-23 10:13 ` Samuel Ortiz [this message]
2008-05-11 17:36 ` [PATCH -mm 4/5] asic3: use dev_* macros Samuel Ortiz
2008-05-11 17:36 ` [PATCH -mm 5/5] asic3: Use uppercase only for macros and defines Samuel Ortiz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20080623101311.GE7008@caravaggio \
--to=sameo@openedhand.com \
--cc=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=philipp.zabel@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.