[PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Ramakrishna Pallala]

Add new charger driver support for BQ24261 charger IC.

BQ24261 charger driver relies on extcon notifications to get the
charger cable type and based on that it will set the charging parameters.

Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Jennt TC <jenny.tc-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/power/bq24261.txt          |   37 +
 drivers/power/Kconfig                              |    6 +
 drivers/power/Makefile                             |    1 +
 drivers/power/bq24261_charger.c                    | 1208 ++++++++++++++++++++
 include/linux/power/bq24261_charger.h              |   27 +
 5 files changed, 1279 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/bq24261.txt
 create mode 100644 drivers/power/bq24261_charger.c
 create mode 100644 include/linux/power/bq24261_charger.h

diff --git a/Documentation/devicetree/bindings/power/bq24261.txt b/Documentation/devicetree/bindings/power/bq24261.txt
new file mode 100644
index 0000000..25fc5c4
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/bq24261.txt
@@ -0,0 +1,37 @@
+Binding for TI bq24261 Li-Ion Charger
+
+Required properties:
+- compatible: Should contain one of the following:
+    * "ti,bq24261"
+- reg: integer, i2c address of the device.
+- ti,charge-current: integer, default charging current (in mA);
+- ti,charge-voltage: integer, default charging voltage (in mV);
+- ti,termination-current: integer, charge will be terminated when current in
+    constant-voltage phase drops below this value (in mA);
+- ti,max-charge-current: integer, maximum charging current (in mA);
+- ti,max-charge-voltage: integer, maximum charging voltage (in mV);
+- ti,min-charge-temperature: integer, minimum charging temperature (in DegC);
+- ti,max-charge-temperature: integer, maximum charging temperature (in DegC).
+
+Optional properties:
+- ti,thermal-sensing: boolean, if present thermal regulation will be enabled;
+- ti,enable-user-write: boolean, if present driver will allow the user space
+    to control the charging current and voltage through sysfs;
+
+Example:
+
+bq25890 {
+        compatible = "ti,bq24261
+        reg = <0x6b>;
+
+        ti,charge-current = <1000>;
+        ti,charge-voltage = <4200>;
+        ti,termination-current = <128>;
+        ti,max-charge-current = <3000>;
+        ti,max-charge-voltage = <4350>;
+        ti,min-charge-temperature = <0>;
+        ti,max-charge-temperature = <60>;
+
+        ti,thermal-sensing;
+        ti,enable-user-write;
+};
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index f8758d6..cd47d0d 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -396,6 +396,12 @@ config CHARGER_BQ24190
 	help
 	  Say Y to enable support for the TI BQ24190 battery charger.
 
+config CHARGER_BQ24261
+	tristate "TI BQ24261 charger driver"
+	depends on I2C && EXTCON
+	help
+	  Say Y here to enable support for TI BQ24261 battery charger.
+
 config CHARGER_BQ24257
 	tristate "TI BQ24257 battery charger driver"
 	depends on I2C
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 5752ce8..bec8409 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_CHARGER_MAX8997)	+= max8997_charger.o
 obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
 obj-$(CONFIG_CHARGER_BQ2415X)	+= bq2415x_charger.o
 obj-$(CONFIG_CHARGER_BQ24190)	+= bq24190_charger.o
+obj-$(CONFIG_CHARGER_BQ24261)	+= bq24261_charger.o
 obj-$(CONFIG_CHARGER_BQ24257)	+= bq24257_charger.o
 obj-$(CONFIG_CHARGER_BQ24735)	+= bq24735-charger.o
 obj-$(CONFIG_CHARGER_BQ25890)	+= bq25890_charger.o
diff --git a/drivers/power/bq24261_charger.c b/drivers/power/bq24261_charger.c
new file mode 100644
index 0000000..b01114e
--- /dev/null
+++ b/drivers/power/bq24261_charger.c
@@ -0,0 +1,1208 @@
+/*
+ * bq24261_charger.c - BQ24261 Charger driver
+ *
+ * Copyright (C) 2014 Intel Corporation
+ * Author:	Jenny TC <jenny.tc-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+ *		Ramakrishna Pallala <ramakrishna.pallala-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+ *
+ * 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.
+ *
+ * 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.
+ */
+
+#include <linux/version.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/power_supply.h>
+#include <linux/extcon.h>
+#include <linux/pm_runtime.h>
+#include <linux/io.h>
+#include <linux/sched.h>
+#include <linux/delay.h>
+#include <linux/acpi.h>
+#include <linux/power/bq24261_charger.h>
+
+#define DEV_NAME "bq24261_charger"
+
+#define EXCEPTION_MONITOR_DELAY		(60 * HZ)
+#define WDT_RESET_DELAY			(15 * HZ)
+
+/* BQ24261 registers */
+#define BQ24261_STAT_CTRL0_ADDR		0x00
+#define BQ24261_CTRL_ADDR		0x01
+#define BQ24261_BATT_VOL_CTRL_ADDR	0x02
+#define BQ24261_VENDOR_REV_ADDR		0x03
+#define BQ24261_TERM_FCC_ADDR		0x04
+#define BQ24261_VINDPM_STAT_ADDR	0x05
+#define BQ24261_ST_NTC_MON_ADDR		0x06
+
+#define BQ24261_RESET_MASK		(0x01 << 7)
+#define BQ24261_RESET_ENABLE		(0x01 << 7)
+
+#define BQ24261_FAULT_MASK		0x07
+#define BQ24261_STAT_MASK		(0x03 << 4)
+#define BQ24261_BOOST_MASK		(0x01 << 6)
+#define BQ24261_TMR_RST_MASK		(0x01 << 7)
+#define BQ24261_TMR_RST			(0x01 << 7)
+
+#define BQ24261_ENABLE_BOOST		(0x01 << 6)
+
+#define BQ24261_VOVP			0x01
+#define BQ24261_LOW_SUPPLY		0x02
+#define BQ24261_THERMAL_SHUTDOWN	0x03
+#define BQ24261_BATT_TEMP_FAULT		0x04
+#define BQ24261_TIMER_FAULT		0x05
+#define BQ24261_BATT_OVP		0x06
+#define BQ24261_NO_BATTERY		0x07
+#define BQ24261_STAT_READY		0x00
+
+#define BQ24261_STAT_CHRG_PRGRSS	(0x01 << 4)
+#define BQ24261_STAT_CHRG_DONE		(0x02 << 4)
+#define BQ24261_STAT_FAULT		(0x03 << 4)
+
+#define BQ24261_CE_MASK			(0x01 << 1)
+#define BQ24261_CE_DISABLE		(0x01 << 1)
+
+#define BQ24261_HiZ_MASK			(0x01)
+#define BQ24261_HiZ_ENABLE		(0x01)
+
+#define BQ24261_ICHRG_MASK		(0x1F << 3)
+
+#define BQ24261_ITERM_MASK		(0x03)
+#define BQ24261_MIN_ITERM		50 /* 50 mA */
+#define BQ24261_MAX_ITERM		300 /* 300 mA */
+
+#define BQ24261_VBREG_MASK		(0x3F << 2)
+#define BQ24261_VBREG_MIN_CV		3500
+#define BQ24261_VBREG_MAX_CV		4440
+#define BQ24261_VBREG_CV_DIV		20
+#define BQ24261_VBREG_CV_BIT_POS	2
+
+#define BQ24261_INLMT_MASK		(0x07 << 4)
+#define BQ24261_INLMT_100		0x00
+#define BQ24261_INLMT_150		(0x01 << 4)
+#define BQ24261_INLMT_500		(0x02 << 4)
+#define BQ24261_INLMT_900		(0x03 << 4)
+#define BQ24261_INLMT_1500		(0x04 << 4)
+#define BQ24261_INLMT_2000		(0x05 << 4)
+#define BQ24261_INLMT_2500		(0x06 << 4)
+
+#define BQ24261_TE_MASK			(0x01 << 2)
+#define BQ24261_TE_ENABLE		(0x01 << 2)
+#define BQ24261_STAT_ENABLE_MASK	(0x01 << 3)
+#define BQ24261_STAT_ENABLE		(0x01 << 3)
+
+#define BQ24261_VENDOR_MASK		(0x07 << 5)
+#define BQ24261_PART_MASK		(0x03 << 3)
+#define BQ24261_REV_MASK		(0x07)
+#define VENDOR_BQ2426X			(0x02 << 5)
+#define REV_BQ24261			(0x06)
+
+#define BQ24261_TS_MASK			(0x01 << 3)
+#define BQ24261_TS_ENABLED		(0x01 << 3)
+#define BQ24261_BOOST_ILIM_MASK		(0x01 << 4)
+#define BQ24261_BOOST_ILIM_500ma	(0x0)
+#define BQ24261_BOOST_ILIM_1A		(0x01 << 4)
+#define BQ24261_VINDPM_OFF_MASK		(0x01 << 0)
+#define BQ24261_VINDPM_OFF_5V		(0x0)
+#define BQ24261_VINDPM_OFF_12V		(0x01 << 0)
+
+#define BQ24261_SAFETY_TIMER_MASK	(0x03 << 5)
+#define BQ24261_SAFETY_TIMER_40MIN	0x00
+#define BQ24261_SAFETY_TIMER_6HR	(0x01 << 5)
+#define BQ24261_SAFETY_TIMER_9HR	(0x02 << 5)
+#define BQ24261_SAFETY_TIMER_DISABLED	(0x03 << 5)
+
+/* 1% above voltage max design to report over voltage */
+#define BQ24261_OVP_MULTIPLIER			1010
+#define BQ24261_OVP_RECOVER_MULTIPLIER		990
+#define BQ24261_DEF_BAT_VOLT_MAX_DESIGN		4200000
+
+/* Settings for Voltage / DPPM Register (05) */
+#define BQ24261_VBATT_LEVEL1		3700000
+#define BQ24261_VBATT_LEVEL2		3960000
+#define BQ24261_VINDPM_MASK		(0x07)
+#define BQ24261_VINDPM_320MV		(0x01 << 2)
+#define BQ24261_VINDPM_160MV		(0x01 << 1)
+#define BQ24261_VINDPM_80MV		(0x01 << 0)
+#define BQ24261_CD_STATUS_MASK		(0x01 << 3)
+#define BQ24261_DPM_EN_MASK		(0x01 << 4)
+#define BQ24261_DPM_EN_FORCE		(0x01 << 4)
+#define BQ24261_LOW_CHG_MASK		(0x01 << 5)
+#define BQ24261_LOW_CHG_EN		(0x01 << 5)
+#define BQ24261_LOW_CHG_DIS		(~BQ24261_LOW_CHG_EN)
+#define BQ24261_DPM_STAT_MASK		(0x01 << 6)
+#define BQ24261_MINSYS_STAT_MASK	(0x01 << 7)
+
+#define BQ24261_MIN_CC			500	/* 500mA */
+#define BQ24261_MAX_CC			3000	/* 3A */
+#define BQ24261_DEF_CC			1300	/* 1300mA */
+#define BQ24261_MAX_CV			4350	/*4350mV */
+#define BQ24261_DEF_CV			4350	/* 4350mV */
+#define BQ24261_DEF_ITERM		128	/* 128mA */
+#define BQ24261_MIN_TEMP		0	/* 0 degC */
+#define BQ24261_MAX_TEMP		60	/* 60 DegC */
+
+#define ILIM_100MA			100	/* 100mA */
+#define ILIM_500MA			500	/* 500mA */
+#define ILIM_900MA			900	/* 900mA */
+#define ILIM_1500MA			1500	/* 1500mA */
+#define ILIM_2000MA			2000	/* 2000mA */
+#define ILIM_2500MA			2500	/* 2500mA */
+#define ILIM_3000MA			3000	/* 3000mA */
+
+u16 bq24261_inlmt[][2] = {
+	{100, BQ24261_INLMT_100},
+	{150, BQ24261_INLMT_150},
+	{500, BQ24261_INLMT_500},
+	{900, BQ24261_INLMT_900},
+	{1500, BQ24261_INLMT_1500},
+	{2000, BQ24261_INLMT_2000},
+	{2500, BQ24261_INLMT_2500},
+};
+
+
+enum bq24261_status {
+	BQ24261_CHRGR_STAT_UNKNOWN,
+	BQ24261_CHRGR_STAT_READY,
+	BQ24261_CHRGR_STAT_CHARGING,
+	BQ24261_CHRGR_STAT_FULL,
+	BQ24261_CHRGR_STAT_FAULT,
+};
+
+enum bq2426x_model {
+	BQ2426X = 0,
+	BQ24260,
+	BQ24261,
+};
+
+struct bq24261_charger {
+	struct i2c_client *client;
+	struct bq24261_platform_data *pdata;
+	struct power_supply *psy_usb;
+	struct delayed_work fault_mon_work;
+	struct mutex lock;
+	enum bq2426x_model model;
+	struct delayed_work wdt_work;
+	struct work_struct irq_work;
+	struct list_head irq_queue;
+
+	/* extcon charger cables */
+	struct {
+		struct work_struct work;
+		struct notifier_block nb;
+		struct extcon_specific_cable_nb sdp;
+		struct extcon_specific_cable_nb cdp;
+		struct extcon_specific_cable_nb dcp;
+		struct extcon_specific_cable_nb otg;
+		enum power_supply_type chg_type;
+		bool boost;
+		bool connected;
+	} cable;
+
+	bool online;
+	bool present;
+	int chg_health;
+	enum bq24261_status chg_status;
+	int cc;
+	int cv;
+	int inlmt;
+	int max_cc;
+	int max_cv;
+	int iterm;
+	int max_temp;
+	int min_temp;
+	bool is_charging_enabled;
+};
+
+static inline int bq24261_read_reg(struct i2c_client *client, u8 reg)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, reg);
+	if (ret < 0)
+		dev_err(&client->dev,
+			"error(%d) in reading reg %d\n", ret, reg);
+	return ret;
+}
+
+static inline int bq24261_write_reg(struct i2c_client *client, u8 reg, u8 data)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client, reg, data);
+	if (ret < 0)
+		dev_err(&client->dev,
+			"error(%d) in writing %d to reg %d\n", ret, data, reg);
+	return ret;
+}
+
+static inline int bq24261_update_reg(struct i2c_client *client, u8 reg,
+					  u8 mask, u8 val)
+{
+	int ret;
+
+	ret = bq24261_read_reg(client, reg);
+	if (ret < 0)
+		return ret;
+
+	ret = (ret & ~mask) | (mask & val);
+	return bq24261_write_reg(client, reg, ret);
+}
+
+static void lookup_regval(u16 tbl[][2], size_t size, u16 in_val, u8 *out_val)
+{
+	int i;
+
+	for (i = 1; i < size; ++i) {
+		if (in_val < tbl[i][0])
+			break;
+	}
+
+	*out_val = (u8) tbl[i - 1][1];
+}
+
+void bq24261_cc_to_reg(int cc, u8 *reg_val)
+{
+	/* Ichrg bits are B3-B7
+	 * Icharge = 500mA + IchrgCode * 100mA
+	 */
+	cc = clamp_t(int, cc, BQ24261_MIN_CC, BQ24261_MAX_CC);
+	cc = cc - BQ24261_MIN_CC;
+	*reg_val = (cc / 100) << 3;
+}
+
+void bq24261_cv_to_reg(int cv, u8 *reg_val)
+{
+	int val;
+
+	val = clamp_t(int, cv, BQ24261_VBREG_MIN_CV, BQ24261_VBREG_MAX_CV);
+	*reg_val = (((val - BQ24261_VBREG_MIN_CV) / BQ24261_VBREG_CV_DIV)
+			<< BQ24261_VBREG_CV_BIT_POS);
+}
+
+void bq24261_inlmt_to_reg(int inlmt, u8 *regval)
+{
+	return lookup_regval(bq24261_inlmt, ARRAY_SIZE(bq24261_inlmt),
+			     inlmt, regval);
+}
+
+static inline void bq24261_iterm_to_reg(int iterm, u8 *regval)
+{
+	/* Iterm bits are B0-B2
+	 * Icharge = 50mA + ItermCode * 50mA
+	 */
+	iterm = clamp_t(int, iterm, BQ24261_MIN_ITERM,  BQ24261_MAX_ITERM);
+	iterm = iterm - BQ24261_MIN_ITERM;
+	*regval =  iterm / 50;
+}
+
+static inline int bq24261_init_timers(struct bq24261_charger *chip)
+{
+	u8 reg_val;
+	int ret;
+
+	reg_val = BQ24261_SAFETY_TIMER_9HR;
+
+	if (chip->pdata->thermal_sensing)
+		reg_val |= BQ24261_TS_ENABLED;
+
+	ret = bq24261_update_reg(chip->client, BQ24261_ST_NTC_MON_ADDR,
+			BQ24261_TS_MASK | BQ24261_SAFETY_TIMER_MASK |
+			BQ24261_BOOST_ILIM_MASK, reg_val);
+
+	return ret;
+}
+
+static inline int bq24261_reset_wdt_timer(struct bq24261_charger *chip)
+{
+	u8 mask = BQ24261_TMR_RST_MASK, val = BQ24261_TMR_RST;
+
+	if (chip->cable.boost) {
+		mask |= BQ24261_BOOST_MASK;
+		val |= BQ24261_ENABLE_BOOST;
+	}
+
+	return bq24261_update_reg(chip->client, BQ24261_STAT_CTRL0_ADDR,
+							mask, val);
+}
+
+static inline int bq24261_set_cc(struct bq24261_charger *chip, int cc_mA)
+{
+	u8 reg_val;
+	int ret;
+
+	dev_dbg(&chip->client->dev, "%s=%d\n", __func__, cc_mA);
+
+	if (cc_mA && (cc_mA < BQ24261_MIN_CC)) {
+		dev_dbg(&chip->client->dev, "Set LOW_CHG bit\n");
+		reg_val = BQ24261_LOW_CHG_EN;
+		ret = bq24261_update_reg(chip->client,
+				BQ24261_VINDPM_STAT_ADDR,
+				BQ24261_LOW_CHG_MASK, reg_val);
+		return ret;
+	}
+
+	reg_val = BQ24261_LOW_CHG_DIS;
+	ret = bq24261_update_reg(chip->client, BQ24261_VINDPM_STAT_ADDR,
+					BQ24261_LOW_CHG_MASK, reg_val);
+
+	bq24261_cc_to_reg(cc_mA, &reg_val);
+
+	return bq24261_update_reg(chip->client, BQ24261_TERM_FCC_ADDR,
+			BQ24261_ICHRG_MASK, reg_val);
+}
+
+static inline int bq24261_set_cv(struct bq24261_charger *chip, int cv_mV)
+{
+	u8 reg_val;
+
+	dev_dbg(&chip->client->dev, "%s=%d\n", __func__, cv_mV);
+
+	bq24261_cv_to_reg(cv_mV, &reg_val);
+
+	return bq24261_update_reg(chip->client, BQ24261_BATT_VOL_CTRL_ADDR,
+				       BQ24261_VBREG_MASK, reg_val);
+}
+
+static inline int bq24261_set_inlmt(struct bq24261_charger *chip, int inlmt)
+{
+	u8 reg_val;
+
+	dev_dbg(&chip->client->dev, "%s=%d\n", __func__, inlmt);
+
+	bq24261_inlmt_to_reg(inlmt, &reg_val);
+
+	/*
+	 * Don't enable reset bit. Setting this
+	 * bit will reset all the registers/
+	 */
+	reg_val &= ~BQ24261_RESET_MASK;
+
+	return bq24261_update_reg(chip->client, BQ24261_CTRL_ADDR,
+		       BQ24261_RESET_MASK|BQ24261_INLMT_MASK, reg_val);
+
+}
+
+static inline int bq24261_set_iterm(struct bq24261_charger *chip, int iterm)
+{
+	u8 reg_val;
+
+	bq24261_iterm_to_reg(iterm, &reg_val);
+
+	return bq24261_update_reg(chip->client, BQ24261_TERM_FCC_ADDR,
+				       BQ24261_ITERM_MASK, reg_val);
+}
+
+static inline int bq24261_enable_charging(struct bq24261_charger *chip,
+								bool enable)
+{
+	int ret;
+	u8 reg_val;
+
+	if (enable) {
+		reg_val = (~BQ24261_CE_DISABLE & BQ24261_CE_MASK);
+		reg_val |= BQ24261_TE_ENABLE;
+	} else {
+		reg_val = BQ24261_CE_DISABLE;
+	}
+
+	reg_val |=  BQ24261_STAT_ENABLE;
+
+	/*
+	 * Don't enable reset bit. Setting this
+	 * bit will reset all the registers/
+	 */
+	reg_val &= ~BQ24261_RESET_MASK;
+
+	ret = bq24261_update_reg(chip->client, BQ24261_CTRL_ADDR,
+		       BQ24261_STAT_ENABLE_MASK|BQ24261_RESET_MASK|
+				BQ24261_CE_MASK|BQ24261_TE_MASK,
+					reg_val);
+	if (ret || !enable)
+		return ret;
+
+	/* Set termination current */
+	ret = bq24261_set_iterm(chip, chip->iterm);
+	if (ret < 0)
+		dev_err(&chip->client->dev, "failed to set iTerm(%d)\n", ret);
+
+	/* Start WDT and Safety timers */
+	ret = bq24261_init_timers(chip);
+	if (ret)
+		dev_err(&chip->client->dev, "failed to set timers(%d)\n", ret);
+
+	return ret;
+}
+
+static inline int bq24261_enable_charger(struct bq24261_charger *chip,
+								int enable)
+{
+	u8 reg_val;
+	int ret;
+
+	reg_val = enable ? (~BQ24261_HiZ_ENABLE & BQ24261_HiZ_MASK) :
+			BQ24261_HiZ_ENABLE;
+
+	/*
+	 * Don't enable reset bit. Setting this
+	 * bit will reset all the registers/
+	 */
+	reg_val &= ~BQ24261_RESET_MASK;
+
+	ret = bq24261_update_reg(chip->client, BQ24261_CTRL_ADDR,
+		       BQ24261_HiZ_MASK|BQ24261_RESET_MASK, reg_val);
+	if (ret)
+		return ret;
+
+	return bq24261_reset_wdt_timer(chip);
+}
+
+static void bq24261_handle_health(struct bq24261_charger *chip, u8 stat_reg)
+{
+	struct i2c_client *client = chip->client;
+	bool fault_worker = false;
+
+	switch (stat_reg & BQ24261_FAULT_MASK) {
+	case BQ24261_VOVP:
+		chip->chg_health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+		fault_worker = true;
+		dev_err(&client->dev, "Charger Over Voltage Fault\n");
+		break;
+	case BQ24261_LOW_SUPPLY:
+		chip->chg_health = POWER_SUPPLY_HEALTH_DEAD;
+		fault_worker = true;
+		dev_err(&client->dev, "Charger Low Supply Fault\n");
+		break;
+	case BQ24261_THERMAL_SHUTDOWN:
+		chip->chg_health = POWER_SUPPLY_HEALTH_OVERHEAT;
+		dev_err(&client->dev, "Charger Thermal Fault\n");
+		break;
+	case BQ24261_BATT_TEMP_FAULT:
+		dev_err(&client->dev, "Battery Temperature Fault\n");
+		break;
+	case BQ24261_TIMER_FAULT:
+		chip->chg_health = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
+		dev_err(&client->dev, "Charger Timer Fault\n");
+		break;
+	case BQ24261_BATT_OVP:
+		chip->chg_health = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
+		dev_err(&client->dev, "Battery Over Voltage Fault\n");
+		break;
+	case BQ24261_NO_BATTERY:
+		dev_err(&client->dev, "No Battery Connected\n");
+		break;
+	default:
+		chip->chg_health = POWER_SUPPLY_HEALTH_GOOD;
+	}
+
+	if (fault_worker)
+		schedule_delayed_work(&chip->fault_mon_work,
+					EXCEPTION_MONITOR_DELAY);
+}
+
+static void bq24261_handle_status(struct bq24261_charger *chip, u8 stat_reg)
+{
+	struct i2c_client *client = chip->client;
+
+	switch (stat_reg & BQ24261_STAT_MASK) {
+	case BQ24261_STAT_READY:
+		chip->chg_status = BQ24261_CHRGR_STAT_READY;
+		dev_info(&client->dev, "Charger Status: Ready\n");
+		break;
+	case BQ24261_STAT_CHRG_PRGRSS:
+		chip->chg_status = BQ24261_CHRGR_STAT_CHARGING;
+		dev_info(&client->dev, "Charger Status: Charge Progress\n");
+		break;
+	case BQ24261_STAT_CHRG_DONE:
+		chip->chg_status = BQ24261_CHRGR_STAT_FULL;
+		dev_info(&client->dev, "Charger Status: Charge Done\n");
+		break;
+	case BQ24261_STAT_FAULT:
+		chip->chg_status = BQ24261_CHRGR_STAT_FAULT;
+		dev_warn(&client->dev, "Charger Status: Fault\n");
+		break;
+	default:
+		dev_info(&client->dev, "Invalid\n");
+	}
+}
+
+static int bq24261_get_charger_health(struct bq24261_charger *chip)
+{
+	if (!chip->present)
+		return POWER_SUPPLY_HEALTH_UNKNOWN;
+
+	return chip->chg_health;
+}
+
+static int bq24261_get_charging_status(struct bq24261_charger *chip)
+{
+	int status;
+
+	if (!chip->present)
+		return POWER_SUPPLY_STATUS_DISCHARGING;
+
+	switch (chip->chg_status) {
+	case BQ24261_CHRGR_STAT_READY:
+		status = POWER_SUPPLY_STATUS_DISCHARGING;
+		break;
+	case BQ24261_CHRGR_STAT_CHARGING:
+		status = POWER_SUPPLY_STATUS_CHARGING;
+		break;
+	case BQ24261_CHRGR_STAT_FULL:
+		status = POWER_SUPPLY_STATUS_FULL;
+		break;
+	case BQ24261_CHRGR_STAT_FAULT:
+		status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	default:
+		status = POWER_SUPPLY_STATUS_DISCHARGING;
+	}
+
+	return status;
+}
+
+static int bq24261_usb_get_property(struct power_supply *psy,
+		enum power_supply_property psp, union power_supply_propval *val)
+{
+	struct bq24261_charger *chip = power_supply_get_drvdata(psy);
+
+	mutex_lock(&chip->lock);
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = chip->present;
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = chip->online;
+		break;
+	case POWER_SUPPLY_PROP_HEALTH:
+		val->intval = bq24261_get_charger_health(chip);
+		break;
+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = bq24261_get_charging_status(chip);
+		break;
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+		val->intval = chip->pdata->max_cc * 1000;
+		break;
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
+		val->intval = chip->pdata->max_cv * 1000;
+		break;
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
+		val->intval = chip->cc * 1000;
+		break;
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
+		val->intval = chip->cv * 1000;
+		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		val->intval = chip->inlmt * 1000;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
+		val->intval = chip->iterm * 1000;
+		break;
+	case POWER_SUPPLY_PROP_TEMP_MAX:
+		val->intval = chip->pdata->max_temp * 10;
+		break;
+	case POWER_SUPPLY_PROP_TEMP_MIN:
+		val->intval = chip->pdata->min_temp * 10;
+		break;
+	default:
+		mutex_unlock(&chip->lock);
+		return -EINVAL;
+	}
+	mutex_unlock(&chip->lock);
+
+	return 0;
+}
+
+static int bq24261_usb_set_property(struct power_supply *psy,
+	enum power_supply_property psp, const union power_supply_propval *val)
+{
+	struct bq24261_charger *chip = power_supply_get_drvdata(psy);
+	int intval, ret = 0;
+
+	mutex_lock(&chip->lock);
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
+		/* convert uA to mA */
+		intval = val->intval / 1000;
+		if (intval > chip->max_cc) {
+			ret = -EINVAL;
+			break;
+		}
+
+		ret = bq24261_set_cc(chip, intval);
+		if (!ret)
+			chip->cc = intval;
+		break;
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
+		/* convert uA to mV */
+		intval = val->intval / 1000;
+		if (intval > chip->max_cv) {
+			ret = -EINVAL;
+			break;
+		}
+
+		ret = bq24261_set_cv(chip, intval);
+		if (!ret)
+			chip->cv = intval;
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
+static int bq24261_property_is_writeable(struct power_supply *psy,
+		enum power_supply_property psp)
+{
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
+		ret = 1;
+		break;
+	default:
+		ret = 0;
+	}
+
+	return ret;
+}
+
+static enum power_supply_property bq24261_usb_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_TYPE,
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
+	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+	POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
+	POWER_SUPPLY_PROP_TEMP_MAX,
+	POWER_SUPPLY_PROP_TEMP_MIN,
+};
+
+static char *bq24261_charger_supplied_to[] = {
+	"main-battery",
+};
+
+static struct power_supply_desc bq24261_charger_desc = {
+	.name			= DEV_NAME,
+	.type			= POWER_SUPPLY_TYPE_USB,
+	.properties		= bq24261_usb_props,
+	.num_properties		= ARRAY_SIZE(bq24261_usb_props),
+	.get_property		= bq24261_usb_get_property,
+};
+
+static void bq24261_wdt_reset_worker(struct work_struct *work)
+{
+
+	struct bq24261_charger *chip = container_of(work,
+			    struct bq24261_charger, wdt_work.work);
+	int ret;
+
+	ret = bq24261_reset_wdt_timer(chip);
+	if (ret)
+		dev_err(&chip->client->dev, "WDT timer reset error(%d)\n", ret);
+
+	schedule_delayed_work(&chip->wdt_work, WDT_RESET_DELAY);
+}
+
+static void bq24261_irq_worker(struct work_struct *work)
+{
+	struct bq24261_charger *chip =
+	    container_of(work, struct bq24261_charger, irq_work);
+	int ret;
+
+	/*
+	 * Lock to ensure that interrupt register readings are done
+	 * and processed sequentially. The interrupt Fault registers
+	 * are read on clear and without sequential processing double
+	 * fault interrupts or fault recovery cannot be handlled propely
+	 */
+
+	mutex_lock(&chip->lock);
+
+	ret = bq24261_read_reg(chip->client, BQ24261_STAT_CTRL0_ADDR);
+	if (ret < 0) {
+		dev_err(&chip->client->dev,
+			"Error (%d) in reading BQ24261_STAT_CTRL0_ADDR\n", ret);
+		goto irq_out;
+	}
+
+	if (!chip->cable.boost) {
+		bq24261_handle_status(chip, ret);
+		bq24261_handle_health(chip, ret);
+		power_supply_changed(chip->psy_usb);
+	}
+
+irq_out:
+	mutex_unlock(&chip->lock);
+}
+
+static irqreturn_t bq24261_thread_handler(int id, void *data)
+{
+	struct bq24261_charger *chip = (struct bq24261_charger *)data;
+
+	queue_work(system_highpri_wq, &chip->irq_work);
+	return IRQ_HANDLED;
+}
+
+static void bq24261_fault_mon_work(struct work_struct *work)
+{
+	struct bq24261_charger *chip = container_of(work,
+			struct bq24261_charger, fault_mon_work.work);
+	int ret;
+
+	if ((chip->chg_health == POWER_SUPPLY_HEALTH_OVERVOLTAGE) ||
+		(chip->chg_health == POWER_SUPPLY_HEALTH_DEAD)) {
+
+		mutex_lock(&chip->lock);
+		ret = bq24261_read_reg(chip->client, BQ24261_STAT_CTRL0_ADDR);
+		if (ret < 0) {
+			dev_err(&chip->client->dev,
+				"Status register read failed(%d)\n", ret);
+			goto fault_mon_out;
+		}
+
+		if ((ret & BQ24261_STAT_MASK) == BQ24261_STAT_READY) {
+			dev_info(&chip->client->dev,
+					"Charger fault recovered\n");
+			bq24261_handle_status(chip, ret);
+			bq24261_handle_health(chip, ret);
+			power_supply_changed(chip->psy_usb);
+		}
+fault_mon_out:
+		mutex_unlock(&chip->lock);
+	}
+}
+
+static void bq24261_boost_control(struct bq24261_charger *chip, bool enable)
+{
+	int ret;
+
+	if (enable)
+		ret = bq24261_write_reg(chip->client, BQ24261_STAT_CTRL0_ADDR,
+				BQ24261_TMR_RST | BQ24261_ENABLE_BOOST);
+	else
+		ret = bq24261_write_reg(chip->client,
+						BQ24261_STAT_CTRL0_ADDR, 0x0);
+
+	if (ret < 0)
+		dev_err(&chip->client->dev,
+			"stat cntl0 reg access error(%d)\n", ret);
+}
+
+static void bq24261_extcon_event_work(struct work_struct *work)
+{
+	struct bq24261_charger *chip =
+			container_of(work, struct bq24261_charger, cable.work);
+	int ret, current_limit = 0;
+	bool old_connected = chip->cable.connected;
+
+	/* Determine cable/charger type */
+	if (extcon_get_cable_state(chip->cable.sdp.edev,
+					"SLOW-CHARGER") > 0) {
+		chip->cable.connected = true;
+		current_limit = ILIM_500MA;
+		chip->cable.chg_type = POWER_SUPPLY_TYPE_USB;
+		dev_dbg(&chip->client->dev, "USB SDP charger is connected");
+	} else if (extcon_get_cable_state(chip->cable.cdp.edev,
+					"CHARGE-DOWNSTREAM") > 0) {
+		chip->cable.connected = true;
+		current_limit = ILIM_1500MA;
+		chip->cable.chg_type = POWER_SUPPLY_TYPE_USB_CDP;
+		dev_dbg(&chip->client->dev, "USB CDP charger is connected");
+	} else if (extcon_get_cable_state(chip->cable.dcp.edev,
+					"FAST-CHARGER") > 0) {
+		chip->cable.connected = true;
+		current_limit = ILIM_1500MA;
+		chip->cable.chg_type = POWER_SUPPLY_TYPE_USB_DCP;
+		dev_dbg(&chip->client->dev, "USB DCP charger is connected");
+	} else if (extcon_get_cable_state(chip->cable.otg.edev,
+					"USB-Host") > 0) {
+		chip->cable.boost = true;
+		chip->cable.connected = true;
+		dev_dbg(&chip->client->dev, "USB-Host cable is connected");
+	} else {
+		if (old_connected)
+			dev_dbg(&chip->client->dev, "USB Cable disconnected");
+		chip->cable.connected = false;
+		chip->cable.boost = false;
+		chip->cable.chg_type = POWER_SUPPLY_TYPE_USB;
+	}
+
+	/* Cable status changed */
+	if (old_connected == chip->cable.connected)
+		return;
+
+	mutex_lock(&chip->lock);
+	if (chip->cable.connected && !chip->cable.boost) {
+		chip->inlmt = current_limit;
+		/* Set up charging */
+		ret = bq24261_set_cc(chip, chip->cc);
+		if (ret < 0)
+			dev_err(&chip->client->dev, "set CC failed(%d)", ret);
+		ret = bq24261_set_cv(chip, chip->cv);
+		if (ret < 0)
+			dev_err(&chip->client->dev, "set CV failed(%d)", ret);
+		ret = bq24261_set_inlmt(chip, chip->inlmt);
+		if (ret < 0)
+			dev_err(&chip->client->dev, "set ILIM failed(%d)", ret);
+		ret = bq24261_enable_charger(chip, true);
+		if (ret < 0)
+			dev_err(&chip->client->dev,
+					"enable charger failed(%d)", ret);
+		ret = bq24261_enable_charging(chip, true);
+		if (ret < 0)
+			dev_err(&chip->client->dev,
+					"enable charging failed(%d)", ret);
+
+		chip->is_charging_enabled = true;
+		chip->present = true;
+		chip->online = true;
+		schedule_delayed_work(&chip->wdt_work, 0);
+	} else if (chip->cable.connected && chip->cable.boost) {
+		/* Enable VBUS for Host Mode */
+		bq24261_boost_control(chip, true);
+		schedule_delayed_work(&chip->wdt_work, 0);
+	} else {
+		dev_info(&chip->client->dev, "Cable disconnect event\n");
+		cancel_delayed_work_sync(&chip->wdt_work);
+		cancel_delayed_work_sync(&chip->fault_mon_work);
+		bq24261_boost_control(chip, false);
+		ret = bq24261_enable_charging(chip, false);
+		if (ret < 0)
+			dev_err(&chip->client->dev,
+					"charger disable failed(%d)", ret);
+
+		chip->is_charging_enabled = false;
+		chip->present = false;
+		chip->online = false;
+		chip->inlmt = 0;
+	}
+	bq24261_charger_desc.type = chip->cable.chg_type;
+	mutex_unlock(&chip->lock);
+	power_supply_changed(chip->psy_usb);
+}
+
+static int bq24261_handle_extcon_events(struct notifier_block *nb,
+				   unsigned long event, void *param)
+{
+	struct bq24261_charger *chip =
+		container_of(nb, struct bq24261_charger, cable.nb);
+
+	dev_dbg(&chip->client->dev, "external connector event(%ld)\n", event);
+
+	schedule_work(&chip->cable.work);
+	return NOTIFY_OK;
+}
+
+static int bq24261_extcon_register(struct bq24261_charger *chip)
+{
+	int ret;
+
+	INIT_WORK(&chip->cable.work, bq24261_extcon_event_work);
+	chip->cable.nb.notifier_call = bq24261_handle_extcon_events;
+
+	ret = extcon_register_interest(&chip->cable.sdp, NULL,
+				"SLOW-CHARGER", &chip->cable.nb);
+	if (ret < 0) {
+		dev_warn(&chip->client->dev,
+				"extcon SDP registration failed(%d)\n", ret);
+		goto sdp_reg_failed;
+	}
+
+	ret = extcon_register_interest(&chip->cable.cdp, NULL,
+				"CHARGE-DOWNSTREAM", &chip->cable.nb);
+	if (ret < 0) {
+		dev_warn(&chip->client->dev,
+				"extcon CDP registration failed(%d)\n", ret);
+		goto cdp_reg_failed;
+	}
+
+	ret = extcon_register_interest(&chip->cable.dcp, NULL,
+				"FAST-CHARGER", &chip->cable.nb);
+	if (ret < 0) {
+		dev_warn(&chip->client->dev,
+				"extcon DCP registration failed(%d)\n", ret);
+		goto dcp_reg_failed;
+	}
+
+	ret = extcon_register_interest(&chip->cable.otg, NULL,
+				"USB-Host", &chip->cable.nb);
+	if (ret < 0) {
+		dev_warn(&chip->client->dev,
+			"extcon USB-Host registration failed(%d)\n", ret);
+		goto otg_reg_failed;
+	}
+
+	return 0;
+
+otg_reg_failed:
+	extcon_unregister_interest(&chip->cable.dcp);
+dcp_reg_failed:
+	extcon_unregister_interest(&chip->cable.cdp);
+cdp_reg_failed:
+	extcon_unregister_interest(&chip->cable.sdp);
+sdp_reg_failed:
+	return -EPROBE_DEFER;
+}
+
+static void bq24261_of_pdata(struct bq24261_charger *chip)
+{
+	static struct bq24261_platform_data pdata;
+	struct device *dev = &chip->client->dev;
+	int ret;
+
+	ret = device_property_read_u32(dev,
+				"ti,charge-current", &pdata.def_cc);
+	if (ret < 0)
+		goto of_err;
+	ret = device_property_read_u32(dev,
+				"ti,charge-voltage", &pdata.def_cv);
+	if (ret < 0)
+		goto of_err;
+	ret = device_property_read_u32(dev,
+				"ti,termination-current", &pdata.iterm);
+	if (ret < 0)
+		goto of_err;
+	ret = device_property_read_u32(dev,
+				"ti,max-charge-current", &pdata.max_cc);
+	if (ret < 0)
+		goto of_err;
+	ret = device_property_read_u32(dev,
+				"ti,max-charge-voltage", &pdata.max_cv);
+	if (ret < 0)
+		goto of_err;
+	ret = device_property_read_u32(dev,
+				"ti,min-charge-temperature", &pdata.min_temp);
+	if (ret < 0)
+		goto of_err;
+	ret = device_property_read_u32(dev,
+				"ti,max-charge-temperature", &pdata.max_temp);
+	if (ret < 0)
+		goto of_err;
+
+	pdata.thermal_sensing = device_property_read_bool(dev,
+						"ti,thermal-sensing");
+	pdata.en_user_write = device_property_read_bool(dev,
+						"ti,enable-user-write");
+
+	chip->pdata = &pdata;
+	return;
+of_err:
+	dev_err(dev, "error in getting DT property(%d)\n", ret);
+}
+
+static int bq24261_get_model(struct i2c_client *client,
+			enum bq2426x_model *model)
+{
+	int ret;
+
+	ret = bq24261_read_reg(client, BQ24261_VENDOR_REV_ADDR);
+	if (ret < 0)
+		return ret;
+
+	if ((ret & BQ24261_VENDOR_MASK) != VENDOR_BQ2426X)
+		return -EINVAL;
+
+	switch (ret & BQ24261_REV_MASK) {
+	case REV_BQ24261:
+		*model = BQ24261;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int bq24261_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+	struct power_supply_config charger_cfg = {};
+	struct bq24261_charger *chip;
+	int ret;
+	enum bq2426x_model model;
+
+	adapter = to_i2c_adapter(client->dev.parent);
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		dev_err(&client->dev,
+			"I2C adapter %s doesn'tsupport BYTE DATA transfer\n",
+			adapter->name);
+		return -EIO;
+	}
+
+	ret = bq24261_get_model(client, &model);
+	if (ret < 0) {
+		dev_err(&client->dev, "chip detection error (%d)\n", ret);
+		return -ENODEV;
+	}
+
+	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->client = client;
+	if (client->dev.platform_data)
+		chip->pdata = client->dev.platform_data;
+	else if (id->driver_data)
+		chip->pdata = (struct bq24261_platform_data *)id->driver_data;
+	else
+		bq24261_of_pdata(chip);
+
+	if (!chip->pdata) {
+		dev_err(&client->dev, "platform data not found");
+		return -ENODEV;
+	}
+
+	i2c_set_clientdata(client, chip);
+	mutex_init(&chip->lock);
+	chip->model = model;
+
+	/* Initialize charger parameters */
+	chip->cc = chip->pdata->def_cc;
+	chip->cv = chip->pdata->def_cv;
+	chip->iterm = chip->pdata->iterm;
+	chip->chg_status = BQ24261_CHRGR_STAT_UNKNOWN;
+	chip->chg_health = POWER_SUPPLY_HEALTH_UNKNOWN;
+
+	charger_cfg.drv_data = chip;
+	charger_cfg.supplied_to = bq24261_charger_supplied_to;
+	charger_cfg.num_supplicants = ARRAY_SIZE(bq24261_charger_supplied_to);
+	if (chip->pdata->en_user_write) {
+		bq24261_charger_desc.set_property = bq24261_usb_set_property;
+		bq24261_charger_desc.property_is_writeable =
+						bq24261_property_is_writeable;
+	}
+	chip->psy_usb = power_supply_register(&client->dev,
+				&bq24261_charger_desc, &charger_cfg);
+	if (IS_ERR(chip->psy_usb)) {
+		dev_err(&client->dev,
+			"power supply registration failed(%d)\n", ret);
+		return ret;
+	}
+
+	INIT_DELAYED_WORK(&chip->wdt_work, bq24261_wdt_reset_worker);
+	INIT_DELAYED_WORK(&chip->fault_mon_work, bq24261_fault_mon_work);
+
+	ret = bq24261_extcon_register(chip);
+	if (ret < 0)
+		goto extcon_reg_failed;
+
+	if (chip->client->irq) {
+		ret = request_threaded_irq(chip->client->irq,
+					   NULL, bq24261_thread_handler,
+					   IRQF_SHARED | IRQF_NO_SUSPEND,
+					   DEV_NAME, chip);
+		if (ret) {
+			dev_err(&client->dev,
+				"irq request failed (%d)\n", ret);
+			goto irq_reg_failed;
+		}
+		INIT_WORK(&chip->irq_work, bq24261_irq_worker);
+	}
+
+	/* Check for charger connecetd boot case */
+	schedule_work(&chip->cable.work);
+
+	return 0;
+
+irq_reg_failed:
+	extcon_unregister_interest(&chip->cable.sdp);
+	extcon_unregister_interest(&chip->cable.cdp);
+	extcon_unregister_interest(&chip->cable.dcp);
+	extcon_unregister_interest(&chip->cable.otg);
+extcon_reg_failed:
+	power_supply_unregister(chip->psy_usb);
+	return ret;
+}
+
+static int bq24261_remove(struct i2c_client *client)
+{
+	struct bq24261_charger *chip = i2c_get_clientdata(client);
+
+	free_irq(client->irq, chip);
+	flush_scheduled_work();
+	extcon_unregister_interest(&chip->cable.sdp);
+	extcon_unregister_interest(&chip->cable.cdp);
+	extcon_unregister_interest(&chip->cable.dcp);
+	extcon_unregister_interest(&chip->cable.otg);
+	power_supply_unregister(chip->psy_usb);
+	return 0;
+}
+
+static int bq24261_suspend(struct device *dev)
+{
+	struct bq24261_charger *chip = dev_get_drvdata(dev);
+
+	dev_dbg(&chip->client->dev, "bq24261 suspend\n");
+	return 0;
+}
+
+static int bq24261_resume(struct device *dev)
+{
+	struct bq24261_charger *chip = dev_get_drvdata(dev);
+
+	dev_dbg(&chip->client->dev, "bq24261 resume\n");
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(bq24261_pm_ops, bq24261_suspend,
+			bq24261_resume);
+
+static const struct i2c_device_id bq24261_id[] = {
+	{"bq24261", 0},
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, bq24261_id);
+
+static const struct acpi_device_id bq24261_acpi_match[] = {
+	{"BQ24261", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, bq24261_acpi_match);
+
+static const struct of_device_id bq24261_of_match[] = {
+	{ .compatible = "ti,bq24261", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, bq24261_of_match);
+
+static struct i2c_driver bq24261_driver = {
+	.driver = {
+		.name = DEV_NAME,
+		.acpi_match_table = ACPI_PTR(bq24261_acpi_match),
+		.of_match_table = of_match_ptr(bq24261_of_match),
+		.pm = &bq24261_pm_ops,
+	},
+	.probe = bq24261_probe,
+	.remove = bq24261_remove,
+	.id_table = bq24261_id,
+};
+
+module_i2c_driver(bq24261_driver);
+
+MODULE_AUTHOR("Jenny TC <jenny.tc-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
+MODULE_AUTHOR("Ramakrishna Pallala <ramakrishna.pallala-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("BQ24261 Charger Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/power/bq24261_charger.h b/include/linux/power/bq24261_charger.h
new file mode 100644
index 0000000..3ac2986
--- /dev/null
+++ b/include/linux/power/bq24261_charger.h
@@ -0,0 +1,27 @@
+/*
+ * bq24261_charger.h: platform data structure for bq24261 driver
+ *
+ * Copyright (C) 2014 Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+
+#ifndef __BQ24261_CHARGER_H__
+#define __BQ24261_CHARGER_H__
+
+struct bq24261_platform_data {
+	int def_cc;	/* in mA */
+	int def_cv;	/* in mV */
+	int iterm;	/* in mA */
+	int max_cc;	/* in mA */
+	int max_cv;	/* in mV */
+	int min_temp;	/* in DegC */
+	int max_temp;	/* in DegC */
+	bool thermal_sensing;
+	bool en_user_write;
+};
+
+#endif
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Krzysztof Kozlowski]

2015-09-07 2:23 GMT+09:00 Ramakrishna Pallala <ramakrishna.pallala@intel.com>:
>
> Add new charger driver support for BQ24261 charger IC.
>
> BQ24261 charger driver relies on extcon notifications to get the
> charger cable type and based on that it will set the charging parameters.
>
> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> Signed-off-by: Jennt TC <jenny.tc@intel.com>
> ---
>  .../devicetree/bindings/power/bq24261.txt          |   37 +
>  drivers/power/Kconfig                              |    6 +
>  drivers/power/Makefile                             |    1 +
>  drivers/power/bq24261_charger.c                    | 1208 ++++++++++++++++++++
>  include/linux/power/bq24261_charger.h              |   27 +
>  5 files changed, 1279 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/bq24261.txt
>  create mode 100644 drivers/power/bq24261_charger.c
>  create mode 100644 include/linux/power/bq24261_charger.h
>
> diff --git a/Documentation/devicetree/bindings/power/bq24261.txt b/Documentation/devicetree/bindings/power/bq24261.txt
> new file mode 100644
> index 0000000..25fc5c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/bq24261.txt
> @@ -0,0 +1,37 @@
> +Binding for TI bq24261 Li-Ion Charger

Please split the bindings into separate patch (the first patch in patchset).

> +
> +Required properties:
> +- compatible: Should contain one of the following:
> +    * "ti,bq24261"
> +- reg: integer, i2c address of the device.
> +- ti,charge-current: integer, default charging current (in mA);
> +- ti,charge-voltage: integer, default charging voltage (in mV);
> +- ti,termination-current: integer, charge will be terminated when current in
> +    constant-voltage phase drops below this value (in mA);
> +- ti,max-charge-current: integer, maximum charging current (in mA);
> +- ti,max-charge-voltage: integer, maximum charging voltage (in mV);
> +- ti,min-charge-temperature: integer, minimum charging temperature (in DegC);
> +- ti,max-charge-temperature: integer, maximum charging temperature (in DegC).

Before accepting "[PATCH 13/13] dt: power: bq24257-charger: Cover
additional devices"
http://www.spinics.net/lists/devicetree/msg92134.html

could you and Andreas figure out common bindings? Look at this:

+- ti,charge-current: integer, maximum charging current in uA.
+- ti,charge-current: integer, default charging current (in mA);

Different meaning and different units. This is madness! :)

In the same time you are adding TI-common bindings (not device
specific, there is no prefix) so I would expect exactly the same
bindings if it is possible.


> +
> +Optional properties:
> +- ti,thermal-sensing: boolean, if present thermal regulation will be enabled;

What is the requirement for thermal-sensing? Can it be enabled always?
If yes, then this is not really a hardware property.

> +- ti,enable-user-write: boolean, if present driver will allow the user space
> +    to control the charging current and voltage through sysfs;

This is not DT property. It does not describe hardware.

Best regards,
Krzysztof

> +
> +Example:
> +
> +bq25890 {
> +        compatible = "ti,bq24261
> +        reg = <0x6b>;
> +
> +        ti,charge-current = <1000>;
> +        ti,charge-voltage = <4200>;
> +        ti,termination-current = <128>;
> +        ti,max-charge-current = <3000>;
> +        ti,max-charge-voltage = <4350>;
> +        ti,min-charge-temperature = <0>;
> +        ti,max-charge-temperature = <60>;
> +
> +        ti,thermal-sensing;
> +        ti,enable-user-write;
> +};
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index f8758d6..cd47d0d 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -396,6 +396,12 @@ config CHARGER_BQ24190
>         help
>           Say Y to enable support for the TI BQ24190 battery charger.
>
> +config CHARGER_BQ24261
> +       tristate "TI BQ24261 charger driver"
> +       depends on I2C && EXTCON
> +       help
> +         Say Y here to enable support for TI BQ24261 battery charger.
> +
>  config CHARGER_BQ24257
>         tristate "TI BQ24257 battery charger driver"
>         depends on I2C
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index 5752ce8..bec8409 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -59,6 +59,7 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
>  obj-$(CONFIG_CHARGER_MAX8998)  += max8998_charger.o
>  obj-$(CONFIG_CHARGER_BQ2415X)  += bq2415x_charger.o
>  obj-$(CONFIG_CHARGER_BQ24190)  += bq24190_charger.o
> +obj-$(CONFIG_CHARGER_BQ24261)  += bq24261_charger.o
>  obj-$(CONFIG_CHARGER_BQ24257)  += bq24257_charger.o
>  obj-$(CONFIG_CHARGER_BQ24735)  += bq24735-charger.o
>  obj-$(CONFIG_CHARGER_BQ25890)  += bq25890_charger.o

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Andreas Dannenberg]

On Mon, Sep 07, 2015 at 12:57:56PM +0900, Krzysztof Kozlowski wrote:
> 2015-09-07 2:23 GMT+09:00 Ramakrishna Pallala <ramakrishna.pallala@intel.com>:
> >
> > Add new charger driver support for BQ24261 charger IC.
> >
> > BQ24261 charger driver relies on extcon notifications to get the
> > charger cable type and based on that it will set the charging parameters.
> >
> > Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> > Signed-off-by: Jennt TC <jenny.tc@intel.com>
> > ---
> >  .../devicetree/bindings/power/bq24261.txt          |   37 +
> >  drivers/power/Kconfig                              |    6 +
> >  drivers/power/Makefile                             |    1 +
> >  drivers/power/bq24261_charger.c                    | 1208 ++++++++++++++++++++
> >  include/linux/power/bq24261_charger.h              |   27 +
> >  5 files changed, 1279 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/power/bq24261.txt
> >  create mode 100644 drivers/power/bq24261_charger.c
> >  create mode 100644 include/linux/power/bq24261_charger.h
> >
> > diff --git a/Documentation/devicetree/bindings/power/bq24261.txt b/Documentation/devicetree/bindings/power/bq24261.txt
> > new file mode 100644
> > index 0000000..25fc5c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/bq24261.txt
> > @@ -0,0 +1,37 @@
> > +Binding for TI bq24261 Li-Ion Charger
> 
> Please split the bindings into separate patch (the first patch in patchset).
> 
> > +
> > +Required properties:
> > +- compatible: Should contain one of the following:
> > +    * "ti,bq24261"
> > +- reg: integer, i2c address of the device.
> > +- ti,charge-current: integer, default charging current (in mA);
> > +- ti,charge-voltage: integer, default charging voltage (in mV);
> > +- ti,termination-current: integer, charge will be terminated when current in
> > +    constant-voltage phase drops below this value (in mA);
> > +- ti,max-charge-current: integer, maximum charging current (in mA);
> > +- ti,max-charge-voltage: integer, maximum charging voltage (in mV);
> > +- ti,min-charge-temperature: integer, minimum charging temperature (in DegC);
> > +- ti,max-charge-temperature: integer, maximum charging temperature (in DegC).
> 
> Before accepting "[PATCH 13/13] dt: power: bq24257-charger: Cover
> additional devices"
> http://www.spinics.net/lists/devicetree/msg92134.html
> 
> could you and Andreas figure out common bindings? Look at this:
> 
> +- ti,charge-current: integer, maximum charging current in uA.
> +- ti,charge-current: integer, default charging current (in mA);
> 
> Different meaning and different units. This is madness! :)
> 
> In the same time you are adding TI-common bindings (not device
> specific, there is no prefix) so I would expect exactly the same
> bindings if it is possible.

