[PATCH 0/3] max77693: USB event listener for charger

[PATCH 0/3] max77693: USB event listener for charger

[Wolfgang Wiedmeyer]

This patch series adds an USB event listener to the charger driver
that enables charging if an USB cable is connected. Before the charger
can be enabled, the maximum input current and the fast charge current
need to be set. In order to make this possible, the regulator function
that sets the current needs to be extended. 

Wolfgang Wiedmeyer (3):
  mfd: max77693: Add defines for charger current control
  regulator: max77693: Also manipulate the fast charge current
  power_supply: max77693: Listen for cable events and enable charging

 drivers/power/Kconfig                  |   2 +-
 drivers/power/max77693_charger.c       | 171 +++++++++++++++++++++++++++++++++
 drivers/regulator/max77693-regulator.c |  45 ++++++++-
 include/linux/mfd/max77693-private.h   |   7 ++
 4 files changed, 221 insertions(+), 4 deletions(-)

-- 
2.8.0.rc3

[PATCH 1/3] mfd: max77693: Add defines for charger current control

[Wolfgang Wiedmeyer]

This prepares for an updated regulator and charger driver. The defines
are needed to set the maximum input current and the fast charge
current.

Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
---
 include/linux/mfd/max77693-private.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/mfd/max77693-private.h b/include/linux/mfd/max77693-private.h
