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

[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

[Sebastian Reichel]

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

Hi,

On Tue, Mar 31, 2015 at 10:34:05PM +0900, Beomho Seo wrote:
> [...]
>  	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);

Looks like you inverted the logic.

> [...]

-- Sebastian

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

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

[Beomho Seo]

On 04/01/2015 12:31 AM, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Mar 31, 2015 at 10:34:05PM +0900, Beomho Seo wrote:
>> [...]
>>  	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);
> 
> Looks like you inverted the logic.
> 
>> [...]
> 
> -- Sebastian
> 

OK I will fix it.
Thank you for comment.

Best regards,
Beomho Seo

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