Krzysztof, good observation! In bq2425x_charger.c (formerly known as
bq24257_charger.c :) that I worked on the unit used was uA. At that time
I did a quick check and there didn't seem to be a clear standard whether
to use the "micro" or "milli" units - different drivers use different
units. However there seems to be a tendency for the TI drivers to prefer
"milli" (bq2415x_charger.c, bq24735-charger.c)

Personally I think "milli" units are more appropriate for chargers since
they provide sufficient granularity and the numbers don't become too big
(try typing a voltage in the Volt-range in uV, it's very easy to get the
number of 0s wrong). However since the driver was already there I left
that aspect alone to preserve compatibility.

> > +
> > +Optional properties:
> > +- ti,thermal-sensing: boolean, if present thermal regulation will be enabled;
> 
> What is the requirement for thermal-sensing? Can it be enabled always?
> If yes, then this is not really a hardware property.
> 
> > +- ti,enable-user-write: boolean, if present driver will allow the user space
> > +    to control the charging current and voltage through sysfs;
> 
> This is not DT property. It does not describe hardware.

I take responsibility for this one :) In an earlier thread we were
discussing that it will be better to prevent sysfs write access to
"dangerous" charger parameters (charge voltage, current, ...) but I
argued that such access might still be desirable during development and
debug, and that a DT property could be used to allow such write access
(with the default being the charger properties being made read-only) -
this way giving the best of both worlds. However then when I updated the
bq24157_charger.c driver I ended up not really needing and implementing
this feature.

Out of curiosity, if it against best-practice to expose non hardware
properties, how would one best address the above use case? Compile time
switch? A sysfs property that allows write-enabling the other sysfs
properties (doesn't sound right). Or...?

--
Andreas Dannenberg
Texas Instruments Inc

> Best regards,
> Krzysztof
> 
> > +
> > +Example:
> > +
> > +bq25890 {
> > +        compatible = "ti,bq24261
> > +        reg = <0x6b>;
> > +
> > +        ti,charge-current = <1000>;
> > +        ti,charge-voltage = <4200>;
> > +        ti,termination-current = <128>;
> > +        ti,max-charge-current = <3000>;
> > +        ti,max-charge-voltage = <4350>;
> > +        ti,min-charge-temperature = <0>;
> > +        ti,max-charge-temperature = <60>;
> > +
> > +        ti,thermal-sensing;
> > +        ti,enable-user-write;
> > +};
> > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> > index f8758d6..cd47d0d 100644
> > --- a/drivers/power/Kconfig
> > +++ b/drivers/power/Kconfig
> > @@ -396,6 +396,12 @@ config CHARGER_BQ24190
> >         help
> >           Say Y to enable support for the TI BQ24190 battery charger.
> >
> > +config CHARGER_BQ24261
> > +       tristate "TI BQ24261 charger driver"
> > +       depends on I2C && EXTCON
> > +       help
> > +         Say Y here to enable support for TI BQ24261 battery charger.
> > +
> >  config CHARGER_BQ24257
> >         tristate "TI BQ24257 battery charger driver"
> >         depends on I2C
> > diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> > index 5752ce8..bec8409 100644
> > --- a/drivers/power/Makefile
> > +++ b/drivers/power/Makefile
> > @@ -59,6 +59,7 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
> >  obj-$(CONFIG_CHARGER_MAX8998)  += max8998_charger.o
> >  obj-$(CONFIG_CHARGER_BQ2415X)  += bq2415x_charger.o
> >  obj-$(CONFIG_CHARGER_BQ24190)  += bq24190_charger.o
> > +obj-$(CONFIG_CHARGER_BQ24261)  += bq24261_charger.o
> >  obj-$(CONFIG_CHARGER_BQ24257)  += bq24257_charger.o
> >  obj-$(CONFIG_CHARGER_BQ24735)  += bq24735-charger.o
> >  obj-$(CONFIG_CHARGER_BQ25890)  += bq25890_charger.o

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Fabio Estevam]

On Sun, Sep 6, 2015 at 2:23 PM, Ramakrishna Pallala
<ramakrishna.pallala-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> +       chip->psy_usb = power_supply_register(&client->dev,
> +                               &bq24261_charger_desc, &charger_cfg);
> +       if (IS_ERR(chip->psy_usb)) {
> +               dev_err(&client->dev,
> +                       "power supply registration failed(%d)\n", ret);
> +               return ret;

You should not print and return ret here.

You should print and return IS_ERR(chip->psy_usb) instead.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Krzysztof Kozlowski]

On 09.09.2015 11:26, Andreas Dannenberg wrote:
> On Mon, Sep 07, 2015 at 12:57:56PM +0900, Krzysztof Kozlowski wrote:
>> 2015-09-07 2:23 GMT+09:00 Ramakrishna Pallala <ramakrishna.pallala@intel.com>:
>>>
>>> Add new charger driver support for BQ24261 charger IC.
>>>
>>> BQ24261 charger driver relies on extcon notifications to get the
>>> charger cable type and based on that it will set the charging parameters.
>>>
>>> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>>> Signed-off-by: Jennt TC <jenny.tc@intel.com>
>>> ---
>>>  .../devicetree/bindings/power/bq24261.txt          |   37 +
>>>  drivers/power/Kconfig                              |    6 +
>>>  drivers/power/Makefile                             |    1 +
>>>  drivers/power/bq24261_charger.c                    | 1208 ++++++++++++++++++++
>>>  include/linux/power/bq24261_charger.h              |   27 +
>>>  5 files changed, 1279 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/power/bq24261.txt
>>>  create mode 100644 drivers/power/bq24261_charger.c
>>>  create mode 100644 include/linux/power/bq24261_charger.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/bq24261.txt b/Documentation/devicetree/bindings/power/bq24261.txt
>>> new file mode 100644
>>> index 0000000..25fc5c4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/bq24261.txt
>>> @@ -0,0 +1,37 @@
>>> +Binding for TI bq24261 Li-Ion Charger
>>
>> Please split the bindings into separate patch (the first patch in patchset).
>>
>>> +
>>> +Required properties:
>>> +- compatible: Should contain one of the following:
>>> +    * "ti,bq24261"
>>> +- reg: integer, i2c address of the device.
>>> +- ti,charge-current: integer, default charging current (in mA);
>>> +- ti,charge-voltage: integer, default charging voltage (in mV);
>>> +- ti,termination-current: integer, charge will be terminated when current in
>>> +    constant-voltage phase drops below this value (in mA);
>>> +- ti,max-charge-current: integer, maximum charging current (in mA);
>>> +- ti,max-charge-voltage: integer, maximum charging voltage (in mV);
>>> +- ti,min-charge-temperature: integer, minimum charging temperature (in DegC);
>>> +- ti,max-charge-temperature: integer, maximum charging temperature (in DegC).
>>
>> Before accepting "[PATCH 13/13] dt: power: bq24257-charger: Cover
>> additional devices"
>> http://www.spinics.net/lists/devicetree/msg92134.html
>>
>> could you and Andreas figure out common bindings? Look at this:
>>
>> +- ti,charge-current: integer, maximum charging current in uA.
>> +- ti,charge-current: integer, default charging current (in mA);
>>
>> Different meaning and different units. This is madness! :)
>>
>> In the same time you are adding TI-common bindings (not device
>> specific, there is no prefix) so I would expect exactly the same
>> bindings if it is possible.
> 
> Krzysztof, good observation! In bq2425x_charger.c (formerly known as
> bq24257_charger.c :) that I worked on the unit used was uA. At that time
> I did a quick check and there didn't seem to be a clear standard whether
> to use the "micro" or "milli" units - different drivers use different
> units. However there seems to be a tendency for the TI drivers to prefer
> "milli" (bq2415x_charger.c, bq24735-charger.c)
> 
> Personally I think "milli" units are more appropriate for chargers since
> they provide sufficient granularity and the numbers don't become too big
> (try typing a voltage in the Volt-range in uV, it's very easy to get the
> number of 0s wrong). However since the driver was already there I left
> that aspect alone to preserve compatibility.

I am fine with both units but milli indeed seems easier to judge by fast
looking and less error-prone. Whatever you choose - choose the same one. :)


>>> +
>>> +Optional properties:
>>> +- ti,thermal-sensing: boolean, if present thermal regulation will be enabled;
>>
>> What is the requirement for thermal-sensing? Can it be enabled always?
>> If yes, then this is not really a hardware property.
>>
>>> +- ti,enable-user-write: boolean, if present driver will allow the user space
>>> +    to control the charging current and voltage through sysfs;
>>
>> This is not DT property. It does not describe hardware.
> 
> I take responsibility for this one :) In an earlier thread we were
> discussing that it will be better to prevent sysfs write access to
> "dangerous" charger parameters (charge voltage, current, ...) but I
> argued that such access might still be desirable during development and
> debug, and that a DT property could be used to allow such write access
> (with the default being the charger properties being made read-only) -
> this way giving the best of both worlds. However then when I updated the
> bq24157_charger.c driver I ended up not really needing and implementing
> this feature.
> 
> Out of curiosity, if it against best-practice to expose non hardware
> properties, how would one best address the above use case? Compile time
> switch? A sysfs property that allows write-enabling the other sysfs
> properties (doesn't sound right). Or...?

Device Tree describes only the hardware configuration. This means that
it does not fully replaces platform data. Definitely DT should not be
used to configure debug options.

AFAIU, you have some board (always the same type of board) and:
1. during development you want to enable certain sysfs dangerous
operation/feature,
2. for production use you want to disable this feature?

This means that the "feature" would be present in mainline kernel always
so it is not possible to disallow the use of it. For example if this was
implemented in DT, the user could replace a DT with different one.

Assuming that this feature should be in mainline kernel at all, then
probably compile time option is the best way. Some subsystems don't
offer dangerous operations (e.g. setting voltage for regulators or force
suspending devices) but they provide certain development drivers which
expose such interfaces. Such cases should be rather done per subsystem,
not per device.

Best regards,
Krzysztof

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Andreas Dannenberg]

On Wed, Sep 09, 2015 at 01:17:11PM +0900, Krzysztof Kozlowski wrote:
> On 09.09.2015 11:26, Andreas Dannenberg wrote:
> > Krzysztof, good observation! In bq2425x_charger.c (formerly known as
> > bq24257_charger.c :) that I worked on the unit used was uA. At that time
> > I did a quick check and there didn't seem to be a clear standard whether
> > to use the "micro" or "milli" units - different drivers use different
> > units. However there seems to be a tendency for the TI drivers to prefer
> > "milli" (bq2415x_charger.c, bq24735-charger.c)
> > 
> > Personally I think "milli" units are more appropriate for chargers since
> > they provide sufficient granularity and the numbers don't become too big
> > (try typing a voltage in the Volt-range in uV, it's very easy to get the
> > number of 0s wrong). However since the driver was already there I left
> > that aspect alone to preserve compatibility.
> 
> I am fine with both units but milli indeed seems easier to judge by fast
> looking and less error-prone. Whatever you choose - choose the same one. :)

Ok sounds good. If so, I could go ahead and change the units in the
bq2425x_charger.c over to mA and mV? It would be a bit labor some and I
also want to see what Laurentiu thinks but this way we could have most
of those TI charger drivers use the same units (the new bq24261 driver
Ram posted also uses mA/mV). Except bq25890_charger.c.... that would
still use uA/uV....

Laurentiu -- what made you chose the "micro" units for bq24257_charger.c
and bq25890_charger.c?

Thanks,

--
Andreas Dannenberg
Texas Instruments Inc

RE: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi, 

> From: k.kozlowski.k@gmail.com [mailto:k.kozlowski.k@gmail.com] On Behalf
> Of Krzysztof Kozlowski
> Sent: Monday, September 7, 2015 9:28 AM
> To: Pallala, Ramakrishna
> Cc: linux-kernel@vger.kernel.org; linux-pm@vger.kernel.org;
> devicetree@vger.kernel.org; Sebastian Reichel; Tc, Jenny; Andreas Dannenberg
> Subject: Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261
> charger
> 
> 2015-09-07 2:23 GMT+09:00 Ramakrishna Pallala
> <ramakrishna.pallala@intel.com>:
> >
> > Add new charger driver support for BQ24261 charger IC.
> >
> > BQ24261 charger driver relies on extcon notifications to get the
> > charger cable type and based on that it will set the charging parameters.
> >
> > Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> > Signed-off-by: Jennt TC <jenny.tc@intel.com>
> > ---
> >  .../devicetree/bindings/power/bq24261.txt          |   37 +
> >  drivers/power/Kconfig                              |    6 +
> >  drivers/power/Makefile                             |    1 +
> >  drivers/power/bq24261_charger.c                    | 1208 ++++++++++++++++++++
> >  include/linux/power/bq24261_charger.h              |   27 +
> >  5 files changed, 1279 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/power/bq24261.txt
> >  create mode 100644 drivers/power/bq24261_charger.c  create mode
> > 100644 include/linux/power/bq24261_charger.h
> >
> > diff --git a/Documentation/devicetree/bindings/power/bq24261.txt
> > b/Documentation/devicetree/bindings/power/bq24261.txt
> > new file mode 100644
> > index 0000000..25fc5c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/bq24261.txt
> > @@ -0,0 +1,37 @@
> > +Binding for TI bq24261 Li-Ion Charger
> 
> Please split the bindings into separate patch (the first patch in patchset).
Ok.


> > +
> > +Required properties:
> > +- compatible: Should contain one of the following:
> > +    * "ti,bq24261"
> > +- reg: integer, i2c address of the device.
> > +- ti,charge-current: integer, default charging current (in mA);
> > +- ti,charge-voltage: integer, default charging voltage (in mV);
> > +- ti,termination-current: integer, charge will be terminated when current in
> > +    constant-voltage phase drops below this value (in mA);
> > +- ti,max-charge-current: integer, maximum charging current (in mA);
> > +- ti,max-charge-voltage: integer, maximum charging voltage (in mV);
> > +- ti,min-charge-temperature: integer, minimum charging temperature
> > +(in DegC);
> > +- ti,max-charge-temperature: integer, maximum charging temperature (in
> DegC).
> 
> Before accepting "[PATCH 13/13] dt: power: bq24257-charger: Cover additional
> devices"
> http://www.spinics.net/lists/devicetree/msg92134.html
> 
> could you and Andreas figure out common bindings? Look at this:
> 
> +- ti,charge-current: integer, maximum charging current in uA.
> +- ti,charge-current: integer, default charging current (in mA);
> 
> Different meaning and different units. This is madness! :)
This is being closed by Andreas and we would probably go with mA/mV

> In the same time you are adding TI-common bindings (not device specific, there
> is no prefix) so I would expect exactly the same bindings if it is possible.
Ok...i will check with other charger driver DT settings and fix it.

> > +Optional properties:
> > +- ti,thermal-sensing: boolean, if present thermal regulation will be
> > +enabled;
> 
> What is the requirement for thermal-sensing? Can it be enabled always?
> If yes, then this is not really a hardware property.
TI BQ24261 has provision to add Battery Pack thermistor but it has no ADC read it.
So a HW designer would or may not add the thermistor to charger and instead he can
connect to the Fuel Gauge.

> > +- ti,enable-user-write: boolean, if present driver will allow the user space
> > +    to control the charging current and voltage through sysfs;
> 
> This is not DT property. It does not describe hardware.
We needed a mechanism to enable the sysfs writes on certain properties.
If DT is not the place where should it go?

Thanks,
Ram

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Andreas Dannenberg]

Hi Ram, thanks for submitting this, please see some feedback inlined...

On Sun, Sep 06, 2015 at 10:53:07PM +0530, Ramakrishna Pallala wrote:
> Add new charger driver support for BQ24261 charger IC.
> 
> BQ24261 charger driver relies on extcon notifications to get the
> charger cable type and based on that it will set the charging parameters.
> 
> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> Signed-off-by: Jennt TC <jenny.tc@intel.com>
> ---
>  .../devicetree/bindings/power/bq24261.txt          |   37 +
>  drivers/power/Kconfig                              |    6 +
>  drivers/power/Makefile                             |    1 +
>  drivers/power/bq24261_charger.c                    | 1208 ++++++++++++++++++++
>  include/linux/power/bq24261_charger.h              |   27 +
>  5 files changed, 1279 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/bq24261.txt
>  create mode 100644 drivers/power/bq24261_charger.c
>  create mode 100644 include/linux/power/bq24261_charger.h
> 
> diff --git a/Documentation/devicetree/bindings/power/bq24261.txt b/Documentation/devicetree/bindings/power/bq24261.txt
> new file mode 100644
> index 0000000..25fc5c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/bq24261.txt
> @@ -0,0 +1,37 @@
> +Binding for TI bq24261 Li-Ion Charger
> +
> +Required properties:
> +- compatible: Should contain one of the following:
> +    * "ti,bq24261"
> +- reg: integer, i2c address of the device.
> +- ti,charge-current: integer, default charging current (in mA);
> +- ti,charge-voltage: integer, default charging voltage (in mV);

If you look at other chargers (bq2415x, bq24257, ...) this property
should probably be called ti,battery-regulation-voltage.

> +- ti,termination-current: integer, charge will be terminated when current in
> +    constant-voltage phase drops below this value (in mA);

> +- ti,max-charge-current: integer, maximum charging current (in mA);
> +- ti,max-charge-voltage: integer, maximum charging voltage (in mV);

