MFD: LP3974 PMIC support

PATCH Superseded Kyungmin Park 3 participants 6 messages Aug 2, 2010 – Aug 20, 2010 Multifunction devices (MFD)

lore

Share & tools

lore

Kyungmin Park

author

2010-08-02 03:54 UTC #0 lore raw

PATCH 1/1 MFD: LP3974 PMIC support

From: Kyungmin Park <[email protected]>

LP3974 PMIC support. It has same functionality with max8998.

Signed-off-by Kyungmin Park <[email protected]>

---

drivers/mfd/Kconfig +12 -0

index db63d40..50383b1 100644
@@ -303,6 +303,18 @@ config MFD_MAX8998
 	  accessing the device, additional drivers must be enabled in order
 	  to use the functionality of the device.
 
+config MFD_LP3974
+	bool "National Semiconductor LP3974 PMIC Support"
+	depends on I2C=y
+	select MFD_CORE
+	select MFD_MAX8998
+	help
+	  Say yes here to support for National Semiconductor LP3974. This is
+	  a Power Management IC. This driver provies common support for
+	  accessing the device, additional drivers must be enabled in order
+	  to use the functionality of the device.
+	  Note that it has same functionality with max8998.
+
 config MFD_WM8400
 	tristate "Support Wolfson Microelectronics WM8400"
 	select MFD_CORE

drivers/mfd/max8998.c +7 -2

index 0d68de2..cea9f48 100644
@@ -30,6 +30,11 @@
 #include <linux/mfd/max8998.h>
 #include <linux/mfd/max8998-private.h>
 
