[PATCH 0/3] power: max17042-battery: Modify max17042 battery driver

[PATCH 0/3] power: max17042-battery: Modify max17042 battery driver

[Beomho Seo]

This patch modify max17042-battery.c fuel gauge driver for use max77693 and
max77843 fuel gauge. and then, fix minor cording style issue.

This patchset is written based on recently battery git tree.

Beomho Seo (3):
  power: max17042_battery: Use reg type instead of chip type
  power: max17042_battery: Add support for max77693/843 chip
  power: max17042_battery: add missed blank

 drivers/power/Kconfig                  |    6 +--
 drivers/power/max17042_battery.c       |   65 ++++++++++++++++++++------------
 include/linux/power/max17042_battery.h |   19 +++++++++-
 3 files changed, 62 insertions(+), 28 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] power: max17042_battery: Use reg type instead of chip type

[Beomho Seo]

Currently, max17042 battery driver choose register map by MAX17042_DevName
register. But thid register is return IC specific firmware version. So other
maxim chip hard to use this drvier. This patch choose reg_type by driver_data.

Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
---
 drivers/power/max17042_battery.c       |   55 ++++++++++++++++++--------------
 include/linux/power/max17042_battery.h |   17 +++++++++-
 2 files changed, 47 insertions(+), 25 deletions(-)

diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
index 40e7056..1db87fd 100644
--- a/drivers/power/max17042_battery.c
+++ b/drivers/power/max17042_battery.c
@@ -63,14 +63,12 @@
 #define dP_ACC_100	0x1900
 #define dP_ACC_200	0x3200
 