index 3c7a63b..ca18344 100644
--- a/include/linux/mfd/max77693-private.h
+++ b/include/linux/mfd/max77693-private.h
@@ -222,6 +222,9 @@ enum max77693_charger_battery_state {
 #define CHG_CNFG_00_CHG_MASK		0x1
 #define CHG_CNFG_00_BUCK_MASK		0x4
 
+/* MAX77693 CHG_CNFG_02 register */
+#define CHG_CNFG_02_CC_MASK		0x3F
+
 /* MAX77693_CHG_REG_CHG_CNFG_01 register */
 #define CHG_CNFG_01_FCHGTIME_SHIFT	0
 #define CHG_CNFG_01_CHGRSTRT_SHIFT	4
@@ -258,6 +261,10 @@ enum max77693_charger_battery_state {
 
 /* MAX77693 CHG_CNFG_09 Register */
 #define CHG_CNFG_09_CHGIN_ILIM_MASK	0x7F
+#define CHG_CNFG_09_CHGIN_ILIM_500_MAX	500000
+#define CHG_CNFG_09_CHGIN_ILIM_500_MIN	470000
+#define CHG_CNFG_09_CHGIN_ILIM_0_MAX	60000
+#define CHG_CNFG_09_CHGIN_ILIM_0_MIN	0
 
 /* MAX77693 CHG_CTRL Register */
 #define SAFEOUT_CTRL_SAFEOUT1_MASK	0x3
-- 
2.8.0.rc3

Re: [PATCH 1/3] mfd: max77693: Add defines for charger current control

[Krzysztof Kozlowski]

On Tue, Sep 27, 2016 at 01:31:08AM +0200, Wolfgang Wiedmeyer wrote:
> This prepares for an updated regulator and charger driver. The defines
> are needed to set the maximum input current and the fast charge
> current.
> 
> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>

This shouldn't be separate patch, because:
1. You are touching later power and regulator so this patch should be
applied to one and then pulled by other... a lot of unneeded work.
2. It is difficult to find which new code is needed for which driver.

Simpler approach might work - when changing the regulator or psy driver,
just change the header as needed. Unless there are conflicts and both of
them need the same?

Best regards,
Krzysztof

> ---
>  include/linux/mfd/max77693-private.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/mfd/max77693-private.h b/include/linux/mfd/max77693-private.h
> index 3c7a63b..ca18344 100644
> --- a/include/linux/mfd/max77693-private.h
> +++ b/include/linux/mfd/max77693-private.h
> @@ -222,6 +222,9 @@ enum max77693_charger_battery_state {
>  #define CHG_CNFG_00_CHG_MASK		0x1
>  #define CHG_CNFG_00_BUCK_MASK		0x4
>  
> +/* MAX77693 CHG_CNFG_02 register */
> +#define CHG_CNFG_02_CC_MASK		0x3F
> +
>  /* MAX77693_CHG_REG_CHG_CNFG_01 register */
>  #define CHG_CNFG_01_FCHGTIME_SHIFT	0
>  #define CHG_CNFG_01_CHGRSTRT_SHIFT	4
> @@ -258,6 +261,10 @@ enum max77693_charger_battery_state {
>  
>  /* MAX77693 CHG_CNFG_09 Register */
>  #define CHG_CNFG_09_CHGIN_ILIM_MASK	0x7F
> +#define CHG_CNFG_09_CHGIN_ILIM_500_MAX	500000
> +#define CHG_CNFG_09_CHGIN_ILIM_500_MIN	470000
> +#define CHG_CNFG_09_CHGIN_ILIM_0_MAX	60000
> +#define CHG_CNFG_09_CHGIN_ILIM_0_MIN	0
>  
>  /* MAX77693 CHG_CTRL Register */
>  #define SAFEOUT_CTRL_SAFEOUT1_MASK	0x3
> -- 
> 2.8.0.rc3
> 

Re: [PATCH 1/3] mfd: max77693: Add defines for charger current control

[Wolfgang Wiedmeyer]

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


Krzysztof Kozlowski writes:

> On Tue, Sep 27, 2016 at 01:31:08AM +0200, Wolfgang Wiedmeyer wrote:
>> This prepares for an updated regulator and charger driver. The defines
>> are needed to set the maximum input current and the fast charge
>> current.
>> 
>> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
>
> This shouldn't be separate patch, because:
> 1. You are touching later power and regulator so this patch should be
> applied to one and then pulled by other... a lot of unneeded work.
> 2. It is difficult to find which new code is needed for which driver.
>
> Simpler approach might work - when changing the regulator or psy driver,
> just change the header as needed. Unless there are conflicts and both of
> them need the same?

No, no conflicts. I will change the header as needed and won't do a
separate patch.

Thanks,
Wolfgang

> Best regards,
> Krzysztof
>
>> ---
>>  include/linux/mfd/max77693-private.h | 7 +++++++
>>  1 file changed, 7 insertions(+)
>> 
>> diff --git a/include/linux/mfd/max77693-private.h b/include/linux/mfd/max77693-private.h
>> index 3c7a63b..ca18344 100644
>> --- a/include/linux/mfd/max77693-private.h
>> +++ b/include/linux/mfd/max77693-private.h
>> @@ -222,6 +222,9 @@ enum max77693_charger_battery_state {
>>  #define CHG_CNFG_00_CHG_MASK		0x1
>>  #define CHG_CNFG_00_BUCK_MASK		0x4
>>  
>> +/* MAX77693 CHG_CNFG_02 register */
>> +#define CHG_CNFG_02_CC_MASK		0x3F
>> +
>>  /* MAX77693_CHG_REG_CHG_CNFG_01 register */
>>  #define CHG_CNFG_01_FCHGTIME_SHIFT	0
>>  #define CHG_CNFG_01_CHGRSTRT_SHIFT	4
>> @@ -258,6 +261,10 @@ enum max77693_charger_battery_state {
>>  
>>  /* MAX77693 CHG_CNFG_09 Register */
>>  #define CHG_CNFG_09_CHGIN_ILIM_MASK	0x7F
>> +#define CHG_CNFG_09_CHGIN_ILIM_500_MAX	500000
>> +#define CHG_CNFG_09_CHGIN_ILIM_500_MIN	470000
>> +#define CHG_CNFG_09_CHGIN_ILIM_0_MAX	60000
>> +#define CHG_CNFG_09_CHGIN_ILIM_0_MIN	0
>>  
>>  /* MAX77693 CHG_CTRL Register */
>>  #define SAFEOUT_CTRL_SAFEOUT1_MASK	0x3
>> -- 
>> 2.8.0.rc3
>> 


-- 
Website: https://fossencdi.org
OpenPGP: 0F30 D1A0 2F73 F70A 6FEE  048E 5816 A24C 1075 7FC4
Key download: https://wiedmeyer.de/keys/ww.asc

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[PATCH 2/3] regulator: max77693: Also manipulate the fast charge current

[Wolfgang Wiedmeyer]

For MAX77693, the fast charge current also needs to be manipulated for
proper charging. The fast charge current is only set in the case of
the MAX77693 type, as the MAX77843 properly manipulates the fast
charge current.
The fast charge current is set to the next possible value below the
maximum input current.

Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
---
 drivers/regulator/max77693-regulator.c | 45 +++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/max77693-regulator.c b/drivers/regulator/max77693-regulator.c
index cfbb951..e2f7584 100644
--- a/drivers/regulator/max77693-regulator.c
+++ b/drivers/regulator/max77693-regulator.c
@@ -54,14 +54,19 @@ struct chg_reg_data {
 	unsigned int linear_mask;
 	unsigned int uA_step;
 	unsigned int min_sel;
+
+	bool set_fast;
+	unsigned int fast_reg;
+	unsigned int fast_mask;
 };
 
 /*
  * MAX77693 CHARGER regulator - Min : 20mA, Max : 2580mA, step : 20mA
  * 0x00, 0x01, 0x2, 0x03	= 60 mA
  * 0x04 ~ 0x7E			= (60 + (X - 3) * 20) mA
- * Actually for MAX77693 the driver manipulates the maximum input current,
- * not the fast charge current (output). This should be fixed.
+ * Actually for MAX77693 the driver manipulates the maximum input current
+ * and the fast charge current (output) because the fast charge current
+ * is not set.
  *
  * On MAX77843 the calculation formula is the same (except values).
  * Fortunately it properly manipulates the fast charge current.
@@ -100,6 +105,8 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev,
 	const struct chg_reg_data *reg_data = rdev_get_drvdata(rdev);
 	unsigned int chg_min_uA = rdev->constraints->min_uA;
 	int sel = 0;
+	unsigned int data;
+	int ret;
 
 	while (chg_min_uA + reg_data->uA_step * sel < min_uA)
 		sel++;
@@ -110,7 +117,35 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev,
 	/* the first four codes for charger current are all 60mA */
 	sel += reg_data->min_sel;
 
-	return regmap_write(rdev->regmap, reg_data->linear_reg, sel);
+	ret = regmap_write(rdev->regmap, reg_data->linear_reg, sel);
+	if (ret < 0)
+		return ret;
+
+	if (reg_data->set_fast) {
+		/* disable fast charge if minimum value */
+		if (sel == reg_data->min_sel)
+			data = 0;
+		else {
+			/*
+			 * set the fast charge current to the closest value
+			 * below the input current
+			 */
+			ret = regmap_read(rdev->regmap, reg_data->fast_reg,
+					  &data);
+			if (ret < 0)
+				return ret;
+
+			sel *= reg_data->uA_step / 1000; /* convert to mA */
+			data &= ~reg_data->fast_mask;
+			data |= sel * 10 / 333; /* 0.1A/3 steps */
+		}
+
+		ret = regmap_write(rdev->regmap, reg_data->fast_reg, data);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
 }
 /* end of CHARGER regulator ops */
 
@@ -197,6 +232,9 @@ static const struct chg_reg_data max77693_chg_reg_data = {
 	.linear_mask	= CHG_CNFG_09_CHGIN_ILIM_MASK,
 	.uA_step	= 20000,
 	.min_sel	= 3,
+	.set_fast	= true,
+	.fast_reg	= MAX77693_CHG_REG_CHG_CNFG_02,
+	.fast_mask	= CHG_CNFG_02_CC_MASK,
 };
 
 #define	max77843_regulator_desc_esafeout(num)	{			\
@@ -237,6 +275,7 @@ static const struct chg_reg_data max77843_chg_reg_data = {
 	.linear_mask	= MAX77843_CHG_FAST_CHG_CURRENT_MASK,
 	.uA_step	= MAX77843_CHG_FAST_CHG_CURRENT_STEP,
 	.min_sel	= 2,
+	.set_fast	= false,
 };
 
 static int max77693_pmic_probe(struct platform_device *pdev)
-- 
2.8.0.rc3

Re: [PATCH 2/3] regulator: max77693: Also manipulate the fast charge current

[Krzysztof Kozlowski]

On Tue, Sep 27, 2016 at 01:31:09AM +0200, Wolfgang Wiedmeyer wrote:
> For MAX77693, the fast charge current also needs to be manipulated for
> proper charging. The fast charge current is only set in the case of
> the MAX77693 type, as the MAX77843 properly manipulates the fast
> charge current.

Are you sure it has to be manipulated? Some time I didn't dig into this.
Now I looked at the datasheet and it says that usually there is no need
for changing the charge current during the operation. Maxim recommends
to setting it to a maximum safe value for the battery. The device will
manage charge current on its own.

However I agree that the charge current should be set... maybe once, to
a maximum value appropriate for battery. Probably this should be done
by max77693_charger in max77693_dt_init().

Best regards,
Krzysztof


> The fast charge current is set to the next possible value below the
> maximum input current.


> 
> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
> ---
>  drivers/regulator/max77693-regulator.c | 45 +++++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/regulator/max77693-regulator.c b/drivers/regulator/max77693-regulator.c
> index cfbb951..e2f7584 100644
> --- a/drivers/regulator/max77693-regulator.c
> +++ b/drivers/regulator/max77693-regulator.c
> @@ -54,14 +54,19 @@ struct chg_reg_data {
>  	unsigned int linear_mask;
>  	unsigned int uA_step;
>  	unsigned int min_sel;
> +
> +	bool set_fast;
> +	unsigned int fast_reg;
> +	unsigned int fast_mask;
>  };
>  
>  /*
>   * MAX77693 CHARGER regulator - Min : 20mA, Max : 2580mA, step : 20mA
>   * 0x00, 0x01, 0x2, 0x03	= 60 mA
>   * 0x04 ~ 0x7E			= (60 + (X - 3) * 20) mA
> - * Actually for MAX77693 the driver manipulates the maximum input current,
> - * not the fast charge current (output). This should be fixed.
> + * Actually for MAX77693 the driver manipulates the maximum input current
> + * and the fast charge current (output) because the fast charge current
> + * is not set.
>   *
>   * On MAX77843 the calculation formula is the same (except values).
>   * Fortunately it properly manipulates the fast charge current.
> @@ -100,6 +105,8 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev,
>  	const struct chg_reg_data *reg_data = rdev_get_drvdata(rdev);
>  	unsigned int chg_min_uA = rdev->constraints->min_uA;
>  	int sel = 0;
> +	unsigned int data;
> +	int ret;
>  
>  	while (chg_min_uA + reg_data->uA_step * sel < min_uA)
>  		sel++;
> @@ -110,7 +117,35 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev,
>  	/* the first four codes for charger current are all 60mA */
>  	sel += reg_data->min_sel;
>  
> -	return regmap_write(rdev->regmap, reg_data->linear_reg, sel);
> +	ret = regmap_write(rdev->regmap, reg_data->linear_reg, sel);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (reg_data->set_fast) {
> +		/* disable fast charge if minimum value */
> +		if (sel == reg_data->min_sel)
> +			data = 0;
> +		else {
> +			/*
> +			 * set the fast charge current to the closest value
> +			 * below the input current
> +			 */
> +			ret = regmap_read(rdev->regmap, reg_data->fast_reg,
> +					  &data);
> +			if (ret < 0)
> +				return ret;
> +
> +			sel *= reg_data->uA_step / 1000; /* convert to mA */
> +			data &= ~reg_data->fast_mask;
> +			data |= sel * 10 / 333; /* 0.1A/3 steps */
> +		}
> +
> +		ret = regmap_write(rdev->regmap, reg_data->fast_reg, data);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
>  }
>  /* end of CHARGER regulator ops */
>  
> @@ -197,6 +232,9 @@ static const struct chg_reg_data max77693_chg_reg_data = {
>  	.linear_mask	= CHG_CNFG_09_CHGIN_ILIM_MASK,
>  	.uA_step	= 20000,
>  	.min_sel	= 3,
> +	.set_fast	= true,
> +	.fast_reg	= MAX77693_CHG_REG_CHG_CNFG_02,
> +	.fast_mask	= CHG_CNFG_02_CC_MASK,
>  };
>  
>  #define	max77843_regulator_desc_esafeout(num)	{			\
> @@ -237,6 +275,7 @@ static const struct chg_reg_data max77843_chg_reg_data = {
>  	.linear_mask	= MAX77843_CHG_FAST_CHG_CURRENT_MASK,
>  	.uA_step	= MAX77843_CHG_FAST_CHG_CURRENT_STEP,
>  	.min_sel	= 2,
> +	.set_fast	= false,
>  };
>  
>  static int max77693_pmic_probe(struct platform_device *pdev)
> -- 
> 2.8.0.rc3
> 

Re: [PATCH 2/3] regulator: max77693: Also manipulate the fast charge current

[Wolfgang Wiedmeyer]

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


Krzysztof Kozlowski writes:

> On Tue, Sep 27, 2016 at 01:31:09AM +0200, Wolfgang Wiedmeyer wrote:
>> For MAX77693, the fast charge current also needs to be manipulated for
>> proper charging. The fast charge current is only set in the case of
>> the MAX77693 type, as the MAX77843 properly manipulates the fast
>> charge current.
>
> Are you sure it has to be manipulated? Some time I didn't dig into this.
> Now I looked at the datasheet and it says that usually there is no need
> for changing the charge current during the operation. Maxim recommends
> to setting it to a maximum safe value for the battery. The device will
> manage charge current on its own.
>
> However I agree that the charge current should be set... maybe once, to
> a maximum value appropriate for battery. Probably this should be done
> by max77693_charger in max77693_dt_init().

When charging is disabled (e.g. by removing the USB cable) the charge
current is not reset to zero. So if I expose the current by the
CURRENT_NOW property, it incorrectly reports the current that was set
when charging was enabled, although there is no charging going on
anymore. So I felt the need to update the charge current every time the
charger gets enabled or disabled.
Initially, the charge current is set to zero, so I think it needs to be
set at least at the beginning to enable charging.

Thanks,
Wolfgang

> Best regards,
> Krzysztof
>
>
>> The fast charge current is set to the next possible value below the
>> maximum input current.
>
>
>> 
>> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
>> ---
>>  drivers/regulator/max77693-regulator.c | 45 +++++++++++++++++++++++++++++++---
>>  1 file changed, 42 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/regulator/max77693-regulator.c b/drivers/regulator/max77693-regulator.c
>> index cfbb951..e2f7584 100644
>> --- a/drivers/regulator/max77693-regulator.c
>> +++ b/drivers/regulator/max77693-regulator.c
>> @@ -54,14 +54,19 @@ struct chg_reg_data {
>>  	unsigned int linear_mask;
>>  	unsigned int uA_step;
>>  	unsigned int min_sel;
>> +
>> +	bool set_fast;
>> +	unsigned int fast_reg;
>> +	unsigned int fast_mask;
>>  };
>>  
>>  /*
>>   * MAX77693 CHARGER regulator - Min : 20mA, Max : 2580mA, step : 20mA
>>   * 0x00, 0x01, 0x2, 0x03	= 60 mA
>>   * 0x04 ~ 0x7E			= (60 + (X - 3) * 20) mA
>> - * Actually for MAX77693 the driver manipulates the maximum input current,
>> - * not the fast charge current (output). This should be fixed.
>> + * Actually for MAX77693 the driver manipulates the maximum input current
>> + * and the fast charge current (output) because the fast charge current
>> + * is not set.
>>   *
>>   * On MAX77843 the calculation formula is the same (except values).
>>   * Fortunately it properly manipulates the fast charge current.
>> @@ -100,6 +105,8 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev,
>>  	const struct chg_reg_data *reg_data = rdev_get_drvdata(rdev);
>>  	unsigned int chg_min_uA = rdev->constraints->min_uA;
>>  	int sel = 0;
>> +	unsigned int data;
>> +	int ret;
>>  
>>  	while (chg_min_uA + reg_data->uA_step * sel < min_uA)
>>  		sel++;
>> @@ -110,7 +117,35 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev,
>>  	/* the first four codes for charger current are all 60mA */
>>  	sel += reg_data->min_sel;
>>  
>> -	return regmap_write(rdev->regmap, reg_data->linear_reg, sel);
>> +	ret = regmap_write(rdev->regmap, reg_data->linear_reg, sel);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (reg_data->set_fast) {
>> +		/* disable fast charge if minimum value */
>> +		if (sel == reg_data->min_sel)
>> +			data = 0;
>> +		else {
>> +			/*
>> +			 * set the fast charge current to the closest value
>> +			 * below the input current
>> +			 */
>> +			ret = regmap_read(rdev->regmap, reg_data->fast_reg,
>> +					  &data);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			sel *= reg_data->uA_step / 1000; /* convert to mA */
>> +			data &= ~reg_data->fast_mask;
>> +			data |= sel * 10 / 333; /* 0.1A/3 steps */
>> +		}
>> +
>> +		ret = regmap_write(rdev->regmap, reg_data->fast_reg, data);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>>  }
>>  /* end of CHARGER regulator ops */
>>  
>> @@ -197,6 +232,9 @@ static const struct chg_reg_data max77693_chg_reg_data = {
>>  	.linear_mask	= CHG_CNFG_09_CHGIN_ILIM_MASK,
>>  	.uA_step	= 20000,
>>  	.min_sel	= 3,
>> +	.set_fast	= true,
>> +	.fast_reg	= MAX77693_CHG_REG_CHG_CNFG_02,
>> +	.fast_mask	= CHG_CNFG_02_CC_MASK,
>>  };
>>  
>>  #define	max77843_regulator_desc_esafeout(num)	{			\
>> @@ -237,6 +275,7 @@ static const struct chg_reg_data max77843_chg_reg_data = {
>>  	.linear_mask	= MAX77843_CHG_FAST_CHG_CURRENT_MASK,
>>  	.uA_step	= MAX77843_CHG_FAST_CHG_CURRENT_STEP,
>>  	.min_sel	= 2,
>> +	.set_fast	= false,
>>  };
>>  
>>  static int max77693_pmic_probe(struct platform_device *pdev)
>> -- 
>> 2.8.0.rc3
>> 


-- 
Website: https://fossencdi.org
OpenPGP: 0F30 D1A0 2F73 F70A 6FEE  048E 5816 A24C 1075 7FC4
Key download: https://wiedmeyer.de/keys/ww.asc

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/3] regulator: max77693: Also manipulate the fast charge current

[Mark Brown]

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

On Tue, Sep 27, 2016 at 03:50:42PM +0200, Wolfgang Wiedmeyer wrote:

> When charging is disabled (e.g. by removing the USB cable) the charge
> current is not reset to zero. So if I expose the current by the
> CURRENT_NOW property, it incorrectly reports the current that was set
> when charging was enabled, although there is no charging going on
> anymore. So I felt the need to update the charge current every time the
> charger gets enabled or disabled.
> Initially, the charge current is set to zero, so I think it needs to be
> set at least at the beginning to enable charging.

Are you sure that the register value you're looking at is the actual
charge current right now and not just the maximum that the charger will
try to use depending on the conditions (supply available, battery
state...)?  It seems like you're acting as though it's the latter but
that's not what the chip is doing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH 2/3] regulator: max77693: Also manipulate the fast charge current

[Wolfgang Wiedmeyer]

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


Mark Brown writes:

> On Tue, Sep 27, 2016 at 03:50:42PM +0200, Wolfgang Wiedmeyer wrote:
>
>> When charging is disabled (e.g. by removing the USB cable) the charge
>> current is not reset to zero. So if I expose the current by the
>> CURRENT_NOW property, it incorrectly reports the current that was set
>> when charging was enabled, although there is no charging going on
>> anymore. So I felt the need to update the charge current every time the
>> charger gets enabled or disabled.
>> Initially, the charge current is set to zero, so I think it needs to be
>> set at least at the beginning to enable charging.
>
> Are you sure that the register value you're looking at is the actual
> charge current right now and not just the maximum that the charger will
> try to use depending on the conditions (supply available, battery
> state...)?  It seems like you're acting as though it's the latter but
> that's not what the chip is doing.

I was looking at the vendor code that was released for the Galaxy S3 and
there the same register gets accessed for getting the current for
the CURRENT_NOW property [1] and for setting the current [2]. So is this
probably the wrong use of the CURRENT_NOW property because not the
actual charge current is read but the maximum value that was
set? Unfortunately, I don't have access to the datasheet and I didn't
find it online so I don't know where the actual current can be
accessed.

Thanks,
Wolfgang

[1] https://code.fossencdi.org/kernel_samsung_smdk4412.git/tree/drivers/battery/max77693_charger.c#n531

[2] https://code.fossencdi.org/kernel_samsung_smdk4412.git/tree/drivers/battery/max77693_charger.c#n552

-- 
Website: https://fossencdi.org
OpenPGP: 0F30 D1A0 2F73 F70A 6FEE  048E 5816 A24C 1075 7FC4
Key download: https://wiedmeyer.de/keys/ww.asc

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/3] regulator: max77693: Also manipulate the fast charge current

[Krzysztof Kozlowski]

On 09/27/2016 07:51 PM, Wolfgang Wiedmeyer wrote:
> 
> Mark Brown writes:
> 
>> On Tue, Sep 27, 2016 at 03:50:42PM +0200, Wolfgang Wiedmeyer wrote:
>>
>>> When charging is disabled (e.g. by removing the USB cable) the charge
>>> current is not reset to zero. So if I expose the current by the
>>> CURRENT_NOW property, it incorrectly reports the current that was set
>>> when charging was enabled, although there is no charging going on
>>> anymore. So I felt the need to update the charge current every time the
>>> charger gets enabled or disabled.
>>> Initially, the charge current is set to zero, so I think it needs to be
>>> set at least at the beginning to enable charging.
>>
>> Are you sure that the register value you're looking at is the actual
>> charge current right now and not just the maximum that the charger will
>> try to use depending on the conditions (supply available, battery
>> state...)?  It seems like you're acting as though it's the latter but
>> that's not what the chip is doing.
> 
> I was looking at the vendor code that was released for the Galaxy S3 and
> there the same register gets accessed for getting the current for
> the CURRENT_NOW property [1] and for setting the current [2]. So is this
> probably the wrong use of the CURRENT_NOW property because not the
> actual charge current is read but the maximum value that was
> set?

Yes, reading from this register will give only information about
currently set charge current. Not the real current.

Best regards,
Krzysztof

[PATCH 3/3] power_supply: max77693: Listen for cable events and enable charging

[Wolfgang Wiedmeyer]

This patch adds a listener for extcon cable events and enables
charging if an USB cable is connected. It recognizes SDP and DCP cable
types and treats them the same (same input current and fast charge
current). The maximum input current is set before the charger is
enabled and before the charger gets disabled, the maximum input
current is set to zero. The listener is inspired by the listener
implementation that was used for the AXP288 Charger driver.

The patch also adds support for the CURRENT_NOW property. It reads the
fast charge current that gets set before the charger is enabled or
disabled.

Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
---
 drivers/power/Kconfig            |   2 +-
 drivers/power/max77693_charger.c | 171 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 172 insertions(+), 1 deletion(-)

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index acd4a15..28254d3 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -372,7 +372,7 @@ config CHARGER_MAX14577
 
 config CHARGER_MAX77693
 	tristate "Maxim MAX77693 battery charger driver"
-	depends on MFD_MAX77693
+	depends on MFD_MAX77693 && REGULATOR_MAX77693
 	help
 	  Say Y to enable support for the Maxim MAX77693 battery charger.
 
diff --git a/drivers/power/max77693_charger.c b/drivers/power/max77693_charger.c
index 060cab5..922f43f 100644
--- a/drivers/power/max77693_charger.c
+++ b/drivers/power/max77693_charger.c
@@ -22,8 +22,11 @@
 #include <linux/mfd/max77693.h>
 #include <linux/mfd/max77693-common.h>
 #include <linux/mfd/max77693-private.h>
+#include <linux/extcon.h>
+#include <linux/regulator/consumer.h>
 
 #define MAX77693_CHARGER_NAME				"max77693-charger"
+#define MAX77693_EXTCON_DEV_NAME			"max77693-muic"
 static const char *max77693_charger_model		= "MAX77693";
 static const char *max77693_charger_manufacturer	= "Maxim Integrated";
 
@@ -31,12 +34,21 @@ struct max77693_charger {
 	struct device		*dev;
 	struct max77693_dev	*max77693;
 	struct power_supply	*charger;
+	struct regulator	*regu;
 
 	u32 constant_volt;
 	u32 min_system_volt;
 	u32 thermal_regulation_temp;
 	u32 batttery_overcurrent;
 	u32 charge_input_threshold_volt;
+
+	/* SDP/DCP USB charging cable notifications */
+	struct {
+		struct extcon_dev *edev;
+		bool connected;
+		struct notifier_block nb;
+		struct work_struct work;
+	} cable;
 };
 
 static int max77693_get_charger_state(struct regmap *regmap, int *val)
@@ -207,12 +219,28 @@ static int max77693_get_online(struct regmap *regmap, int *val)
 	return 0;
 }
 
+int max77693_get_charge_current(struct regmap *regmap, int *val)
+{
+	unsigned int data;
+	int ret;
+
+	ret = regmap_read(regmap, MAX77693_CHG_REG_CHG_CNFG_02, &data);
+	if (ret < 0)
+		return ret;
+
+	data &= CHG_CNFG_02_CC_MASK;
+	*val = data * 333 / 10; /* 3 steps/0.1A */
+
+	return 0;
+}
+
 static enum power_supply_property max77693_charger_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_CHARGE_TYPE,
 	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
@@ -241,6 +269,9 @@ static int max77693_charger_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_ONLINE:
 		ret = max77693_get_online(regmap, &val->intval);
 		break;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		ret = max77693_get_charge_current(regmap, &val->intval);
+		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = max77693_charger_model;
 		break;
@@ -295,6 +326,7 @@ static ssize_t fast_charge_timer_show(struct device *dev,
 
 	data &= CHG_CNFG_01_FCHGTIME_MASK;
 	data >>= CHG_CNFG_01_FCHGTIME_SHIFT;
+
 	switch (data) {
 	case 0x1 ... 0x7:
 		/* Starting from 4 hours, step by 2 hours */
@@ -582,6 +614,97 @@ static int max77693_set_charge_input_threshold_volt(struct max77693_charger *chg
 			CHG_CNFG_12_VCHGINREG_MASK, data);
 }
 
+static int max77693_enable_charger(struct max77693_charger *chg, bool enable)
+{
+	int ret;
+
+	if (enable) {
+		regulator_set_current_limit(chg->regu,
+					    CHG_CNFG_09_CHGIN_ILIM_500_MIN,
+					    CHG_CNFG_09_CHGIN_ILIM_500_MAX);
+		if (ret < 0)
+			return ret;
+
+		ret = regulator_enable(chg->regu);
+		if (ret < 0)
+			return ret;
+	} else {
+		/* sets fast charge current to zero */
+		ret = regulator_set_current_limit(chg->regu,
+						  CHG_CNFG_09_CHGIN_ILIM_0_MIN,
+						  CHG_CNFG_09_CHGIN_ILIM_0_MAX);
+		if (ret < 0)
+			return ret;
+
+		ret = regulator_disable(chg->regu);
+		if (ret < 0)
+			return ret;
+	}
+
+	return ret;
+}
+
+static void max77693_extcon_evt_worker(struct work_struct *work)
+{
+	struct max77693_charger *chg =
+	    container_of(work, struct max77693_charger, cable.work);
+	bool changed = false;
+	struct extcon_dev *edev = chg->cable.edev;
+	bool old_connected = chg->cable.connected;
+	bool is_charger_enabled;
+	int ret;
+
+	/* Determine cable/charger type */
+	if (extcon_get_cable_state_(edev, EXTCON_CHG_USB_SDP) ||
+	extcon_get_cable_state_(edev, EXTCON_CHG_USB_DCP)) {
+		dev_dbg(chg->dev, "USB charger is connected");
+		chg->cable.connected = true;
+	} else {
+		if (old_connected)
+			dev_dbg(chg->dev, "USB charger disconnected");
+
+		chg->cable.connected = false;
+	}
+
+	/* Cable status changed */
+	if (old_connected != chg->cable.connected)
+		changed = true;
+
+	if (!changed)
+		return;
+
+	if (regulator_is_enabled(chg->regu))
+		is_charger_enabled = true;
+	else
+		is_charger_enabled = false;
+
+	if (is_charger_enabled && !chg->cable.connected) {
+		ret = max77693_enable_charger(chg, false);
+		if (ret < 0)
+			dev_err(chg->dev,
+				"failed to disable charger (%d)", ret);
+	} else if (!is_charger_enabled && chg->cable.connected) {
+		ret = max77693_enable_charger(chg, true);
+		if (ret < 0)
+			dev_err(chg->dev,
+				"cannot enable charger (%d)", ret);
+	}
+
+	if (changed)
+		power_supply_changed(chg->charger);
+}
+
+static int max77693_handle_cable_evt(struct notifier_block *nb,
+				unsigned long event, void *param)
+{
+	struct max77693_charger *chg =
+	    container_of(nb, struct max77693_charger, cable.nb);
+
+	schedule_work(&chg->cable.work);
+
+	return NOTIFY_OK;
+}
+
 /*
  * Sets charger registers to proper and safe default values.
  */
@@ -693,6 +816,45 @@ static int max77693_charger_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	chg->regu = devm_regulator_get(chg->dev, "CHARGER");
+	if (IS_ERR(chg->regu)) {
+		ret = PTR_ERR(chg->regu);
+		dev_err(&pdev->dev,
+			"failed to get charger regulator %d\n", ret);
+		return ret;
+	}
+
+	chg->cable.edev = extcon_get_extcon_dev(MAX77693_EXTCON_DEV_NAME);
+	if (chg->cable.edev == NULL) {
+		dev_dbg(&pdev->dev, "%s is not ready, probe deferred\n",
+			MAX77693_EXTCON_DEV_NAME);
+		return -EPROBE_DEFER;
+	}
+
+	/* set initial value */
+	chg->cable.connected = false;
+
+	/* Register for extcon notification */
+	INIT_WORK(&chg->cable.work, max77693_extcon_evt_worker);
+	chg->cable.nb.notifier_call = max77693_handle_cable_evt;
+	ret = extcon_register_notifier(chg->cable.edev, EXTCON_CHG_USB_SDP,
+					&chg->cable.nb);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to register extcon notifier for SDP %d\n", ret);
+		return ret;
+	}
+
+	ret = extcon_register_notifier(chg->cable.edev, EXTCON_CHG_USB_DCP,
+					&chg->cable.nb);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to register extcon notifier for DCP %d\n", ret);
+		extcon_unregister_notifier(chg->cable.edev,
+				EXTCON_CHG_USB_SDP, &chg->cable.nb);
+		return ret;
+	}
+
 	ret = max77693_reg_init(chg);
 	if (ret)
 		return ret;
@@ -733,6 +895,10 @@ err:
 	device_remove_file(&pdev->dev, &dev_attr_top_off_timer);
 	device_remove_file(&pdev->dev, &dev_attr_top_off_threshold_current);
 	device_remove_file(&pdev->dev, &dev_attr_fast_charge_timer);
+	extcon_unregister_notifier(chg->cable.edev, EXTCON_CHG_USB_SDP,
+				   &chg->cable.nb);
+	extcon_unregister_notifier(chg->cable.edev, EXTCON_CHG_USB_DCP,
+					&chg->cable.nb);
 
 	return ret;
 }
@@ -745,6 +911,11 @@ static int max77693_charger_remove(struct platform_device *pdev)
 	device_remove_file(&pdev->dev, &dev_attr_top_off_threshold_current);
 	device_remove_file(&pdev->dev, &dev_attr_fast_charge_timer);
 
+	extcon_unregister_notifier(chg->cable.edev, EXTCON_CHG_USB_SDP,
+					&chg->cable.nb);
+	extcon_unregister_notifier(chg->cable.edev, EXTCON_CHG_USB_DCP,
+					&chg->cable.nb);
+
 	power_supply_unregister(chg->charger);
 
 	return 0;
-- 
2.8.0.rc3

Re: [PATCH 3/3] power_supply: max77693: Listen for cable events and enable charging

[Krzysztof Kozlowski]

On Tue, Sep 27, 2016 at 01:31:10AM +0200, Wolfgang Wiedmeyer wrote:
> This patch adds a listener for extcon cable events and enables
> charging if an USB cable is connected. It recognizes SDP and DCP cable
> types and treats them the same (same input current and fast charge
> current). The maximum input current is set before the charger is
> enabled and before the charger gets disabled, the maximum input
> current is set to zero. The listener is inspired by the listener
> implementation that was used for the AXP288 Charger driver.
> 
> The patch also adds support for the CURRENT_NOW property. It reads the
> fast charge current that gets set before the charger is enabled or
> disabled.
> 
> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>

No. This power supply driver should not manage regulators. It is not a
regulator consumer. For that specific need, there is a charger-manager driver.

I agree that you might configure here the charger. You might even expose
some writeable properties through power supply class. However the
purpose of this driver is to expose the battery charger to user-space,
not to replace the user-space with its work.

So... NACK.

If you would like to play with charger-manager, here is my old DTS for
Trats2 (might need updates):

index 595ad4ba6977..b4361b4a9de7 100644
--- a/arch/arm/boot/dts/exynos4412-trats2.dts
+++ b/arch/arm/boot/dts/exynos4412-trats2.dts
@@ -856,6 +856,44 @@
                };
        };
 
+       charger-manager@0 {
+               compatible = "charger-manager";
+               status = "okay";
+               chg-reg-supply = <&charger_reg>;
+
+               cm-name = "battery";
+               /* Polling only for external power source */
+               cm-poll-mode = <2>;
+               cm-poll-interval = <30000>;
+
+               cm-fullbatt-vchkdrop-ms = <30000>;
+               cm-fullbatt-vchkdrop-volt = <150000>;
+               cm-fullbatt-soc = <100>;
+
+               cm-battery-stat = <0>;
+               cm-fuel-gauge = "max170xx_battery";
+
+               /* Allow charging for 5hr */
+               cm-charging-max = <18000000>;
+               /* Allow discharging for 2hr */
+               cm-discharging-max = <7200000>;
+
+               cm-num-chargers = <1>;
+               cm-chargers = "max77693-charger";
+
+               charger@0 {
+                       cm-regulator-name = "chg-reg";
+                       cable@0 {
+                               cm-cable-name = "USB";
+                               cm-cable-extcon = "max77693-muic";
+                       };
+                       cable@1 {
+                               cm-cable-name = "TA";
+                               cm-cable-extcon = "max77693-muic";
+                       };
+               };
+       };
+
        exynos-usbphy@125B0000 {
                status = "okay";
        };

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 3/3] power_supply: max77693: Listen for cable events and enable charging

