On 09/16/2012 09:14 AM, anish kumar wrote:
> From: anish kumar <[email protected]>
>
> In last version:
> Addressed concerns raised by lars:
> a. made the adc_bat per device.
> b. get the IIO channel using hardcoded channel names.
> c. Minor issues related to gpio_is_valid and some code
> refactoring.
>
> In this version:
> Addressed concerns raised by Anton:
> a. changed the struct name to gab(generic adc battery).
> b. Added some functions to neaten the code.
> c. Some minor coding guidelines changes.
> d. Used the latest function introduce by lars:
> iio_read_channel_processed to streamline the code.
>
> Signed-off-by: anish kumar <[email protected]>
> ---
> drivers/power/generic-adc-battery.c | 442
> +++++++++++++++++++++++++++++
> include/linux/power/generic-adc-battery.h | 19 ++
> 2 files changed, 461 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..fd62916
> --- /dev/null
> +++ b/drivers/power/generic-adc-battery.c
> @@ -0,0 +1,442 @@
> +/*
> + * Generic battery driver code using IIO
> + * Copyright (C) 2012, Anish Kumar <[email protected]>
> + * based on jz4740-battery.c
> + * based on s3c_adc_battery.c
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License. See the file COPYING in the main directory of this archive for
> + * more details.
> + *
> + */
> +#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>
> +
> +enum gab_chan_type {
> + VOLTAGE = 0,
> + CURRENT,
> + POWER,
> + MAX_CHAN_TYPE
> +};
> +
> +/*
> + * gab_chan_name suggests the standard channel names for commonly used
> + * channel types.
> + */
> +static char *gab_chan_name[] = {
const char *const
> + [VOLTAGE] = "voltage",
> + [CURRENT] = "current",
> + [POWER] = "power",
> +};
> +
> +struct gab {
> + struct power_supply psy;
> + struct iio_channel **channel;
> + struct gab_platform_data *pdata;
> + struct delayed_work bat_work;
> + int was_plugged;
> + int volt_value;
> + int cur_value;
The two above are never really used.
> + int level;
> + int status;
> + int cable_plugged:1;
> +};
[...]
> +static enum power_supply_property gab_props[] = {
const
> + 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 gab_dyn_props[] = {
const
> + POWER_SUPPLY_PROP_VOLTAGE_NOW,
> + POWER_SUPPLY_PROP_CURRENT_NOW,
> + POWER_SUPPLY_PROP_POWER_NOW,
> +};
> +
> +static bool charge_finished(struct gab *adc_bat)
missing prefix
> +{
> + bool ret = gpio_get_value(adc_bat->pdata->gpio_charge_finished);
> + bool inv = adc_bat->pdata->gpio_inverted;
> +
> + return ret ^ inv;
> +}
> +
> +int gab_get_status(struct gab *adc_bat)
static
> +{
> + struct gab_platform_data *pdata = adc_bat->pdata;
> + int chg_gpio = pdata->gpio_charge_finished;
> +
> + if (!gpio_is_valid(chg_gpio) || adc_bat->level < 100000)
level is never really set, is it? Also the 100000 seems to come directly from
the s3c_adc_battery driver, while this value may be different for every
battery. I'd use the bat_info->charge_full_design here. Also I'd remove the
!gpio_is_valid(chg_gpio) I don't see a reason why the battery could not be
fully charged even though no charger gpio is given.
> + return adc_bat->status;
> +
> + return POWER_SUPPLY_STATUS_FULL;
> +}
> +
> +enum gab_chan_type gab_prop_to_chan(enum power_supply_property psp)
static
> +{
> + switch (psp) {
> + case POWER_SUPPLY_PROP_POWER_NOW:
> + return POWER;
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + return VOLTAGE;
> + case POWER_SUPPLY_PROP_CURRENT_NOW:
> + return CURRENT;
> + default:
> + WARN_ON(1);
This WARN_ON will be hit quite often.
> + }
> + return POWER;
> +}
> +
> +int read_channel(struct gab *adc_bat, enum power_supply_property psp,
> + int *result)
static
> +{
> + int ret = 0;
> + int chan_index;
> +
> + chan_index = gab_prop_to_chan(psp);
> + ret = iio_read_channel_processed(adc_bat->channel[chan_index],
> + result);
> + if (ret < 0)
> + pr_err("read channel error\n");
> + return ret;
> +}
> +
> +static int gab_get_property(struct power_supply *psy,
> + enum power_supply_property psp, union power_supply_propval *val)
> +{
> + struct gab *adc_bat;
> + struct gab_platform_data *pdata;
> + struct power_supply_info *bat_info;
> + int result;
> + int ret = 0;
> +
> + adc_bat = to_generic_bat(psy);
> + if (!adc_bat) {
> + dev_err(psy->dev, "no battery infos ?!\n");
> + return -EINVAL;
> + }
> + pdata = adc_bat->pdata;
> + bat_info = &pdata->battery_info;
> +
> + ret = read_channel(adc_bat, psp, &result);
> + if (ret < 0)
> + goto err;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_STATUS:
> + gab_get_status(adc_bat);
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
> + val->intval = 0;
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_NOW:
> + val->intval = 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;
I'd do it this way, that also avoids all the WARN_ONs earlier
case POWER_SUPPLY_PROP_VOLTAGE_NOW
case POWER_SUPPLY_PROP_CURRENT_NOW:
case POWER_SUPPLY_PROP_POWER_NOW:
ret = read_channel(adc_bat, psp, &result);
if (ret < 0)
return ret
val->intval = result;
> + break;
> + case POWER_SUPPLY_PROP_TECHNOLOGY:
> + val->intval = bat_info->technology;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> + val->intval = bat_info->voltage_min_design;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> + val->intval = bat_info->voltage_max_design;
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> + val->intval = bat_info->charge_full_design;
> + break;
> + case POWER_SUPPLY_PROP_MODEL_NAME:
> + val->strval = bat_info->name;
> + break;
> + default:
> + return -EINVAL;
> + }
> +err:
> + return ret;
> +}
> +
> +static void gab_work(struct work_struct *work)
> +{
> + struct gab *adc_bat;
> + struct gab_platform_data *pdata;
> + 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 gab, bat_work);
> + pdata = adc_bat->pdata;
> +
> + 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;
I don't thin was_plugged is necessary. If is unplugged it is discharging, if it
is plugged it is either charging or full. And I think that's the logic that
should be used to set status. Something like.
if (!is_plugged)
adc_bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
else if (charge_finished(adc_bat))
adc_bat->status = POWER_SUPPLY_STATUS_FULL;
Maybe POWER_SUPPLY_STATUS_NOT_CHARGING is the better status here since we do
not necessarily know that it is full.
else
adc_bat->status = POWER_SUPPLY_STATUS_CHARGING;
This assumes that the gpio_is_valid checking is done in charge_finished and if
it returns false charge_finished just returns false.
> + if (is_plugged)
> + adc_bat->status = POWER_SUPPLY_STATUS_CHARGING;
> + else
> + adc_bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
> + } else {
> + if (gpio_is_valid(pdata->gpio_charge_finished)) {
> + if (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);
It makes probably sense to make a copy of adc_bat->status and if it did not
change don't generate an event.
> +}
> +
> +static irqreturn_t gab_charged(int irq, void *dev_id)
> +{
> + struct gab *adc_bat = dev_id;
> +
> + schedule_delayed_work(&adc_bat->bat_work,
> + msecs_to_jiffies(adc_bat->pdata->jitter_delay));
I would use a default jitter delay if it is 0. Does this make sense?
> + return IRQ_HANDLED;
> +}
> +
> +static int __devinit gab_probe(struct platform_device *pdev)
> +{
> + struct gab *adc_bat;
> + struct power_supply *psy;
> + struct gab_platform_data *pdata = pdev->dev.platform_data;
> + int ret;
> + int num_chans;
> + int fail = 0;
> +
> + adc_bat = devm_kzalloc(&pdev->dev, sizeof(*adc_bat), GFP_KERNEL);
> + if (!adc_bat) {
> + dev_err(&pdev->dev, "failed to allocate memory\n");
> + return -ENOMEM;
> + }
> +
> + psy = &adc_bat->psy;
> +
> + /* copying the battery name from platform data */
> + 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;
> + psy->type = POWER_SUPPLY_TYPE_BATTERY;
> + psy->get_property = gab_get_property;
> + psy->external_power_changed = gab_ext_power_changed;
> + adc_bat->pdata = pdata;
> +
> + /* calculate the total number of channels */
> + num_chans = ARRAY_SIZE(gab_chan_name);
> +
> + /*
> + * copying the static properties and allocating extra memory for holding
> + * the extra configurable properties received from platform data.
> + */
> + psy->properties = devm_kzalloc(&pdev->dev, sizeof(gab_props) +
> + sizeof(*(psy->properties)) * num_chans, GFP_KERNEL);
Maybe use kcalloc here with ARRAY_SIZE(gab_props) + ARRAY_SIZE(gab_chan_name)
as first parameter and sizeof(*psy->properties) as second.
> + if (!psy->properties) {
> + ret = -ENOMEM;
> + goto first_mem_fail;
> + }
> +
> + memcpy(psy->properties, gab_props, sizeof(gab_props));
> +
> + /* allocating memory for iio channels */
> + adc_bat->channel = devm_kzalloc(&pdev->dev,
> + num_chans * sizeof(struct iio_channel),
> + GFP_KERNEL);
Since the size is const you could just make channel an aray in the gab struct
and this allocation would not be needed.
> + if (!adc_bat->channel) {
> + ret = -ENOMEM;
> + goto first_mem_fail;
> + }
> +
> + /*
> + * getting channel from iio and copying the battery properties
> + * based on the channel supported by consumer device.
> + */
> + for (num_chans = 0; gab_chan_name[num_chans]; num_chans++) {
This won't work since gab_chan_name is not NULL terminated. Just make it
num_chans < ARRAY_SIZE(gab_chan_name). Btw. num_chans is really a bad name in
this case since it is an iterator variable. Just call it chan or something like
that.
> + adc_bat->channel[num_chans] =
> + iio_channel_get(dev_name(&pdev->dev),
> + gab_chan_name[num_chans]);
> + /* we skip for channels which fail */
> + if (IS_ERR(adc_bat->channel[num_chans])) {
> + pr_err("%s failed\n", gab_chan_name[num_chans]);
No error message here it will be quite likely that a battery does not provide
all three types of channels.
> + fail++;
> + } else {
> + static int index;
static?! Move it outsize of the loop an initialize it to ARRAY_SIZE(gab_props)
then you can skip the "extra + sizeof(gab_props)" here.
> + memcpy(psy->properties + sizeof(gab_props) +
> + sizeof(*(psy->properties))*index,
> + &gab_dyn_props[num_chans],
> + sizeof(gab_dyn_props[num_chans]));
> + index++;
> + }
> + }
> +
> + /* none of the channels are supported so let's bail out */
> + if (fail == num_chans) {
Just check whether index is != 0, you don't need to count the failed requests
then.
> + ret = PTR_ERR(adc_bat->channel[num_chans]);
num_chans will be outside of bounds of channel at this point.
> + goto first_mem_fail;
> + }
> +
> + /*
> + * Total number of properties is equal to static properties
> + * plus the dynamic properties.Some properties may not be set
> + * as come channels may be not be supported by the device.So
> + * we need to take care of that.
> + */
> + psy->num_properties = ARRAY_SIZE(gab_props) +
> + ARRAY_SIZE(gab_dyn_props) - fail;
> +
> + ret = power_supply_register(&pdev->dev, psy);
> + if (ret)
> + goto err_reg_fail;
> +
> + INIT_DELAYED_WORK(&adc_bat->bat_work, gab_work);
> +
> + if (gpio_is_valid(pdata->gpio_charge_finished)) {
> + int irq;
> + ret = gpio_request(pdata->gpio_charge_finished, "charged");
> + if (ret)
> + goto err_gpio;
> +
> + irq = gpio_to_irq(pdata->gpio_charge_finished);
> + ret = request_any_context_irq(irq, gab_charged,
> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> + "battery charged", &adc_bat);
Just "adc_bat", not "&adc_bat"
> + if (ret)
> + goto err_gpio;
You also need to free the if requesting the gpio fails.
> + } else
> + goto err_gpio; /* let's bail out */
> +
> + platform_set_drvdata(pdev, &adc_bat);
Same here
> +
> + /* Schedule timer to check current status */
> + schedule_delayed_work(&adc_bat->bat_work,
> + msecs_to_jiffies(0));
> + return 0;
> +
> +err_gpio:
> + power_supply_unregister(psy);
> +err_reg_fail:
> + for (num_chans = 0; gab_chan_name[num_chans]; num_chans++)
Here again, it's not NULL terminated
> + iio_channel_release(adc_bat->channel[num_chans]);
> +first_mem_fail:
> + return ret;
> +}
> +
> +static int gab_remove(struct platform_device *pdev)
__devexit
> +{
> + int num_chans;
> + struct gab_platform_data *pdata;
> + struct gab *adc_bat = platform_get_drvdata(pdev);
> +
> + pdata = adc_bat->pdata;
> + power_supply_unregister(&adc_bat->psy);
> +
> + if (gpio_is_valid(pdata->gpio_charge_finished)) {
> + free_irq(gpio_to_irq(pdata->gpio_charge_finished), NULL);
Needs adc_bat as the second agument
> + gpio_free(pdata->gpio_charge_finished);
> + }
> +
> + for (num_chans = 0; gab_chan_name[num_chans]; num_chans++)
And also here, it's not NULL terminated
> + iio_channel_release(adc_bat->channel[num_chans]);
> + cancel_delayed_work(&adc_bat->bat_work);
> + return 0;
> +}
> +
[...]
> +
> +static struct platform_driver gab_driver = {
> + .driver = {
> + .name = "generic-adc-battery",
> + .owner = THIS_MODULE,
> + .pm = GAB_PM_OPS
> + },
> + .probe = gab_probe,
> + .remove = gab_remove,
__devexit_p
> +};
> +module_platform_driver(gab_driver);
> +
> +MODULE_AUTHOR("anish kumar <[email protected]>");
> +MODULE_DESCRIPTION("generic battery driver using IIO");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/power/generic-adc-battery.h
> b/include/linux/power/generic-adc-battery.h
> new file mode 100644
> index 0000000..8f02e9e
> --- /dev/null
> +++ b/include/linux/power/generic-adc-battery.h
> @@ -0,0 +1,19 @@
> +/*
> + * 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
> +
Kernel doc for the struct would be nice.
> +struct gab_platform_data {
> + struct power_supply_info battery_info;
> + char *battery_name;
The power_supply_info struct also has a name field, it would make sense to use
it.
> + int (*cal_charge)(long value);
> + int gpio_charge_finished;
> + bool gpio_inverted;
> + int jitter_delay;
> +};
> +
> +#endif /* GENERIC_ADC_BATTERY_H */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
