[PATCH v3 2/3] power_supply: add new lp8788 charger driver

[PATCH v3 2/3] power_supply: add new lp8788 charger driver

[Kim, Milo]

Patch v3.

(a) use irq domain for handling charger interrupts
(b) use scaled adc value rather than raw value
    : replace iio_read_channel_raw() with iio_read_channel_scale()
(c) clean up charger-platform-data code
(d) remove goto statement in _probe()
(e) name change : from lp8788_unregister_psy() to lp8788_psy_unregister()

Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
---
 drivers/power/Kconfig          |    7 +
 drivers/power/Makefile         |    1 +
 drivers/power/lp8788-charger.c |  785 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 793 insertions(+), 0 deletions(-)
 create mode 100644 drivers/power/lp8788-charger.c

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index c1892f3..ff86469 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -255,6 +255,13 @@ config CHARGER_LP8727
 	help
 	  Say Y here to enable support for LP8727 Charger Driver.
 
+config CHARGER_LP8788
+	tristate "TI LP8788 charger driver"
+	depends on MFD_LP8788
+	depends on LP8788_ADC
+	help
+	  Say Y to enable support for the LP8788 linear charger.
+
 config CHARGER_GPIO
 	tristate "GPIO charger"
 	depends on GPIOLIB
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index ee58afb..587c5f1 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_CHARGER_ISP1704)	+= isp1704_charger.o
 obj-$(CONFIG_CHARGER_MAX8903)	+= max8903_charger.o
 obj-$(CONFIG_CHARGER_TWL4030)	+= twl4030_charger.o
 obj-$(CONFIG_CHARGER_LP8727)	+= lp8727_charger.o
+obj-$(CONFIG_CHARGER_LP8788)	+= lp8788-charger.o
 obj-$(CONFIG_CHARGER_GPIO)	+= gpio-charger.o
 obj-$(CONFIG_CHARGER_MANAGER)	+= charger-manager.o
 obj-$(CONFIG_CHARGER_MAX8997)	+= max8997_charger.o