What's the background of having these two properties? They don't impact
any device HW settings, but rather look like some safeguard if somebody
manipulates the sysfs to not exceed certain boundaries.

> +- ti,min-charge-temperature: integer, minimum charging temperature (in DegC);
> +- ti,max-charge-temperature: integer, maximum charging temperature (in DegC).

All this does is passing these values down to the sysfs and exposing
them through POWER_SUPPLY_PROP_TEMP_MAX/POWER_SUPPLY_PROP_TEMP_MIN. What
value does this provide?

> +
> +Optional properties:
> +- ti,thermal-sensing: boolean, if present thermal regulation will be enabled;

I received feedback for one of my recent patches that it's better to use an
integer property as it can be overridden and with that is more
flexible.

> +- ti,enable-user-write: boolean, if present driver will allow the user space
> +    to control the charging current and voltage through sysfs;

This idea came from me :) but we should probably get rid of this
capability. And the writability of charge current/voltage properties
through sysfs altogether. My use case for this was to enable better
testing but there is other ways this can be accomplished. However since
some other drivers expose such capability I wonder what other
reasons/use cases there might be (besides helping development and
testing)?

> +
> +Example:
> +
> +bq25890 {
> +        compatible = "ti,bq24261
> +        reg = <0x6b>;
> +
> +        ti,charge-current = <1000>;
> +        ti,charge-voltage = <4200>;
> +        ti,termination-current = <128>;
> +        ti,max-charge-current = <3000>;
> +        ti,max-charge-voltage = <4350>;
> +        ti,min-charge-temperature = <0>;
> +        ti,max-charge-temperature = <60>;
> +
> +        ti,thermal-sensing;
> +        ti,enable-user-write;
> +};
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index f8758d6..cd47d0d 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -396,6 +396,12 @@ config CHARGER_BQ24190
>  	help
>  	  Say Y to enable support for the TI BQ24190 battery charger.
>  
> +config CHARGER_BQ24261
> +	tristate "TI BQ24261 charger driver"
> +	depends on I2C && EXTCON
> +	help
> +	  Say Y here to enable support for TI BQ24261 battery charger.
> +
>  config CHARGER_BQ24257
>  	tristate "TI BQ24257 battery charger driver"
>  	depends on I2C
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index 5752ce8..bec8409 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -59,6 +59,7 @@ obj-$(CONFIG_CHARGER_MAX8997)	+= max8997_charger.o
>  obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
>  obj-$(CONFIG_CHARGER_BQ2415X)	+= bq2415x_charger.o
>  obj-$(CONFIG_CHARGER_BQ24190)	+= bq24190_charger.o
> +obj-$(CONFIG_CHARGER_BQ24261)	+= bq24261_charger.o
>  obj-$(CONFIG_CHARGER_BQ24257)	+= bq24257_charger.o
>  obj-$(CONFIG_CHARGER_BQ24735)	+= bq24735-charger.o
>  obj-$(CONFIG_CHARGER_BQ25890)	+= bq25890_charger.o
> diff --git a/drivers/power/bq24261_charger.c b/drivers/power/bq24261_charger.c
> new file mode 100644
> index 0000000..b01114e
> --- /dev/null
> +++ b/drivers/power/bq24261_charger.c
> @@ -0,0 +1,1208 @@
> +/*
> + * bq24261_charger.c - BQ24261 Charger driver
> + *
> + * Copyright (C) 2014 Intel Corporation
> + * Author:	Jenny TC <jenny.tc@intel.com>
> + *		Ramakrishna Pallala <ramakrishna.pallala@intel.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.
> + *
> + * 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.
> + */
> +
> +#include <linux/version.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/power_supply.h>
> +#include <linux/extcon.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/io.h>
> +#include <linux/sched.h>
> +#include <linux/delay.h>
> +#include <linux/acpi.h>
> +#include <linux/power/bq24261_charger.h>
> +
> +#define DEV_NAME "bq24261_charger"

Can we use a dash/hyphen rather than an underscore? This would align
better with bq2415x and bq24257 (=bq2425x).

> +
> +#define EXCEPTION_MONITOR_DELAY		(60 * HZ)
> +#define WDT_RESET_DELAY			(15 * HZ)
> +
> +/* BQ24261 registers */
> +#define BQ24261_STAT_CTRL0_ADDR		0x00
> +#define BQ24261_CTRL_ADDR		0x01
> +#define BQ24261_BATT_VOL_CTRL_ADDR	0x02
> +#define BQ24261_VENDOR_REV_ADDR		0x03
> +#define BQ24261_TERM_FCC_ADDR		0x04
> +#define BQ24261_VINDPM_STAT_ADDR	0x05
> +#define BQ24261_ST_NTC_MON_ADDR		0x06
> +
> +#define BQ24261_RESET_MASK		(0x01 << 7)

What about using the BIT() macro for all of these?

> +#define BQ24261_RESET_ENABLE		(0x01 << 7)
> +
> +#define BQ24261_FAULT_MASK		0x07
> +#define BQ24261_STAT_MASK		(0x03 << 4)

What about the GENMASK() macro for all bit fields?

