lkml.org 
[lkml]   [2010]   [Apr]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2010-04-22 09:17    [W:0.099 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site