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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
> > + * 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@xxxxxxxxx>");
> > +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 */
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
