linux-kernel - Re: [PATCH v3] ds2782_battery: add support for ds2786 battery gas gauge

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 26 Apr 2010 22:22:00 +0400
From:	Anton Vorontsov <cbouatmailru@...il.com>
To:	Mike Rapoport <mike@...pulab.co.il>
Cc:	Ryan Mallon <ryan@...ewatersys.com>, linux-kernel@...r.kernel.org,
	Yulia Vilensky <vilensky@...pulab.co.il>
Subject: Re: [PATCH v3] ds2782_battery: add support for ds2786 battery gas
 gauge

On Mon, Apr 26, 2010 at 02:05:25PM +0300, Mike Rapoport wrote:
> From: Yulia Vilensky <vilensky@...pulab.co.il>
> 
> v2 changes:
>  * add ds278x_platform_data
>  * address Anton comments
> v3 changes:
>  * use ds278x_get_temp for both ds2782 and ds2786
>  * update math as per Ryan comments
> 
> Signed-off-by: Yulia Vilensky <vilensky@...pulab.co.il>
> Signed-off-by: Mike Rapoport <mike@...pulab.co.il>

Thanks, applied to battery-2.6 with the following change
(we shouldn't compare pointers and integers):

diff --git a/drivers/power/ds2782_battery.c b/drivers/power/ds2782_battery.c
index 6b3cee0..c665e80 100644
--- a/drivers/power/ds2782_battery.c
+++ b/drivers/power/ds2782_battery.c
@@ -325,7 +325,7 @@ static int ds278x_battery_probe(struct i2c_client *client,
 	 * ds2786 should have the sense resistor value set
 	 * in the platform data
 	 */
-	if (id->driver_data == 1 && pdata == 0) {
+	if (id->driver_data == 1 && !pdata) {
 		dev_err(&client->dev, "missing platform data for ds2786\n");
 		return -EINVAL;
 	}

----

Btw, I don't quite like the 'if (id->driver_data == 1)' stuff.
How about the following patch on top?

>From acf917d3880465b76875f671ee450a8fdff62c9f Mon Sep 17 00:00:00 2001
From: Anton Vorontsov <cbouatmailru@...il.com>
Date: Mon, 26 Apr 2010 22:10:52 +0400
Subject: [PATCH] ds2782_battery: Get rid of magic numbers in driver_data

Constructions like 'if (id->driver_data == 1)' look quite weird.
This patch introduces 'enum ds278x_num_id', which makes things
much more understandable, i.e. 'if (id->driver_data == DS2786)'.

Signed-off-by: Anton Vorontsov <cbouatmailru@...il.com>
---
 drivers/power/ds2782_battery.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/power/ds2782_battery.c b/drivers/power/ds2782_battery.c
index c665e80..d762a0c 100644
--- a/drivers/power/ds2782_battery.c
+++ b/drivers/power/ds2782_battery.c
@@ -300,13 +300,18 @@ static int ds278x_battery_remove(struct i2c_client *client)
 	return 0;
 }
 
+enum ds278x_num_id {
+	DS2782 = 0,
+	DS2786,
+};
+
 static struct ds278x_battery_ops ds278x_ops[] = {
-	[0] = {
+	[DS2782] = {
 		.get_current  = ds2782_get_current,
 		.get_voltage  = ds2782_get_voltage,
 		.get_capacity = ds2782_get_capacity,
 	},
-	[1] = {
+	[DS2786] = {
 		.get_current  = ds2786_get_current,
 		.get_voltage  = ds2786_get_voltage,
 		.get_capacity = ds2786_get_capacity,
@@ -325,7 +330,7 @@ static int ds278x_battery_probe(struct i2c_client *client,
 	 * ds2786 should have the sense resistor value set
 	 * in the platform data
 	 */
-	if (id->driver_data == 1 && !pdata) {
+	if (id->driver_data == DS2786 && !pdata) {
 		dev_err(&client->dev, "missing platform data for ds2786\n");
 		return -EINVAL;
 	}
@@ -355,7 +360,7 @@ static int ds278x_battery_probe(struct i2c_client *client,
 		goto fail_name;
 	}
 
-	if (id->driver_data == 1)
+	if (id->driver_data == DS2786)
 		info->rsns = pdata->rsns;
 
 	i2c_set_clientdata(client, info);
@@ -385,8 +390,8 @@ fail_id:
 }
 
 static const struct i2c_device_id ds278x_id[] = {
-	{"ds2782", 0},
-	{"ds2786", 1},
+	{"ds2782", DS2782},
+	{"ds2786", DS2786},
 	{},
 };
 
-- 
1.7.0.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/