[Wolfgang Wiedmeyer]

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


Krzysztof Kozlowski writes:

> On Tue, Sep 27, 2016 at 01:31:10AM +0200, Wolfgang Wiedmeyer wrote:
>> This patch adds a listener for extcon cable events and enables
>> charging if an USB cable is connected. It recognizes SDP and DCP cable
>> types and treats them the same (same input current and fast charge
>> current). The maximum input current is set before the charger is
>> enabled and before the charger gets disabled, the maximum input
>> current is set to zero. The listener is inspired by the listener
>> implementation that was used for the AXP288 Charger driver.
>> 
>> The patch also adds support for the CURRENT_NOW property. It reads the
>> fast charge current that gets set before the charger is enabled or
>> disabled.
>> 
>> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
>
> No. This power supply driver should not manage regulators. It is not a
> regulator consumer. For that specific need, there is a charger-manager driver.

When I was in the middle of implementing this, I noticed that the
charger manager does everything that is needed. But it took me quite
some time to configure the DTS correctly until I realized that the
charger manager used a deprecated function
(extcon_register_interest()) and thus couldn't work. And as I didn't see
the charger-manager in any other device's DTS, I thought that this might
not be right way.
But Chanwoo Choi has a fix: https://patchwork.kernel.org/patch/8898541/
So I will try to get it working with this patch.