> +#define BQ24261_BOOST_MASK		(0x01 << 6)
> +#define BQ24261_TMR_RST_MASK		(0x01 << 7)
> +#define BQ24261_TMR_RST			(0x01 << 7)
> +
> +#define BQ24261_ENABLE_BOOST		(0x01 << 6)
> +
> +#define BQ24261_VOVP			0x01
> +#define BQ24261_LOW_SUPPLY		0x02
> +#define BQ24261_THERMAL_SHUTDOWN	0x03
> +#define BQ24261_BATT_TEMP_FAULT		0x04
> +#define BQ24261_TIMER_FAULT		0x05
> +#define BQ24261_BATT_OVP		0x06
> +#define BQ24261_NO_BATTERY		0x07
> +#define BQ24261_STAT_READY		0x00
> +
> +#define BQ24261_STAT_CHRG_PRGRSS	(0x01 << 4)
> +#define BQ24261_STAT_CHRG_DONE		(0x02 << 4)
> +#define BQ24261_STAT_FAULT		(0x03 << 4)
> +
> +#define BQ24261_CE_MASK			(0x01 << 1)
> +#define BQ24261_CE_DISABLE		(0x01 << 1)
> +
> +#define BQ24261_HiZ_MASK			(0x01)
> +#define BQ24261_HiZ_ENABLE		(0x01)
> +
> +#define BQ24261_ICHRG_MASK		(0x1F << 3)
> +
> +#define BQ24261_ITERM_MASK		(0x03)
> +#define BQ24261_MIN_ITERM		50 /* 50 mA */
> +#define BQ24261_MAX_ITERM		300 /* 300 mA */
> +
> +#define BQ24261_VBREG_MASK		(0x3F << 2)
> +#define BQ24261_VBREG_MIN_CV		3500
> +#define BQ24261_VBREG_MAX_CV		4440
> +#define BQ24261_VBREG_CV_DIV		20
> +#define BQ24261_VBREG_CV_BIT_POS	2
> +
> +#define BQ24261_INLMT_MASK		(0x07 << 4)
> +#define BQ24261_INLMT_100		0x00
> +#define BQ24261_INLMT_150		(0x01 << 4)
> +#define BQ24261_INLMT_500		(0x02 << 4)
> +#define BQ24261_INLMT_900		(0x03 << 4)
> +#define BQ24261_INLMT_1500		(0x04 << 4)
> +#define BQ24261_INLMT_2000		(0x05 << 4)
> +#define BQ24261_INLMT_2500		(0x06 << 4)
> +
> +#define BQ24261_TE_MASK			(0x01 << 2)
> +#define BQ24261_TE_ENABLE		(0x01 << 2)
> +#define BQ24261_STAT_ENABLE_MASK	(0x01 << 3)
> +#define BQ24261_STAT_ENABLE		(0x01 << 3)
> +
> +#define BQ24261_VENDOR_MASK		(0x07 << 5)
> +#define BQ24261_PART_MASK		(0x03 << 3)
> +#define BQ24261_REV_MASK		(0x07)
> +#define VENDOR_BQ2426X			(0x02 << 5)
> +#define REV_BQ24261			(0x06)
> +
> +#define BQ24261_TS_MASK			(0x01 << 3)
> +#define BQ24261_TS_ENABLED		(0x01 << 3)
> +#define BQ24261_BOOST_ILIM_MASK		(0x01 << 4)
> +#define BQ24261_BOOST_ILIM_500ma	(0x0)
> +#define BQ24261_BOOST_ILIM_1A		(0x01 << 4)
> +#define BQ24261_VINDPM_OFF_MASK		(0x01 << 0)
> +#define BQ24261_VINDPM_OFF_5V		(0x0)
> +#define BQ24261_VINDPM_OFF_12V		(0x01 << 0)
> +
> +#define BQ24261_SAFETY_TIMER_MASK	(0x03 << 5)
> +#define BQ24261_SAFETY_TIMER_40MIN	0x00
> +#define BQ24261_SAFETY_TIMER_6HR	(0x01 << 5)
> +#define BQ24261_SAFETY_TIMER_9HR	(0x02 << 5)
> +#define BQ24261_SAFETY_TIMER_DISABLED	(0x03 << 5)
> +
> +/* 1% above voltage max design to report over voltage */
> +#define BQ24261_OVP_MULTIPLIER			1010
> +#define BQ24261_OVP_RECOVER_MULTIPLIER		990
> +#define BQ24261_DEF_BAT_VOLT_MAX_DESIGN		4200000
> +
> +/* Settings for Voltage / DPPM Register (05) */
> +#define BQ24261_VBATT_LEVEL1		3700000
> +#define BQ24261_VBATT_LEVEL2		3960000
> +#define BQ24261_VINDPM_MASK		(0x07)
> +#define BQ24261_VINDPM_320MV		(0x01 << 2)
> +#define BQ24261_VINDPM_160MV		(0x01 << 1)
> +#define BQ24261_VINDPM_80MV		(0x01 << 0)
> +#define BQ24261_CD_STATUS_MASK		(0x01 << 3)
> +#define BQ24261_DPM_EN_MASK		(0x01 << 4)
> +#define BQ24261_DPM_EN_FORCE		(0x01 << 4)
> +#define BQ24261_LOW_CHG_MASK		(0x01 << 5)
> +#define BQ24261_LOW_CHG_EN		(0x01 << 5)
> +#define BQ24261_LOW_CHG_DIS		(~BQ24261_LOW_CHG_EN)
> +#define BQ24261_DPM_STAT_MASK		(0x01 << 6)
> +#define BQ24261_MINSYS_STAT_MASK	(0x01 << 7)
> +
> +#define BQ24261_MIN_CC			500	/* 500mA */
> +#define BQ24261_MAX_CC			3000	/* 3A */
> +#define BQ24261_DEF_CC			1300	/* 1300mA */
> +#define BQ24261_MAX_CV			4350	/*4350mV */
> +#define BQ24261_DEF_CV			4350	/* 4350mV */
> +#define BQ24261_DEF_ITERM		128	/* 128mA */
> +#define BQ24261_MIN_TEMP		0	/* 0 degC */
> +#define BQ24261_MAX_TEMP		60	/* 60 DegC */
> +
> +#define ILIM_100MA			100	/* 100mA */
> +#define ILIM_500MA			500	/* 500mA */
> +#define ILIM_900MA			900	/* 900mA */
> +#define ILIM_1500MA			1500	/* 1500mA */
> +#define ILIM_2000MA			2000	/* 2000mA */
> +#define ILIM_2500MA			2500	/* 2500mA */
> +#define ILIM_3000MA			3000	/* 3000mA */
> +
> +u16 bq24261_inlmt[][2] = {
> +	{100, BQ24261_INLMT_100},
> +	{150, BQ24261_INLMT_150},
> +	{500, BQ24261_INLMT_500},
> +	{900, BQ24261_INLMT_900},
> +	{1500, BQ24261_INLMT_1500},
> +	{2000, BQ24261_INLMT_2000},
> +	{2500, BQ24261_INLMT_2500},
> +};
> +
> +
> +enum bq24261_status {
> +	BQ24261_CHRGR_STAT_UNKNOWN,
> +	BQ24261_CHRGR_STAT_READY,
> +	BQ24261_CHRGR_STAT_CHARGING,
> +	BQ24261_CHRGR_STAT_FULL,
> +	BQ24261_CHRGR_STAT_FAULT,
> +};
> +
> +enum bq2426x_model {
> +	BQ2426X = 0,
> +	BQ24260,

The BQ24260 definition doesn't seem to get used in this driver.

> +	BQ24261,
> +};
> +
> +struct bq24261_charger {
> +	struct i2c_client *client;
> +	struct bq24261_platform_data *pdata;
> +	struct power_supply *psy_usb;
> +	struct delayed_work fault_mon_work;
> +	struct mutex lock;
> +	enum bq2426x_model model;
> +	struct delayed_work wdt_work;
> +	struct work_struct irq_work;
> +	struct list_head irq_queue;
> +
> +	/* extcon charger cables */
> +	struct {
> +		struct work_struct work;
> +		struct notifier_block nb;
> +		struct extcon_specific_cable_nb sdp;
> +		struct extcon_specific_cable_nb cdp;
> +		struct extcon_specific_cable_nb dcp;
> +		struct extcon_specific_cable_nb otg;
> +		enum power_supply_type chg_type;
> +		bool boost;
> +		bool connected;
> +	} cable;
> +
> +	bool online;
> +	bool present;
> +	int chg_health;
> +	enum bq24261_status chg_status;
> +	int cc;
> +	int cv;
> +	int inlmt;
> +	int max_cc;
> +	int max_cv;
> +	int iterm;
> +	int max_temp;
> +	int min_temp;
> +	bool is_charging_enabled;
> +};
> +
> +static inline int bq24261_read_reg(struct i2c_client *client, u8 reg)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, reg);
> +	if (ret < 0)
> +		dev_err(&client->dev,
> +			"error(%d) in reading reg %d\n", ret, reg);
> +	return ret;
> +}
> +
> +static inline int bq24261_write_reg(struct i2c_client *client, u8 reg, u8 data)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, reg, data);
> +	if (ret < 0)
> +		dev_err(&client->dev,
> +			"error(%d) in writing %d to reg %d\n", ret, data, reg);
> +	return ret;
> +}
> +
> +static inline int bq24261_update_reg(struct i2c_client *client, u8 reg,
> +					  u8 mask, u8 val)
> +{
> +	int ret;
> +
> +	ret = bq24261_read_reg(client, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = (ret & ~mask) | (mask & val);
> +	return bq24261_write_reg(client, reg, ret);
> +}
> +
> +static void lookup_regval(u16 tbl[][2], size_t size, u16 in_val, u8 *out_val)
> +{
> +	int i;
> +
> +	for (i = 1; i < size; ++i) {
> +		if (in_val < tbl[i][0])
> +			break;
> +	}
> +
> +	*out_val = (u8) tbl[i - 1][1];
> +}
> +
> +void bq24261_cc_to_reg(int cc, u8 *reg_val)
> +{
> +	/* Ichrg bits are B3-B7
> +	 * Icharge = 500mA + IchrgCode * 100mA
> +	 */
> +	cc = clamp_t(int, cc, BQ24261_MIN_CC, BQ24261_MAX_CC);
> +	cc = cc - BQ24261_MIN_CC;
> +	*reg_val = (cc / 100) << 3;
> +}
> +
> +void bq24261_cv_to_reg(int cv, u8 *reg_val)
> +{
> +	int val;
> +
> +	val = clamp_t(int, cv, BQ24261_VBREG_MIN_CV, BQ24261_VBREG_MAX_CV);
> +	*reg_val = (((val - BQ24261_VBREG_MIN_CV) / BQ24261_VBREG_CV_DIV)
> +			<< BQ24261_VBREG_CV_BIT_POS);
> +}
> +
> +void bq24261_inlmt_to_reg(int inlmt, u8 *regval)
> +{
> +	return lookup_regval(bq24261_inlmt, ARRAY_SIZE(bq24261_inlmt),
> +			     inlmt, regval);
> +}
> +
> +static inline void bq24261_iterm_to_reg(int iterm, u8 *regval)
> +{
> +	/* Iterm bits are B0-B2
> +	 * Icharge = 50mA + ItermCode * 50mA
> +	 */

Comment style.

> +	iterm = clamp_t(int, iterm, BQ24261_MIN_ITERM,  BQ24261_MAX_ITERM);
> +	iterm = iterm - BQ24261_MIN_ITERM;
> +	*regval =  iterm / 50;
> +}
> +
> +static inline int bq24261_init_timers(struct bq24261_charger *chip)
> +{
> +	u8 reg_val;
> +	int ret;
> +
> +	reg_val = BQ24261_SAFETY_TIMER_9HR;
> +
> +	if (chip->pdata->thermal_sensing)
> +		reg_val |= BQ24261_TS_ENABLED;
> +
> +	ret = bq24261_update_reg(chip->client, BQ24261_ST_NTC_MON_ADDR,
> +			BQ24261_TS_MASK | BQ24261_SAFETY_TIMER_MASK |
> +			BQ24261_BOOST_ILIM_MASK, reg_val);
> +
> +	return ret;
> +}
> +
> +static inline int bq24261_reset_wdt_timer(struct bq24261_charger *chip)
> +{
> +	u8 mask = BQ24261_TMR_RST_MASK, val = BQ24261_TMR_RST;
> +
> +	if (chip->cable.boost) {
> +		mask |= BQ24261_BOOST_MASK;
> +		val |= BQ24261_ENABLE_BOOST;
> +	}
> +
> +	return bq24261_update_reg(chip->client, BQ24261_STAT_CTRL0_ADDR,
> +							mask, val);
> +}
> +
> +static inline int bq24261_set_cc(struct bq24261_charger *chip, int cc_mA)
> +{
> +	u8 reg_val;
> +	int ret;
> +
> +	dev_dbg(&chip->client->dev, "%s=%d\n", __func__, cc_mA);
> +
> +	if (cc_mA && (cc_mA < BQ24261_MIN_CC)) {
> +		dev_dbg(&chip->client->dev, "Set LOW_CHG bit\n");
> +		reg_val = BQ24261_LOW_CHG_EN;
> +		ret = bq24261_update_reg(chip->client,
> +				BQ24261_VINDPM_STAT_ADDR,
> +				BQ24261_LOW_CHG_MASK, reg_val);
> +		return ret;
> +	}
> +
> +	reg_val = BQ24261_LOW_CHG_DIS;
> +	ret = bq24261_update_reg(chip->client, BQ24261_VINDPM_STAT_ADDR,
> +					BQ24261_LOW_CHG_MASK, reg_val);
> +
> +	bq24261_cc_to_reg(cc_mA, &reg_val);
> +
> +	return bq24261_update_reg(chip->client, BQ24261_TERM_FCC_ADDR,
> +			BQ24261_ICHRG_MASK, reg_val);
> +}
> +
> +static inline int bq24261_set_cv(struct bq24261_charger *chip, int cv_mV)
> +{
> +	u8 reg_val;
> +
> +	dev_dbg(&chip->client->dev, "%s=%d\n", __func__, cv_mV);
> +
> +	bq24261_cv_to_reg(cv_mV, &reg_val);
> +
> +	return bq24261_update_reg(chip->client, BQ24261_BATT_VOL_CTRL_ADDR,
> +				       BQ24261_VBREG_MASK, reg_val);
> +}
> +
> +static inline int bq24261_set_inlmt(struct bq24261_charger *chip, int inlmt)
> +{
> +	u8 reg_val;
> +
> +	dev_dbg(&chip->client->dev, "%s=%d\n", __func__, inlmt);
> +
> +	bq24261_inlmt_to_reg(inlmt, &reg_val);
> +
> +	/*
> +	 * Don't enable reset bit. Setting this
> +	 * bit will reset all the registers/

Extra character at the line end.

> +	 */
> +	reg_val &= ~BQ24261_RESET_MASK;
> +
> +	return bq24261_update_reg(chip->client, BQ24261_CTRL_ADDR,
> +		       BQ24261_RESET_MASK|BQ24261_INLMT_MASK, reg_val);
> +
> +}
> +
> +static inline int bq24261_set_iterm(struct bq24261_charger *chip, int iterm)
> +{
> +	u8 reg_val;
> +
> +	bq24261_iterm_to_reg(iterm, &reg_val);
> +
> +	return bq24261_update_reg(chip->client, BQ24261_TERM_FCC_ADDR,
> +				       BQ24261_ITERM_MASK, reg_val);
> +}
> +
> +static inline int bq24261_enable_charging(struct bq24261_charger *chip,
> +								bool enable)
> +{
> +	int ret;
> +	u8 reg_val;
> +
> +	if (enable) {
> +		reg_val = (~BQ24261_CE_DISABLE & BQ24261_CE_MASK);
> +		reg_val |= BQ24261_TE_ENABLE;
> +	} else {
> +		reg_val = BQ24261_CE_DISABLE;
> +	}
> +
> +	reg_val |=  BQ24261_STAT_ENABLE;
> +
> +	/*
> +	 * Don't enable reset bit. Setting this
> +	 * bit will reset all the registers/

Extra character.

> +	 */
> +	reg_val &= ~BQ24261_RESET_MASK;
> +
> +	ret = bq24261_update_reg(chip->client, BQ24261_CTRL_ADDR,
> +		       BQ24261_STAT_ENABLE_MASK|BQ24261_RESET_MASK|
> +				BQ24261_CE_MASK|BQ24261_TE_MASK,
> +					reg_val);

Should use spaces around operators.

> +	if (ret || !enable)
> +		return ret;
> +
> +	/* Set termination current */
> +	ret = bq24261_set_iterm(chip, chip->iterm);
> +	if (ret < 0)
> +		dev_err(&chip->client->dev, "failed to set iTerm(%d)\n", ret);
> +
> +	/* Start WDT and Safety timers */
> +	ret = bq24261_init_timers(chip);
> +	if (ret)
> +		dev_err(&chip->client->dev, "failed to set timers(%d)\n", ret);
> +
> +	return ret;
> +}
> +
> +static inline int bq24261_enable_charger(struct bq24261_charger *chip,
> +								int enable)
> +{
> +	u8 reg_val;
> +	int ret;
> +
> +	reg_val = enable ? (~BQ24261_HiZ_ENABLE & BQ24261_HiZ_MASK) :
> +			BQ24261_HiZ_ENABLE;
> +
> +	/*
> +	 * Don't enable reset bit. Setting this
> +	 * bit will reset all the registers/
> +	 */
> +	reg_val &= ~BQ24261_RESET_MASK;
> +
> +	ret = bq24261_update_reg(chip->client, BQ24261_CTRL_ADDR,
> +		       BQ24261_HiZ_MASK|BQ24261_RESET_MASK, reg_val);
> +	if (ret)
> +		return ret;
> +
> +	return bq24261_reset_wdt_timer(chip);
> +}
> +
> +static void bq24261_handle_health(struct bq24261_charger *chip, u8 stat_reg)
> +{
> +	struct i2c_client *client = chip->client;
> +	bool fault_worker = false;
> +
> +	switch (stat_reg & BQ24261_FAULT_MASK) {
> +	case BQ24261_VOVP:
> +		chip->chg_health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> +		fault_worker = true;
> +		dev_err(&client->dev, "Charger Over Voltage Fault\n");
> +		break;
> +	case BQ24261_LOW_SUPPLY:
> +		chip->chg_health = POWER_SUPPLY_HEALTH_DEAD;
> +		fault_worker = true;
> +		dev_err(&client->dev, "Charger Low Supply Fault\n");
> +		break;
> +	case BQ24261_THERMAL_SHUTDOWN:
> +		chip->chg_health = POWER_SUPPLY_HEALTH_OVERHEAT;
> +		dev_err(&client->dev, "Charger Thermal Fault\n");
> +		break;
> +	case BQ24261_BATT_TEMP_FAULT:
> +		dev_err(&client->dev, "Battery Temperature Fault\n");
> +		break;
> +	case BQ24261_TIMER_FAULT:
> +		chip->chg_health = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> +		dev_err(&client->dev, "Charger Timer Fault\n");
> +		break;
> +	case BQ24261_BATT_OVP:
> +		chip->chg_health = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> +		dev_err(&client->dev, "Battery Over Voltage Fault\n");
> +		break;
> +	case BQ24261_NO_BATTERY:
> +		dev_err(&client->dev, "No Battery Connected\n");
> +		break;
> +	default:
> +		chip->chg_health = POWER_SUPPLY_HEALTH_GOOD;
> +	}
> +
> +	if (fault_worker)
> +		schedule_delayed_work(&chip->fault_mon_work,
> +					EXCEPTION_MONITOR_DELAY);
> +}
> +
> +static void bq24261_handle_status(struct bq24261_charger *chip, u8 stat_reg)
> +{
> +	struct i2c_client *client = chip->client;
> +
> +	switch (stat_reg & BQ24261_STAT_MASK) {
> +	case BQ24261_STAT_READY:
> +		chip->chg_status = BQ24261_CHRGR_STAT_READY;
> +		dev_info(&client->dev, "Charger Status: Ready\n");
> +		break;
> +	case BQ24261_STAT_CHRG_PRGRSS:
> +		chip->chg_status = BQ24261_CHRGR_STAT_CHARGING;
> +		dev_info(&client->dev, "Charger Status: Charge Progress\n");
> +		break;
> +	case BQ24261_STAT_CHRG_DONE:
> +		chip->chg_status = BQ24261_CHRGR_STAT_FULL;
> +		dev_info(&client->dev, "Charger Status: Charge Done\n");
> +		break;
> +	case BQ24261_STAT_FAULT:
> +		chip->chg_status = BQ24261_CHRGR_STAT_FAULT;
> +		dev_warn(&client->dev, "Charger Status: Fault\n");
> +		break;
> +	default:
> +		dev_info(&client->dev, "Invalid\n");
> +	}
> +}
> +
> +static int bq24261_get_charger_health(struct bq24261_charger *chip)
> +{
> +	if (!chip->present)
> +		return POWER_SUPPLY_HEALTH_UNKNOWN;
> +
> +	return chip->chg_health;
> +}
> +
> +static int bq24261_get_charging_status(struct bq24261_charger *chip)
> +{
> +	int status;
> +
> +	if (!chip->present)
> +		return POWER_SUPPLY_STATUS_DISCHARGING;
> +
> +	switch (chip->chg_status) {
> +	case BQ24261_CHRGR_STAT_READY:
> +		status = POWER_SUPPLY_STATUS_DISCHARGING;
> +		break;
> +	case BQ24261_CHRGR_STAT_CHARGING:
> +		status = POWER_SUPPLY_STATUS_CHARGING;
> +		break;
> +	case BQ24261_CHRGR_STAT_FULL:
> +		status = POWER_SUPPLY_STATUS_FULL;
> +		break;
> +	case BQ24261_CHRGR_STAT_FAULT:
> +		status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		break;
> +	default:
> +		status = POWER_SUPPLY_STATUS_DISCHARGING;
> +	}
> +
> +	return status;
> +}
> +
> +static int bq24261_usb_get_property(struct power_supply *psy,
> +		enum power_supply_property psp, union power_supply_propval *val)
> +{
> +	struct bq24261_charger *chip = power_supply_get_drvdata(psy);
> +
> +	mutex_lock(&chip->lock);
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = chip->present;
> +		break;
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		val->intval = chip->online;
> +		break;
> +	case POWER_SUPPLY_PROP_HEALTH:
> +		val->intval = bq24261_get_charger_health(chip);
> +		break;
> +	case POWER_SUPPLY_PROP_STATUS:
> +		val->intval = bq24261_get_charging_status(chip);
> +		break;
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +		val->intval = chip->pdata->max_cc * 1000;
> +		break;
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> +		val->intval = chip->pdata->max_cv * 1000;
> +		break;
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> +		val->intval = chip->cc * 1000;
> +		break;
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> +		val->intval = chip->cv * 1000;
> +		break;
> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		val->intval = chip->inlmt * 1000;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
> +		val->intval = chip->iterm * 1000;
> +		break;
> +	case POWER_SUPPLY_PROP_TEMP_MAX:
> +		val->intval = chip->pdata->max_temp * 10;
> +		break;
> +	case POWER_SUPPLY_PROP_TEMP_MIN:
> +		val->intval = chip->pdata->min_temp * 10;
> +		break;
> +	default:
> +		mutex_unlock(&chip->lock);
> +		return -EINVAL;
> +	}
> +	mutex_unlock(&chip->lock);
> +
> +	return 0;
> +}
> +
> +static int bq24261_usb_set_property(struct power_supply *psy,
> +	enum power_supply_property psp, const union power_supply_propval *val)
> +{
> +	struct bq24261_charger *chip = power_supply_get_drvdata(psy);
> +	int intval, ret = 0;
> +
> +	mutex_lock(&chip->lock);
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> +		/* convert uA to mA */
> +		intval = val->intval / 1000;
> +		if (intval > chip->max_cc) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		ret = bq24261_set_cc(chip, intval);
> +		if (!ret)
> +			chip->cc = intval;
> +		break;
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> +		/* convert uA to mV */
> +		intval = val->intval / 1000;
> +		if (intval > chip->max_cv) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		ret = bq24261_set_cv(chip, intval);
> +		if (!ret)
> +			chip->cv = intval;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&chip->lock);
> +
> +	return ret;
> +}
> +
> +static int bq24261_property_is_writeable(struct power_supply *psy,
> +		enum power_supply_property psp)
> +{
> +	int ret;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> +		ret = 1;
> +		break;
> +	default:
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
> +static enum power_supply_property bq24261_usb_props[] = {
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_TYPE,
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
> +	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +	POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
> +	POWER_SUPPLY_PROP_TEMP_MAX,
> +	POWER_SUPPLY_PROP_TEMP_MIN,

What about adding support for POWER_SUPPLY_PROP_MODEL_NAME and
POWER_SUPPLY_PROP_MANUFACTURER?

> +};
> +
> +static char *bq24261_charger_supplied_to[] = {
> +	"main-battery",
> +};
> +
> +static struct power_supply_desc bq24261_charger_desc = {
> +	.name			= DEV_NAME,
> +	.type			= POWER_SUPPLY_TYPE_USB,
> +	.properties		= bq24261_usb_props,
> +	.num_properties		= ARRAY_SIZE(bq24261_usb_props),
> +	.get_property		= bq24261_usb_get_property,
> +};
> +
> +static void bq24261_wdt_reset_worker(struct work_struct *work)
> +{
> +
> +	struct bq24261_charger *chip = container_of(work,
> +			    struct bq24261_charger, wdt_work.work);
> +	int ret;
> +
> +	ret = bq24261_reset_wdt_timer(chip);
> +	if (ret)
> +		dev_err(&chip->client->dev, "WDT timer reset error(%d)\n", ret);
> +
> +	schedule_delayed_work(&chip->wdt_work, WDT_RESET_DELAY);
> +}
> +
> +static void bq24261_irq_worker(struct work_struct *work)
> +{
> +	struct bq24261_charger *chip =
> +	    container_of(work, struct bq24261_charger, irq_work);
> +	int ret;
> +
> +	/*
> +	 * Lock to ensure that interrupt register readings are done
> +	 * and processed sequentially. The interrupt Fault registers
> +	 * are read on clear and without sequential processing double
> +	 * fault interrupts or fault recovery cannot be handlled propely
> +	 */
> +
> +	mutex_lock(&chip->lock);
> +
> +	ret = bq24261_read_reg(chip->client, BQ24261_STAT_CTRL0_ADDR);
> +	if (ret < 0) {
> +		dev_err(&chip->client->dev,
> +			"Error (%d) in reading BQ24261_STAT_CTRL0_ADDR\n", ret);
> +		goto irq_out;
> +	}
> +
> +	if (!chip->cable.boost) {
> +		bq24261_handle_status(chip, ret);
> +		bq24261_handle_health(chip, ret);
> +		power_supply_changed(chip->psy_usb);
> +	}
> +
> +irq_out:
> +	mutex_unlock(&chip->lock);
> +}
> +
> +static irqreturn_t bq24261_thread_handler(int id, void *data)
> +{
> +	struct bq24261_charger *chip = (struct bq24261_charger *)data;
> +
> +	queue_work(system_highpri_wq, &chip->irq_work);
> +	return IRQ_HANDLED;
> +}
> +
> +static void bq24261_fault_mon_work(struct work_struct *work)
> +{
> +	struct bq24261_charger *chip = container_of(work,
> +			struct bq24261_charger, fault_mon_work.work);
> +	int ret;
> +
> +	if ((chip->chg_health == POWER_SUPPLY_HEALTH_OVERVOLTAGE) ||
> +		(chip->chg_health == POWER_SUPPLY_HEALTH_DEAD)) {
> +
> +		mutex_lock(&chip->lock);
> +		ret = bq24261_read_reg(chip->client, BQ24261_STAT_CTRL0_ADDR);
> +		if (ret < 0) {
> +			dev_err(&chip->client->dev,
> +				"Status register read failed(%d)\n", ret);
> +			goto fault_mon_out;
> +		}
> +
> +		if ((ret & BQ24261_STAT_MASK) == BQ24261_STAT_READY) {
> +			dev_info(&chip->client->dev,
> +					"Charger fault recovered\n");
> +			bq24261_handle_status(chip, ret);
> +			bq24261_handle_health(chip, ret);
> +			power_supply_changed(chip->psy_usb);
> +		}
> +fault_mon_out:
> +		mutex_unlock(&chip->lock);
> +	}
> +}
> +
> +static void bq24261_boost_control(struct bq24261_charger *chip, bool enable)
> +{
> +	int ret;
> +
> +	if (enable)
> +		ret = bq24261_write_reg(chip->client, BQ24261_STAT_CTRL0_ADDR,
> +				BQ24261_TMR_RST | BQ24261_ENABLE_BOOST);
> +	else
> +		ret = bq24261_write_reg(chip->client,
> +						BQ24261_STAT_CTRL0_ADDR, 0x0);
> +
> +	if (ret < 0)
> +		dev_err(&chip->client->dev,
> +			"stat cntl0 reg access error(%d)\n", ret);
> +}
> +
> +static void bq24261_extcon_event_work(struct work_struct *work)
> +{
> +	struct bq24261_charger *chip =
> +			container_of(work, struct bq24261_charger, cable.work);
> +	int ret, current_limit = 0;
> +	bool old_connected = chip->cable.connected;
> +
> +	/* Determine cable/charger type */
> +	if (extcon_get_cable_state(chip->cable.sdp.edev,
> +					"SLOW-CHARGER") > 0) {
> +		chip->cable.connected = true;
> +		current_limit = ILIM_500MA;
> +		chip->cable.chg_type = POWER_SUPPLY_TYPE_USB;
> +		dev_dbg(&chip->client->dev, "USB SDP charger is connected");
> +	} else if (extcon_get_cable_state(chip->cable.cdp.edev,
> +					"CHARGE-DOWNSTREAM") > 0) {
> +		chip->cable.connected = true;
> +		current_limit = ILIM_1500MA;
> +		chip->cable.chg_type = POWER_SUPPLY_TYPE_USB_CDP;
> +		dev_dbg(&chip->client->dev, "USB CDP charger is connected");
> +	} else if (extcon_get_cable_state(chip->cable.dcp.edev,
> +					"FAST-CHARGER") > 0) {
> +		chip->cable.connected = true;
> +		current_limit = ILIM_1500MA;
> +		chip->cable.chg_type = POWER_SUPPLY_TYPE_USB_DCP;
> +		dev_dbg(&chip->client->dev, "USB DCP charger is connected");
> +	} else if (extcon_get_cable_state(chip->cable.otg.edev,
> +					"USB-Host") > 0) {
> +		chip->cable.boost = true;
> +		chip->cable.connected = true;
> +		dev_dbg(&chip->client->dev, "USB-Host cable is connected");
> +	} else {
> +		if (old_connected)
> +			dev_dbg(&chip->client->dev, "USB Cable disconnected");
> +		chip->cable.connected = false;
> +		chip->cable.boost = false;
> +		chip->cable.chg_type = POWER_SUPPLY_TYPE_USB;
> +	}
> +
> +	/* Cable status changed */
> +	if (old_connected == chip->cable.connected)
> +		return;
> +
> +	mutex_lock(&chip->lock);
> +	if (chip->cable.connected && !chip->cable.boost) {
> +		chip->inlmt = current_limit;
> +		/* Set up charging */
> +		ret = bq24261_set_cc(chip, chip->cc);
> +		if (ret < 0)
> +			dev_err(&chip->client->dev, "set CC failed(%d)", ret);
> +		ret = bq24261_set_cv(chip, chip->cv);
> +		if (ret < 0)
> +			dev_err(&chip->client->dev, "set CV failed(%d)", ret);
> +		ret = bq24261_set_inlmt(chip, chip->inlmt);
> +		if (ret < 0)
> +			dev_err(&chip->client->dev, "set ILIM failed(%d)", ret);
> +		ret = bq24261_enable_charger(chip, true);
> +		if (ret < 0)
> +			dev_err(&chip->client->dev,
> +					"enable charger failed(%d)", ret);
> +		ret = bq24261_enable_charging(chip, true);
> +		if (ret < 0)
> +			dev_err(&chip->client->dev,
> +					"enable charging failed(%d)", ret);
> +
> +		chip->is_charging_enabled = true;
> +		chip->present = true;
> +		chip->online = true;
> +		schedule_delayed_work(&chip->wdt_work, 0);
> +	} else if (chip->cable.connected && chip->cable.boost) {
> +		/* Enable VBUS for Host Mode */
> +		bq24261_boost_control(chip, true);
> +		schedule_delayed_work(&chip->wdt_work, 0);
> +	} else {
> +		dev_info(&chip->client->dev, "Cable disconnect event\n");
> +		cancel_delayed_work_sync(&chip->wdt_work);
> +		cancel_delayed_work_sync(&chip->fault_mon_work);
> +		bq24261_boost_control(chip, false);
> +		ret = bq24261_enable_charging(chip, false);
> +		if (ret < 0)
> +			dev_err(&chip->client->dev,
> +					"charger disable failed(%d)", ret);
> +
> +		chip->is_charging_enabled = false;
> +		chip->present = false;
> +		chip->online = false;
> +		chip->inlmt = 0;
> +	}
> +	bq24261_charger_desc.type = chip->cable.chg_type;
> +	mutex_unlock(&chip->lock);
> +	power_supply_changed(chip->psy_usb);
> +}
> +
> +static int bq24261_handle_extcon_events(struct notifier_block *nb,
> +				   unsigned long event, void *param)
> +{
> +	struct bq24261_charger *chip =
> +		container_of(nb, struct bq24261_charger, cable.nb);
> +
> +	dev_dbg(&chip->client->dev, "external connector event(%ld)\n", event);
> +
> +	schedule_work(&chip->cable.work);
> +	return NOTIFY_OK;
> +}
> +
> +static int bq24261_extcon_register(struct bq24261_charger *chip)
> +{
> +	int ret;
> +
> +	INIT_WORK(&chip->cable.work, bq24261_extcon_event_work);
> +	chip->cable.nb.notifier_call = bq24261_handle_extcon_events;
> +
> +	ret = extcon_register_interest(&chip->cable.sdp, NULL,
> +				"SLOW-CHARGER", &chip->cable.nb);
> +	if (ret < 0) {
> +		dev_warn(&chip->client->dev,
> +				"extcon SDP registration failed(%d)\n", ret);
> +		goto sdp_reg_failed;
> +	}
> +
> +	ret = extcon_register_interest(&chip->cable.cdp, NULL,
> +				"CHARGE-DOWNSTREAM", &chip->cable.nb);
> +	if (ret < 0) {
> +		dev_warn(&chip->client->dev,
> +				"extcon CDP registration failed(%d)\n", ret);
> +		goto cdp_reg_failed;
> +	}
> +
> +	ret = extcon_register_interest(&chip->cable.dcp, NULL,
> +				"FAST-CHARGER", &chip->cable.nb);
> +	if (ret < 0) {
> +		dev_warn(&chip->client->dev,
> +				"extcon DCP registration failed(%d)\n", ret);
> +		goto dcp_reg_failed;
> +	}
> +
> +	ret = extcon_register_interest(&chip->cable.otg, NULL,
> +				"USB-Host", &chip->cable.nb);
> +	if (ret < 0) {
> +		dev_warn(&chip->client->dev,
> +			"extcon USB-Host registration failed(%d)\n", ret);
> +		goto otg_reg_failed;
> +	}
> +
> +	return 0;
> +
> +otg_reg_failed:
> +	extcon_unregister_interest(&chip->cable.dcp);
> +dcp_reg_failed:
> +	extcon_unregister_interest(&chip->cable.cdp);
> +cdp_reg_failed:
> +	extcon_unregister_interest(&chip->cable.sdp);
> +sdp_reg_failed:
> +	return -EPROBE_DEFER;
> +}
> +
> +static void bq24261_of_pdata(struct bq24261_charger *chip)
> +{
> +	static struct bq24261_platform_data pdata;
> +	struct device *dev = &chip->client->dev;
> +	int ret;
> +
> +	ret = device_property_read_u32(dev,
> +				"ti,charge-current", &pdata.def_cc);
> +	if (ret < 0)
> +		goto of_err;
> +	ret = device_property_read_u32(dev,
> +				"ti,charge-voltage", &pdata.def_cv);
> +	if (ret < 0)
> +		goto of_err;
> +	ret = device_property_read_u32(dev,
> +				"ti,termination-current", &pdata.iterm);
> +	if (ret < 0)
> +		goto of_err;
> +	ret = device_property_read_u32(dev,
> +				"ti,max-charge-current", &pdata.max_cc);
> +	if (ret < 0)
> +		goto of_err;
> +	ret = device_property_read_u32(dev,
> +				"ti,max-charge-voltage", &pdata.max_cv);
> +	if (ret < 0)
> +		goto of_err;
> +	ret = device_property_read_u32(dev,
> +				"ti,min-charge-temperature", &pdata.min_temp);
> +	if (ret < 0)
> +		goto of_err;
> +	ret = device_property_read_u32(dev,
> +				"ti,max-charge-temperature", &pdata.max_temp);
> +	if (ret < 0)
> +		goto of_err;
> +
> +	pdata.thermal_sensing = device_property_read_bool(dev,
> +						"ti,thermal-sensing");
> +	pdata.en_user_write = device_property_read_bool(dev,
> +						"ti,enable-user-write");
> +
> +	chip->pdata = &pdata;
> +	return;
> +of_err:
> +	dev_err(dev, "error in getting DT property(%d)\n", ret);
> +}
> +
> +static int bq24261_get_model(struct i2c_client *client,
> +			enum bq2426x_model *model)
> +{
> +	int ret;
> +
> +	ret = bq24261_read_reg(client, BQ24261_VENDOR_REV_ADDR);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((ret & BQ24261_VENDOR_MASK) != VENDOR_BQ2426X)
> +		return -EINVAL;
> +
> +	switch (ret & BQ24261_REV_MASK) {
> +	case REV_BQ24261:

Where do you get the information that a value of 0x6 corresponds to the
BQ24261 and not another device? When looking at the publicly available
datasheet these revision bits are not documented, so technically we
can't use them.
http://www.ti.com/product/BQ24261/datasheet/detailed_description#SLUSBU41233

It might be better to have the user specify which model of device to
use, like done here (shameless plug)
http://marc.info/?l=linux-pm&m=144175761831319

> +		*model = BQ24261;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int bq24261_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +	struct power_supply_config charger_cfg = {};
> +	struct bq24261_charger *chip;
> +	int ret;
> +	enum bq2426x_model model;
> +
> +	adapter = to_i2c_adapter(client->dev.parent);
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> +		dev_err(&client->dev,
> +			"I2C adapter %s doesn'tsupport BYTE DATA transfer\n",
> +			adapter->name);
> +		return -EIO;
> +	}
> +
> +	ret = bq24261_get_model(client, &model);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "chip detection error (%d)\n", ret);
> +		return -ENODEV;
> +	}
> +
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->client = client;
> +	if (client->dev.platform_data)
> +		chip->pdata = client->dev.platform_data;
> +	else if (id->driver_data)
> +		chip->pdata = (struct bq24261_platform_data *)id->driver_data;

What do the above two lines do? I understand that the statement above it
is for using platform data, and the statement below is for getting the
initial parameters from the device tree. But using/casting
id->driver_data as platform data?

> +	else
> +		bq24261_of_pdata(chip);
> +
> +	if (!chip->pdata) {
> +		dev_err(&client->dev, "platform data not found");
> +		return -ENODEV;
> +	}
> +
> +	i2c_set_clientdata(client, chip);
> +	mutex_init(&chip->lock);
> +	chip->model = model;
> +
> +	/* Initialize charger parameters */
> +	chip->cc = chip->pdata->def_cc;
> +	chip->cv = chip->pdata->def_cv;
> +	chip->iterm = chip->pdata->iterm;
> +	chip->chg_status = BQ24261_CHRGR_STAT_UNKNOWN;
> +	chip->chg_health = POWER_SUPPLY_HEALTH_UNKNOWN;
> +
> +	charger_cfg.drv_data = chip;
> +	charger_cfg.supplied_to = bq24261_charger_supplied_to;
> +	charger_cfg.num_supplicants = ARRAY_SIZE(bq24261_charger_supplied_to);
> +	if (chip->pdata->en_user_write) {
> +		bq24261_charger_desc.set_property = bq24261_usb_set_property;
> +		bq24261_charger_desc.property_is_writeable =
> +						bq24261_property_is_writeable;
> +	}
> +	chip->psy_usb = power_supply_register(&client->dev,
> +				&bq24261_charger_desc, &charger_cfg);
> +	if (IS_ERR(chip->psy_usb)) {
> +		dev_err(&client->dev,
> +			"power supply registration failed(%d)\n", ret);
> +		return ret;
> +	}
> +
> +	INIT_DELAYED_WORK(&chip->wdt_work, bq24261_wdt_reset_worker);
> +	INIT_DELAYED_WORK(&chip->fault_mon_work, bq24261_fault_mon_work);
> +
> +	ret = bq24261_extcon_register(chip);
> +	if (ret < 0)
> +		goto extcon_reg_failed;
> +
> +	if (chip->client->irq) {
> +		ret = request_threaded_irq(chip->client->irq,
> +					   NULL, bq24261_thread_handler,
> +					   IRQF_SHARED | IRQF_NO_SUSPEND,
> +					   DEV_NAME, chip);

Should use the managed version of requesting an IRQ
(devm_request_threaded_irq), this way it doesn't need to get freed at
the end.

> +		if (ret) {
> +			dev_err(&client->dev,
> +				"irq request failed (%d)\n", ret);
> +			goto irq_reg_failed;
> +		}
> +		INIT_WORK(&chip->irq_work, bq24261_irq_worker);
> +	}
> +
> +	/* Check for charger connecetd boot case */

Spelling (connected).

> +	schedule_work(&chip->cable.work);
> +
> +	return 0;
> +
> +irq_reg_failed:
> +	extcon_unregister_interest(&chip->cable.sdp);
> +	extcon_unregister_interest(&chip->cable.cdp);
> +	extcon_unregister_interest(&chip->cable.dcp);
> +	extcon_unregister_interest(&chip->cable.otg);
> +extcon_reg_failed:
> +	power_supply_unregister(chip->psy_usb);
> +	return ret;
> +}
> +
> +static int bq24261_remove(struct i2c_client *client)
> +{
> +	struct bq24261_charger *chip = i2c_get_clientdata(client);
> +
> +	free_irq(client->irq, chip);
> +	flush_scheduled_work();
> +	extcon_unregister_interest(&chip->cable.sdp);
> +	extcon_unregister_interest(&chip->cable.cdp);
> +	extcon_unregister_interest(&chip->cable.dcp);
> +	extcon_unregister_interest(&chip->cable.otg);
> +	power_supply_unregister(chip->psy_usb);
> +	return 0;
> +}
> +
> +static int bq24261_suspend(struct device *dev)
> +{
> +	struct bq24261_charger *chip = dev_get_drvdata(dev);
> +
> +	dev_dbg(&chip->client->dev, "bq24261 suspend\n");
> +	return 0;
> +}
> +
> +static int bq24261_resume(struct device *dev)
> +{
> +	struct bq24261_charger *chip = dev_get_drvdata(dev);
> +
> +	dev_dbg(&chip->client->dev, "bq24261 resume\n");
> +	return 0;
> +}

The resume function is basically empty... Does this mean we can just
remove all of the pm stuff as well as the entry in struct i2c_driver
bq24261_driver?

> +
> +static SIMPLE_DEV_PM_OPS(bq24261_pm_ops, bq24261_suspend,
> +			bq24261_resume);
> +
> +static const struct i2c_device_id bq24261_id[] = {
> +	{"bq24261", 0},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, bq24261_id);
> +
> +static const struct acpi_device_id bq24261_acpi_match[] = {
> +	{"BQ24261", 0},

ACPI IDs should be 8 characters long, see
https://lkml.org/lkml/2015/7/30/143

In analogy to the bq24257 driver Laurentiu developed I suggest simply
adding a trailing zero (0) to the string.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(acpi, bq24261_acpi_match);
> +
> +static const struct of_device_id bq24261_of_match[] = {
> +	{ .compatible = "ti,bq24261", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, bq24261_of_match);
> +
> +static struct i2c_driver bq24261_driver = {
> +	.driver = {
> +		.name = DEV_NAME,
> +		.acpi_match_table = ACPI_PTR(bq24261_acpi_match),
> +		.of_match_table = of_match_ptr(bq24261_of_match),
> +		.pm = &bq24261_pm_ops,
> +	},
> +	.probe = bq24261_probe,
> +	.remove = bq24261_remove,
> +	.id_table = bq24261_id,
> +};
> +
> +module_i2c_driver(bq24261_driver);
> +
> +MODULE_AUTHOR("Jenny TC <jenny.tc@intel.com>");
> +MODULE_AUTHOR("Ramakrishna Pallala <ramakrishna.pallala@intel.com>");
> +MODULE_DESCRIPTION("BQ24261 Charger Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/power/bq24261_charger.h b/include/linux/power/bq24261_charger.h
> new file mode 100644
> index 0000000..3ac2986
> --- /dev/null
> +++ b/include/linux/power/bq24261_charger.h
> @@ -0,0 +1,27 @@
> +/*
> + * bq24261_charger.h: platform data structure for bq24261 driver
> + *
> + * Copyright (C) 2014 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +
> +#ifndef __BQ24261_CHARGER_H__
> +#define __BQ24261_CHARGER_H__
> +
> +struct bq24261_platform_data {
> +	int def_cc;	/* in mA */
> +	int def_cv;	/* in mV */
> +	int iterm;	/* in mA */
> +	int max_cc;	/* in mA */
> +	int max_cv;	/* in mV */
> +	int min_temp;	/* in DegC */
> +	int max_temp;	/* in DegC */
> +	bool thermal_sensing;
> +	bool en_user_write;
> +};
> +
> +#endif
> -- 
> 1.7.9.5
> 

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Alexey Klimov]

Hi Ramakrishna,

On Sun, Sep 6, 2015 at 8:23 PM, Ramakrishna Pallala
<ramakrishna.pallala@intel.com> wrote:
> Add new charger driver support for BQ24261 charger IC.
>
> BQ24261 charger driver relies on extcon notifications to get the
> charger cable type and based on that it will set the charging parameters.
>
> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> Signed-off-by: Jennt TC <jenny.tc@intel.com>
> ---
>  .../devicetree/bindings/power/bq24261.txt          |   37 +
>  drivers/power/Kconfig                              |    6 +
>  drivers/power/Makefile                             |    1 +
>  drivers/power/bq24261_charger.c                    | 1208 ++++++++++++++++++++

[..]

> +static int bq24261_probe(struct i2c_client *client,
> +                        const struct i2c_device_id *id)
> +{
> +       struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +       struct power_supply_config charger_cfg = {};
> +       struct bq24261_charger *chip;
> +       int ret;
> +       enum bq2426x_model model;
> +
> +       adapter = to_i2c_adapter(client->dev.parent);

Please also check if you really need to call to_i2c_adapter() twice.

-- 
Best regards, Klimov Alexey

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Krzysztof Kozlowski]

On 10.09.2015 03:11, Pallala, Ramakrishna wrote:
>>> +Optional properties:
>>> +- ti,thermal-sensing: boolean, if present thermal regulation will be
>>> +enabled;
>>
>> What is the requirement for thermal-sensing? Can it be enabled always?
>> If yes, then this is not really a hardware property.
> TI BQ24261 has provision to add Battery Pack thermistor but it has no ADC read it.
> So a HW designer would or may not add the thermistor to charger and instead he can
> connect to the Fuel Gauge.

Thanks for explanation, makes sense.

> 
>>> +- ti,enable-user-write: boolean, if present driver will allow the user space
>>> +    to control the charging current and voltage through sysfs;
>>
>> This is not DT property. It does not describe hardware.
> We needed a mechanism to enable the sysfs writes on certain properties.
> If DT is not the place where should it go?

DT is not the place. As I discussed later with Andreas, if you really
need this and if mainline is a place for that then probably this should
be compile option (a Kconfig symbol).

I found one more issue after Andreas comments but I respond in that email.

Best regards,
Krzysztof

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Krzysztof Kozlowski]

On 10.09.2015 02:31, Andreas Dannenberg wrote:
> On Wed, Sep 09, 2015 at 01:17:11PM +0900, Krzysztof Kozlowski wrote:
>> On 09.09.2015 11:26, Andreas Dannenberg wrote:
>>> Krzysztof, good observation! In bq2425x_charger.c (formerly known as
>>> bq24257_charger.c :) that I worked on the unit used was uA. At that time
>>> I did a quick check and there didn't seem to be a clear standard whether
>>> to use the "micro" or "milli" units - different drivers use different
>>> units. However there seems to be a tendency for the TI drivers to prefer
>>> "milli" (bq2415x_charger.c, bq24735-charger.c)
>>>
>>> Personally I think "milli" units are more appropriate for chargers since
>>> they provide sufficient granularity and the numbers don't become too big
>>> (try typing a voltage in the Volt-range in uV, it's very easy to get the
>>> number of 0s wrong). However since the driver was already there I left
>>> that aspect alone to preserve compatibility.
>>
>> I am fine with both units but milli indeed seems easier to judge by fast
>> looking and less error-prone. Whatever you choose - choose the same one. :)
> 
> Ok sounds good. If so, I could go ahead and change the units in the
> bq2425x_charger.c over to mA and mV?

Wait, these are existing bindings (for bq24257). You cannot change
existing binding.

Best regards,
Krzysztof

> It would be a bit labor some and I
> also want to see what Laurentiu thinks but this way we could have most
> of those TI charger drivers use the same units (the new bq24261 driver
> Ram posted also uses mA/mV). Except bq25890_charger.c.... that would
> still use uA/uV....
> 
> Laurentiu -- what made you chose the "micro" units for bq24257_charger.c
> and bq25890_charger.c?

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Laurentiu Palcu]

On Thu, Sep 10, 2015 at 08:49:26AM +0900, Krzysztof Kozlowski wrote:
> On 10.09.2015 02:31, Andreas Dannenberg wrote:
> > On Wed, Sep 09, 2015 at 01:17:11PM +0900, Krzysztof Kozlowski wrote:
> >> On 09.09.2015 11:26, Andreas Dannenberg wrote:
> >>> Krzysztof, good observation! In bq2425x_charger.c (formerly known as
> >>> bq24257_charger.c :) that I worked on the unit used was uA. At that time
> >>> I did a quick check and there didn't seem to be a clear standard whether
> >>> to use the "micro" or "milli" units - different drivers use different
> >>> units. However there seems to be a tendency for the TI drivers to prefer
> >>> "milli" (bq2415x_charger.c, bq24735-charger.c)
> >>>
> >>> Personally I think "milli" units are more appropriate for chargers since
> >>> they provide sufficient granularity and the numbers don't become too big
> >>> (try typing a voltage in the Volt-range in uV, it's very easy to get the
> >>> number of 0s wrong). However since the driver was already there I left
> >>> that aspect alone to preserve compatibility.
> >>
> >> I am fine with both units but milli indeed seems easier to judge by fast
> >> looking and less error-prone. Whatever you choose - choose the same one. :)
> > 
> > Ok sounds good. If so, I could go ahead and change the units in the
> > bq2425x_charger.c over to mA and mV?
> 
> Wait, these are existing bindings (for bq24257). You cannot change
> existing binding.
> 
> Best regards,
> Krzysztof
> 
> > It would be a bit labor some and I
> > also want to see what Laurentiu thinks but this way we could have most
> > of those TI charger drivers use the same units (the new bq24261 driver
> > Ram posted also uses mA/mV). Except bq25890_charger.c.... that would
> > still use uA/uV....
> > 
> > Laurentiu -- what made you chose the "micro" units for bq24257_charger.c
> > and bq25890_charger.c?
When I started writing the BQ24257 driver, I had a look on what existed
already. Somehow, I don't know why, I settled on bq24190_charger which
has all the units in uA/uV. I thought it's the de-facto standard for
charger drivers... :/

For bindings, I looked at the existing TI chips bindings and chose the
same binding names, where possible. But for some dumb reason, I missed
that the units already used where mainly in mA/mV and, honestly, I
didn't even suspect units must be consistent from device to device. What
happens if a chip needs a more fine grained setting? I agree on binding
name consistency though: ti,charge-current (for example) should mean the
same for all TI chargers.

That said, is not much we can do on the mA/uA or mV/uV front... At least
we can agree on using the same binding names.

laurentiu

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Andrew F. Davis]

On 09/09/2015 06:47 PM, Krzysztof Kozlowski wrote:
>>>> +- ti,enable-user-write: boolean, if present driver will allow the user space
>>>> +    to control the charging current and voltage through sysfs;
>>>
>>> This is not DT property. It does not describe hardware.
>> We needed a mechanism to enable the sysfs writes on certain properties.
>> If DT is not the place where should it go?
>
> DT is not the place. As I discussed later with Andreas, if you really
> need this and if mainline is a place for that then probably this should
> be compile option (a Kconfig symbol).
>

I think this would actually be a good use for module parameters, this way
it could still be set at boot without re-compiling.

I think compile-time disabling sysfs properties because they are "dangerous" is
a little bit too artificially restricting and controlling, you can set permissions
so only root can change them already. The kernel should not be restricting root,
I understand the fear of someone rooting a machine and remotely over charging
a LiPo[1], but these physical limits are hardware descriptions and can and should
be set by DT, beyond this root should have full control over their machine.
Besides root can already just unbind your driver and issue raw I2C commands to do
the same thing. </rant>

[1] http://i.imgur.com/vszJJ.jpg

Regards,
Andrew

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Krzysztof Kozlowski]

On 11.09.2015 01:42, Andrew F. Davis wrote:
> On 09/09/2015 06:47 PM, Krzysztof Kozlowski wrote:
>>>>> +- ti,enable-user-write: boolean, if present driver will allow the
>>>>> user space
>>>>> +    to control the charging current and voltage through sysfs;
>>>>
>>>> This is not DT property. It does not describe hardware.
>>> We needed a mechanism to enable the sysfs writes on certain properties.
>>> If DT is not the place where should it go?
>>
>> DT is not the place. As I discussed later with Andreas, if you really
>> need this and if mainline is a place for that then probably this should
>> be compile option (a Kconfig symbol).
>>
> 
> I think this would actually be a good use for module parameters, this way
> it could still be set at boot without re-compiling.
> 
> I think compile-time disabling sysfs properties because they are
> "dangerous" is
> a little bit too artificially restricting and controlling, you can set
> permissions
> so only root can change them already. The kernel should not be
> restricting root,
> I understand the fear of someone rooting a machine and remotely over
> charging
> a LiPo[1], but these physical limits are hardware descriptions and can
> and should
> be set by DT, beyond this root should have full control over their machine.


Indeed module parameters could be used for enabling/disabling debug
options... but as fair as I understand these are for purely development
purposes. That is why they got into DT initially, right? To allow the
developer to play with it on the development board?

This is why I am really not convinced that this should go to mainline.

Anyway if it goes, then maybe compiling it out is the safest choice?
What's the purpose of having it in kernel all the time? If this was a
debug option, than some experienced user could turn it on and report to
LKML with extended debug data. But it's not a debug but development option?

> Besides root can already just unbind your driver and issue raw I2C
> commands to do
> the same thing. </rant>
> 
> [1] http://i.imgur.com/vszJJ.jpg

Oh, these weird sickos... That's why I am using IPoAC and always check
the bits by myself for weird looking I2C commands. :)

Best regards,
Krzysztof

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2321 bytes --]

Hi,

On Fri, Sep 11, 2015 at 09:58:40AM +0900, Krzysztof Kozlowski wrote:
> On 11.09.2015 01:42, Andrew F. Davis wrote:
> > On 09/09/2015 06:47 PM, Krzysztof Kozlowski wrote:
> >>>>> +- ti,enable-user-write: boolean, if present driver will allow the
> >>>>> user space
> >>>>> +    to control the charging current and voltage through sysfs;
> >>>>
> >>>> This is not DT property. It does not describe hardware.
> >>> We needed a mechanism to enable the sysfs writes on certain properties.
> >>> If DT is not the place where should it go?
> >>
> >> DT is not the place. As I discussed later with Andreas, if you really
> >> need this and if mainline is a place for that then probably this should
> >> be compile option (a Kconfig symbol).
> >>
> > 
> > I think this would actually be a good use for module parameters, this way
> > it could still be set at boot without re-compiling.
> > 
> > I think compile-time disabling sysfs properties because they are
> > "dangerous" is
> > a little bit too artificially restricting and controlling, you can set
> > permissions
> > so only root can change them already. The kernel should not be
> > restricting root,
> > I understand the fear of someone rooting a machine and remotely over
> > charging
> > a LiPo[1], but these physical limits are hardware descriptions and can
> > and should
> > be set by DT, beyond this root should have full control over their machine.
> 
> 
> Indeed module parameters could be used for enabling/disabling debug
> options... but as fair as I understand these are for purely development
> purposes. That is why they got into DT initially, right? To allow the
> developer to play with it on the development board?
> 
> This is why I am really not convinced that this should go to mainline.
> 
> Anyway if it goes, then maybe compiling it out is the safest choice?
> What's the purpose of having it in kernel all the time? If this was a
> debug option, than some experienced user could turn it on and report to
> LKML with extended debug data. But it's not a debug but development option?

Changing the current limit is useful for "expert" users with custom
usb power supplies, that are not correctly detected by extcon. I
also think a module parameter would be the best option here.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

> Hi,
> 
> On Fri, Sep 11, 2015 at 09:58:40AM +0900, Krzysztof Kozlowski wrote:
> > On 11.09.2015 01:42, Andrew F. Davis wrote:
> > > On 09/09/2015 06:47 PM, Krzysztof Kozlowski wrote:
> > >>>>> +- ti,enable-user-write: boolean, if present driver will allow
> > >>>>> +the
> > >>>>> user space
> > >>>>> +    to control the charging current and voltage through sysfs;
> > >>>>
> > >>>> This is not DT property. It does not describe hardware.
> > >>> We needed a mechanism to enable the sysfs writes on certain properties.
> > >>> If DT is not the place where should it go?
> > >>
> > >> DT is not the place. As I discussed later with Andreas, if you
> > >> really need this and if mainline is a place for that then probably
> > >> this should be compile option (a Kconfig symbol).
> > >>
> > >
> > > I think this would actually be a good use for module parameters,
> > > this way it could still be set at boot without re-compiling.
> > >
> > > I think compile-time disabling sysfs properties because they are
> > > "dangerous" is a little bit too artificially restricting and
> > > controlling, you can set permissions so only root can change them
> > > already. The kernel should not be restricting root, I understand the
> > > fear of someone rooting a machine and remotely over charging a
> > > LiPo[1], but these physical limits are hardware descriptions and can
> > > and should be set by DT, beyond this root should have full control
> > > over their machine.
> >
> >
> > Indeed module parameters could be used for enabling/disabling debug
> > options... but as fair as I understand these are for purely
> > development purposes. That is why they got into DT initially, right?
> > To allow the developer to play with it on the development board?
> >
> > This is why I am really not convinced that this should go to mainline.
> >
> > Anyway if it goes, then maybe compiling it out is the safest choice?
> > What's the purpose of having it in kernel all the time? If this was a
> > debug option, than some experienced user could turn it on and report
> > to LKML with extended debug data. But it's not a debug but development
> option?
> 
> Changing the current limit is useful for "expert" users with custom usb power
> supplies, that are not correctly detected by extcon. I also think a module
> parameter would be the best option here.

Ok. I will resubmit the patches along with fixing the other comments.

Thanks,
Ram

RE: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi Andreas, 

> Hi Ram, thanks for submitting this, please see some feedback inlined...
> 
> On Sun, Sep 06, 2015 at 10:53:07PM +0530, Ramakrishna Pallala wrote:
> > Add new charger driver support for BQ24261 charger IC.
> >
> > BQ24261 charger driver relies on extcon notifications to get the
> > charger cable type and based on that it will set the charging parameters.
> >
> > Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> > Signed-off-by: Jennt TC <jenny.tc@intel.com>
> > ---
> >  .../devicetree/bindings/power/bq24261.txt          |   37 +
> >  drivers/power/Kconfig                              |    6 +
> >  drivers/power/Makefile                             |    1 +
> >  drivers/power/bq24261_charger.c                    | 1208 ++++++++++++++++++++
> >  include/linux/power/bq24261_charger.h              |   27 +
> >  5 files changed, 1279 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/power/bq24261.txt
> >  create mode 100644 drivers/power/bq24261_charger.c  create mode
> > 100644 include/linux/power/bq24261_charger.h
> >
> > diff --git a/Documentation/devicetree/bindings/power/bq24261.txt
> > b/Documentation/devicetree/bindings/power/bq24261.txt
> > new file mode 100644
> > index 0000000..25fc5c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/bq24261.txt
> > @@ -0,0 +1,37 @@
> > +Binding for TI bq24261 Li-Ion Charger
> > +
> > +Required properties:
> > +- compatible: Should contain one of the following:
> > +    * "ti,bq24261"
> > +- reg: integer, i2c address of the device.
> > +- ti,charge-current: integer, default charging current (in mA);
> > +- ti,charge-voltage: integer, default charging voltage (in mV);
> 
> If you look at other chargers (bq2415x, bq24257, ...) this property should
> probably be called ti,battery-regulation-voltage.
Ok.


> > +- ti,termination-current: integer, charge will be terminated when current in
> > +    constant-voltage phase drops below this value (in mA);
> 
> > +- ti,max-charge-current: integer, maximum charging current (in mA);
> > +- ti,max-charge-voltage: integer, maximum charging voltage (in mV);
> 
> What's the background of having these two properties? They don't impact any
> device HW settings, but rather look like some safeguard if somebody
> manipulates the sysfs to not exceed certain boundaries.
As we have mechanism to control the charge current and voltage from user space we need safety limits.


> > +- ti,min-charge-temperature: integer, minimum charging temperature
> > +(in DegC);
> > +- ti,max-charge-temperature: integer, maximum charging temperature (in
> DegC).
> 
> All this does is passing these values down to the sysfs and exposing them
> through
> POWER_SUPPLY_PROP_TEMP_MAX/POWER_SUPPLY_PROP_TEMP_MIN. What
> value does this provide?
Same as above reason if we disable thermal sensing.


> > +
> > +Optional properties:
> > +- ti,thermal-sensing: boolean, if present thermal regulation will be
> > +enabled;
> 
> I received feedback for one of my recent patches that it's better to use an
> integer property as it can be overridden and with that is more flexible.
> 
> > +- ti,enable-user-write: boolean, if present driver will allow the user space
> > +    to control the charging current and voltage through sysfs;
> 
> This idea came from me :) but we should probably get rid of this capability. And
> the writability of charge current/voltage properties through sysfs altogether. My
> use case for this was to enable better testing but there is other ways this can be
> accomplished. However since some other drivers expose such capability I
> wonder what other reasons/use cases there might be (besides helping
> development and testing)?

I have added module parameter to disable the sysfs write.

> > +#include <linux/version.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/slab.h>
> > +#include <linux/device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/power_supply.h>
> > +#include <linux/extcon.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/io.h>
> > +#include <linux/sched.h>
> > +#include <linux/delay.h>
> > +#include <linux/acpi.h>
> > +#include <linux/power/bq24261_charger.h>
> > +
> > +#define DEV_NAME "bq24261_charger"
> 
> Can we use a dash/hyphen rather than an underscore? This would align better
Ok.

> with bq2415x and bq24257 (=bq2425x).
> 
> > +
> > +#define EXCEPTION_MONITOR_DELAY		(60 * HZ)
> > +#define WDT_RESET_DELAY			(15 * HZ)
> > +
> > +/* BQ24261 registers */
> > +#define BQ24261_STAT_CTRL0_ADDR		0x00
> > +#define BQ24261_CTRL_ADDR		0x01
> > +#define BQ24261_BATT_VOL_CTRL_ADDR	0x02
> > +#define BQ24261_VENDOR_REV_ADDR		0x03
> > +#define BQ24261_TERM_FCC_ADDR		0x04
> > +#define BQ24261_VINDPM_STAT_ADDR	0x05
> > +#define BQ24261_ST_NTC_MON_ADDR		0x06
> > +
> > +#define BQ24261_RESET_MASK		(0x01 << 7)
> 
> What about using the BIT() macro for all of these?
Ok.

> 
> > +#define BQ24261_RESET_ENABLE		(0x01 << 7)
> > +
> > +#define BQ24261_FAULT_MASK		0x07
> > +#define BQ24261_STAT_MASK		(0x03 << 4)
> 
> What about the GENMASK() macro for all bit fields?
OK.