diff --git a/drivers/power/lp8788-charger.c b/drivers/power/lp8788-charger.c
new file mode 100644
index 0000000..a874eff
--- /dev/null
+++ b/drivers/power/lp8788-charger.c
@@ -0,0 +1,785 @@
+/*
+ * TI LP8788 MFD - battery charger driver
+ *
+ * Copyright 2012 Texas Instruments
+ *
+ * Author: Milo(Woogyom) Kim <milo.kim@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/iio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
+#include <linux/mfd/lp8788.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+/* register address */
+#define LP8788_CHG_STATUS		0x07
+#define LP8788_CHG_IDCIN		0x13
+#define LP8788_CHG_IBATT		0x14
+#define LP8788_CHG_VTERM		0x15
+#define LP8788_CHG_EOC			0x16
+
+/* mask/shift bits */
+#define LP8788_CHG_INPUT_STATE_M	0x03	/* Addr 07h */
+#define LP8788_CHG_STATE_M		0x3C
+#define LP8788_CHG_STATE_S		2
+#define LP8788_NO_BATT_M		BIT(6)
+#define LP8788_BAD_BATT_M		BIT(7)
+#define LP8788_CHG_IBATT_M		0x1F	/* Addr 14h */
+#define LP8788_CHG_VTERM_M		0x0F	/* Addr 15h */
+#define LP8788_CHG_EOC_LEVEL_M		0x30	/* Addr 16h */
+#define LP8788_CHG_EOC_LEVEL_S		4
+#define LP8788_CHG_EOC_TIME_M		0x0E
+#define LP8788_CHG_EOC_TIME_S		1
+#define LP8788_CHG_EOC_MODE_M		BIT(0)
+
+#define CHARGER_NAME			"charger"
+#define BATTERY_NAME			"main_batt"
+
+#define LP8788_CHG_START		0x11
+#define LP8788_CHG_END			0x1C
+
+#define BUF_SIZE			40
+#define LP8788_ISEL_MAX			23
+#define LP8788_ISEL_STEP		50
+#define LP8788_VTERM_MIN		4100
+#define LP8788_VTERM_STEP		25
+#define MAX_BATT_CAPACITY		100
+#define MAX_CHG_IRQS			11
+
+enum lp8788_charging_state {
+	OFF,
+	WARM_UP,
+	LOW_INPUT = 0x3,
+	PRECHARGE,
+	CC,
+	CV,
+	MAINTENANCE,
+	BATTERY_FAULT,
+	SYSTEM_SUPPORT = 0xC,
+	HIGH_CURRENT = 0xF,
+	MAX_CHG_STATE,
+};
+
+enum lp8788_charger_adc_sel {
+	VBATT,
+	BATT_TEMP,
+	NUM_CHG_ADC,
+};
+
+enum lp8788_charger_input_state {
+	SYSTEM_SUPPLY = 1,
+	FULL_FUNCTION,
+};
+
+/*
+ * struct lp8788_chg_irq
+ * @which        : lp8788 interrupt id
+ * @virq         : Linux IRQ number from irq_domain
+ */
+struct lp8788_chg_irq {
+	enum lp8788_int_id which;
+	int virq;
+};
+
+/*
+ * struct lp8788_charger
+ * @lp           : used for accessing the registers of mfd lp8788 device
+ * @charger      : power supply driver for the battery charger
+ * @battery      : power supply driver for the battery
+ * @charger_work : work queue for charger input interrupts
+ * @chan         : iio channels for getting adc values
+ *                 eg) battery voltage, capacity and temperature
+ * @irqs         : charger dedicated interrupts
+ * @num_irqs     : total numbers of charger interrupts
+ * @pdata        : charger platform specific data
+ */
+struct lp8788_charger {
+	struct lp8788 *lp;
+	struct power_supply charger;
+	struct power_supply battery;
+	struct work_struct charger_work;
+	struct iio_channel *chan[NUM_CHG_ADC];
+	struct lp8788_chg_irq irqs[MAX_CHG_IRQS];
+	int num_irqs;
+	struct lp8788_charger_platform_data *pdata;
+};
+
+static char *battery_supplied_to[] = {
+	BATTERY_NAME,
+};
+
+static enum power_supply_property lp8788_charger_prop[] = {
+	POWER_SUPPLY_PROP_ONLINE,
+};
+
+static enum power_supply_property lp8788_battery_prop[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_TEMP,
+};
+
+static bool lp8788_is_charger_detected(struct lp8788_charger *pchg)
+{
+	u8 data;
+
+	lp8788_read_byte(pchg->lp, LP8788_CHG_STATUS, &data);
+	data &= LP8788_CHG_INPUT_STATE_M;
+
+	return (data == SYSTEM_SUPPLY || data == FULL_FUNCTION);
+}
+
+static int lp8788_charger_get_property(struct power_supply *psy,
+					enum power_supply_property psp,
+					union power_supply_propval *val)
+{
+	struct lp8788_charger *pchg = dev_get_drvdata(psy->dev->parent);
+
+	if (psp != POWER_SUPPLY_PROP_ONLINE)
+		return -EINVAL;
+
+	val->intval = lp8788_is_charger_detected(pchg);
+	return 0;
+}
+
+static int lp8788_get_battery_status(struct lp8788_charger *pchg,
+				union power_supply_propval *val)
+{
+	enum lp8788_charging_state state;
+	u8 data;
+	int ret;
+
+	ret = lp8788_read_byte(pchg->lp, LP8788_CHG_STATUS, &data);
+	if (ret)
+		return ret;
+
+	state = (data & LP8788_CHG_STATE_M) >> LP8788_CHG_STATE_S;
+	switch (state) {
+	case OFF:
+		val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		break;
+	case PRECHARGE:
+	case CC:
+	case CV:
+	case HIGH_CURRENT:
+		val->intval = POWER_SUPPLY_STATUS_CHARGING;
+		break;
+	case MAINTENANCE:
+		val->intval = POWER_SUPPLY_STATUS_FULL;
+		break;
+	default:
+		val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	}
+
+	return 0;
+}
+
+static int lp8788_get_battery_health(struct lp8788_charger *pchg,
+				union power_supply_propval *val)
+{
+	u8 data;
+	int ret;
+
+	ret = lp8788_read_byte(pchg->lp, LP8788_CHG_STATUS, &data);
+	if (ret)
+		return ret;
+
+	if (data & LP8788_NO_BATT_M)
+		val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
+	else if (data & LP8788_BAD_BATT_M)
+		val->intval = POWER_SUPPLY_HEALTH_DEAD;
+	else
+		val->intval = POWER_SUPPLY_HEALTH_GOOD;
+
+	return 0;
+}
+
+static int lp8788_get_battery_present(struct lp8788_charger *pchg,
+				union power_supply_propval *val)
+{
+	u8 data;
+	int ret;
+
+	ret = lp8788_read_byte(pchg->lp, LP8788_CHG_STATUS, &data);
+	if (ret)
+		return ret;
+
+	val->intval = !(data & LP8788_NO_BATT_M);
+	return 0;
+}
+
+static int lp8788_get_vbatt_adc(struct lp8788_charger *pchg,
+				unsigned int *result)
+{
+	struct iio_channel *channel = pchg->chan[VBATT];
+	int ret, scaleint, scalepart;
+
+	if (!channel)
+		return -EINVAL;
+
+	ret = iio_read_channel_scale(channel, &scaleint, &scalepart);
+	if (ret != IIO_VAL_INT_PLUS_MICRO)
+		return -EINVAL;
+
+	/* unit: mV */
+	*result = (scaleint + scalepart * 1000000) / 1000;
+
+	return 0;
+}
+
+static int lp8788_get_battery_voltage(struct lp8788_charger *pchg,
+				union power_supply_propval *val)
+{
+	return lp8788_get_vbatt_adc(pchg, &val->intval);
+}
+
+static int lp8788_get_battery_capacity(struct lp8788_charger *pchg,
+				union power_supply_propval *val)
+{
+	struct lp8788 *lp = pchg->lp;
+	unsigned int max_vbatt, vbatt;
+	enum lp8788_charging_state state;
+	struct lp8788_charger_platform_data *pdata = pchg->pdata;
+	u8 data;
+	int ret;
+
+	if (!pdata)
+		return -EINVAL;
+
+	max_vbatt = pdata->max_vbatt_mv;
+	if (max_vbatt == 0)
+		return -EINVAL;
+
+	ret = lp8788_read_byte(lp, LP8788_CHG_STATUS, &data);
+	if (ret)
+		return ret;
+
+	state = (data & LP8788_CHG_STATE_M) >> LP8788_CHG_STATE_S;
+
+	if (state == MAINTENANCE) {
+		val->intval = MAX_BATT_CAPACITY;
+	} else {
+		ret = lp8788_get_vbatt_adc(pchg, &vbatt);
+		if (ret)
+			return ret;
+
+		val->intval = (vbatt * MAX_BATT_CAPACITY) / max_vbatt;
+		val->intval = min(val->intval, MAX_BATT_CAPACITY);
+	}
+
+	return 0;
+}
+
+static int lp8788_get_battery_temperature(struct lp8788_charger *pchg,
+				union power_supply_propval *val)
+{
+	struct iio_channel *channel = pchg->chan[BATT_TEMP];
+	int ret, scaleint, scalepart;
+
+	if (!channel)
+		return -EINVAL;
+
+	ret = iio_read_channel_scale(channel, &scaleint, &scalepart);
+	if (ret != IIO_VAL_INT_PLUS_MICRO)
+		return -EINVAL;
+
+	/* unit: 0.1 'C */
+	val->intval = (scaleint + scalepart * 1000000) / 100;
+
+	return 0;
+}
+
+static int lp8788_battery_get_property(struct power_supply *psy,
+					enum power_supply_property psp,
+					union power_supply_propval *val)
+{
+	struct lp8788_charger *pchg = dev_get_drvdata(psy->dev->parent);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		return lp8788_get_battery_status(pchg, val);
+	case POWER_SUPPLY_PROP_HEALTH:
+		return lp8788_get_battery_health(pchg, val);
+	case POWER_SUPPLY_PROP_PRESENT:
+		return lp8788_get_battery_present(pchg, val);
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		return lp8788_get_battery_voltage(pchg, val);
+	case POWER_SUPPLY_PROP_CAPACITY:
+		return lp8788_get_battery_capacity(pchg, val);
+	case POWER_SUPPLY_PROP_TEMP:
+		return lp8788_get_battery_temperature(pchg, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static inline bool lp8788_is_valid_charger_register(u8 addr)
+{
+	return (addr >= LP8788_CHG_START && addr <= LP8788_CHG_END);
+}
+
+static int lp8788_update_charger_params(struct lp8788_charger *pchg)
+{
+	struct lp8788 *lp = pchg->lp;
+	struct lp8788_charger_platform_data *pdata = pchg->pdata;
+	struct lp8788_chg_param *param;
+	int i, ret;
+
+	if (!pdata || !pdata->chg_params) {
+		dev_info(lp->dev, "skip updating charger parameters\n");
+		return 0;
+	}
+
+	/* settting charging parameters */
+	for (i = 0 ; i < pdata->num_chg_params ; i++) {
+		param = pdata->chg_params + i;
+
+		if (!param)
+			continue;
+
+		if (lp8788_is_valid_charger_register(param->addr)) {
+			ret = lp8788_write_byte(lp, param->addr, param->val);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int lp8788_psy_register(struct platform_device *pdev,
+				struct lp8788_charger *pchg)
+{
+	pchg->charger.name = CHARGER_NAME;
+	pchg->charger.type = POWER_SUPPLY_TYPE_MAINS;
+	pchg->charger.properties = lp8788_charger_prop;
+	pchg->charger.num_properties = ARRAY_SIZE(lp8788_charger_prop);
+	pchg->charger.get_property = lp8788_charger_get_property;
+	pchg->charger.supplied_to = battery_supplied_to;
+	pchg->charger.num_supplicants = ARRAY_SIZE(battery_supplied_to);
+
+	if (power_supply_register(&pdev->dev, &pchg->charger))
+		return -EPERM;
+
+	pchg->battery.name = BATTERY_NAME;
+	pchg->battery.type = POWER_SUPPLY_TYPE_BATTERY;
+	pchg->battery.properties = lp8788_battery_prop;
+	pchg->battery.num_properties = ARRAY_SIZE(lp8788_battery_prop);
+	pchg->battery.get_property = lp8788_battery_get_property;
+
+	if (power_supply_register(&pdev->dev, &pchg->battery))
+		return -EPERM;
+
+	return 0;
+}
+
+static void lp8788_psy_unregister(struct lp8788_charger *pchg)
+{
+	power_supply_unregister(&pchg->battery);
+	power_supply_unregister(&pchg->charger);
+}
+
+static void lp8788_charger_event(struct work_struct *work)
+{
+	struct lp8788_charger *pchg =
+		container_of(work, struct lp8788_charger, charger_work);
+	struct lp8788_charger_platform_data *pdata = pchg->pdata;
+	enum lp8788_charger_event event = lp8788_is_charger_detected(pchg);
+
+	pdata->charger_event(pchg->lp, event);
+}
+
+static bool lp8788_find_irq_id(struct lp8788_charger *pchg, int virq, int *id)
+{
+	bool found;
+	int i;
+
+	for (i = 0 ; i < pchg->num_irqs ; i++) {
+		if (pchg->irqs[i].virq == virq) {
+			*id = pchg->irqs[i].which;
+			found = true;
+			break;
+		}
+	}
+
+	return found;
+}
+
+static irqreturn_t lp8788_charger_irq_thread(int virq, void *ptr)
+{
+	struct lp8788_charger *pchg = ptr;
+	struct lp8788_charger_platform_data *pdata = pchg->pdata;
+	int id = -1;
+
+	if (!lp8788_find_irq_id(pchg, virq, &id))
+		return IRQ_NONE;
+
+	switch (id) {
+	case LP8788_INT_CHG_INPUT_STATE:
+	case LP8788_INT_CHG_STATE:
+	case LP8788_INT_EOC:
+	case LP8788_INT_BATT_LOW:
+	case LP8788_INT_NO_BATT:
+		power_supply_changed(&pchg->charger);
+		power_supply_changed(&pchg->battery);
+		break;
+	default:
+		break;
+	}
+
+	/* report charger dectection event if used */
+	if (!pdata)
+		goto irq_handled;
+
+	if (pdata->charger_event && id == LP8788_INT_CHG_INPUT_STATE)
+		schedule_work(&pchg->charger_work);
+
+irq_handled:
+	return IRQ_HANDLED;
+}
+
+static int lp8788_set_irqs(struct platform_device *pdev,
+			struct lp8788_charger *pchg, const char *name)
+{
+	struct resource *r;
+	struct irq_domain *irqdm = pchg->lp->irqdm;
+	int irq_start, irq_end;
+	int i, ret, virq, nr_irq;
+
+	/* no error even if no irq resource */
+	r = platform_get_resource_byname(pdev, IORESOURCE_IRQ, name);
+	if (!r)
+		return 0;
+
+	irq_start = r->start;
+	irq_end = r->end;
+
+	for (i = irq_start ; i <= irq_end ; i++) {
+		nr_irq = pchg->num_irqs;
+
+		virq = irq_create_mapping(irqdm, i);
+		pchg->irqs[nr_irq].virq = virq;
+		pchg->irqs[nr_irq].which = i;
+		pchg->num_irqs++;
+
+		ret = request_threaded_irq(virq, NULL,
+					lp8788_charger_irq_thread,
+					0, name, pchg);
+		if (ret)
+			break;
+	}
+
+	if (i <= irq_end)
+		goto err_free_irq;
+
+	return 0;
+
+err_free_irq:
+	for (i = 0 ; i < pchg->num_irqs ; i++)
+		free_irq(pchg->irqs[i].virq, pchg);
+	return ret;
+}
+
+static int lp8788_irq_register(struct platform_device *pdev,
+				struct lp8788_charger *pchg)
+{
+	int ret, i;
+	struct lp8788 *lp = pchg->lp;
+	const char *name[] = {
+		LP8788_CHG_IRQ, LP8788_PRSW_IRQ, LP8788_BATT_IRQ
+	};
+
+	pchg->num_irqs = 0;
+
+	for (i = 0 ; i < ARRAY_SIZE(name) ; i++) {
+		ret = lp8788_set_irqs(pdev, pchg, name[i]);
+		if (ret) {
+			dev_warn(lp->dev, "irq setup failed: %s\n", name[i]);
+			return ret;
+		}
+	}
+
+	if (pchg->num_irqs > MAX_CHG_IRQS) {
+		dev_err(lp->dev, "invalid total number of irqs: %d\n",
+			pchg->num_irqs);
+		return -EINVAL;
+	}
+
+	INIT_WORK(&pchg->charger_work, lp8788_charger_event);
+
+	return 0;
+}
+
+static void lp8788_irq_unregister(struct platform_device *pdev,
+				struct lp8788_charger *pchg)
+{
+	int i;
+
+	for (i = 0 ; i < pchg->num_irqs ; i++)
+		free_irq(pchg->irqs[i].virq, pchg);
+}
+
+static void lp8788_setup_adc_channel(struct lp8788_charger *pchg)
+{
+	struct lp8788_charger_platform_data *pdata = pchg->pdata;
+	struct device *dev = pchg->lp->dev;
+	struct iio_channel *chan;
+	enum lp8788_adc_id id;
+	const char *chan_name[LPADC_MAX] = {
+		[LPADC_VBATT_5P5] = "vbatt-5p5",
+		[LPADC_VBATT_6P0] = "vbatt-6p0",
+		[LPADC_VBATT_5P0] = "vbatt-5p0",
+		[LPADC_ADC1]      = "adc1",
+		[LPADC_ADC2]      = "adc2",
+		[LPADC_ADC3]      = "adc3",
+		[LPADC_ADC4]      = "adc4",
+	};
+
+	if (!pdata)
+		return;
+
+	id = pdata->vbatt_adc;
+	switch (id) {
+	case LPADC_VBATT_5P5:
+	case LPADC_VBATT_6P0:
+	case LPADC_VBATT_5P0:
+		chan = iio_channel_get(NULL, chan_name[id]);
+		pchg->chan[VBATT] = IS_ERR(chan) ? NULL : chan;
+		break;
+	default:
+		dev_err(dev, "invalid ADC id for VBATT: %d\n", id);
+		pchg->chan[VBATT] = NULL;
+		break;
+	}
+
+	id = pdata->batt_temp_adc;
+	switch (id) {
+	case LPADC_ADC1:
+	case LPADC_ADC2:
+	case LPADC_ADC3:
+	case LPADC_ADC4:
+		chan = iio_channel_get(NULL, chan_name[id]);
+		pchg->chan[BATT_TEMP] = IS_ERR(chan) ? NULL : chan;
+		break;
+	default:
+		dev_err(dev, "invalid ADC id for BATT_TEMP : %d\n", id);
+		pchg->chan[BATT_TEMP] = NULL;
+		break;
+	}
+}
+
+static void lp8788_release_adc_channel(struct lp8788_charger *pchg)
+{
+	int i;
+
+	for (i = 0 ; i < NUM_CHG_ADC ; i++) {
+		if (!pchg->chan[i])
+			continue;
+
+		iio_channel_release(pchg->chan[i]);
+		pchg->chan[i] = NULL;
+	}
+}
+
+static ssize_t lp8788_show_charger_status(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct lp8788_charger *pchg = dev_get_drvdata(dev);
+	enum lp8788_charging_state state;
+	char *desc[MAX_CHG_STATE] = {
+		[OFF] = "CHARGER OFF",
+		[WARM_UP] = "WARM UP",
+		[LOW_INPUT] = "LOW INPUT STATE",
+		[PRECHARGE] = "CHARGING - PRECHARGE",
+		[CC] = "CHARGING - CC",
+		[CV] = "CHARGING - CV",
+		[MAINTENANCE] = "NO CHARGING - MAINTENANCE",
+		[BATTERY_FAULT] = "BATTERY FAULT",
+		[SYSTEM_SUPPORT] = "SYSTEM SUPPORT",
+		[HIGH_CURRENT] = "HIGH CURRENT",
+	};
+	u8 data;
+
+	lp8788_read_byte(pchg->lp, LP8788_CHG_STATUS, &data);
+	state = (data & LP8788_CHG_STATE_M) >> LP8788_CHG_STATE_S;
+
+	return scnprintf(buf, BUF_SIZE, "%s\n", desc[state]);
+}
+
+static ssize_t lp8788_show_idcin(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct lp8788_charger *pchg = dev_get_drvdata(dev);
+	int isel;
+	u8 val;
+
+	lp8788_read_byte(pchg->lp, LP8788_CHG_IDCIN, &val);
+	isel = LP8788_ISEL_STEP * (min_t(int, val, LP8788_ISEL_MAX) + 1);
+
+	return scnprintf(buf, BUF_SIZE,
+			"Charger Input Current Limit: %dmA\n", isel);
+}
+
+static ssize_t lp8788_show_ibatt(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct lp8788_charger *pchg = dev_get_drvdata(dev);
+	int isel;
+	u8 val;
+
+	lp8788_read_byte(pchg->lp, LP8788_CHG_IBATT, &val);
+	val &= LP8788_CHG_IBATT_M;
+	isel = LP8788_ISEL_STEP * (min_t(int, val, LP8788_ISEL_MAX) + 1);
+
+	return scnprintf(buf, BUF_SIZE,
+			"Battery Charging Current: %dmA\n", isel);
+}
+
+static ssize_t lp8788_show_vterm(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct lp8788_charger *pchg = dev_get_drvdata(dev);
+	int vsel;
+	u8 val;
+
+	lp8788_read_byte(pchg->lp, LP8788_CHG_VTERM, &val);
+	val &= LP8788_CHG_VTERM_M;
+	vsel = LP8788_VTERM_MIN + LP8788_VTERM_STEP * val;
+
+	return scnprintf(buf, BUF_SIZE,
+			"Charger Termination Voltage: %dmV\n", vsel);
+}
+
+static ssize_t lp8788_show_eoc_time(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct lp8788_charger *pchg = dev_get_drvdata(dev);
+	char *stime[] = { "400ms", "5min", "10min", "15min",
+			"20min", "25min", "30min" "No timeout" };
+	u8 val;
+
+	lp8788_read_byte(pchg->lp, LP8788_CHG_EOC, &val);
+	val = (val & LP8788_CHG_EOC_TIME_M) >> LP8788_CHG_EOC_TIME_S;
+
+	return scnprintf(buf, BUF_SIZE, "End Of Charge Time: %s\n", stime[val]);
+}
+
+static ssize_t lp8788_show_eoc_level(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct lp8788_charger *pchg = dev_get_drvdata(dev);
+	char *abs_level[] = { "25mA", "49mA", "75mA", "98mA" };
+	char *relative_level[] = { "5%", "10%", "15%", "20%" };
+	char *level;
+	u8 val, mode;
+
+	lp8788_read_byte(pchg->lp, LP8788_CHG_EOC, &val);
+
+	mode = val & LP8788_CHG_EOC_MODE_M;
+	val = (val & LP8788_CHG_EOC_LEVEL_M) >> LP8788_CHG_EOC_LEVEL_S;
+	level = mode ? abs_level[val] : relative_level[val];
+
+	return scnprintf(buf, BUF_SIZE, "End Of Charge Level: %s\n", level);
+}
+
+static DEVICE_ATTR(charger_status, S_IRUSR, lp8788_show_charger_status, NULL);
+static DEVICE_ATTR(idcin, S_IRUSR, lp8788_show_idcin, NULL);
+static DEVICE_ATTR(ibatt, S_IRUSR, lp8788_show_ibatt, NULL);
+static DEVICE_ATTR(vterm, S_IRUSR, lp8788_show_vterm, NULL);
+static DEVICE_ATTR(eoc_time, S_IRUSR, lp8788_show_eoc_time, NULL);
+static DEVICE_ATTR(eoc_level, S_IRUSR, lp8788_show_eoc_level, NULL);
+
+static struct attribute *lp8788_charger_attr[] = {
+	&dev_attr_charger_status.attr,
+	&dev_attr_idcin.attr,
+	&dev_attr_ibatt.attr,
+	&dev_attr_vterm.attr,
+	&dev_attr_eoc_time.attr,
+	&dev_attr_eoc_level.attr,
+	NULL,
+};
+
+static const struct attribute_group lp8788_attr_group = {
+	.attrs = lp8788_charger_attr,
+};
+
+static __devinit int lp8788_charger_probe(struct platform_device *pdev)
+{
+	struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
+	struct lp8788_charger *pchg;
+	int ret;
+
+	pchg = devm_kzalloc(lp->dev, sizeof(struct lp8788_charger), GFP_KERNEL);
+	if (!pchg)
+		return -ENOMEM;
+
+	pchg->lp = lp;
+	pchg->pdata = lp->pdata ? lp->pdata->chg_pdata : NULL;
+	platform_set_drvdata(pdev, pchg);
+
+	ret = lp8788_update_charger_params(pchg);
+	if (ret)
+		return ret;
+
+	lp8788_setup_adc_channel(pchg);
+
+	ret = lp8788_psy_register(pdev, pchg);
+	if (ret)
+		return ret;
+
+	ret = sysfs_create_group(&pdev->dev.kobj, &lp8788_attr_group);
+	if (ret) {
+		lp8788_psy_unregister(pchg);
+		return ret;
+	}
+
+	ret = lp8788_irq_register(pdev, pchg);
+	if (ret)
+		dev_warn(lp->dev, "failed to register charger irq: %d\n", ret);
+
+	return 0;
+}
+
+static int __devexit lp8788_charger_remove(struct platform_device *pdev)
+{
+	struct lp8788_charger *pchg = platform_get_drvdata(pdev);
+
+	if (pchg->charger_work.func)
+		flush_work_sync(&pchg->charger_work);
+
+	lp8788_irq_unregister(pdev, pchg);
+	sysfs_remove_group(&pdev->dev.kobj, &lp8788_attr_group);
+	lp8788_psy_unregister(pchg);
+	lp8788_release_adc_channel(pchg);
+
+	return 0;
+}
+
+static struct platform_driver lp8788_charger_driver = {
+	.probe = lp8788_charger_probe,
+	.remove = __devexit_p(lp8788_charger_remove),
+	.driver = {
+		.name = LP8788_DEV_CHARGER,
+		.owner = THIS_MODULE,
+	},
+};
+module_platform_driver(lp8788_charger_driver);
+
+MODULE_DESCRIPTION("TI LP8788 Charger Driver");
+MODULE_AUTHOR("Milo Kim");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:lp8788-charger");
-- 
1.7.2.5

Best Regards,
Milo

Re: [PATCH v3 2/3] power_supply: add new lp8788 charger driver

[Anton Vorontsov]

On Tue, Aug 14, 2012 at 02:32:50AM +0000, Kim, Milo wrote:
> Patch v3.

Thanks for the driver! It looks great, mostly cosmetic comments
down below.

> (a) use irq domain for handling charger interrupts
> (b) use scaled adc value rather than raw value
>     : replace iio_read_channel_raw() with iio_read_channel_scale()
> (c) clean up charger-platform-data code
> (d) remove goto statement in _probe()
> (e) name change : from lp8788_unregister_psy() to lp8788_psy_unregister()

Only changelog? No description for the driver? Nothing exciting to
tell us about the hardware, e.g. why it's so great? :-)

> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> ---
[...]
> @@ -0,0 +1,785 @@
> +/*
> + * TI LP8788 MFD - battery charger driver
> + *
> + * Copyright 2012 Texas Instruments
> + *
> + * Author: Milo(Woogyom) Kim <milo.kim@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mfd/lp8788.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +
> +/* register address */
> +#define LP8788_CHG_STATUS		0x07
> +#define LP8788_CHG_IDCIN		0x13
> +#define LP8788_CHG_IBATT		0x14
> +#define LP8788_CHG_VTERM		0x15
> +#define LP8788_CHG_EOC			0x16
> +
> +/* mask/shift bits */
> +#define LP8788_CHG_INPUT_STATE_M	0x03	/* Addr 07h */
> +#define LP8788_CHG_STATE_M		0x3C
> +#define LP8788_CHG_STATE_S		2
> +#define LP8788_NO_BATT_M		BIT(6)
> +#define LP8788_BAD_BATT_M		BIT(7)
> +#define LP8788_CHG_IBATT_M		0x1F	/* Addr 14h */
> +#define LP8788_CHG_VTERM_M		0x0F	/* Addr 15h */
> +#define LP8788_CHG_EOC_LEVEL_M		0x30	/* Addr 16h */
> +#define LP8788_CHG_EOC_LEVEL_S		4
> +#define LP8788_CHG_EOC_TIME_M		0x0E
> +#define LP8788_CHG_EOC_TIME_S		1
> +#define LP8788_CHG_EOC_MODE_M		BIT(0)
> +
> +#define CHARGER_NAME			"charger"
> +#define BATTERY_NAME			"main_batt"
> +
> +#define LP8788_CHG_START		0x11
> +#define LP8788_CHG_END			0x1C
> +
> +#define BUF_SIZE			40

This is in the global namespace, although not exported, true.

Anyways, seems way too generic. I'd prepend it with LP8788_, or
at least LP_ for short.

> +#define LP8788_ISEL_MAX			23
> +#define LP8788_ISEL_STEP		50
> +#define LP8788_VTERM_MIN		4100
> +#define LP8788_VTERM_STEP		25
> +#define MAX_BATT_CAPACITY		100
> +#define MAX_CHG_IRQS			11

ditto.

> +
> +enum lp8788_charging_state {
> +	OFF,
> +	WARM_UP,
> +	LOW_INPUT = 0x3,
> +	PRECHARGE,
> +	CC,
> +	CV,

ditto.

> +	MAINTENANCE,
> +	BATTERY_FAULT,
> +	SYSTEM_SUPPORT = 0xC,
> +	HIGH_CURRENT = 0xF,
> +	MAX_CHG_STATE,
> +};
> +
> +enum lp8788_charger_adc_sel {
> +	VBATT,
> +	BATT_TEMP,
> +	NUM_CHG_ADC,
> +};
> +
> +enum lp8788_charger_input_state {
> +	SYSTEM_SUPPLY = 1,
> +	FULL_FUNCTION,

ditto ditto..

> +};
> +
> +/*
> + * struct lp8788_chg_irq
> + * @which        : lp8788 interrupt id
> + * @virq         : Linux IRQ number from irq_domain
> + */
> +struct lp8788_chg_irq {
> +	enum lp8788_int_id which;
> +	int virq;
> +};
[...]
> +static inline bool lp8788_is_valid_charger_register(u8 addr)
> +{
> +	return (addr >= LP8788_CHG_START && addr <= LP8788_CHG_END);

No need for the outermost parenthesis.

> +}
> +
> +static int lp8788_update_charger_params(struct lp8788_charger *pchg)
> +{
> +	struct lp8788 *lp = pchg->lp;
> +	struct lp8788_charger_platform_data *pdata = pchg->pdata;
> +	struct lp8788_chg_param *param;
> +	int i, ret;

One declaration per line, please. I.e.

int i;
int ret;

> +
> +	if (!pdata || !pdata->chg_params) {
> +		dev_info(lp->dev, "skip updating charger parameters\n");
> +		return 0;
> +	}
[...]
> +static ssize_t lp8788_show_eoc_time(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct lp8788_charger *pchg = dev_get_drvdata(dev);
> +	char *stime[] = { "400ms", "5min", "10min", "15min",
> +			"20min", "25min", "30min" "No timeout" };
> +	u8 val;
> +
> +	lp8788_read_byte(pchg->lp, LP8788_CHG_EOC, &val);
> +	val = (val & LP8788_CHG_EOC_TIME_M) >> LP8788_CHG_EOC_TIME_S;
> +
> +	return scnprintf(buf, BUF_SIZE, "End Of Charge Time: %s\n", stime[val]);
> +}
> +
> +static ssize_t lp8788_show_eoc_level(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct lp8788_charger *pchg = dev_get_drvdata(dev);
> +	char *abs_level[] = { "25mA", "49mA", "75mA", "98mA" };
> +	char *relative_level[] = { "5%", "10%", "15%", "20%" };
> +	char *level;
> +	u8 val, mode;

One declaration per line please, i.e.

u8 val;
u8 mode;

> +
> +	lp8788_read_byte(pchg->lp, LP8788_CHG_EOC, &val);
> +
> +	mode = val & LP8788_CHG_EOC_MODE_M;
> +	val = (val & LP8788_CHG_EOC_LEVEL_M) >> LP8788_CHG_EOC_LEVEL_S;
> +	level = mode ? abs_level[val] : relative_level[val];
> +
> +	return scnprintf(buf, BUF_SIZE, "End Of Charge Level: %s\n", level);
> +}
> +
> +static DEVICE_ATTR(charger_status, S_IRUSR, lp8788_show_charger_status, NULL);
> +static DEVICE_ATTR(idcin, S_IRUSR, lp8788_show_idcin, NULL);
> +static DEVICE_ATTR(ibatt, S_IRUSR, lp8788_show_ibatt, NULL);
> +static DEVICE_ATTR(vterm, S_IRUSR, lp8788_show_vterm, NULL);
> +static DEVICE_ATTR(eoc_time, S_IRUSR, lp8788_show_eoc_time, NULL);
> +static DEVICE_ATTR(eoc_level, S_IRUSR, lp8788_show_eoc_level, NULL);
> +
> +static struct attribute *lp8788_charger_attr[] = {
> +	&dev_attr_charger_status.attr,
> +	&dev_attr_idcin.attr,
> +	&dev_attr_ibatt.attr,
> +	&dev_attr_vterm.attr,
> +	&dev_attr_eoc_time.attr,
> +	&dev_attr_eoc_level.attr,

Are you sure you want to have the driver-specific properties?
I.e., maybe some of them generic enough to them to the standard
POWER_SUPPLY_PROP_* set?

At least 'charging current' seems generic enough...

> +	NULL,
> +};
> +
> +static const struct attribute_group lp8788_attr_group = {
> +	.attrs = lp8788_charger_attr,
> +};
> +
> +static __devinit int lp8788_charger_probe(struct platform_device *pdev)
> +{
> +	struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
> +	struct lp8788_charger *pchg;
> +	int ret;
> +
> +	pchg = devm_kzalloc(lp->dev, sizeof(struct lp8788_charger), GFP_KERNEL);
> +	if (!pchg)
> +		return -ENOMEM;
> +
> +	pchg->lp = lp;
> +	pchg->pdata = lp->pdata ? lp->pdata->chg_pdata : NULL;
> +	platform_set_drvdata(pdev, pchg);
> +
> +	ret = lp8788_update_charger_params(pchg);
> +	if (ret)
> +		return ret;
> +
> +	lp8788_setup_adc_channel(pchg);
> +
> +	ret = lp8788_psy_register(pdev, pchg);
> +	if (ret)
> +		return ret;
> +
> +	ret = sysfs_create_group(&pdev->dev.kobj, &lp8788_attr_group);
> +	if (ret) {
> +		lp8788_psy_unregister(pchg);
> +		return ret;
> +	}
> +
> +	ret = lp8788_irq_register(pdev, pchg);
> +	if (ret)
> +		dev_warn(lp->dev, "failed to register charger irq: %d\n", ret);
> +
> +	return 0;
> +}
> +
> +static int __devexit lp8788_charger_remove(struct platform_device *pdev)
> +{
> +	struct lp8788_charger *pchg = platform_get_drvdata(pdev);
> +
> +	if (pchg->charger_work.func)
> +		flush_work_sync(&pchg->charger_work);

You need to flush the work after freeing irqs, otherwise irq might
reschedule it again.

> +	lp8788_irq_unregister(pdev, pchg);

This will probably blow up if you _probe failed to register irq.
To fix this, you probably want to set pchg->num_irqs to 0 if
irq_register() failed.

> +	sysfs_remove_group(&pdev->dev.kobj, &lp8788_attr_group);
> +	lp8788_psy_unregister(pchg);
> +	lp8788_release_adc_channel(pchg);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver lp8788_charger_driver = {
> +	.probe = lp8788_charger_probe,
> +	.remove = __devexit_p(lp8788_charger_remove),
> +	.driver = {
> +		.name = LP8788_DEV_CHARGER,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +module_platform_driver(lp8788_charger_driver);
> +
> +MODULE_DESCRIPTION("TI LP8788 Charger Driver");
> +MODULE_AUTHOR("Milo Kim");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:lp8788-charger");

Thanks!

Anton.

RE: [PATCH v3 2/3] power_supply: add new lp8788 charger driver

[Kim, Milo]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 771 bytes --]


> -----Original Message-----
> From: Anton Vorontsov [mailto:anton.vorontsov@linaro.org]
> Sent: Thursday, August 23, 2012 2:54 PM
> To: Kim, Milo
> Cc: sameo@linux.intel.com; linux-kernel@vger.kernel.org; Jonathan
> Cameron
> Subject: Re: [PATCH v3 2/3] power_supply: add new lp8788 charger driver
> 
> On Tue, Aug 14, 2012 at 02:32:50AM +0000, Kim, Milo wrote:
> > Patch v3.
> 
> Thanks for the driver! It looks great, mostly cosmetic comments
> down below.

Thanks a lot for your detailed feedback.
Updated patch was sent.

Title : [PATCH v4] power_supply: add new lp8788 charger driver

Best Regards,
Milo
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