+enum max8998_type {
+	TYPE_MAX8998,
+	TYPE_LP3974,
+};
+
 static struct mfd_cell max8998_devs[] = {
 	{
 		.name = "max8998-pmic",

@@ -127,8 +132,8 @@ static int max8998_i2c_remove(struct i2c_client *i2c)
 }
 
 static const struct i2c_device_id max8998_i2c_id[] = {
-       { "max8998", 0 },
-       { }
+       { "max8998", TYPE_MAX8998 },
+       { "lp3974", TYPE_LP3974 },
 };
 MODULE_DEVICE_TABLE(i2c, max8998_i2c_id);

Kyungmin Park

author

2010-08-19 05:08 UTC #1 lore raw

Any comments? I hope it's included the 2.6.36 if possible.

Thank you,
Kyungmin Park

Quoted 62 lines · Kyungmin Park

From: Kyungmin Park <[email protected]>
LP3974 PMIC support. It has same functionality with max8998.
Signed-off-by Kyungmin Park <[email protected]>
---
drivers/mfd/Kconfig +0 -0
index db63d40..50383b1 100644
@@ -303,6 +303,18 @@ config MFD_MAX8998
accessing the device, additional drivers must be enabled in order to use the functionality of the device.
drivers/mfd/Kconfig +12 -0
+config MFD_LP3974
+       bool "National Semiconductor LP3974 PMIC Support"
+       depends on I2C=y
+       select MFD_CORE
+       select MFD_MAX8998
+       help
+         Say yes here to support for National Semiconductor LP3974. This is
+         a Power Management IC. This driver provies common support for
+         accessing the device, additional drivers must be enabled in order
+         to use the functionality of the device.
+         Note that it has same functionality with max8998.
+
config MFD_WM8400
tristate "Support Wolfson Microelectronics WM8400" select MFD_CORE
drivers/mfd/max8998.c +7 -3
index 0d68de2..cea9f48 100644
@@ -30,6 +30,11 @@
  #include <linux/mfd/max8998.h>
  #include <linux/mfd/max8998-private.h>
 
+enum max8998_type {
+       TYPE_MAX8998,
+       TYPE_LP3974,
+};
+
  static struct mfd_cell max8998_devs[] = {
        {
                .name = "max8998-pmic",
@@ -127,8 +132,8 @@ static int max8998_i2c_remove(struct i2c_client *i2c)
  }
 
  static const struct i2c_device_id max8998_i2c_id[] = {
-       { "max8998", 0 },
-       { }
+       { "max8998", TYPE_MAX8998 },
+       { "lp3974", TYPE_LP3974 },
  };
  MODULE_DEVICE_TABLE(i2c, max8998_i2c_id);
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jonathan Cameron

2010-08-19 09:54 UTC #2 lore raw

Quoted 1 line · Kyungmin Park

Any comments? I hope it's included the 2.6.36 if possible.

One request for clarification below....

Quoted 19 lines

Thank you,
Kyungmin Park
Quoted 14 lines · Kyungmin Park
From: Kyungmin Park <[email protected]>
LP3974 PMIC support. It has same functionality with max8998.
Signed-off-by Kyungmin Park <[email protected]>
---
drivers/mfd/Kconfig +0 -0
index db63d40..50383b1 100644
@@ -303,6 +303,18 @@ config MFD_MAX8998
         accessing the device, additional drivers must be enabled in order
         to use the functionality of the device.
 

Quoted 11 lines

drivers/mfd/Kconfig config MFD_MAX8998 · line 316

Quoted 11 lines

drivers/mfd/Kconfig +11 -0

+config MFD_LP3974
+       bool "National Semiconductor LP3974 PMIC Support"
+       depends on I2C=y
+       select MFD_CORE
+       select MFD_MAX8998
+       help
+         Say yes here to support for National Semiconductor LP3974. This is
+         a Power Management IC. This driver provies common support for
+         accessing the device, additional drivers must be enabled in order
+         to use the functionality of the device.
+         Note that it has same functionality with max8998.

What is gained from adding a second kconfig option?
Numerous drivers throughout the kernel support very differently named parts, so why not just change the text for the MFD_MAX8998 to say it supports this part as well?

Quoted 42 lines

Quoted 32 lines
drivers/mfd/Kconfig +1 -0
+
 config MFD_WM8400
       tristate "Support Wolfson Microelectronics WM8400"
       select MFD_CORE
drivers/mfd/max8998.c +7 -3
index 0d68de2..cea9f48 100644
@@ -30,6 +30,11 @@
 #include <linux/mfd/max8998.h>
 #include <linux/mfd/max8998-private.h>
 
+enum max8998_type {
+       TYPE_MAX8998,
+       TYPE_LP3974,
+};
+
 static struct mfd_cell max8998_devs[] = {
       {
               .name = "max8998-pmic",
@@ -127,8 +132,8 @@ static int max8998_i2c_remove(struct i2c_client *i2c)
 }
 
 static const struct i2c_device_id max8998_i2c_id[] = {
-       { "max8998", 0 },
-       { }
+       { "max8998", TYPE_MAX8998 },
+       { "lp3974", TYPE_LP3974 },
 };
 MODULE_DEVICE_TABLE(i2c, max8998_i2c_id);
 
--
Quoted 5 lines
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Kyungmin Park

author

2010-08-19 09:56 UTC #3 lore raw

Quoted 22 lines · Jonathan Cameron

Quoted 1 line · Kyungmin Park
Any comments? I hope it's included the 2.6.36 if possible.
One request for clarification below....
Quoted 19 lines
Thank you,
Kyungmin Park
Quoted 14 lines · Kyungmin Park
From: Kyungmin Park <[email protected]>
LP3974 PMIC support. It has same functionality with max8998.
Signed-off-by Kyungmin Park <[email protected]>
---
drivers/mfd/Kconfig +0 -0
index db63d40..50383b1 100644
@@ -303,6 +303,18 @@ config MFD_MAX8998
accessing the device, additional drivers must be enabled in order to use the functionality of the device.

Quoted 14 lines

Quoted 11 lines
Quoted 11 lines
+config MFD_LP3974
+ bool "National Semiconductor LP3974 PMIC Support"
+ depends on I2C=y
+ select MFD_CORE
+ select MFD_MAX8998
+ help
+ Say yes here to support for National Semiconductor LP3974. This is
+ a Power Management IC. This driver provies common support for
+ accessing the device, additional drivers must be enabled in order
+ to use the functionality of the device.
+ Note that it has same functionality with max8998.
What is gained from adding a second kconfig option?
Numerous drivers throughout the kernel support very differently named parts, so why not just change the text for the MFD_MAX8998 to say it supports this part as well?

I also consider that. If I got the LP3974 but can't find a config so it's some confused.
Well, I will modify the MAX8998 comment. if you want

Thank you,
Kyungmin Park

Quoted 44 lines

Quoted 42 lines
Quoted 32 lines
+
config MFD_WM8400
tristate "Support Wolfson Microelectronics WM8400" select MFD_CORE
drivers/mfd/max8998.c +7 -3
index 0d68de2..cea9f48 100644
@@ -30,6 +30,11 @@
  #include <linux/mfd/max8998.h>
  #include <linux/mfd/max8998-private.h>
 
+enum max8998_type {
+       TYPE_MAX8998,
+       TYPE_LP3974,
+};
+
  static struct mfd_cell max8998_devs[] = {
        {
                .name = "max8998-pmic",
@@ -127,8 +132,8 @@ static int max8998_i2c_remove(struct i2c_client *i2c)
  }
 
  static const struct i2c_device_id max8998_i2c_id[] = {
-       { "max8998", 0 },
-       { }
+       { "max8998", TYPE_MAX8998 },
+       { "lp3974", TYPE_LP3974 },
  };
  MODULE_DEVICE_TABLE(i2c, max8998_i2c_id);
 
--
Quoted 5 lines
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jonathan Cameron

2010-08-19 10:04 UTC #4 lore raw

Quoted 39 lines · Kyungmin Park

Quoted 22 lines · Jonathan Cameron
Quoted 1 line · Kyungmin Park
Any comments? I hope it's included the 2.6.36 if possible.
One request for clarification below....
Quoted 19 lines
Thank you,
Kyungmin Park
Quoted 14 lines · Kyungmin Park
From: Kyungmin Park <[email protected]>
LP3974 PMIC support. It has same functionality with max8998.
Signed-off-by Kyungmin Park <[email protected]>
---
drivers/mfd/Kconfig +0 -0
index db63d40..50383b1 100644
@@ -303,6 +303,18 @@ config MFD_MAX8998
         accessing the device, additional drivers must be enabled in order
         to use the functionality of the device.
 
Quoted 14 lines
Quoted 11 lines
drivers/mfd/Kconfig config MFD_MAX8998 · line 316
Quoted 11 lines
drivers/mfd/Kconfig +11 -0
+config MFD_LP3974
+       bool "National Semiconductor LP3974 PMIC Support"
+       depends on I2C=y
+       select MFD_CORE
+       select MFD_MAX8998
+       help
+         Say yes here to support for National Semiconductor LP3974. This is
+         a Power Management IC. This driver provies common support for
+         accessing the device, additional drivers must be enabled in order
+         to use the functionality of the device.
+         Note that it has same functionality with max8998.
What is gained from adding a second kconfig option?
Numerous drivers throughout the kernel support very differently named parts, so why not just change the text for the MFD_MAX8998 to say it supports this part as well?

Quoted 3 lines

I also consider that. If I got the LP3974 but can't find a config so it's some confused.
Well, I will modify the MAX8998 comment. if you want

Not my area of the kernel, so up to maintainers for what they would prefer. Having worked on drivers supporting 10s of different parts I'm personally anti this as I see it as bloat.

Actually, looking a little more closely...

Currently no use is being made of the enum.
You could turn this patch into a simple change to the Kconfig comment and
{ "max8998", 0 },
+{ "lp3874", 0 },
{}

Unless there is a follow up patch using the enum, the usual principle of don't add code that isn't used applies. Also, that makes the patch even more trivial and so easier to merge post merge window.

Jonathan

Quoted 48 lines

Thank you,
Kyungmin Park
Quoted 44 lines
Quoted 42 lines
Quoted 32 lines
drivers/mfd/Kconfig +1 -0
+
 config MFD_WM8400
       tristate "Support Wolfson Microelectronics WM8400"
       select MFD_CORE
drivers/mfd/max8998.c +7 -3
index 0d68de2..cea9f48 100644
@@ -30,6 +30,11 @@
 #include <linux/mfd/max8998.h>
 #include <linux/mfd/max8998-private.h>
 
+enum max8998_type {
+       TYPE_MAX8998,
+       TYPE_LP3974,
+};
+
 static struct mfd_cell max8998_devs[] = {
       {
               .name = "max8998-pmic",
@@ -127,8 +132,8 @@ static int max8998_i2c_remove(struct i2c_client *i2c)
 }
 
 static const struct i2c_device_id max8998_i2c_id[] = {
-       { "max8998", 0 },
-       { }
+       { "max8998", TYPE_MAX8998 },
+       { "lp3974", TYPE_LP3974 },
 };
 MODULE_DEVICE_TABLE(i2c, max8998_i2c_id);
 
--
Quoted 5 lines
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Samuel Ortiz

2010-08-20 22:09 UTC #5 lore raw

In reply to Kyungmin Park

Hi Kyungmin,

Quoted 1 line · Kyungmin Park

Any comments? I hope it's included the 2.6.36 if possible.

It hopefully will be. I received the patch after the merge window opened though.
Some comments on your patch:

Quoted 11 lines

Quoted 11 lines
+config MFD_LP3974
+ bool "National Semiconductor LP3974 PMIC Support"
+ depends on I2C=y
+ select MFD_CORE
+ select MFD_MAX8998
+ help
+ Say yes here to support for National Semiconductor LP3974. This is
+ a Power Management IC. This driver provies common support for
+ accessing the device, additional drivers must be enabled in order
+ to use the functionality of the device.
+ Note that it has same functionality with max8998.

As Jonathan mentioned, there really is no need for a Kconfig symbol. Just change the existing Kconfig comments to add your device there.

Quoted 11 lines

Quoted 11 lines
config MFD_WM8400
tristate "Support Wolfson Microelectronics WM8400" select MFD_CORE
drivers/mfd/max8998.c +0 -0
index 0d68de2..cea9f48 100644
@@ -30,6 +30,11 @@
  #include <linux/mfd/max8998.h>
  #include <linux/mfd/max8998-private.h>
 

Quoted 4 lines

drivers/mfd/max8998.c line 36
Quoted 4 lines
drivers/mfd/max8998.c +4 -0
+enum max8998_type {
+       TYPE_MAX8998,
+       TYPE_LP3974,
+};

Not used, so please remove that.

Cheers,
Samuel.

Signature · 2 lines

Intel Open Source Technology Centre
http://oss.intel.com/