On Thu, Sep 13, 2012 at 09:46:32PM +0530, anish kumar wrote: > From: anish kumar <[email protected]> > > In this version: > Addressed concerns raised by lars: > a. made the adc_bat per device. > b. get the IIO channel using hardcoded channel names. > c. Minor issues related to gpio_is_valid and some code > refactoring. > > Signed-off-by: anish kumar <[email protected]> > ---
Anish, much thanks for your work! This is much appreciated. There are a few comments below, they don't require any design changes, just some technical issues, which wouldn't hard to fix, I guess. And again, thanks for your efforts! > drivers/power/generic-adc-battery.c | 460 > +++++++++++++++++++++++++++++ > include/linux/power/generic-adc-battery.h | 33 ++ > 2 files changed, 493 insertions(+), 0 deletions(-) > create mode 100644 drivers/power/generic-adc-battery.c > create mode 100644 include/linux/power/generic-adc-battery.h > > diff --git a/drivers/power/generic-adc-battery.c > b/drivers/power/generic-adc-battery.c > new file mode 100644 > index 0000000..003e0e1 > --- /dev/null > +++ b/drivers/power/generic-adc-battery.c > @@ -0,0 +1,460 @@ > +/* > + * Generic battery driver code using IIO > + * Copyright (C) 2012, Anish Kumar <[email protected]> > + * based on jz4740-battery.c > + * based on s3c_adc_battery.c > + * > + * This file is subject to the terms and conditions of the GNU General Public > + * License. See the file COPYING in the main directory of this archive for > + * more details. > + * No need for this empty comment line. > + */ > + > +#include <linux/interrupt.h> > +#include <linux/platform_device.h> > +#include <linux/power_supply.h> > +#include <linux/gpio.h> > +#include <linux/err.h> > +#include <linux/timer.h> > +#include <linux/jiffies.h> > +#include <linux/errno.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/iio/consumer.h> > +#include <linux/iio/types.h> > + No need for this empty line. > +#include <linux/power/generic-adc-battery.h> > + > +#define to_generic_bat(ptr) \ > + container_of(ptr, struct generic_adc_bat, ptr) This will fit on one line. Plus, it look strange that you use 'struct generic_adc_bat' before actually declaring it. :-) Surely it works because it's a macro. Also, while this is a simple one-liner, I'd really suggest making it a function. That way you also won't have a risk to evaluate 'ptr' expression twice, plus the function is type-safe (unlike the macro). > + > +enum chan_type { > + VOLTAGE = 0, > + CURRENT, > + POWER, > + MAX_CHAN_TYPE The enum is in the global namespace. Please prefix items with GEN_ADC_BAT (or GENERIC_ADC_BAT, but that would be too long). Personally, I like making acronyms. I.e., while file name already makes it pretty clear that it is generic_adc_battery, in the file itself you can use short gab_ and GAB_ everywhere. Anyways, it's up to you. generic_adc_bat_ prefix is also fine. But be consistent. The same with chan_type, please prefix it with something. > +}; > + > +#define CHAN_MAX_NAME 30 > +/* > + * channel_name suggests the standard channel names for commonly used > + * channel types. > + */ > +char channel_name[][CHAN_MAX_NAME + 1] = { Ditto. gab_channel_name. Plus, you can make it char *gab_channel_name[] = { [VOLTAGE] = "voltage", [...] = ..., }; Thus no need for MAX_NAME. > + [VOLTAGE] = "voltage", > + [CURRENT] = "current", > + [POWER] = "power", > +}; > + > +struct generic_adc_bat { > + struct power_supply psy; > + struct iio_channel **channel; > + struct iio_battery_platform_data *pdata; > + struct delayed_work bat_work; > + int was_plugged; > + int volt_value; > + int cur_value; > + int level; > + int status; > + int cable_plugged:1; > +}; > + > +static void generic_adc_bat_ext_power_changed(struct power_supply *psy) > +{ > + struct generic_adc_bat *adc_bat; > + adc_bat = to_generic_bat(psy); This would fit into one line. > + > + schedule_delayed_work(&adc_bat->bat_work, > + msecs_to_jiffies(adc_bat->pdata->jitter_delay)); > +} > + > +static enum power_supply_property bat_props[] = { While it's static, it still looks awkward that you don't prefix it. > + POWER_SUPPLY_PROP_STATUS, > + POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, > + POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN, > + POWER_SUPPLY_PROP_CHARGE_NOW, > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > + POWER_SUPPLY_PROP_CURRENT_NOW, > + POWER_SUPPLY_PROP_TECHNOLOGY, > + POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN, > + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, > + POWER_SUPPLY_PROP_MODEL_NAME, > +}; > + > +/* > + * This properties are set based on the received platform data and this > + * should correspond one-to-one with enum chan_type. > + */ > +static enum power_supply_property dyn_props[] = { > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > + POWER_SUPPLY_PROP_CURRENT_NOW, > + POWER_SUPPLY_PROP_POWER_NOW, > +}; > + > +static int charge_finished(struct generic_adc_bat *adc_bat) > +{ > + return adc_bat->pdata->gpio_inverted ? > + !gpio_get_value(adc_bat->pdata->gpio_charge_finished) : > + gpio_get_value(adc_bat->pdata->gpio_charge_finished); That looks unreadable. How about: bool ret = gpio_get_value(adc_bat->pdata->gpio_charge_finished); bool inv = adc_bat->pdata->gpio_inverted; return ret ^ inv; (you can also make gpio_inverted bool, as well as return value of charge_finished function) > +} > + > +long read_scale_channel(struct generic_adc_bat *adc_bat, > + enum power_supply_property psp) static? prefix? > +{ > + int scaleint, scalepart, iio_val, ret = 0; One variable declaration per line, please. int scaleint; int scalepart; etc. > + int result = 0; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_POWER_NOW: > + ret = iio_read_channel_raw(adc_bat->channel[POWER], > + &iio_val); you don't use this return value, why assign? Also, this is very repeated. How about introducing a function int gab_read_chan(enum gab_chan, int *val, int *scaleint, int *scalepart) { int ret; ret = iio_read_channel_raw(chan, val); if (ret < 0) return ret; ret = iio_read_channel_scale(chan, scaleint, scalepart); if (ret < 0) return ret; return 0; } Then you'll have switch (psp) { case POWER_SUPPLY_PROP_POWER_NOW: ret = gab_read_chan(adc_bat->channel[POWER], &iio_val, &scaleint, &scalepart); break; case POWER_SUPPLY_PROP_VOLTAGE_NOW: ret = gab_read_chan(adc_bat->channel[VOLTAGE], &iio_val, &scaleint, &scalepart); break; .... } if (ret < 0) return ret; It is easily noticed that this is quite repeating too. So we can do better, let's introduce property-to-channel function: enum gab_chan gab_prop_to_chan(enum power_supply_property psp) { switch (psp) { case POWER_SUPPLY_PROP_POWER_NOW: return GAB_POWER; case POWER_SUPPLY_PROP_VOLTAGE_NOW: return GAB_VOLTAGE; .... } WARN_ON(1); return GAB_POWER; } And then we can just do this: enum gab_chan chan = gab_prop_to_chan(psp); ret = gab_read_chan(chan, &iio_val, &scaleint, &scalepart); if (ret < 0) return 0; Which is much nicer. :-) > + ret = iio_read_channel_scale(adc_bat->channel[POWER], > + &scaleint, &scalepart); > + if (ret < 0) > + return ret; > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + ret = iio_read_channel_raw(adc_bat->channel[VOLTAGE], > + &iio_val); > + ret = iio_read_channel_scale(adc_bat->channel[VOLTAGE], > + &scaleint, &scalepart); > + if (ret < 0) > + return ret; > + break; > + case POWER_SUPPLY_PROP_CURRENT_NOW: > + ret = iio_read_channel_raw(adc_bat->channel[CURRENT], > + &iio_val); > + ret = iio_read_channel_scale(adc_bat->channel[CURRENT], > + &scaleint, &scalepart); > + if (ret < 0) > + return ret; > + break; > + default: > + break; > + } > + > + switch (ret) { > + case IIO_VAL_INT: > + result = iio_val * scaleint; > + break; > + case IIO_VAL_INT_PLUS_MICRO: > + result = (s64)iio_val * (s64)scaleint + > + div_s64((s64)iio_val * (s64)scalepart, 1000000LL); > + break; > + case IIO_VAL_INT_PLUS_NANO: > + result = (s64)iio_val * (s64)scaleint + > + div_s64((s64)iio_val * (s64)scalepart, 1000000000LL); > + break; > + } > + return result; > +} > + > +static int generic_adc_bat_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct generic_adc_bat *adc_bat; > + int ret = 0; > + long result; > + > + adc_bat = to_generic_bat(psy); > + if (!adc_bat) { > + dev_err(psy->dev, "no battery infos ?!\n"); > + return -EINVAL; > + } > + > + result = read_scale_channel(adc_bat, psp); > + if (result < 0) { > + dev_err(psy->dev, "read channel error\n"); > + ret = -EINVAL; > + goto err; > + } > + > + switch (psp) { > + case POWER_SUPPLY_PROP_STATUS: > + if (gpio_is_valid(adc_bat->pdata->gpio_charge_finished)) { > + if (adc_bat->pdata->gpio_charge_finished < 0) > + val->intval = adc_bat->level == 100000 ? > + POWER_SUPPLY_STATUS_FULL : > + adc_bat->status; This is u?n(r)e:a<d?a->b=l&e. :-) Plus, I guess there's some logic error: you check gpio_charge_finished for gpio_is_valid, and then for it "< 0", which is always false. I think you don't need "< 0" check. How about a separate function for this? I.e. int gab_get_status(struct generic_adc_bat *gab) { struct gab_platform_data *pdata = gab->pdata; int chg_gpio = pdata->gpio_charge_finished if (!gpio_is_valid(chg_gpio) || adc_bat->level < 100000) return adc_bat->status; return POWER_SUPPLY_STATUS_FULL; } > + else > + val->intval = adc_bat->status; > + } > + break; > + case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN: > + val->intval = 0; > + break; > + case POWER_SUPPLY_PROP_CHARGE_NOW: > + val->intval = adc_bat->pdata->cal_charge(result); > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + val->intval = result; > + break; > + case POWER_SUPPLY_PROP_CURRENT_NOW: > + val->intval = result; > + break; > + case POWER_SUPPLY_PROP_POWER_NOW: > + val->intval = result; > + break; > + case POWER_SUPPLY_PROP_TECHNOLOGY: > + val->intval = adc_bat->pdata->battery_info.technology; > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: > + val->intval = adc_bat->pdata->battery_info.voltage_min_design; You repeat 'adc_bat->pdata->battery_info' too much. Please introduce short a variable. I.e. struct power_supply_info *bi = &adc_bat->pdata->battery_info; Then you can type bi->voltage_min_design; > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: > + val->intval = adc_bat->pdata->battery_info.voltage_max_design; > + break; > + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: > + val->intval = adc_bat->pdata->battery_info.charge_full_design; > + break; > + case POWER_SUPPLY_PROP_MODEL_NAME: > + val->strval = adc_bat->pdata->battery_info.name; > + break; > + default: > + return -EINVAL; > + } > +err: > + return ret; > +} > + > +static void generic_adc_bat_work(struct work_struct *work) > +{ > + struct generic_adc_bat *adc_bat; > + struct delayed_work *delayed_work; > + int is_charged; > + int is_plugged; > + > + delayed_work = container_of(work, > + struct delayed_work, work); This fits into one line. > + adc_bat = container_of(delayed_work, > + struct generic_adc_bat, bat_work); Ditto. > + is_plugged = power_supply_am_i_supplied(&adc_bat->psy); > + adc_bat->cable_plugged = is_plugged; > + if (is_plugged != adc_bat->was_plugged) { > + adc_bat->was_plugged = is_plugged; > + if (is_plugged) > + adc_bat->status = POWER_SUPPLY_STATUS_CHARGING; > + else > + adc_bat->status = POWER_SUPPLY_STATUS_DISCHARGING; > + } else { > + if (gpio_is_valid(adc_bat->pdata->gpio_charge_finished)) { > + if ((adc_bat->pdata->gpio_charge_finished >= 0) && If you use gpio_is_valid, you don't need another >= check. > + is_plugged) { > + is_charged = charge_finished(adc_bat); > + if (is_charged) > + adc_bat->status = > + POWER_SUPPLY_STATUS_FULL; > + else > + adc_bat->status = > + POWER_SUPPLY_STATUS_CHARGING; > + } > + } > + } > + > + power_supply_changed(&adc_bat->psy); > +} > + > +static irqreturn_t generic_adc_bat_charged(int irq, void *dev_id) > +{ > + struct generic_adc_bat *adc_bat = dev_id; Need an empty line here. > + adc_bat = container_of(&adc_bat->bat_work, > + struct generic_adc_bat, bat_work); Something is seriously wrong here. You probably don't need container_of. Plus you pass pointer-to-pointer to request_irq()... I doubt it will work. > + schedule_delayed_work(&adc_bat->bat_work, > + msecs_to_jiffies(adc_bat->pdata->jitter_delay)); > + return IRQ_HANDLED; > +} > + > +static int __devinit generic_adc_bat_probe(struct platform_device *pdev) > +{ > + struct generic_adc_bat *adc_bat; > + struct iio_battery_platform_data *pdata = pdev->dev.platform_data; > + int ret, chan_index, fail = 0; One variable per line. int ret; int chan_index; ... > + > + adc_bat = devm_kzalloc(&pdev->dev, sizeof(*adc_bat), GFP_KERNEL); > + if (!adc_bat) { > + dev_err(&pdev->dev, "failed to allocate memory\n"); > + return -ENOMEM; > + } > + > + /* copying the battery name from platform data */ > + adc_bat->psy.name = pdata->battery_name; > + > + /* bootup default values for the battery */ > + adc_bat->volt_value = -1; > + adc_bat->cur_value = -1; > + adc_bat->cable_plugged = 0; > + adc_bat->status = POWER_SUPPLY_STATUS_DISCHARGING; > + adc_bat->psy.type = POWER_SUPPLY_TYPE_BATTERY; > + adc_bat->psy.get_property = > + generic_adc_bat_get_property; > + adc_bat->psy.external_power_changed = > + generic_adc_bat_ext_power_changed; These four lines will fit into two. > + adc_bat->pdata = pdata; > + > + /* calculate the total number of channels */ > + for (chan_index = 0; channel_name[chan_index]; chan_index++) > + ; I think you can use ARRAY_SIZE(channel_name) macro from linux/kernel.h instead of the loop. Plus, you should really name it num_chans. > + > + if (!chan_index) { > + pr_err("atleast provide one channel\n"); The chan_index is calculated from the static array that never changes. I think you don't need this check. > + return -EINVAL; > + } > + > + /* > + * copying the static properties and allocating extra memory for holding > + * the extra configurable properties received from platform data. > + */ */ is wrongly indented. > + adc_bat->psy.properties = kzalloc(sizeof(bat_props) > + + sizeof(int)*chan_index, GFP_KERNEL); Instead of sizeof(int), better sizeof(*adc_bat->psy.properties). Also, you need whitespaces around *. And I don't see where you're filling-in num_properties. How does it work without it? > + if (!adc_bat->psy.properties) { > + ret = -ENOMEM; > + goto first_mem_fail; > + } > + memcpy(adc_bat->psy.properties, bat_props, sizeof(bat_props)); > + > + /* allocating memory for iio channels */ > + adc_bat->channel = kzalloc(chan_index * sizeof(struct iio_channel), > + GFP_KERNEL); adc_bat->channels = kzalloc(num_chans * sizeof(*adc_bat->channels), GFP_KERNEL); Plus, I think you can use devm_kzalloc() here, and above. > + if (!adc_bat->channel) { > + ret = -ENOMEM; > + goto second_mem_fail; > + } > + > + /* > + * getting channel from iio and copying the battery properties > + * based on the channel supported by consumer device. > + */ > + for (chan_index = 0; channel_name[chan_index]; chan_index++) { > + adc_bat->channel[chan_index] = > + iio_channel_get(dev_name(&pdev->dev), > + channel_name[chan_index]); > + /* we skip for channels which fail */ > + if (IS_ERR(adc_bat->channel[chan_index])) > + fail++; > + else { Need {} braces for if statement too, so it'll have to be } else{, per coding style. > + static int index; empty line here please. > + memcpy(adc_bat->psy.properties + > + sizeof(bat_props) + sizeof(int)*index, > + &dyn_props[chan_index], > + sizeof(dyn_props[chan_index])); I'm too lazy to even think what it does... I hope it works. :-D > + index++; > + } > + } > + > + /* none of the channels are supported so let's bail out */ > + if (fail == chan_index) { > + ret = PTR_ERR(adc_bat->channel[chan_index]); > + goto second_mem_fail; > + } > + > + ret = power_supply_register(&pdev->dev, &adc_bat->psy); > + if (ret) > + goto err_reg_fail; > + > + INIT_DELAYED_WORK(&adc_bat->bat_work, generic_adc_bat_work); > + > + if (gpio_is_valid(pdata->gpio_charge_finished)) { > + ret = gpio_request(pdata->gpio_charge_finished, "charged"); > + if (ret) > + goto err_gpio; > + > + ret = request_any_context_irq(gpio_to_irq > + (pdata->gpio_charge_finished), Very unfortunate line break, at first I thought that you don't need parenthesis, but then I noticed that it's actually a function call. Heh. please introduce a local variable int irq; above (just after if (gpio_is_valid)... i.e. int irq = gpio_to_irq(pdata->gpio_charge_finished); ret = request_any_context_irq(irq, .....). > + generic_adc_bat_charged, > + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, > + "battery charged", &adc_bat); Here, &adc_bat (pointer-to-pointer) looks very wrong. Why do you do this? > + if (ret) > + goto err_gpio; > + } else > + goto err_gpio; /* let's bail out */ } else { ... } > + platform_set_drvdata(pdev, &adc_bat); > + > + /* Schedule timer to check current status */ > + schedule_delayed_work(&adc_bat->bat_work, > + msecs_to_jiffies(0)); > + return 0; > + > +err_gpio: > + power_supply_unregister(&adc_bat->psy); > +err_reg_fail: > + for (chan_index = 0; channel_name[chan_index]; chan_index++) > + iio_channel_release(adc_bat->channel[chan_index]); > + kfree(adc_bat->channel); > +second_mem_fail: > + kfree(adc_bat->psy.properties); > +first_mem_fail: > + return ret; > +} > + > +static int generic_adc_bat_remove(struct platform_device *pdev) > +{ > + int chan_index; > + struct generic_adc_bat *adc_bat = platform_get_drvdata(pdev); > + > + power_supply_unregister(&adc_bat->psy); > + > + if (gpio_is_valid(adc_bat->pdata->gpio_charge_finished)) { > + if (adc_bat->pdata->gpio_charge_finished >= 0) { You don't need >= check. gpio_is_valid is used for this. > + free_irq(gpio_to_irq( > + adc_bat->pdata->gpio_charge_finished), > + NULL); Please don't be scared to introduce temporary variables if they make code more readable. Here, introduce 'int irq = gpio_to_irq(...)'; > + gpio_free(adc_bat->pdata->gpio_charge_finished); > + } > + } > + > + for (chan_index = 0; channel_name[chan_index]; chan_index++) > + iio_channel_release(adc_bat->channel[chan_index]); > + cancel_delayed_work(&adc_bat->bat_work); > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int generic_adc_bat_suspend(struct device *dev) > +{ > + struct generic_adc_bat *adc_bat = dev_get_drvdata(dev); > + > + cancel_delayed_work_sync(&adc_bat->bat_work); > + adc_bat->status = POWER_SUPPLY_STATUS_UNKNOWN; > + return 0; > +} > + > +static int generic_adc_bat_resume(struct device *dev) > +{ > + struct generic_adc_bat *adc_bat = dev_get_drvdata(dev); > + > + /* Schedule timer to check current status */ > + schedule_delayed_work(&adc_bat->bat_work, > + msecs_to_jiffies(adc_bat->pdata->jitter_delay)); > + return 0; > +} > + > +static const struct dev_pm_ops generic_adc_battery_pm_ops = { > + .suspend = generic_adc_bat_suspend, > + .resume = generic_adc_bat_resume, > +}; > + > +#define GENERIC_ADC_BATTERY_PM_OPS (&generic_adc_battery_pm_ops) > +#else > +#define GENERIC_ADC_BATTERY_PM_OPS (NULL) > +#endif > + > +static struct platform_driver generic_adc_bat_driver = { > + .driver = { > + .name = "generic-adc-battery", > + .owner = THIS_MODULE, > + .pm = GENERIC_ADC_BATTERY_PM_OPS > + }, > + .probe = generic_adc_bat_probe, > + .remove = generic_adc_bat_remove, > +}; > + No need for this empty line. > +module_platform_driver(generic_adc_bat_driver); > + > +MODULE_AUTHOR("anish kumar <[email protected]>"); > +MODULE_DESCRIPTION("generic battery driver using IIO"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/power/generic-adc-battery.h > b/include/linux/power/generic-adc-battery.h > new file mode 100644 > index 0000000..d7c6d20 > --- /dev/null > +++ b/include/linux/power/generic-adc-battery.h > @@ -0,0 +1,33 @@ > +/* > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#ifndef GENERIC_ADC_BATTERY_H > +#define GENERIC_ADC_BATTERY_H > + > +/* should be enough for the channel names */ > +#define BAT_MAX_NAME 30 > + > +enum chan_type { > + VOLTAGE = 0, > + CURRENT, > + POWER, > + MAX_CHAN_TYPE > +}; You define chan_type twice. Keep only one declaration, this one, in the header file. > + > +extern char channel_name[][BAT_MAX_NAME + 1]; Uhm. Making it extern means that you're no longer able to use this driver as a module. And the module isn't a problem per se, it just a sign of some bigger problem... Maybe you should rethink how you pass channel name, dunno. > +struct iio_battery_platform_data { Inconsistent. Either use generic_adc_battery_ prefix, or iio_battery_, or anything you like, but please be consistent across all the driver and files. > + struct power_supply_info battery_info; > + char **channel_name; > + char *battery_name; > + int (*cal_charge)(long); Long what? :-) Please specify a variable name. > + int gpio_charge_finished; > + int gpio_inverted; > + int bat_poll_interval; > + int jitter_delay; > +}; > + > +#endif /* GENERIC_ADC_BATTERY_H */ Cheers, Anton. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