> 
> > +#define BQ24261_BOOST_MASK		(0x01 << 6)
> > +#define BQ24261_TMR_RST_MASK		(0x01 << 7)
> > +#define BQ24261_TMR_RST			(0x01 << 7)
> > +
> > +#define BQ24261_ENABLE_BOOST		(0x01 << 6)
> > +
> > +#define BQ24261_VOVP			0x01
> > +#define BQ24261_LOW_SUPPLY		0x02
> > +#define BQ24261_THERMAL_SHUTDOWN	0x03
> > +#define BQ24261_BATT_TEMP_FAULT		0x04
> > +#define BQ24261_TIMER_FAULT		0x05
> > +#define BQ24261_BATT_OVP		0x06
> > +#define BQ24261_NO_BATTERY		0x07
> > +#define BQ24261_STAT_READY		0x00
> > +
> > +#define BQ24261_STAT_CHRG_PRGRSS	(0x01 << 4)
> > +#define BQ24261_STAT_CHRG_DONE		(0x02 << 4)
> > +#define BQ24261_STAT_FAULT		(0x03 << 4)
> > +
> > +#define BQ24261_CE_MASK			(0x01 << 1)
> > +#define BQ24261_CE_DISABLE		(0x01 << 1)
> > +
> > +#define BQ24261_HiZ_MASK			(0x01)
> > +#define BQ24261_HiZ_ENABLE		(0x01)
> > +
> > +#define BQ24261_ICHRG_MASK		(0x1F << 3)
> > +
> > +#define BQ24261_ITERM_MASK		(0x03)
> > +#define BQ24261_MIN_ITERM		50 /* 50 mA */
> > +#define BQ24261_MAX_ITERM		300 /* 300 mA */
> > +
> > +#define BQ24261_VBREG_MASK		(0x3F << 2)
> > +#define BQ24261_VBREG_MIN_CV		3500
> > +#define BQ24261_VBREG_MAX_CV		4440
> > +#define BQ24261_VBREG_CV_DIV		20
> > +#define BQ24261_VBREG_CV_BIT_POS	2
> > +
> > +#define BQ24261_INLMT_MASK		(0x07 << 4)
> > +#define BQ24261_INLMT_100		0x00
> > +#define BQ24261_INLMT_150		(0x01 << 4)
> > +#define BQ24261_INLMT_500		(0x02 << 4)
> > +#define BQ24261_INLMT_900		(0x03 << 4)
> > +#define BQ24261_INLMT_1500		(0x04 << 4)
> > +#define BQ24261_INLMT_2000		(0x05 << 4)
> > +#define BQ24261_INLMT_2500		(0x06 << 4)
> > +
> > +#define BQ24261_TE_MASK			(0x01 << 2)
> > +#define BQ24261_TE_ENABLE		(0x01 << 2)
> > +#define BQ24261_STAT_ENABLE_MASK	(0x01 << 3)
> > +#define BQ24261_STAT_ENABLE		(0x01 << 3)
> > +
> > +#define BQ24261_VENDOR_MASK		(0x07 << 5)
> > +#define BQ24261_PART_MASK		(0x03 << 3)
> > +#define BQ24261_REV_MASK		(0x07)
> > +#define VENDOR_BQ2426X			(0x02 << 5)
> > +#define REV_BQ24261			(0x06)
> > +
> > +#define BQ24261_TS_MASK			(0x01 << 3)
> > +#define BQ24261_TS_ENABLED		(0x01 << 3)
> > +#define BQ24261_BOOST_ILIM_MASK		(0x01 << 4)
> > +#define BQ24261_BOOST_ILIM_500ma	(0x0)
> > +#define BQ24261_BOOST_ILIM_1A		(0x01 << 4)
> > +#define BQ24261_VINDPM_OFF_MASK		(0x01 << 0)
> > +#define BQ24261_VINDPM_OFF_5V		(0x0)
> > +#define BQ24261_VINDPM_OFF_12V		(0x01 << 0)
> > +
> > +#define BQ24261_SAFETY_TIMER_MASK	(0x03 << 5)
> > +#define BQ24261_SAFETY_TIMER_40MIN	0x00
> > +#define BQ24261_SAFETY_TIMER_6HR	(0x01 << 5)
> > +#define BQ24261_SAFETY_TIMER_9HR	(0x02 << 5)
> > +#define BQ24261_SAFETY_TIMER_DISABLED	(0x03 << 5)
> > +
> > +/* 1% above voltage max design to report over voltage */
> > +#define BQ24261_OVP_MULTIPLIER			1010
> > +#define BQ24261_OVP_RECOVER_MULTIPLIER		990
> > +#define BQ24261_DEF_BAT_VOLT_MAX_DESIGN		4200000
> > +
> > +/* Settings for Voltage / DPPM Register (05) */
> > +#define BQ24261_VBATT_LEVEL1		3700000
> > +#define BQ24261_VBATT_LEVEL2		3960000
> > +#define BQ24261_VINDPM_MASK		(0x07)
> > +#define BQ24261_VINDPM_320MV		(0x01 << 2)
> > +#define BQ24261_VINDPM_160MV		(0x01 << 1)
> > +#define BQ24261_VINDPM_80MV		(0x01 << 0)
> > +#define BQ24261_CD_STATUS_MASK		(0x01 << 3)
> > +#define BQ24261_DPM_EN_MASK		(0x01 << 4)
> > +#define BQ24261_DPM_EN_FORCE		(0x01 << 4)
> > +#define BQ24261_LOW_CHG_MASK		(0x01 << 5)
> > +#define BQ24261_LOW_CHG_EN		(0x01 << 5)
> > +#define BQ24261_LOW_CHG_DIS		(~BQ24261_LOW_CHG_EN)
> > +#define BQ24261_DPM_STAT_MASK		(0x01 << 6)
> > +#define BQ24261_MINSYS_STAT_MASK	(0x01 << 7)
> > +
> > +#define BQ24261_MIN_CC			500	/* 500mA */
> > +#define BQ24261_MAX_CC			3000	/* 3A */
> > +#define BQ24261_DEF_CC			1300	/* 1300mA */
> > +#define BQ24261_MAX_CV			4350	/*4350mV */
> > +#define BQ24261_DEF_CV			4350	/* 4350mV */
> > +#define BQ24261_DEF_ITERM		128	/* 128mA */
> > +#define BQ24261_MIN_TEMP		0	/* 0 degC */
> > +#define BQ24261_MAX_TEMP		60	/* 60 DegC */
> > +
> > +#define ILIM_100MA			100	/* 100mA */
> > +#define ILIM_500MA			500	/* 500mA */
> > +#define ILIM_900MA			900	/* 900mA */
> > +#define ILIM_1500MA			1500	/* 1500mA */
> > +#define ILIM_2000MA			2000	/* 2000mA */
> > +#define ILIM_2500MA			2500	/* 2500mA */
> > +#define ILIM_3000MA			3000	/* 3000mA */
> > +
> > +u16 bq24261_inlmt[][2] = {
> > +	{100, BQ24261_INLMT_100},
> > +	{150, BQ24261_INLMT_150},
> > +	{500, BQ24261_INLMT_500},
> > +	{900, BQ24261_INLMT_900},
> > +	{1500, BQ24261_INLMT_1500},
> > +	{2000, BQ24261_INLMT_2000},
> > +	{2500, BQ24261_INLMT_2500},
> > +};
> > +
> > +
> > +enum bq24261_status {
> > +	BQ24261_CHRGR_STAT_UNKNOWN,
> > +	BQ24261_CHRGR_STAT_READY,
> > +	BQ24261_CHRGR_STAT_CHARGING,
> > +	BQ24261_CHRGR_STAT_FULL,
> > +	BQ24261_CHRGR_STAT_FAULT,
> > +};
> > +
> > +enum bq2426x_model {
> > +	BQ2426X = 0,
> > +	BQ24260,
> 
> The BQ24260 definition doesn't seem to get used in this driver.
> 
> > +	BQ24261,
> > +};
> > +
> > +struct bq24261_charger {
> > +	struct i2c_client *client;
> > +	struct bq24261_platform_data *pdata;
> > +	struct power_supply *psy_usb;
> > +	struct delayed_work fault_mon_work;
> > +	struct mutex lock;
> > +	enum bq2426x_model model;
> > +	struct delayed_work wdt_work;
> > +	struct work_struct irq_work;
> > +	struct list_head irq_queue;
> > +
> > +	/* extcon charger cables */
> > +	struct {
> > +		struct work_struct work;
> > +		struct notifier_block nb;
> > +		struct extcon_specific_cable_nb sdp;
> > +		struct extcon_specific_cable_nb cdp;
> > +		struct extcon_specific_cable_nb dcp;
> > +		struct extcon_specific_cable_nb otg;
> > +		enum power_supply_type chg_type;
> > +		bool boost;
> > +		bool connected;
> > +	} cable;
> > +
> > +	bool online;
> > +	bool present;
> > +	int chg_health;
> > +	enum bq24261_status chg_status;
> > +	int cc;
> > +	int cv;
> > +	int inlmt;
> > +	int max_cc;
> > +	int max_cv;
> > +	int iterm;
> > +	int max_temp;
> > +	int min_temp;
> > +	bool is_charging_enabled;
> > +};
> > +
> > +static inline int bq24261_read_reg(struct i2c_client *client, u8 reg)
> > +{
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_byte_data(client, reg);
> > +	if (ret < 0)
> > +		dev_err(&client->dev,
> > +			"error(%d) in reading reg %d\n", ret, reg);
> > +	return ret;
> > +}
> > +
> > +static inline int bq24261_write_reg(struct i2c_client *client, u8
> > +reg, u8 data) {
> > +	int ret;
> > +
> > +	ret = i2c_smbus_write_byte_data(client, reg, data);
> > +	if (ret < 0)
> > +		dev_err(&client->dev,
> > +			"error(%d) in writing %d to reg %d\n", ret, data, reg);
> > +	return ret;
> > +}
> > +
> > +static inline int bq24261_update_reg(struct i2c_client *client, u8 reg,
> > +					  u8 mask, u8 val)
> > +{
> > +	int ret;
> > +
> > +	ret = bq24261_read_reg(client, reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = (ret & ~mask) | (mask & val);
> > +	return bq24261_write_reg(client, reg, ret); }
> > +
> > +static void lookup_regval(u16 tbl[][2], size_t size, u16 in_val, u8
> > +*out_val) {
> > +	int i;
> > +
> > +	for (i = 1; i < size; ++i) {
> > +		if (in_val < tbl[i][0])
> > +			break;
> > +	}
> > +
> > +	*out_val = (u8) tbl[i - 1][1];
> > +}
> > +
> > +void bq24261_cc_to_reg(int cc, u8 *reg_val) {
> > +	/* Ichrg bits are B3-B7
> > +	 * Icharge = 500mA + IchrgCode * 100mA
> > +	 */
> > +	cc = clamp_t(int, cc, BQ24261_MIN_CC, BQ24261_MAX_CC);
> > +	cc = cc - BQ24261_MIN_CC;
> > +	*reg_val = (cc / 100) << 3;
> > +}
> > +
> > +void bq24261_cv_to_reg(int cv, u8 *reg_val) {
> > +	int val;
> > +
> > +	val = clamp_t(int, cv, BQ24261_VBREG_MIN_CV,
> BQ24261_VBREG_MAX_CV);
> > +	*reg_val = (((val - BQ24261_VBREG_MIN_CV) /
> BQ24261_VBREG_CV_DIV)
> > +			<< BQ24261_VBREG_CV_BIT_POS);
> > +}
> > +
> > +void bq24261_inlmt_to_reg(int inlmt, u8 *regval) {
> > +	return lookup_regval(bq24261_inlmt, ARRAY_SIZE(bq24261_inlmt),
> > +			     inlmt, regval);
> > +}
> > +
> > +static inline void bq24261_iterm_to_reg(int iterm, u8 *regval) {
> > +	/* Iterm bits are B0-B2
> > +	 * Icharge = 50mA + ItermCode * 50mA
> > +	 */
> 
> Comment style.
> 
> > +	iterm = clamp_t(int, iterm, BQ24261_MIN_ITERM,
> BQ24261_MAX_ITERM);
> > +	iterm = iterm - BQ24261_MIN_ITERM;
> > +	*regval =  iterm / 50;
> > +}
> > +
> > +static inline int bq24261_init_timers(struct bq24261_charger *chip) {
> > +	u8 reg_val;
> > +	int ret;
> > +
> > +	reg_val = BQ24261_SAFETY_TIMER_9HR;
> > +
> > +	if (chip->pdata->thermal_sensing)
> > +		reg_val |= BQ24261_TS_ENABLED;
> > +
> > +	ret = bq24261_update_reg(chip->client,
> BQ24261_ST_NTC_MON_ADDR,
> > +			BQ24261_TS_MASK | BQ24261_SAFETY_TIMER_MASK
> |
> > +			BQ24261_BOOST_ILIM_MASK, reg_val);
> > +
> > +	return ret;
> > +}
> > +
> > +static inline int bq24261_reset_wdt_timer(struct bq24261_charger
> > +*chip) {
> > +	u8 mask = BQ24261_TMR_RST_MASK, val = BQ24261_TMR_RST;
> > +
> > +	if (chip->cable.boost) {
> > +		mask |= BQ24261_BOOST_MASK;
> > +		val |= BQ24261_ENABLE_BOOST;
> > +	}
> > +
> > +	return bq24261_update_reg(chip->client, BQ24261_STAT_CTRL0_ADDR,
> > +							mask, val);
> > +}
> > +
> > +static inline int bq24261_set_cc(struct bq24261_charger *chip, int
> > +cc_mA) {
> > +	u8 reg_val;
> > +	int ret;
> > +
> > +	dev_dbg(&chip->client->dev, "%s=%d\n", __func__, cc_mA);
> > +
> > +	if (cc_mA && (cc_mA < BQ24261_MIN_CC)) {
> > +		dev_dbg(&chip->client->dev, "Set LOW_CHG bit\n");
> > +		reg_val = BQ24261_LOW_CHG_EN;
> > +		ret = bq24261_update_reg(chip->client,
> > +				BQ24261_VINDPM_STAT_ADDR,
> > +				BQ24261_LOW_CHG_MASK, reg_val);
> > +		return ret;
> > +	}
> > +
> > +	reg_val = BQ24261_LOW_CHG_DIS;
> > +	ret = bq24261_update_reg(chip->client,
> BQ24261_VINDPM_STAT_ADDR,
> > +					BQ24261_LOW_CHG_MASK, reg_val);
> > +
> > +	bq24261_cc_to_reg(cc_mA, &reg_val);
> > +
> > +	return bq24261_update_reg(chip->client, BQ24261_TERM_FCC_ADDR,
> > +			BQ24261_ICHRG_MASK, reg_val);
> > +}
> > +
> > +static inline int bq24261_set_cv(struct bq24261_charger *chip, int
> > +cv_mV) {
> > +	u8 reg_val;
> > +
> > +	dev_dbg(&chip->client->dev, "%s=%d\n", __func__, cv_mV);
> > +
> > +	bq24261_cv_to_reg(cv_mV, &reg_val);
> > +
> > +	return bq24261_update_reg(chip->client,
> BQ24261_BATT_VOL_CTRL_ADDR,
> > +				       BQ24261_VBREG_MASK, reg_val); }
> > +
> > +static inline int bq24261_set_inlmt(struct bq24261_charger *chip, int
> > +inlmt) {
> > +	u8 reg_val;
> > +
> > +	dev_dbg(&chip->client->dev, "%s=%d\n", __func__, inlmt);
> > +
> > +	bq24261_inlmt_to_reg(inlmt, &reg_val);
> > +
> > +	/*
> > +	 * Don't enable reset bit. Setting this
> > +	 * bit will reset all the registers/
> 
> Extra character at the line end.
> 
> > +	 */
> > +	reg_val &= ~BQ24261_RESET_MASK;
> > +
> > +	return bq24261_update_reg(chip->client, BQ24261_CTRL_ADDR,
> > +		       BQ24261_RESET_MASK|BQ24261_INLMT_MASK, reg_val);
> > +
> > +}
> > +
> > +static inline int bq24261_set_iterm(struct bq24261_charger *chip, int
> > +iterm) {
> > +	u8 reg_val;
> > +
> > +	bq24261_iterm_to_reg(iterm, &reg_val);
> > +
> > +	return bq24261_update_reg(chip->client, BQ24261_TERM_FCC_ADDR,
> > +				       BQ24261_ITERM_MASK, reg_val); }
> > +
> > +static inline int bq24261_enable_charging(struct bq24261_charger *chip,
> > +								bool enable)
> > +{
> > +	int ret;
> > +	u8 reg_val;
> > +
> > +	if (enable) {
> > +		reg_val = (~BQ24261_CE_DISABLE & BQ24261_CE_MASK);
> > +		reg_val |= BQ24261_TE_ENABLE;
> > +	} else {
> > +		reg_val = BQ24261_CE_DISABLE;
> > +	}
> > +
> > +	reg_val |=  BQ24261_STAT_ENABLE;
> > +
> > +	/*
> > +	 * Don't enable reset bit. Setting this
> > +	 * bit will reset all the registers/
> 
> Extra character.
> 
> > +	 */
> > +	reg_val &= ~BQ24261_RESET_MASK;
> > +
> > +	ret = bq24261_update_reg(chip->client, BQ24261_CTRL_ADDR,
> > +		       BQ24261_STAT_ENABLE_MASK|BQ24261_RESET_MASK|
> > +				BQ24261_CE_MASK|BQ24261_TE_MASK,
> > +					reg_val);
> 
> Should use spaces around operators.
> 
> > +	if (ret || !enable)
> > +		return ret;
> > +
> > +	/* Set termination current */
> > +	ret = bq24261_set_iterm(chip, chip->iterm);
> > +	if (ret < 0)
> > +		dev_err(&chip->client->dev, "failed to set iTerm(%d)\n", ret);
> > +
> > +	/* Start WDT and Safety timers */
> > +	ret = bq24261_init_timers(chip);
> > +	if (ret)
> > +		dev_err(&chip->client->dev, "failed to set timers(%d)\n", ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static inline int bq24261_enable_charger(struct bq24261_charger *chip,
> > +								int enable)
> > +{
> > +	u8 reg_val;
> > +	int ret;
> > +
> > +	reg_val = enable ? (~BQ24261_HiZ_ENABLE & BQ24261_HiZ_MASK) :
> > +			BQ24261_HiZ_ENABLE;
> > +
> > +	/*
> > +	 * Don't enable reset bit. Setting this
> > +	 * bit will reset all the registers/
> > +	 */
> > +	reg_val &= ~BQ24261_RESET_MASK;
> > +
> > +	ret = bq24261_update_reg(chip->client, BQ24261_CTRL_ADDR,
> > +		       BQ24261_HiZ_MASK|BQ24261_RESET_MASK, reg_val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return bq24261_reset_wdt_timer(chip); }
> > +
> > +static void bq24261_handle_health(struct bq24261_charger *chip, u8
> > +stat_reg) {
> > +	struct i2c_client *client = chip->client;
> > +	bool fault_worker = false;
> > +
> > +	switch (stat_reg & BQ24261_FAULT_MASK) {
> > +	case BQ24261_VOVP:
> > +		chip->chg_health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> > +		fault_worker = true;
> > +		dev_err(&client->dev, "Charger Over Voltage Fault\n");
> > +		break;
> > +	case BQ24261_LOW_SUPPLY:
> > +		chip->chg_health = POWER_SUPPLY_HEALTH_DEAD;
> > +		fault_worker = true;
> > +		dev_err(&client->dev, "Charger Low Supply Fault\n");
> > +		break;
> > +	case BQ24261_THERMAL_SHUTDOWN:
> > +		chip->chg_health = POWER_SUPPLY_HEALTH_OVERHEAT;
> > +		dev_err(&client->dev, "Charger Thermal Fault\n");
> > +		break;
> > +	case BQ24261_BATT_TEMP_FAULT:
> > +		dev_err(&client->dev, "Battery Temperature Fault\n");
> > +		break;
> > +	case BQ24261_TIMER_FAULT:
> > +		chip->chg_health =
> POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> > +		dev_err(&client->dev, "Charger Timer Fault\n");
> > +		break;
> > +	case BQ24261_BATT_OVP:
> > +		chip->chg_health =
> POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> > +		dev_err(&client->dev, "Battery Over Voltage Fault\n");
> > +		break;
> > +	case BQ24261_NO_BATTERY:
> > +		dev_err(&client->dev, "No Battery Connected\n");
> > +		break;
> > +	default:
> > +		chip->chg_health = POWER_SUPPLY_HEALTH_GOOD;
> > +	}
> > +
> > +	if (fault_worker)
> > +		schedule_delayed_work(&chip->fault_mon_work,
> > +					EXCEPTION_MONITOR_DELAY);
> > +}
> > +
> > +static void bq24261_handle_status(struct bq24261_charger *chip, u8
> > +stat_reg) {
> > +	struct i2c_client *client = chip->client;
> > +
> > +	switch (stat_reg & BQ24261_STAT_MASK) {
> > +	case BQ24261_STAT_READY:
> > +		chip->chg_status = BQ24261_CHRGR_STAT_READY;
> > +		dev_info(&client->dev, "Charger Status: Ready\n");
> > +		break;
> > +	case BQ24261_STAT_CHRG_PRGRSS:
> > +		chip->chg_status = BQ24261_CHRGR_STAT_CHARGING;
> > +		dev_info(&client->dev, "Charger Status: Charge Progress\n");
> > +		break;
> > +	case BQ24261_STAT_CHRG_DONE:
> > +		chip->chg_status = BQ24261_CHRGR_STAT_FULL;
> > +		dev_info(&client->dev, "Charger Status: Charge Done\n");
> > +		break;
> > +	case BQ24261_STAT_FAULT:
> > +		chip->chg_status = BQ24261_CHRGR_STAT_FAULT;
> > +		dev_warn(&client->dev, "Charger Status: Fault\n");
> > +		break;
> > +	default:
> > +		dev_info(&client->dev, "Invalid\n");
> > +	}
> > +}
> > +
> > +static int bq24261_get_charger_health(struct bq24261_charger *chip) {
> > +	if (!chip->present)
> > +		return POWER_SUPPLY_HEALTH_UNKNOWN;
> > +
> > +	return chip->chg_health;
> > +}
> > +
> > +static int bq24261_get_charging_status(struct bq24261_charger *chip)
> > +{
> > +	int status;
> > +
> > +	if (!chip->present)
> > +		return POWER_SUPPLY_STATUS_DISCHARGING;
> > +
> > +	switch (chip->chg_status) {
> > +	case BQ24261_CHRGR_STAT_READY:
> > +		status = POWER_SUPPLY_STATUS_DISCHARGING;
> > +		break;
> > +	case BQ24261_CHRGR_STAT_CHARGING:
> > +		status = POWER_SUPPLY_STATUS_CHARGING;
> > +		break;
> > +	case BQ24261_CHRGR_STAT_FULL:
> > +		status = POWER_SUPPLY_STATUS_FULL;
> > +		break;
> > +	case BQ24261_CHRGR_STAT_FAULT:
> > +		status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > +		break;
> > +	default:
> > +		status = POWER_SUPPLY_STATUS_DISCHARGING;
> > +	}
> > +
> > +	return status;
> > +}
> > +
> > +static int bq24261_usb_get_property(struct power_supply *psy,
> > +		enum power_supply_property psp, union power_supply_propval
> *val) {
> > +	struct bq24261_charger *chip = power_supply_get_drvdata(psy);
> > +
> > +	mutex_lock(&chip->lock);
> > +	switch (psp) {
> > +	case POWER_SUPPLY_PROP_PRESENT:
> > +		val->intval = chip->present;
> > +		break;
> > +	case POWER_SUPPLY_PROP_ONLINE:
> > +		val->intval = chip->online;
> > +		break;
> > +	case POWER_SUPPLY_PROP_HEALTH:
> > +		val->intval = bq24261_get_charger_health(chip);
> > +		break;
> > +	case POWER_SUPPLY_PROP_STATUS:
> > +		val->intval = bq24261_get_charging_status(chip);
> > +		break;
> > +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> > +		val->intval = chip->pdata->max_cc * 1000;
> > +		break;
> > +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> > +		val->intval = chip->pdata->max_cv * 1000;
> > +		break;
> > +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> > +		val->intval = chip->cc * 1000;
> > +		break;
> > +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> > +		val->intval = chip->cv * 1000;
> > +		break;
> > +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> > +		val->intval = chip->inlmt * 1000;
> > +		break;
> > +	case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
> > +		val->intval = chip->iterm * 1000;
> > +		break;
> > +	case POWER_SUPPLY_PROP_TEMP_MAX:
> > +		val->intval = chip->pdata->max_temp * 10;
> > +		break;
> > +	case POWER_SUPPLY_PROP_TEMP_MIN:
> > +		val->intval = chip->pdata->min_temp * 10;
> > +		break;
> > +	default:
> > +		mutex_unlock(&chip->lock);
> > +		return -EINVAL;
> > +	}
> > +	mutex_unlock(&chip->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int bq24261_usb_set_property(struct power_supply *psy,
> > +	enum power_supply_property psp, const union power_supply_propval
> > +*val) {
> > +	struct bq24261_charger *chip = power_supply_get_drvdata(psy);
> > +	int intval, ret = 0;
> > +
> > +	mutex_lock(&chip->lock);
> > +	switch (psp) {
> > +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> > +		/* convert uA to mA */
> > +		intval = val->intval / 1000;
> > +		if (intval > chip->max_cc) {
> > +			ret = -EINVAL;
> > +			break;
> > +		}
> > +
> > +		ret = bq24261_set_cc(chip, intval);
> > +		if (!ret)
> > +			chip->cc = intval;
> > +		break;
> > +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> > +		/* convert uA to mV */
> > +		intval = val->intval / 1000;
> > +		if (intval > chip->max_cv) {
> > +			ret = -EINVAL;
> > +			break;
> > +		}
> > +
> > +		ret = bq24261_set_cv(chip, intval);
> > +		if (!ret)
> > +			chip->cv = intval;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +	}
> > +	mutex_unlock(&chip->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int bq24261_property_is_writeable(struct power_supply *psy,
> > +		enum power_supply_property psp)
> > +{
> > +	int ret;
> > +
> > +	switch (psp) {
> > +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> > +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> > +		ret = 1;
> > +		break;
> > +	default:
> > +		ret = 0;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static enum power_supply_property bq24261_usb_props[] = {
> > +	POWER_SUPPLY_PROP_PRESENT,
> > +	POWER_SUPPLY_PROP_ONLINE,
> > +	POWER_SUPPLY_PROP_TYPE,
> > +	POWER_SUPPLY_PROP_HEALTH,
> > +	POWER_SUPPLY_PROP_STATUS,
> > +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> > +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
> > +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> > +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
> > +	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> > +	POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
> > +	POWER_SUPPLY_PROP_TEMP_MAX,
> > +	POWER_SUPPLY_PROP_TEMP_MIN,
> 
> What about adding support for POWER_SUPPLY_PROP_MODEL_NAME and
> POWER_SUPPLY_PROP_MANUFACTURER?
Can you add these properties in your patch and add the model name support as well.

> 
> > +};
> > +
> > +static char *bq24261_charger_supplied_to[] = {
> > +	"main-battery",
> > +};
> > +
> > +static struct power_supply_desc bq24261_charger_desc = {
> > +	.name			= DEV_NAME,
> > +	.type			= POWER_SUPPLY_TYPE_USB,
> > +	.properties		= bq24261_usb_props,
> > +	.num_properties		= ARRAY_SIZE(bq24261_usb_props),
> > +	.get_property		= bq24261_usb_get_property,
> > +};
> > +
> > +static void bq24261_wdt_reset_worker(struct work_struct *work) {
> > +
> > +	struct bq24261_charger *chip = container_of(work,
> > +			    struct bq24261_charger, wdt_work.work);
> > +	int ret;
> > +
> > +	ret = bq24261_reset_wdt_timer(chip);
> > +	if (ret)
> > +		dev_err(&chip->client->dev, "WDT timer reset error(%d)\n",
> ret);
> > +
> > +	schedule_delayed_work(&chip->wdt_work, WDT_RESET_DELAY); }
> > +
> > +static void bq24261_irq_worker(struct work_struct *work) {
> > +	struct bq24261_charger *chip =
> > +	    container_of(work, struct bq24261_charger, irq_work);
> > +	int ret;
> > +
> > +	/*
> > +	 * Lock to ensure that interrupt register readings are done
> > +	 * and processed sequentially. The interrupt Fault registers
> > +	 * are read on clear and without sequential processing double
> > +	 * fault interrupts or fault recovery cannot be handlled propely
> > +	 */
> > +
> > +	mutex_lock(&chip->lock);
> > +
> > +	ret = bq24261_read_reg(chip->client, BQ24261_STAT_CTRL0_ADDR);
> > +	if (ret < 0) {
> > +		dev_err(&chip->client->dev,
> > +			"Error (%d) in reading BQ24261_STAT_CTRL0_ADDR\n",
> ret);
> > +		goto irq_out;
> > +	}
> > +
> > +	if (!chip->cable.boost) {
> > +		bq24261_handle_status(chip, ret);
> > +		bq24261_handle_health(chip, ret);
> > +		power_supply_changed(chip->psy_usb);
> > +	}
> > +
> > +irq_out:
> > +	mutex_unlock(&chip->lock);
> > +}
> > +
> > +static irqreturn_t bq24261_thread_handler(int id, void *data) {
> > +	struct bq24261_charger *chip = (struct bq24261_charger *)data;
> > +
> > +	queue_work(system_highpri_wq, &chip->irq_work);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void bq24261_fault_mon_work(struct work_struct *work) {
> > +	struct bq24261_charger *chip = container_of(work,
> > +			struct bq24261_charger, fault_mon_work.work);
> > +	int ret;
> > +
> > +	if ((chip->chg_health == POWER_SUPPLY_HEALTH_OVERVOLTAGE) ||
> > +		(chip->chg_health == POWER_SUPPLY_HEALTH_DEAD)) {
> > +
> > +		mutex_lock(&chip->lock);
> > +		ret = bq24261_read_reg(chip->client,
> BQ24261_STAT_CTRL0_ADDR);
> > +		if (ret < 0) {
> > +			dev_err(&chip->client->dev,
> > +				"Status register read failed(%d)\n", ret);
> > +			goto fault_mon_out;
> > +		}
> > +
> > +		if ((ret & BQ24261_STAT_MASK) == BQ24261_STAT_READY) {
> > +			dev_info(&chip->client->dev,
> > +					"Charger fault recovered\n");
> > +			bq24261_handle_status(chip, ret);
> > +			bq24261_handle_health(chip, ret);
> > +			power_supply_changed(chip->psy_usb);
> > +		}
> > +fault_mon_out:
> > +		mutex_unlock(&chip->lock);
> > +	}
> > +}
> > +
> > +static void bq24261_boost_control(struct bq24261_charger *chip, bool
> > +enable) {
> > +	int ret;
> > +
> > +	if (enable)
> > +		ret = bq24261_write_reg(chip->client,
> BQ24261_STAT_CTRL0_ADDR,
> > +				BQ24261_TMR_RST |
> BQ24261_ENABLE_BOOST);
> > +	else
> > +		ret = bq24261_write_reg(chip->client,
> > +						BQ24261_STAT_CTRL0_ADDR,
> 0x0);
> > +
> > +	if (ret < 0)
> > +		dev_err(&chip->client->dev,
> > +			"stat cntl0 reg access error(%d)\n", ret); }
> > +
> > +static void bq24261_extcon_event_work(struct work_struct *work) {
> > +	struct bq24261_charger *chip =
> > +			container_of(work, struct bq24261_charger,
> cable.work);
> > +	int ret, current_limit = 0;
> > +	bool old_connected = chip->cable.connected;
> > +
> > +	/* Determine cable/charger type */
> > +	if (extcon_get_cable_state(chip->cable.sdp.edev,
> > +					"SLOW-CHARGER") > 0) {
> > +		chip->cable.connected = true;
> > +		current_limit = ILIM_500MA;
> > +		chip->cable.chg_type = POWER_SUPPLY_TYPE_USB;
> > +		dev_dbg(&chip->client->dev, "USB SDP charger is connected");
> > +	} else if (extcon_get_cable_state(chip->cable.cdp.edev,
> > +					"CHARGE-DOWNSTREAM") > 0) {
> > +		chip->cable.connected = true;
> > +		current_limit = ILIM_1500MA;
> > +		chip->cable.chg_type = POWER_SUPPLY_TYPE_USB_CDP;
> > +		dev_dbg(&chip->client->dev, "USB CDP charger is connected");
> > +	} else if (extcon_get_cable_state(chip->cable.dcp.edev,
> > +					"FAST-CHARGER") > 0) {
> > +		chip->cable.connected = true;
> > +		current_limit = ILIM_1500MA;
> > +		chip->cable.chg_type = POWER_SUPPLY_TYPE_USB_DCP;
> > +		dev_dbg(&chip->client->dev, "USB DCP charger is connected");
> > +	} else if (extcon_get_cable_state(chip->cable.otg.edev,
> > +					"USB-Host") > 0) {
> > +		chip->cable.boost = true;
> > +		chip->cable.connected = true;
> > +		dev_dbg(&chip->client->dev, "USB-Host cable is connected");
> > +	} else {
> > +		if (old_connected)
> > +			dev_dbg(&chip->client->dev, "USB Cable
> disconnected");
> > +		chip->cable.connected = false;
> > +		chip->cable.boost = false;
> > +		chip->cable.chg_type = POWER_SUPPLY_TYPE_USB;
> > +	}
> > +
> > +	/* Cable status changed */
> > +	if (old_connected == chip->cable.connected)
> > +		return;
> > +
> > +	mutex_lock(&chip->lock);
> > +	if (chip->cable.connected && !chip->cable.boost) {
> > +		chip->inlmt = current_limit;
> > +		/* Set up charging */
> > +		ret = bq24261_set_cc(chip, chip->cc);
> > +		if (ret < 0)
> > +			dev_err(&chip->client->dev, "set CC failed(%d)", ret);
> > +		ret = bq24261_set_cv(chip, chip->cv);
> > +		if (ret < 0)
> > +			dev_err(&chip->client->dev, "set CV failed(%d)", ret);
> > +		ret = bq24261_set_inlmt(chip, chip->inlmt);
> > +		if (ret < 0)
> > +			dev_err(&chip->client->dev, "set ILIM failed(%d)", ret);
> > +		ret = bq24261_enable_charger(chip, true);
> > +		if (ret < 0)
> > +			dev_err(&chip->client->dev,
> > +					"enable charger failed(%d)", ret);
> > +		ret = bq24261_enable_charging(chip, true);
> > +		if (ret < 0)
> > +			dev_err(&chip->client->dev,
> > +					"enable charging failed(%d)", ret);
> > +
> > +		chip->is_charging_enabled = true;
> > +		chip->present = true;
> > +		chip->online = true;
> > +		schedule_delayed_work(&chip->wdt_work, 0);
> > +	} else if (chip->cable.connected && chip->cable.boost) {
> > +		/* Enable VBUS for Host Mode */
> > +		bq24261_boost_control(chip, true);
> > +		schedule_delayed_work(&chip->wdt_work, 0);
> > +	} else {
> > +		dev_info(&chip->client->dev, "Cable disconnect event\n");
> > +		cancel_delayed_work_sync(&chip->wdt_work);
> > +		cancel_delayed_work_sync(&chip->fault_mon_work);
> > +		bq24261_boost_control(chip, false);
> > +		ret = bq24261_enable_charging(chip, false);
> > +		if (ret < 0)
> > +			dev_err(&chip->client->dev,
> > +					"charger disable failed(%d)", ret);
> > +
> > +		chip->is_charging_enabled = false;
> > +		chip->present = false;
> > +		chip->online = false;
> > +		chip->inlmt = 0;
> > +	}
> > +	bq24261_charger_desc.type = chip->cable.chg_type;
> > +	mutex_unlock(&chip->lock);
> > +	power_supply_changed(chip->psy_usb);
> > +}
> > +
> > +static int bq24261_handle_extcon_events(struct notifier_block *nb,
> > +				   unsigned long event, void *param) {
> > +	struct bq24261_charger *chip =
> > +		container_of(nb, struct bq24261_charger, cable.nb);
> > +
> > +	dev_dbg(&chip->client->dev, "external connector event(%ld)\n",
> > +event);
> > +
> > +	schedule_work(&chip->cable.work);
> > +	return NOTIFY_OK;
> > +}
> > +
> > +static int bq24261_extcon_register(struct bq24261_charger *chip) {
> > +	int ret;
> > +
> > +	INIT_WORK(&chip->cable.work, bq24261_extcon_event_work);
> > +	chip->cable.nb.notifier_call = bq24261_handle_extcon_events;
> > +
> > +	ret = extcon_register_interest(&chip->cable.sdp, NULL,
> > +				"SLOW-CHARGER", &chip->cable.nb);
> > +	if (ret < 0) {
> > +		dev_warn(&chip->client->dev,
> > +				"extcon SDP registration failed(%d)\n", ret);
> > +		goto sdp_reg_failed;
> > +	}
> > +
> > +	ret = extcon_register_interest(&chip->cable.cdp, NULL,
> > +				"CHARGE-DOWNSTREAM", &chip->cable.nb);
> > +	if (ret < 0) {
> > +		dev_warn(&chip->client->dev,
> > +				"extcon CDP registration failed(%d)\n", ret);
> > +		goto cdp_reg_failed;
> > +	}
> > +
> > +	ret = extcon_register_interest(&chip->cable.dcp, NULL,
> > +				"FAST-CHARGER", &chip->cable.nb);
> > +	if (ret < 0) {
> > +		dev_warn(&chip->client->dev,
> > +				"extcon DCP registration failed(%d)\n", ret);
> > +		goto dcp_reg_failed;
> > +	}
> > +
> > +	ret = extcon_register_interest(&chip->cable.otg, NULL,
> > +				"USB-Host", &chip->cable.nb);
> > +	if (ret < 0) {
> > +		dev_warn(&chip->client->dev,
> > +			"extcon USB-Host registration failed(%d)\n", ret);
> > +		goto otg_reg_failed;
> > +	}
> > +
> > +	return 0;
> > +
> > +otg_reg_failed:
> > +	extcon_unregister_interest(&chip->cable.dcp);
> > +dcp_reg_failed:
> > +	extcon_unregister_interest(&chip->cable.cdp);
> > +cdp_reg_failed:
> > +	extcon_unregister_interest(&chip->cable.sdp);
> > +sdp_reg_failed:
> > +	return -EPROBE_DEFER;
> > +}
> > +
> > +static void bq24261_of_pdata(struct bq24261_charger *chip) {
> > +	static struct bq24261_platform_data pdata;
> > +	struct device *dev = &chip->client->dev;
> > +	int ret;
> > +
> > +	ret = device_property_read_u32(dev,
> > +				"ti,charge-current", &pdata.def_cc);
> > +	if (ret < 0)
> > +		goto of_err;
> > +	ret = device_property_read_u32(dev,
> > +				"ti,charge-voltage", &pdata.def_cv);
> > +	if (ret < 0)
> > +		goto of_err;
> > +	ret = device_property_read_u32(dev,
> > +				"ti,termination-current", &pdata.iterm);
> > +	if (ret < 0)
> > +		goto of_err;
> > +	ret = device_property_read_u32(dev,
> > +				"ti,max-charge-current", &pdata.max_cc);
> > +	if (ret < 0)
> > +		goto of_err;
> > +	ret = device_property_read_u32(dev,
> > +				"ti,max-charge-voltage", &pdata.max_cv);
> > +	if (ret < 0)
> > +		goto of_err;
> > +	ret = device_property_read_u32(dev,
> > +				"ti,min-charge-temperature",
> &pdata.min_temp);
> > +	if (ret < 0)
> > +		goto of_err;
> > +	ret = device_property_read_u32(dev,
> > +				"ti,max-charge-temperature",
> &pdata.max_temp);
> > +	if (ret < 0)
> > +		goto of_err;
> > +
> > +	pdata.thermal_sensing = device_property_read_bool(dev,
> > +						"ti,thermal-sensing");
> > +	pdata.en_user_write = device_property_read_bool(dev,
> > +						"ti,enable-user-write");
> > +
> > +	chip->pdata = &pdata;
> > +	return;
> > +of_err:
> > +	dev_err(dev, "error in getting DT property(%d)\n", ret); }
> > +
> > +static int bq24261_get_model(struct i2c_client *client,
> > +			enum bq2426x_model *model)
> > +{
> > +	int ret;
> > +
> > +	ret = bq24261_read_reg(client, BQ24261_VENDOR_REV_ADDR);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if ((ret & BQ24261_VENDOR_MASK) != VENDOR_BQ2426X)
> > +		return -EINVAL;
> > +
> > +	switch (ret & BQ24261_REV_MASK) {
> > +	case REV_BQ24261:
> 
> Where do you get the information that a value of 0x6 corresponds to the
> BQ24261 and not another device? When looking at the publicly available
> datasheet these revision bits are not documented, so technically we can't use
> them.
> http://www.ti.com/product/BQ24261/datasheet/detailed_description#SLUSBU
> 41233
> 
> It might be better to have the user specify which model of device to use, like
> done here (shameless plug)
> http://marc.info/?l=linux-pm&m=144175761831319

I actually the read the register from my board :-). But if it is not the right method then I will leave it you.


> 
> > +		*model = BQ24261;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int bq24261_probe(struct i2c_client *client,
> > +			 const struct i2c_device_id *id)
> > +{
> > +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> > +	struct power_supply_config charger_cfg = {};
> > +	struct bq24261_charger *chip;
> > +	int ret;
> > +	enum bq2426x_model model;
> > +
> > +	adapter = to_i2c_adapter(client->dev.parent);
> > +
> > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > +		dev_err(&client->dev,
> > +			"I2C adapter %s doesn'tsupport BYTE DATA transfer\n",
> > +			adapter->name);
> > +		return -EIO;
> > +	}
> > +
> > +	ret = bq24261_get_model(client, &model);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "chip detection error (%d)\n", ret);
> > +		return -ENODEV;
> > +	}
> > +
> > +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> > +	if (!chip)
> > +		return -ENOMEM;
> > +
> > +	chip->client = client;
> > +	if (client->dev.platform_data)
> > +		chip->pdata = client->dev.platform_data;
> > +	else if (id->driver_data)
> > +		chip->pdata = (struct bq24261_platform_data *)id->driver_data;
> 
> What do the above two lines do? I understand that the statement above it is for
> using platform data, and the statement below is for getting the initial
> parameters from the device tree. But using/casting
> id->driver_data as platform data?
We can use the id's driver data strcut/ptr to pass the platform data as well.

> 
> > +	else
> > +		bq24261_of_pdata(chip);
> > +
> > +	if (!chip->pdata) {
> > +		dev_err(&client->dev, "platform data not found");
> > +		return -ENODEV;
> > +	}
> > +
> > +	i2c_set_clientdata(client, chip);
> > +	mutex_init(&chip->lock);
> > +	chip->model = model;
> > +
> > +	/* Initialize charger parameters */
> > +	chip->cc = chip->pdata->def_cc;
> > +	chip->cv = chip->pdata->def_cv;
> > +	chip->iterm = chip->pdata->iterm;
> > +	chip->chg_status = BQ24261_CHRGR_STAT_UNKNOWN;
> > +	chip->chg_health = POWER_SUPPLY_HEALTH_UNKNOWN;
> > +
> > +	charger_cfg.drv_data = chip;
> > +	charger_cfg.supplied_to = bq24261_charger_supplied_to;
> > +	charger_cfg.num_supplicants =
> ARRAY_SIZE(bq24261_charger_supplied_to);
> > +	if (chip->pdata->en_user_write) {
> > +		bq24261_charger_desc.set_property =
> bq24261_usb_set_property;
> > +		bq24261_charger_desc.property_is_writeable =
> > +
> 	bq24261_property_is_writeable;
> > +	}
> > +	chip->psy_usb = power_supply_register(&client->dev,
> > +				&bq24261_charger_desc, &charger_cfg);
> > +	if (IS_ERR(chip->psy_usb)) {
> > +		dev_err(&client->dev,
> > +			"power supply registration failed(%d)\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	INIT_DELAYED_WORK(&chip->wdt_work, bq24261_wdt_reset_worker);
> > +	INIT_DELAYED_WORK(&chip->fault_mon_work,
> bq24261_fault_mon_work);
> > +
> > +	ret = bq24261_extcon_register(chip);
> > +	if (ret < 0)
> > +		goto extcon_reg_failed;
> > +
> > +	if (chip->client->irq) {
> > +		ret = request_threaded_irq(chip->client->irq,
> > +					   NULL, bq24261_thread_handler,
> > +					   IRQF_SHARED | IRQF_NO_SUSPEND,
> > +					   DEV_NAME, chip);
> 
> Should use the managed version of requesting an IRQ
> (devm_request_threaded_irq), this way it doesn't need to get freed at the end.
Ok.

> 
> > +		if (ret) {
> > +			dev_err(&client->dev,
> > +				"irq request failed (%d)\n", ret);
> > +			goto irq_reg_failed;
> > +		}
> > +		INIT_WORK(&chip->irq_work, bq24261_irq_worker);
> > +	}
> > +
> > +	/* Check for charger connecetd boot case */
> 
> Spelling (connected).
Ok.

> 
> > +	schedule_work(&chip->cable.work);
> > +
> > +	return 0;
> > +
> > +irq_reg_failed:
> > +	extcon_unregister_interest(&chip->cable.sdp);
> > +	extcon_unregister_interest(&chip->cable.cdp);
> > +	extcon_unregister_interest(&chip->cable.dcp);
> > +	extcon_unregister_interest(&chip->cable.otg);
> > +extcon_reg_failed:
> > +	power_supply_unregister(chip->psy_usb);
> > +	return ret;
> > +}
> > +
> > +static int bq24261_remove(struct i2c_client *client) {
> > +	struct bq24261_charger *chip = i2c_get_clientdata(client);
> > +
> > +	free_irq(client->irq, chip);
> > +	flush_scheduled_work();
> > +	extcon_unregister_interest(&chip->cable.sdp);
> > +	extcon_unregister_interest(&chip->cable.cdp);
> > +	extcon_unregister_interest(&chip->cable.dcp);
> > +	extcon_unregister_interest(&chip->cable.otg);
> > +	power_supply_unregister(chip->psy_usb);
> > +	return 0;
> > +}
> > +
> > +static int bq24261_suspend(struct device *dev) {
> > +	struct bq24261_charger *chip = dev_get_drvdata(dev);
> > +
> > +	dev_dbg(&chip->client->dev, "bq24261 suspend\n");
> > +	return 0;
> > +}
> > +
> > +static int bq24261_resume(struct device *dev) {
> > +	struct bq24261_charger *chip = dev_get_drvdata(dev);
> > +
> > +	dev_dbg(&chip->client->dev, "bq24261 resume\n");
> > +	return 0;
> > +}
> 
> The resume function is basically empty... Does this mean we can just remove all
> of the pm stuff as well as the entry in struct i2c_driver bq24261_driver?
Yes. I will remove the stubs.

> 
> > +
> > +static SIMPLE_DEV_PM_OPS(bq24261_pm_ops, bq24261_suspend,
> > +			bq24261_resume);
> > +
> > +static const struct i2c_device_id bq24261_id[] = {
> > +	{"bq24261", 0},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, bq24261_id);
> > +
> > +static const struct acpi_device_id bq24261_acpi_match[] = {
> > +	{"BQ24261", 0},
> 
> ACPI IDs should be 8 characters long, see
> https://lkml.org/lkml/2015/7/30/143
> 
> In analogy to the bq24257 driver Laurentiu developed I suggest simply adding a
> trailing zero (0) to the string.
Ok will fix it.

> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(acpi, bq24261_acpi_match);
> > +
> > +static const struct of_device_id bq24261_of_match[] = {
> > +	{ .compatible = "ti,bq24261", },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, bq24261_of_match);
> > +
> > +static struct i2c_driver bq24261_driver = {
> > +	.driver = {
> > +		.name = DEV_NAME,
> > +		.acpi_match_table = ACPI_PTR(bq24261_acpi_match),
> > +		.of_match_table = of_match_ptr(bq24261_of_match),
> > +		.pm = &bq24261_pm_ops,
> > +	},
> > +	.probe = bq24261_probe,
> > +	.remove = bq24261_remove,
> > +	.id_table = bq24261_id,
> > +};
> > +
> > +module_i2c_driver(bq24261_driver);
> > +
> > +MODULE_AUTHOR("Jenny TC <jenny.tc@intel.com>");
> > +MODULE_AUTHOR("Ramakrishna Pallala <ramakrishna.pallala@intel.com>");
> > +MODULE_DESCRIPTION("BQ24261 Charger Driver"); MODULE_LICENSE("GPL
> > +v2");
> > diff --git a/include/linux/power/bq24261_charger.h
> > b/include/linux/power/bq24261_charger.h
> > new file mode 100644
> > index 0000000..3ac2986
> > --- /dev/null
> > +++ b/include/linux/power/bq24261_charger.h
> > @@ -0,0 +1,27 @@
> > +/*
> > + * bq24261_charger.h: platform data structure for bq24261 driver
> > + *
> > + * Copyright (C) 2014 Intel Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; version 2
> > + * of the License.
> > + */
> > +
> > +#ifndef __BQ24261_CHARGER_H__
> > +#define __BQ24261_CHARGER_H__
> > +
> > +struct bq24261_platform_data {
> > +	int def_cc;	/* in mA */
> > +	int def_cv;	/* in mV */
> > +	int iterm;	/* in mA */
> > +	int max_cc;	/* in mA */
> > +	int max_cv;	/* in mV */
> > +	int min_temp;	/* in DegC */
> > +	int max_temp;	/* in DegC */
> > +	bool thermal_sensing;
> > +	bool en_user_write;
> > +};
> > +
> > +#endif
> > --
> > 1.7.9.5
> >

Thanks,
Ram