From: lee.jones@linaro.org (Lee Jones)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] mfd: Implement devicetree support for AB8500 Btemp
Date: Tue, 10 Jul 2012 18:20:13 +0200 [thread overview]
Message-ID: <4FFC563D.2080807@linaro.org> (raw)
In-Reply-To: <1341926639-9956-1-git-send-email-rajanikanth.hv@stericsson.com>
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@stericsson.com>
>
> This patch adds device tree support for
> battery temperature monitor driver
>
> Signed-off-by: Rajanikanth H.V <rajanikanth.hv@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@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
WARNING: multiple messages have this Message-ID (diff)
From: Lee Jones <lee.jones@linaro.org>
To: "Rajanikanth H.V" <rajanikanth.hv@stericsson.com>
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linaro-dev@lists.linaro.org,
patches@linaro.org,
STEricsson_nomadik_linux <STEricsson_nomadik_linux@list.st.com>
Subject: Re: [PATCH] mfd: Implement devicetree support for AB8500 Btemp
Date: Tue, 10 Jul 2012 18:20:13 +0200 [thread overview]
Message-ID: <4FFC563D.2080807@linaro.org> (raw)
In-Reply-To: <1341926639-9956-1-git-send-email-rajanikanth.hv@stericsson.com>
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@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@stericsson.com>
>
> This patch adds device tree support for
> battery temperature monitor driver
>
> Signed-off-by: Rajanikanth H.V <rajanikanth.hv@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@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
next prev parent reply other threads:[~2012-07-10 16:20 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-07-10 13:23 [PATCH] mfd: Implement devicetree support for AB8500 Btemp Rajanikanth H.V
2012-07-10 14:12 ` Arnd Bergmann
2012-07-10 14:12 ` Arnd Bergmann
2012-07-10 16:20 ` Lee Jones [this message]
2012-07-10 16:20 ` Lee Jones
[not found] <mailman.377.1341937217.1590.linaro-dev@lists.linaro.org>
2012-07-13 11:27 ` Rajanikanth HV
2012-07-13 11:27 ` Rajanikanth HV
2012-07-13 11:56 ` Lee Jones
2012-07-13 11:56 ` Lee Jones
-- strict thread matches above, loose matches on Subject: below --
2012-07-13 11:35 Rajanikanth HV
2012-07-13 11:35 ` Rajanikanth HV
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4FFC563D.2080807@linaro.org \
--to=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.