[1/3] mfd: pm8921: Expose pm8xxx_read_irq_status

Commit Message

Bjorn Andersson July 8, 2014, 1:26 a.m. UTC

Most status bits, e.g. for GPIO and MPP input, is retrieved by reading
the interrupt status registers, so this needs to be exposed to clients.

Signed-off-by: Bjorn Andersson <[e-mail]>
---
 drivers/mfd/pm8921-core.c       |   36 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/pm8921-core.h |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)
 create mode 100644 include/linux/mfd/pm8921-core.h

Comments

Stephen Boyd July 8, 2014, 11:20 p.m. UTC | #1

On 07/07/14 18:26, Bjorn Andersson wrote:
> @@ -65,6 +66,41 @@ struct pm_irq_chip {
>  	u8			config[0];
>  };
>  
> +int pm8xxx_read_irq_status(int irq)
> +{
> +	struct irq_data *d = irq_get_irq_data(irq);
> +	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> +	unsigned int pmirq = irqd_to_hwirq(d);
> +	unsigned int bits;
> +	int irq_bit;
> +	u8 block;
> +	int rc;
> +
> +	if (!chip) {
> +		pr_err("Failed to resolve pm_irq_chip\n");
> +		return -EINVAL;
> +	}

Is this actually necessary? Presumably the driver wouldn't have even
probed unless there was a pmic to begin with.

> +
> +	block = pmirq / 8;
> +	irq_bit = pmirq % 8;
> +
> +	spin_lock(&chip->pm_irq_lock);
> +	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
> +	if (rc) {
> +		pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
> +		goto bail;
> +	}
> +
> +	rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
> +	if (rc)
> +		pr_err("Failed Reading Status rc=%d\n", rc);
> +bail:
> +	spin_unlock(&chip->pm_irq_lock);
> +
> +	return rc ? rc : !!(bits & BIT(irq_bit));
> +}
> +EXPORT_SYMBOL(pm8xxx_read_irq_status);
> +
>  static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
>  				 unsigned int *ip)
>  {
> diff --git a/include/linux/mfd/pm8921-core.h b/include/linux/mfd/pm8921-core.h
> new file mode 100644
> index 0000000..77f7cb5
> --- /dev/null
> +++ b/include/linux/mfd/pm8921-core.h
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright (c) 2014, Sony Mobile Communications AB
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +#ifndef __MFD_PM8921_CORE_H
> +#define __MFD_PM8921_CORE_H
> +
> +#include <linux/err.h>
> +
> +#ifdef CONFIG_MFD_PM8921_CORE
> +
> +int pm8xxx_read_irq_status(int irq);
> +
> +#else
> +
> +static inline int pm8xxx_read_irq_status(int irq)
> +{
> +	return -ENOSYS;
> +}
> +
> +#endif

Sad, the header file came back. I guess there isn't a way to put the
pinctrl driver inside the core mfd driver? Then we wouldn't need to
expose an "irq read line" function.

Actually Abhijeet proposed such an API in 2011 but it didn't go
anywhere[1]. If we had that API we should be able to call
read_irq_line() from the pinctrl driver whenever we want to get the
state of the gpio, plus the API is generic. We're going to need that API
anyway for things like USB insertion detection so it might make sense to
add it sooner rather than later.

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/048319.html

Bjorn Andersson July 8, 2014, 11:43 p.m. UTC | #2

On Tue, Jul 8, 2014 at 4:20 PM, Stephen Boyd <[e-mail]> wrote:
> On 07/07/14 18:26, Bjorn Andersson wrote:
>> @@ -65,6 +66,41 @@ struct pm_irq_chip {
>>       u8                      config[0];
>>  };
>>
>> +int pm8xxx_read_irq_status(int irq)
>> +{
>> +     struct irq_data *d = irq_get_irq_data(irq);
>> +     struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> +     unsigned int pmirq = irqd_to_hwirq(d);
>> +     unsigned int bits;
>> +     int irq_bit;
>> +     u8 block;
>> +     int rc;
>> +
>> +     if (!chip) {
>> +             pr_err("Failed to resolve pm_irq_chip\n");
>> +             return -EINVAL;
>> +     }
>
> Is this actually necessary? Presumably the driver wouldn't have even
> probed unless there was a pmic to begin with.
>

I had a bug in the calling driver, passing the wrong irq down to this
function. But now that I think more about it I should probably check
for "d" being non-NULL. On the other hand, that will still just catch
a minor set of bugs as both of those will in most cases produce
"valid" pointers.

Maybe it's okay to just trust the caller to pass a valid irq? Or do
you have any other suggestion of sanity checking the input value?
Preferably without also passing the pm_irq_chip pointer.

[...]
> Sad, the header file came back. I guess there isn't a way to put the
> pinctrl driver inside the core mfd driver? Then we wouldn't need to
> expose an "irq read line" function.
>

I continued my search and this needs to be accessed by gpio, mpp, adc,
charger, bms and usb(?). So we have to expose it in some form.

> Actually Abhijeet proposed such an API in 2011 but it didn't go
> anywhere[1]. If we had that API we should be able to call
> read_irq_line() from the pinctrl driver whenever we want to get the
> state of the gpio, plus the API is generic. We're going to need that API
> anyway for things like USB insertion detection so it might make sense to
> add it sooner rather than later.
>
> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/048319.html

From what I can see of this thread it was exposed as a way for drivers
to be able to query if an interrupt handler was called on raising or
falling edge. And based on the locking limitations of the
implementation we couldn't have used it anyways.

Our use case is different in that we're at any point in time
interested in reading out the status of the irq line, as the only way
of getting that status.

It might be a long shot, but let's spin a patch for our purpose and
run it by Tomas again.

Regards,
Bjorn

Ivan T. Ivanov July 9, 2014, 7:24 a.m. UTC | #3

On Tue, 2014-07-08 at 16:43 -0700, Bjorn Andersson wrote:
> On Tue, Jul 8, 2014 at 4:20 PM, Stephen Boyd <[e-mail]> wrote:
> > On 07/07/14 18:26, Bjorn Andersson wrote:

<snip>

> [...]
> > Sad, the header file came back. I guess there isn't a way to put the
> > pinctrl driver inside the core mfd driver? Then we wouldn't need to
> > expose an "irq read line" function.
> >
> 
> I continued my search and this needs to be accessed by gpio, mpp, adc,
> charger, bms and usb(?). So we have to expose it in some form.
> 
> > Actually Abhijeet proposed such an API in 2011 but it didn't go
> > anywhere[1]. If we had that API we should be able to call
> > read_irq_line() from the pinctrl driver whenever we want to get the
> > state of the gpio, plus the API is generic. We're going to need that API
> > anyway for things like USB insertion detection so it might make sense to
> > add it sooner rather than later.
> >
> > [1]
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/048319.html
> 
> From what I can see of this thread it was exposed as a way for drivers
> to be able to query if an interrupt handler was called on raising or
> falling edge. And based on the locking limitations of the
> implementation we couldn't have used it anyways.
> 
> Our use case is different in that we're at any point in time
> interested in reading out the status of the irq line, as the only way
> of getting that status.

How about using extcon framework? 

Regards,
Ivan

Linus Walleij July 9, 2014, 7:59 a.m. UTC | #4

On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
<[e-mail]> wrote:

> Most status bits, e.g. for GPIO and MPP input, is retrieved by reading
> the interrupt status registers, so this needs to be exposed to clients.
>
> Signed-off-by: Bjorn Andersson <[e-mail]>

Hm do you mean you read the input *values* from the interrupt status
registers?

What madness in that case.... :-)

Anyway, since the driver is based on regmap, can't the children just
get a regmap * somehow and then just go read the same register
instead of having to add a special function for it?

When I look at it it seems like it's doing regmap strangely or
something, like it's one write then one read operation to get
the register(s) and isn't that all supposed to be hidden behind
regmap so you don't need the local lock chip->pm_irq_lock?

Yours,
Linus Walleij

Bjorn Andersson July 9, 2014, 2:13 p.m. UTC | #5

On Wed, Jul 9, 2014 at 12:59 AM, Linus Walleij <[e-mail]> wrote:
> On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
> <[e-mail]> wrote:
>
>> Most status bits, e.g. for GPIO and MPP input, is retrieved by reading
>> the interrupt status registers, so this needs to be exposed to clients.
>>
>> Signed-off-by: Bjorn Andersson <[e-mail]>
>
> Hm do you mean you read the input *values* from the interrupt status
> registers?
>

Due to the limited address space (I presume), many of the status bits
on the pm8xxx are not exposed in any other place than through a banked
register in the interrupt "block". So we have to read the interrupt
status in order to get information related to things like gpio status
or if a battery is present for charging.

> What madness in that case.... :-)
>

