Subject: [PATCH] mfd: Implement devicetree support for AB8500 Btemp

From: [email-masked] (Rajanikanth HV)
Date: Fri, 13 Jul 2012 16:57:49 +0530

> Date: Tue, 10 Jul 2012 18:20:13 +0200
> From: Lee Jones <[email-masked]>
> To: "Rajanikanth H.V" <[email-masked]>
> Cc: STEricsson_nomadik_linux <[email-masked]>,
>         linaro-dev at lists.linaro.org, linux-kernel at vger.kernel.org,
>         linux-arm-kernel at lists.infradead.org, patches at linaro.org
> Subject: Re: [PATCH] mfd: Implement devicetree support for AB8500
>         Btemp
> Message-ID: <[email-masked]>
> 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_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" <[email-masked]>
>>
>>      This patch adds device tree support for
>>      battery temperature monitor driver
>>
>> Signed-off-by: Rajanikanth H.V <[email-masked]>
>> ---
>>   .../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 <[email-masked]>
>> + * 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 at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
> 
> 
> End of linaro-dev Digest, Vol 26, Issue 38
> ******************************************
>