> Date: Tue, 10 Jul 2012 18:20:13 +0200
> From: Lee Jones <lee.jo...@linaro.org>
> To: "Rajanikanth H.V" <rajanikanth...@stericsson.com>
> Cc: STEricsson_nomadik_linux <stericsson_nomadik_li...@list.st.com>,
> linaro-dev@lists.linaro.org, linux-ker...@vger.kernel.org,
> linux-arm-ker...@lists.infradead.org, patc...@linaro.org
> Subject: Re: [PATCH] mfd: Implement devicetree support for AB8500
> Btemp
> Message-ID: <4ffc563d.2080...@linaro.org>
> Content-Type: text/plain; charset=UTF-8; format=flowed
>
> 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_li...@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...@stericsson.com>
>>
>> This patch adds device tree support for
>> battery temperature monitor driver
>>
>> Signed-off-by: Rajanikanth H.V <rajanikanth...@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.
your previous comment was: "No, it's meant to work with _both_ platform
and Device Tree registation."
Why is this required when platform specific information/file is removed?
>
>> + 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.radhe...@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
>
>
>
>
>
> ------------------------------
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>
>
> End of linaro-dev Digest, Vol 26, Issue 38
> ******************************************
>
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
