Messages in this thread | | | Date | Thu, 22 Apr 2010 11:14:38 +0400 | From | Anton Vorontsov <> | Subject | Re: [PATCH] ds2782_battery: add support for ds2786 battery gas gauge |
| |
Hi,
On Thu, Apr 22, 2010 at 09:52:52AM +0300, Mike Rapoport wrote: > From: Yulia Vilensky <vilensky@compulab.co.il> > > Signed-off-by: Yulia Vilensky <vilensky@compulab.co.il> > Signed-off-by: Mike Rapoport <mike@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@gmail.com irc://irc.freenode.net/bd2
| |