Re: [PATCH] mfd: Implement devicetree support for AB8500 Btemp
From: Lee Jones
Date: Tue Jul 10 2012 - 12:20:17 EST
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@xxxxxxxxxxx
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@xxxxxxxxxxxxxx>
This patch adds device tree support for
battery temperature monitor driver
Signed-off-by: Rajanikanth H.V <rajanikanth.hv@xxxxxxxxxxxxxx>
---
.../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@xxxxxxxxxxxxxx>
+ * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/