Skip to contentSkip to conversation

power-supply: smb347-charger: convert to use module_i2c_driver()

PATCH In review Mika Westerberg 2 participants 7 messages Apr 16, 2012 – May 6, 2012

lore
Share & tools
lore
#0 lore raw

PATCH 1/4 power-supply: smb347-charger: convert to use module_i2c_driver()

This reduces the amount of boilerplate code in the driver and makes it a bit simpler.

---
 drivers/power/smb347-charger.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)
drivers/power/smb347-charger.c +1 -11
index ce1694d..066794d 100644
@@ -1275,17 +1275,7 @@ static struct i2c_driver smb347_driver = {
 	.id_table     = smb347_id,
 };
 
-static int __init smb347_init(void)
-{
-	return i2c_add_driver(&smb347_driver);
-}
-module_init(smb347_init);
-
-static void __exit smb347_exit(void)
-{
-	i2c_del_driver(&smb347_driver);
-}
-module_exit(smb347_exit);
+module_i2c_driver(smb347_driver);
 
 MODULE_AUTHOR("Bruce E. Robertson <[email protected]>");
 MODULE_AUTHOR("Mika Westerberg <[email protected]>");
Signature · 1 line
1.7.9.1
#1 lore raw

PATCH 2/4 power-supply: smb347-charger: rename few functions to match better what they are doing

The naming used in the driver for some functions is not very clear what the functions are really doing. To make this a bit easier to understand we rename few functions which were badly named.

---
 drivers/power/smb347-charger.c |   42 ++++++++++++++++++++--------------------
 1 files changed, 21 insertions(+), 21 deletions(-)
drivers/power/smb347-charger.c +21 -21
index 066794d..d774fd3 100644
@@ -235,14 +235,14 @@ static int smb347_write(struct smb347_charger *smb, u8 reg, u8 val)
 }
 
 /**
- * smb347_update_status - updates the charging status
+ * smb347_update_ps_status - refreshes the power source status
  * @smb: pointer to smb347 charger instance
  *
- * Function checks status of the charging and updates internal state
- * accordingly. Returns %0 if there is no change in status, %1 if the
- * status has changed and negative errno in case of failure.
+ * Function checks whether any power source is connected to the charger and
+ * updates internal state accordingly. If there is a change to previous state
+ * function returns %1, otherwise %0 and negative errno in case of errror.
  */
-static int smb347_update_status(struct smb347_charger *smb)
+static int smb347_update_ps_status(struct smb347_charger *smb)
 {
 	bool usb = false;
 	bool dc = false;
@@ -271,15 +271,15 @@ static int smb347_update_status(struct smb347_charger *smb)
 }
 
 /*
- * smb347_is_online - returns whether input power source is connected
+ * smb347_is_ps_online - returns whether input power source is connected
  * @smb: pointer to smb347 charger instance
  *
  * Returns %true if input power source is connected. Note that this is
  * dependent on what platform has configured for usable power sources. For
- * example if USB is disabled, this will return %false even if the USB
- * cable is connected.
+ * example if USB is disabled, this will return %false even if the USB cable
+ * is connected.
  */