Totally!

> Anyway, since the driver is based on regmap, can't the children just
> get a regmap * somehow and then just go read the same register
> instead of having to add a special function for it?
>

That we have, and we have access to that part of the ssbi address
space. Unfortunately, like everything else in these pmics, things are
banked. So we need first a bank selector and then a read; so it's racy
with the interrupt handler code doing the same thing.

> When I look at it it seems like it's doing regmap strangely or
> something, like it's one write then one read operation to get
> the register(s) and isn't that all supposed to be hidden behind
> regmap so you don't need the local lock chip->pm_irq_lock?
>

I guess one could have exposed a regmap that instead of exposing the
ssbi address space presented a logical view of the pm8xxx registers;
something like the bitplanes on Amiga. I prefer having a ssbi regmap
for simplicity (and sanity) but then we need to have this read in the
irq driver.

Regards,
Bjorn

Patch

diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/pm8921-core.c
index 9595138..7dda0c2 100644
--- a/drivers/mfd/pm8921-core.c
+++ b/drivers/mfd/pm8921-core.c
@@ -26,6 +26,7 @@ 
 #include <linux/regmap.h>
 #include <linux/of_platform.h>
 #include <linux/mfd/core.h>
+#include <linux/mfd/pm8921-core.h>
 
 #define	SSBI_REG_ADDR_IRQ_BASE		0x1BB
 
@@ -65,6 +66,41 @@  struct pm_irq_chip {
 	u8			config[0];
 };
 
+int pm8xxx_read_irq_status(int irq)
+{
+	struct irq_data *d = irq_get_irq_data(irq);
+	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
+	unsigned int pmirq = irqd_to_hwirq(d);
+	unsigned int bits;
+	int irq_bit;
+	u8 block;
+	int rc;
+
+	if (!chip) {
+		pr_err("Failed to resolve pm_irq_chip\n");
+		return -EINVAL;
+	}
+
+	block = pmirq / 8;
+	irq_bit = pmirq % 8;
+
+	spin_lock(&chip->pm_irq_lock);
+	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
+	if (rc) {
+		pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
+		goto bail;
+	}
+
+	rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
+	if (rc)
+		pr_err("Failed Reading Status rc=%d\n", rc);
+bail:
+	spin_unlock(&chip->pm_irq_lock);
+
+	return rc ? rc : !!(bits & BIT(irq_bit));
+}
+EXPORT_SYMBOL(pm8xxx_read_irq_status);
+
 static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
 				 unsigned int *ip)
 {
diff --git a/include/linux/mfd/pm8921-core.h b/include/linux/mfd/pm8921-core.h
new file mode 100644
index 0000000..77f7cb5
--- /dev/null
+++ b/include/linux/mfd/pm8921-core.h
@@ -0,0 +1,32 @@ 
+/*
+ * Copyright (c) 2014, Sony Mobile Communications AB
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * 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.
+ */
+
+#ifndef __MFD_PM8921_CORE_H
+#define __MFD_PM8921_CORE_H
+
+#include <linux/err.h>
+
+#ifdef CONFIG_MFD_PM8921_CORE
+
+int pm8xxx_read_irq_status(int irq);
+
+#else
+
+static inline int pm8xxx_read_irq_status(int irq)
+{
+	return -ENOSYS;
+}
+
+#endif
+
+#endif