> I agree that you might configure here the charger. You might even expose
> some writeable properties through power supply class. However the
> purpose of this driver is to expose the battery charger to user-space,
> not to replace the user-space with its work.
>
> So... NACK.

Ok, then I will try to reduce the patch to the CURRENT_NOW property
support.

> If you would like to play with charger-manager, here is my old DTS for
> Trats2 (might need updates):

Is there a reason that this patch is not in the kernel? It would have
been very helpful for me :)

Thanks,
Wolfgang

> index 595ad4ba6977..b4361b4a9de7 100644
> --- a/arch/arm/boot/dts/exynos4412-trats2.dts
> +++ b/arch/arm/boot/dts/exynos4412-trats2.dts
> @@ -856,6 +856,44 @@
>                 };
>         };
>  
> +       charger-manager@0 {
> +               compatible = "charger-manager";
> +               status = "okay";
> +               chg-reg-supply = <&charger_reg>;
> +
> +               cm-name = "battery";
> +               /* Polling only for external power source */
> +               cm-poll-mode = <2>;
> +               cm-poll-interval = <30000>;
> +
> +               cm-fullbatt-vchkdrop-ms = <30000>;
> +               cm-fullbatt-vchkdrop-volt = <150000>;
> +               cm-fullbatt-soc = <100>;
> +
> +               cm-battery-stat = <0>;
> +               cm-fuel-gauge = "max170xx_battery";
> +
> +               /* Allow charging for 5hr */
> +               cm-charging-max = <18000000>;
> +               /* Allow discharging for 2hr */
> +               cm-discharging-max = <7200000>;
> +
> +               cm-num-chargers = <1>;
> +               cm-chargers = "max77693-charger";
> +
> +               charger@0 {
> +                       cm-regulator-name = "chg-reg";
> +                       cable@0 {
> +                               cm-cable-name = "USB";
> +                               cm-cable-extcon = "max77693-muic";
> +                       };
> +                       cable@1 {
> +                               cm-cable-name = "TA";
> +                               cm-cable-extcon = "max77693-muic";
> +                       };
> +               };
> +       };
> +
>         exynos-usbphy@125B0000 {
>                 status = "okay";
>         };
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>
> Best regards,
> Krzysztof


