Re: [RFC/PATCH 3/3] mfd: twl4030-irq: implement bus_*lock
From: Felipe Balbi
Date: Wed Dec 29 2010 - 07:28:21 EST
Hi,
On Wed, Dec 29, 2010 at 02:38:28AM +0200, Felipe Balbi wrote:
On Tue, 2010-12-28 at 23:58 +0000, Mark Brown wrote:
On Tue, Dec 28, 2010 at 07:14:19PM +0200, Felipe Balbi wrote:
> +static void twl4030_sih_bus_sync_unlock(unsigned int irq)
> +{
> + struct sih_agent *agent = get_irq_chip_data(irq);
> +
> + mutex_unlock(&agent->irq_lock);
> +}
I suspect you need to do some sort of sync with the hardware here - the
_sync bit of the name comes from the fact that the mask and unmask stuff
is still called with IRQs disabled and so can't touch and I2C chip, this
is called after reenabling them give a chance for the updates done to
be reflected in the hardware. The implementation everyone else has done
is to update a register cache in the other functions then write that
out here before dropping the mutex.
now that I look at some gpio chips I see what you're saying, will update
that tomorrow. Thanks
I believe you meant something like below, I'll get back to this in a
little while. Have lots to test:
From f15801a5d57041b7669222bdb7cccf8b592c2f63 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <balbi@xxxxxx>
Date: Tue, 28 Dec 2010 19:07:33 +0200
Subject: [PATCH 1/2] mfd: twl4030-irq: implement bus_*lock
Organization: Texas Instruments\n
drop all the locking around mask/unmask and
implement bus_lock and bus_sync_unlock methods.
Signed-off-by: Felipe Balbi <balbi@xxxxxx>
---
drivers/mfd/twl4030-irq.c | 101 ++++++++++++++++++++++-----------------------
1 files changed, 50 insertions(+), 51 deletions(-)
diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index 298956d..84f6066 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -440,6 +440,7 @@ struct sih_agent {
const struct sih *sih;
u32 imr;
+ bool imr_change_pending;
u32 edge_change;
struct mutex irq_lock;
@@ -450,64 +451,23 @@ struct sih_agent {
static void twl4030_sih_mask(unsigned irq)
{
struct sih_agent *agent = get_irq_chip_data(irq);
- const struct sih *sih = agent->sih;
-
- union {
- u8 bytes[4];
- u32 word;
- } imr;
-
- int status;
agent->imr |= BIT(irq - agent->irq_base);
-
- mutex_lock(&agent->irq_lock);
-
- /* byte[0] gets overwritten as we write ... */
- imr.word = cpu_to_le32(agent->imr << 8);
-
- /* write the whole mask ... simpler than subsetting it */
- status = twl_i2c_write(sih->module, imr.bytes,
- sih->mask[irq_line].imr_offset, sih->bytes_ixr);
- if (status)
- pr_err("twl4030: %s, %s --> %d\n", __func__,
- "write", status);
- mutex_unlock(&agent->irq_lock);
+ agent->imr_change_pending = true;
}
static void twl4030_sih_unmask(unsigned irq)
{
struct sih_agent *agent = get_irq_chip_data(irq);
- const struct sih *sih = agent->sih;
- union {
- u8 bytes[4];
- u32 word;
- } imr;
-
- int status;
-
- mutex_lock(&agent->irq_lock);
agent->imr &= ~BIT(irq - agent->irq_base);
-
- /* byte[0] gets overwritten as we write ... */
- imr.word = cpu_to_le32(agent->imr << 8);
-
- /* write the whole mask ... simpler than subsetting it */
- status = twl_i2c_write(sih->module, imr.bytes,
- sih->mask[irq_line].imr_offset, sih->bytes_ixr);
- if (status)
- pr_err("twl4030: %s, %s --> %d\n", __func__,
- "write", status);
- mutex_unlock(&agent->irq_lock);
+ agent->imr_change_pending = true;
}
static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
{
struct sih_agent *agent = get_irq_chip_data(irq);
- const struct sih *sih = agent->sih;
struct irq_desc *desc = irq_to_desc(irq);
- int status = 0;
if (!desc) {
pr_err("twl4030: Invalid IRQ: %d\n", irq);
@@ -517,17 +477,57 @@ static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
return -EINVAL;
- mutex_lock(&agent->irq_lock);
if ((desc->status & IRQ_TYPE_SENSE_MASK) != trigger) {
- u8 bytes[6];
- u32 edge_change;
+ agent->edge_change |= BIT(irq - agent->irq_base);
desc->status &= ~IRQ_TYPE_SENSE_MASK;
desc->status |= trigger;
- agent->edge_change |= BIT(irq - agent->irq_base);
+ }
+
+ return 0;
+}
+
+static void twl4030_sih_bus_lock(unsigned int irq)
+{
+ struct sih_agent *agent = get_irq_chip_data(irq);
+
+ mutex_lock(&agent->irq_lock);
+}
+
+static void twl4030_sih_bus_sync_unlock(unsigned int irq)
+{
+ struct sih_agent *agent = get_irq_chip_data(irq);
+ const struct sih *sih = agent->sih;
+
+ int status;
+
+ union {
+ u8 bytes[4];
+ u32 word;
+ } imr;
+
+ if (agent->imr_change_pending) {
+ /* byte[0] gets overwritten as we write ... */
+ imr.word = cpu_to_le32(agent->imr << 8);
+
+ /* write the whole mask ... simpler than subsetting it */
+ status = twl_i2c_write(sih->module, imr.bytes,
+ sih->mask[irq_line].imr_offset, sih->bytes_ixr);
+ if (status)
+ pr_err("twl4030: %s, %s --> %d\n", __func__,
+ "write", status);
+ agent->imr_change_pending = false;
+ }
+
+ if (agent->edge_change) {
+ u32 edge_change;
+ u8 bytes[6];
+
edge_change = agent->edge_change;
+ agent->edge_change = 0;
- /* Read, reserving first byte for write scratch. Yes, this
+ /*
+ * Read, reserving first byte for write scratch. Yes, this
* could be cached for some speedup ... but be careful about
* any processor on the other IRQ line, EDR registers are
* shared.
@@ -550,7 +550,6 @@ static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
if (!d) {
pr_err("twl4030: Invalid IRQ: %d\n",
i + agent->irq_base);
- status = -ENODEV;
goto out;
}
@@ -576,8 +575,6 @@ static int twl4030_sih_set_type(unsigned irq, unsigned trigger)
out:
mutex_unlock(&agent->irq_lock);
-
- return status;
}
static struct irq_chip twl4030_sih_irq_chip = {
@@ -585,6 +582,8 @@ static struct irq_chip twl4030_sih_irq_chip = {
.mask = twl4030_sih_mask,
.unmask = twl4030_sih_unmask,
.set_type = twl4030_sih_set_type,
+ .bus_lock = twl4030_sih_bus_lock,
+ .bus_sync_unlock = twl4030_sih_bus_sync_unlock,
};
/*----------------------------------------------------------------------*/
--
1.7.3.4.598.g85356
--
balbi
--
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/