-#define MAX17042_IC_VERSION	0x0092
-#define MAX17047_IC_VERSION	0x00AC	/* same for max17050 */
-
 struct max17042_chip {
 	struct i2c_client *client;
 	struct regmap *regmap;
 	struct power_supply *battery;
 	enum max170xx_chip_type chip_type;
+	enum max170xx_reg_type reg_type;
 	struct max17042_platform_data *pdata;
 	struct work_struct work;
 	int    init_complete;
@@ -131,7 +129,7 @@ static int max17042_get_property(struct power_supply *psy,
 		val->intval *= 20000; /* Units of LSB = 20mV */
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
-		if (chip->chip_type == MAX17042)
+		if (chip->reg_type == MAXIM_REG_TYPE_MAX17047)
 			ret = regmap_read(map, MAX17042_V_empty, &data);
 		else
 			ret = regmap_read(map, MAX17047_V_empty, &data);
@@ -378,7 +376,7 @@ static void max17042_write_config_regs(struct max17042_chip *chip)
 	regmap_write(map, MAX17042_FilterCFG,
 			config->filter_cfg);
 	regmap_write(map, MAX17042_RelaxCFG, config->relax_cfg);
-	if (chip->chip_type == MAX17047)
+	if (chip->reg_type == MAXIM_REG_TYPE_MAX17047)
 		regmap_write(map, MAX17047_FullSOCThr,
 						config->full_soc_thresh);
 }
@@ -391,7 +389,7 @@ static void  max17042_write_custom_regs(struct max17042_chip *chip)
 	max17042_write_verify_reg(map, MAX17042_RCOMP0, config->rcomp0);
 	max17042_write_verify_reg(map, MAX17042_TempCo,	config->tcompc0);
 	max17042_write_verify_reg(map, MAX17042_ICHGTerm, config->ichgt_term);
-	if (chip->chip_type == MAX17042) {
+	if (chip->reg_type == MAXIM_REG_TYPE_MAX17042) {
 		regmap_write(map, MAX17042_EmptyTempCo,	config->empty_tempco);
 		max17042_write_verify_reg(map, MAX17042_K_empty0,
 					config->kempty0);
@@ -500,14 +498,14 @@ static inline void max17042_override_por_values(struct max17042_chip *chip)
 
 	max17042_override_por(map, MAX17042_FullCAP, config->fullcap);
 	max17042_override_por(map, MAX17042_FullCAPNom, config->fullcapnom);
-	if (chip->chip_type == MAX17042)
+	if (chip->reg_type == MAXIM_REG_TYPE_MAX17042)
 		max17042_override_por(map, MAX17042_SOC_empty,
 						config->socempty);
 	max17042_override_por(map, MAX17042_LAvg_empty, config->lavg_empty);
 	max17042_override_por(map, MAX17042_dQacc, config->dqacc);
 	max17042_override_por(map, MAX17042_dPacc, config->dpacc);
 
-	if (chip->chip_type == MAX17042)
+	if (chip->reg_type == MAXIM_REG_TYPE_MAX17042)
 		max17042_override_por(map, MAX17042_V_empty, config->vempty);
 	else
 		max17042_override_por(map, MAX17047_V_empty, config->vempty);
@@ -516,7 +514,7 @@ static inline void max17042_override_por_values(struct max17042_chip *chip)
 	max17042_override_por(map, MAX17042_FCTC, config->fctc);
 	max17042_override_por(map, MAX17042_RCOMP0, config->rcomp0);
 	max17042_override_por(map, MAX17042_TempCo, config->tcompc0);
-	if (chip->chip_type) {
+	if (chip->reg_type) {
 		max17042_override_por(map, MAX17042_EmptyTempCo,
 						config->empty_tempco);
 		max17042_override_por(map, MAX17042_K_empty0,
@@ -623,6 +621,25 @@ static void max17042_init_worker(struct work_struct *work)
 	chip->init_complete = 1;
 }
 
+static int max17042_get_reg_type(struct max17042_chip *chip)
+{
+	int reg_type;
+
+	switch (chip->chip_type) {
+	case MAXIM_DEVICE_TYPE_MAX17042:
+		reg_type = MAXIM_REG_TYPE_MAX17042;
+		break;
+	case MAXIM_DEVICE_TYPE_MAX17047:
+	case MAXIM_DEVICE_TYPE_MAX17050:
+		reg_type = MAXIM_REG_TYPE_MAX17047;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return reg_type;
+}
+
 #ifdef CONFIG_OF
 static struct max17042_platform_data *
 max17042_get_pdata(struct device *dev)
@@ -711,20 +728,10 @@ static int max17042_probe(struct i2c_client *client,
 	}
 
 	i2c_set_clientdata(client, chip);
+	chip->chip_type = id->driver_data;
+	chip->reg_type = max17042_get_reg_type(chip);
 	psy_cfg.drv_data = chip;
 
-	regmap_read(chip->regmap, MAX17042_DevName, &val);
-	if (val == MAX17042_IC_VERSION) {
-		dev_dbg(&client->dev, "chip type max17042 detected\n");
-		chip->chip_type = MAX17042;
-	} else if (val == MAX17047_IC_VERSION) {
-		dev_dbg(&client->dev, "chip type max17047/50 detected\n");
-		chip->chip_type = MAX17047;
-	} else {
-		dev_err(&client->dev, "device version mismatch: %x\n", val);
-		return -EIO;
-	}
-
 	/* When current is not measured,
 	 * CURRENT_NOW and CURRENT_AVG properties should be invisible. */
 	if (!chip->pdata->enable_current_sense)
@@ -836,9 +843,9 @@ MODULE_DEVICE_TABLE(of, max17042_dt_match);
 #endif
 
 static const struct i2c_device_id max17042_id[] = {
-	{ "max17042", 0 },
-	{ "max17047", 1 },
-	{ "max17050", 2 },
+	{ "max17042", MAXIM_DEVICE_TYPE_MAX17042 },
+	{ "max17047", MAXIM_DEVICE_TYPE_MAX17047 },
+	{ "max17050", MAXIM_DEVICE_TYPE_MAX17050 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, max17042_id);
diff --git a/include/linux/power/max17042_battery.h b/include/linux/power/max17042_battery.h
index 89dd84f..8dd7b70 100644
--- a/include/linux/power/max17042_battery.h
+++ b/include/linux/power/max17042_battery.h
@@ -126,7 +126,22 @@ enum max17047_register {
 	MAX17047_QRTbl30	= 0x42,
 };
 
-enum max170xx_chip_type {MAX17042, MAX17047};
+enum max170xx_chip_type {
+	MAXIM_DEVICE_TYPE_UNKNOWN	= 0,
+	MAXIM_DEVICE_TYPE_MAX17042,
+	MAXIM_DEVICE_TYPE_MAX17047,
+	MAXIM_DEVICE_TYPE_MAX17050,
+
+	MAXIM_DEVICE_TYPE_NUM
+};
+
+enum max170xx_reg_type {
+	MAXIM_REG_TYPE_UNKNOWN	= 0,
+	MAXIM_REG_TYPE_MAX17042,
+	MAXIM_REG_TYPE_MAX17047,
+
+	MAXIM_REG_TYPE_NUM
+};
 
 /*
  * used for setting a register to a desired value
-- 
1.7.9.5

Re: [PATCH 1/3] power: max17042_battery: Use reg type instead of chip type

[Krzysztof Kozlowski]

2015-03-31 15:29 GMT+02:00 Beomho Seo <beomho.seo@samsung.com>:
> Currently, max17042 battery driver choose register map by MAX17042_DevName
> register. But thid register is return IC specific firmware version. So other
> maxim chip hard to use this drvier. This patch choose reg_type by driver_data.

I don't quite get the concept of "reg_type" and why it replaces chip
type? It seems that you choose reg_type based on given chip type so
there is direct mapping chip_type->reg_type. If max17047 and max17050
are the same from the point of view of interface (registers) then they
should use the same compatible or the same device type. Something
like:

>  static const struct i2c_device_id max17042_id[] = {
> -       { "max17042", 0 },
> -       { "max17047", 1 },
> -       { "max17050", 2 },
> +       { "max17042", MAXIM_DEVICE_TYPE_MAX17042 },
> +       { "max17047", MAXIM_DEVICE_TYPE_MAX17047 },
> +       { "max17050", MAXIM_DEVICE_TYPE_MAX17047 }, /* Same as 17047 */
>         { }

So why you are adding the conversion from i2c_device_id -> reg_type?

Beside that, thanks for integrating this into existing driver! Much appreciated.

Best regards,
Krzysztof

Re: [PATCH 1/3] power: max17042_battery: Use reg type instead of chip type

[Beomho Seo]

On 04/01/2015 05:18 PM, Krzysztof Kozlowski wrote:
> 2015-03-31 15:29 GMT+02:00 Beomho Seo <beomho.seo@samsung.com>:
>> Currently, max17042 battery driver choose register map by MAX17042_DevName
>> register. But thid register is return IC specific firmware version. So other
>> maxim chip hard to use this drvier. This patch choose reg_type by driver_data.
> 
> I don't quite get the concept of "reg_type" and why it replaces chip
> type? It seems that you choose reg_type based on given chip type so
> there is direct mapping chip_type->reg_type. If max17047 and max17050
> are the same from the point of view of interface (registers) then they
> should use the same compatible or the same device type. Something
> like:
> 

When I check datasheet, MAX17042_DevName register return Firmware version.
Firmware version not chip type. For use other maxim chip, be better use
of_id->data or id->driver_data(be like other maxim mfd driver)

I will remove reg_type. and chip_type will be assigned through
of_id->data or id->driver_data.

>>  static const struct i2c_device_id max17042_id[] = {
>> -       { "max17042", 0 },
>> -       { "max17047", 1 },
>> -       { "max17050", 2 },
>> +       { "max17042", MAXIM_DEVICE_TYPE_MAX17042 },
>> +       { "max17047", MAXIM_DEVICE_TYPE_MAX17047 },
>> +       { "max17050", MAXIM_DEVICE_TYPE_MAX17047 }, /* Same as 17047 */
>>         { }
> 
> So why you are adding the conversion from i2c_device_id -> reg_type?
> 
> Beside that, thanks for integrating this into existing driver! Much appreciated.
> 
> Best regards,
> Krzysztof
> 

Best regards,
Beomho

[PATCH 2/3] power: max17042_battery: Add support for max77693/843 chip

[Beomho Seo]

Max77693/843 fuelgauge is similar to max17042 fuelgauge. This patch add
supports max77693/843 fuelgague use max17042_battery.c fuel gauge driver.

Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
---
 drivers/power/Kconfig                  |    6 +++---
 drivers/power/max17042_battery.c       |    8 ++++++++
 include/linux/power/max17042_battery.h |    2 ++
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 4091fb0..887f28e 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -222,7 +222,7 @@ config BATTERY_MAX17040
 	  to operate with a single lithium cell
 
 config BATTERY_MAX17042
-	tristate "Maxim MAX17042/17047/17050/8997/8966 Fuel Gauge"
+	tristate "Maxim MAX17042/17047/17050/77693/77843/8997/8966 Fuel Gauge"
 	depends on I2C
 	select REGMAP_I2C
 	help
@@ -230,8 +230,8 @@ config BATTERY_MAX17042
 	  in handheld and portable equipment. The MAX17042 is configured
 	  to operate with a single lithium cell. MAX8997 and MAX8966 are
 	  multi-function devices that include fuel gauages that are compatible
-	  with MAX17042. This driver also supports max17047/50 chips which are
-	  improved version of max17042.
+	  with MAX17042. This driver also supports max17047/50 and max77693/843
+	  chips which are improved version of max17042.
 
 config BATTERY_Z2
 	tristate "Z2 battery driver"
diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
index 1db87fd..806ac2e 100644
--- a/drivers/power/max17042_battery.c
+++ b/drivers/power/max17042_battery.c
@@ -627,6 +627,8 @@ static int max17042_get_reg_type(struct max17042_chip *chip)
 
 	switch (chip->chip_type) {
 	case MAXIM_DEVICE_TYPE_MAX17042:
+	case MAXIM_DEVICE_TYPE_MAX77693:
+	case MAXIM_DEVICE_TYPE_MAX77843:
 		reg_type = MAXIM_REG_TYPE_MAX17042;
 		break;
 	case MAXIM_DEVICE_TYPE_MAX17047:
@@ -837,6 +839,9 @@ static const struct of_device_id max17042_dt_match[] = {
 	{ .compatible = "maxim,max17042" },
 	{ .compatible = "maxim,max17047" },
 	{ .compatible = "maxim,max17050" },
+	{ .compatible = "maxim,max77693" },
+	{ .compatible = "maxim,max77843" },
+
 	{ },
 };
 MODULE_DEVICE_TABLE(of, max17042_dt_match);
@@ -846,6 +851,9 @@ static const struct i2c_device_id max17042_id[] = {
 	{ "max17042", MAXIM_DEVICE_TYPE_MAX17042 },
 	{ "max17047", MAXIM_DEVICE_TYPE_MAX17047 },
 	{ "max17050", MAXIM_DEVICE_TYPE_MAX17050 },
+	{ "max77693", MAXIM_DEVICE_TYPE_MAX77693 },
+	{ "max77843", MAXIM_DEVICE_TYPE_MAX77843 },
+
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, max17042_id);
diff --git a/include/linux/power/max17042_battery.h b/include/linux/power/max17042_battery.h
index 8dd7b70..72de625 100644
--- a/include/linux/power/max17042_battery.h
+++ b/include/linux/power/max17042_battery.h
@@ -131,6 +131,8 @@ enum max170xx_chip_type {
 	MAXIM_DEVICE_TYPE_MAX17042,
 	MAXIM_DEVICE_TYPE_MAX17047,
 	MAXIM_DEVICE_TYPE_MAX17050,
+	MAXIM_DEVICE_TYPE_MAX77693,
+	MAXIM_DEVICE_TYPE_MAX77843,
 
 	MAXIM_DEVICE_TYPE_NUM
 };
-- 
1.7.9.5

Re: [PATCH 2/3] power: max17042_battery: Add support for max77693/843 chip

[Krzysztof Kozlowski]

2015-03-31 15:29 GMT+02:00 Beomho Seo <beomho.seo@samsung.com>:
> Max77693/843 fuelgauge is similar to max17042 fuelgauge. This patch add
> supports max77693/843 fuelgague use max17042_battery.c fuel gauge driver.
>
> Cc: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>

Nack.
There are the same, so max77693 and max77843 are already supported. We
don't have to add new compatibles for devices which are the compatible
with existing ones. Just use
"maxim,max17042" in DTS. Just like max77693 fuel gauge on Trats2:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e8614292cd41971b54e60188d4e99abdc8695073

Best regards,
Krzysztof

Re: [PATCH 2/3] power: max17042_battery: Add support for max77693/843 chip

[Beomho Seo]

On 04/01/2015 05:27 PM, Krzysztof Kozlowski wrote:
> 2015-03-31 15:29 GMT+02:00 Beomho Seo <beomho.seo@samsung.com>:
>> Max77693/843 fuelgauge is similar to max17042 fuelgauge. This patch add
>> supports max77693/843 fuelgague use max17042_battery.c fuel gauge driver.
>>
>> Cc: Sebastian Reichel <sre@kernel.org>
>> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
> 
> Nack.
> There are the same, so max77693 and max77843 are already supported. We
> don't have to add new compatibles for devices which are the compatible
> with existing ones. Just use
> "maxim,max17042" in DTS. Just like max77693 fuel gauge on Trats2:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e8614292cd41971b54e60188d4e99abdc8695073
> 
> Best regards,
> Krzysztof
> 

OK, I will just use "maxim,max17042" in DTS.

Best regards,
Beomho

[PATCH 3/3] power: max17042_battery: add missed blank

[Beomho Seo]

This patch add missed blank line after decalations.

Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
---
 drivers/power/max17042_battery.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
index 806ac2e..c05f1e7 100644
--- a/drivers/power/max17042_battery.c
+++ b/drivers/power/max17042_battery.c
@@ -269,6 +269,7 @@ static inline void max17042_override_por(struct regmap *map,
 static inline void max10742_unlock_model(struct max17042_chip *chip)
 {
 	struct regmap *map = chip->regmap;
+
 	regmap_write(map, MAX17042_MLOCKReg1, MODEL_UNLOCK1);
 	regmap_write(map, MAX17042_MLOCKReg2, MODEL_UNLOCK2);
 }
@@ -286,6 +287,7 @@ static inline void max17042_write_model_data(struct max17042_chip *chip,
 {
 	struct regmap *map = chip->regmap;
 	int i;
+
 	for (i = 0; i < size; i++)
 		regmap_write(map, addr + i,
 			chip->pdata->config_data->cell_char_tbl[i]);
-- 
1.7.9.5

Re: [PATCH 3/3] power: max17042_battery: add missed blank

[Krzysztof Kozlowski]

2015-03-31 15:29 GMT+02:00 Beomho Seo <beomho.seo@samsung.com>:
> This patch add missed blank line after decalations.
>
> Cc: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
> ---
>  drivers/power/max17042_battery.c |    2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof