[PATCH 1/3] MFD: MAX8997: IRQ Handling Bugfix

[PATCH 1/3] MFD: MAX8997: IRQ Handling Bugfix

[MyungJoo Ham]

Required platform information is not handed to max8997-irq.c properly.
This patch enables to hand over such information to max8997-irq.c so
that max8997-irq functions properly.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/mfd/max8997.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
index 5d1fca0..f83103b 100644
--- a/drivers/mfd/max8997.c
+++ b/drivers/mfd/max8997.c
@@ -135,10 +135,13 @@ static int max8997_i2c_probe(struct i2c_client *i2c,
 	max8997->dev = &i2c->dev;
 	max8997->i2c = i2c;
 	max8997->type = id->driver_data;
+	max8997->irq = i2c->irq;
 
 	if (!pdata)
 		goto err;
 
+	max8997->irq_base = pdata->irq_base;
+	max8997->ono = pdata->ono;
 	max8997->wakeup = pdata->wakeup;
 
 	mutex_init(&max8997->iolock);
@@ -152,6 +155,8 @@ static int max8997_i2c_probe(struct i2c_client *i2c,
 
 	pm_runtime_set_active(max8997->dev);
 
+	max8997_irq_init(max8997);
+
 	mfd_add_devices(max8997->dev, -1, max8997_devs,
 			ARRAY_SIZE(max8997_devs),
 			NULL, 0);
-- 
1.7.4.1

[PATCH 2/3] MFD: MAX8997: Support Wake-up from Suspend

[MyungJoo Ham]

- Support wake-up from suspend-to-ram.
- Handle pending interrupt after a resume.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/mfd/max8997.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
index f83103b..4ae42c6 100644
--- a/drivers/mfd/max8997.c
+++ b/drivers/mfd/max8997.c
@@ -23,6 +23,7 @@
 
 #include <linux/slab.h>
 #include <linux/i2c.h>
+#include <linux/interrupt.h>
 #include <linux/pm_runtime.h>
 #include <linux/mutex.h>
 #include <linux/mfd/core.h>
@@ -398,7 +399,29 @@ static int max8997_restore(struct device *dev)
 	return 0;
 }
 
+static int max8997_suspend(struct device *dev)
+{
+	struct i2c_client *i2c = container_of(dev, struct i2c_client, dev);
+	struct max8997_dev *max8997 = i2c_get_clientdata(i2c);
+
+	if (max8997->wakeup && max8997->irq)
+		irq_set_irq_wake(max8997->irq, 1);
+	return 0;
+}
+
+static int max8997_resume(struct device *dev)
+{
+	struct i2c_client *i2c = container_of(dev, struct i2c_client, dev);
+	struct max8997_dev *max8997 = i2c_get_clientdata(i2c);
+
+	if (max8997->wakeup && max8997->irq)
+		irq_set_irq_wake(max8997->irq, 0);
+	return max8997_irq_resume(max8997);
+}
+
 const struct dev_pm_ops max8997_pm = {
+	.suspend = max8997_suspend,
+	.resume = max8997_resume,
 	.freeze = max8997_freeze,
 	.restore = max8997_restore,
 };
-- 
1.7.4.1

[PATCH 3/3] MFD: MAX8997: IRQ definition moved to public header.

[MyungJoo Ham]

IRQ definitions are needed to be accessed by board support package codes
as well. Because they are not only used by MAX8997 MFD device drivers,
this patch pulls such definitions out of private header, which is meant
for MAX8997 MFD device drivers.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 include/linux/mfd/max8997-private.h |   46 -----------------------------------
 include/linux/mfd/max8997.h         |   46 +++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/include/linux/mfd/max8997-private.h b/include/linux/mfd/max8997-private.h
index 5ff2400..4e50a62 100644
--- a/include/linux/mfd/max8997-private.h
+++ b/include/linux/mfd/max8997-private.h
@@ -265,52 +265,6 @@ enum max8997_irq_source {
 	MAX8997_IRQ_GROUP_NR,
 };
 
-enum max8997_irq {
-	MAX8997_PMICIRQ_PWRONR,
-	MAX8997_PMICIRQ_PWRONF,
-	MAX8997_PMICIRQ_PWRON1SEC,
-	MAX8997_PMICIRQ_JIGONR,
-	MAX8997_PMICIRQ_JIGONF,
-	MAX8997_PMICIRQ_LOWBAT2,
-	MAX8997_PMICIRQ_LOWBAT1,
-
-	MAX8997_PMICIRQ_JIGR,
-	MAX8997_PMICIRQ_JIGF,
-	MAX8997_PMICIRQ_MR,
-	MAX8997_PMICIRQ_DVS1OK,
-	MAX8997_PMICIRQ_DVS2OK,
-	MAX8997_PMICIRQ_DVS3OK,
-	MAX8997_PMICIRQ_DVS4OK,
-
-	MAX8997_PMICIRQ_CHGINS,
-	MAX8997_PMICIRQ_CHGRM,
-	MAX8997_PMICIRQ_DCINOVP,
-	MAX8997_PMICIRQ_TOPOFFR,
-	MAX8997_PMICIRQ_CHGRSTF,
-	MAX8997_PMICIRQ_MBCHGTMEXPD,
-
-	MAX8997_PMICIRQ_RTC60S,
-	MAX8997_PMICIRQ_RTCA1,
-	MAX8997_PMICIRQ_RTCA2,
-	MAX8997_PMICIRQ_SMPL_INT,
-	MAX8997_PMICIRQ_RTC1S,
-	MAX8997_PMICIRQ_WTSR,
-
-	MAX8997_MUICIRQ_ADCError,
-	MAX8997_MUICIRQ_ADCLow,
-	MAX8997_MUICIRQ_ADC,
-
-	MAX8997_MUICIRQ_VBVolt,
-	MAX8997_MUICIRQ_DBChg,
-	MAX8997_MUICIRQ_DCDTmr,
-	MAX8997_MUICIRQ_ChgDetRun,
-	MAX8997_MUICIRQ_ChgTyp,
-
-	MAX8997_MUICIRQ_OVP,
-
-	MAX8997_IRQ_NR,
-};
-
 #define MAX8997_NUM_GPIO	12
 struct max8997_dev {
 	struct device *dev;
diff --git a/include/linux/mfd/max8997.h b/include/linux/mfd/max8997.h
index 0bbd13d..90e7731 100644
--- a/include/linux/mfd/max8997.h
+++ b/include/linux/mfd/max8997.h
@@ -119,4 +119,50 @@ struct max8997_platform_data {
 	/* Flash: Not implemented */
 };
 
+enum max8997_irq {
+	MAX8997_PMICIRQ_PWRONR,
+	MAX8997_PMICIRQ_PWRONF,
+	MAX8997_PMICIRQ_PWRON1SEC,
+	MAX8997_PMICIRQ_JIGONR,
+	MAX8997_PMICIRQ_JIGONF,
+	MAX8997_PMICIRQ_LOWBAT2,
+	MAX8997_PMICIRQ_LOWBAT1,
+
+	MAX8997_PMICIRQ_JIGR,
+	MAX8997_PMICIRQ_JIGF,
+	MAX8997_PMICIRQ_MR,
+	MAX8997_PMICIRQ_DVS1OK,
+	MAX8997_PMICIRQ_DVS2OK,
+	MAX8997_PMICIRQ_DVS3OK,
+	MAX8997_PMICIRQ_DVS4OK,
+
+	MAX8997_PMICIRQ_CHGINS,
+	MAX8997_PMICIRQ_CHGRM,
+	MAX8997_PMICIRQ_DCINOVP,
+	MAX8997_PMICIRQ_TOPOFFR,
+	MAX8997_PMICIRQ_CHGRSTF,
+	MAX8997_PMICIRQ_MBCHGTMEXPD,
+
+	MAX8997_PMICIRQ_RTC60S,
+	MAX8997_PMICIRQ_RTCA1,
+	MAX8997_PMICIRQ_RTCA2,
+	MAX8997_PMICIRQ_SMPL_INT,
+	MAX8997_PMICIRQ_RTC1S,
+	MAX8997_PMICIRQ_WTSR,
+
+	MAX8997_MUICIRQ_ADCError,
+	MAX8997_MUICIRQ_ADCLow,
+	MAX8997_MUICIRQ_ADC,
+
+	MAX8997_MUICIRQ_VBVolt,
+	MAX8997_MUICIRQ_DBChg,
+	MAX8997_MUICIRQ_DCDTmr,
+	MAX8997_MUICIRQ_ChgDetRun,
+	MAX8997_MUICIRQ_ChgTyp,
+
+	MAX8997_MUICIRQ_OVP,
+
+	MAX8997_IRQ_NR,
+};
+
 #endif /* __LINUX_MFD_MAX8998_H */
-- 
1.7.4.1

Re: [PATCH 3/3] MFD: MAX8997: IRQ definition moved to public header.

[Mark Brown]

On Thu, Jun 30, 2011 at 10:31:47AM +0900, MyungJoo Ham wrote:
> IRQ definitions are needed to be accessed by board support package codes
> as well. Because they are not only used by MAX8997 MFD device drivers,
> this patch pulls such definitions out of private header, which is meant
> for MAX8997 MFD device drivers.

Why is this needed?

Re: [PATCH 3/3] MFD: MAX8997: IRQ definition moved to public header.

[MyungJoo Ham]

On Thu, Jun 30, 2011 at 2:28 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Jun 30, 2011 at 10:31:47AM +0900, MyungJoo Ham wrote:
>> IRQ definitions are needed to be accessed by board support package codes
>> as well. Because they are not only used by MAX8997 MFD device drivers,
>> this patch pulls such definitions out of private header, which is meant
>> for MAX8997 MFD device drivers.
>
> Why is this needed?
>

In order to request MAX8997's IRQs, these IRQ enums are required by
board files (such as /arch/arm/mach-exynos4/mach-*.c). So, these IRQ
enums are no more "private" to max8997 device drivers. Without this
patch, board files need to include max8997-private.h.

It works properly with or without the patch. The patch is only for
some aesthetics reasons.

However, this max8997-private.h was meant to be included by device
drivers of MAX8997-MFD (such as MAX8997-PMIC, MAX8997-RTC,
MAX8997-IRQ, ...); thus, including that private header at board files
(or any other non-max8997 device drivers) didn't look "proper". If
these "private" MFD headers are to be included by non "subdevices" of
the same MFD, it is meaningless to seperate into two headers
(max8997.h and max8997-private.h) and we'd better merge public and
private headers of MFDs.


Cheers!

- MyungJoo
-- 
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

Re: [PATCH 3/3] MFD: MAX8997: IRQ definition moved to public header.

[Mark Brown]

On Thu, Jun 30, 2011 at 02:56:27PM +0900, MyungJoo Ham wrote:

> In order to request MAX8997's IRQs, these IRQ enums are required by
> board files (such as /arch/arm/mach-exynos4/mach-*.c). So, these IRQ
> enums are no more "private" to max8997 device drivers. Without this
> patch, board files need to include max8997-private.h.

Why does the board file need to request these IRQs?

Re: [PATCH 3/3] MFD: MAX8997: IRQ definition moved to public header.

[MyungJoo Ham]

On Thu, Jun 30, 2011 at 2:57 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Jun 30, 2011 at 02:56:27PM +0900, MyungJoo Ham wrote:
>
>> In order to request MAX8997's IRQs, these IRQ enums are required by
>> board files (such as /arch/arm/mach-exynos4/mach-*.c). So, these IRQ
>> enums are no more "private" to max8997 device drivers. Without this
>> patch, board files need to include max8997-private.h.
>
> Why does the board file need to request these IRQs?
>


It specifies which interrupts of MAX8997 are used.

In our code:
http://git.infradead.org/users/kmpark/linux-2.6-samsung/blob/eba500212699c0ad8d6bde0dc01c3ec7101f8154:/arch/arm/mach-exynos4/setup-charger.c
around line 85, it specifies which interrupt is going to be a
wakeup-source ("true"), how the power-managing S/W should interpret
them.

The interrupts not listed in the array are not used as a wakeup-source.

-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

Re: [PATCH 3/3] MFD: MAX8997: IRQ definition moved to public header.

[Mark Brown]

On Thu, Jun 30, 2011 at 05:00:13PM +0900, MyungJoo Ham wrote:

> In our code:
> http://git.infradead.org/users/kmpark/linux-2.6-samsung/blob/eba500212699c0ad8d6bde0dc01c3ec7101f8154:/arch/arm/mach-exynos4/setup-charger.c
> around line 85, it specifies which interrupt is going to be a
> wakeup-source ("true"), how the power-managing S/W should interpret
> them.

This looks like charger specific configuration which should be done by
the charger driver rather than by directly working with the IRQs?

Re: [PATCH 3/3] MFD: MAX8997: IRQ definition moved to public header.

[MyungJoo Ham]

On Fri, Jul 1, 2011 at 12:56 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Jun 30, 2011 at 05:00:13PM +0900, MyungJoo Ham wrote:
>
>> In our code:
>> http://git.infradead.org/users/kmpark/linux-2.6-samsung/blob/eba500212699c0ad8d6bde0dc01c3ec7101f8154:/arch/arm/mach-exynos4/setup-charger.c
>> around line 85, it specifies which interrupt is going to be a
>> wakeup-source ("true"), how the power-managing S/W should interpret
>> them.
>
> This looks like charger specific configuration which should be done by
> the charger driver rather than by directly working with the IRQs?
>

Well, the issue is that the charger driver just does not know what to
do with its own interrupts.

For example, each board has different regulator setting for DCIN
depending on the specification of the board (some uses 450mA
constantly, some uses 450mA and 700mA depending on the connection
information, which is not known to charger driver, some uses 900mA
unconditionally, and so on).

Sometimes setting the attributes of a charger at its own interrupts
depends on the status of another charger; when we have USB charger,
wireless charger, and solar panels, which may be enabled independently
and have their own device drivers.

Scenarios that we are going to support include limiting the aggregated
charging flow, stopping every charger for an error detected at a
single charger (only for really critical ones that imply that the
battery itself has a major issue) as usually only one of the chargers
has sophiscated sensors and controls, and disabling a charger when
others are active (e.g., when CC charging is finished and CV charging
is going on with little current).

In that charger specific configuration and its driver, we are trying
to aggregate multiple chargers (although we specified only two
independent chargers, not three, in that example).


Thanks,
- MyungJoo
-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

Re: [PATCH 3/3] MFD: MAX8997: IRQ definition moved to public header.

[Mark Brown]

On Fri, Jul 01, 2011 at 09:43:31AM +0900, MyungJoo Ham wrote:
> On Fri, Jul 1, 2011 at 12:56 AM, Mark Brown

> > This looks like charger specific configuration which should be done by
> > the charger driver rather than by directly working with the IRQs?

> Well, the issue is that the charger driver just does not know what to
> do with its own interrupts.

> For example, each board has different regulator setting for DCIN
> depending on the specification of the board (some uses 450mA
> constantly, some uses 450mA and 700mA depending on the connection
> information, which is not known to charger driver, some uses 900mA
> unconditionally, and so on).

That sounds like the charger driver just needs some platform data.

> Sometimes setting the attributes of a charger at its own interrupts
> depends on the status of another charger; when we have USB charger,
> wireless charger, and solar panels, which may be enabled independently
> and have their own device drivers.

My understanding was that one of the goals of the power_supply subsystem
was to support this sort of interaction?  This (and your subsequent
paragraphs) all sounds entirely sensible but it should be being dealt
with at a higher level with the various charger drivers delivering
events into a subsystem or board driver which coordinates them all.  It
seems like the driver should be doing the work of dealing with the
actual interrupts.

Re: [PATCH 3/3] MFD: MAX8997: IRQ definition moved to public header.

[MyungJoo Ham]

Hi!

On Tue, Jul 5, 2011 at 2:16 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Jul 01, 2011 at 09:43:31AM +0900, MyungJoo Ham wrote:
>> On Fri, Jul 1, 2011 at 12:56 AM, Mark Brown
>
>> > This looks like charger specific configuration which should be done by
>> > the charger driver rather than by directly working with the IRQs?
>
>> Well, the issue is that the charger driver just does not know what to
>> do with its own interrupts.
>
>> For example, each board has different regulator setting for DCIN
>> depending on the specification of the board (some uses 450mA
>> constantly, some uses 450mA and 700mA depending on the connection
>> information, which is not known to charger driver, some uses 900mA
>> unconditionally, and so on).
>
> That sounds like the charger driver just needs some platform data.

Right. And, that platform data includes IRQs: in this case, the IRQs
of MAX8997, which have charger and USB connection information.
MAX8997's IRQ handlers need to notify the charger driver and the
charger driver may use other PMIC, USB switch (such as FSA9480), and
any devices with external power connections.

>
>> Sometimes setting the attributes of a charger at its own interrupts
>> depends on the status of another charger; when we have USB charger,
>> wireless charger, and solar panels, which may be enabled independently
>> and have their own device drivers.
>
> My understanding was that one of the goals of the power_supply subsystem
> was to support this sort of interaction? This (and your subsequent
> paragraphs) all sounds entirely sensible but it should be being dealt
> with at a higher level with the various charger drivers delivering
> events into a subsystem or board driver which coordinates them all.  It
> seems like the driver should be doing the work of dealing with the
> actual interrupts.
>

Yes, I also think that it is supposed to read and update attributes of
chargers. However, I don't see any ways to interconnect chargers and
related devices with power_supply subsystem.

If we let a user process interconnect the chargers and related
devices, we need to allow userland to access (both read and write)
related attributes including power_supply_class entries and regulators
(charger on/off and adjusting charger speed). But, we don't support
userland r/w access to regulators. Also, I feel a bit wierd to rely on
user programs for specifying hardware dependencies between on-board
chips (e.g., setting charger limit at 450mA if USB is attached, at
900mA if TA is attached, at 1.5A if high-speed TA is attached, or at
300mA if wireless charger is attached / disable charger B if charger A
started CV charging, ...).

Besides, we want charger control/monitoring while user tasks are
frozen, which was the reason why we have been working on
"suspend_again" callback for suspend:
http://us.generation-nt.com/answer/patch-v5-pm-core-suspend-again-callback-suspend-ops-help-203628722.html
. For this, we will need in-kernel code (but out of max8997 device
driver code) to have information about the IRQs of max8997 (or any
other similar chips).

Anyway, even if we are going to implement charger control/monitoring
service in userspace directly accessing the device drivers of PMIC,
chargers, and switches, those user programs need to access IRQ (need
to get notifications from the interrupts). Isn't including
"non-private" kernel headers more preferable for user programs than
including "private" kernel headers or hard coding IRQ numbers, is it?



Thank you.


Cheers!
- MyungJoo.

-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

Re: [PATCH 3/3] MFD: MAX8997: IRQ definition moved to public header.

[Mark Brown]

On Tue, Jul 05, 2011 at 02:57:54PM +0900, MyungJoo Ham wrote:
> On Tue, Jul 5, 2011 at 2:16 AM, Mark Brown

> > My understanding was that one of the goals of the power_supply subsystem
> > was to support this sort of interaction? This (and your subsequent
> > paragraphs) all sounds entirely sensible but it should be being dealt
> > with at a higher level with the various charger drivers delivering
> > events into a subsystem or board driver which coordinates them all.  It
> > seems like the driver should be doing the work of dealing with the
> > actual interrupts.

> Yes, I also think that it is supposed to read and update attributes of
> chargers. However, I don't see any ways to interconnect chargers and
> related devices with power_supply subsystem.

Nor do I, but this is just software so we should be able to make it do
what's needed here.

> If we let a user process interconnect the chargers and related
> devices, we need to allow userland to access (both read and write)

I didn't say anything about userspace, and I wouldn't expect userspace
to do anything except policy here.

Re: [PATCH 3/3] MFD: MAX8997: IRQ definition moved to public header.

[MyungJoo Ham]

On Tue, Jul 5, 2011 at 3:19 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Jul 05, 2011 at 02:57:54PM +0900, MyungJoo Ham wrote:
>> On Tue, Jul 5, 2011 at 2:16 AM, Mark Brown
>
>> > My understanding was that one of the goals of the power_supply subsystem
>> > was to support this sort of interaction? This (and your subsequent
>> > paragraphs) all sounds entirely sensible but it should be being dealt
>> > with at a higher level with the various charger drivers delivering
>> > events into a subsystem or board driver which coordinates them all.  It
>> > seems like the driver should be doing the work of dealing with the
>> > actual interrupts.
>
>> Yes, I also think that it is supposed to read and update attributes of
>> chargers. However, I don't see any ways to interconnect chargers and
>> related devices with power_supply subsystem.
>
> Nor do I, but this is just software so we should be able to make it do
> what's needed here.
>
>> If we let a user process interconnect the chargers and related
>> devices, we need to allow userland to access (both read and write)
>
> I didn't say anything about userspace, and I wouldn't expect userspace
> to do anything except policy here.
>

Ah.. then, I misunderstood "a higher level" in your previous reply was
userspace. Sorry, my bad.

I was just trying to say that
a) as long as we are connecting IRQ events of a device to another
device, the IRQ information is going to be needed as non-private, and
b) in the above case, we need to let the IRQ events of a device notify
other devices' drivers. (defined in a form of platform_data in the
case, specifying IRQ numbers for the notified devices requiring the
IRQ definitions to be included)

Anyway, I've been trying to implement some virtual device driver or
psuedo framework in kernel to interconnect charger related devices,
"charger manager". This code mentioned is to support that "charger
manager". It is sort of at a higher level of charger related devices,
taking device names of them as a platform data. The platform data
taking the IRQ numbers is the one for that "charger manager".



Thank you.

- MyungJoo

-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

Re: [PATCH 3/3] MFD: MAX8997: IRQ definition moved to public header.

[Mark Brown]

On Tue, Jul 05, 2011 at 03:49:39PM +0900, MyungJoo Ham wrote:

> Anyway, I've been trying to implement some virtual device driver or
> psuedo framework in kernel to interconnect charger related devices,
> "charger manager". This code mentioned is to support that "charger
> manager". It is sort of at a higher level of charger related devices,
> taking device names of them as a platform data. The platform data
> taking the IRQ numbers is the one for that "charger manager".

Right, so what I'm saying is that the interface between the two blocks
should be changed so that the manager code isn't talking directly to the
interrupt but is talking to something higher level.