* [PATCH] [RFC]power: battery: Generic battery driver using IIO
@ 2012-09-10 15:40 anish kumar
2012-09-11 9:59 ` Lars-Peter Clausen
0 siblings, 1 reply; 19+ messages in thread
From: anish kumar @ 2012-09-10 15:40 UTC (permalink / raw)
To: lars, jic23, cbou; +Cc: linux-kernel, linux-iio, anish kumar
From: anish kumar <anish198519851985@gmail.com>
This is the cleaned up code after the valuable inputs from
the Jonathan, Lars and Anton.
I have tried to accomodate all the concerns however please
let me know incase something is missed out.
Signed-off-by: anish kumar <anish198519851985@gmail.com>
---
drivers/power/generic-adc-battery.c | 431 +++++++++++++++++++++++++++++
include/linux/power/generic-adc-battery.h | 33 +++
2 files changed, 464 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..3459740
--- /dev/null
+++ b/drivers/power/generic-adc-battery.c
@@ -0,0 +1,431 @@
+/*
+ * Generic battery driver code using IIO
+ * Copyright (C) 2012, Anish Kumar <anish198519851985@gmail.com>
+ * 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.
+ *
+ */
+
+#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>
+
+#include <linux/power/generic-adc-battery.h>
+
+#define to_generic_bat(ptr) \
+ container_of(ptr, struct generic_adc_bat, ptr)
+
+/*
+ * channel_name suggests the standard channel names for commonly used
+ * channel types. Please use this names in board file if you support
+ * all the channel names.
+ */
+char channel_name[][BAT_MAX_NAME + 1] = {
+ [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);
+
+ schedule_delayed_work(&adc_bat->bat_work,
+ msecs_to_jiffies(adc_bat->pdata.jitter_delay));
+}
+
+static enum power_supply_property bat_props[] = {
+ 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);
+}
+
+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 scaleint, scalepart, iio_val, ret = 0;
+ long result = 0;
+
+ adc_bat = to_generic_bat(psy);
+ if (!adc_bat) {
+ dev_err(psy->dev, "no battery infos ?!\n");
+ return -EINVAL;
+ }
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_POWER_NOW:
+ ret = iio_read_channel_raw(adc_bat->channel[POWER],
+ &iio_val);
+ 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;
+ }
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_STATUS:
+ if (adc_bat->pdata.gpio_charge_finished < 0)
+ val->intval = adc_bat->level == 100000 ?
+ POWER_SUPPLY_STATUS_FULL : adc_bat->status;
+ 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;
+ 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;
+ }
+ return ret;
+}
+
+static struct generic_adc_bat adc_bat = {
+ .psy.type = POWER_SUPPLY_TYPE_BATTERY,
+ .psy.get_property =
+ generic_adc_bat_get_property,
+ .psy.external_power_changed =
+ generic_adc_bat_ext_power_changed,
+};
+
+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);
+ adc_bat = container_of(delayed_work,
+ struct generic_adc_bat, bat_work);
+
+ /* backup battery doesn't have current monitoring capability anyway */
+ 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 ((adc_bat->pdata.gpio_charge_finished >= 0) && 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;
+ adc_bat = container_of(&adc_bat->bat_work,
+ struct generic_adc_bat, bat_work);
+ schedule_delayed_work(&adc_bat->bat_work,
+ msecs_to_jiffies(adc_bat->pdata.jitter_delay));
+ return IRQ_HANDLED;
+}
+
+/*
+ * compare the pdata->channel names with the predefined channels and
+ * returns the index of the channel in channel_name array or -1 in the
+ * case of not-found.
+ */
+int channel_cmp(char *name)
+{
+ if (!strcmp(name, channel_name[VOLTAGE]))
+ return VOLTAGE;
+ else if (!strcmp(name, channel_name[CURRENT]))
+ return CURRENT;
+ else if (!strcmp(name, channel_name[POWER]))
+ return POWER;
+ else
+ return -1;
+}
+
+static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
+{
+ struct iio_battery_platform_data *pdata = pdev->dev.platform_data;
+ int ret, chan_index;
+
+ /* 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;
+
+ /* calculate the total number of channels */
+ for (chan_index = 0; pdata->channel_name[chan_index]; chan_index++)
+ ;
+
+ if (!chan_index) {
+ pr_err("atleast provide one channel\n");
+ return -EINVAL;
+ }
+
+ /* copying the static properties and allocating extra memory for holding
+ * the extra configurable properties received from platform data.
+ */
+ adc_bat.psy.properties = kzalloc(sizeof(bat_props)
+ + sizeof(int)*chan_index, GFP_KERNEL);
+ 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);
+ if (!adc_bat.channel) {
+ ret = -ENOMEM;
+ goto second_mem_fail;
+ }
+
+ /*
+ * getting channel from iio and copying the battery properties
+ * based on the channel set in the platform data.
+ */
+ for (chan_index = 0; pdata->channel_name[chan_index]; chan_index++) {
+ int channel = channel_cmp(pdata->channel_name[chan_index]);
+ if (channel < 0) {
+ ret = -EINVAL;
+ goto second_mem_fail;
+ }
+
+ adc_bat.channel[chan_index] =
+ iio_channel_get(dev_name(&pdev->dev),
+ pdata->channel_name[chan_index]);
+ if (IS_ERR(adc_bat.channel[chan_index])) {
+ ret = PTR_ERR(adc_bat.channel[chan_index]);
+ goto second_mem_fail;
+ }
+
+ memcpy(adc_bat.psy.properties+sizeof(bat_props),
+ &dyn_props[channel], sizeof(dyn_props[channel]));
+ }
+
+ 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_irq(gpio_to_irq(pdata->gpio_charge_finished),
+ generic_adc_bat_charged,
+ IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+ "battery charged", &adc_bat);
+ if (ret)
+ goto err_gpio;
+ } else
+ goto err_gpio; /* let's bail out */
+
+ 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; pdata->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 iio_battery_platform_data *pdata = pdev->dev.platform_data;
+ struct generic_adc_bat *adc_bat = to_generic_bat(pdata);
+
+ power_supply_unregister(&adc_bat->psy);
+
+ if (pdata->gpio_charge_finished >= 0) {
+ free_irq(gpio_to_irq(pdata->gpio_charge_finished), NULL);
+ gpio_free(pdata->gpio_charge_finished);
+ }
+
+ for (chan_index = 0; pdata->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,
+};
+
+module_platform_driver(generic_adc_bat_driver);
+
+MODULE_AUTHOR("anish kumar <anish198519851985@gmail.com>");
+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
+};
+
+extern char channel_name[][BAT_MAX_NAME + 1];
+
+struct iio_battery_platform_data {
+ struct power_supply_info battery_info;
+ char **channel_name;
+ char *battery_name;
+ int (*cal_charge)(long);
+ int gpio_charge_finished;
+ int gpio_inverted;
+ int bat_poll_interval;
+ int jitter_delay;
+};
+
+#endif /* GENERIC_ADC_BATTERY_H */
--
1.7.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-10 15:40 [PATCH] [RFC]power: battery: Generic battery driver using IIO anish kumar
@ 2012-09-11 9:59 ` Lars-Peter Clausen
2012-09-12 8:30 ` anish singh
0 siblings, 1 reply; 19+ messages in thread
From: Lars-Peter Clausen @ 2012-09-11 9:59 UTC (permalink / raw)
To: anish kumar; +Cc: jic23, cbou, linux-kernel, linux-iio
On 09/10/2012 05:40 PM, anish kumar wrote:
> From: anish kumar <anish198519851985@gmail.com>
>
> This is the cleaned up code after the valuable inputs from
> the Jonathan, Lars and Anton.
>
> I have tried to accomodate all the concerns however please
> let me know incase something is missed out.
>
> Signed-off-by: anish kumar <anish198519851985@gmail.com>
Hi,
> ---
> drivers/power/generic-adc-battery.c | 431 +++++++++++++++++++++++++++++
> include/linux/power/generic-adc-battery.h | 33 +++
> 2 files changed, 464 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..3459740
> --- /dev/null
> +++ b/drivers/power/generic-adc-battery.c
> @@ -0,0 +1,431 @@
[...]
> +
> +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 scaleint, scalepart, iio_val, ret = 0;
> + long result = 0;
> +
> + adc_bat = to_generic_bat(psy);
> + if (!adc_bat) {
> + dev_err(psy->dev, "no battery infos ?!\n");
> + return -EINVAL;
> + }
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_POWER_NOW:
> + ret = iio_read_channel_raw(adc_bat->channel[POWER],
> + &iio_val);
> + 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;
> + }
I think it's a good idea to factor the channel reading and scale conversion
out into a helper function.
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_STATUS:
> + if (adc_bat->pdata.gpio_charge_finished < 0)
gpio_is_valid
> + val->intval = adc_bat->level == 100000 ?
> + POWER_SUPPLY_STATUS_FULL : adc_bat->status;
> + else
> + val->intval = adc_bat->status;
> + break;
[...]
> + 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);
> + adc_bat = container_of(delayed_work,
> + struct generic_adc_bat, bat_work);
> +
> + /* backup battery doesn't have current monitoring capability anyway */
> + 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 ((adc_bat->pdata.gpio_charge_finished >= 0) && is_plugged) {
gpio_is_valid
> + 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);
> +}
[...]
> +
> +/*
> + * compare the pdata->channel names with the predefined channels and
> + * returns the index of the channel in channel_name array or -1 in the
> + * case of not-found.
> + */
> +int channel_cmp(char *name)
> +{
> + if (!strcmp(name, channel_name[VOLTAGE]))
> + return VOLTAGE;
> + else if (!strcmp(name, channel_name[CURRENT]))
> + return CURRENT;
> + else if (!strcmp(name, channel_name[POWER]))
> + return POWER;
> + else
> + return -1;
> +}
> +
> +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
> +{
> + struct iio_battery_platform_data *pdata = pdev->dev.platform_data;
> + int ret, chan_index;
> +
> + /* copying the battery name from platform data */
> + adc_bat.psy.name = pdata->battery_name;
You should make a per device copy of adc_bat, otherwise there can only be
one device at a time.
> +
> + /* 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;
> +
> + /* calculate the total number of channels */
> + for (chan_index = 0; pdata->channel_name[chan_index]; chan_index++)
> + ;
> +
> + if (!chan_index) {
> + pr_err("atleast provide one channel\n");
> + return -EINVAL;
> + }
> +
> + /* copying the static properties and allocating extra memory for holding
> + * the extra configurable properties received from platform data.
> + */
> + adc_bat.psy.properties = kzalloc(sizeof(bat_props)
> + + sizeof(int)*chan_index, GFP_KERNEL);
> + 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);
> + if (!adc_bat.channel) {
> + ret = -ENOMEM;
> + goto second_mem_fail;
> + }
> +
> + /*
> + * getting channel from iio and copying the battery properties
> + * based on the channel set in the platform data.
> + */
> + for (chan_index = 0; pdata->channel_name[chan_index]; chan_index++) {
> + int channel = channel_cmp(pdata->channel_name[chan_index]);
> + if (channel < 0) {
> + ret = -EINVAL;
> + goto second_mem_fail;
> + }
> +
> + adc_bat.channel[chan_index] =
> + iio_channel_get(dev_name(&pdev->dev),
> + pdata->channel_name[chan_index]);
Just request the channel with the hardcoded channel names. There is no need
to pass these in via platform data. If a channel is not found just skip it
and continue with the next one. Only if 0 channels were found return an error.
> + if (IS_ERR(adc_bat.channel[chan_index])) {
> + ret = PTR_ERR(adc_bat.channel[chan_index]);
> + goto second_mem_fail;
> + }
> +
> + memcpy(adc_bat.psy.properties+sizeof(bat_props),
> + &dyn_props[channel], sizeof(dyn_props[channel]));
You need to increase the offset for each new property.
> + }
> +
> + 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_irq(gpio_to_irq(pdata->gpio_charge_finished),
> + generic_adc_bat_charged,
> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> + "battery charged", &adc_bat);
I'd make this request_any_context_irq, some IRQ expander use nested IRQs.
> + if (ret)
> + goto err_gpio;
> + } else
> + goto err_gpio; /* let's bail out */
> +
> + 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; pdata->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 iio_battery_platform_data *pdata = pdev->dev.platform_data;
> + struct generic_adc_bat *adc_bat = to_generic_bat(pdata);
> +
> + power_supply_unregister(&adc_bat->psy);
> +
> + if (pdata->gpio_charge_finished >= 0) {
gpio_is_valid
> + free_irq(gpio_to_irq(pdata->gpio_charge_finished), NULL);
> + gpio_free(pdata->gpio_charge_finished);
> + }
> +
> + for (chan_index = 0; pdata->channel_name[chan_index]; chan_index++)
> + iio_channel_release(adc_bat->channel[chan_index]);
> + cancel_delayed_work(&adc_bat->bat_work);
> + return 0;
> +}
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-11 9:59 ` Lars-Peter Clausen
@ 2012-09-12 8:30 ` anish singh
2012-09-13 12:26 ` Jonathan Cameron
0 siblings, 1 reply; 19+ messages in thread
From: anish singh @ 2012-09-12 8:30 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: jic23, cbou, linux-kernel, linux-iio
On Tue, Sep 11, 2012 at 3:29 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 09/10/2012 05:40 PM, anish kumar wrote:
>> From: anish kumar <anish198519851985@gmail.com>
>>
>> This is the cleaned up code after the valuable inputs from
>> the Jonathan, Lars and Anton.
>>
>> I have tried to accomodate all the concerns however please
>> let me know incase something is missed out.
>>
>> Signed-off-by: anish kumar <anish198519851985@gmail.com>
>
> Hi,
>
>
>> ---
>> drivers/power/generic-adc-battery.c | 431 +++++++++++++++++++++++++++++
>> include/linux/power/generic-adc-battery.h | 33 +++
>> 2 files changed, 464 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..3459740
>> --- /dev/null
>> +++ b/drivers/power/generic-adc-battery.c
>> @@ -0,0 +1,431 @@
> [...]
>> +
>> +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 scaleint, scalepart, iio_val, ret = 0;
>> + long result = 0;
>> +
>> + adc_bat = to_generic_bat(psy);
>> + if (!adc_bat) {
>> + dev_err(psy->dev, "no battery infos ?!\n");
>> + return -EINVAL;
>> + }
>> +
>> + switch (psp) {
>> + case POWER_SUPPLY_PROP_POWER_NOW:
>> + ret = iio_read_channel_raw(adc_bat->channel[POWER],
>> + &iio_val);
>> + 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;
>> + }
>
> I think it's a good idea to factor the channel reading and scale conversion
> out into a helper function.
>
>> +
>> + switch (psp) {
>> + case POWER_SUPPLY_PROP_STATUS:
>> + if (adc_bat->pdata.gpio_charge_finished < 0)
>
> gpio_is_valid
>
>> + val->intval = adc_bat->level == 100000 ?
>> + POWER_SUPPLY_STATUS_FULL : adc_bat->status;
>> + else
>> + val->intval = adc_bat->status;
>> + break;
> [...]
>> + 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);
>> + adc_bat = container_of(delayed_work,
>> + struct generic_adc_bat, bat_work);
>> +
>> + /* backup battery doesn't have current monitoring capability anyway */
>> + 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 ((adc_bat->pdata.gpio_charge_finished >= 0) && is_plugged) {
>
> gpio_is_valid
>
>> + 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);
>> +}
>
> [...]
>
>> +
>> +/*
>> + * compare the pdata->channel names with the predefined channels and
>> + * returns the index of the channel in channel_name array or -1 in the
>> + * case of not-found.
>> + */
>> +int channel_cmp(char *name)
>> +{
>> + if (!strcmp(name, channel_name[VOLTAGE]))
>> + return VOLTAGE;
>> + else if (!strcmp(name, channel_name[CURRENT]))
>> + return CURRENT;
>> + else if (!strcmp(name, channel_name[POWER]))
>> + return POWER;
>> + else
>> + return -1;
>> +}
>> +
>> +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
>> +{
>> + struct iio_battery_platform_data *pdata = pdev->dev.platform_data;
>> + int ret, chan_index;
>> +
>> + /* copying the battery name from platform data */
>> + adc_bat.psy.name = pdata->battery_name;
>
> You should make a per device copy of adc_bat, otherwise there can only be
> one device at a time.
>
>> +
>> + /* 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;
>> +
>> + /* calculate the total number of channels */
>> + for (chan_index = 0; pdata->channel_name[chan_index]; chan_index++)
>> + ;
>> +
>> + if (!chan_index) {
>> + pr_err("atleast provide one channel\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* copying the static properties and allocating extra memory for holding
>> + * the extra configurable properties received from platform data.
>> + */
>> + adc_bat.psy.properties = kzalloc(sizeof(bat_props)
>> + + sizeof(int)*chan_index, GFP_KERNEL);
>> + 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);
>> + if (!adc_bat.channel) {
>> + ret = -ENOMEM;
>> + goto second_mem_fail;
>> + }
>> +
>> + /*
>> + * getting channel from iio and copying the battery properties
>> + * based on the channel set in the platform data.
>> + */
>> + for (chan_index = 0; pdata->channel_name[chan_index]; chan_index++) {
>> + int channel = channel_cmp(pdata->channel_name[chan_index]);
>> + if (channel < 0) {
>> + ret = -EINVAL;
>> + goto second_mem_fail;
>> + }
>> +
>> + adc_bat.channel[chan_index] =
>> + iio_channel_get(dev_name(&pdev->dev),
>> + pdata->channel_name[chan_index]);
>
> Just request the channel with the hardcoded channel names. There is no need
> to pass these in via platform data. If a channel is not found just skip it
> and continue with the next one. Only if 0 channels were found return an error.
>
>> + if (IS_ERR(adc_bat.channel[chan_index])) {
>> + ret = PTR_ERR(adc_bat.channel[chan_index]);
>> + goto second_mem_fail;
>> + }
>> +
>> + memcpy(adc_bat.psy.properties+sizeof(bat_props),
>> + &dyn_props[channel], sizeof(dyn_props[channel]));
>
> You need to increase the offset for each new property.
>
>> + }
>> +
>> + 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_irq(gpio_to_irq(pdata->gpio_charge_finished),
>> + generic_adc_bat_charged,
>> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>> + "battery charged", &adc_bat);
>
> I'd make this request_any_context_irq, some IRQ expander use nested IRQs.
>
>> + if (ret)
>> + goto err_gpio;
>> + } else
>> + goto err_gpio; /* let's bail out */
>> +
>> + 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; pdata->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 iio_battery_platform_data *pdata = pdev->dev.platform_data;
>> + struct generic_adc_bat *adc_bat = to_generic_bat(pdata);
>> +
>> + power_supply_unregister(&adc_bat->psy);
>> +
>> + if (pdata->gpio_charge_finished >= 0) {
>
> gpio_is_valid
>
>> + free_irq(gpio_to_irq(pdata->gpio_charge_finished), NULL);
>> + gpio_free(pdata->gpio_charge_finished);
>> + }
>> +
>> + for (chan_index = 0; pdata->channel_name[chan_index]; chan_index++)
>> + iio_channel_release(adc_bat->channel[chan_index]);
>> + cancel_delayed_work(&adc_bat->bat_work);
>> + return 0;
>> +}
>
Thanks Lars, all of your comments are valid and I will accordingly update.
I am waiting for some more review comments if there is any and will send
the updated code.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-12 8:30 ` anish singh
@ 2012-09-13 12:26 ` Jonathan Cameron
2012-09-13 12:33 ` anish singh
0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2012-09-13 12:26 UTC (permalink / raw)
To: anish singh; +Cc: Lars-Peter Clausen, cbou, linux-kernel, linux-iio
>>
> Thanks Lars, all of your comments are valid and I will accordingly update.
> I am waiting for some more review comments if there is any and will send
> the updated code.
Beware of that strategy as I for one am guilty of the old approach of
'oh look someone else has commented, I'll wait for the
next version before taking a look' - particularly when it's a good
reviewer like Lars
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-13 12:26 ` Jonathan Cameron
@ 2012-09-13 12:33 ` anish singh
0 siblings, 0 replies; 19+ messages in thread
From: anish singh @ 2012-09-13 12:33 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Lars-Peter Clausen, cbou, linux-kernel, linux-iio
On Thu, Sep 13, 2012 at 5:56 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>
>> Thanks Lars, all of your comments are valid and I will accordingly update.
>> I am waiting for some more review comments if there is any and will send
>> the updated code.
>
> Beware of that strategy as I for one am guilty of the old approach of 'oh
> look someone else has commented, I'll wait for the
> next version before taking a look' - particularly when it's a good
> reviewer like Lars
Completely agree.
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] [RFC]power: battery: Generic battery driver using IIO
@ 2012-09-02 15:39 anish kumar
2012-09-02 16:14 ` Sannu K
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: anish kumar @ 2012-09-02 15:39 UTC (permalink / raw)
To: jic23, linux-iio, cbou, dwmw2; +Cc: linux-kernel, anish kumar
From: anish kumar <anish198519851985@gmail.com>
This patch is to use IIO to write a generic batttery driver.
There are some inherent assumptions here:
1.User is having both main battery and backup battery.
2.Both batteries use same channel to get the information.
Signed-off-by: anish kumar <anish198519851985@gmail.com>
---
drivers/power/Kconfig | 8 +
drivers/power/Makefile | 1 +
drivers/power/generic-battery.c | 374 +++++++++++++++++++++++++++++++++
include/linux/power/generic-battery.h | 26 +++
4 files changed, 409 insertions(+), 0 deletions(-)
create mode 100644 drivers/power/generic-battery.c
create mode 100644 include/linux/power/generic-battery.h
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index fcc1bb0..546e86b 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -309,6 +309,14 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
help
Say Y to enable battery temperature measurements using
thermistor connected on BATCTRL ADC.
+
+config GENERIC_BATTERY
+ tristate "Generic battery support using IIO"
+ depends on IIO
+ help
+ Say Y here to enable support for the generic battery driver
+ which uses IIO framework to read adc for it's main and backup
+ battery.
endif # POWER_SUPPLY
source "drivers/power/avs/Kconfig"
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index ee58afb..8284f9c 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -45,3 +45,4 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
obj-$(CONFIG_POWER_AVS) += avs/
obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
+obj-$(CONFIG_GENERIC_BATTERY) += generic-battery.o
diff --git a/drivers/power/generic-battery.c b/drivers/power/generic-battery.c
new file mode 100644
index 0000000..33170b7
--- /dev/null
+++ b/drivers/power/generic-battery.c
@@ -0,0 +1,374 @@
+/*
+ * Generice battery driver code using IIO
+ * Copyright (C) 2012, Anish Kumar <anish198519851985@gmail.com>
+ * 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.
+ *
+ */
+
+#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>
+
+#include <linux/power/generic-battery.h>
+
+#define BAT_POLL_INTERVAL 10000 /* ms */
+#define JITTER_DELAY 500 /* ms */
+
+enum BAT {
+ MAIN_BAT = 0,
+ BACKUP_BAT,
+ BAT_MAX,
+};
+
+struct generic_adc_bat {
+ struct power_supply psy;
+ struct iio_channel *channels;
+ int channel_index;
+};
+
+struct generic_bat {
+ struct generic_adc_bat bat[BAT_MAX];
+ struct generic_platform_data pdata;
+ int volt_value;
+ int cur_value;
+ unsigned int timestamp;
+ int level;
+ int status;
+ int cable_plugged:1;
+};
+
+static struct generic_bat generic_bat = {
+ .bat[MAIN_BAT] = {
+ .psy.name = "main-bat",
+ },
+ .bat[BACKUP_BAT] = {
+ .psy.name = "backup-bat",
+ },
+};
+
+static struct delayed_work bat_work;
+
+static void generic_adc_bat_ext_power_changed(struct power_supply *psy)
+{
+ schedule_delayed_work(&bat_work,
+ msecs_to_jiffies(JITTER_DELAY));
+}
+
+static enum power_supply_property generic_adc_main_bat_props[] = {
+ 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,
+};
+
+static int charge_finished(struct generic_bat *bat)
+{
+ return bat->pdata.gpio_inverted ?
+ !gpio_get_value(bat->pdata.gpio_charge_finished) :
+ gpio_get_value(bat->pdata.gpio_charge_finished);
+}
+
+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;
+ struct generic_bat *bat;
+ int ret, scaleint, scalepart, iio_val;
+ long result = 0;
+
+ if (!strcmp(psy->name, "main-battery")) {
+ adc_bat = container_of(psy,
+ struct generic_adc_bat, psy);
+ bat = container_of(adc_bat,
+ struct generic_bat, bat[MAIN_BAT]);
+ } else if (!strcmp(psy->name, "backup-battery")) {
+ adc_bat = container_of(psy, struct generic_adc_bat, psy);
+ bat = container_of(adc_bat,
+ struct generic_bat, bat[BACKUP_BAT]);
+ } else {
+ /* Ideally this should never happen */
+ WARN_ON(1);
+ return -EINVAL;
+ }
+
+ if (!bat) {
+ dev_err(psy->dev, "no battery infos ?!\n");
+ return -EINVAL;
+ }
+ ret = iio_read_channel_raw(&adc_bat->channels[adc_bat->channel_index],
+ &iio_val);
+ ret = iio_read_channel_scale(&adc_bat->channels[adc_bat->channel_index],
+ &scaleint, &scalepart);
+ if (ret < 0)
+ return ret;
+
+ 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;
+ }
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_STATUS:
+ if (bat->pdata.gpio_charge_finished < 0)
+ val->intval = bat->level == 100000 ?
+ POWER_SUPPLY_STATUS_FULL : bat->status;
+ else
+ val->intval = bat->status;
+ return 0;
+ case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
+ val->intval = 0;
+ return 0;
+ case POWER_SUPPLY_PROP_CHARGE_NOW:
+ val->intval = bat->pdata.cal_charge(result);
+ return 0;
+ case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ val->intval = result;
+ return 0;
+ case POWER_SUPPLY_PROP_CURRENT_NOW:
+ val->intval = result;
+ return 0;
+ case POWER_SUPPLY_PROP_TECHNOLOGY:
+ val->intval = bat->pdata.battery_info.technology;
+ break;
+ case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+ val->intval = bat->pdata.battery_info.voltage_min_design;
+ break;
+ case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+ val->intval = bat->pdata.battery_info.voltage_max_design;
+ break;
+ case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+ val->intval = bat->pdata.battery_info.charge_full_design;
+ break;
+ case POWER_SUPPLY_PROP_MODEL_NAME:
+ val->strval = bat->pdata.battery_info.name;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return ret;
+}
+
+static void generic_adc_bat_work(struct work_struct *work)
+{
+ struct generic_bat *bat = &generic_bat;
+ int is_charged;
+ int is_plugged;
+ static int was_plugged;
+
+ /* backup battery doesn't have current monitoring capability anyway */
+ is_plugged = power_supply_am_i_supplied(&bat->bat[MAIN_BAT].psy);
+ bat->cable_plugged = is_plugged;
+ if (is_plugged != was_plugged) {
+ was_plugged = is_plugged;
+ if (is_plugged)
+ bat->status = POWER_SUPPLY_STATUS_CHARGING;
+ else
+ bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
+ } else {
+ if ((bat->pdata.gpio_charge_finished >= 0) && is_plugged) {
+ is_charged = charge_finished(&generic_bat);
+ if (is_charged)
+ bat->status = POWER_SUPPLY_STATUS_FULL;
+ else
+ bat->status = POWER_SUPPLY_STATUS_CHARGING;
+ }
+ }
+
+ power_supply_changed(&bat->bat[MAIN_BAT].psy);
+}
+
+static irqreturn_t generic_adc_bat_charged(int irq, void *dev_id)
+{
+ schedule_delayed_work(&bat_work,
+ msecs_to_jiffies(JITTER_DELAY));
+ return IRQ_HANDLED;
+}
+
+static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
+{
+ struct generic_platform_data *pdata = pdev->dev.platform_data;
+ struct iio_channel *channels;
+ int num_channels = 0;
+ int ret, i, j, k = 0;
+ enum iio_chan_type type;
+
+ for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++) {
+ generic_bat.bat[i].psy.type = POWER_SUPPLY_TYPE_BATTERY;
+ generic_bat.bat[i].psy.properties =
+ generic_adc_main_bat_props;
+ generic_bat.bat[i].psy.num_properties =
+ ARRAY_SIZE(generic_adc_main_bat_props);
+ generic_bat.bat[i].psy.get_property =
+ generic_adc_bat_get_property;
+ generic_bat.bat[i].psy.external_power_changed =
+ generic_adc_bat_ext_power_changed;
+ }
+
+ generic_bat.volt_value = -1;
+ generic_bat.cur_value = -1;
+ generic_bat.cable_plugged = 0;
+ generic_bat.status = POWER_SUPPLY_STATUS_DISCHARGING;
+
+ memcpy(&generic_bat.pdata, pdata, sizeof(struct generic_platform_data));
+
+ for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++) {
+ ret = power_supply_register(&pdev->dev,
+ &generic_bat.bat[i].psy);
+ if (ret)
+ goto err_reg_main;
+ }
+
+ channels = iio_channel_get_all(dev_name(&pdev->dev));
+ if (IS_ERR(channels)) {
+ ret = PTR_ERR(channels);
+ goto err_reg_backup;
+ }
+
+ while (channels[num_channels].indio_dev)
+ num_channels++;
+
+ for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
+ generic_bat.bat[i].channels = kzalloc(sizeof(struct iio_channel)
+ * BAT_MAX, GFP_KERNEL);
+
+ /* assuming main battery and backup battery is using the same channel */
+ for (i = 0; i < num_channels; i++) {
+ ret = iio_get_channel_type(&channels[i], &type);
+ if (ret < 0)
+ goto err_gpio;
+
+ if (type == IIO_VOLTAGE || type == IIO_CURRENT) {
+ for (j = 0; j < BAT_MAX; j++) {
+ generic_bat.bat[j].channel_index = k;
+ generic_bat.bat[j].channels[k] = channels[i];
+ }
+ k++;
+ }
+ continue;
+ }
+
+ INIT_DELAYED_WORK(&bat_work, generic_adc_bat_work);
+
+ if (pdata->gpio_charge_finished >= 0) {
+ ret = gpio_request(pdata->gpio_charge_finished, "charged");
+ if (ret)
+ goto err_gpio;
+
+ ret = request_irq(gpio_to_irq(pdata->gpio_charge_finished),
+ generic_adc_bat_charged,
+ IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+ "battery charged", NULL);
+ if (ret)
+ goto err_gpio;
+ }
+
+ dev_info(&pdev->dev, "successfully loaded\n");
+
+ /* Schedule timer to check current status */
+ schedule_delayed_work(&bat_work,
+ msecs_to_jiffies(JITTER_DELAY));
+
+ iio_channel_release_all(channels);
+
+ return 0;
+
+err_gpio:
+ iio_channel_release_all(channels);
+err_reg_backup:
+ for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
+ power_supply_unregister(&generic_bat.bat[i].psy);
+err_reg_main:
+ return ret;
+}
+
+static int generic_adc_bat_remove(struct platform_device *pdev)
+{
+ int i;
+ struct generic_platform_data *pdata = pdev->dev.platform_data;
+
+ for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
+ power_supply_unregister(&generic_bat.bat[i].psy);
+
+ if (pdata->gpio_charge_finished >= 0) {
+ free_irq(gpio_to_irq(pdata->gpio_charge_finished), NULL);
+ gpio_free(pdata->gpio_charge_finished);
+ }
+
+ cancel_delayed_work(&bat_work);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int generic_adc_bat_suspend(struct platform_device *pdev,
+ pm_message_t state)
+{
+ struct generic_platform_data *pdata = pdev->dev.platform_data;
+ struct generic_bat *bat = container_of(pdata,
+ struct generic_bat, pdata);
+
+ cancel_delayed_work_sync(&bat_work);
+ bat->status = POWER_SUPPLY_STATUS_UNKNOWN;
+
+ return 0;
+}
+
+static int generic_adc_bat_resume(struct platform_device *pdev)
+{
+ /* Schedule timer to check current status */
+ schedule_delayed_work(&bat_work,
+ msecs_to_jiffies(JITTER_DELAY));
+
+ return 0;
+}
+#else
+#define generic_adc_bat_suspend NULL
+#define generic_adc_bat_resume NULL
+#endif
+
+static struct platform_driver generic_adc_bat_driver = {
+ .driver = {
+ .name = "generic-adc-battery",
+ },
+ .probe = generic_adc_bat_probe,
+ .remove = generic_adc_bat_remove,
+ .suspend = generic_adc_bat_suspend,
+ .resume = generic_adc_bat_resume,
+};
+
+module_platform_driver(generic_adc_bat_driver);
+
+MODULE_AUTHOR("anish kumar <anish198519851985@gmail.com>");
+MODULE_DESCRIPTION("generic battery driver using IIO");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/power/generic-battery.h b/include/linux/power/generic-battery.h
new file mode 100644
index 0000000..7a43aa0
--- /dev/null
+++ b/include/linux/power/generic-battery.h
@@ -0,0 +1,26 @@
+/*
+ * 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_BATTERY_H
+#define GENERIC_BATTERY_H
+
+#include <linux/power_supply.h>
+
+/**
+ * struct generic_platform_data - platform data for generic battery
+ * @battery_info: Information about the battery
+ * @cal_charge: platform callback to calculate charge
+ * @gpio_charge_finished: GPIO number used for interrupts
+ * @gpio_inverted: Is the gpio inverted?
+ */
+struct generic_platform_data {
+ struct power_supply_info battery_info;
+ int (*cal_charge)(long);
+ int gpio_charge_finished;
+ int gpio_inverted;
+};
+
+#endif /* GENERIC_BATTERY_H */
--
1.7.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-02 15:39 anish kumar
@ 2012-09-02 16:14 ` Sannu K
2012-09-02 21:34 ` Anton Vorontsov
2012-09-07 7:52 ` Jonathan Cameron
2012-09-07 8:49 ` Lars-Peter Clausen
2 siblings, 1 reply; 19+ messages in thread
From: Sannu K @ 2012-09-02 16:14 UTC (permalink / raw)
To: anish kumar; +Cc: jic23, linux-iio, cbou, dwmw2, linux-kernel
On Sun, Sep 2, 2012 at 9:09 PM, anish kumar <anish198519851985@gmail.com> wrote:
> From: anish kumar <anish198519851985@gmail.com>
>
> This patch is to use IIO to write a generic batttery driver.
> There are some inherent assumptions here:
> 1.User is having both main battery and backup battery.
> 2.Both batteries use same channel to get the information.
>
> Signed-off-by: anish kumar <anish198519851985@gmail.com>
> ---
> drivers/power/Kconfig | 8 +
> drivers/power/Makefile | 1 +
> drivers/power/generic-battery.c | 374 +++++++++++++++++++++++++++++++++
> include/linux/power/generic-battery.h | 26 +++
> 4 files changed, 409 insertions(+), 0 deletions(-)
> create mode 100644 drivers/power/generic-battery.c
> create mode 100644 include/linux/power/generic-battery.h
>
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index fcc1bb0..546e86b 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -309,6 +309,14 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
> help
> Say Y to enable battery temperature measurements using
> thermistor connected on BATCTRL ADC.
> +
> +config GENERIC_BATTERY
> + tristate "Generic battery support using IIO"
> + depends on IIO
> + help
> + Say Y here to enable support for the generic battery driver
> + which uses IIO framework to read adc for it's main and backup
> + battery.
> endif # POWER_SUPPLY
>
> source "drivers/power/avs/Kconfig"
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index ee58afb..8284f9c 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -45,3 +45,4 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
> obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
> obj-$(CONFIG_POWER_AVS) += avs/
> obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
> +obj-$(CONFIG_GENERIC_BATTERY) += generic-battery.o
> diff --git a/drivers/power/generic-battery.c b/drivers/power/generic-battery.c
> new file mode 100644
> index 0000000..33170b7
> --- /dev/null
> +++ b/drivers/power/generic-battery.c
> @@ -0,0 +1,374 @@
> +/*
> + * Generice battery driver code using IIO
> + * Copyright (C) 2012, Anish Kumar <anish198519851985@gmail.com>
> + * 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.
> + *
> + */
> +
> +#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>
> +
> +#include <linux/power/generic-battery.h>
> +
> +#define BAT_POLL_INTERVAL 10000 /* ms */
> +#define JITTER_DELAY 500 /* ms */
> +
> +enum BAT {
> + MAIN_BAT = 0,
> + BACKUP_BAT,
> + BAT_MAX,
> +};
> +
> +struct generic_adc_bat {
> + struct power_supply psy;
> + struct iio_channel *channels;
> + int channel_index;
> +};
> +
> +struct generic_bat {
> + struct generic_adc_bat bat[BAT_MAX];
> + struct generic_platform_data pdata;
> + int volt_value;
> + int cur_value;
> + unsigned int timestamp;
> + int level;
> + int status;
> + int cable_plugged:1;
> +};
> +
> +static struct generic_bat generic_bat = {
> + .bat[MAIN_BAT] = {
> + .psy.name = "main-bat",
> + },
> + .bat[BACKUP_BAT] = {
> + .psy.name = "backup-bat",
> + },
> +};
> +
> +static struct delayed_work bat_work;
> +
> +static void generic_adc_bat_ext_power_changed(struct power_supply *psy)
> +{
> + schedule_delayed_work(&bat_work,
> + msecs_to_jiffies(JITTER_DELAY));
> +}
> +
> +static enum power_supply_property generic_adc_main_bat_props[] = {
> + 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,
> +};
> +
> +static int charge_finished(struct generic_bat *bat)
> +{
> + return bat->pdata.gpio_inverted ?
> + !gpio_get_value(bat->pdata.gpio_charge_finished) :
> + gpio_get_value(bat->pdata.gpio_charge_finished);
> +}
> +
> +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;
> + struct generic_bat *bat;
> + int ret, scaleint, scalepart, iio_val;
> + long result = 0;
> +
> + if (!strcmp(psy->name, "main-battery")) {
> + adc_bat = container_of(psy,
> + struct generic_adc_bat, psy);
> + bat = container_of(adc_bat,
> + struct generic_bat, bat[MAIN_BAT]);
> + } else if (!strcmp(psy->name, "backup-battery")) {
> + adc_bat = container_of(psy, struct generic_adc_bat, psy);
> + bat = container_of(adc_bat,
> + struct generic_bat, bat[BACKUP_BAT]);
> + } else {
> + /* Ideally this should never happen */
> + WARN_ON(1);
> + return -EINVAL;
> + }
> +
> + if (!bat) {
> + dev_err(psy->dev, "no battery infos ?!\n");
> + return -EINVAL;
> + }
> + ret = iio_read_channel_raw(&adc_bat->channels[adc_bat->channel_index],
> + &iio_val);
> + ret = iio_read_channel_scale(&adc_bat->channels[adc_bat->channel_index],
> + &scaleint, &scalepart);
> + if (ret < 0)
> + return ret;
> +
> + 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;
> + }
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_STATUS:
> + if (bat->pdata.gpio_charge_finished < 0)
> + val->intval = bat->level == 100000 ?
> + POWER_SUPPLY_STATUS_FULL : bat->status;
> + else
> + val->intval = bat->status;
> + return 0;
> + case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
> + val->intval = 0;
> + return 0;
> + case POWER_SUPPLY_PROP_CHARGE_NOW:
> + val->intval = bat->pdata.cal_charge(result);
> + return 0;
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + val->intval = result;
> + return 0;
> + case POWER_SUPPLY_PROP_CURRENT_NOW:
> + val->intval = result;
> + return 0;
> + case POWER_SUPPLY_PROP_TECHNOLOGY:
> + val->intval = bat->pdata.battery_info.technology;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> + val->intval = bat->pdata.battery_info.voltage_min_design;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> + val->intval = bat->pdata.battery_info.voltage_max_design;
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> + val->intval = bat->pdata.battery_info.charge_full_design;
> + break;
> + case POWER_SUPPLY_PROP_MODEL_NAME:
> + val->strval = bat->pdata.battery_info.name;
> + break;
> + default:
> + return -EINVAL;
> + }
> + return ret;
> +}
> +
> +static void generic_adc_bat_work(struct work_struct *work)
> +{
> + struct generic_bat *bat = &generic_bat;
> + int is_charged;
> + int is_plugged;
> + static int was_plugged;
> +
> + /* backup battery doesn't have current monitoring capability anyway */
> + is_plugged = power_supply_am_i_supplied(&bat->bat[MAIN_BAT].psy);
> + bat->cable_plugged = is_plugged;
> + if (is_plugged != was_plugged) {
> + was_plugged = is_plugged;
> + if (is_plugged)
> + bat->status = POWER_SUPPLY_STATUS_CHARGING;
> + else
> + bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
> + } else {
> + if ((bat->pdata.gpio_charge_finished >= 0) && is_plugged) {
> + is_charged = charge_finished(&generic_bat);
> + if (is_charged)
> + bat->status = POWER_SUPPLY_STATUS_FULL;
> + else
> + bat->status = POWER_SUPPLY_STATUS_CHARGING;
> + }
> + }
> +
> + power_supply_changed(&bat->bat[MAIN_BAT].psy);
> +}
> +
> +static irqreturn_t generic_adc_bat_charged(int irq, void *dev_id)
> +{
> + schedule_delayed_work(&bat_work,
> + msecs_to_jiffies(JITTER_DELAY));
> + return IRQ_HANDLED;
> +}
> +
> +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
> +{
> + struct generic_platform_data *pdata = pdev->dev.platform_data;
> + struct iio_channel *channels;
> + int num_channels = 0;
> + int ret, i, j, k = 0;
> + enum iio_chan_type type;
> +
> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++) {
> + generic_bat.bat[i].psy.type = POWER_SUPPLY_TYPE_BATTERY;
> + generic_bat.bat[i].psy.properties =
> + generic_adc_main_bat_props;
> + generic_bat.bat[i].psy.num_properties =
> + ARRAY_SIZE(generic_adc_main_bat_props);
> + generic_bat.bat[i].psy.get_property =
> + generic_adc_bat_get_property;
> + generic_bat.bat[i].psy.external_power_changed =
> + generic_adc_bat_ext_power_changed;
> + }
> +
> + generic_bat.volt_value = -1;
> + generic_bat.cur_value = -1;
> + generic_bat.cable_plugged = 0;
> + generic_bat.status = POWER_SUPPLY_STATUS_DISCHARGING;
> +
> + memcpy(&generic_bat.pdata, pdata, sizeof(struct generic_platform_data));
> +
> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++) {
> + ret = power_supply_register(&pdev->dev,
> + &generic_bat.bat[i].psy);
> + if (ret)
> + goto err_reg_main;
> + }
> +
> + channels = iio_channel_get_all(dev_name(&pdev->dev));
> + if (IS_ERR(channels)) {
> + ret = PTR_ERR(channels);
> + goto err_reg_backup;
> + }
> +
> + while (channels[num_channels].indio_dev)
> + num_channels++;
> +
> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> + generic_bat.bat[i].channels = kzalloc(sizeof(struct iio_channel)
> + * BAT_MAX, GFP_KERNEL);
> +
> + /* assuming main battery and backup battery is using the same channel */
> + for (i = 0; i < num_channels; i++) {
> + ret = iio_get_channel_type(&channels[i], &type);
> + if (ret < 0)
> + goto err_gpio;
> +
> + if (type == IIO_VOLTAGE || type == IIO_CURRENT) {
> + for (j = 0; j < BAT_MAX; j++) {
> + generic_bat.bat[j].channel_index = k;
> + generic_bat.bat[j].channels[k] = channels[i];
> + }
> + k++;
> + }
> + continue;
> + }
> +
> + INIT_DELAYED_WORK(&bat_work, generic_adc_bat_work);
> +
> + if (pdata->gpio_charge_finished >= 0) {
> + ret = gpio_request(pdata->gpio_charge_finished, "charged");
> + if (ret)
> + goto err_gpio;
> +
> + ret = request_irq(gpio_to_irq(pdata->gpio_charge_finished),
> + generic_adc_bat_charged,
> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> + "battery charged", NULL);
> + if (ret)
> + goto err_gpio;
> + }
> +
> + dev_info(&pdev->dev, "successfully loaded\n");
> +
> + /* Schedule timer to check current status */
> + schedule_delayed_work(&bat_work,
> + msecs_to_jiffies(JITTER_DELAY));
> +
> + iio_channel_release_all(channels);
> +
> + return 0;
> +
> +err_gpio:
> + iio_channel_release_all(channels);
> +err_reg_backup:
> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> + power_supply_unregister(&generic_bat.bat[i].psy);
> +err_reg_main:
> + return ret;
> +}
> +
> +static int generic_adc_bat_remove(struct platform_device *pdev)
> +{
> + int i;
> + struct generic_platform_data *pdata = pdev->dev.platform_data;
> +
> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> + power_supply_unregister(&generic_bat.bat[i].psy);
> +
> + if (pdata->gpio_charge_finished >= 0) {
> + free_irq(gpio_to_irq(pdata->gpio_charge_finished), NULL);
> + gpio_free(pdata->gpio_charge_finished);
> + }
> +
> + cancel_delayed_work(&bat_work);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int generic_adc_bat_suspend(struct platform_device *pdev,
> + pm_message_t state)
> +{
> + struct generic_platform_data *pdata = pdev->dev.platform_data;
> + struct generic_bat *bat = container_of(pdata,
> + struct generic_bat, pdata);
> +
> + cancel_delayed_work_sync(&bat_work);
> + bat->status = POWER_SUPPLY_STATUS_UNKNOWN;
> +
> + return 0;
> +}
> +
> +static int generic_adc_bat_resume(struct platform_device *pdev)
> +{
> + /* Schedule timer to check current status */
> + schedule_delayed_work(&bat_work,
> + msecs_to_jiffies(JITTER_DELAY));
> +
> + return 0;
> +}
> +#else
> +#define generic_adc_bat_suspend NULL
> +#define generic_adc_bat_resume NULL
> +#endif
> +
> +static struct platform_driver generic_adc_bat_driver = {
> + .driver = {
> + .name = "generic-adc-battery",
> + },
> + .probe = generic_adc_bat_probe,
> + .remove = generic_adc_bat_remove,
> + .suspend = generic_adc_bat_suspend,
> + .resume = generic_adc_bat_resume,
> +};
> +
> +module_platform_driver(generic_adc_bat_driver);
> +
> +MODULE_AUTHOR("anish kumar <anish198519851985@gmail.com>");
> +MODULE_DESCRIPTION("generic battery driver using IIO");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/power/generic-battery.h b/include/linux/power/generic-battery.h
> new file mode 100644
> index 0000000..7a43aa0
> --- /dev/null
> +++ b/include/linux/power/generic-battery.h
> @@ -0,0 +1,26 @@
> +/*
> + * 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_BATTERY_H
> +#define GENERIC_BATTERY_H
> +
> +#include <linux/power_supply.h>
> +
> +/**
> + * struct generic_platform_data - platform data for generic battery
> + * @battery_info: Information about the battery
> + * @cal_charge: platform callback to calculate charge
> + * @gpio_charge_finished: GPIO number used for interrupts
> + * @gpio_inverted: Is the gpio inverted?
> + */
> +struct generic_platform_data {
> + struct power_supply_info battery_info;
> + int (*cal_charge)(long);
> + int gpio_charge_finished;
> + int gpio_inverted;
> +};
> +
> +#endif /* GENERIC_BATTERY_H */
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Just Curious what is the use of this module? Is there any user space
program uses this? When ACPI drivers for battery is available how
useful will this be?
Thanks,
Sannu K
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-02 16:14 ` Sannu K
@ 2012-09-02 21:34 ` Anton Vorontsov
2012-09-04 15:05 ` anish kumar
0 siblings, 1 reply; 19+ messages in thread
From: Anton Vorontsov @ 2012-09-02 21:34 UTC (permalink / raw)
To: Sannu K; +Cc: anish kumar, jic23, linux-iio, dwmw2, linux-kernel
On Sun, Sep 02, 2012 at 09:44:04PM +0530, Sannu K wrote: [...]
> Just Curious what is the use of this module? Is there any user space
> program uses this? When ACPI drivers for battery is available how
> useful will this be?
That's mostly for embedded devices. They often have batteries directly
connected to ADC (analog to digital converters) channels, so that's the
only way to read e.g. voltage.
As for userspace, since it's battery class driver, pretty much every
userspace stack can use it.
Thanks,
Anton.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-02 21:34 ` Anton Vorontsov
@ 2012-09-04 15:05 ` anish kumar
0 siblings, 0 replies; 19+ messages in thread
From: anish kumar @ 2012-09-04 15:05 UTC (permalink / raw)
Cc: Sannu K, jic23, linux-iio, dwmw2, linux-kernel, Anton Vorontsov
On Sun, 2012-09-02 at 14:34 -0700, Anton Vorontsov wrote:
> On Sun, Sep 02, 2012 at 09:44:04PM +0530, Sannu K wrote: [...]
> > Just Curious what is the use of this module? Is there any user space
> > program uses this? When ACPI drivers for battery is available how
> > useful will this be?
>
> That's mostly for embedded devices. They often have batteries directly
> connected to ADC (analog to digital converters) channels, so that's the
> only way to read e.g. voltage.
>
> As for userspace, since it's battery class driver, pretty much every
> userspace stack can use it.
Can I get some review comments on this?
>
> Thanks,
> Anton.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-02 15:39 anish kumar
2012-09-02 16:14 ` Sannu K
@ 2012-09-07 7:52 ` Jonathan Cameron
2012-09-08 7:10 ` anish kumar
2012-09-07 8:49 ` Lars-Peter Clausen
2 siblings, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2012-09-07 7:52 UTC (permalink / raw)
To: anish kumar; +Cc: linux-iio, cbou, dwmw2, linux-kernel
On 02/09/12 16:39, anish kumar wrote:
> From: anish kumar <anish198519851985@gmail.com>
>
> This patch is to use IIO to write a generic batttery driver.
battery
> There are some inherent assumptions here:
> 1.User is having both main battery and backup battery.
Seems rather like that could be easily enough relaxed or configured via
platform data?
> 2.Both batteries use same channel to get the information.
Do you mean same actual channel (physical pin) or same ADC
(with appropriate mux etc)?
>
Mostly fine. There are a few corners I didn't understand as I don't
have time to dive into the battery framework in the near future. Will
leave it up to those more familiar with that side of things!
My only real issue is that it would be cleaner to do the individual
channels by name rather than a get all as you 'know' here how many
channels are expected.
Also don't release the IIO channels until you are actually finished
using them as holding them should prevent the ADC module from going away
underneath you (which is nasty :)
Sorry it took so long to get back to you on this.
So what happened in 1985?
Jonathan
> Signed-off-by: anish kumar <anish198519851985@gmail.com>
> ---
> drivers/power/Kconfig | 8 +
> drivers/power/Makefile | 1 +
> drivers/power/generic-battery.c | 374 +++++++++++++++++++++++++++++++++
> include/linux/power/generic-battery.h | 26 +++
> 4 files changed, 409 insertions(+), 0 deletions(-)
> create mode 100644 drivers/power/generic-battery.c
> create mode 100644 include/linux/power/generic-battery.h
>
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index fcc1bb0..546e86b 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -309,6 +309,14 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
> help
> Say Y to enable battery temperature measurements using
> thermistor connected on BATCTRL ADC.
> +
> +config GENERIC_BATTERY
> + tristate "Generic battery support using IIO"
> + depends on IIO
> + help
> + Say Y here to enable support for the generic battery driver
> + which uses IIO framework to read adc for it's main and backup
ADC
> + battery.
> endif # POWER_SUPPLY
>
> source "drivers/power/avs/Kconfig"
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index ee58afb..8284f9c 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -45,3 +45,4 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
> obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
> obj-$(CONFIG_POWER_AVS) += avs/
> obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
> +obj-$(CONFIG_GENERIC_BATTERY) += generic-battery.o
> diff --git a/drivers/power/generic-battery.c b/drivers/power/generic-battery.c
> new file mode 100644
> index 0000000..33170b7
> --- /dev/null
> +++ b/drivers/power/generic-battery.c
> @@ -0,0 +1,374 @@
> +/*
> + * Generice battery driver code using IIO
Generic
> + * Copyright (C) 2012, Anish Kumar <anish198519851985@gmail.com>
> + * 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.
> + *
bonus blank line to remove
> + */
> +
> +#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>
> +
> +#include <linux/power/generic-battery.h>
> +
> +#define BAT_POLL_INTERVAL 10000 /* ms */
> +#define JITTER_DELAY 500 /* ms */
Elements to make configurable?
> +
> +enum BAT {
> + MAIN_BAT = 0,
> + BACKUP_BAT,
> + BAT_MAX,
> +};
> +
Document this please. It's not immediately obvious what
channel_index is.
Why not just have a direct pointer to the correct channel?
> +struct generic_adc_bat {
> + struct power_supply psy;
> + struct iio_channel *channels;
> + int channel_index;
> +};
> +
> +struct generic_bat {
Spacing is a bit random in here
> + struct generic_adc_bat bat[BAT_MAX];
> + struct generic_platform_data pdata;
> + int volt_value;
> + int cur_value;
> + unsigned int timestamp;
> + int level;
> + int status;
> + int cable_plugged:1;
> +};
> +
> +static struct generic_bat generic_bat = {
> + .bat[MAIN_BAT] = {
> + .psy.name = "main-bat",
> + },
> + .bat[BACKUP_BAT] = {
> + .psy.name = "backup-bat",
> + },
> +};
> +
> +static struct delayed_work bat_work;
> +
> +static void generic_adc_bat_ext_power_changed(struct power_supply *psy)
> +{
> + schedule_delayed_work(&bat_work,
> + msecs_to_jiffies(JITTER_DELAY));
> +}
> +
> +static enum power_supply_property generic_adc_main_bat_props[] = {
> + 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,
> +};
> +
> +static int charge_finished(struct generic_bat *bat)
> +{
> + return bat->pdata.gpio_inverted ?
> + !gpio_get_value(bat->pdata.gpio_charge_finished) :
> + gpio_get_value(bat->pdata.gpio_charge_finished);
> +}
> +
> +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;
> + struct generic_bat *bat;
> + int ret, scaleint, scalepart, iio_val;
> + long result = 0;
> +
> + if (!strcmp(psy->name, "main-battery")) {
> + adc_bat = container_of(psy,
> + struct generic_adc_bat, psy);
This first statement is I think shared so move it out of the if / else
> + bat = container_of(adc_bat,
> + struct generic_bat, bat[MAIN_BAT]);
> + } else if (!strcmp(psy->name, "backup-battery")) {
> + adc_bat = container_of(psy, struct generic_adc_bat, psy);
> + bat = container_of(adc_bat,
> + struct generic_bat, bat[BACKUP_BAT]);
> + } else {
> + /* Ideally this should never happen */
> + WARN_ON(1);
> + return -EINVAL;
> + }
> +
> + if (!bat) {
> + dev_err(psy->dev, "no battery infos ?!\n");
> + return -EINVAL;
> + }
> + ret = iio_read_channel_raw(&adc_bat->channels[adc_bat->channel_index],
> + &iio_val);
> + ret = iio_read_channel_scale(&adc_bat->channels[adc_bat->channel_index],
> + &scaleint, &scalepart);
> + if (ret < 0)
> + return ret;
> +
A quick comment here on what the battery framework is expecting vs IIO
supplying would be good. It may be that this particular output is one
that we should lift over into the core rather than end up with multiple
drivers doing the same thing.
> + 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;
> + }
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_STATUS:
> + if (bat->pdata.gpio_charge_finished < 0)
> + val->intval = bat->level == 100000 ?
> + POWER_SUPPLY_STATUS_FULL : bat->status;
> + else
> + val->intval = bat->status;
> + return 0;
> + case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
> + val->intval = 0;
> + return 0;
> + case POWER_SUPPLY_PROP_CHARGE_NOW:
> + val->intval = bat->pdata.cal_charge(result);
> + return 0;
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + val->intval = result;
> + return 0;
> + case POWER_SUPPLY_PROP_CURRENT_NOW:
> + val->intval = result;
> + return 0;
why return from these and break from the later ones (to return much the
same I think...)?
> + case POWER_SUPPLY_PROP_TECHNOLOGY:
> + val->intval = bat->pdata.battery_info.technology;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> + val->intval = bat->pdata.battery_info.voltage_min_design;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> + val->intval = bat->pdata.battery_info.voltage_max_design;
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> + val->intval = bat->pdata.battery_info.charge_full_design;
> + break;
> + case POWER_SUPPLY_PROP_MODEL_NAME:
> + val->strval = bat->pdata.battery_info.name;
> + break;
> + default:
> + return -EINVAL;
> + }
> + return ret;
> +}
> +
> +static void generic_adc_bat_work(struct work_struct *work)
> +{
> + struct generic_bat *bat = &generic_bat;
> + int is_charged;
> + int is_plugged;
> + static int was_plugged;
That's nasty. Prevents even the theoretical posibility of multiple
copies of the driver. That should be in the device specific data.
> +
> + /* backup battery doesn't have current monitoring capability anyway */
Always or in this particular configuration?
> + is_plugged = power_supply_am_i_supplied(&bat->bat[MAIN_BAT].psy);
> + bat->cable_plugged = is_plugged;
> + if (is_plugged != was_plugged) {
> + was_plugged = is_plugged;
> + if (is_plugged)
> + bat->status = POWER_SUPPLY_STATUS_CHARGING;
> + else
> + bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
> + } else {
> + if ((bat->pdata.gpio_charge_finished >= 0) && is_plugged) {
> + is_charged = charge_finished(&generic_bat);
> + if (is_charged)
> + bat->status = POWER_SUPPLY_STATUS_FULL;
> + else
> + bat->status = POWER_SUPPLY_STATUS_CHARGING;
> + }
> + }
> +
> + power_supply_changed(&bat->bat[MAIN_BAT].psy);
> +}
> +
> +static irqreturn_t generic_adc_bat_charged(int irq, void *dev_id)
> +{
> + schedule_delayed_work(&bat_work,
> + msecs_to_jiffies(JITTER_DELAY));
> + return IRQ_HANDLED;
> +}
> +
> +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
> +{
> + struct generic_platform_data *pdata = pdev->dev.platform_data;
> + struct iio_channel *channels;
> + int num_channels = 0;
> + int ret, i, j, k = 0;
> + enum iio_chan_type type;
> +
> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++) {
> + generic_bat.bat[i].psy.type = POWER_SUPPLY_TYPE_BATTERY;
> + generic_bat.bat[i].psy.properties =
> + generic_adc_main_bat_props;
> + generic_bat.bat[i].psy.num_properties =
> + ARRAY_SIZE(generic_adc_main_bat_props);
> + generic_bat.bat[i].psy.get_property =
> + generic_adc_bat_get_property;
> + generic_bat.bat[i].psy.external_power_changed =
> + generic_adc_bat_ext_power_changed;
Could you do this with a static const array? Looks like there is
nothing dynamic in here and that would make for cleaner code to read.
Given other elements of psy are clearly dynamic (dev pointer) you will
have to memcpy the static version in which is tedious. Still easier
to read though in my view.
> + }
> +
> + generic_bat.volt_value = -1;
> + generic_bat.cur_value = -1;
> + generic_bat.cable_plugged = 0;
> + generic_bat.status = POWER_SUPPLY_STATUS_DISCHARGING;
> +
> + memcpy(&generic_bat.pdata, pdata, sizeof(struct generic_platform_data));
> +
Is there anything dynamic in the pdata? If not it would be cleaner just
to use a pointer to it rather than make a copy (I can't immediately spot
anything but them I'm not familiar with the battery
side of things.
> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++) {
> + ret = power_supply_register(&pdev->dev,
> + &generic_bat.bat[i].psy);
> + if (ret)
> + goto err_reg_main;
> + }
> +
> + channels = iio_channel_get_all(dev_name(&pdev->dev));
I would give these explicit names and get the two individual channels by
name. I think it will give you cleaner code.
> + if (IS_ERR(channels)) {
> + ret = PTR_ERR(channels);
> + goto err_reg_backup;
> + }
> +
> + while (channels[num_channels].indio_dev)
> + num_channels++;
> +
> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> + generic_bat.bat[i].channels = kzalloc(sizeof(struct iio_channel)
> + * BAT_MAX, GFP_KERNEL);
> +
> + /* assuming main battery and backup battery is using the same channel */
> + for (i = 0; i < num_channels; i++) {
> + ret = iio_get_channel_type(&channels[i], &type);
Using named channels you should 'know' you have the correct type. You
can of course check if you don't trust your platform data though!
> + if (ret < 0)
> + goto err_gpio;
> +
> + if (type == IIO_VOLTAGE || type == IIO_CURRENT) {
> + for (j = 0; j < BAT_MAX; j++) {
> + generic_bat.bat[j].channel_index = k;
> + generic_bat.bat[j].channels[k] = channels[i];
> + }
> + k++;
> + }
> + continue;
> + }
> +
> + INIT_DELAYED_WORK(&bat_work, generic_adc_bat_work);
> +
> + if (pdata->gpio_charge_finished >= 0) {
> + ret = gpio_request(pdata->gpio_charge_finished, "charged");
> + if (ret)
> + goto err_gpio;
> +
> + ret = request_irq(gpio_to_irq(pdata->gpio_charge_finished),
> + generic_adc_bat_charged,
> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> + "battery charged", NULL);
> + if (ret)
> + goto err_gpio;
> + }
> +
> + dev_info(&pdev->dev, "successfully loaded\n");
> +
> + /* Schedule timer to check current status */
> + schedule_delayed_work(&bat_work,
> + msecs_to_jiffies(JITTER_DELAY));
> +
> + iio_channel_release_all(channels);
Not if you want to prevent the adc driver from being removed from under
you. They should only be released in the driver removal.
> +
> + return 0;
> +
> +err_gpio:
> + iio_channel_release_all(channels);
> +err_reg_backup:
> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> + power_supply_unregister(&generic_bat.bat[i].psy);
> +err_reg_main:
> + return ret;
> +}
> +
> +static int generic_adc_bat_remove(struct platform_device *pdev)
> +{
> + int i;
> + struct generic_platform_data *pdata = pdev->dev.platform_data;
> +
> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> + power_supply_unregister(&generic_bat.bat[i].psy);
You should release the IIO channels here when they are no longer needed.
> +
> + if (pdata->gpio_charge_finished >= 0) {
I forget, is a gpio index of 0 definitely invalid?
> + free_irq(gpio_to_irq(pdata->gpio_charge_finished), NULL);
> + gpio_free(pdata->gpio_charge_finished);
> + }
> +
> + cancel_delayed_work(&bat_work);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int generic_adc_bat_suspend(struct platform_device *pdev,
> + pm_message_t state)
> +{
> + struct generic_platform_data *pdata = pdev->dev.platform_data;
> + struct generic_bat *bat = container_of(pdata,
> + struct generic_bat, pdata);
> +
> + cancel_delayed_work_sync(&bat_work);
> + bat->status = POWER_SUPPLY_STATUS_UNKNOWN;
> +
> + return 0;
> +}
> +
> +static int generic_adc_bat_resume(struct platform_device *pdev)
> +{
> + /* Schedule timer to check current status */
> + schedule_delayed_work(&bat_work,
> + msecs_to_jiffies(JITTER_DELAY));
> +
> + return 0;
> +}
> +#else
> +#define generic_adc_bat_suspend NULL
> +#define generic_adc_bat_resume NULL
> +#endif
> +
> +static struct platform_driver generic_adc_bat_driver = {
> + .driver = {
> + .name = "generic-adc-battery",
> + },
> + .probe = generic_adc_bat_probe,
> + .remove = generic_adc_bat_remove,
> + .suspend = generic_adc_bat_suspend,
> + .resume = generic_adc_bat_resume,
> +};
> +
> +module_platform_driver(generic_adc_bat_driver);
> +
> +MODULE_AUTHOR("anish kumar <anish198519851985@gmail.com>");
> +MODULE_DESCRIPTION("generic battery driver using IIO");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/power/generic-battery.h b/include/linux/power/generic-battery.h
> new file mode 100644
> index 0000000..7a43aa0
> --- /dev/null
> +++ b/include/linux/power/generic-battery.h
> @@ -0,0 +1,26 @@
> +/*
> + * 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_BATTERY_H
> +#define GENERIC_BATTERY_H
> +
> +#include <linux/power_supply.h>
> +
> +/**
> + * struct generic_platform_data - platform data for generic battery
A little inconsistent on capitalization. (might as well do it now or
one of those people who delight in trivial patches will 'tidy' it
up for you :)
> + * @battery_info: Information about the battery
> + * @cal_charge: platform callback to calculate charge
> + * @gpio_charge_finished: GPIO number used for interrupts
> + * @gpio_inverted: Is the gpio inverted?
> + */
> +struct generic_platform_data {
> + struct power_supply_info battery_info;
> + int (*cal_charge)(long);
> + int gpio_charge_finished;
> + int gpio_inverted;
> +};
> +
> +#endif /* GENERIC_BATTERY_H */
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-07 7:52 ` Jonathan Cameron
@ 2012-09-08 7:10 ` anish kumar
2012-09-08 8:34 ` Anton Vorontsov
2012-09-08 10:09 ` Jonathan Cameron
0 siblings, 2 replies; 19+ messages in thread
From: anish kumar @ 2012-09-08 7:10 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, cbou, dwmw2, linux-kernel, lars
Thanks for your time.
On Fri, 2012-09-07 at 08:52 +0100, Jonathan Cameron wrote:
> On 02/09/12 16:39, anish kumar wrote:
> > From: anish kumar <anish198519851985@gmail.com>
> >
> > This patch is to use IIO to write a generic batttery driver.
> battery
> > There are some inherent assumptions here:
> > 1.User is having both main battery and backup battery.
> Seems rather like that could be easily enough relaxed or configured via
> platform data?
Yes
> > 2.Both batteries use same channel to get the information.
> Do you mean same actual channel (physical pin) or same ADC
> (with appropriate mux etc)?
As lars pointed out it would be better if we have multiple channels
per battery.The channel what I am referring here is the name of
different channels which will be exported by adc driver to get
voltage, current and power.
> >
> Mostly fine. There are a few corners I didn't understand as I don't
> have time to dive into the battery framework in the near future. Will
> leave it up to those more familiar with that side of things!
>
> My only real issue is that it would be cleaner to do the individual
> channels by name rather than a get all as you 'know' here how many
> channels are expected.
agreed.
>
> Also don't release the IIO channels until you are actually finished
> using them as holding them should prevent the ADC module from going away
> underneath you (which is nasty :)
agreed.
>
> Sorry it took so long to get back to you on this.
That is the biggest advantage of working in open-source.
We can take our own time.
>
> So what happened in 1985?
Was hurrying to get an email id :)
>
> Jonathan
> > Signed-off-by: anish kumar <anish198519851985@gmail.com>
> > ---
> > drivers/power/Kconfig | 8 +
> > drivers/power/Makefile | 1 +
> > drivers/power/generic-battery.c | 374 +++++++++++++++++++++++++++++++++
> > include/linux/power/generic-battery.h | 26 +++
> > 4 files changed, 409 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/power/generic-battery.c
> > create mode 100644 include/linux/power/generic-battery.h
> >
> > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> > index fcc1bb0..546e86b 100644
> > --- a/drivers/power/Kconfig
> > +++ b/drivers/power/Kconfig
> > @@ -309,6 +309,14 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
> > help
> > Say Y to enable battery temperature measurements using
> > thermistor connected on BATCTRL ADC.
> > +
> > +config GENERIC_BATTERY
> > + tristate "Generic battery support using IIO"
> > + depends on IIO
> > + help
> > + Say Y here to enable support for the generic battery driver
> > + which uses IIO framework to read adc for it's main and backup
> ADC
> > + battery.
> > endif # POWER_SUPPLY
> >
> > source "drivers/power/avs/Kconfig"
> > diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> > index ee58afb..8284f9c 100644
> > --- a/drivers/power/Makefile
> > +++ b/drivers/power/Makefile
> > @@ -45,3 +45,4 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
> > obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
> > obj-$(CONFIG_POWER_AVS) += avs/
> > obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
> > +obj-$(CONFIG_GENERIC_BATTERY) += generic-battery.o
> > diff --git a/drivers/power/generic-battery.c b/drivers/power/generic-battery.c
> > new file mode 100644
> > index 0000000..33170b7
> > --- /dev/null
> > +++ b/drivers/power/generic-battery.c
> > @@ -0,0 +1,374 @@
> > +/*
> > + * Generice battery driver code using IIO
> Generic
> > + * Copyright (C) 2012, Anish Kumar <anish198519851985@gmail.com>
> > + * 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.
> > + *
> bonus blank line to remove
> > + */
> > +
> > +#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>
> > +
> > +#include <linux/power/generic-battery.h>
> > +
> > +#define BAT_POLL_INTERVAL 10000 /* ms */
> > +#define JITTER_DELAY 500 /* ms */
> Elements to make configurable?
Yes probably make it platform data?
> > +
> > +enum BAT {
> > + MAIN_BAT = 0,
> > + BACKUP_BAT,
> > + BAT_MAX,
> > +};
> > +
> Document this please. It's not immediately obvious what
> channel_index is.
This will be removed as we will be using only one device.
>
> Why not just have a direct pointer to the correct channel?
> > +struct generic_adc_bat {
> > + struct power_supply psy;
> > + struct iio_channel *channels;
> > + int channel_index;
> > +};
> > +
> > +struct generic_bat {
> Spacing is a bit random in here
> > + struct generic_adc_bat bat[BAT_MAX];
> > + struct generic_platform_data pdata;
> > + int volt_value;
> > + int cur_value;
> > + unsigned int timestamp;
> > + int level;
> > + int status;
> > + int cable_plugged:1;
> > +};
> > +
> > +static struct generic_bat generic_bat = {
> > + .bat[MAIN_BAT] = {
> > + .psy.name = "main-bat",
> > + },
> > + .bat[BACKUP_BAT] = {
> > + .psy.name = "backup-bat",
> > + },
> > +};
> > +
> > +static struct delayed_work bat_work;
> > +
> > +static void generic_adc_bat_ext_power_changed(struct power_supply *psy)
> > +{
> > + schedule_delayed_work(&bat_work,
> > + msecs_to_jiffies(JITTER_DELAY));
> > +}
> > +
> > +static enum power_supply_property generic_adc_main_bat_props[] = {
> > + 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,
> > +};
> > +
> > +static int charge_finished(struct generic_bat *bat)
> > +{
> > + return bat->pdata.gpio_inverted ?
> > + !gpio_get_value(bat->pdata.gpio_charge_finished) :
> > + gpio_get_value(bat->pdata.gpio_charge_finished);
> > +}
> > +
> > +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;
> > + struct generic_bat *bat;
> > + int ret, scaleint, scalepart, iio_val;
> > + long result = 0;
> > +
> > + if (!strcmp(psy->name, "main-battery")) {
> > + adc_bat = container_of(psy,
> > + struct generic_adc_bat, psy);
> This first statement is I think shared so move it out of the if / else
This whole logic will change as we will be using only one device and
if has user has multiple batteries then he can register multiple
devices.
>
> > + bat = container_of(adc_bat,
> > + struct generic_bat, bat[MAIN_BAT]);
> > + } else if (!strcmp(psy->name, "backup-battery")) {
> > + adc_bat = container_of(psy, struct generic_adc_bat, psy);
> > + bat = container_of(adc_bat,
> > + struct generic_bat, bat[BACKUP_BAT]);
> > + } else {
> > + /* Ideally this should never happen */
> > + WARN_ON(1);
> > + return -EINVAL;
> > + }
> > +
> > + if (!bat) {
> > + dev_err(psy->dev, "no battery infos ?!\n");
> > + return -EINVAL;
> > +
> > + ret = iio_read_channel_raw(&adc_bat->channels[adc_bat->channel_index],
> > + &iio_val);
> > + ret = iio_read_channel_scale(&adc_bat->channels[adc_bat->channel_index],
> > + &scaleint, &scalepart);
> > + if (ret < 0)
> > + return ret;
> > +
> A quick comment here on what the battery framework is expecting vs IIO
> supplying would be good. It may be that this particular output is one
> that we should lift over into the core rather than end up with multiple
> drivers doing the same thing.
I think for this we need to get all the channels supported by a
particular adc device and once we get the information regarding the
channel(current/voltage/power) supported we can find out "What
information is exported about this channel which may include scale or
raw adc value".This can be used to call the corresponding API's.
>
> > + 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;
> > + }
> > +
> > + switch (psp) {
> > + case POWER_SUPPLY_PROP_STATUS:
> > + if (bat->pdata.gpio_charge_finished < 0)
> > + val->intval = bat->level == 100000 ?
> > + POWER_SUPPLY_STATUS_FULL : bat->status;
> > + else
> > + val->intval = bat->status;
> > + return 0;
> > + case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
> > + val->intval = 0;
> > + return 0;
> > + case POWER_SUPPLY_PROP_CHARGE_NOW:
> > + val->intval = bat->pdata.cal_charge(result);
> > + return 0;
> > + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > + val->intval = result;
> > + return 0;
> > + case POWER_SUPPLY_PROP_CURRENT_NOW:
> > + val->intval = result;
> > + return 0;
> why return from these and break from the later ones (to return much the
> same I think...)?
my mistake.
> > + case POWER_SUPPLY_PROP_TECHNOLOGY:
> > + val->intval = bat->pdata.battery_info.technology;
> > + break;
> > + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> > + val->intval = bat->pdata.battery_info.voltage_min_design;
> > + break;
> > + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> > + val->intval = bat->pdata.battery_info.voltage_max_design;
> > + break;
> > + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> > + val->intval = bat->pdata.battery_info.charge_full_design;
> > + break;
> > + case POWER_SUPPLY_PROP_MODEL_NAME:
> > + val->strval = bat->pdata.battery_info.name;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + return ret;
> > +}
> > +
> > +static void generic_adc_bat_work(struct work_struct *work)
> > +{
> > + struct generic_bat *bat = &generic_bat;
> > + int is_charged;
> > + int is_plugged;
> > + static int was_plugged;
> That's nasty. Prevents even the theoretical posibility of multiple
> copies of the driver. That should be in the device specific data.
yes should be part of device specific data.
> > +
> > + /* backup battery doesn't have current monitoring capability anyway */
> Always or in this particular configuration?
I was told by Anton Vorontsov.Anyway now this driver will be per device
so shouldn't matter.
>
> > + is_plugged = power_supply_am_i_supplied(&bat->bat[MAIN_BAT].psy);
> > + bat->cable_plugged = is_plugged;
> > + if (is_plugged != was_plugged) {
> > + was_plugged = is_plugged;
> > + if (is_plugged)
> > + bat->status = POWER_SUPPLY_STATUS_CHARGING;
> > + else
> > + bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
> > + } else {
> > + if ((bat->pdata.gpio_charge_finished >= 0) && is_plugged) {
> > + is_charged = charge_finished(&generic_bat);
> > + if (is_charged)
> > + bat->status = POWER_SUPPLY_STATUS_FULL;
> > + else
> > + bat->status = POWER_SUPPLY_STATUS_CHARGING;
> > + }
> > + }
> > +
> > + power_supply_changed(&bat->bat[MAIN_BAT].psy);
> > +}
> > +
> > +static irqreturn_t generic_adc_bat_charged(int irq, void *dev_id)
> > +{
> > + schedule_delayed_work(&bat_work,
> > + msecs_to_jiffies(JITTER_DELAY));
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
> > +{
> > + struct generic_platform_data *pdata = pdev->dev.platform_data;
> > + struct iio_channel *channels;
> > + int num_channels = 0;
> > + int ret, i, j, k = 0;
> > + enum iio_chan_type type;
> > +
> > + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++) {
> > + generic_bat.bat[i].psy.type = POWER_SUPPLY_TYPE_BATTERY;
> > + generic_bat.bat[i].psy.properties =
> > + generic_adc_main_bat_props;
> > + generic_bat.bat[i].psy.num_properties =
> > + ARRAY_SIZE(generic_adc_main_bat_props);
> > + generic_bat.bat[i].psy.get_property =
> > + generic_adc_bat_get_property;
> > + generic_bat.bat[i].psy.external_power_changed =
> > + generic_adc_bat_ext_power_changed;
> Could you do this with a static const array? Looks like there is
> nothing dynamic in here and that would make for cleaner code to read.
> Given other elements of psy are clearly dynamic (dev pointer) you will
> have to memcpy the static version in which is tedious. Still easier
> to read though in my view.
> > + }
right.
> > +
> > + generic_bat.volt_value = -1;
> > + generic_bat.cur_value = -1;
> > + generic_bat.cable_plugged = 0;
> > + generic_bat.status = POWER_SUPPLY_STATUS_DISCHARGING;
> > +
> > + memcpy(&generic_bat.pdata, pdata, sizeof(struct generic_platform_data));
> > +
> Is there anything dynamic in the pdata? If not it would be cleaner just
> to use a pointer to it rather than make a copy (I can't immediately spot
> anything but them I'm not familiar with the battery
> side of things.
We can just assign it to generic_bat so that it can be used in the
relevant functions.Otherwise we need to use dev_set_drvdata and
dev_get_drvdata.
> > + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++) {
> > + ret = power_supply_register(&pdev->dev,
> > + &generic_bat.bat[i].psy);
> > + if (ret)
> > + goto err_reg_main;
> > + }
> > +
> > + channels = iio_channel_get_all(dev_name(&pdev->dev));
> I would give these explicit names and get the two individual channels by
> name. I think it will give you cleaner code.
Yes, now it will be based on pdata->voltage_channel,
pdata->current_channel and so on.We will use this to get the channel.
> > + if (IS_ERR(channels)) {
> > + ret = PTR_ERR(channels);
> > + goto err_reg_backup;
> > + }
> > +
> > + while (channels[num_channels].indio_dev)
> > + num_channels++;
> > +
> > + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> > + generic_bat.bat[i].channels = kzalloc(sizeof(struct iio_channel)
> > + * BAT_MAX, GFP_KERNEL);
> > +
> > + /* assuming main battery and backup battery is using the same channel */
> > + for (i = 0; i < num_channels; i++) {
> > + ret = iio_get_channel_type(&channels[i], &type);
> Using named channels you should 'know' you have the correct type. You
> can of course check if you don't trust your platform data though!
right.
> > + if (ret < 0)
> > + goto err_gpio;
> > +
> > + if (type == IIO_VOLTAGE || type == IIO_CURRENT) {
> > + for (j = 0; j < BAT_MAX; j++) {
> > + generic_bat.bat[j].channel_index = k;
> > + generic_bat.bat[j].channels[k] = channels[i];
> > + }
> > + k++;
> > + }
> > + continue;
> > + }
> > +
> > + INIT_DELAYED_WORK(&bat_work, generic_adc_bat_work);
> > +
> > + if (pdata->gpio_charge_finished >= 0) {
> > + ret = gpio_request(pdata->gpio_charge_finished, "charged");
> > + if (ret)
> > + goto err_gpio;
> > +
> > + ret = request_irq(gpio_to_irq(pdata->gpio_charge_finished),
> > + generic_adc_bat_charged,
> > + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> > + "battery charged", NULL);
> > + if (ret)
> > + goto err_gpio;
> > + }
> > +
> > + dev_info(&pdev->dev, "successfully loaded\n");
> > +
> > + /* Schedule timer to check current status */
> > + schedule_delayed_work(&bat_work,
> > + msecs_to_jiffies(JITTER_DELAY));
> > +
> > + iio_channel_release_all(channels);
> Not if you want to prevent the adc driver from being removed from under
> you. They should only be released in the driver removal.
right.
> > +
> > + return 0;
> > +
> > +err_gpio:
> > + iio_channel_release_all(channels);
> > +err_reg_backup:
> > + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> > + power_supply_unregister(&generic_bat.bat[i].psy);
> > +err_reg_main:
> > + return ret;
> > +}
> > +
> > +static int generic_adc_bat_remove(struct platform_device *pdev)
> > +{
> > + int i;
> > + struct generic_platform_data *pdata = pdev->dev.platform_data;
> > +
> > + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> > + power_supply_unregister(&generic_bat.bat[i].psy);
>
> You should release the IIO channels here when they are no longer needed.
right.
> > +
> > + if (pdata->gpio_charge_finished >= 0) {
> I forget, is a gpio index of 0 definitely invalid?
> > + free_irq(gpio_to_irq(pdata->gpio_charge_finished), NULL);
> > + gpio_free(pdata->gpio_charge_finished);
> > + }
> > +
> > + cancel_delayed_work(&bat_work);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int generic_adc_bat_suspend(struct platform_device *pdev,
> > + pm_message_t state)
> > +{
> > + struct generic_platform_data *pdata = pdev->dev.platform_data;
> > + struct generic_bat *bat = container_of(pdata,
> > + struct generic_bat, pdata);
> > +
> > + cancel_delayed_work_sync(&bat_work);
> > + bat->status = POWER_SUPPLY_STATUS_UNKNOWN;
> > +
> > + return 0;
> > +}
> > +
> > +static int generic_adc_bat_resume(struct platform_device *pdev)
> > +{
> > + /* Schedule timer to check current status */
> > + schedule_delayed_work(&bat_work,
> > + msecs_to_jiffies(JITTER_DELAY));
> > +
> > + return 0;
> > +}
> > +#else
> > +#define generic_adc_bat_suspend NULL
> > +#define generic_adc_bat_resume NULL
> > +#endif
> > +
> > +static struct platform_driver generic_adc_bat_driver = {
> > + .driver = {
> > + .name = "generic-adc-battery",
> > + },
> > + .probe = generic_adc_bat_probe,
> > + .remove = generic_adc_bat_remove,
> > + .suspend = generic_adc_bat_suspend,
> > + .resume = generic_adc_bat_resume,
> > +};
> > +
> > +module_platform_driver(generic_adc_bat_driver);
> > +
> > +MODULE_AUTHOR("anish kumar <anish198519851985@gmail.com>");
> > +MODULE_DESCRIPTION("generic battery driver using IIO");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/power/generic-battery.h b/include/linux/power/generic-battery.h
> > new file mode 100644
> > index 0000000..7a43aa0
> > --- /dev/null
> > +++ b/include/linux/power/generic-battery.h
> > @@ -0,0 +1,26 @@
> > +/*
> > + * 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_BATTERY_H
> > +#define GENERIC_BATTERY_H
> > +
> > +#include <linux/power_supply.h>
> > +
> > +/**
> > + * struct generic_platform_data - platform data for generic battery
> A little inconsistent on capitalization. (might as well do it now or
> one of those people who delight in trivial patches will 'tidy' it
> up for you :)
I don't mind as it is a good start for people to get familiar with the
linux patch process.
However I will address this valuable comments.
> > + * @battery_info: Information about the battery
> > + * @cal_charge: platform callback to calculate charge
> > + * @gpio_charge_finished: GPIO number used for interrupts
> > + * @gpio_inverted: Is the gpio inverted?
> > + */
> > +struct generic_platform_data {
> > + struct power_supply_info battery_info;
> > + int (*cal_charge)(long);
> > + int gpio_charge_finished;
> > + int gpio_inverted;
> > +};
> > +
> > +#endif /* GENERIC_BATTERY_H */
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-08 7:10 ` anish kumar
@ 2012-09-08 8:34 ` Anton Vorontsov
2012-09-08 10:09 ` Jonathan Cameron
1 sibling, 0 replies; 19+ messages in thread
From: Anton Vorontsov @ 2012-09-08 8:34 UTC (permalink / raw)
To: anish kumar; +Cc: Jonathan Cameron, linux-iio, dwmw2, linux-kernel, lars
On Sat, Sep 08, 2012 at 12:40:18PM +0530, anish kumar wrote:
[...]
> > > Signed-off-by: anish kumar <anish198519851985@gmail.com>
> > So what happened in 1985?
> Was hurrying to get an email id :)
I so much understand... ;-)
Much thanks to Lars-Peter and Jonathan, saved me a lot of time reviewing
the driver.
Anish, the idea of generic IIO driver looks great, I'm impatiently
waiting for a new revision!
One suggestion: the file name says generic-battery, the code sometimes
uses generic_adc_battery sometimes generic_battery. I'd suggest being
consistent, and stick to one naming scheme. I'd prefer
generic_adc_battery.
Thanks!
Anton.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-08 7:10 ` anish kumar
2012-09-08 8:34 ` Anton Vorontsov
@ 2012-09-08 10:09 ` Jonathan Cameron
2012-09-09 9:32 ` anish kumar
1 sibling, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2012-09-08 10:09 UTC (permalink / raw)
To: anish kumar; +Cc: linux-iio, cbou, dwmw2, linux-kernel, lars
On 09/08/2012 08:10 AM, anish kumar wrote:
> Thanks for your time.
> On Fri, 2012-09-07 at 08:52 +0100, Jonathan Cameron wrote:
>> On 02/09/12 16:39, anish kumar wrote:
>>> From: anish kumar <anish198519851985@gmail.com>
>>>
>>> This patch is to use IIO to write a generic batttery driver.
>> battery
>>> There are some inherent assumptions here:
>>> 1.User is having both main battery and backup battery.
>> Seems rather like that could be easily enough relaxed or configured via
>> platform data?
> Yes
>>> 2.Both batteries use same channel to get the information.
>> Do you mean same actual channel (physical pin) or same ADC
>> (with appropriate mux etc)?
> As lars pointed out it would be better if we have multiple channels
> per battery.The channel what I am referring here is the name of
> different channels which will be exported by adc driver to get
> voltage, current and power.
I agree entirely with what Lars-Peter said. A separate instance of
the driver per battery will make life much cleaner and allow as you
say for separate channels for voltage, current and power for each one.
I guess it may be a case of trying to get each one for every battery
and continue with whatever turns out to be available.
>>>
>> Mostly fine. There are a few corners I didn't understand as I don't
>> have time to dive into the battery framework in the near future. Will
>> leave it up to those more familiar with that side of things!
>>
>> My only real issue is that it would be cleaner to do the individual
>> channels by name rather than a get all as you 'know' here how many
>> channels are expected.
> agreed.
>>
>> Also don't release the IIO channels until you are actually finished
>> using them as holding them should prevent the ADC module from going away
>> underneath you (which is nasty :)
> agreed.
>>
>> Sorry it took so long to get back to you on this.
> That is the biggest advantage of working in open-source.
> We can take our own time.
:)
>>
>> So what happened in 1985?
> Was hurrying to get an email id :)
And there I was imagining a magical significance to it....
>>
>> Jonathan
>>> Signed-off-by: anish kumar <anish198519851985@gmail.com>
>>> ---
>>> drivers/power/Kconfig | 8 +
>>> drivers/power/Makefile | 1 +
>>> drivers/power/generic-battery.c | 374 +++++++++++++++++++++++++++++++++
>>> include/linux/power/generic-battery.h | 26 +++
>>> 4 files changed, 409 insertions(+), 0 deletions(-)
>>> create mode 100644 drivers/power/generic-battery.c
>>> create mode 100644 include/linux/power/generic-battery.h
>>>
>>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
>>> index fcc1bb0..546e86b 100644
>>> --- a/drivers/power/Kconfig
>>> +++ b/drivers/power/Kconfig
>>> @@ -309,6 +309,14 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
>>> help
>>> Say Y to enable battery temperature measurements using
>>> thermistor connected on BATCTRL ADC.
>>> +
>>> +config GENERIC_BATTERY
>>> + tristate "Generic battery support using IIO"
>>> + depends on IIO
>>> + help
>>> + Say Y here to enable support for the generic battery driver
>>> + which uses IIO framework to read adc for it's main and backup
>> ADC
>>> + battery.
>>> endif # POWER_SUPPLY
>>>
>>> source "drivers/power/avs/Kconfig"
>>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>>> index ee58afb..8284f9c 100644
>>> --- a/drivers/power/Makefile
>>> +++ b/drivers/power/Makefile
>>> @@ -45,3 +45,4 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
>>> obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
>>> obj-$(CONFIG_POWER_AVS) += avs/
>>> obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
>>> +obj-$(CONFIG_GENERIC_BATTERY) += generic-battery.o
>>> diff --git a/drivers/power/generic-battery.c b/drivers/power/generic-battery.c
>>> new file mode 100644
>>> index 0000000..33170b7
>>> --- /dev/null
>>> +++ b/drivers/power/generic-battery.c
>>> @@ -0,0 +1,374 @@
>>> +/*
>>> + * Generice battery driver code using IIO
>> Generic
>>> + * Copyright (C) 2012, Anish Kumar <anish198519851985@gmail.com>
>>> + * 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.
>>> + *
>> bonus blank line to remove
>>> + */
>>> +
>>> +#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>
>>> +
>>> +#include <linux/power/generic-battery.h>
>>> +
>>> +#define BAT_POLL_INTERVAL 10000 /* ms */
>>> +#define JITTER_DELAY 500 /* ms */
>> Elements to make configurable?
> Yes probably make it platform data?
>>> +
>>> +enum BAT {
>>> + MAIN_BAT = 0,
>>> + BACKUP_BAT,
>>> + BAT_MAX,
>>> +};
>>> +
>> Document this please. It's not immediately obvious what
>> channel_index is.
> This will be removed as we will be using only one device.
>>
>> Why not just have a direct pointer to the correct channel?
>>> +struct generic_adc_bat {
>>> + struct power_supply psy;
>>> + struct iio_channel *channels;
>>> + int channel_index;
>>> +};
>>> +
>>> +struct generic_bat {
>> Spacing is a bit random in here
>>> + struct generic_adc_bat bat[BAT_MAX];
>>> + struct generic_platform_data pdata;
>>> + int volt_value;
>>> + int cur_value;
>>> + unsigned int timestamp;
>>> + int level;
>>> + int status;
>>> + int cable_plugged:1;
>>> +};
>>> +
>>> +static struct generic_bat generic_bat = {
>>> + .bat[MAIN_BAT] = {
>>> + .psy.name = "main-bat",
>>> + },
>>> + .bat[BACKUP_BAT] = {
>>> + .psy.name = "backup-bat",
>>> + },
>>> +};
>>> +
>>> +static struct delayed_work bat_work;
>>> +
>>> +static void generic_adc_bat_ext_power_changed(struct power_supply *psy)
>>> +{
>>> + schedule_delayed_work(&bat_work,
>>> + msecs_to_jiffies(JITTER_DELAY));
>>> +}
>>> +
>>> +static enum power_supply_property generic_adc_main_bat_props[] = {
>>> + 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,
>>> +};
>>> +
>>> +static int charge_finished(struct generic_bat *bat)
>>> +{
>>> + return bat->pdata.gpio_inverted ?
>>> + !gpio_get_value(bat->pdata.gpio_charge_finished) :
>>> + gpio_get_value(bat->pdata.gpio_charge_finished);
>>> +}
>>> +
>>> +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;
>>> + struct generic_bat *bat;
>>> + int ret, scaleint, scalepart, iio_val;
>>> + long result = 0;
>>> +
>>> + if (!strcmp(psy->name, "main-battery")) {
>>> + adc_bat = container_of(psy,
>>> + struct generic_adc_bat, psy);
>> This first statement is I think shared so move it out of the if / else
> This whole logic will change as we will be using only one device and
> if has user has multiple batteries then he can register multiple
> devices.
Yes, much cleaner that way.
>>
>>> + bat = container_of(adc_bat,
>>> + struct generic_bat, bat[MAIN_BAT]);
>>> + } else if (!strcmp(psy->name, "backup-battery")) {
>>> + adc_bat = container_of(psy, struct generic_adc_bat, psy);
>>> + bat = container_of(adc_bat,
>>> + struct generic_bat, bat[BACKUP_BAT]);
>>> + } else {
>>> + /* Ideally this should never happen */
>>> + WARN_ON(1);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (!bat) {
>> > + dev_err(psy->dev, "no battery infos ?!\n");
>>> + return -EINVAL;
>>> +
>>> + ret = iio_read_channel_raw(&adc_bat->channels[adc_bat->channel_index],
>>> + &iio_val);
>>> + ret = iio_read_channel_scale(&adc_bat->channels[adc_bat->channel_index],
>>> + &scaleint, &scalepart);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>> A quick comment here on what the battery framework is expecting vs IIO
>> supplying would be good. It may be that this particular output is one
>> that we should lift over into the core rather than end up with multiple
>> drivers doing the same thing.
> I think for this we need to get all the channels supported by a
> particular adc device and once we get the information regarding the
> channel(current/voltage/power) supported we can find out "What
> information is exported about this channel which may include scale or
> raw adc value".This can be used to call the corresponding API's.
Agreed. If I'd read this far I'd never have said the same thing at the top
of this email ;)
>>
>>> + 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;
>>> + }
>>> +
>>> + switch (psp) {
>>> + case POWER_SUPPLY_PROP_STATUS:
>>> + if (bat->pdata.gpio_charge_finished < 0)
>>> + val->intval = bat->level == 100000 ?
>>> + POWER_SUPPLY_STATUS_FULL : bat->status;
>>> + else
>>> + val->intval = bat->status;
>>> + return 0;
>>> + case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
>>> + val->intval = 0;
>>> + return 0;
>>> + case POWER_SUPPLY_PROP_CHARGE_NOW:
>>> + val->intval = bat->pdata.cal_charge(result);
>>> + return 0;
>>> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>>> + val->intval = result;
>>> + return 0;
>>> + case POWER_SUPPLY_PROP_CURRENT_NOW:
>>> + val->intval = result;
>>> + return 0;
>> why return from these and break from the later ones (to return much the
>> same I think...)?
> my mistake.
>>> + case POWER_SUPPLY_PROP_TECHNOLOGY:
>>> + val->intval = bat->pdata.battery_info.technology;
>>> + break;
>>> + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
>>> + val->intval = bat->pdata.battery_info.voltage_min_design;
>>> + break;
>>> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
>>> + val->intval = bat->pdata.battery_info.voltage_max_design;
>>> + break;
>>> + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>>> + val->intval = bat->pdata.battery_info.charge_full_design;
>>> + break;
>>> + case POWER_SUPPLY_PROP_MODEL_NAME:
>>> + val->strval = bat->pdata.battery_info.name;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> + return ret;
>>> +}
>>> +
>>> +static void generic_adc_bat_work(struct work_struct *work)
>>> +{
>>> + struct generic_bat *bat = &generic_bat;
>>> + int is_charged;
>>> + int is_plugged;
>>> + static int was_plugged;
>> That's nasty. Prevents even the theoretical posibility of multiple
>> copies of the driver. That should be in the device specific data.
> yes should be part of device specific data.
>>> +
>>> + /* backup battery doesn't have current monitoring capability anyway */
>> Always or in this particular configuration?
> I was told by Anton Vorontsov.Anyway now this driver will be per device
> so shouldn't matter.
Maybe one day people will stick enough batteries on my phone for it to last
one whole day ;)
>>
>>> + is_plugged = power_supply_am_i_supplied(&bat->bat[MAIN_BAT].psy);
>>> + bat->cable_plugged = is_plugged;
>>> + if (is_plugged != was_plugged) {
>>> + was_plugged = is_plugged;
>>> + if (is_plugged)
>>> + bat->status = POWER_SUPPLY_STATUS_CHARGING;
>>> + else
>>> + bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
>>> + } else {
>>> + if ((bat->pdata.gpio_charge_finished >= 0) && is_plugged) {
>>> + is_charged = charge_finished(&generic_bat);
>>> + if (is_charged)
>>> + bat->status = POWER_SUPPLY_STATUS_FULL;
>>> + else
>>> + bat->status = POWER_SUPPLY_STATUS_CHARGING;
>>> + }
>>> + }
>>> +
>>> + power_supply_changed(&bat->bat[MAIN_BAT].psy);
>>> +}
>>> +
>>> +static irqreturn_t generic_adc_bat_charged(int irq, void *dev_id)
>>> +{
>>> + schedule_delayed_work(&bat_work,
>>> + msecs_to_jiffies(JITTER_DELAY));
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
>>> +{
>>> + struct generic_platform_data *pdata = pdev->dev.platform_data;
>>> + struct iio_channel *channels;
>>> + int num_channels = 0;
>>> + int ret, i, j, k = 0;
>>> + enum iio_chan_type type;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++) {
>>> + generic_bat.bat[i].psy.type = POWER_SUPPLY_TYPE_BATTERY;
>>> + generic_bat.bat[i].psy.properties =
>>> + generic_adc_main_bat_props;
>>> + generic_bat.bat[i].psy.num_properties =
>>> + ARRAY_SIZE(generic_adc_main_bat_props);
>>> + generic_bat.bat[i].psy.get_property =
>>> + generic_adc_bat_get_property;
>>> + generic_bat.bat[i].psy.external_power_changed =
>>> + generic_adc_bat_ext_power_changed;
>> Could you do this with a static const array? Looks like there is
>> nothing dynamic in here and that would make for cleaner code to read.
>> Given other elements of psy are clearly dynamic (dev pointer) you will
>> have to memcpy the static version in which is tedious. Still easier
>> to read though in my view.
>>> + }
> right.
>>> +
>>> + generic_bat.volt_value = -1;
>>> + generic_bat.cur_value = -1;
>>> + generic_bat.cable_plugged = 0;
>>> + generic_bat.status = POWER_SUPPLY_STATUS_DISCHARGING;
>>> +
>>> + memcpy(&generic_bat.pdata, pdata, sizeof(struct generic_platform_data));
>>> +
>> Is there anything dynamic in the pdata? If not it would be cleaner just
>> to use a pointer to it rather than make a copy (I can't immediately spot
>> anything but them I'm not familiar with the battery
>> side of things.
> We can just assign it to generic_bat so that it can be used in the
> relevant functions.Otherwise we need to use dev_set_drvdata and
> dev_get_drvdata.
>>> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++) {
>>> + ret = power_supply_register(&pdev->dev,
>>> + &generic_bat.bat[i].psy);
>>> + if (ret)
>>> + goto err_reg_main;
>>> + }
>>> +
>>> + channels = iio_channel_get_all(dev_name(&pdev->dev));
>> I would give these explicit names and get the two individual channels by
>> name. I think it will give you cleaner code.
> Yes, now it will be based on pdata->voltage_channel,
> pdata->current_channel and so on.We will use this to get the channel.
Maybe better to just do this via the iio_map structures in the platform data
and standard naming for each channel.
"voltage", "current", "power" would do nicely. Note we'll have to add the
relevant naming for the other side to more IIO device drivers via the
'datasheet_name' element of iio_chan_spec. A quick grep shows this
is only done in drivers/staging/iio/max1363_core.c so far (bet you can't
guess what is soldered to my test board :)
>>> + if (IS_ERR(channels)) {
>>> + ret = PTR_ERR(channels);
>>> + goto err_reg_backup;
>>> + }
>>> +
>>> + while (channels[num_channels].indio_dev)
>>> + num_channels++;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
>>> + generic_bat.bat[i].channels = kzalloc(sizeof(struct iio_channel)
>>> + * BAT_MAX, GFP_KERNEL);
>>> +
>>> + /* assuming main battery and backup battery is using the same channel */
>>> + for (i = 0; i < num_channels; i++) {
>>> + ret = iio_get_channel_type(&channels[i], &type);
>> Using named channels you should 'know' you have the correct type. You
>> can of course check if you don't trust your platform data though!
> right.
>>> + if (ret < 0)
>>> + goto err_gpio;
>>> +
>>> + if (type == IIO_VOLTAGE || type == IIO_CURRENT) {
>>> + for (j = 0; j < BAT_MAX; j++) {
>>> + generic_bat.bat[j].channel_index = k;
>>> + generic_bat.bat[j].channels[k] = channels[i];
>>> + }
>>> + k++;
>>> + }
>>> + continue;
>>> + }
>>> +
>>> + INIT_DELAYED_WORK(&bat_work, generic_adc_bat_work);
>>> +
>>> + if (pdata->gpio_charge_finished >= 0) {
>>> + ret = gpio_request(pdata->gpio_charge_finished, "charged");
>>> + if (ret)
>>> + goto err_gpio;
>>> +
>>> + ret = request_irq(gpio_to_irq(pdata->gpio_charge_finished),
>>> + generic_adc_bat_charged,
>>> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>> + "battery charged", NULL);
>>> + if (ret)
>>> + goto err_gpio;
>>> + }
>>> +
>>> + dev_info(&pdev->dev, "successfully loaded\n");
>>> +
>>> + /* Schedule timer to check current status */
>>> + schedule_delayed_work(&bat_work,
>>> + msecs_to_jiffies(JITTER_DELAY));
>>> +
>>> + iio_channel_release_all(channels);
>> Not if you want to prevent the adc driver from being removed from under
>> you. They should only be released in the driver removal.
> right.
>>> +
>>> + return 0;
>>> +
>>> +err_gpio:
>>> + iio_channel_release_all(channels);
>>> +err_reg_backup:
>>> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
>>> + power_supply_unregister(&generic_bat.bat[i].psy);
>>> +err_reg_main:
>>> + return ret;
>>> +}
>>> +
>>> +static int generic_adc_bat_remove(struct platform_device *pdev)
>>> +{
>>> + int i;
>>> + struct generic_platform_data *pdata = pdev->dev.platform_data;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
>>> + power_supply_unregister(&generic_bat.bat[i].psy);
>>
>> You should release the IIO channels here when they are no longer needed.
> right.
>>> +
>>> + if (pdata->gpio_charge_finished >= 0) {
>> I forget, is a gpio index of 0 definitely invalid?
>>> + free_irq(gpio_to_irq(pdata->gpio_charge_finished), NULL);
>>> + gpio_free(pdata->gpio_charge_finished);
>>> + }
>>> +
>>> + cancel_delayed_work(&bat_work);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +#ifdef CONFIG_PM
>>> +static int generic_adc_bat_suspend(struct platform_device *pdev,
>>> + pm_message_t state)
>>> +{
>>> + struct generic_platform_data *pdata = pdev->dev.platform_data;
>>> + struct generic_bat *bat = container_of(pdata,
>>> + struct generic_bat, pdata);
>>> +
>>> + cancel_delayed_work_sync(&bat_work);
>>> + bat->status = POWER_SUPPLY_STATUS_UNKNOWN;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int generic_adc_bat_resume(struct platform_device *pdev)
>>> +{
>>> + /* Schedule timer to check current status */
>>> + schedule_delayed_work(&bat_work,
>>> + msecs_to_jiffies(JITTER_DELAY));
>>> +
>>> + return 0;
>>> +}
>>> +#else
>>> +#define generic_adc_bat_suspend NULL
>>> +#define generic_adc_bat_resume NULL
>>> +#endif
>>> +
>>> +static struct platform_driver generic_adc_bat_driver = {
>>> + .driver = {
>>> + .name = "generic-adc-battery",
>>> + },
>>> + .probe = generic_adc_bat_probe,
>>> + .remove = generic_adc_bat_remove,
>>> + .suspend = generic_adc_bat_suspend,
>>> + .resume = generic_adc_bat_resume,
>>> +};
>>> +
>>> +module_platform_driver(generic_adc_bat_driver);
>>> +
>>> +MODULE_AUTHOR("anish kumar <anish198519851985@gmail.com>");
>>> +MODULE_DESCRIPTION("generic battery driver using IIO");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/include/linux/power/generic-battery.h b/include/linux/power/generic-battery.h
>>> new file mode 100644
>>> index 0000000..7a43aa0
>>> --- /dev/null
>>> +++ b/include/linux/power/generic-battery.h
>>> @@ -0,0 +1,26 @@
>>> +/*
>>> + * 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_BATTERY_H
>>> +#define GENERIC_BATTERY_H
>>> +
>>> +#include <linux/power_supply.h>
>>> +
>>> +/**
>>> + * struct generic_platform_data - platform data for generic battery
>> A little inconsistent on capitalization. (might as well do it now or
>> one of those people who delight in trivial patches will 'tidy' it
>> up for you :)
> I don't mind as it is a good start for people to get familiar with the
> linux patch process.
Can tell you don't get to deal with the tedious merge conflicts that result ;)
> However I will address this valuable comments.
>>> + * @battery_info: Information about the battery
>>> + * @cal_charge: platform callback to calculate charge
>>> + * @gpio_charge_finished: GPIO number used for interrupts
>>> + * @gpio_inverted: Is the gpio inverted?
>>> + */
>>> +struct generic_platform_data {
>>> + struct power_supply_info battery_info;
>>> + int (*cal_charge)(long);
>>> + int gpio_charge_finished;
>>> + int gpio_inverted;
>>> +};
>>> +
>>> +#endif /* GENERIC_BATTERY_H */
>>>
>>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-08 10:09 ` Jonathan Cameron
@ 2012-09-09 9:32 ` anish kumar
2012-09-09 9:46 ` Lars-Peter Clausen
0 siblings, 1 reply; 19+ messages in thread
From: anish kumar @ 2012-09-09 9:32 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, cbou, dwmw2, linux-kernel, lars
On Sat, 2012-09-08 at 11:09 +0100, Jonathan Cameron wrote:
> On 09/08/2012 08:10 AM, anish kumar wrote:
> > Thanks for your time.
> > On Fri, 2012-09-07 at 08:52 +0100, Jonathan Cameron wrote:
> >> On 02/09/12 16:39, anish kumar wrote:
> >>> From: anish kumar <anish198519851985@gmail.com>
> >>>
> >>> This patch is to use IIO to write a generic batttery driver.
> >> battery
> >>> There are some inherent assumptions here:
> >>> 1.User is having both main battery and backup battery.
> >> Seems rather like that could be easily enough relaxed or configured via
> >> platform data?
> > Yes
> >>> 2.Both batteries use same channel to get the information.
> >> Do you mean same actual channel (physical pin) or same ADC
> >> (with appropriate mux etc)?
> > As lars pointed out it would be better if we have multiple channels
> > per battery.The channel what I am referring here is the name of
> > different channels which will be exported by adc driver to get
> > voltage, current and power.
>
> I agree entirely with what Lars-Peter said. A separate instance of
> the driver per battery will make life much cleaner and allow as you
> say for separate channels for voltage, current and power for each one.
> I guess it may be a case of trying to get each one for every battery
> and continue with whatever turns out to be available.
>
> >>>
> >> Mostly fine. There are a few corners I didn't understand as I don't
> >> have time to dive into the battery framework in the near future. Will
> >> leave it up to those more familiar with that side of things!
> >>
> >> My only real issue is that it would be cleaner to do the individual
> >> channels by name rather than a get all as you 'know' here how many
> >> channels are expected.
> > agreed.
> >>
> >> Also don't release the IIO channels until you are actually finished
> >> using them as holding them should prevent the ADC module from going away
> >> underneath you (which is nasty :)
> > agreed.
> >>
> >> Sorry it took so long to get back to you on this.
> > That is the biggest advantage of working in open-source.
> > We can take our own time.
> :)
> >>
> >> So what happened in 1985?
> > Was hurrying to get an email id :)
> And there I was imagining a magical significance to it....
>
> >>
> >> Jonathan
> >>> Signed-off-by: anish kumar <anish198519851985@gmail.com>
> >>> ---
> >>> drivers/power/Kconfig | 8 +
> >>> drivers/power/Makefile | 1 +
> >>> drivers/power/generic-battery.c | 374 +++++++++++++++++++++++++++++++++
> >>> include/linux/power/generic-battery.h | 26 +++
> >>> 4 files changed, 409 insertions(+), 0 deletions(-)
> >>> create mode 100644 drivers/power/generic-battery.c
> >>> create mode 100644 include/linux/power/generic-battery.h
> >>>
> >>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> >>> index fcc1bb0..546e86b 100644
> >>> --- a/drivers/power/Kconfig
> >>> +++ b/drivers/power/Kconfig
> >>> @@ -309,6 +309,14 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
> >>> help
> >>> Say Y to enable battery temperature measurements using
> >>> thermistor connected on BATCTRL ADC.
> >>> +
> >>> +config GENERIC_BATTERY
> >>> + tristate "Generic battery support using IIO"
> >>> + depends on IIO
> >>> + help
> >>> + Say Y here to enable support for the generic battery driver
> >>> + which uses IIO framework to read adc for it's main and backup
> >> ADC
> >>> + battery.
> >>> endif # POWER_SUPPLY
> >>>
> >>> source "drivers/power/avs/Kconfig"
> >>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> >>> index ee58afb..8284f9c 100644
> >>> --- a/drivers/power/Makefile
> >>> +++ b/drivers/power/Makefile
> >>> @@ -45,3 +45,4 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
> >>> obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
> >>> obj-$(CONFIG_POWER_AVS) += avs/
> >>> obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
> >>> +obj-$(CONFIG_GENERIC_BATTERY) += generic-battery.o
> >>> diff --git a/drivers/power/generic-battery.c b/drivers/power/generic-battery.c
> >>> new file mode 100644
> >>> index 0000000..33170b7
> >>> --- /dev/null
> >>> +++ b/drivers/power/generic-battery.c
> >>> @@ -0,0 +1,374 @@
> >>> +/*
> >>> + * Generice battery driver code using IIO
> >> Generic
> >>> + * Copyright (C) 2012, Anish Kumar <anish198519851985@gmail.com>
> >>> + * 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.
> >>> + *
> >> bonus blank line to remove
> >>> + */
> >>> +
> >>> +#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>
> >>> +
> >>> +#include <linux/power/generic-battery.h>
> >>> +
> >>> +#define BAT_POLL_INTERVAL 10000 /* ms */
> >>> +#define JITTER_DELAY 500 /* ms */
> >> Elements to make configurable?
> > Yes probably make it platform data?
> >>> +
> >>> +enum BAT {
> >>> + MAIN_BAT = 0,
> >>> + BACKUP_BAT,
> >>> + BAT_MAX,
> >>> +};
> >>> +
> >> Document this please. It's not immediately obvious what
> >> channel_index is.
> > This will be removed as we will be using only one device.
> >>
> >> Why not just have a direct pointer to the correct channel?
> >>> +struct generic_adc_bat {
> >>> + struct power_supply psy;
> >>> + struct iio_channel *channels;
> >>> + int channel_index;
> >>> +};
> >>> +
> >>> +struct generic_bat {
> >> Spacing is a bit random in here
> >>> + struct generic_adc_bat bat[BAT_MAX];
> >>> + struct generic_platform_data pdata;
> >>> + int volt_value;
> >>> + int cur_value;
> >>> + unsigned int timestamp;
> >>> + int level;
> >>> + int status;
> >>> + int cable_plugged:1;
> >>> +};
> >>> +
> >>> +static struct generic_bat generic_bat = {
> >>> + .bat[MAIN_BAT] = {
> >>> + .psy.name = "main-bat",
> >>> + },
> >>> + .bat[BACKUP_BAT] = {
> >>> + .psy.name = "backup-bat",
> >>> + },
> >>> +};
> >>> +
> >>> +static struct delayed_work bat_work;
> >>> +
> >>> +static void generic_adc_bat_ext_power_changed(struct power_supply *psy)
> >>> +{
> >>> + schedule_delayed_work(&bat_work,
> >>> + msecs_to_jiffies(JITTER_DELAY));
> >>> +}
> >>> +
> >>> +static enum power_supply_property generic_adc_main_bat_props[] = {
> >>> + 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,
> >>> +};
> >>> +
> >>> +static int charge_finished(struct generic_bat *bat)
> >>> +{
> >>> + return bat->pdata.gpio_inverted ?
> >>> + !gpio_get_value(bat->pdata.gpio_charge_finished) :
> >>> + gpio_get_value(bat->pdata.gpio_charge_finished);
> >>> +}
> >>> +
> >>> +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;
> >>> + struct generic_bat *bat;
> >>> + int ret, scaleint, scalepart, iio_val;
> >>> + long result = 0;
> >>> +
> >>> + if (!strcmp(psy->name, "main-battery")) {
> >>> + adc_bat = container_of(psy,
> >>> + struct generic_adc_bat, psy);
> >> This first statement is I think shared so move it out of the if / else
> > This whole logic will change as we will be using only one device and
> > if has user has multiple batteries then he can register multiple
> > devices.
> Yes, much cleaner that way.
> >>
> >>> + bat = container_of(adc_bat,
> >>> + struct generic_bat, bat[MAIN_BAT]);
> >>> + } else if (!strcmp(psy->name, "backup-battery")) {
> >>> + adc_bat = container_of(psy, struct generic_adc_bat, psy);
> >>> + bat = container_of(adc_bat,
> >>> + struct generic_bat, bat[BACKUP_BAT]);
> >>> + } else {
> >>> + /* Ideally this should never happen */
> >>> + WARN_ON(1);
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + if (!bat) {
> >> > + dev_err(psy->dev, "no battery infos ?!\n");
> >>> + return -EINVAL;
> >>> +
> >>> + ret = iio_read_channel_raw(&adc_bat->channels[adc_bat->channel_index],
> >>> + &iio_val);
> >>> + ret = iio_read_channel_scale(&adc_bat->channels[adc_bat->channel_index],
> >>> + &scaleint, &scalepart);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >> A quick comment here on what the battery framework is expecting vs IIO
> >> supplying would be good. It may be that this particular output is one
> >> that we should lift over into the core rather than end up with multiple
> >> drivers doing the same thing.
> > I think for this we need to get all the channels supported by a
> > particular adc device and once we get the information regarding the
> > channel(current/voltage/power) supported we can find out "What
> > information is exported about this channel which may include scale or
> > raw adc value".This can be used to call the corresponding API's.
> Agreed. If I'd read this far I'd never have said the same thing at the top
> of this email ;)
> >>
> >>> + 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;
> >>> + }
> >>> +
> >>> + switch (psp) {
> >>> + case POWER_SUPPLY_PROP_STATUS:
> >>> + if (bat->pdata.gpio_charge_finished < 0)
> >>> + val->intval = bat->level == 100000 ?
> >>> + POWER_SUPPLY_STATUS_FULL : bat->status;
> >>> + else
> >>> + val->intval = bat->status;
> >>> + return 0;
> >>> + case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
> >>> + val->intval = 0;
> >>> + return 0;
> >>> + case POWER_SUPPLY_PROP_CHARGE_NOW:
> >>> + val->intval = bat->pdata.cal_charge(result);
> >>> + return 0;
> >>> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> >>> + val->intval = result;
> >>> + return 0;
> >>> + case POWER_SUPPLY_PROP_CURRENT_NOW:
> >>> + val->intval = result;
> >>> + return 0;
> >> why return from these and break from the later ones (to return much the
> >> same I think...)?
> > my mistake.
> >>> + case POWER_SUPPLY_PROP_TECHNOLOGY:
> >>> + val->intval = bat->pdata.battery_info.technology;
> >>> + break;
> >>> + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> >>> + val->intval = bat->pdata.battery_info.voltage_min_design;
> >>> + break;
> >>> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> >>> + val->intval = bat->pdata.battery_info.voltage_max_design;
> >>> + break;
> >>> + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> >>> + val->intval = bat->pdata.battery_info.charge_full_design;
> >>> + break;
> >>> + case POWER_SUPPLY_PROP_MODEL_NAME:
> >>> + val->strval = bat->pdata.battery_info.name;
> >>> + break;
> >>> + default:
> >>> + return -EINVAL;
> >>> + }
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static void generic_adc_bat_work(struct work_struct *work)
> >>> +{
> >>> + struct generic_bat *bat = &generic_bat;
> >>> + int is_charged;
> >>> + int is_plugged;
> >>> + static int was_plugged;
> >> That's nasty. Prevents even the theoretical posibility of multiple
> >> copies of the driver. That should be in the device specific data.
> > yes should be part of device specific data.
> >>> +
> >>> + /* backup battery doesn't have current monitoring capability anyway */
> >> Always or in this particular configuration?
> > I was told by Anton Vorontsov.Anyway now this driver will be per device
> > so shouldn't matter.
> Maybe one day people will stick enough batteries on my phone for it to last
> one whole day ;)
> >>
> >>> + is_plugged = power_supply_am_i_supplied(&bat->bat[MAIN_BAT].psy);
> >>> + bat->cable_plugged = is_plugged;
> >>> + if (is_plugged != was_plugged) {
> >>> + was_plugged = is_plugged;
> >>> + if (is_plugged)
> >>> + bat->status = POWER_SUPPLY_STATUS_CHARGING;
> >>> + else
> >>> + bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
> >>> + } else {
> >>> + if ((bat->pdata.gpio_charge_finished >= 0) && is_plugged) {
> >>> + is_charged = charge_finished(&generic_bat);
> >>> + if (is_charged)
> >>> + bat->status = POWER_SUPPLY_STATUS_FULL;
> >>> + else
> >>> + bat->status = POWER_SUPPLY_STATUS_CHARGING;
> >>> + }
> >>> + }
> >>> +
> >>> + power_supply_changed(&bat->bat[MAIN_BAT].psy);
> >>> +}
> >>> +
> >>> +static irqreturn_t generic_adc_bat_charged(int irq, void *dev_id)
> >>> +{
> >>> + schedule_delayed_work(&bat_work,
> >>> + msecs_to_jiffies(JITTER_DELAY));
> >>> + return IRQ_HANDLED;
> >>> +}
> >>> +
> >>> +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
> >>> +{
> >>> + struct generic_platform_data *pdata = pdev->dev.platform_data;
> >>> + struct iio_channel *channels;
> >>> + int num_channels = 0;
> >>> + int ret, i, j, k = 0;
> >>> + enum iio_chan_type type;
> >>> +
> >>> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++) {
> >>> + generic_bat.bat[i].psy.type = POWER_SUPPLY_TYPE_BATTERY;
> >>> + generic_bat.bat[i].psy.properties =
> >>> + generic_adc_main_bat_props;
> >>> + generic_bat.bat[i].psy.num_properties =
> >>> + ARRAY_SIZE(generic_adc_main_bat_props);
> >>> + generic_bat.bat[i].psy.get_property =
> >>> + generic_adc_bat_get_property;
> >>> + generic_bat.bat[i].psy.external_power_changed =
> >>> + generic_adc_bat_ext_power_changed;
> >> Could you do this with a static const array? Looks like there is
> >> nothing dynamic in here and that would make for cleaner code to read.
> >> Given other elements of psy are clearly dynamic (dev pointer) you will
> >> have to memcpy the static version in which is tedious. Still easier
> >> to read though in my view.
> >>> + }
> > right.
> >>> +
> >>> + generic_bat.volt_value = -1;
> >>> + generic_bat.cur_value = -1;
> >>> + generic_bat.cable_plugged = 0;
> >>> + generic_bat.status = POWER_SUPPLY_STATUS_DISCHARGING;
> >>> +
> >>> + memcpy(&generic_bat.pdata, pdata, sizeof(struct generic_platform_data));
> >>> +
> >> Is there anything dynamic in the pdata? If not it would be cleaner just
> >> to use a pointer to it rather than make a copy (I can't immediately spot
> >> anything but them I'm not familiar with the battery
> >> side of things.
> > We can just assign it to generic_bat so that it can be used in the
> > relevant functions.Otherwise we need to use dev_set_drvdata and
> > dev_get_drvdata.
> >>> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++) {
> >>> + ret = power_supply_register(&pdev->dev,
> >>> + &generic_bat.bat[i].psy);
> >>> + if (ret)
> >>> + goto err_reg_main;
> >>> + }
> >>> +
> >>> + channels = iio_channel_get_all(dev_name(&pdev->dev));
> >> I would give these explicit names and get the two individual channels by
> >> name. I think it will give you cleaner code.
> > Yes, now it will be based on pdata->voltage_channel,
> > pdata->current_channel and so on.We will use this to get the channel.
> Maybe better to just do this via the iio_map structures in the platform data
> and standard naming for each channel.
>
> "voltage", "current", "power" would do nicely. Note we'll have to add the
> relevant naming for the other side to more IIO device drivers via the
> 'datasheet_name' element of iio_chan_spec. A quick grep shows this
> is only done in drivers/staging/iio/max1363_core.c so far (bet you can't
> guess what is soldered to my test board :)
Sorry I couldn't understand this but this is what I came up with.Hope it
doesn't disappoint.
It has the advantage of getting extended easily.
enum chan_type{
VOLTAGE,
CURRENT,
POWER,
MAX_TYPE
};
struct channel_map {
struct iio_map channel;
enum chan_type type;
};
struct iio_battery_platform_data {
struct power_supply_info battery_info;
struct channel_map *map;
int num_map;
char battery_name[BAT_MAX_NAME];
int (*cal_charge)(long);
int gpio_charge_finished;
int gpio_inverted;
int bat_poll_interval;
int jitter_delay;
};
here goes the assignment:
for(i = 0; i < pdata->num_map; i++) {
switch(pdata->map[i].type)
case VOLTAGE:
adc_bat->channel[VOLTAGE] =
iio_channel_get(
pdata->map[i].channel.consumer_dev_name,
pdata->map[i].channel.adc_channel_label);
adc_bat->chan_index = 1 << VOLTAGE;
break;
case CURRENT:
adc_bat->channel[CURRENT] =
iio_channel_get(
pdata->map->channel->consumer_dev_name,
pdata->map[i].channel.adc_channel_label);
adc_bat->chan_index = 1 << CURRENT;
break;
case POWER:
adc_bat->channel[POWER] =
iio_channel_get(
pdata->map[i].channel.consumer_dev_name,
pdata->map[i].channel.adc_channel_label);
adc_bat->chan_index = 1 << POWER;
break;
default:
pr_info("add any other channels here in the case\n");
}
With the chan_index we can know which property is set or every time we
need to run through pdata->map[i].type to know the property set.
>
> >>> + if (IS_ERR(channels)) {
> >>> + ret = PTR_ERR(channels);
> >>> + goto err_reg_backup;
> >>> + }
> >>> +
> >>> + while (channels[num_channels].indio_dev)
> >>> + num_channels++;
> >>> +
> >>> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> >>> + generic_bat.bat[i].channels = kzalloc(sizeof(struct iio_channel)
> >>> + * BAT_MAX, GFP_KERNEL);
> >>> +
> >>> + /* assuming main battery and backup battery is using the same channel */
> >>> + for (i = 0; i < num_channels; i++) {
> >>> + ret = iio_get_channel_type(&channels[i], &type);
> >> Using named channels you should 'know' you have the correct type. You
> >> can of course check if you don't trust your platform data though!
> > right.
> >>> + if (ret < 0)
> >>> + goto err_gpio;
> >>> +
> >>> + if (type == IIO_VOLTAGE || type == IIO_CURRENT) {
> >>> + for (j = 0; j < BAT_MAX; j++) {
> >>> + generic_bat.bat[j].channel_index = k;
> >>> + generic_bat.bat[j].channels[k] = channels[i];
> >>> + }
> >>> + k++;
> >>> + }
> >>> + continue;
> >>> + }
> >>> +
> >>> + INIT_DELAYED_WORK(&bat_work, generic_adc_bat_work);
> >>> +
> >>> + if (pdata->gpio_charge_finished >= 0) {
> >>> + ret = gpio_request(pdata->gpio_charge_finished, "charged");
> >>> + if (ret)
> >>> + goto err_gpio;
> >>> +
> >>> + ret = request_irq(gpio_to_irq(pdata->gpio_charge_finished),
> >>> + generic_adc_bat_charged,
> >>> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> >>> + "battery charged", NULL);
> >>> + if (ret)
> >>> + goto err_gpio;
> >>> + }
> >>> +
> >>> + dev_info(&pdev->dev, "successfully loaded\n");
> >>> +
> >>> + /* Schedule timer to check current status */
> >>> + schedule_delayed_work(&bat_work,
> >>> + msecs_to_jiffies(JITTER_DELAY));
> >>> +
> >>> + iio_channel_release_all(channels);
> >> Not if you want to prevent the adc driver from being removed from under
> >> you. They should only be released in the driver removal.
> > right.
> >>> +
> >>> + return 0;
> >>> +
> >>> +err_gpio:
> >>> + iio_channel_release_all(channels);
> >>> +err_reg_backup:
> >>> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> >>> + power_supply_unregister(&generic_bat.bat[i].psy);
> >>> +err_reg_main:
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static int generic_adc_bat_remove(struct platform_device *pdev)
> >>> +{
> >>> + int i;
> >>> + struct generic_platform_data *pdata = pdev->dev.platform_data;
> >>> +
> >>> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> >>> + power_supply_unregister(&generic_bat.bat[i].psy);
> >>
> >> You should release the IIO channels here when they are no longer needed.
> > right.
> >>> +
> >>> + if (pdata->gpio_charge_finished >= 0) {
> >> I forget, is a gpio index of 0 definitely invalid?
> >>> + free_irq(gpio_to_irq(pdata->gpio_charge_finished), NULL);
> >>> + gpio_free(pdata->gpio_charge_finished);
> >>> + }
> >>> +
> >>> + cancel_delayed_work(&bat_work);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +#ifdef CONFIG_PM
> >>> +static int generic_adc_bat_suspend(struct platform_device *pdev,
> >>> + pm_message_t state)
> >>> +{
> >>> + struct generic_platform_data *pdata = pdev->dev.platform_data;
> >>> + struct generic_bat *bat = container_of(pdata,
> >>> + struct generic_bat, pdata);
> >>> +
> >>> + cancel_delayed_work_sync(&bat_work);
> >>> + bat->status = POWER_SUPPLY_STATUS_UNKNOWN;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int generic_adc_bat_resume(struct platform_device *pdev)
> >>> +{
> >>> + /* Schedule timer to check current status */
> >>> + schedule_delayed_work(&bat_work,
> >>> + msecs_to_jiffies(JITTER_DELAY));
> >>> +
> >>> + return 0;
> >>> +}
> >>> +#else
> >>> +#define generic_adc_bat_suspend NULL
> >>> +#define generic_adc_bat_resume NULL
> >>> +#endif
> >>> +
> >>> +static struct platform_driver generic_adc_bat_driver = {
> >>> + .driver = {
> >>> + .name = "generic-adc-battery",
> >>> + },
> >>> + .probe = generic_adc_bat_probe,
> >>> + .remove = generic_adc_bat_remove,
> >>> + .suspend = generic_adc_bat_suspend,
> >>> + .resume = generic_adc_bat_resume,
> >>> +};
> >>> +
> >>> +module_platform_driver(generic_adc_bat_driver);
> >>> +
> >>> +MODULE_AUTHOR("anish kumar <anish198519851985@gmail.com>");
> >>> +MODULE_DESCRIPTION("generic battery driver using IIO");
> >>> +MODULE_LICENSE("GPL");
> >>> diff --git a/include/linux/power/generic-battery.h b/include/linux/power/generic-battery.h
> >>> new file mode 100644
> >>> index 0000000..7a43aa0
> >>> --- /dev/null
> >>> +++ b/include/linux/power/generic-battery.h
> >>> @@ -0,0 +1,26 @@
> >>> +/*
> >>> + * 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_BATTERY_H
> >>> +#define GENERIC_BATTERY_H
> >>> +
> >>> +#include <linux/power_supply.h>
> >>> +
> >>> +/**
> >>> + * struct generic_platform_data - platform data for generic battery
> >> A little inconsistent on capitalization. (might as well do it now or
> >> one of those people who delight in trivial patches will 'tidy' it
> >> up for you :)
> > I don't mind as it is a good start for people to get familiar with the
> > linux patch process.
> Can tell you don't get to deal with the tedious merge conflicts that result ;)
>
> > However I will address this valuable comments.
> >>> + * @battery_info: Information about the battery
> >>> + * @cal_charge: platform callback to calculate charge
> >>> + * @gpio_charge_finished: GPIO number used for interrupts
> >>> + * @gpio_inverted: Is the gpio inverted?
> >>> + */
> >>> +struct generic_platform_data {
> >>> + struct power_supply_info battery_info;
> >>> + int (*cal_charge)(long);
> >>> + int gpio_charge_finished;
> >>> + int gpio_inverted;
> >>> +};
> >>> +
> >>> +#endif /* GENERIC_BATTERY_H */
> >>>
> >>
> >
> >
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-09 9:32 ` anish kumar
@ 2012-09-09 9:46 ` Lars-Peter Clausen
2012-09-09 9:59 ` Jonathan Cameron
0 siblings, 1 reply; 19+ messages in thread
From: Lars-Peter Clausen @ 2012-09-09 9:46 UTC (permalink / raw)
To: anish kumar; +Cc: Jonathan Cameron, linux-iio, cbou, dwmw2, linux-kernel
On 09/09/2012 11:32 AM, anish kumar wrote:
>>>>> + channels = iio_channel_get_all(dev_name(&pdev->dev));
>>>> I would give these explicit names and get the two individual channels by
>>>> name. I think it will give you cleaner code.
>>> Yes, now it will be based on pdata->voltage_channel,
>>> pdata->current_channel and so on.We will use this to get the channel.
>> Maybe better to just do this via the iio_map structures in the platform data
>> and standard naming for each channel.
>>
>> "voltage", "current", "power" would do nicely. Note we'll have to add the
>> relevant naming for the other side to more IIO device drivers via the
>> 'datasheet_name' element of iio_chan_spec. A quick grep shows this
>> is only done in drivers/staging/iio/max1363_core.c so far (bet you can't
>> guess what is soldered to my test board :)
> Sorry I couldn't understand this but this is what I came up with.Hope it
> doesn't disappoint.
> It has the advantage of getting extended easily.
>
> enum chan_type{
> VOLTAGE,
> CURRENT,
> POWER,
> MAX_TYPE
> };
>
> struct channel_map {
> struct iio_map channel;
> enum chan_type type;
> };
>
> struct iio_battery_platform_data {
> struct power_supply_info battery_info;
> struct channel_map *map;
> int num_map;
> char battery_name[BAT_MAX_NAME];
> int (*cal_charge)(long);
> int gpio_charge_finished;
> int gpio_inverted;
> int bat_poll_interval;
> int jitter_delay;
> };
>
> here goes the assignment:
> for(i = 0; i < pdata->num_map; i++) {
> switch(pdata->map[i].type)
> case VOLTAGE:
> adc_bat->channel[VOLTAGE] =
> iio_channel_get(
> pdata->map[i].channel.consumer_dev_name,
> pdata->map[i].channel.adc_channel_label);
> adc_bat->chan_index = 1 << VOLTAGE;
> break;
> case CURRENT:
> adc_bat->channel[CURRENT] =
> iio_channel_get(
> pdata->map->channel->consumer_dev_name,
> pdata->map[i].channel.adc_channel_label);
> adc_bat->chan_index = 1 << CURRENT;
> break;
> case POWER:
> adc_bat->channel[POWER] =
> iio_channel_get(
> pdata->map[i].channel.consumer_dev_name,
> pdata->map[i].channel.adc_channel_label);
> adc_bat->chan_index = 1 << POWER;
> break;
> default:
> pr_info("add any other channels here in the case\n");
> }
>
> With the chan_index we can know which property is set or every time we
> need to run through pdata->map[i].type to know the property set.
Hi,
I think what Jonathon meant is something like this.
adc_bat->channel[VOLTAGE] = iio_channel_get(dev_name(&pdev->dev), "voltage");
...
adc_bat->channel[CURRENT] = iio_channel_get(dev_name(&pdev->dev), "current");
...
adc_bat->channel[POWER] = iio_channel_get(dev_name(&pdev->dev), "power");
That's the beauty of the channel iio_map you can map an arbitrary channel of
your ADC to a virtual identifier.
E.g. an example map could look like.
static struct iio_map map[] = {
{ "AIN1", "iio-battery.1", "voltage" },
};
With this when the battery driver calls iio_channel_get(... "voltage") it will
get the "AIN1" channel of the device.
- Lars
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-09 9:46 ` Lars-Peter Clausen
@ 2012-09-09 9:59 ` Jonathan Cameron
0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2012-09-09 9:59 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: anish kumar, linux-iio, cbou, dwmw2, linux-kernel
On 09/09/2012 10:46 AM, Lars-Peter Clausen wrote:
> On 09/09/2012 11:32 AM, anish kumar wrote:
>>>>>> + channels = iio_channel_get_all(dev_name(&pdev->dev));
>>>>> I would give these explicit names and get the two individual channels by
>>>>> name. I think it will give you cleaner code.
>>>> Yes, now it will be based on pdata->voltage_channel,
>>>> pdata->current_channel and so on.We will use this to get the channel.
>>> Maybe better to just do this via the iio_map structures in the platform data
>>> and standard naming for each channel.
>>>
>>> "voltage", "current", "power" would do nicely. Note we'll have to add the
>>> relevant naming for the other side to more IIO device drivers via the
>>> 'datasheet_name' element of iio_chan_spec. A quick grep shows this
>>> is only done in drivers/staging/iio/max1363_core.c so far (bet you can't
>>> guess what is soldered to my test board :)
>> Sorry I couldn't understand this but this is what I came up with.Hope it
>> doesn't disappoint.
>> It has the advantage of getting extended easily.
>>
>> enum chan_type{
>> VOLTAGE,
>> CURRENT,
>> POWER,
>> MAX_TYPE
>> };
>>
>> struct channel_map {
>> struct iio_map channel;
>> enum chan_type type;
>> };
>>
>> struct iio_battery_platform_data {
>> struct power_supply_info battery_info;
>> struct channel_map *map;
>> int num_map;
>> char battery_name[BAT_MAX_NAME];
>> int (*cal_charge)(long);
>> int gpio_charge_finished;
>> int gpio_inverted;
>> int bat_poll_interval;
>> int jitter_delay;
>> };
>>
>> here goes the assignment:
>> for(i = 0; i < pdata->num_map; i++) {
>> switch(pdata->map[i].type)
>> case VOLTAGE:
>> adc_bat->channel[VOLTAGE] =
>> iio_channel_get(
>> pdata->map[i].channel.consumer_dev_name,
>> pdata->map[i].channel.adc_channel_label);
>> adc_bat->chan_index = 1 << VOLTAGE;
>> break;
>> case CURRENT:
>> adc_bat->channel[CURRENT] =
>> iio_channel_get(
>> pdata->map->channel->consumer_dev_name,
>> pdata->map[i].channel.adc_channel_label);
>> adc_bat->chan_index = 1 << CURRENT;
>> break;
>> case POWER:
>> adc_bat->channel[POWER] =
>> iio_channel_get(
>> pdata->map[i].channel.consumer_dev_name,
>> pdata->map[i].channel.adc_channel_label);
>> adc_bat->chan_index = 1 << POWER;
>> break;
>> default:
>> pr_info("add any other channels here in the case\n");
>> }
>>
>> With the chan_index we can know which property is set or every time we
>> need to run through pdata->map[i].type to know the property set.
>
> Hi,
>
> I think what Jonathon meant is something like this.
>
> adc_bat->channel[VOLTAGE] = iio_channel_get(dev_name(&pdev->dev), "voltage");
> ...
> adc_bat->channel[CURRENT] = iio_channel_get(dev_name(&pdev->dev), "current");
> ...
> adc_bat->channel[POWER] = iio_channel_get(dev_name(&pdev->dev), "power");
>
> That's the beauty of the channel iio_map you can map an arbitrary channel of
> your ADC to a virtual identifier.
>
> E.g. an example map could look like.
>
> static struct iio_map map[] = {
> { "AIN1", "iio-battery.1", "voltage" },
> };
>
> With this when the battery driver calls iio_channel_get(... "voltage") it will
> get the "AIN1" channel of the device.
>
Exactly what I meant, thanks Lars.
I'm not sure any consumers are actually using that interface at the moment
as the main ones are hwmon and input (in my wip branch) and in both those
cases the association doesn't matter.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-02 15:39 anish kumar
2012-09-02 16:14 ` Sannu K
2012-09-07 7:52 ` Jonathan Cameron
@ 2012-09-07 8:49 ` Lars-Peter Clausen
2012-09-08 7:30 ` anish kumar
2 siblings, 1 reply; 19+ messages in thread
From: Lars-Peter Clausen @ 2012-09-07 8:49 UTC (permalink / raw)
To: anish kumar; +Cc: jic23, linux-iio, cbou, dwmw2, linux-kernel
On 09/02/2012 05:39 PM, anish kumar wrote:
> From: anish kumar <anish198519851985@gmail.com>
>
> This patch is to use IIO to write a generic batttery driver.
> There are some inherent assumptions here:
> 1.User is having both main battery and backup battery.
> 2.Both batteries use same channel to get the information.
>
Hi thanks for stepping up to take care of this and sorry for the late reply.
> Signed-off-by: anish kumar <anish198519851985@gmail.com>
> ---
[...]
> +
> +struct generic_adc_bat {
> + struct power_supply psy;
> + struct iio_channel *channels;
> + int channel_index;
> +};
> +
> +struct generic_bat {
> + struct generic_adc_bat bat[BAT_MAX];
> + struct generic_platform_data pdata;
> + int volt_value;
> + int cur_value;
> + unsigned int timestamp;
> + int level;
> + int status;
> + int cable_plugged:1;
> +};
> +
> +static struct generic_bat generic_bat = {
> + .bat[MAIN_BAT] = {
> + .psy.name = "main-bat",
> + },
> + .bat[BACKUP_BAT] = {
> + .psy.name = "backup-bat",
> + },
> +};
This should be per device. I'd also just use one power_supply per device. If
somebody has multiple batteries in their system they can register multiple
devices. This should make the driver more flexible.
> +
> +static struct delayed_work bat_work;
This should also go into the generic_bat struct.
+
> +static void generic_adc_bat_ext_power_changed(struct power_supply *psy)
> +{
> + schedule_delayed_work(&bat_work,
> + msecs_to_jiffies(JITTER_DELAY));
> +}
> +
> +static enum power_supply_property generic_adc_main_bat_props[] = {
> + 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,
> +};
It probably make sense to create this at runtime, depending on which kind of
IIO channels are provided. E.g. for VOLTAGE_NOW/CURRENT_NOW.
[...]
> +
> +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;
> + struct generic_bat *bat;
> + int ret, scaleint, scalepart, iio_val;
> + long result = 0;
> +
> + if (!strcmp(psy->name, "main-battery")) {
> + adc_bat = container_of(psy,
> + struct generic_adc_bat, psy);
> + bat = container_of(adc_bat,
> + struct generic_bat, bat[MAIN_BAT]);
> + } else if (!strcmp(psy->name, "backup-battery")) {
> + adc_bat = container_of(psy, struct generic_adc_bat, psy);
> + bat = container_of(adc_bat,
> + struct generic_bat, bat[BACKUP_BAT]);
> + } else {
> + /* Ideally this should never happen */
> + WARN_ON(1);
> + return -EINVAL;
> + }
> +
> + if (!bat) {
> + dev_err(psy->dev, "no battery infos ?!\n");
> + return -EINVAL;
> + }
> + ret = iio_read_channel_raw(&adc_bat->channels[adc_bat->channel_index],
> + &iio_val);
> + ret = iio_read_channel_scale(&adc_bat->channels[adc_bat->channel_index],
> + &scaleint, &scalepart);
It would probably make sense to only sample if the attribute for
VOLTAGE_NOW/CURRENT_NOW property is read.
[...]
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_STATUS:
> + if (bat->pdata.gpio_charge_finished < 0)
> + val->intval = bat->level == 100000 ?
> + POWER_SUPPLY_STATUS_FULL : bat->status;
> + else
> + val->intval = bat->status;
> + return 0;
> + case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
> + val->intval = 0;
> + return 0;
> + case POWER_SUPPLY_PROP_CHARGE_NOW:
> + val->intval = bat->pdata.cal_charge(result);
> + return 0;
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + val->intval = result;
> + return 0;
> + case POWER_SUPPLY_PROP_CURRENT_NOW:
> + val->intval = result;
> + return 0;
also this will return the same value for VOLTAGE_NOW and CURRENT_NOW since
there is only one channel per battery. It might make sense to allow multiple
channels per battery, one reporting current one voltage and also one for power.
> + case POWER_SUPPLY_PROP_TECHNOLOGY:
> + val->intval = bat->pdata.battery_info.technology;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> + val->intval = bat->pdata.battery_info.voltage_min_design;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> + val->intval = bat->pdata.battery_info.voltage_max_design;
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> + val->intval = bat->pdata.battery_info.charge_full_design;
> + break;
> + case POWER_SUPPLY_PROP_MODEL_NAME:
> + val->strval = bat->pdata.battery_info.name;
> + break;
> + default:
> + return -EINVAL;
> + }
> + return ret;
> +}
> +
[...]
> +
> +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
> +{
> +[...]
> +
> + /* assuming main battery and backup battery is using the same channel */
> + for (i = 0; i < num_channels; i++) {
> + ret = iio_get_channel_type(&channels[i], &type);
> + if (ret < 0)
> + goto err_gpio;
> +
> + if (type == IIO_VOLTAGE || type == IIO_CURRENT) {
> + for (j = 0; j < BAT_MAX; j++) {
> + generic_bat.bat[j].channel_index = k;
> + generic_bat.bat[j].channels[k] = channels[i];
> + }
> + k++;
> + }
> + continue;
continue at the end of a loop is a noop.
> + }
> +
> + INIT_DELAYED_WORK(&bat_work, generic_adc_bat_work);
> +
> + if (pdata->gpio_charge_finished >= 0) {
gpio_is_valid
> + ret = gpio_request(pdata->gpio_charge_finished, "charged");
> + if (ret)
> + goto err_gpio;
> +
> + ret = request_irq(gpio_to_irq(pdata->gpio_charge_finished),
> + generic_adc_bat_charged,
> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> + "battery charged", NULL);
> + if (ret)
> + goto err_gpio;
> + }
> +
> + dev_info(&pdev->dev, "successfully loaded\n");
I'd remove that message.
> +
> + /* Schedule timer to check current status */
> + schedule_delayed_work(&bat_work,
> + msecs_to_jiffies(JITTER_DELAY));
> +
> + iio_channel_release_all(channels);
> +
> + return 0;
> +
> +err_gpio:
> + iio_channel_release_all(channels);
> +err_reg_backup:
> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> + power_supply_unregister(&generic_bat.bat[i].psy);
> +err_reg_main:
> + return ret;
> +}
> +
[...]
> +
> +#ifdef CONFIG_PM
> +static int generic_adc_bat_suspend(struct platform_device *pdev,
> + pm_message_t state)
> +{
> + struct generic_platform_data *pdata = pdev->dev.platform_data;
> + struct generic_bat *bat = container_of(pdata,
> + struct generic_bat, pdata);
> +
> + cancel_delayed_work_sync(&bat_work);
> + bat->status = POWER_SUPPLY_STATUS_UNKNOWN;
> +
> + return 0;
> +}
> +
> +static int generic_adc_bat_resume(struct platform_device *pdev)
> +{
> + /* Schedule timer to check current status */
> + schedule_delayed_work(&bat_work,
> + msecs_to_jiffies(JITTER_DELAY));
> +
> + return 0;
> +}
> +#else
> +#define generic_adc_bat_suspend NULL
> +#define generic_adc_bat_resume NULL
> +#endif
> +
> +static struct platform_driver generic_adc_bat_driver = {
> + .driver = {
> + .name = "generic-adc-battery",
> + },
> + .probe = generic_adc_bat_probe,
> + .remove = generic_adc_bat_remove,
> + .suspend = generic_adc_bat_suspend,
> + .resume = generic_adc_bat_resume,
Use dev_pm_ops instead. Using the suspend and resume callbacks is deprecated.
> +};
> +
> +module_platform_driver(generic_adc_bat_driver);
> +
> +MODULE_AUTHOR("anish kumar <anish198519851985@gmail.com>");
> +MODULE_DESCRIPTION("generic battery driver using IIO");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/power/generic-battery.h b/include/linux/power/generic-battery.h
> new file mode 100644
> index 0000000..7a43aa0
> --- /dev/null
> +++ b/include/linux/power/generic-battery.h
> @@ -0,0 +1,26 @@
> +/*
> + * 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_BATTERY_H
> +#define GENERIC_BATTERY_H
> +
> +#include <linux/power_supply.h>
> +
> +/**
> + * struct generic_platform_data - platform data for generic battery
> + * @battery_info: Information about the battery
> + * @cal_charge: platform callback to calculate charge
> + * @gpio_charge_finished: GPIO number used for interrupts
> + * @gpio_inverted: Is the gpio inverted?
> + */
> +struct generic_platform_data {
The name might be a bit to generic ;) I'd go for
generic_battery_platform_data or iio_battery_platform_data.
> + struct power_supply_info battery_info;
> + int (*cal_charge)(long);
> + int gpio_charge_finished;
> + int gpio_inverted;
> +};
> +
> +#endif /* GENERIC_BATTERY_H */
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-07 8:49 ` Lars-Peter Clausen
@ 2012-09-08 7:30 ` anish kumar
2012-09-08 10:10 ` Jonathan Cameron
0 siblings, 1 reply; 19+ messages in thread
From: anish kumar @ 2012-09-08 7:30 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: jic23, linux-iio, cbou, dwmw2, linux-kernel
On Fri, 2012-09-07 at 10:49 +0200, Lars-Peter Clausen wrote:
> On 09/02/2012 05:39 PM, anish kumar wrote:
> > From: anish kumar <anish198519851985@gmail.com>
> >
> > This patch is to use IIO to write a generic batttery driver.
> > There are some inherent assumptions here:
> > 1.User is having both main battery and backup battery.
> > 2.Both batteries use same channel to get the information.
> >
>
> Hi thanks for stepping up to take care of this and sorry for the late reply.
Thanks to you.
>
> > Signed-off-by: anish kumar <anish198519851985@gmail.com>
> > ---
> [...]
> > +
> > +struct generic_adc_bat {
> > + struct power_supply psy;
> > + struct iio_channel *channels;
> > + int channel_index;
> > +};
> > +
> > +struct generic_bat {
> > + struct generic_adc_bat bat[BAT_MAX];
> > + struct generic_platform_data pdata;
> > + int volt_value;
> > + int cur_value;
> > + unsigned int timestamp;
> > + int level;
> > + int status;
> > + int cable_plugged:1;
> > +};
> > +
> > +static struct generic_bat generic_bat = {
> > + .bat[MAIN_BAT] = {
> > + .psy.name = "main-bat",
> > + },
> > + .bat[BACKUP_BAT] = {
> > + .psy.name = "backup-bat",
> > + },
> > +};
>
> This should be per device. I'd also just use one power_supply per device. If
> somebody has multiple batteries in their system they can register multiple
> devices. This should make the driver more flexible.
Yes makes sense of having one driver/device and if someone has more
devices to register than it can be done by registering multiple devices
with different platform data.
>
> > +
> > +static struct delayed_work bat_work;
>
> This should also go into the generic_bat struct.
right.
>
> +
> > +static void generic_adc_bat_ext_power_changed(struct power_supply *psy)
> > +{
> > + schedule_delayed_work(&bat_work,
> > + msecs_to_jiffies(JITTER_DELAY));
> > +}
> > +
> > +static enum power_supply_property generic_adc_main_bat_props[] = {
> > + 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,
> > +};
>
> It probably make sense to create this at runtime, depending on which kind of
> IIO channels are provided. E.g. for VOLTAGE_NOW/CURRENT_NOW.
Yes, something like
pdata->voltage_channel/pdata->current_channel/pdata->power_channel and
if this property is set then register the VOLTAGE_NOW/CURRENT_NOW/POWER_
property.
>
> [...]
> > +
> > +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;
> > + struct generic_bat *bat;
> > + int ret, scaleint, scalepart, iio_val;
> > + long result = 0;
> > +
> > + if (!strcmp(psy->name, "main-battery")) {
> > + adc_bat = container_of(psy,
> > + struct generic_adc_bat, psy);
> > + bat = container_of(adc_bat,
> > + struct generic_bat, bat[MAIN_BAT]);
> > + } else if (!strcmp(psy->name, "backup-battery")) {
> > + adc_bat = container_of(psy, struct generic_adc_bat, psy);
> > + bat = container_of(adc_bat,
> > + struct generic_bat, bat[BACKUP_BAT]);
> > + } else {
> > + /* Ideally this should never happen */
> > + WARN_ON(1);
> > + return -EINVAL;
> > + }
> > +
> > + if (!bat) {
> > + dev_err(psy->dev, "no battery infos ?!\n");
> > + return -EINVAL;
> > + }
> > + ret = iio_read_channel_raw(&adc_bat->channels[adc_bat->channel_index],
> > + &iio_val);
> > + ret = iio_read_channel_scale(&adc_bat->channels[adc_bat->channel_index],
> > + &scaleint, &scalepart);
>
> It would probably make sense to only sample if the attribute for
> VOLTAGE_NOW/CURRENT_NOW property is read.
right.
>
> [...]
>
> > +
> > + switch (psp) {
> > + case POWER_SUPPLY_PROP_STATUS:
> > + if (bat->pdata.gpio_charge_finished < 0)
> > + val->intval = bat->level == 100000 ?
> > + POWER_SUPPLY_STATUS_FULL : bat->status;
> > + else
> > + val->intval = bat->status;
> > + return 0;
> > + case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
> > + val->intval = 0;
> > + return 0;
> > + case POWER_SUPPLY_PROP_CHARGE_NOW:
> > + val->intval = bat->pdata.cal_charge(result);
> > + return 0;
> > + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > + val->intval = result;
> > + return 0;
> > + case POWER_SUPPLY_PROP_CURRENT_NOW:
> > + val->intval = result;
> > + return 0;
>
> also this will return the same value for VOLTAGE_NOW and CURRENT_NOW since
> there is only one channel per battery. It might make sense to allow multiple
> channels per battery, one reporting current one voltage and also one for power.
absolutely makes sense.
>
>
> > + case POWER_SUPPLY_PROP_TECHNOLOGY:
> > + val->intval = bat->pdata.battery_info.technology;
> > + break;
> > + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> > + val->intval = bat->pdata.battery_info.voltage_min_design;
> > + break;
> > + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> > + val->intval = bat->pdata.battery_info.voltage_max_design;
> > + break;
> > + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> > + val->intval = bat->pdata.battery_info.charge_full_design;
> > + break;
> > + case POWER_SUPPLY_PROP_MODEL_NAME:
> > + val->strval = bat->pdata.battery_info.name;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + return ret;
> > +}
> > +
> [...]
> > +
> > +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
> > +{
> > +[...]
> > +
> > + /* assuming main battery and backup battery is using the same channel */
> > + for (i = 0; i < num_channels; i++) {
> > + ret = iio_get_channel_type(&channels[i], &type);
> > + if (ret < 0)
> > + goto err_gpio;
> > +
> > + if (type == IIO_VOLTAGE || type == IIO_CURRENT) {
> > + for (j = 0; j < BAT_MAX; j++) {
> > + generic_bat.bat[j].channel_index = k;
> > + generic_bat.bat[j].channels[k] = channels[i];
> > + }
> > + k++;
> > + }
> > + continue;
>
> continue at the end of a loop is a noop.
right.
>
> > + }
> > +
> > + INIT_DELAYED_WORK(&bat_work, generic_adc_bat_work);
> > +
> > + if (pdata->gpio_charge_finished >= 0) {
>
> gpio_is_valid
right.
>
> > + ret = gpio_request(pdata->gpio_charge_finished, "charged");
> > + if (ret)
> > + goto err_gpio;
> > +
> > + ret = request_irq(gpio_to_irq(pdata->gpio_charge_finished),
> > + generic_adc_bat_charged,
> > + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> > + "battery charged", NULL);
> > + if (ret)
> > + goto err_gpio;
> > + }
> > +
> > + dev_info(&pdev->dev, "successfully loaded\n");
>
> I'd remove that message.
ok.
>
> > +
> > + /* Schedule timer to check current status */
> > + schedule_delayed_work(&bat_work,
> > + msecs_to_jiffies(JITTER_DELAY));
> > +
> > + iio_channel_release_all(channels);
> > +
> > + return 0;
> > +
> > +err_gpio:
> > + iio_channel_release_all(channels);
> > +err_reg_backup:
> > + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
> > + power_supply_unregister(&generic_bat.bat[i].psy);
> > +err_reg_main:
> > + return ret;
> > +}
> > +
>
> [...]
>
> > +
> > +#ifdef CONFIG_PM
> > +static int generic_adc_bat_suspend(struct platform_device *pdev,
> > + pm_message_t state)
> > +{
> > + struct generic_platform_data *pdata = pdev->dev.platform_data;
> > + struct generic_bat *bat = container_of(pdata,
> > + struct generic_bat, pdata);
> > +
> > + cancel_delayed_work_sync(&bat_work);
> > + bat->status = POWER_SUPPLY_STATUS_UNKNOWN;
> > +
> > + return 0;
> > +}
> > +
> > +static int generic_adc_bat_resume(struct platform_device *pdev)
> > +{
> > + /* Schedule timer to check current status */
> > + schedule_delayed_work(&bat_work,
> > + msecs_to_jiffies(JITTER_DELAY));
> > +
> > + return 0;
> > +}
> > +#else
> > +#define generic_adc_bat_suspend NULL
> > +#define generic_adc_bat_resume NULL
> > +#endif
> > +
> > +static struct platform_driver generic_adc_bat_driver = {
> > + .driver = {
> > + .name = "generic-adc-battery",
> > + },
> > + .probe = generic_adc_bat_probe,
> > + .remove = generic_adc_bat_remove,
> > + .suspend = generic_adc_bat_suspend,
> > + .resume = generic_adc_bat_resume,
>
> Use dev_pm_ops instead. Using the suspend and resume callbacks is deprecated.
makes sense.
>
> > +};
> > +
> > +module_platform_driver(generic_adc_bat_driver);
> > +
> > +MODULE_AUTHOR("anish kumar <anish198519851985@gmail.com>");
> > +MODULE_DESCRIPTION("generic battery driver using IIO");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/power/generic-battery.h b/include/linux/power/generic-battery.h
> > new file mode 100644
> > index 0000000..7a43aa0
> > --- /dev/null
> > +++ b/include/linux/power/generic-battery.h
> > @@ -0,0 +1,26 @@
> > +/*
> > + * 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_BATTERY_H
> > +#define GENERIC_BATTERY_H
> > +
> > +#include <linux/power_supply.h>
> > +
> > +/**
> > + * struct generic_platform_data - platform data for generic battery
> > + * @battery_info: Information about the battery
> > + * @cal_charge: platform callback to calculate charge
> > + * @gpio_charge_finished: GPIO number used for interrupts
> > + * @gpio_inverted: Is the gpio inverted?
> > + */
> > +struct generic_platform_data {
>
> The name might be a bit to generic ;) I'd go for
> generic_battery_platform_data or iio_battery_platform_data.
it will be iio_battery_platform_data.
>
> > + struct power_supply_info battery_info;
> > + int (*cal_charge)(long);
> > + int gpio_charge_finished;
> > + int gpio_inverted;
> > +};
> > +
> > +#endif /* GENERIC_BATTERY_H */
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO
2012-09-08 7:30 ` anish kumar
@ 2012-09-08 10:10 ` Jonathan Cameron
0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2012-09-08 10:10 UTC (permalink / raw)
To: anish kumar
Cc: Lars-Peter Clausen, jic23, linux-iio, cbou, dwmw2, linux-kernel
On 09/08/2012 08:30 AM, anish kumar wrote:
> On Fri, 2012-09-07 at 10:49 +0200, Lars-Peter Clausen wrote:
>> On 09/02/2012 05:39 PM, anish kumar wrote:
>>> From: anish kumar <anish198519851985@gmail.com>
>>>
>>> This patch is to use IIO to write a generic batttery driver.
>>> There are some inherent assumptions here:
>>> 1.User is having both main battery and backup battery.
>>> 2.Both batteries use same channel to get the information.
>>>
>>
>> Hi thanks for stepping up to take care of this and sorry for the late reply.
> Thanks to you.
>>
>>> Signed-off-by: anish kumar <anish198519851985@gmail.com>
>>> ---
>> [...]
>>> +
>>> +struct generic_adc_bat {
>>> + struct power_supply psy;
>>> + struct iio_channel *channels;
>>> + int channel_index;
>>> +};
>>> +
>>> +struct generic_bat {
>>> + struct generic_adc_bat bat[BAT_MAX];
>>> + struct generic_platform_data pdata;
>>> + int volt_value;
>>> + int cur_value;
>>> + unsigned int timestamp;
>>> + int level;
>>> + int status;
>>> + int cable_plugged:1;
>>> +};
>>> +
>>> +static struct generic_bat generic_bat = {
>>> + .bat[MAIN_BAT] = {
>>> + .psy.name = "main-bat",
>>> + },
>>> + .bat[BACKUP_BAT] = {
>>> + .psy.name = "backup-bat",
>>> + },
>>> +};
>>
>> This should be per device. I'd also just use one power_supply per device. If
>> somebody has multiple batteries in their system they can register multiple
>> devices. This should make the driver more flexible.
> Yes makes sense of having one driver/device and if someone has more
> devices to register than it can be done by registering multiple devices
> with different platform data.
>>
>>> +
>>> +static struct delayed_work bat_work;
>>
>> This should also go into the generic_bat struct.
> right.
>>
>> +
>>> +static void generic_adc_bat_ext_power_changed(struct power_supply *psy)
>>> +{
>>> + schedule_delayed_work(&bat_work,
>>> + msecs_to_jiffies(JITTER_DELAY));
>>> +}
>>> +
>>> +static enum power_supply_property generic_adc_main_bat_props[] = {
>>> + 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,
>>> +};
>>
>> It probably make sense to create this at runtime, depending on which kind of
>> IIO channels are provided. E.g. for VOLTAGE_NOW/CURRENT_NOW.
> Yes, something like
> pdata->voltage_channel/pdata->current_channel/pdata->power_channel and
> if this property is set then register the VOLTAGE_NOW/CURRENT_NOW/POWER_
> property.
As stated in the other branch of this thread, just use standard naming and do this
via the iio_map that you'll need in platform data anyway.
>>
>> [...]
>>> +
>>> +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;
>>> + struct generic_bat *bat;
>>> + int ret, scaleint, scalepart, iio_val;
>>> + long result = 0;
>>> +
>>> + if (!strcmp(psy->name, "main-battery")) {
>>> + adc_bat = container_of(psy,
>>> + struct generic_adc_bat, psy);
>>> + bat = container_of(adc_bat,
>>> + struct generic_bat, bat[MAIN_BAT]);
>>> + } else if (!strcmp(psy->name, "backup-battery")) {
>>> + adc_bat = container_of(psy, struct generic_adc_bat, psy);
>>> + bat = container_of(adc_bat,
>>> + struct generic_bat, bat[BACKUP_BAT]);
>>> + } else {
>>> + /* Ideally this should never happen */
>>> + WARN_ON(1);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (!bat) {
>>> + dev_err(psy->dev, "no battery infos ?!\n");
>>> + return -EINVAL;
>>> + }
>>> + ret = iio_read_channel_raw(&adc_bat->channels[adc_bat->channel_index],
>>> + &iio_val);
>>> + ret = iio_read_channel_scale(&adc_bat->channels[adc_bat->channel_index],
>>> + &scaleint, &scalepart);
>>
>> It would probably make sense to only sample if the attribute for
>> VOLTAGE_NOW/CURRENT_NOW property is read.
> right.
>>
>> [...]
>>
>>> +
>>> + switch (psp) {
>>> + case POWER_SUPPLY_PROP_STATUS:
>>> + if (bat->pdata.gpio_charge_finished < 0)
>>> + val->intval = bat->level == 100000 ?
>>> + POWER_SUPPLY_STATUS_FULL : bat->status;
>>> + else
>>> + val->intval = bat->status;
>>> + return 0;
>>> + case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
>>> + val->intval = 0;
>>> + return 0;
>>> + case POWER_SUPPLY_PROP_CHARGE_NOW:
>>> + val->intval = bat->pdata.cal_charge(result);
>>> + return 0;
>>> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>>> + val->intval = result;
>>> + return 0;
>>> + case POWER_SUPPLY_PROP_CURRENT_NOW:
>>> + val->intval = result;
>>> + return 0;
>>
>> also this will return the same value for VOLTAGE_NOW and CURRENT_NOW since
>> there is only one channel per battery. It might make sense to allow multiple
>> channels per battery, one reporting current one voltage and also one for power.
> absolutely makes sense.
>>
>>
>>> + case POWER_SUPPLY_PROP_TECHNOLOGY:
>>> + val->intval = bat->pdata.battery_info.technology;
>>> + break;
>>> + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
>>> + val->intval = bat->pdata.battery_info.voltage_min_design;
>>> + break;
>>> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
>>> + val->intval = bat->pdata.battery_info.voltage_max_design;
>>> + break;
>>> + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>>> + val->intval = bat->pdata.battery_info.charge_full_design;
>>> + break;
>>> + case POWER_SUPPLY_PROP_MODEL_NAME:
>>> + val->strval = bat->pdata.battery_info.name;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> + return ret;
>>> +}
>>> +
>> [...]
>>> +
>>> +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
>>> +{
>>> +[...]
>>> +
>>> + /* assuming main battery and backup battery is using the same channel */
>>> + for (i = 0; i < num_channels; i++) {
>>> + ret = iio_get_channel_type(&channels[i], &type);
>>> + if (ret < 0)
>>> + goto err_gpio;
>>> +
>>> + if (type == IIO_VOLTAGE || type == IIO_CURRENT) {
>>> + for (j = 0; j < BAT_MAX; j++) {
>>> + generic_bat.bat[j].channel_index = k;
>>> + generic_bat.bat[j].channels[k] = channels[i];
>>> + }
>>> + k++;
>>> + }
>>> + continue;
>>
>> continue at the end of a loop is a noop.
> right.
>>
>>> + }
>>> +
>>> + INIT_DELAYED_WORK(&bat_work, generic_adc_bat_work);
>>> +
>>> + if (pdata->gpio_charge_finished >= 0) {
>>
>> gpio_is_valid
> right.
>>
>>> + ret = gpio_request(pdata->gpio_charge_finished, "charged");
>>> + if (ret)
>>> + goto err_gpio;
>>> +
>>> + ret = request_irq(gpio_to_irq(pdata->gpio_charge_finished),
>>> + generic_adc_bat_charged,
>>> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>> + "battery charged", NULL);
>>> + if (ret)
>>> + goto err_gpio;
>>> + }
>>> +
>>> + dev_info(&pdev->dev, "successfully loaded\n");
>>
>> I'd remove that message.
> ok.
>>
>>> +
>>> + /* Schedule timer to check current status */
>>> + schedule_delayed_work(&bat_work,
>>> + msecs_to_jiffies(JITTER_DELAY));
>>> +
>>> + iio_channel_release_all(channels);
>>> +
>>> + return 0;
>>> +
>>> +err_gpio:
>>> + iio_channel_release_all(channels);
>>> +err_reg_backup:
>>> + for (i = 0; i < ARRAY_SIZE(generic_bat.bat); i++)
>>> + power_supply_unregister(&generic_bat.bat[i].psy);
>>> +err_reg_main:
>>> + return ret;
>>> +}
>>> +
>>
>> [...]
>>
>>> +
>>> +#ifdef CONFIG_PM
>>> +static int generic_adc_bat_suspend(struct platform_device *pdev,
>>> + pm_message_t state)
>>> +{
>>> + struct generic_platform_data *pdata = pdev->dev.platform_data;
>>> + struct generic_bat *bat = container_of(pdata,
>>> + struct generic_bat, pdata);
>>> +
>>> + cancel_delayed_work_sync(&bat_work);
>>> + bat->status = POWER_SUPPLY_STATUS_UNKNOWN;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int generic_adc_bat_resume(struct platform_device *pdev)
>>> +{
>>> + /* Schedule timer to check current status */
>>> + schedule_delayed_work(&bat_work,
>>> + msecs_to_jiffies(JITTER_DELAY));
>>> +
>>> + return 0;
>>> +}
>>> +#else
>>> +#define generic_adc_bat_suspend NULL
>>> +#define generic_adc_bat_resume NULL
>>> +#endif
>>> +
>>> +static struct platform_driver generic_adc_bat_driver = {
>>> + .driver = {
>>> + .name = "generic-adc-battery",
>>> + },
>>> + .probe = generic_adc_bat_probe,
>>> + .remove = generic_adc_bat_remove,
>>> + .suspend = generic_adc_bat_suspend,
>>> + .resume = generic_adc_bat_resume,
>>
>> Use dev_pm_ops instead. Using the suspend and resume callbacks is deprecated.
> makes sense.
>>
>>> +};
>>> +
>>> +module_platform_driver(generic_adc_bat_driver);
>>> +
>>> +MODULE_AUTHOR("anish kumar <anish198519851985@gmail.com>");
>>> +MODULE_DESCRIPTION("generic battery driver using IIO");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/include/linux/power/generic-battery.h b/include/linux/power/generic-battery.h
>>> new file mode 100644
>>> index 0000000..7a43aa0
>>> --- /dev/null
>>> +++ b/include/linux/power/generic-battery.h
>>> @@ -0,0 +1,26 @@
>>> +/*
>>> + * 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_BATTERY_H
>>> +#define GENERIC_BATTERY_H
>>> +
>>> +#include <linux/power_supply.h>
>>> +
>>> +/**
>>> + * struct generic_platform_data - platform data for generic battery
>>> + * @battery_info: Information about the battery
>>> + * @cal_charge: platform callback to calculate charge
>>> + * @gpio_charge_finished: GPIO number used for interrupts
>>> + * @gpio_inverted: Is the gpio inverted?
>>> + */
>>> +struct generic_platform_data {
>>
>> The name might be a bit to generic ;) I'd go for
>> generic_battery_platform_data or iio_battery_platform_data.
> it will be iio_battery_platform_data.
>>
>>> + struct power_supply_info battery_info;
>>> + int (*cal_charge)(long);
>>> + int gpio_charge_finished;
>>> + int gpio_inverted;
>>> +};
>>> +
>>> +#endif /* GENERIC_BATTERY_H */
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2012-09-13 12:33 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-10 15:40 [PATCH] [RFC]power: battery: Generic battery driver using IIO anish kumar
2012-09-11 9:59 ` Lars-Peter Clausen
2012-09-12 8:30 ` anish singh
2012-09-13 12:26 ` Jonathan Cameron
2012-09-13 12:33 ` anish singh
-- strict thread matches above, loose matches on Subject: below --
2012-09-02 15:39 anish kumar
2012-09-02 16:14 ` Sannu K
2012-09-02 21:34 ` Anton Vorontsov
2012-09-04 15:05 ` anish kumar
2012-09-07 7:52 ` Jonathan Cameron
2012-09-08 7:10 ` anish kumar
2012-09-08 8:34 ` Anton Vorontsov
2012-09-08 10:09 ` Jonathan Cameron
2012-09-09 9:32 ` anish kumar
2012-09-09 9:46 ` Lars-Peter Clausen
2012-09-09 9:59 ` Jonathan Cameron
2012-09-07 8:49 ` Lars-Peter Clausen
2012-09-08 7:30 ` anish kumar
2012-09-08 10:10 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).