[PATCH] mfd: Implement devicetree support for AB8500 Btemp

[Lee Jones]

Some of my comments are picky, but if it's going into Mainline, it 
should at least look professional.

You didn't CC the ST-Ericsson internal mailing list.

Address is STEricsson_nomadik_linux at list.st.com

I'll do it for now, but please do so on your next submission.

On 10/07/12 15:23, Rajanikanth H.V wrote:
> From: "Rajanikanth H.V" <rajanikanth.hv at stericsson.com>
>
>      This patch adds device tree support for
>      battery temperature monitor driver
>
> Signed-off-by: Rajanikanth H.V <rajanikanth.hv at stericsson.com>
> ---
>   .../bindings/power_supply/ab8500/btemp.txt         |   54 ++
>   arch/arm/boot/dts/db8500.dtsi                      |   17 +
>   arch/arm/mach-ux500/Makefile                       |    4 +-
>   arch/arm/mach-ux500/board-mop500-bm.c              |  532 ++++++++++++++++++++
>   arch/arm/mach-ux500/include/mach/board-mop500-bm.h |   24 +
>   drivers/mfd/ab8500-core.c                          |    1 +
>   drivers/power/Kconfig                              |    8 +-
>   drivers/power/ab8500_btemp.c                       |   79 ++-
>   include/linux/mfd/ab8500/pwmleds.h                 |   20 +
>   9 files changed, 722 insertions(+), 17 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/power_supply/ab8500/btemp.txt
>   create mode 100644 arch/arm/mach-ux500/board-mop500-bm.c
>   create mode 100644 arch/arm/mach-ux500/include/mach/board-mop500-bm.h
>   create mode 100644 include/linux/mfd/ab8500/pwmleds.h
>
> diff --git a/Documentation/devicetree/bindings/power_supply/ab8500/btemp.txt b/Documentation/devicetree/bindings/power_supply/ab8500/btemp.txt
> new file mode 100644
> index 0000000..8543ed1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power_supply/ab8500/btemp.txt
> @@ -0,0 +1,54 @@
> +AB8500 Battery Termperature Monitor Driver

Spelling.

> +
> +AB8500 is a mixed signal multimedia and power management
> +device comprising: power and energy management module,
> +WalliCharger and USB charger interface, audio, general
> +purpose ADC TVOut, clock management and SIM card Interface.

Mixture of capitalised and otherwise device names.

> +
> +Battery temperature monitoring support is part of 'energy
> +management module', the other components of this module
> +are: 'main and USB Combo charger' and fuel guage.

Spelling. Mixed use of ''.

> +The properties below describes the node for battery
> +temperature monitor driver.
> +
> +Required Properties:
> +- compatible = "stericsson,ab8500-btemp"
> +
> +interrupts:
> +	Four battery temperature ranges are be defined
> +	which results in interrupt events as:
> +	- Btemp
> +	- BtempLow
> +	- BtempMedium
> +	- BtempHigh
> +
> +
> +Supplied-to:
> +	This shall be power supply class dependency where in the runtime battery
> +	properties will be shared across fuel guage and charging algorithm driver.

Spelling.

> +
> +Thermister-interface:
> +	'btemp' and 'batctrl' are the pins interfaced for battery temperature
> +	measurement, btemp is used when NTC(Negative Termperature coefficient)

Spelling.