-- 
Website: https://fossencdi.org
OpenPGP: 0F30 D1A0 2F73 F70A 6FEE  048E 5816 A24C 1075 7FC4
Key download: https://wiedmeyer.de/keys/ww.asc

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 3/3] power_supply: max77693: Listen for cable events and enable charging

[Krzysztof Kozlowski]

On 09/27/2016 03:34 PM, Wolfgang Wiedmeyer wrote:
> 
> Krzysztof Kozlowski writes:
> 
>> On Tue, Sep 27, 2016 at 01:31:10AM +0200, Wolfgang Wiedmeyer wrote:
>>> This patch adds a listener for extcon cable events and enables
>>> charging if an USB cable is connected. It recognizes SDP and DCP cable
>>> types and treats them the same (same input current and fast charge
>>> current). The maximum input current is set before the charger is
>>> enabled and before the charger gets disabled, the maximum input
>>> current is set to zero. The listener is inspired by the listener
>>> implementation that was used for the AXP288 Charger driver.
>>>
>>> The patch also adds support for the CURRENT_NOW property. It reads the
>>> fast charge current that gets set before the charger is enabled or
>>> disabled.
>>>
>>> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
>>
>> No. This power supply driver should not manage regulators. It is not a
>> regulator consumer. For that specific need, there is a charger-manager driver.
> 
> When I was in the middle of implementing this, I noticed that the
> charger manager does everything that is needed. But it took me quite
> some time to configure the DTS correctly until I realized that the
> charger manager used a deprecated function
> (extcon_register_interest()) and thus couldn't work. And as I didn't see
> the charger-manager in any other device's DTS, I thought that this might
> not be right way.
> But Chanwoo Choi has a fix: https://patchwork.kernel.org/patch/8898541/
> So I will try to get it working with this patch.
> 
>> I agree that you might configure here the charger. You might even expose
>> some writeable properties through power supply class. However the
>> purpose of this driver is to expose the battery charger to user-space,
>> not to replace the user-space with its work.
>>
>> So... NACK.
> 
> Ok, then I will try to reduce the patch to the CURRENT_NOW property
> support.
> 
>> If you would like to play with charger-manager, here is my old DTS for
>> Trats2 (might need updates):
> 
> Is there a reason that this patch is not in the kernel? It would have
> been very helpful for me :)

In general, DeviceTree should describe the hardware. Charger manager is
an abstract device, not a real one. This DT node does not describe real
hardware but it is a driver-specific glue needed to get things done.

I am not convinced that we should add to DTS such abstract devices. What
about people who want to control charging from user-space? They would
have to disable charger-manager from config.

Best regards,
Krzysztof