* [PATCH v2] TWL4030: Initial support for TWL5031
@ 2009-10-16 14:12 Ilkka Koskinen
2009-11-03 11:29 ` Ilkka Koskinen
2009-11-04 16:08 ` Samuel Ortiz
0 siblings, 2 replies; 5+ messages in thread
From: Ilkka Koskinen @ 2009-10-16 14:12 UTC (permalink / raw)
To: linux-kernel, sameo; +Cc: linux-omap, ilkka.koskinen, felipe.balbi
TWL5031 introduces two new interrupts in PIH. Moreover, BCI
has changed remarkably and, thus, it's disabled when TWL5031
is in use.
Signed-off-by: Ilkka Koskinen <ilkka.koskinen@nokia.com>
---
drivers/mfd/twl4030-core.c | 12 ++++-
drivers/mfd/twl4030-irq.c | 127 +++++++++++++++++++++++++++++++++++++++++--
include/linux/i2c/twl4030.h | 47 ++++++++++++++--
3 files changed, 173 insertions(+), 13 deletions(-)
diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c
index 5596bb4..5dc5062 100644
--- a/drivers/mfd/twl4030-core.c
+++ b/drivers/mfd/twl4030-core.c
@@ -152,6 +152,9 @@
#define TWL4030_BASEADD_PWMB 0x00F1
#define TWL4030_BASEADD_KEYPAD 0x00D2
+#define TWL5031_BASEADD_ACCESSORY 0x0074 /* Replaces Main Charge */
+#define TWL5031_BASEADD_INTERRUPTS 0x00B9 /* Different to TWL4030's one */
+
/* subchip/slave 3 - POWER ID */
#define TWL4030_BASEADD_BACKUP 0x0014
#define TWL4030_BASEADD_INT 0x002E
@@ -183,6 +186,7 @@
/* chip-specific feature flags, for i2c_device_id.driver_data */
#define TWL4030_VAUX2 BIT(0) /* pre-5030 voltage ranges */
#define TPS_SUBSET BIT(1) /* tps659[23]0 have fewer LDOs */
+#define TWL5031 BIT(2) /* twl5031 has different registers */
/*----------------------------------------------------------------------*/
@@ -235,6 +239,8 @@ static struct twl4030mapping twl4030_map[TWL4030_MODULE_LAST + 1] = {
{ 2, TWL4030_BASEADD_PWM1 },
{ 2, TWL4030_BASEADD_PWMA },
{ 2, TWL4030_BASEADD_PWMB },
+ { 2, TWL5031_BASEADD_ACCESSORY },
+ { 2, TWL5031_BASEADD_INTERRUPTS },
{ 3, TWL4030_BASEADD_BACKUP },
{ 3, TWL4030_BASEADD_INT },
@@ -483,7 +489,8 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
struct device *child;
struct device *usb_transceiver = NULL;
- if (twl_has_bci() && pdata->bci && !(features & TPS_SUBSET)) {
+ if (twl_has_bci() && pdata->bci &&
+ !((features & TPS_SUBSET) || (features & TWL5031))) {
child = add_child(3, "twl4030_bci",
pdata->bci, sizeof(*pdata->bci),
false,
@@ -743,6 +750,7 @@ static void clocks_init(struct device *dev,
int twl_init_irq(int irq_num, unsigned irq_base, unsigned irq_end);
int twl_exit_irq(void);
+int twl_init_chip_irq(const char *chip);
static int twl4030_remove(struct i2c_client *client)
{
@@ -820,6 +828,7 @@ twl4030_probe(struct i2c_client *client, const struct i2c_device_id *id)
if (client->irq
&& pdata->irq_base
&& pdata->irq_end > pdata->irq_base) {
+ twl_init_chip_irq(id->name);
status = twl_init_irq(client->irq, pdata->irq_base, pdata->irq_end);
if (status < 0)
goto fail;
@@ -835,6 +844,7 @@ fail:
static const struct i2c_device_id twl4030_ids[] = {
{ "twl4030", TWL4030_VAUX2 }, /* "Triton 2" */
{ "twl5030", 0 }, /* T2 updated */
+ { "twl5031", TWL5031 }, /* TWL5030 updated */
{ "tps65950", 0 }, /* catalog version of twl5030 */
{ "tps65930", TPS_SUBSET }, /* fewer LDOs and DACs; no charger */
{ "tps65920", TPS_SUBSET }, /* fewer LDOs; no codec or charger */
diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index fb194fe..a64994e 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -61,6 +61,7 @@
/* Linux could (eventually) use either IRQ line */
static int irq_line;
+static int chip_is_twl5031;
struct sih {
char name[8];
@@ -82,6 +83,9 @@ struct sih {
/* + 2 bytes padding */
};
+static const struct sih *sih_modules;
+static int nr_sih_modules;
+
#define SIH_INITIALIZER(modname, nbits) \
.module = TWL4030_MODULE_ ## modname, \
.control_offset = TWL4030_ ## modname ## _SIH_CTRL, \
@@ -107,7 +111,8 @@ struct sih {
/* Order in this table matches order in PIH_ISR. That is,
* BIT(n) in PIH_ISR is sih_modules[n].
*/
-static const struct sih sih_modules[6] = {
+/* sih_modules_twl4030 is used for twl4030 and twl5030 */
+static const struct sih sih_modules_twl4030[6] = {
[0] = {
.name = "gpio",
.module = TWL4030_MODULE_GPIO,
@@ -164,6 +169,84 @@ static const struct sih sih_modules[6] = {
/* there are no SIH modules #6 or #7 ... */
};
+static const struct sih sih_modules_twl5031[8] = {
+ [0] = {
+ .name = "gpio",
+ .module = TWL4030_MODULE_GPIO,
+ .control_offset = REG_GPIO_SIH_CTRL,
+ .set_cor = true,
+ .bits = TWL4030_GPIO_MAX,
+ .bytes_ixr = 3,
+ /* Note: *all* of these IRQs default to no-trigger */
+ .edr_offset = REG_GPIO_EDR1,
+ .bytes_edr = 5,
+ .mask = { {
+ .isr_offset = REG_GPIO_ISR1A,
+ .imr_offset = REG_GPIO_IMR1A,
+ }, {
+ .isr_offset = REG_GPIO_ISR1B,
+ .imr_offset = REG_GPIO_IMR1B,
+ }, },
+ },
+ [1] = {
+ .name = "keypad",
+ .set_cor = true,
+ SIH_INITIALIZER(KEYPAD_KEYP, 4)
+ },
+ [2] = {
+ .name = "bci",
+ .module = TWL5031_MODULE_INTERRUPTS,
+ .control_offset = TWL5031_INTERRUPTS_BCISIHCTRL,
+ .bits = 7,
+ .bytes_ixr = 1,
+ .edr_offset = TWL5031_INTERRUPTS_BCIEDR1,
+ /* Note: most of these IRQs default to no-trigger */
+ .bytes_edr = 2,
+ .mask = { {
+ .isr_offset = TWL5031_INTERRUPTS_BCIISR1,
+ .imr_offset = TWL5031_INTERRUPTS_BCIIMR1,
+ }, {
+ .isr_offset = TWL5031_INTERRUPTS_BCIISR2,
+ .imr_offset = TWL5031_INTERRUPTS_BCIIMR2,
+ }, },
+ },
+ [3] = {
+ .name = "madc",
+ SIH_INITIALIZER(MADC, 4)
+ },
+ [4] = {
+ /* USB doesn't use the same SIH organization */
+ .name = "usb",
+ },
+ [5] = {
+ .name = "power",
+ .set_cor = true,
+ SIH_INITIALIZER(INT_PWR, 8)
+ },
+ [6] = {
+ /* ACI doesn't use the same SIH organization */
+ .name = "aci",
+ },
+ [7] = {
+ /* Accessory */
+ .name = "acc",
+ .module = TWL5031_MODULE_ACCESSORY,
+ .control_offset = TWL5031_ACCSIHCTRL,
+ .bits = 2,
+ .bytes_ixr = 1,
+ .edr_offset = TWL5031_ACCEDR1,
+ /* Note: most of these IRQs default to no-trigger */
+ .bytes_edr = 1,
+ .mask = { {
+ .isr_offset = TWL5031_ACCISR1,
+ .imr_offset = TWL5031_ACCIMR1,
+ }, {
+ .isr_offset = TWL5031_ACCISR2,
+ .imr_offset = TWL5031_ACCIMR2,
+ }, },
+ },
+};
+
#undef TWL4030_MODULE_KEYPAD_KEYP
#undef TWL4030_MODULE_INT_PWR
#undef TWL4030_INT_PWR_EDR
@@ -284,11 +367,20 @@ static int twl4030_init_sih_modules(unsigned line)
/* disable all interrupts on our line */
memset(buf, 0xff, sizeof buf);
sih = sih_modules;
- for (i = 0; i < ARRAY_SIZE(sih_modules); i++, sih++) {
+ for (i = 0; i < nr_sih_modules; i++, sih++) {
/* skip USB -- it's funky */
- if (!sih->bytes_ixr)
+ /* But don't skip TWL5031's ACI */
+ if (!sih->bytes_ixr) {
+ if (chip_is_twl5031 && !strcmp(sih->name, "aci")) {
+ twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
+ buf, TWL5031_ACIIMR_LSB, 1);
+ twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
+ buf, TWL5031_ACIIMR_MSB, 1);
+ }
+
continue;
+ }
status = twl4030_i2c_write(sih->module, buf,
sih->mask[line].imr_offset, sih->bytes_ixr);
@@ -314,13 +406,22 @@ static int twl4030_init_sih_modules(unsigned line)
}
sih = sih_modules;
- for (i = 0; i < ARRAY_SIZE(sih_modules); i++, sih++) {
+ for (i = 0; i < nr_sih_modules; i++, sih++) {
u8 rxbuf[4];
int j;
/* skip USB */
- if (!sih->bytes_ixr)
+ /* But don't skip TWL5031's ACI */
+ if (!sih->bytes_ixr) {
+ if (chip_is_twl5031 && !strcmp(sih->name, "aci")) {
+ twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
+ buf, TWL5031_ACIIDR_LSB, 1);
+ twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
+ buf, TWL5031_ACIIDR_MSB, 1);
+ }
+
continue;
+ }
/* Clear pending interrupt status. Either the read was
* enough, or we need to write those bits. Repeat, in
@@ -611,7 +712,7 @@ int twl4030_sih_setup(int module)
/* only support modules with standard clear-on-read for now */
for (sih_mod = 0, sih = sih_modules;
- sih_mod < ARRAY_SIZE(sih_modules);
+ sih_mod < nr_sih_modules;
sih_mod++, sih++) {
if (sih->module == module && sih->set_cor) {
if (!WARN((irq_base + sih->bits) > NR_IRQS,
@@ -756,3 +857,17 @@ int twl_exit_irq(void)
}
return 0;
}
+
+int twl_init_chip_irq(const char *chip)
+{
+ if (!strcmp(chip, "twl5031")) {
+ chip_is_twl5031 = 1;
+ sih_modules = sih_modules_twl5031;
+ nr_sih_modules = ARRAY_SIZE(sih_modules_twl5031);
+ } else {
+ sih_modules = sih_modules_twl4030;
+ nr_sih_modules = ARRAY_SIZE(sih_modules_twl4030);
+ }
+
+ return 0;
+}
diff --git a/include/linux/i2c/twl4030.h b/include/linux/i2c/twl4030.h
index c8d5078..3fd5791 100644
--- a/include/linux/i2c/twl4030.h
+++ b/include/linux/i2c/twl4030.h
@@ -61,13 +61,16 @@
#define TWL4030_MODULE_PWMA 0x0E
#define TWL4030_MODULE_PWMB 0x0F
+#define TWL5031_MODULE_ACCESSORY 0x10
+#define TWL5031_MODULE_INTERRUPTS 0x11
+
/* Slave 3 (i2c address 0x4b) */
-#define TWL4030_MODULE_BACKUP 0x10
-#define TWL4030_MODULE_INT 0x11
-#define TWL4030_MODULE_PM_MASTER 0x12
-#define TWL4030_MODULE_PM_RECEIVER 0x13
-#define TWL4030_MODULE_RTC 0x14
-#define TWL4030_MODULE_SECURED_REG 0x15
+#define TWL4030_MODULE_BACKUP 0x12
+#define TWL4030_MODULE_INT 0x13
+#define TWL4030_MODULE_PM_MASTER 0x14
+#define TWL4030_MODULE_PM_RECEIVER 0x15
+#define TWL4030_MODULE_RTC 0x16
+#define TWL4030_MODULE_SECURED_REG 0x17
/*
* Read and write single 8-bit registers
@@ -221,6 +224,38 @@ int twl4030_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes);
/*----------------------------------------------------------------------*/
+/*
+ * Accessory Interrupts
+ */
+#define TWL5031_ACIIMR_LSB 0x05
+#define TWL5031_ACIIMR_MSB 0x06
+#define TWL5031_ACIIDR_LSB 0x07
+#define TWL5031_ACIIDR_MSB 0x08
+#define TWL5031_ACCISR1 0x0F
+#define TWL5031_ACCIMR1 0x10
+#define TWL5031_ACCISR2 0x11
+#define TWL5031_ACCIMR2 0x12
+#define TWL5031_ACCSIR 0x13
+#define TWL5031_ACCEDR1 0x14
+#define TWL5031_ACCSIHCTRL 0x15
+
+/*----------------------------------------------------------------------*/
+
+/*
+ * Battery Charger Controller
+ */
+
+#define TWL5031_INTERRUPTS_BCIISR1 0x0
+#define TWL5031_INTERRUPTS_BCIIMR1 0x1
+#define TWL5031_INTERRUPTS_BCIISR2 0x2
+#define TWL5031_INTERRUPTS_BCIIMR2 0x3
+#define TWL5031_INTERRUPTS_BCISIR 0x4
+#define TWL5031_INTERRUPTS_BCIEDR1 0x5
+#define TWL5031_INTERRUPTS_BCIEDR2 0x6
+#define TWL5031_INTERRUPTS_BCISIHCTRL 0x7
+
+/*----------------------------------------------------------------------*/
+
/* Power bus message definitions */
/* The TWL4030/5030 splits its power-management resources (the various
--
1.6.0.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] TWL4030: Initial support for TWL5031
2009-10-16 14:12 [PATCH v2] TWL4030: Initial support for TWL5031 Ilkka Koskinen
@ 2009-11-03 11:29 ` Ilkka Koskinen
2009-11-04 16:08 ` Samuel Ortiz
1 sibling, 0 replies; 5+ messages in thread
From: Ilkka Koskinen @ 2009-11-03 11:29 UTC (permalink / raw)
To: Samuel Ortiz, linux-kernel; +Cc: linux-omap
Hi Samuel,
On Fri, 16 Oct 2009, Koskinen Ilkka (Nokia-D/Tampere) wrote:
> TWL5031 introduces two new interrupts in PIH. Moreover, BCI
> has changed remarkably and, thus, it's disabled when TWL5031
> is in use.
>
> Signed-off-by: Ilkka Koskinen <ilkka.koskinen@nokia.com>
> ---
> drivers/mfd/twl4030-core.c | 12 ++++-
> drivers/mfd/twl4030-irq.c | 127 +++++++++++++++++++++++++++++++++++++++++--
> include/linux/i2c/twl4030.h | 47 ++++++++++++++--
> 3 files changed, 173 insertions(+), 13 deletions(-)
Sorry to bothering you again, but I'd like to know the status
of this patch. Does it need more work or could you apply it
to your tree at some point?
Cheers, Ilkka
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] TWL4030: Initial support for TWL5031
2009-10-16 14:12 [PATCH v2] TWL4030: Initial support for TWL5031 Ilkka Koskinen
2009-11-03 11:29 ` Ilkka Koskinen
@ 2009-11-04 16:08 ` Samuel Ortiz
2009-11-05 9:08 ` Kauppi Ari (EXT-Ixonos/Oulu)
2009-11-06 14:32 ` Ilkka Koskinen
1 sibling, 2 replies; 5+ messages in thread
From: Samuel Ortiz @ 2009-11-04 16:08 UTC (permalink / raw)
To: Ilkka Koskinen; +Cc: linux-kernel, linux-omap, felipe.balbi
Hi Ilkka,
Some comments below:
On Fri, Oct 16, 2009 at 05:12:40PM +0300, Ilkka Koskinen wrote:
> TWL5031 introduces two new interrupts in PIH. Moreover, BCI
> has changed remarkably and, thus, it's disabled when TWL5031
> is in use.
>
> Signed-off-by: Ilkka Koskinen <ilkka.koskinen@nokia.com>
> ---
> drivers/mfd/twl4030-core.c | 12 ++++-
> drivers/mfd/twl4030-irq.c | 127 +++++++++++++++++++++++++++++++++++++++++--
> include/linux/i2c/twl4030.h | 47 ++++++++++++++--
> 3 files changed, 173 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c
> index 5596bb4..5dc5062 100644
> --- a/drivers/mfd/twl4030-core.c
> +++ b/drivers/mfd/twl4030-core.c
> @@ -152,6 +152,9 @@
> #define TWL4030_BASEADD_PWMB 0x00F1
> #define TWL4030_BASEADD_KEYPAD 0x00D2
>
> +#define TWL5031_BASEADD_ACCESSORY 0x0074 /* Replaces Main Charge */
> +#define TWL5031_BASEADD_INTERRUPTS 0x00B9 /* Different to TWL4030's one */
Should be "Different than..."
> +
> /* subchip/slave 3 - POWER ID */
> #define TWL4030_BASEADD_BACKUP 0x0014
> #define TWL4030_BASEADD_INT 0x002E
> @@ -183,6 +186,7 @@
> /* chip-specific feature flags, for i2c_device_id.driver_data */
> #define TWL4030_VAUX2 BIT(0) /* pre-5030 voltage ranges */
> #define TPS_SUBSET BIT(1) /* tps659[23]0 have fewer LDOs */
> +#define TWL5031 BIT(2) /* twl5031 has different registers */
>
> /*----------------------------------------------------------------------*/
>
> @@ -235,6 +239,8 @@ static struct twl4030mapping twl4030_map[TWL4030_MODULE_LAST + 1] = {
> { 2, TWL4030_BASEADD_PWM1 },
> { 2, TWL4030_BASEADD_PWMA },
> { 2, TWL4030_BASEADD_PWMB },
> + { 2, TWL5031_BASEADD_ACCESSORY },
> + { 2, TWL5031_BASEADD_INTERRUPTS },
>
> { 3, TWL4030_BASEADD_BACKUP },
> { 3, TWL4030_BASEADD_INT },
> @@ -483,7 +489,8 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
> struct device *child;
> struct device *usb_transceiver = NULL;
>
> - if (twl_has_bci() && pdata->bci && !(features & TPS_SUBSET)) {
> + if (twl_has_bci() && pdata->bci &&
> + !((features & TPS_SUBSET) || (features & TWL5031))) {
could be simpler: !((features & (TPS_SUBSET | TWL5031))
> child = add_child(3, "twl4030_bci",
> pdata->bci, sizeof(*pdata->bci),
> false,
> @@ -743,6 +750,7 @@ static void clocks_init(struct device *dev,
>
> int twl_init_irq(int irq_num, unsigned irq_base, unsigned irq_end);
> int twl_exit_irq(void);
> +int twl_init_chip_irq(const char *chip);
>
> static int twl4030_remove(struct i2c_client *client)
> {
> @@ -820,6 +828,7 @@ twl4030_probe(struct i2c_client *client, const struct i2c_device_id *id)
> if (client->irq
> && pdata->irq_base
> && pdata->irq_end > pdata->irq_base) {
> + twl_init_chip_irq(id->name);
> status = twl_init_irq(client->irq, pdata->irq_base, pdata->irq_end);
> if (status < 0)
> goto fail;
> @@ -835,6 +844,7 @@ fail:
> static const struct i2c_device_id twl4030_ids[] = {
> { "twl4030", TWL4030_VAUX2 }, /* "Triton 2" */
> { "twl5030", 0 }, /* T2 updated */
> + { "twl5031", TWL5031 }, /* TWL5030 updated */
> { "tps65950", 0 }, /* catalog version of twl5030 */
> { "tps65930", TPS_SUBSET }, /* fewer LDOs and DACs; no charger */
> { "tps65920", TPS_SUBSET }, /* fewer LDOs; no codec or charger */
> diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
> index fb194fe..a64994e 100644
> --- a/drivers/mfd/twl4030-irq.c
> +++ b/drivers/mfd/twl4030-irq.c
> @@ -61,6 +61,7 @@
>
> /* Linux could (eventually) use either IRQ line */
> static int irq_line;
> +static int chip_is_twl5031;
>
> struct sih {
> char name[8];
> @@ -82,6 +83,9 @@ struct sih {
> /* + 2 bytes padding */
> };
>
> +static const struct sih *sih_modules;
> +static int nr_sih_modules;
> +
> #define SIH_INITIALIZER(modname, nbits) \
> .module = TWL4030_MODULE_ ## modname, \
> .control_offset = TWL4030_ ## modname ## _SIH_CTRL, \
> @@ -107,7 +111,8 @@ struct sih {
> /* Order in this table matches order in PIH_ISR. That is,
> * BIT(n) in PIH_ISR is sih_modules[n].
> */
> -static const struct sih sih_modules[6] = {
> +/* sih_modules_twl4030 is used for twl4030 and twl5030 */
> +static const struct sih sih_modules_twl4030[6] = {
> [0] = {
> .name = "gpio",
> .module = TWL4030_MODULE_GPIO,
> @@ -164,6 +169,84 @@ static const struct sih sih_modules[6] = {
> /* there are no SIH modules #6 or #7 ... */
> };
>
> +static const struct sih sih_modules_twl5031[8] = {
> + [0] = {
> + .name = "gpio",
> + .module = TWL4030_MODULE_GPIO,
> + .control_offset = REG_GPIO_SIH_CTRL,
> + .set_cor = true,
> + .bits = TWL4030_GPIO_MAX,
> + .bytes_ixr = 3,
> + /* Note: *all* of these IRQs default to no-trigger */
> + .edr_offset = REG_GPIO_EDR1,
> + .bytes_edr = 5,
> + .mask = { {
> + .isr_offset = REG_GPIO_ISR1A,
> + .imr_offset = REG_GPIO_IMR1A,
> + }, {
> + .isr_offset = REG_GPIO_ISR1B,
> + .imr_offset = REG_GPIO_IMR1B,
> + }, },
> + },
> + [1] = {
> + .name = "keypad",
> + .set_cor = true,
> + SIH_INITIALIZER(KEYPAD_KEYP, 4)
> + },
> + [2] = {
> + .name = "bci",
> + .module = TWL5031_MODULE_INTERRUPTS,
> + .control_offset = TWL5031_INTERRUPTS_BCISIHCTRL,
> + .bits = 7,
> + .bytes_ixr = 1,
> + .edr_offset = TWL5031_INTERRUPTS_BCIEDR1,
> + /* Note: most of these IRQs default to no-trigger */
> + .bytes_edr = 2,
> + .mask = { {
> + .isr_offset = TWL5031_INTERRUPTS_BCIISR1,
> + .imr_offset = TWL5031_INTERRUPTS_BCIIMR1,
> + }, {
> + .isr_offset = TWL5031_INTERRUPTS_BCIISR2,
> + .imr_offset = TWL5031_INTERRUPTS_BCIIMR2,
> + }, },
> + },
> + [3] = {
> + .name = "madc",
> + SIH_INITIALIZER(MADC, 4)
> + },
> + [4] = {
> + /* USB doesn't use the same SIH organization */
> + .name = "usb",
> + },
> + [5] = {
> + .name = "power",
> + .set_cor = true,
> + SIH_INITIALIZER(INT_PWR, 8)
> + },
> + [6] = {
> + /* ACI doesn't use the same SIH organization */
> + .name = "aci",
> + },
> + [7] = {
> + /* Accessory */
> + .name = "acc",
> + .module = TWL5031_MODULE_ACCESSORY,
> + .control_offset = TWL5031_ACCSIHCTRL,
> + .bits = 2,
> + .bytes_ixr = 1,
> + .edr_offset = TWL5031_ACCEDR1,
> + /* Note: most of these IRQs default to no-trigger */
> + .bytes_edr = 1,
> + .mask = { {
> + .isr_offset = TWL5031_ACCISR1,
> + .imr_offset = TWL5031_ACCIMR1,
> + }, {
> + .isr_offset = TWL5031_ACCISR2,
> + .imr_offset = TWL5031_ACCIMR2,
> + }, },
> + },
> +};
> +
> #undef TWL4030_MODULE_KEYPAD_KEYP
> #undef TWL4030_MODULE_INT_PWR
> #undef TWL4030_INT_PWR_EDR
> @@ -284,11 +367,20 @@ static int twl4030_init_sih_modules(unsigned line)
> /* disable all interrupts on our line */
> memset(buf, 0xff, sizeof buf);
> sih = sih_modules;
> - for (i = 0; i < ARRAY_SIZE(sih_modules); i++, sih++) {
> + for (i = 0; i < nr_sih_modules; i++, sih++) {
>
> /* skip USB -- it's funky */
> - if (!sih->bytes_ixr)
> + /* But don't skip TWL5031's ACI */
> + if (!sih->bytes_ixr) {
> + if (chip_is_twl5031 && !strcmp(sih->name, "aci")) {
> + twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
> + buf, TWL5031_ACIIMR_LSB, 1);
> + twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
> + buf, TWL5031_ACIIMR_MSB, 1);
> + }
> +
A few comments on this:
- Why do we have to do that for the accessory ? Some comments would be
appropriate.
- You could use twl4030_i2c_write_u8(), couldnt you ?
- I'd like it to be more generic: TWL5031_MODULE_ACCESSORY should be replaced
by sih->module, and the TWL5031_ACII* register should be somehow integrated in
the sih struct. The idea is to get rid of this chip_is_twl5031.
> continue;
> + }
>
> status = twl4030_i2c_write(sih->module, buf,
> sih->mask[line].imr_offset, sih->bytes_ixr);
> @@ -314,13 +406,22 @@ static int twl4030_init_sih_modules(unsigned line)
> }
>
> sih = sih_modules;
> - for (i = 0; i < ARRAY_SIZE(sih_modules); i++, sih++) {
> + for (i = 0; i < nr_sih_modules; i++, sih++) {
> u8 rxbuf[4];
> int j;
>
> /* skip USB */
> - if (!sih->bytes_ixr)
> + /* But don't skip TWL5031's ACI */
> + if (!sih->bytes_ixr) {
> + if (chip_is_twl5031 && !strcmp(sih->name, "aci")) {
> + twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
> + buf, TWL5031_ACIIDR_LSB, 1);
> + twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
> + buf, TWL5031_ACIIDR_MSB, 1);
> + }
> +
> continue;
> + }
>
> /* Clear pending interrupt status. Either the read was
> * enough, or we need to write those bits. Repeat, in
> @@ -611,7 +712,7 @@ int twl4030_sih_setup(int module)
>
> /* only support modules with standard clear-on-read for now */
> for (sih_mod = 0, sih = sih_modules;
> - sih_mod < ARRAY_SIZE(sih_modules);
> + sih_mod < nr_sih_modules;
> sih_mod++, sih++) {
> if (sih->module == module && sih->set_cor) {
> if (!WARN((irq_base + sih->bits) > NR_IRQS,
> @@ -756,3 +857,17 @@ int twl_exit_irq(void)
> }
> return 0;
> }
> +
> +int twl_init_chip_irq(const char *chip)
> +{
> + if (!strcmp(chip, "twl5031")) {
> + chip_is_twl5031 = 1;
> + sih_modules = sih_modules_twl5031;
> + nr_sih_modules = ARRAY_SIZE(sih_modules_twl5031);
I dont think you need nr_sih_modules at all. sih_modules is pointing to the
right array, and then you can keep the ARRAY_SIZE(sih_modules) call instead of
defining yet another static variable.
Talking about static variables and such, I really think the twl driver needs
some cleanup. It should really be allocating a private device structure at
probe time containing all those static crap. As this driver is growing and
supporting more and more HW, it starts to be messy.
Cheers,
Samuel.
> + } else {
> + sih_modules = sih_modules_twl4030;
> + nr_sih_modules = ARRAY_SIZE(sih_modules_twl4030);
> + }
> +
> + return 0;
> +}
> diff --git a/include/linux/i2c/twl4030.h b/include/linux/i2c/twl4030.h
> index c8d5078..3fd5791 100644
> --- a/include/linux/i2c/twl4030.h
> +++ b/include/linux/i2c/twl4030.h
> @@ -61,13 +61,16 @@
> #define TWL4030_MODULE_PWMA 0x0E
> #define TWL4030_MODULE_PWMB 0x0F
>
> +#define TWL5031_MODULE_ACCESSORY 0x10
> +#define TWL5031_MODULE_INTERRUPTS 0x11
> +
> /* Slave 3 (i2c address 0x4b) */
> -#define TWL4030_MODULE_BACKUP 0x10
> -#define TWL4030_MODULE_INT 0x11
> -#define TWL4030_MODULE_PM_MASTER 0x12
> -#define TWL4030_MODULE_PM_RECEIVER 0x13
> -#define TWL4030_MODULE_RTC 0x14
> -#define TWL4030_MODULE_SECURED_REG 0x15
> +#define TWL4030_MODULE_BACKUP 0x12
> +#define TWL4030_MODULE_INT 0x13
> +#define TWL4030_MODULE_PM_MASTER 0x14
> +#define TWL4030_MODULE_PM_RECEIVER 0x15
> +#define TWL4030_MODULE_RTC 0x16
> +#define TWL4030_MODULE_SECURED_REG 0x17
>
> /*
> * Read and write single 8-bit registers
> @@ -221,6 +224,38 @@ int twl4030_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes);
>
> /*----------------------------------------------------------------------*/
>
> +/*
> + * Accessory Interrupts
> + */
> +#define TWL5031_ACIIMR_LSB 0x05
> +#define TWL5031_ACIIMR_MSB 0x06
> +#define TWL5031_ACIIDR_LSB 0x07
> +#define TWL5031_ACIIDR_MSB 0x08
> +#define TWL5031_ACCISR1 0x0F
> +#define TWL5031_ACCIMR1 0x10
> +#define TWL5031_ACCISR2 0x11
> +#define TWL5031_ACCIMR2 0x12
> +#define TWL5031_ACCSIR 0x13
> +#define TWL5031_ACCEDR1 0x14
> +#define TWL5031_ACCSIHCTRL 0x15
> +
> +/*----------------------------------------------------------------------*/
> +
> +/*
> + * Battery Charger Controller
> + */
> +
> +#define TWL5031_INTERRUPTS_BCIISR1 0x0
> +#define TWL5031_INTERRUPTS_BCIIMR1 0x1
> +#define TWL5031_INTERRUPTS_BCIISR2 0x2
> +#define TWL5031_INTERRUPTS_BCIIMR2 0x3
> +#define TWL5031_INTERRUPTS_BCISIR 0x4
> +#define TWL5031_INTERRUPTS_BCIEDR1 0x5
> +#define TWL5031_INTERRUPTS_BCIEDR2 0x6
> +#define TWL5031_INTERRUPTS_BCISIHCTRL 0x7
> +
> +/*----------------------------------------------------------------------*/
> +
> /* Power bus message definitions */
>
> /* The TWL4030/5030 splits its power-management resources (the various
> --
> 1.6.0.4
>
--
Intel Open Source Technology Centre
http://oss.intel.com/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] TWL4030: Initial support for TWL5031
2009-11-04 16:08 ` Samuel Ortiz
@ 2009-11-05 9:08 ` Kauppi Ari (EXT-Ixonos/Oulu)
2009-11-06 14:32 ` Ilkka Koskinen
1 sibling, 0 replies; 5+ messages in thread
From: Kauppi Ari (EXT-Ixonos/Oulu) @ 2009-11-05 9:08 UTC (permalink / raw)
To: Koskinen Ilkka (Nokia-D/Tampere)
Cc: linux-kernel, linux-omap, Balbi Felipe (Nokia-D/Helsinki),
ext Samuel Ortiz
On Wed, 2009-11-04 at 17:08 +0100, ext Samuel Ortiz wrote:
> Hi Ilkka,
>
> Some comments below:
One additional comment. Sorry for replying to Samuel's email, I don't
have the original one.
> On Fri, Oct 16, 2009 at 05:12:40PM +0300, Ilkka Koskinen wrote:
> > TWL5031 introduces two new interrupts in PIH. Moreover, BCI
> > has changed remarkably and, thus, it's disabled when TWL5031
> > is in use.
> >
> > Signed-off-by: Ilkka Koskinen <ilkka.koskinen@nokia.com>
> > ---
> > drivers/mfd/twl4030-core.c | 12 ++++-
> > drivers/mfd/twl4030-irq.c | 127 +++++++++++++++++++++++++++++++++++++++++--
> > include/linux/i2c/twl4030.h | 47 ++++++++++++++--
> > 3 files changed, 173 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c
> > index 5596bb4..5dc5062 100644
> > --- a/drivers/mfd/twl4030-core.c
> > +++ b/drivers/mfd/twl4030-core.c
> > @@ -152,6 +152,9 @@
> > #define TWL4030_BASEADD_PWMB 0x00F1
> > #define TWL4030_BASEADD_KEYPAD 0x00D2
> >
> > +#define TWL5031_BASEADD_ACCESSORY 0x0074 /* Replaces Main Charge */
> > +#define TWL5031_BASEADD_INTERRUPTS 0x00B9 /* Different to TWL4030's one */
> Should be "Different than..."
>
>
> > +
> > /* subchip/slave 3 - POWER ID */
> > #define TWL4030_BASEADD_BACKUP 0x0014
> > #define TWL4030_BASEADD_INT 0x002E
> > @@ -183,6 +186,7 @@
> > /* chip-specific feature flags, for i2c_device_id.driver_data */
> > #define TWL4030_VAUX2 BIT(0) /* pre-5030 voltage ranges */
> > #define TPS_SUBSET BIT(1) /* tps659[23]0 have fewer LDOs */
> > +#define TWL5031 BIT(2) /* twl5031 has different registers */
> >
> > /*----------------------------------------------------------------------*/
> >
> > @@ -235,6 +239,8 @@ static struct twl4030mapping twl4030_map[TWL4030_MODULE_LAST + 1] = {
> > { 2, TWL4030_BASEADD_PWM1 },
> > { 2, TWL4030_BASEADD_PWMA },
> > { 2, TWL4030_BASEADD_PWMB },
> > + { 2, TWL5031_BASEADD_ACCESSORY },
> > + { 2, TWL5031_BASEADD_INTERRUPTS },
> >
> > { 3, TWL4030_BASEADD_BACKUP },
> > { 3, TWL4030_BASEADD_INT },
> > @@ -483,7 +489,8 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
> > struct device *child;
> > struct device *usb_transceiver = NULL;
> >
> > - if (twl_has_bci() && pdata->bci && !(features & TPS_SUBSET)) {
> > + if (twl_has_bci() && pdata->bci &&
> > + !((features & TPS_SUBSET) || (features & TWL5031))) {
> could be simpler: !((features & (TPS_SUBSET | TWL5031))
>
>
> > child = add_child(3, "twl4030_bci",
> > pdata->bci, sizeof(*pdata->bci),
> > false,
> > @@ -743,6 +750,7 @@ static void clocks_init(struct device *dev,
> >
> > int twl_init_irq(int irq_num, unsigned irq_base, unsigned irq_end);
> > int twl_exit_irq(void);
> > +int twl_init_chip_irq(const char *chip);
> >
> > static int twl4030_remove(struct i2c_client *client)
> > {
> > @@ -820,6 +828,7 @@ twl4030_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > if (client->irq
> > && pdata->irq_base
> > && pdata->irq_end > pdata->irq_base) {
> > + twl_init_chip_irq(id->name);
> > status = twl_init_irq(client->irq, pdata->irq_base, pdata->irq_end);
> > if (status < 0)
> > goto fail;
> > @@ -835,6 +844,7 @@ fail:
> > static const struct i2c_device_id twl4030_ids[] = {
> > { "twl4030", TWL4030_VAUX2 }, /* "Triton 2" */
> > { "twl5030", 0 }, /* T2 updated */
> > + { "twl5031", TWL5031 }, /* TWL5030 updated */
> > { "tps65950", 0 }, /* catalog version of twl5030 */
> > { "tps65930", TPS_SUBSET }, /* fewer LDOs and DACs; no charger */
> > { "tps65920", TPS_SUBSET }, /* fewer LDOs; no codec or charger */
> > diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
> > index fb194fe..a64994e 100644
> > --- a/drivers/mfd/twl4030-irq.c
> > +++ b/drivers/mfd/twl4030-irq.c
> > @@ -61,6 +61,7 @@
> >
> > /* Linux could (eventually) use either IRQ line */
> > static int irq_line;
> > +static int chip_is_twl5031;
> >
> > struct sih {
> > char name[8];
> > @@ -82,6 +83,9 @@ struct sih {
> > /* + 2 bytes padding */
> > };
> >
> > +static const struct sih *sih_modules;
> > +static int nr_sih_modules;
> > +
> > #define SIH_INITIALIZER(modname, nbits) \
> > .module = TWL4030_MODULE_ ## modname, \
> > .control_offset = TWL4030_ ## modname ## _SIH_CTRL, \
> > @@ -107,7 +111,8 @@ struct sih {
> > /* Order in this table matches order in PIH_ISR. That is,
> > * BIT(n) in PIH_ISR is sih_modules[n].
> > */
> > -static const struct sih sih_modules[6] = {
> > +/* sih_modules_twl4030 is used for twl4030 and twl5030 */
> > +static const struct sih sih_modules_twl4030[6] = {
> > [0] = {
> > .name = "gpio",
> > .module = TWL4030_MODULE_GPIO,
> > @@ -164,6 +169,84 @@ static const struct sih sih_modules[6] = {
> > /* there are no SIH modules #6 or #7 ... */
> > };
> >
> > +static const struct sih sih_modules_twl5031[8] = {
> > + [0] = {
> > + .name = "gpio",
> > + .module = TWL4030_MODULE_GPIO,
> > + .control_offset = REG_GPIO_SIH_CTRL,
> > + .set_cor = true,
> > + .bits = TWL4030_GPIO_MAX,
> > + .bytes_ixr = 3,
> > + /* Note: *all* of these IRQs default to no-trigger */
> > + .edr_offset = REG_GPIO_EDR1,
> > + .bytes_edr = 5,
> > + .mask = { {
> > + .isr_offset = REG_GPIO_ISR1A,
> > + .imr_offset = REG_GPIO_IMR1A,
> > + }, {
> > + .isr_offset = REG_GPIO_ISR1B,
> > + .imr_offset = REG_GPIO_IMR1B,
> > + }, },
> > + },
> > + [1] = {
> > + .name = "keypad",
> > + .set_cor = true,
> > + SIH_INITIALIZER(KEYPAD_KEYP, 4)
> > + },
> > + [2] = {
> > + .name = "bci",
> > + .module = TWL5031_MODULE_INTERRUPTS,
> > + .control_offset = TWL5031_INTERRUPTS_BCISIHCTRL,
> > + .bits = 7,
> > + .bytes_ixr = 1,
> > + .edr_offset = TWL5031_INTERRUPTS_BCIEDR1,
> > + /* Note: most of these IRQs default to no-trigger */
> > + .bytes_edr = 2,
> > + .mask = { {
> > + .isr_offset = TWL5031_INTERRUPTS_BCIISR1,
> > + .imr_offset = TWL5031_INTERRUPTS_BCIIMR1,
> > + }, {
> > + .isr_offset = TWL5031_INTERRUPTS_BCIISR2,
> > + .imr_offset = TWL5031_INTERRUPTS_BCIIMR2,
> > + }, },
> > + },
> > + [3] = {
> > + .name = "madc",
> > + SIH_INITIALIZER(MADC, 4)
> > + },
> > + [4] = {
> > + /* USB doesn't use the same SIH organization */
> > + .name = "usb",
> > + },
> > + [5] = {
> > + .name = "power",
> > + .set_cor = true,
> > + SIH_INITIALIZER(INT_PWR, 8)
> > + },
> > + [6] = {
> > + /* ACI doesn't use the same SIH organization */
> > + .name = "aci",
> > + },
> > + [7] = {
> > + /* Accessory */
> > + .name = "acc",
> > + .module = TWL5031_MODULE_ACCESSORY,
> > + .control_offset = TWL5031_ACCSIHCTRL,
> > + .bits = 2,
> > + .bytes_ixr = 1,
> > + .edr_offset = TWL5031_ACCEDR1,
> > + /* Note: most of these IRQs default to no-trigger */
> > + .bytes_edr = 1,
> > + .mask = { {
> > + .isr_offset = TWL5031_ACCISR1,
> > + .imr_offset = TWL5031_ACCIMR1,
> > + }, {
> > + .isr_offset = TWL5031_ACCISR2,
> > + .imr_offset = TWL5031_ACCIMR2,
> > + }, },
> > + },
> > +};
> > +
> > #undef TWL4030_MODULE_KEYPAD_KEYP
> > #undef TWL4030_MODULE_INT_PWR
> > #undef TWL4030_INT_PWR_EDR
> > @@ -284,11 +367,20 @@ static int twl4030_init_sih_modules(unsigned line)
> > /* disable all interrupts on our line */
> > memset(buf, 0xff, sizeof buf);
> > sih = sih_modules;
> > - for (i = 0; i < ARRAY_SIZE(sih_modules); i++, sih++) {
> > + for (i = 0; i < nr_sih_modules; i++, sih++) {
> >
> > /* skip USB -- it's funky */
> > - if (!sih->bytes_ixr)
> > + /* But don't skip TWL5031's ACI */
> > + if (!sih->bytes_ixr) {
> > + if (chip_is_twl5031 && !strcmp(sih->name, "aci")) {
> > + twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
> > + buf, TWL5031_ACIIMR_LSB, 1);
> > + twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
> > + buf, TWL5031_ACIIMR_MSB, 1);
> > + }
> > +
> A few comments on this:
> - Why do we have to do that for the accessory ? Some comments would be
> appropriate.
> - You could use twl4030_i2c_write_u8(), couldnt you ?
> - I'd like it to be more generic: TWL5031_MODULE_ACCESSORY should be replaced
> by sih->module, and the TWL5031_ACII* register should be somehow integrated in
> the sih struct. The idea is to get rid of this chip_is_twl5031.
> > continue;
> > + }
> >
> > status = twl4030_i2c_write(sih->module, buf,
> > sih->mask[line].imr_offset, sih->bytes_ixr);
> > @@ -314,13 +406,22 @@ static int twl4030_init_sih_modules(unsigned line)
> > }
> >
> > sih = sih_modules;
> > - for (i = 0; i < ARRAY_SIZE(sih_modules); i++, sih++) {
> > + for (i = 0; i < nr_sih_modules; i++, sih++) {
> > u8 rxbuf[4];
> > int j;
> >
> > /* skip USB */
> > - if (!sih->bytes_ixr)
> > + /* But don't skip TWL5031's ACI */
> > + if (!sih->bytes_ixr) {
> > + if (chip_is_twl5031 && !strcmp(sih->name, "aci")) {
> > + twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
> > + buf, TWL5031_ACIIDR_LSB, 1);
> > + twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
> > + buf, TWL5031_ACIIDR_MSB, 1);
> > + }
> > +
> > continue;
> > + }
I found out yesterday that with the above code the interrupts aren't
actually masked/cleared as expected, at least with some TWL5031
revisions. If 0xff is written to ACIIDR_MSB (as is done above) some
interrupts are still not masked/cleared properly and I'm seeing:
irq 374: nobody cared (try booting with the "irqpoll" option)
[<8003144c>] (dump_stack+0x0/0x14) from [<80084b14>] (__report_bad_irq+0x38/0x90)
[<80084adc>] (__report_bad_irq+0x0/0x90) from [<80084cc0>] (note_interrupt+0x154/0x1c8) r5:00000000 r4:8038076c
[<80084b6c>] (note_interrupt+0x0/0x1c8) from [<801b0a8c>] (twl4030_irq_thread+0xdc/0x150)
[<801b09b0>] (twl4030_irq_thread+0x0/0x150) from [<8006d68c>] (kthread+0x54/0x80) r7:00000000 r6:00000000 r5:801b09b0 r4:00000007
[<8006d638>] (kthread+0x0/0x80) from [<8005a7d0>] (do_exit+0x0/0x778) r5:00000000 r4:00000000
handlers:
Disabling IRQ #374
Writing 0x01 (proper value according to TRM) to ACIIDR_MSB instead of
0xff seems to fix the issue. Writing 0xff or 0x01 to ACIIMR_MSB does not
seem to make any difference, though.
--
Ari
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] TWL4030: Initial support for TWL5031
2009-11-04 16:08 ` Samuel Ortiz
2009-11-05 9:08 ` Kauppi Ari (EXT-Ixonos/Oulu)
@ 2009-11-06 14:32 ` Ilkka Koskinen
1 sibling, 0 replies; 5+ messages in thread
From: Ilkka Koskinen @ 2009-11-06 14:32 UTC (permalink / raw)
To: Samuel Ortiz
Cc: Koskinen Ilkka (Nokia-D/Tampere),
linux-kernel, linux-omap, Balbi Felipe (Nokia-D/Helsinki)
Hi Samuel,
Thanks for comments. I'll fix them but I'd
still have a couple of comments
On Wed, 4 Nov 2009, Samuel Ortiz wrote:
> Hi Ilkka,
>
> Some comments below:
>
> On Fri, Oct 16, 2009 at 05:12:40PM +0300, Ilkka Koskinen wrote:
>> TWL5031 introduces two new interrupts in PIH. Moreover, BCI
>> has changed remarkably and, thus, it's disabled when TWL5031
>> is in use.
>>
>> Signed-off-by: Ilkka Koskinen <ilkka.koskinen@nokia.com>
>> ---
>> drivers/mfd/twl4030-core.c | 12 ++++-
>> drivers/mfd/twl4030-irq.c | 127 +++++++++++++++++++++++++++++++++++++++++--
>> include/linux/i2c/twl4030.h | 47 ++++++++++++++--
>> 3 files changed, 173 insertions(+), 13 deletions(-)
>>
>> @@ -284,11 +367,20 @@ static int twl4030_init_sih_modules(unsigned line)
>> /* disable all interrupts on our line */
>> memset(buf, 0xff, sizeof buf);
>> sih = sih_modules;
>> - for (i = 0; i < ARRAY_SIZE(sih_modules); i++, sih++) {
>> + for (i = 0; i < nr_sih_modules; i++, sih++) {
>>
>> /* skip USB -- it's funky */
>> - if (!sih->bytes_ixr)
>> + /* But don't skip TWL5031's ACI */
>> + if (!sih->bytes_ixr) {
>> + if (chip_is_twl5031 && !strcmp(sih->name, "aci")) {
>> + twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
>> + buf, TWL5031_ACIIMR_LSB, 1);
>> + twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
>> + buf, TWL5031_ACIIMR_MSB, 1);
>> + }
>> +
> A few comments on this:
> - Why do we have to do that for the accessory ? Some comments would be
> appropriate.
> - You could use twl4030_i2c_write_u8(), couldnt you ?
> - I'd like it to be more generic: TWL5031_MODULE_ACCESSORY should be replaced
> by sih->module, and the TWL5031_ACII* register should be somehow integrated in
> the sih struct. The idea is to get rid of this chip_is_twl5031.
Very good point. Actually, I could change it to use isr/imr offsets if I
add another variable in sih structure describing a number of supported irq
lines. (ACI supports just one...) I think it would be a lot cleaner
approach that way.
>> @@ -756,3 +857,17 @@ int twl_exit_irq(void)
>> }
>> return 0;
>> }
>> +
>> +int twl_init_chip_irq(const char *chip)
>> +{
>> + if (!strcmp(chip, "twl5031")) {
>> + chip_is_twl5031 = 1;
>> + sih_modules = sih_modules_twl5031;
>> + nr_sih_modules = ARRAY_SIZE(sih_modules_twl5031);
> I dont think you need nr_sih_modules at all. sih_modules is pointing to the
> right array, and then you can keep the ARRAY_SIZE(sih_modules) call instead of
> defining yet another static variable.
What comes to adding another static variable, I don't like it either.
But, as far as I can see ARRAY_SIZE is using sizeof operator, which is
calculated in compilation time, right? Number of sih modules is decided in
probing time based on the used twl chip and, thus, ARRAY_SIZE cannot be
used. Or did I miss something?
> Talking about static variables and such, I really think the twl driver needs
> some cleanup. It should really be allocating a private device structure at
> probe time containing all those static crap. As this driver is growing and
> supporting more and more HW, it starts to be messy.
That would be great. If I only had some time... :(
> Cheers,
> Samuel.
>
>
>> + } else {
>> + sih_modules = sih_modules_twl4030;
>> + nr_sih_modules = ARRAY_SIZE(sih_modules_twl4030);
>> + }
>> +
>> + return 0;
>> +}
Cheers, Ilkka
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-11-06 14:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-10-16 14:12 [PATCH v2] TWL4030: Initial support for TWL5031 Ilkka Koskinen
2009-11-03 11:29 ` Ilkka Koskinen
2009-11-04 16:08 ` Samuel Ortiz
2009-11-05 9:08 ` Kauppi Ari (EXT-Ixonos/Oulu)
2009-11-06 14:32 ` Ilkka Koskinen
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.