> +	resister is interfaced external to battery and batctrl is used when
> +	NTC resister is internal to battery.
> +
> +example:
> +ab8500-btemp {
> +	compatible = "stericsson,ab8500-btemp";
> +
> +	interrupt-names =
> +		"BAT_CTRL_INDB",	/* battery removal indicator */
> +		"BTEMP_LOW", 		/* Btemp < BtempLow, if battery temperature is lower than -10°C */
> +		"BTEMP_HIGH",		/* BtempLow < Btemp < BtempMedium,
> +								if battery temperature is between -10 and 0°C */
> +		"BTEMP_LOW_MEDIUM", /* BtempMedium < Btemp < BtempHigh,
> +								if battery temperature is between 0°C and “MaxTemp”.*/
> +		"BTEMP_MEDIUM_HIGH";/* Btemp > BtempHigh,
> +								if battery temperature is higher than “MaxTemp”. */
> +
> +	supplied-to = "ab8500_chargalg", "ab8500_fg";
> +
> +	thermister-internal-to-battery = <1>;
> +};
> diff --git a/arch/arm/boot/dts/db8500.dtsi b/arch/arm/boot/dts/db8500.dtsi
> index 7279165..527fdc3 100644
> --- a/arch/arm/boot/dts/db8500.dtsi
> +++ b/arch/arm/boot/dts/db8500.dtsi
> @@ -330,6 +330,23 @@
>   					vddadc-supply = <&ab8500_ldo_tvout_reg>;
>   				};
>
> +				ab8500-btemp {
> +					compatible = "stericsson,ab8500-btemp";
> +					interrupts = <20 0x04
> +								80 0x04
> +								81 0x04
> +								82 0x04
> +								83 0x04>;

Odd tabbing.

> +					interrupt-names = "BAT_CTRL_INDB",
> +									"BTEMP_LOW",
> +									"BTEMP_HIGH",
> +									"BTEMP_LOW_MEDIUM",
> +									"BTEMP_MEDIUM_HIGH";

As above.

> +					supplied_to = "ab8500_chargalg", "ab8500_fg";
> +					num_supplicants = <2>;
> +					battery_term_on_batctrl = <1>;
> +				};
> +
>   				ab8500-usb {
>   					compatible = "stericsson,ab8500-usb";
>   					interrupts = < 90 0x4
> diff --git a/arch/arm/mach-ux500/Makefile b/arch/arm/mach-ux500/Makefile
> index 026086f..b474917 100644
> --- a/arch/arm/mach-ux500/Makefile
> +++ b/arch/arm/mach-ux500/Makefile
> @@ -12,6 +12,8 @@ obj-$(CONFIG_MACH_MOP500)	+= board-mop500.o board-mop500-sdi.o \
>   				board-mop500-uib.o board-mop500-stuib.o \
>   				board-mop500-u8500uib.o \
>   				board-mop500-pins.o \
> -				board-mop500-msp.o
> +				board-mop500-msp.o \
> +				board-mop500-bm.o
> +

Unrelated line change.

>   obj-$(CONFIG_SMP)		+= platsmp.o headsmp.o
>   obj-$(CONFIG_HOTPLUG_CPU)	+= hotplug.o

> diff --git a/arch/arm/mach-ux500/board-mop500-bm.c b/arch/arm/mach-ux500/board-mop500-bm.c
> diff --git a/arch/arm/mach-ux500/include/mach/board-mop500-bm.h b/arch/arm/mach-ux500/include/mach/board-mop500-bm.h

It would be better if you can find a way to not upstream these.

I think this data would be better suited for an include header file.

> diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> index 738b9c5..402f630 100644
> --- a/drivers/mfd/ab8500-core.c
> +++ b/drivers/mfd/ab8500-core.c
> @@ -1046,6 +1046,7 @@ static struct mfd_cell __devinitdata ab8500_bm_devs[] = {
>   	},
>   	{
>   		.name = "ab8500-btemp",
> +		.of_compatible = "stericsson,ab8500-btemp",
>   		.num_resources = ARRAY_SIZE(ab8500_btemp_resources),
>   		.resources = ab8500_btemp_resources,
>   	},
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index e3a3b49..00dec0f 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -303,10 +303,10 @@ config AB8500_BM
>   	help
>   	  Say Y to include support for AB5500 battery management.
>
> -config AB8500_BATTERY_THERM_ON_BATCTRL
> -	bool "Thermistor connected on BATCTRL ADC"
> +config AB8500_9100_LI_ION_BATTERY
> +	bool "Enable support of the 9100 Li-ion battery charging"
>   	depends on AB8500_BM
>   	help
> -	  Say Y to enable battery temperature measurements using
> -	  thermistor connected on BATCTRL ADC.
> +		Say Y to enable support of the 9100 Li-ion battery charging.
> +

Random formatting.

>   endif # POWER_SUPPLY
> diff --git a/drivers/power/ab8500_btemp.c b/drivers/power/ab8500_btemp.c
> index bba3cca..1272bba 100644
> --- a/drivers/power/ab8500_btemp.c
> +++ b/drivers/power/ab8500_btemp.c
> @@ -16,6 +16,7 @@
>   #include <linux/interrupt.h>
>   #include <linux/delay.h>
>   #include <linux/slab.h>
> +#include <linux/of.h>
>   #include <linux/platform_device.h>
>   #include <linux/power_supply.h>
>   #include <linux/completion.h>
> @@ -25,6 +26,7 @@
>   #include <linux/mfd/abx500/ab8500-bm.h>
>   #include <linux/mfd/abx500/ab8500-gpadc.h>
>   #include <linux/jiffies.h>
> +#include <mach/board-mop500-bm.h>
>
>   #define VTVOUT_V			1800
>
> @@ -964,11 +966,13 @@ static int __devinit ab8500_btemp_probe(struct platform_device *pdev)
>   {
>   	int irq, i, ret = 0;
>   	u8 val;
> -	struct abx500_bm_plat_data *plat_data = pdev->dev.platform_data;

I already told you about this.

> +	struct device_node *np = pdev->dev.of_node;
>   	struct ab8500_btemp *di;
> +	u32 pval;
> +	const char *sup_val;
>
> -	if (!plat_data) {
> -		dev_err(&pdev->dev, "No platform data\n");

And this. Check the last review I provided.

> +	if (!np) {
> +		dev_err(&pdev->dev, "No DT node for btemp found\n");
>   		return -EINVAL;
>   	}
>
> @@ -981,21 +985,64 @@ static int __devinit ab8500_btemp_probe(struct platform_device *pdev)
>   	di->parent = dev_get_drvdata(pdev->dev.parent);
>   	di->gpadc = ab8500_gpadc_get("ab8500-gpadc.0");
>
> -	/* get btemp specific platform data */
> -	di->pdata = plat_data->btemp;
> -	if (!di->pdata) {
> -		dev_err(di->dev, "no btemp platform data supplied\n");

We still need to support registering from platform code.

> +	di->pdata =
> +		kzalloc(sizeof(struct abx500_btemp_platform_data), GFP_KERNEL);

Use devm_kzalloc instead.

> +	if (di->pdata == NULL) {
> +		ret = -ENOMEM;
> +		goto free_device_info;
> +	}
> +	/* get battery specific platform data */
> +	ret = of_property_read_u32(np, "num_supplicants", &pval);
> +	if (ret) {
> +		dev_err(di->dev, "missing property num_supplicants\n");
> +		kfree(di->pdata);

Then remove this line.

> +		ret = -EINVAL;
> +		goto free_device_info;
> +	}
> +	di->pdata->num_supplicants = pval;
> +	di->pdata->supplied_to =
> +		kzalloc(di->pdata->num_supplicants *
> +			sizeof(const char *), GFP_KERNEL);
> +	if (di->pdata->supplied_to == NULL) {
> +		kfree(di->pdata);
>   		ret = -EINVAL;
>   		goto free_device_info;
>   	}
>
> -	/* get battery specific platform data */
> -	di->bat = plat_data->battery;

Don't remove this, check for it.

> +	for (val = 0; val < di->pdata->num_supplicants; ++val)
> +		if (of_property_read_string_index
> +			(np, "supplied_to", val, &sup_val) == 0)
> +			*(di->pdata->supplied_to + val) = (char *)sup_val;
> +		else {
> +			dev_err(di->dev, "insufficient number of supplied_to data found\n");
> +			kfree(di->pdata);
> +			kfree(di->pdata->supplied_to);
> +			ret = -EINVAL;
> +			goto free_device_info;
> +		}
> +
> +	di->bat = kzalloc(sizeof(struct abx500_bm_data), GFP_KERNEL);
>   	if (!di->bat) {
> -		dev_err(di->dev, "no battery platform data supplied\n");
> -		ret = -EINVAL;
> +		kfree(di->pdata->supplied_to);
> +		kfree(di->pdata);
> +		ret = -ENOMEM;
>   		goto free_device_info;
>   	}
> +	dev_dbg(di->dev, "getting battery information\n");

Is this really necessary?

> +	di->bat = &ab8500_bm_data;
> +	ret = of_property_read_u32(np, "battery_term_on_batctrl", &pval);
> +	if (ret) {
> +		dev_err(di->dev, "missing property battery_term_on_batctrl\n");
> +		kfree(di->pdata->supplied_to);
> +		kfree(di->pdata);
> +		kfree(di->bat);
> +		ret = -ENOMEM;
> +		goto free_device_info;
> +	}
> +	if (pval == 0) {
> +		di->bat->bat_type = bat_type_ext_thermister;
> +		di->bat->adc_therm = ABx500_ADC_THERM_BATTEMP;
> +	}
>
>   	/* BTEMP supply */
>   	di->btemp_psy.name = "ab8500_btemp";
> @@ -1008,7 +1055,6 @@ static int __devinit ab8500_btemp_probe(struct platform_device *pdev)
>   	di->btemp_psy.external_power_changed =
>   		ab8500_btemp_external_power_changed;
>
> -

Unrelated change.

>   	/* Create a work queue for the btemp */
>   	di->btemp_wq =
>   		create_singlethread_workqueue("ab8500_btemp_wq");
> @@ -1090,14 +1136,22 @@ free_irq:
>   		irq = platform_get_irq_byname(pdev, ab8500_btemp_irq[i].name);
>   		free_irq(irq, di);
>   	}
> +

As above.

>   free_btemp_wq:
>   	destroy_workqueue(di->btemp_wq);
> +

As above.

>   free_device_info:
>   	kfree(di);
>
>   	return ret;
>   }
>
> +static const struct of_device_id ab8500_btemp_match[] = {
> +	{.compatible = "stericsson,ab8500-btemp",},

Spacing is not consistent with previous submissions.

> +	{},
> +};
> +
> +
>   static struct platform_driver ab8500_btemp_driver = {
>   	.probe = ab8500_btemp_probe,
>   	.remove = __devexit_p(ab8500_btemp_remove),
> @@ -1106,6 +1160,7 @@ static struct platform_driver ab8500_btemp_driver = {
>   	.driver = {
>   		.name = "ab8500-btemp",
>   		.owner = THIS_MODULE,
> +		.of_match_table = ab8500_btemp_match,
>   	},
>   };
>
> diff --git a/include/linux/mfd/ab8500/pwmleds.h b/include/linux/mfd/ab8500/pwmleds.h
> new file mode 100644
> index 0000000..e316582
> --- /dev/null
> +++ b/include/linux/mfd/ab8500/pwmleds.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright ST-Ericsson 2012.
> + *
> + * Author: Naga Radhesh <naga.radheshy at stericsson.com>
> + * Licensed under GPLv2.
> + */
> +#ifndef _AB8500_PWMLED_H
> +#define _AB8500_PWMLED_H
> +
> +struct ab8500_led_pwm {
> +	int	pwm_id;
> +	int	blink_en;
> +};
> +
> +struct ab8500_pwmled_platform_data {
> +	int	num_pwm;
> +	struct	ab8500_led_pwm *leds;
> +};
> +
> +#endif
>


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog