[PATCH] power: battery: Generic battery driver using IIO

[PATCH] power: battery: Generic battery driver using IIO

[anish kumar]

From: anish kumar <anish198519851985@gmail.com>

In this version:
Addressed concerns raised by lars:
a. made the adc_bat per device.
b. get the IIO channel using hardcoded channel names.
c. Minor issues related to gpio_is_valid and some code
   refactoring.

Signed-off-by: anish kumar <anish198519851985@gmail.com>
---
 drivers/power/generic-adc-battery.c       |  460 +++++++++++++++++++++++++++++
 include/linux/power/generic-adc-battery.h |   33 ++
 2 files changed, 493 insertions(+), 0 deletions(-)
 create mode 100644 drivers/power/generic-adc-battery.c
 create mode 100644 include/linux/power/generic-adc-battery.h

diff --git a/drivers/power/generic-adc-battery.c b/drivers/power/generic-adc-battery.c
new file mode 100644
index 0000000..003e0e1
--- /dev/null
+++ b/drivers/power/generic-adc-battery.c
@@ -0,0 +1,460 @@
+/*
+ * Generic battery driver code using IIO
+ * Copyright (C) 2012, Anish Kumar <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)
+
+enum chan_type {
+	VOLTAGE = 0,
+	CURRENT,
+	POWER,
+	MAX_CHAN_TYPE
+};
+
+#define CHAN_MAX_NAME 30
+/*
+ * channel_name suggests the standard channel names for commonly used
+ * channel types.
+ */
+char channel_name[][CHAN_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);
+}
+
+long read_scale_channel(struct generic_adc_bat *adc_bat,
+			enum power_supply_property psp)
+{
+	int scaleint, scalepart, iio_val, ret = 0;
+	int result = 0;
+
+	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;
+	}
+	return result;
+}
+
+static int generic_adc_bat_get_property(struct power_supply *psy,
+		enum power_supply_property psp,
+		union power_supply_propval *val)
+{
+	struct generic_adc_bat *adc_bat;
+	int ret = 0;
+	long result;
+
+	adc_bat = to_generic_bat(psy);
+	if (!adc_bat) {
+		dev_err(psy->dev, "no battery infos ?!\n");
+		return -EINVAL;
+	}
+
+	result = read_scale_channel(adc_bat, psp);
+	if (result < 0) {
+		dev_err(psy->dev, "read channel error\n");
+		ret =  -EINVAL;
+		goto err;
+	}
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		if (gpio_is_valid(adc_bat->pdata->gpio_charge_finished)) {
+			if (adc_bat->pdata->gpio_charge_finished < 0)
+				val->intval = adc_bat->level == 100000 ?
+					POWER_SUPPLY_STATUS_FULL :
+					adc_bat->status;
+			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;
+	}
+err:
+	return ret;
+}
+
+static void generic_adc_bat_work(struct work_struct *work)
+{
+	struct generic_adc_bat *adc_bat;
+	struct delayed_work *delayed_work;
+	int is_charged;
+	int is_plugged;
+
+	delayed_work = container_of(work,
+				struct delayed_work, work);
+	adc_bat = container_of(delayed_work,
+				struct generic_adc_bat, bat_work);
+
+	is_plugged = power_supply_am_i_supplied(&adc_bat->psy);
+	adc_bat->cable_plugged = is_plugged;
+	if (is_plugged != adc_bat->was_plugged) {
+		adc_bat->was_plugged = is_plugged;
+		if (is_plugged)
+			adc_bat->status = POWER_SUPPLY_STATUS_CHARGING;
+		else
+			adc_bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
+	} else {
+		if (gpio_is_valid(adc_bat->pdata->gpio_charge_finished)) {
+			if ((adc_bat->pdata->gpio_charge_finished >= 0) &&
+								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;
+}
+
+static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
+{
+	struct generic_adc_bat *adc_bat;
+	struct iio_battery_platform_data *pdata = pdev->dev.platform_data;
+	int ret, chan_index, fail = 0;
+
+	adc_bat = devm_kzalloc(&pdev->dev, sizeof(*adc_bat), GFP_KERNEL);
+	if (!adc_bat) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	/* copying the battery name from platform data */
+	adc_bat->psy.name = pdata->battery_name;
+
+	/* bootup default values for the battery */
+	adc_bat->volt_value = -1;
+	adc_bat->cur_value = -1;
+	adc_bat->cable_plugged = 0;
+	adc_bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
+	adc_bat->psy.type = POWER_SUPPLY_TYPE_BATTERY;
+	adc_bat->psy.get_property =
+		generic_adc_bat_get_property;
+	adc_bat->psy.external_power_changed =
+		generic_adc_bat_ext_power_changed;
+
+	adc_bat->pdata = pdata;
+
+	/* calculate the total number of channels */
+	for (chan_index = 0; 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 supported by consumer device.
+	 */
+	for (chan_index = 0; channel_name[chan_index]; chan_index++) {
+		adc_bat->channel[chan_index] =
+		iio_channel_get(dev_name(&pdev->dev),
+			channel_name[chan_index]);
+		/* we skip for channels which fail */
+		if (IS_ERR(adc_bat->channel[chan_index]))
+			fail++;
+		else {
+			static int index;
+			memcpy(adc_bat->psy.properties +
+				sizeof(bat_props) + sizeof(int)*index,
+				&dyn_props[chan_index],
+				sizeof(dyn_props[chan_index]));
+			index++;
+		}
+	}
+
+	/* none of the channels are supported so let's bail out */
+	if (fail == chan_index) {
+		ret = PTR_ERR(adc_bat->channel[chan_index]);
+		goto second_mem_fail;
+	}
+
+	ret = power_supply_register(&pdev->dev, &adc_bat->psy);
+	if (ret)
+		goto err_reg_fail;
+
+	INIT_DELAYED_WORK(&adc_bat->bat_work, generic_adc_bat_work);
+
+	if (gpio_is_valid(pdata->gpio_charge_finished)) {
+		ret = gpio_request(pdata->gpio_charge_finished, "charged");
+		if (ret)
+			goto err_gpio;
+
+		ret = request_any_context_irq(gpio_to_irq
+				(pdata->gpio_charge_finished),
+				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; channel_name[chan_index]; chan_index++)
+		iio_channel_release(adc_bat->channel[chan_index]);
+	kfree(adc_bat->channel);
+second_mem_fail:
+	kfree(adc_bat->psy.properties);
+first_mem_fail:
+	return ret;
+}
+
+static int generic_adc_bat_remove(struct platform_device *pdev)
+{
+	int chan_index;
+	struct generic_adc_bat *adc_bat = platform_get_drvdata(pdev);
+
+	power_supply_unregister(&adc_bat->psy);
+
+	if (gpio_is_valid(adc_bat->pdata->gpio_charge_finished)) {
+		if (adc_bat->pdata->gpio_charge_finished >= 0) {
+			free_irq(gpio_to_irq(
+				adc_bat->pdata->gpio_charge_finished),
+								NULL);
+			gpio_free(adc_bat->pdata->gpio_charge_finished);
+		}
+	}
+
+	for (chan_index = 0; channel_name[chan_index]; chan_index++)
+		iio_channel_release(adc_bat->channel[chan_index]);
+	cancel_delayed_work(&adc_bat->bat_work);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int generic_adc_bat_suspend(struct device *dev)
+{
+	struct generic_adc_bat *adc_bat = dev_get_drvdata(dev);
+
+	cancel_delayed_work_sync(&adc_bat->bat_work);
+	adc_bat->status = POWER_SUPPLY_STATUS_UNKNOWN;
+	return 0;
+}
+
+static int generic_adc_bat_resume(struct device *dev)
+{
+	struct generic_adc_bat *adc_bat = dev_get_drvdata(dev);
+
+	/* Schedule timer to check current status */
+	schedule_delayed_work(&adc_bat->bat_work,
+			msecs_to_jiffies(adc_bat->pdata->jitter_delay));
+	return 0;
+}
+
+static const struct dev_pm_ops generic_adc_battery_pm_ops = {
+	.suspend        = generic_adc_bat_suspend,
+	.resume         = generic_adc_bat_resume,
+};
+
+#define GENERIC_ADC_BATTERY_PM_OPS       (&generic_adc_battery_pm_ops)
+#else
+#define GENERIC_ADC_BATTERY_PM_OPS       (NULL)
+#endif
+
+static struct platform_driver generic_adc_bat_driver = {
+	.driver		= {
+		.name	= "generic-adc-battery",
+		.owner	= THIS_MODULE,
+		.pm	= GENERIC_ADC_BATTERY_PM_OPS
+	},
+	.probe		= generic_adc_bat_probe,
+	.remove		= generic_adc_bat_remove,
+};
+
+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

Re: [PATCH] power: battery: Generic battery driver using IIO

[Jonathan Cameron]

On 09/13/2012 05:16 PM, anish kumar wrote:
> From: anish kumar <anish198519851985@gmail.com>
>
> In this version:
> Addressed concerns raised by lars:
> a. made the adc_bat per device.
> b. get the IIO channel using hardcoded channel names.
Couple of bits related to this inline..

> c. Minor issues related to gpio_is_valid and some code
>    refactoring.
>
> Signed-off-by: anish kumar <anish198519851985@gmail.com>
> ---
>  drivers/power/generic-adc-battery.c       |  460 +++++++++++++++++++++++++++++
>  include/linux/power/generic-adc-battery.h |   33 ++
>  2 files changed, 493 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/power/generic-adc-battery.c
>  create mode 100644 include/linux/power/generic-adc-battery.h
>
> diff --git a/drivers/power/generic-adc-battery.c b/drivers/power/generic-adc-battery.c
> new file mode 100644
> index 0000000..003e0e1
> --- /dev/null
> +++ b/drivers/power/generic-adc-battery.c
> @@ -0,0 +1,460 @@
> +/*
> + * Generic battery driver code using IIO
> + * Copyright (C) 2012, Anish Kumar <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)
> +
> +enum chan_type {
> +	VOLTAGE = 0,
> +	CURRENT,
> +	POWER,
> +	MAX_CHAN_TYPE
> +};
> +
> +#define CHAN_MAX_NAME 30
> +/*
> + * channel_name suggests the standard channel names for commonly used
> + * channel types.
> + */
const
> +char channel_name[][CHAN_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);
> +}
> +
> +long read_scale_channel(struct generic_adc_bat *adc_bat,
> +			enum power_supply_property psp)
> +{
> +	int scaleint, scalepart, iio_val, ret = 0;
> +	int result = 0;
> +
> +	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;
> +	}
> +	return result;
> +}
> +
> +static int generic_adc_bat_get_property(struct power_supply *psy,
> +		enum power_supply_property psp,
> +		union power_supply_propval *val)
> +{
> +	struct generic_adc_bat *adc_bat;
> +	int ret = 0;
> +	long result;
> +
> +	adc_bat = to_generic_bat(psy);
> +	if (!adc_bat) {
> +		dev_err(psy->dev, "no battery infos ?!\n");
> +		return -EINVAL;
> +	}
> +
> +	result = read_scale_channel(adc_bat, psp);
> +	if (result < 0) {
> +		dev_err(psy->dev, "read channel error\n");
> +		ret =  -EINVAL;
> +		goto err;
> +	}
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		if (gpio_is_valid(adc_bat->pdata->gpio_charge_finished)) {
> +			if (adc_bat->pdata->gpio_charge_finished < 0)
> +				val->intval = adc_bat->level == 100000 ?
> +					POWER_SUPPLY_STATUS_FULL :
> +					adc_bat->status;
> +			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;
> +	}
> +err:
> +	return ret;
> +}
> +
> +static void generic_adc_bat_work(struct work_struct *work)
> +{
> +	struct generic_adc_bat *adc_bat;
> +	struct delayed_work *delayed_work;
> +	int is_charged;
> +	int is_plugged;
> +
> +	delayed_work = container_of(work,
> +				struct delayed_work, work);
> +	adc_bat = container_of(delayed_work,
> +				struct generic_adc_bat, bat_work);
> +
> +	is_plugged = power_supply_am_i_supplied(&adc_bat->psy);
> +	adc_bat->cable_plugged = is_plugged;
> +	if (is_plugged != adc_bat->was_plugged) {
> +		adc_bat->was_plugged = is_plugged;
> +		if (is_plugged)
> +			adc_bat->status = POWER_SUPPLY_STATUS_CHARGING;
> +		else
> +			adc_bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
> +	} else {
> +		if (gpio_is_valid(adc_bat->pdata->gpio_charge_finished)) {
> +			if ((adc_bat->pdata->gpio_charge_finished >= 0) &&
> +								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;
> +}
> +
> +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
> +{
> +	struct generic_adc_bat *adc_bat;
> +	struct iio_battery_platform_data *pdata = pdev->dev.platform_data;
> +	int ret, chan_index, fail = 0;
> +
> +	adc_bat = devm_kzalloc(&pdev->dev, sizeof(*adc_bat), GFP_KERNEL);
> +	if (!adc_bat) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* copying the battery name from platform data */
> +	adc_bat->psy.name = pdata->battery_name;
> +
> +	/* bootup default values for the battery */
> +	adc_bat->volt_value = -1;
> +	adc_bat->cur_value = -1;
> +	adc_bat->cable_plugged = 0;
> +	adc_bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
> +	adc_bat->psy.type = POWER_SUPPLY_TYPE_BATTERY;
> +	adc_bat->psy.get_property =
> +		generic_adc_bat_get_property;
> +	adc_bat->psy.external_power_changed =
> +		generic_adc_bat_ext_power_changed;
> +
> +	adc_bat->pdata = pdata;
> +
> +	/* calculate the total number of channels */
> +	for (chan_index = 0; 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 supported by consumer device.
> +	 */
> +	for (chan_index = 0; channel_name[chan_index]; chan_index++) {
> +		adc_bat->channel[chan_index] =
> +		iio_channel_get(dev_name(&pdev->dev),
> +			channel_name[chan_index]);
> +		/* we skip for channels which fail */
> +		if (IS_ERR(adc_bat->channel[chan_index]))
> +			fail++;
> +		else {
> +			static int index;
> +			memcpy(adc_bat->psy.properties +
> +				sizeof(bat_props) + sizeof(int)*index,
> +				&dyn_props[chan_index],
> +				sizeof(dyn_props[chan_index]));
> +			index++;
> +		}
> +	}
> +
> +	/* none of the channels are supported so let's bail out */
> +	if (fail == chan_index) {
> +		ret = PTR_ERR(adc_bat->channel[chan_index]);
> +		goto second_mem_fail;
> +	}
> +
> +	ret = power_supply_register(&pdev->dev, &adc_bat->psy);
> +	if (ret)
> +		goto err_reg_fail;
> +
> +	INIT_DELAYED_WORK(&adc_bat->bat_work, generic_adc_bat_work);
> +
> +	if (gpio_is_valid(pdata->gpio_charge_finished)) {
> +		ret = gpio_request(pdata->gpio_charge_finished, "charged");
> +		if (ret)
> +			goto err_gpio;
> +
> +		ret = request_any_context_irq(gpio_to_irq
> +				(pdata->gpio_charge_finished),
> +				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; channel_name[chan_index]; chan_index++)
> +		iio_channel_release(adc_bat->channel[chan_index]);
> +	kfree(adc_bat->channel);
> +second_mem_fail:
> +	kfree(adc_bat->psy.properties);
> +first_mem_fail:
> +	return ret;
> +}
> +
> +static int generic_adc_bat_remove(struct platform_device *pdev)
> +{
> +	int chan_index;
> +	struct generic_adc_bat *adc_bat = platform_get_drvdata(pdev);
> +
> +	power_supply_unregister(&adc_bat->psy);
> +
> +	if (gpio_is_valid(adc_bat->pdata->gpio_charge_finished)) {
> +		if (adc_bat->pdata->gpio_charge_finished >= 0) {
> +			free_irq(gpio_to_irq(
> +				adc_bat->pdata->gpio_charge_finished),
> +								NULL);
> +			gpio_free(adc_bat->pdata->gpio_charge_finished);
> +		}
> +	}
> +
> +	for (chan_index = 0; channel_name[chan_index]; chan_index++)
> +		iio_channel_release(adc_bat->channel[chan_index]);
> +	cancel_delayed_work(&adc_bat->bat_work);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int generic_adc_bat_suspend(struct device *dev)
> +{
> +	struct generic_adc_bat *adc_bat = dev_get_drvdata(dev);
> +
> +	cancel_delayed_work_sync(&adc_bat->bat_work);
> +	adc_bat->status = POWER_SUPPLY_STATUS_UNKNOWN;
> +	return 0;
> +}
> +
> +static int generic_adc_bat_resume(struct device *dev)
> +{
> +	struct generic_adc_bat *adc_bat = dev_get_drvdata(dev);
> +
> +	/* Schedule timer to check current status */
> +	schedule_delayed_work(&adc_bat->bat_work,
> +			msecs_to_jiffies(adc_bat->pdata->jitter_delay));
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops generic_adc_battery_pm_ops = {
> +	.suspend        = generic_adc_bat_suspend,
> +	.resume         = generic_adc_bat_resume,
> +};
> +
> +#define GENERIC_ADC_BATTERY_PM_OPS       (&generic_adc_battery_pm_ops)
> +#else
> +#define GENERIC_ADC_BATTERY_PM_OPS       (NULL)
> +#endif
> +
> +static struct platform_driver generic_adc_bat_driver = {
> +	.driver		= {
> +		.name	= "generic-adc-battery",
> +		.owner	= THIS_MODULE,
> +		.pm	= GENERIC_ADC_BATTERY_PM_OPS
> +	},
> +	.probe		= generic_adc_bat_probe,
> +	.remove		= generic_adc_bat_remove,
> +};
> +
> +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
> +};
> +
why this definition?
> +extern char channel_name[][BAT_MAX_NAME + 1];
> +
> +struct iio_battery_platform_data {
> +	struct power_supply_info battery_info;

Should this still be here?
> +	char	**channel_name;
> +	char	*battery_name;
> +	int	(*cal_charge)(long);
> +	int	gpio_charge_finished;
> +	int	gpio_inverted;

unused..
> +	int	bat_poll_interval;
> +	int	jitter_delay;
> +};
> +
> +#endif /* GENERIC_ADC_BATTERY_H */
>

Re: [PATCH] power: battery: Generic battery driver using IIO

[Anton Vorontsov]

On Thu, Sep 13, 2012 at 09:46:32PM +0530, anish kumar wrote:
> From: anish kumar <anish198519851985@gmail.com>
> 
> In this version:
> Addressed concerns raised by lars:
> a. made the adc_bat per device.
> b. get the IIO channel using hardcoded channel names.
> c. Minor issues related to gpio_is_valid and some code
>    refactoring.
> 
> Signed-off-by: anish kumar <anish198519851985@gmail.com>
> ---

Anish, much thanks for your work! This is much appreciated.

There are a few comments below, they don't require any design
changes, just some technical issues, which wouldn't hard to fix,
I guess.

And again, thanks for your efforts!

>  drivers/power/generic-adc-battery.c       |  460 +++++++++++++++++++++++++++++
>  include/linux/power/generic-adc-battery.h |   33 ++
>  2 files changed, 493 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/power/generic-adc-battery.c
>  create mode 100644 include/linux/power/generic-adc-battery.h
> 
> diff --git a/drivers/power/generic-adc-battery.c b/drivers/power/generic-adc-battery.c
> new file mode 100644
> index 0000000..003e0e1
> --- /dev/null
> +++ b/drivers/power/generic-adc-battery.c
> @@ -0,0 +1,460 @@
> +/*
> + * Generic battery driver code using IIO
> + * Copyright (C) 2012, Anish Kumar <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.
> + *

No need for this empty comment line.

> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/gpio.h>
> +#include <linux/err.h>
> +#include <linux/timer.h>
> +#include <linux/jiffies.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/types.h>
> +

No need for this empty line.

> +#include <linux/power/generic-adc-battery.h>
> +
> +#define to_generic_bat(ptr)	\
> +	container_of(ptr, struct generic_adc_bat, ptr)

This will fit on one line.

Plus, it look strange that you use 'struct generic_adc_bat' before
actually declaring it. :-) Surely it works because it's a macro.

Also, while this is a simple one-liner, I'd really suggest making
it a function. That way you also won't have a risk to evaluate
'ptr' expression twice, plus the function is type-safe (unlike
the macro).

> +
> +enum chan_type {
> +	VOLTAGE = 0,
> +	CURRENT,
> +	POWER,
> +	MAX_CHAN_TYPE

The enum is in the global namespace. Please prefix items with
GEN_ADC_BAT (or GENERIC_ADC_BAT, but that would be too long).

Personally, I like making acronyms. I.e., while file name already
makes it pretty clear that it is generic_adc_battery, in the file
itself you can use short gab_ and GAB_ everywhere.

Anyways, it's up to you. generic_adc_bat_ prefix is also fine. But
be consistent.

The same with chan_type, please prefix it with something.

> +};
> +
> +#define CHAN_MAX_NAME 30
> +/*
> + * channel_name suggests the standard channel names for commonly used
> + * channel types.
> + */
> +char channel_name[][CHAN_MAX_NAME + 1] = {

Ditto. gab_channel_name.

Plus, you can make it char *gab_channel_name[] = {
	[VOLTAGE] = "voltage",
	[...] = ...,
};

Thus no need for MAX_NAME.

> +	[VOLTAGE]	= "voltage",
> +	[CURRENT]	= "current",
> +	[POWER]		= "power",
> +};
> +
> +struct generic_adc_bat {
> +	struct power_supply	psy;
> +	struct iio_channel	**channel;
> +	struct iio_battery_platform_data	*pdata;
> +	struct delayed_work bat_work;
> +	int		was_plugged;
> +	int		volt_value;
> +	int		cur_value;
> +	int		level;
> +	int		status;
> +	int		cable_plugged:1;
> +};
> +
> +static void generic_adc_bat_ext_power_changed(struct power_supply *psy)
> +{
> +	struct generic_adc_bat *adc_bat;
> +	adc_bat = to_generic_bat(psy);

This would fit into one line.

> +
> +	schedule_delayed_work(&adc_bat->bat_work,
> +			msecs_to_jiffies(adc_bat->pdata->jitter_delay));
> +}
> +
> +static enum power_supply_property bat_props[] = {

While it's static, it still looks awkward that you don't prefix it.

> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> +	POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN,
> +	POWER_SUPPLY_PROP_CHARGE_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> +	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> +	POWER_SUPPLY_PROP_MODEL_NAME,
> +};
> +
> +/*
> + * This properties are set based on the received platform data and this
> + * should correspond one-to-one with enum chan_type.
> + */
> +static enum power_supply_property dyn_props[] = {
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_POWER_NOW,
> +};
> +
> +static int charge_finished(struct generic_adc_bat *adc_bat)
> +{
> +	return adc_bat->pdata->gpio_inverted ?
> +		!gpio_get_value(adc_bat->pdata->gpio_charge_finished) :
> +		gpio_get_value(adc_bat->pdata->gpio_charge_finished);

That looks unreadable.

How about:

bool ret = gpio_get_value(adc_bat->pdata->gpio_charge_finished);
bool inv = adc_bat->pdata->gpio_inverted;

return ret ^ inv;

(you can also make gpio_inverted bool, as well as return value of
charge_finished function)

> +}
> +
> +long read_scale_channel(struct generic_adc_bat *adc_bat,
> +			enum power_supply_property psp)

static? prefix?

> +{
> +	int scaleint, scalepart, iio_val, ret = 0;

One variable declaration per line, please.

int scaleint;
int scalepart;
etc.

> +	int result = 0;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_POWER_NOW:
> +		ret = iio_read_channel_raw(adc_bat->channel[POWER],
> +				&iio_val);

you don't use this return value, why assign?

Also, this is very repeated.

How about introducing a function
int gab_read_chan(enum gab_chan, int *val, int *scaleint, int *scalepart)
{
	int ret;

	ret = iio_read_channel_raw(chan, val);
	if (ret < 0)
		return ret;

	ret = iio_read_channel_scale(chan, scaleint, scalepart);
	if (ret < 0)
		return ret;

	return 0;
}

Then you'll have

switch (psp) {
	case POWER_SUPPLY_PROP_POWER_NOW:
		ret = gab_read_chan(adc_bat->channel[POWER],
				    &iio_val, &scaleint, &scalepart);
		break;
	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
		ret = gab_read_chan(adc_bat->channel[VOLTAGE],
				    &iio_val, &scaleint, &scalepart);
		break;
	....
}

if (ret < 0)
	return ret;

It is easily noticed that this is quite repeating too.

So we can do better, let's introduce property-to-channel function:

enum gab_chan gab_prop_to_chan(enum power_supply_property psp)
{
	switch (psp) {
	case POWER_SUPPLY_PROP_POWER_NOW:
		return GAB_POWER;
	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
		return GAB_VOLTAGE;
	....
	}
	WARN_ON(1);
	return GAB_POWER;
}


And then we can just do this:

enum gab_chan chan = gab_prop_to_chan(psp);

ret = gab_read_chan(chan, &iio_val, &scaleint, &scalepart);
if (ret < 0)
	return 0;

Which is much nicer. :-)

> +		ret = iio_read_channel_scale(adc_bat->channel[POWER],
> +				&scaleint, &scalepart);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		ret = iio_read_channel_raw(adc_bat->channel[VOLTAGE],
> +				&iio_val);
> +		ret = iio_read_channel_scale(adc_bat->channel[VOLTAGE],
> +				&scaleint, &scalepart);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		ret = iio_read_channel_raw(adc_bat->channel[CURRENT],
> +				&iio_val);
> +		ret = iio_read_channel_scale(adc_bat->channel[CURRENT],
> +				&scaleint, &scalepart);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	switch (ret) {
> +	case IIO_VAL_INT:
> +		result = iio_val * scaleint;
> +		break;
> +	case IIO_VAL_INT_PLUS_MICRO:
> +		result = (s64)iio_val * (s64)scaleint +
> +			div_s64((s64)iio_val * (s64)scalepart, 1000000LL);
> +		break;
> +	case IIO_VAL_INT_PLUS_NANO:
> +		result = (s64)iio_val * (s64)scaleint +
> +			div_s64((s64)iio_val * (s64)scalepart, 1000000000LL);
> +		break;
> +	}
> +	return result;
> +}
> +
> +static int generic_adc_bat_get_property(struct power_supply *psy,
> +		enum power_supply_property psp,
> +		union power_supply_propval *val)
> +{
> +	struct generic_adc_bat *adc_bat;
> +	int ret = 0;
> +	long result;
> +
> +	adc_bat = to_generic_bat(psy);
> +	if (!adc_bat) {
> +		dev_err(psy->dev, "no battery infos ?!\n");
> +		return -EINVAL;
> +	}
> +
> +	result = read_scale_channel(adc_bat, psp);
> +	if (result < 0) {
> +		dev_err(psy->dev, "read channel error\n");
> +		ret =  -EINVAL;
> +		goto err;
> +	}
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		if (gpio_is_valid(adc_bat->pdata->gpio_charge_finished)) {
> +			if (adc_bat->pdata->gpio_charge_finished < 0)
> +				val->intval = adc_bat->level == 100000 ?
> +					POWER_SUPPLY_STATUS_FULL :
> +					adc_bat->status;

This is u?n(r)e:a<d?a->b=l&e. :-)

Plus, I guess there's some logic error: you check gpio_charge_finished
for gpio_is_valid, and then for it "< 0", which is always false. I
think you don't need "< 0" check.

How about a separate function for this?

I.e.

int gab_get_status(struct generic_adc_bat *gab)
{
	struct gab_platform_data *pdata = gab->pdata;
	int chg_gpio = pdata->gpio_charge_finished

	if (!gpio_is_valid(chg_gpio) || adc_bat->level < 100000)
		return adc_bat->status;

	return POWER_SUPPLY_STATUS_FULL;
}

> +			else
> +				val->intval = adc_bat->status;
> +		}
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
> +		val->intval = 0;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_NOW:
> +		val->intval = adc_bat->pdata->cal_charge(result);
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		val->intval = result;
> +		break;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		val->intval = result;
> +		break;
> +	case POWER_SUPPLY_PROP_POWER_NOW:
> +		val->intval = result;
> +		break;
> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> +		val->intval = adc_bat->pdata->battery_info.technology;
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> +		val->intval = adc_bat->pdata->battery_info.voltage_min_design;

You repeat 'adc_bat->pdata->battery_info' too much. Please introduce
short a variable. I.e.

struct power_supply_info *bi = &adc_bat->pdata->battery_info;

Then you can type bi->voltage_min_design;

> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> +		val->intval = adc_bat->pdata->battery_info.voltage_max_design;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> +		val->intval = adc_bat->pdata->battery_info.charge_full_design;
> +		break;
> +	case POWER_SUPPLY_PROP_MODEL_NAME:
> +		val->strval = adc_bat->pdata->battery_info.name;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +err:
> +	return ret;
> +}
> +
> +static void generic_adc_bat_work(struct work_struct *work)
> +{
> +	struct generic_adc_bat *adc_bat;
> +	struct delayed_work *delayed_work;
> +	int is_charged;
> +	int is_plugged;
> +
> +	delayed_work = container_of(work,
> +				struct delayed_work, work);
This fits into one line.

> +	adc_bat = container_of(delayed_work,
> +				struct generic_adc_bat, bat_work);

Ditto.

> +	is_plugged = power_supply_am_i_supplied(&adc_bat->psy);
> +	adc_bat->cable_plugged = is_plugged;
> +	if (is_plugged != adc_bat->was_plugged) {
> +		adc_bat->was_plugged = is_plugged;
> +		if (is_plugged)
> +			adc_bat->status = POWER_SUPPLY_STATUS_CHARGING;
> +		else
> +			adc_bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
> +	} else {
> +		if (gpio_is_valid(adc_bat->pdata->gpio_charge_finished)) {
> +			if ((adc_bat->pdata->gpio_charge_finished >= 0) &&

If you use gpio_is_valid, you don't need another >= check.

> +								is_plugged) {
> +				is_charged = charge_finished(adc_bat);
> +				if (is_charged)
> +					adc_bat->status =
> +						POWER_SUPPLY_STATUS_FULL;
> +				else
> +					adc_bat->status =
> +						POWER_SUPPLY_STATUS_CHARGING;
> +			}
> +		}
> +	}
> +
> +	power_supply_changed(&adc_bat->psy);
> +}
> +
> +static irqreturn_t generic_adc_bat_charged(int irq, void *dev_id)
> +{
> +	struct generic_adc_bat *adc_bat = dev_id;

Need an empty line here.

> +	adc_bat = container_of(&adc_bat->bat_work,
> +				struct generic_adc_bat, bat_work);

Something is seriously wrong here. You probably don't need container_of.

Plus you pass pointer-to-pointer to request_irq()... I doubt it will
work.

> +	schedule_delayed_work(&adc_bat->bat_work,
> +			msecs_to_jiffies(adc_bat->pdata->jitter_delay));
> +	return IRQ_HANDLED;
> +}
> +
> +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
> +{
> +	struct generic_adc_bat *adc_bat;
> +	struct iio_battery_platform_data *pdata = pdev->dev.platform_data;
> +	int ret, chan_index, fail = 0;

One variable per line.

int ret;
int chan_index;
...

> +
> +	adc_bat = devm_kzalloc(&pdev->dev, sizeof(*adc_bat), GFP_KERNEL);
> +	if (!adc_bat) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* copying the battery name from platform data */
> +	adc_bat->psy.name = pdata->battery_name;
> +
> +	/* bootup default values for the battery */
> +	adc_bat->volt_value = -1;
> +	adc_bat->cur_value = -1;
> +	adc_bat->cable_plugged = 0;
> +	adc_bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
> +	adc_bat->psy.type = POWER_SUPPLY_TYPE_BATTERY;
> +	adc_bat->psy.get_property =
> +		generic_adc_bat_get_property;
> +	adc_bat->psy.external_power_changed =
> +		generic_adc_bat_ext_power_changed;

These four lines will fit into two.

> +	adc_bat->pdata = pdata;
> +
> +	/* calculate the total number of channels */
> +	for (chan_index = 0; channel_name[chan_index]; chan_index++)
> +		;

I think you can use ARRAY_SIZE(channel_name) macro from linux/kernel.h
instead of the loop.

Plus, you should really name it num_chans.

> +
> +	if (!chan_index) {
> +		pr_err("atleast provide one channel\n");

The chan_index is calculated from the static array that never changes.
I think you don't need this check.

> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * copying the static properties and allocating extra memory for holding
> +	 * the extra configurable properties received from platform data.
> +	*/

*/ is wrongly indented.

> +	adc_bat->psy.properties = kzalloc(sizeof(bat_props)
> +					+ sizeof(int)*chan_index, GFP_KERNEL);

Instead of sizeof(int), better sizeof(*adc_bat->psy.properties).

Also, you need whitespaces around *.

And I don't see where you're filling-in num_properties. How
does it work without it?

> +	if (!adc_bat->psy.properties) {
> +		ret = -ENOMEM;
> +		goto first_mem_fail;
> +	}
> +	memcpy(adc_bat->psy.properties, bat_props, sizeof(bat_props));
> +
> +	/* allocating memory for iio channels */
> +	adc_bat->channel = kzalloc(chan_index * sizeof(struct iio_channel),
> +					GFP_KERNEL);

adc_bat->channels = kzalloc(num_chans * sizeof(*adc_bat->channels),
			    GFP_KERNEL);

Plus, I think you can use devm_kzalloc() here, and above.

> +	if (!adc_bat->channel) {
> +		ret = -ENOMEM;
> +		goto second_mem_fail;
> +	}
> +
> +	/*
> +	 * getting channel from iio and copying the battery properties
> +	 * based on the channel supported by consumer device.
> +	 */
> +	for (chan_index = 0; channel_name[chan_index]; chan_index++) {
> +		adc_bat->channel[chan_index] =
> +		iio_channel_get(dev_name(&pdev->dev),
> +			channel_name[chan_index]);
> +		/* we skip for channels which fail */
> +		if (IS_ERR(adc_bat->channel[chan_index]))
> +			fail++;
> +		else {

Need {} braces for if statement too, so it'll have to be
} else{, per coding style.

> +			static int index;

empty line here please.

> +			memcpy(adc_bat->psy.properties +
> +				sizeof(bat_props) + sizeof(int)*index,
> +				&dyn_props[chan_index],
> +				sizeof(dyn_props[chan_index]));

I'm too lazy to even think what it does... I hope it works. :-D

> +			index++;
> +		}
> +	}
> +
> +	/* none of the channels are supported so let's bail out */
> +	if (fail == chan_index) {
> +		ret = PTR_ERR(adc_bat->channel[chan_index]);
> +		goto second_mem_fail;
> +	}
> +
> +	ret = power_supply_register(&pdev->dev, &adc_bat->psy);
> +	if (ret)
> +		goto err_reg_fail;
> +
> +	INIT_DELAYED_WORK(&adc_bat->bat_work, generic_adc_bat_work);
> +
> +	if (gpio_is_valid(pdata->gpio_charge_finished)) {
> +		ret = gpio_request(pdata->gpio_charge_finished, "charged");
> +		if (ret)
> +			goto err_gpio;
> +
> +		ret = request_any_context_irq(gpio_to_irq
> +				(pdata->gpio_charge_finished),

Very unfortunate line break, at first I thought that you don't
need parenthesis, but then I noticed that it's actually a function
call. Heh. please introduce a local variable int irq; above (just
after if (gpio_is_valid)...

i.e.

int irq = gpio_to_irq(pdata->gpio_charge_finished);

ret = request_any_context_irq(irq, .....).

> +				generic_adc_bat_charged,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				"battery charged", &adc_bat);

Here, &adc_bat (pointer-to-pointer) looks very wrong. Why do you do
this?

> +		if (ret)
> +			goto err_gpio;
> +	} else
> +		goto err_gpio; /* let's bail out */

} else {
	...
}

> +	platform_set_drvdata(pdev, &adc_bat);
> +
> +	/* Schedule timer to check current status */
> +	schedule_delayed_work(&adc_bat->bat_work,
> +			msecs_to_jiffies(0));
> +	return 0;
> +
> +err_gpio:
> +	power_supply_unregister(&adc_bat->psy);
> +err_reg_fail:
> +	for (chan_index = 0; channel_name[chan_index]; chan_index++)
> +		iio_channel_release(adc_bat->channel[chan_index]);
> +	kfree(adc_bat->channel);
> +second_mem_fail:
> +	kfree(adc_bat->psy.properties);
> +first_mem_fail:
> +	return ret;
> +}
> +
> +static int generic_adc_bat_remove(struct platform_device *pdev)
> +{
> +	int chan_index;
> +	struct generic_adc_bat *adc_bat = platform_get_drvdata(pdev);
> +
> +	power_supply_unregister(&adc_bat->psy);
> +
> +	if (gpio_is_valid(adc_bat->pdata->gpio_charge_finished)) {
> +		if (adc_bat->pdata->gpio_charge_finished >= 0) {

You don't need >= check. gpio_is_valid is used for this.

> +			free_irq(gpio_to_irq(
> +				adc_bat->pdata->gpio_charge_finished),
> +								NULL);

Please don't be scared to introduce temporary variables if they make
code more readable. Here, introduce 'int irq = gpio_to_irq(...)';

> +			gpio_free(adc_bat->pdata->gpio_charge_finished);
> +		}
> +	}
> +
> +	for (chan_index = 0; channel_name[chan_index]; chan_index++)
> +		iio_channel_release(adc_bat->channel[chan_index]);
> +	cancel_delayed_work(&adc_bat->bat_work);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int generic_adc_bat_suspend(struct device *dev)
> +{
> +	struct generic_adc_bat *adc_bat = dev_get_drvdata(dev);
> +
> +	cancel_delayed_work_sync(&adc_bat->bat_work);
> +	adc_bat->status = POWER_SUPPLY_STATUS_UNKNOWN;
> +	return 0;
> +}
> +
> +static int generic_adc_bat_resume(struct device *dev)
> +{
> +	struct generic_adc_bat *adc_bat = dev_get_drvdata(dev);
> +
> +	/* Schedule timer to check current status */
> +	schedule_delayed_work(&adc_bat->bat_work,
> +			msecs_to_jiffies(adc_bat->pdata->jitter_delay));
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops generic_adc_battery_pm_ops = {
> +	.suspend        = generic_adc_bat_suspend,
> +	.resume         = generic_adc_bat_resume,
> +};
> +
> +#define GENERIC_ADC_BATTERY_PM_OPS       (&generic_adc_battery_pm_ops)
> +#else
> +#define GENERIC_ADC_BATTERY_PM_OPS       (NULL)
> +#endif
> +
> +static struct platform_driver generic_adc_bat_driver = {
> +	.driver		= {
> +		.name	= "generic-adc-battery",
> +		.owner	= THIS_MODULE,
> +		.pm	= GENERIC_ADC_BATTERY_PM_OPS
> +	},
> +	.probe		= generic_adc_bat_probe,
> +	.remove		= generic_adc_bat_remove,
> +};
> +

No need for this empty line.

> +module_platform_driver(generic_adc_bat_driver);
> +
> +MODULE_AUTHOR("anish kumar <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
> +};

You define chan_type twice. Keep only one declaration, this one,
in the header file.

> +
> +extern char channel_name[][BAT_MAX_NAME + 1];

Uhm. Making it extern means that you're no longer able to use
this driver as a module. And the module isn't a problem per se,
it just a sign of some bigger problem... Maybe you should rethink
how you pass channel name, dunno.

> +struct iio_battery_platform_data {

Inconsistent. Either use generic_adc_battery_ prefix, or
iio_battery_, or anything you like, but please be consistent across
all the driver and files.

> +	struct power_supply_info battery_info;
> +	char	**channel_name;
> +	char	*battery_name;
> +	int	(*cal_charge)(long);

Long what? :-) Please specify a variable name.

> +	int	gpio_charge_finished;
> +	int	gpio_inverted;
> +	int	bat_poll_interval;
> +	int	jitter_delay;
> +};
> +
> +#endif /* GENERIC_ADC_BATTERY_H */

Cheers,
Anton.

Re: [PATCH] power: battery: Generic battery driver using IIO

[anish singh]

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