-static bool smb347_is_online(struct smb347_charger *smb)
+static bool smb347_is_ps_online(struct smb347_charger *smb)
 {
 	bool ret;
 
@@ -301,7 +301,7 @@ static int smb347_charging_status(struct smb347_charger *smb)
 {
 	int ret;
 
-	if (!smb347_is_online(smb))
+	if (!smb347_is_ps_online(smb))
 		return 0;
 
 	ret = smb347_read(smb, STAT_C);
@@ -351,7 +351,7 @@ static inline int smb347_charging_disable(struct smb347_charger *smb)
 	return smb347_charging_set(smb, false);
 }
 
-static int smb347_update_online(struct smb347_charger *smb)
+static int smb347_start_stop_charging(struct smb347_charger *smb)
 {
 	int ret;
 
@@ -360,7 +360,7 @@ static int smb347_update_online(struct smb347_charger *smb)
 	 * disable or enable the charging. We do it manually because it
 	 * depends on how the platform has configured the valid inputs.
 	 */
-	if (smb347_is_online(smb)) {
+	if (smb347_is_ps_online(smb)) {
 		ret = smb347_charging_enable(smb);
 		if (ret < 0)
 			dev_err(&smb->client->dev,
@@ -749,11 +749,11 @@ static int smb347_hw_init(struct smb347_charger *smb)
 	if (ret < 0)
 		goto fail;
 
-	ret = smb347_update_status(smb);
+	ret = smb347_update_ps_status(smb);
 	if (ret < 0)
 		goto fail;
 
-	ret = smb347_update_online(smb);
+	ret = smb347_start_stop_charging(smb);
 
 fail:
 	smb347_set_writable(smb, false);
@@ -814,8 +814,8 @@ static irqreturn_t smb347_interrupt(int irq, void *data)
 	 * was connected or disconnected.
 	 */
 	if (irqstat_e & (IRQSTAT_E_USBIN_UV_IRQ | IRQSTAT_E_DCIN_UV_IRQ)) {
-		if (smb347_update_status(smb) > 0) {
-			smb347_update_online(smb);
+		if (smb347_update_ps_status(smb) > 0) {
+			smb347_start_stop_charging(smb);
 			power_supply_changed(&smb->mains);
 			power_supply_changed(&smb->usb);
 		}
@@ -987,13 +987,13 @@ static int smb347_battery_get_property(struct power_supply *psy,
 	const struct smb347_charger_platform_data *pdata = smb->pdata;
 	int ret;
 
-	ret = smb347_update_status(smb);
+	ret = smb347_update_ps_status(smb);
 	if (ret < 0)
 		return ret;
 
 	switch (prop) {
 	case POWER_SUPPLY_PROP_STATUS:
-		if (!smb347_is_online(smb)) {
+		if (!smb347_is_ps_online(smb)) {
 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
 			break;
 		}
@@ -1004,7 +1004,7 @@ static int smb347_battery_get_property(struct power_supply *psy,
 		break;
 
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
-		if (!smb347_is_online(smb))
+		if (!smb347_is_ps_online(smb))
 			return -ENODATA;
 
 		/*
@@ -1037,7 +1037,7 @@ static int smb347_battery_get_property(struct power_supply *psy,
 		break;
 
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		if (!smb347_is_online(smb))
+		if (!smb347_is_ps_online(smb))
 			return -ENODATA;
 		ret = smb347_read(smb, STAT_A);
 		if (ret < 0)
@@ -1051,7 +1051,7 @@ static int smb347_battery_get_property(struct power_supply *psy,
 		break;
 
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
-		if (!smb347_is_online(smb))
+		if (!smb347_is_ps_online(smb))
 			return -ENODATA;
 
 		ret = smb347_read(smb, STAT_B);
Signature · 1 line
1.7.9.1
#2 lore raw

PATCH 3/4 power-supply: smb347-charger: move IRQ enabling to the end of probe

In reply to Mika Westerberg

There is a potential problem if we call smb347_irq_enable() from smb347_irq_init() because smb347_irq_enable() makes the device registers read-only once it returns and smb347_irq_init() expects them to still be read-write. Currently no harm happens because it is the last call we make in smb347_irq_init().

Anyway a better place for enabling IRQs is at the end of probe function and this is also symmetric to call smb347_irq_disable() which is done at the beginning of remove function.

---
 drivers/power/smb347-charger.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
drivers/power/smb347-charger.c +2 -4
index d774fd3..781c058 100644
@@ -923,10 +923,6 @@ static int smb347_irq_init(struct smb347_charger *smb)
 	if (ret < 0)
 		goto fail_readonly;
 
-	ret = smb347_irq_enable(smb);
-	if (ret < 0)
-		goto fail_readonly;
-
 	smb347_set_writable(smb, false);
 	smb->client->irq = irq;
 	return 0;
@@ -1233,6 +1229,8 @@ static int smb347_probe(struct i2c_client *client,
 		if (ret < 0) {
 			dev_warn(dev, "failed to initialize IRQ: %d\n", ret);
 			dev_warn(dev, "disabling IRQ support\n");
+		} else {
+			smb347_irq_enable(smb);
 		}
 	}
 
Signature · 1 line
1.7.9.1
#3 lore raw

PATCH 4/4 power-supply: smb347-charger: convert to regmap API

In reply to Mika Westerberg

The smb347-charger driver does a lot of read-modify-write to the device registers. Instead of open-coding everything we can take advantage of regmap API which provides nice functions to do this kind of things.

In addition there is no need for custom debugfs file for dumping registers as this is already provided by the regmap API.

---
 drivers/power/Kconfig          |    1 +
 drivers/power/smb347-charger.c |  555 +++++++++++++++++-----------------------
 2 files changed, 230 insertions(+), 326 deletions(-)
drivers/power/Kconfig +1 -0
index 99dc29f..c505295 100644
@@ -291,6 +291,7 @@ config CHARGER_MAX8998
 config CHARGER_SMB347
 	tristate "Summit Microelectronics SMB347 Battery Charger"
 	depends on I2C
+	select REGMAP_I2C
 	help
 	  Say Y to include support for Summit Microelectronics SMB347
 	  Battery Charger.
drivers/power/smb347-charger.c +229 -326

957 lines not on this page · raw message

Signature · 1 line
1.7.9.1
#4 lore raw

Re: [PATCH 4/4] power-supply: smb347-charger: convert to regmap API

Quoted 8 lines · Mika Westerberg

The smb347-charger driver does a lot of read-modify-write to the device registers. Instead of open-coding everything we can take advantage of regmap API which provides nice functions to do this kind of things.

In addition there is no need for custom debugfs file for dumping registers as this is already provided by the regmap API.

[...]

Quoted 14 lines

static int smb347_set_temp_limits(struct smb347_charger *smb)

(quoted diff) +1 -8
@@ -491,22 +468,13 @@ static int smb347_set_temp_limits(struct smb347_charger *smb)
 		val = clamp_val(val, 100, 130) - 100;
 		val /= 10;
 
-		ret = smb347_read(smb, CFG_OTG);
-		if (ret < 0)
-			return ret;
-
-		ret &= ~CFG_OTG_TEMP_THRESHOLD_MASK;
-		ret |= val << CFG_OTG_TEMP_THRESHOLD_SHIFT;
-
-		ret = smb347_write(smb, CFG_OTG, ret);
+		ret = regmap_update_bits(smb->regmap, CFG_OTG,

Review static int smb347_set_temp_limits(struct smb347_charger *smb) · line 509

Quoted 10 lines
(quoted diff) +2 -4
+					 CFG_OTG_TEMP_THRESHOLD_MASK,
+					 val << CFG_OTG_TEMP_THRESHOLD_SHIFT);
 		if (ret < 0)
 			return ret;
 	}
 
-	ret = smb347_read(smb, CFG_TEMP_LIMIT);
-	if (ret < 0)
-		return ret;
-

FYI, removing these hunks causes the following warning:

CC drivers/power/smb347-charger.o
drivers/power/smb347-charger.c: In function ‘smb347_set_temp_limits’:
drivers/power/smb347-charger.c:462:6: warning: ‘ret’ may be used uninitialized in this function [-Wuninitialized]

I fixed this by the following change:

drivers/power/smb347-charger.c +2 -1
index 1f27d21..cf31b31 100644
@@ -459,7 +459,8 @@ static int smb347_set_voltage_limits(struct smb347_charger *smb)
 static int smb347_set_temp_limits(struct smb347_charger *smb)
 {
 	bool enable_therm_monitor = false;
-	int ret, val;
+	int ret = 0;
+	int val;
 
 	if (smb->pdata->chip_temp_threshold) {
 		val = smb->pdata->chip_temp_threshold;

Please check if that's all OK.

Thanks!

Signature · 2 lines
Anton Vorontsov
Email: [email protected]
#5 lore raw

Re: [PATCH 4/4] power-supply: smb347-charger: convert to regmap API

Quoted 18 lines · Anton Vorontsov

I fixed this by the following change:

drivers/power/smb347-charger.c +2 -1
index 1f27d21..cf31b31 100644
@@ -459,7 +459,8 @@ static int smb347_set_voltage_limits(struct smb347_charger *smb)
 static int smb347_set_temp_limits(struct smb347_charger *smb)
 {
 	bool enable_therm_monitor = false;
-	int ret, val;
+	int ret = 0;
+	int val;
 
 	if (smb->pdata->chip_temp_threshold) {
 		val = smb->pdata->chip_temp_threshold;
Quoted 1 line

Please check if that's all OK.

Looks ok to me. Thanks for fixing it.