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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxx>
> > Signed-off-by: Jennt TC <jenny.tc@xxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxx>");
> > +MODULE_AUTHOR("Ramakrishna Pallala <ramakrishna.pallala@xxxxxxxxx>");
> > +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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux