ds2782_battery: add support for ds2786 battery gas gauge
in [Kernel]
|
Prev: tip: origin tree build failure (x86, 32-bit, allyesconfig)
Next: i2c-algo-pca: fix all coding style issues in i2c-algo-pca.c
From: Anton Vorontsov on 22 Apr 2010 03:20 Hi, On Thu, Apr 22, 2010 at 09:52:52AM +0300, Mike Rapoport wrote: > From: Yulia Vilensky <vilensky(a)compulab.co.il> > > Signed-off-by: Yulia Vilensky <vilensky(a)compulab.co.il> > Signed-off-by: Mike Rapoport <mike(a)compulab.co.il> > --- Thanks for the patch, looks good overall. Few comments below. [...] > +struct ds278x_battery_ops { > + int (*get_temp)(struct ds278x_info *info, int *temp); > + int (*get_current)(struct ds278x_info *info, int *current_uA); > + int (*get_voltage)(struct ds278x_info *info, int *voltage_uA); > + int (*get_capacity)(struct ds278x_info *info, int *capacity_uA); > + Unneeded empty line. [...] > > - info->battery.name = kasprintf(GFP_KERNEL, "ds2782-%d", num); > + info->battery.name = kasprintf(GFP_KERNEL, "ds278x-%d", num); I'm a bit worried about this, as this will change sysfs stuff. I tend to think that this won't cause any real issues, but just to be on a safe side, can we change that to kasprintf(GFP_KERNEL, "%s-%d", client->name, num); ? > if (!info->battery.name) { > ret = -ENOMEM; > goto fail_name; > @@ -277,7 +399,10 @@ static int ds2782_battery_probe(struct i2c_client *client, > > i2c_set_clientdata(client, info); > info->client = client; > - ds2782_power_supply_init(&info->battery); > + info->id = num; > + info->ops = &ds278x_ops[id->driver_data]; > + info->rsns = *((int *)info->client->dev.platform_data); Please introduce 'struct ds278x_platform_data {int rsns;};' for this. Place it into include/linux/ds2782_battery.h. Thanks! -- Anton Vorontsov email: cbouatmailru(a)gmail.com irc://irc.freenode.net/bd2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Anton Vorontsov on 26 Apr 2010 06:10 On Mon, Apr 26, 2010 at 08:44:36AM +1200, Ryan Mallon wrote: [...] > >>> config BATTERY_DS2782 > >>> - tristate "DS2782 standalone gas-gauge" > >>> + tristate "DS2782/DS2786 standalone gas-gauge" > >>> depends on I2C > >>> help > >>> - Say Y here to enable support for the DS2782 standalone battery > >>> + Say Y here to enable support for the DS2782/DS2786 standalone > >>> battery > >> > >> I have only used the DS2782 chip. Can we just change this to DS278x? May > >> as well change to CONFIG_BATTERY_DS278x while we are here. > >> > > > > Shall we move ds2782_battery.c to ds278x_battery.c at the same time? > > Changing Kconfig invites the .c file move as well :) > > Yes. Are there other gas-gauges in the same family? Please don't, this is an unneeded churn. I'd won't bother with renaming Kconfig symbol as well (and btw, that renaming will break oldconfig, so I strongly suggest not to do it). So, your current approch of changing the description and help text is OK. Thanks, -- Anton Vorontsov email: cbouatmailru(a)gmail.com irc://irc.freenode.net/bd2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Anton Vorontsov on 26 Apr 2010 14:30 On Mon, Apr 26, 2010 at 02:05:25PM +0300, Mike Rapoport wrote: > From: Yulia Vilensky <vilensky(a)compulab.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(a)compulab.co.il> > Signed-off-by: Mike Rapoport <mike(a)compulab.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(a)gmail.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(a)gmail.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(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Anton Vorontsov on 2 May 2010 16:20
On Mon, Apr 26, 2010 at 10:39:41PM +0300, Mike Rapoport wrote: > On Mon, Apr 26, 2010 at 9:22 PM, Anton Vorontsov <cbouatmailru(a)gmail.com> wrote: > > > > 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(a)gmail.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)'. > > agree > > > Signed-off-by: Anton Vorontsov <cbouatmailru(a)gmail.com> > > Acked-by: Mike Rapoport <mike(a)compulab.co.il> Thanks! Applied. -- Anton Vorontsov email: cbouatmailru(a)gmail.com irc://irc.freenode.net/bd2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |