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 | 52 ++++++ arch/arm/boot/dts/dbx5x0.dtsi | 8 + drivers/mfd/ab8500-core.c | 1 + drivers/power/Kconfig | 6 - drivers/power/ab8500_bmdata.h | 4 +- drivers/power/ab8500_btemp.c | 181 ++++++++++++++++---- 6 files changed, 213 insertions(+), 39 deletions(-) create mode 100644 Documentation/devicetree/bindings/power_supply/ab8500/btemp.txt
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..b214efc --- /dev/null +++ b/Documentation/devicetree/bindings/power_supply/ab8500/btemp.txt @@ -0,0 +1,52 @@ +=== AB8500 Battery Temperature Monitor Driver === + +The properties below describes the node for battery +temperature monitor driver. + +Required Properties: +- compatible = "stericsson,ab8500-btemp" + +supplied-to: + This is a logical binding w.r.t power supply event change + across energy-management-module drivers where in the + runtime battery properties are shared along with uevent + notification. + ref: di->btemp_psy.external_power_changed = + ab8500_btemp_external_power_changed; + ab8500_btemp.c + + Need for this property: + btemp, fg and charger updates power-supply properties + based on the events listed above. + Event handler invokes power supply change notifier + which in-turn invokes registered power supply class call-back + based on the 'supplied_to' string. + ref: + power_supply_changed_work(..) ./drivers/power/power_supply_core.c + + example: + ab8500-btemp { + /* Other enery management module */ + supplied-to = "ab8500_chargalg", "ab8500_fg"; + num_supplicants = <2>; + }; + +thermister-interface: + 'btemp' and 'batctrl' are the pins interfaced for battery temperature + measurement, btemp is used when NTC(negative temperature coefficient) + resister is interfaced external to battery and batctrl is used when + NTC resister is internal to battery. + + +li-ion-9100-battery: + use this to add support for the 9100 Li-ION battery, + this adjust the bkup battery charger parameters + Note: this property is used for tablet version of snowball board. + + example: + ab8500-btemp { + thermister-internal-to-battery = <1>; + li_ion_9100_battery = <0>; + }; +Note: +interrupts are defined and registered in the driver diff --git a/arch/arm/boot/dts/dbx5x0.dtsi b/arch/arm/boot/dts/dbx5x0.dtsi index d69c087..643e7fd 100644 --- a/arch/arm/boot/dts/dbx5x0.dtsi +++ b/arch/arm/boot/dts/dbx5x0.dtsi @@ -360,6 +360,14 @@ li_ion_9100 = <0>; };
+ ab8500-btemp { + compatible = "stericsson,ab8500-btemp"; + supplied_to = "ab8500_chargalg", "ab8500_fg"; + num_supplicants = <2>; + thermister_on_batctrl = <1>; + li_ion_9100 = <0>; + }; + ab8500-usb { compatible = "stericsson,ab8500-usb"; interrupts = < 90 0x4 diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c index c413cfa..7ffba9b 100644 --- a/drivers/mfd/ab8500-core.c +++ b/drivers/mfd/ab8500-core.c @@ -1047,6 +1047,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 c1892f3..1434871 100644 --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -304,12 +304,6 @@ config AB8500_BM help Say Y to include support for AB8500 battery management.
-config AB8500_BATTERY_THERM_ON_BATCTRL - bool "Thermistor connected on BATCTRL ADC" - depends on AB8500_BM - help - Say Y to enable battery temperature measurements using - thermistor connected on BATCTRL ADC. endif # POWER_SUPPLY
source "drivers/power/avs/Kconfig" diff --git a/drivers/power/ab8500_bmdata.h b/drivers/power/ab8500_bmdata.h index 748334a..0bd0a45 100644 --- a/drivers/power/ab8500_bmdata.h +++ b/drivers/power/ab8500_bmdata.h @@ -22,7 +22,7 @@ static struct abx500_res_to_temp temp_tbl_A_thermister[] = { {65, 12500}, }; static struct abx500_res_to_temp temp_tbl_B_thermister[] = { - {-5, 165418}, + {-5, 200000}, { 0, 159024}, { 5, 151921}, {10, 144300}, @@ -229,7 +229,7 @@ static struct abx500_battery_type bat_type_thermister[] = { }, { .name = POWER_SUPPLY_TECHNOLOGY_LIPO, - .resis_high = 165418, + .resis_high = 200000, .resis_low = 82869, .battery_resistance = 300, .charge_full_design = 900, diff --git a/drivers/power/ab8500_btemp.c b/drivers/power/ab8500_btemp.c index bba3cca..ac08f1a 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 "ab8500_bmdata.h"
#define VTVOUT_V 1800
@@ -960,42 +962,153 @@ static int __devexit ab8500_btemp_remove(struct platform_device *pdev) return 0; }
-static int __devinit ab8500_btemp_probe(struct platform_device *pdev) +static int __devinit +btemp_of_probe(struct device *dev, + struct device_node *np, + struct abx500_bm_plat_data *bm_pdata) { - int irq, i, ret = 0; - u8 val; - struct abx500_bm_plat_data *plat_data = pdev->dev.platform_data; - struct ab8500_btemp *di; - - if (!plat_data) { - dev_err(&pdev->dev, "No platform data\n"); - return -EINVAL; + u8 val; + u32 pval; + int i; + int ext_thermister, lion_battery, ret = 0; + const char *bm_dev_name; + struct abx500_btemp_platform_data *btemp = bm_pdata->btemp; + struct abx500_bm_data *bat; + struct abx500_battery_type *btype; + + ret = of_property_read_u32(np, "num_supplicants", &pval); + if (ret) { + dev_err(dev, "missing property num_supplicants\n"); + ret = -EINVAL; + goto inval_pval; } - - di = kzalloc(sizeof(*di), GFP_KERNEL); - if (!di) - return -ENOMEM; - - /* get parent data */ - di->dev = &pdev->dev; - 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"); + btemp->num_supplicants = pval; + btemp->supplied_to = + devm_kzalloc(dev, btemp->num_supplicants * + sizeof(const char *), GFP_KERNEL); + if (btemp->supplied_to == NULL) { + dev_err(dev, "%s no mem for supplied_to\n", __func__); + ret = -ENOMEM; + goto inval_pval; + } + for (val = 0; val < btemp->num_supplicants; ++val) + if (of_property_read_string_index + (np, "supplied_to", val, &bm_dev_name) == 0) + *(btemp->supplied_to + val) = (char *)bm_dev_name; + else { + dev_err(dev, "insufficient number of supplied_to data found\n"); + ret = -EINVAL; + goto free_dev_mem; + } + ret = of_property_read_u32(np, "thermister_on_batctrl", &pval); + if (ret) { + dev_err(dev, "missing property thermister_on_batctrl\n"); ret = -EINVAL; - goto free_device_info; + goto free_dev_mem; + } + bm_pdata->battery = &ab8500_bm_data; + bat = bm_pdata->battery; + ext_thermister = 0; + if (pval == 0) { + bat->n_btypes = 4; + bat->bat_type = bat_type_ext_thermister; + bat->adc_therm = ABx500_ADC_THERM_BATTEMP; + ext_thermister = 1; + } + ret = of_property_read_u32(np, "li_ion_9100", &pval); + if (ret) { + dev_err(dev, "missing property li_ion_9100\n"); + ret = -EINVAL; + goto free_dev_mem; + } + lion_battery = 0; + if (pval == 1) { + bat->no_maintenance = true; + bat->chg_unknown_bat = true; + bat->bat_type[BATTERY_UNKNOWN].charge_full_design = 2600; + bat->bat_type[BATTERY_UNKNOWN].termination_vol = 4150; + bat->bat_type[BATTERY_UNKNOWN].recharge_vol = 4130; + bat->bat_type[BATTERY_UNKNOWN].normal_cur_lvl = 520; + bat->bat_type[BATTERY_UNKNOWN].normal_vol_lvl = 4200; + lion_battery = 1; + } + /* select the battery resolution table */ + for (i = 0; i < bat->n_btypes; ++i) { + btype = (bat->bat_type + i); + if (ext_thermister) { + btype->batres_tbl = + temp_to_batres_tbl_ext_thermister; + } else if (lion_battery) { + btype->batres_tbl = + temp_to_batres_tbl_9100; + } else { + btype->batres_tbl = + temp_to_batres_tbl_thermister; + } + } + return ret; +free_dev_mem: + devm_kfree(dev, btemp->supplied_to); +inval_pval: + return ret; +} + +static int __devinit ab8500_btemp_probe(struct platform_device *pdev) +{ + u8 val; + int i; + int irq, ret = 0; + struct abx500_bm_plat_data *pdata = pdev->dev.platform_data; + struct device_node *np = pdev->dev.of_node; + struct ab8500_btemp *di; + + di = devm_kzalloc(&pdev->dev, sizeof(*di), GFP_KERNEL); + if (!di) { + dev_err(&pdev->dev, "%s no mem for ab8500_btemp\n", __func__); + ret = -ENOMEM; }
- /* get battery specific platform data */ - di->bat = plat_data->battery; - if (!di->bat) { - dev_err(di->dev, "no battery platform data supplied\n"); + if (np) { + if (!pdata) { + pdata = + devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) { + dev_err(&pdev->dev, + "%s no mem for pdata\n", __func__); + ret = -ENOMEM; + goto err_no_mem; + } + pdata->btemp = devm_kzalloc(&pdev->dev, + sizeof(*pdata->btemp), GFP_KERNEL); + if (!pdata->btemp) { + devm_kfree(&pdev->dev, pdata); + dev_err(&pdev->dev, + "%s no mem for pdata->btemp\n", + __func__); + ret = -ENOMEM; + goto free_device_info; + } + } + /* get battery specific platform data */ + ret = btemp_of_probe(&pdev->dev, np, pdata); + if (ret) { + devm_kfree(&pdev->dev, pdata->btemp); + devm_kfree(&pdev->dev, pdata); + goto free_device_info; + } + } + if (!pdata) { + dev_err(&pdev->dev, + "%s no btemp platform data found\n", __func__); ret = -EINVAL; goto free_device_info; } + di->pdata = pdata->btemp; + di->bat = pdata->battery; + /* get parent data */ + di->dev = &pdev->dev; + di->parent = dev_get_drvdata(pdev->dev.parent); + di->gpadc = ab8500_gpadc_get("ab8500-gpadc.0");
/* BTEMP supply */ di->btemp_psy.name = "ab8500_btemp"; @@ -1008,7 +1121,6 @@ static int __devinit ab8500_btemp_probe(struct platform_device *pdev) di->btemp_psy.external_power_changed = ab8500_btemp_external_power_changed;
- /* Create a work queue for the btemp */ di->btemp_wq = create_singlethread_workqueue("ab8500_btemp_wq"); @@ -1065,7 +1177,7 @@ static int __devinit ab8500_btemp_probe(struct platform_device *pdev) IRQF_SHARED | IRQF_NO_SUSPEND, ab8500_btemp_irq[i].name, di);
- if (ret) { + if (ret < 0) { dev_err(di->dev, "failed to request %s IRQ %d: %d\n" , ab8500_btemp_irq[i].name, irq, ret); goto free_irq; @@ -1093,11 +1205,17 @@ free_irq: free_btemp_wq: destroy_workqueue(di->btemp_wq); free_device_info: - kfree(di); + devm_kfree(&pdev->dev, di); +err_no_mem:
return ret; }
+static const struct of_device_id ab8500_btemp_match[] = { + {.compatible = "stericsson,ab8500-btemp",}, + {}, +}; + static struct platform_driver ab8500_btemp_driver = { .probe = ab8500_btemp_probe, .remove = __devexit_p(ab8500_btemp_remove), @@ -1106,6 +1224,7 @@ static struct platform_driver ab8500_btemp_driver = { .driver = { .name = "ab8500-btemp", .owner = THIS_MODULE, + .of_match_table = ab8500_btemp_match, }, };
On Monday 10 September 2012, Rajanikanth HV wrote:
+supplied-to:
This is a logical binding w.r.t power supply event changeacross energy-management-module drivers where in theruntime battery properties are shared along with ueventnotification.ref: di->btemp_psy.external_power_changed =ab8500_btemp_external_power_changed;ab8500_btemp.cNeed for this property:btemp, fg and charger updates power-supply propertiesbased on the events listed above.Event handler invokes power supply change notifierwhich in-turn invokes registered power supply class call-backbased on the 'supplied_to' string.ref:power_supply_changed_work(..) ./drivers/power/power_supply_core.cexample:ab8500-btemp {/* Other enery management module */supplied-to = "ab8500_chargalg", "ab8500_fg";num_supplicants = <2>;};
This looks like you're doing things the opposite way from everyone else. Normally, each device uses phandles to refer to other objects it depends on (gpio lines, regulators, clocks, interrupts, ...), rather than listing things that depend on it.
Can you turn this around to be more like the others?
Note also that device tree identifiers should use '-' as a word separator, not '_', and that a binding document should specify the exact set of possible values. If the properties contain strings, please list every valid string.
thermister-internal-to-battery = <1>;li_ion_9100_battery = <0>;
Boolean properties should be empty when enabled and not present when disabled. In this example, one would just write
thermister-internal-to-battery;
Arnd
On Mon, Sep 10, 2012 at 7:01 AM, Arnd Bergmann arnd@arndb.de wrote:
On Monday 10 September 2012, Rajanikanth HV wrote:
thermister-internal-to-battery = <1>;li_ion_9100_battery = <0>;Boolean properties should be empty when enabled and not present when disabled. In this example, one would just write
thermister-internal-to-battery;
I also think the proper word is "thermistor": http://en.wikipedia.org/wiki/Thermistor
Yours, Linus Walleij
On Monday 10 September 2012 07:31 PM, Arnd Bergmann wrote:
On Monday 10 September 2012, Rajanikanth HV wrote:
+supplied-to:
This is a logical binding w.r.t power supply event changeacross energy-management-module drivers where in theruntime battery properties are shared along with ueventnotification.ref: di->btemp_psy.external_power_changed =ab8500_btemp_external_power_changed;ab8500_btemp.cNeed for this property:btemp, fg and charger updates power-supply propertiesbased on the events listed above.Event handler invokes power supply change notifierwhich in-turn invokes registered power supply class call-backbased on the 'supplied_to' string.ref:power_supply_changed_work(..) ./drivers/power/power_supply_core.cexample:ab8500-btemp {/* Other enery management module */supplied-to = "ab8500_chargalg", "ab8500_fg";num_supplicants = <2>;};This looks like you're doing things the opposite way from everyone else. Normally, each device uses phandles to refer to other objects it depends on (gpio lines, regulators, clocks, interrupts, ...), rather than listing things that depend on it.
Can you turn this around to be more like the others?
We discussed about this on : "13 July 2012 17:05", pasting from that mail thread. ============================
+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.
I probably don't understand enough of this, but shouldn't the other
devices
that are supplied by this have a reference to this node rather than doing it this way around? Why use strings here instead of phandles?
This is a logical binding w.r.t power supply event change across energy-management-module drivers where in runtime battery properties are shared along with uevent notification. ref: di->btemp_psy.external_power_ changed = ab8500_btemp_external_power_changed; ref: ab8500_btemp.c
Need for this property: btemp, fg and charger updates power-supply properties based on the events listed above. Event handler invokes power supply change notifier which in-turn invokes registered power supply class call-back based on the 'supplied_to' string. ref: power_supply_changed_work(..) ./drivers/power/power_supply_core.c
In this case how to approach through phandle? ============================
Note also that device tree identifiers should use '-' as a word separator, not '_', and that a binding document should specify the exact set of possible values. If the properties contain strings, please list every valid string.
thermister-internal-to-battery = <1>;li_ion_9100_battery = <0>;Boolean properties should be empty when enabled and not present when disabled. In this example, one would just write
thermister-internal-to-battery;Arnd
On Tuesday 11 September 2012, Rajanikanth HV wrote:
+Supplied-to:
This shall be power supply class dependency where in theruntime battery
properties will be shared across fuel guage and chargingalgorithm driver.
I probably don't understand enough of this, but shouldn't the other
devices
that are supplied by this have a reference to this node rather than doing it this way around? Why use strings here instead of phandles?
This is a logical binding w.r.t power supply event change across energy-management-module drivers where in runtime battery properties are shared along with uevent notification. ref: di->btemp_psy.external_power_ changed = ab8500_btemp_external_power_changed; ref: ab8500_btemp.c
Need for this property: btemp, fg and charger updates power-supply properties based on the events listed above. Event handler invokes power supply change notifier which in-turn invokes registered power supply class call-back based on the 'supplied_to' string. ref: power_supply_changed_work(..) ./drivers/power/power_supply_core.c
In this case how to approach through phandle?
Sorry, I really tried, but I cannot make sense of what you wrote there. Can you try again and describe in full English sentences how the hardware blocks are connected and what their purpose is?
Arnd
On Tuesday 11 September 2012 04:52 PM, Arnd Bergmann wrote:
On Tuesday 11 September 2012, Rajanikanth HV wrote:
+Supplied-to:
This shall be power supply class dependency where in theruntime battery
properties will be shared across fuel guage and chargingalgorithm driver.
I probably don't understand enough of this, but shouldn't the other
devices
that are supplied by this have a reference to this node rather than doing it this way around? Why use strings here instead of phandles?
This is a logical binding w.r.t power supply event change across energy-management-module drivers where in runtime battery properties are shared along with uevent notification. ref: di->btemp_psy.external_power_ changed = ab8500_btemp_external_power_changed; ref: ab8500_btemp.c
Need for this property: btemp, fg and charger updates power-supply properties based on the events listed above. Event handler invokes power supply change notifier which in-turn invokes registered power supply class call-back based on the 'supplied_to' string. ref: power_supply_changed_work(..) ./drivers/power/power_supply_core.c
In this case how to approach through phandle?
Sorry, I really tried, but I cannot make sense of what you wrote there. Can you try again and describe in full English sentences how the hardware blocks are connected
Consider: USB charging: ______________________ | | --(Vbus)-->| USB Charger with | | Charger FSM (h/w) | |______________________| | | | |(Vbat and other signals) | __|_____ to | | |(GaugeSense Charger FSM| | LION | Signal) _____________ | |Battery |----------->|FuelGauge blk| | |________| |{Coulomb Ctr}| | | ------------- | <Thermistor> | | | | (BatCtrl Signal) |_______|---------->[Btemp blk] | | [ADC] |__Btemp_Low |__Btemp_Med |__Btemp_High
Note: Charging algorithm is a logical entity.
and what their purpose is? a) Coulomb counter comprises '12bit adc' and an 'N sample average/accumulation' logic helps to measure battery capacity Note: The charge and the discharge current of the battery is converted to voltage by an external resistor connected between GaugeSenseP and GaugeSenseN pins.
b) Battery temperature monitoring comprises a comparator which is enabled only by HW (charger state machine) helps to measure the thermal threshold Note: The accuracy of the battery temperature measurement depends of the accuracy of the thermistor used.
c) Charger provides 'Constant Current Constant Voltage' USB and Main(Wall) Charging support, it embeds: voltage detection, thermal protection, Constant voltage charging with programmable level, clock dithering and battery voltage monitoring
e.g. Correlation between charger and Btemp - if the battery temperature is higher than “MaxTemp °C, the charger does not start, but is enabled
- if the battery temperature is between 0°C and “MaxTemp” °C charging is done in AB8500 Hardware control mode
- charging is done in DB8500 Software control mode, if the battery: has a voltage higher than the “BattOK Threshold
- If the battery temperature is between -10°C and 0°C: charging is done in AB8500 Hardware control mode
- If the battery temperature is below -10°C, charging is done in AB8500 Hardware control mode
Arnd
On Wednesday 12 September 2012, Rajanikanth HV wrote:
On Tuesday 11 September 2012 04:52 PM, Arnd Bergmann wrote:
On Tuesday 11 September 2012, Rajanikanth HV wrote:
Consider: USB charging: ______________________ | | --(Vbus)-->| USB Charger with | | Charger FSM (h/w) | |______________________| | | | |(Vbat and other signals) | __|_____ to | | |(GaugeSense Charger FSM| | LION | Signal) _____________ | |Battery |----------->|FuelGauge blk| | |________| |{Coulomb Ctr}| | | ------------- | <Thermistor> | | | | (BatCtrl Signal) |_______|---------->[Btemp blk] | | [ADC] |__Btemp_Low |__Btemp_Med |__Btemp_High
Note: Charging algorithm is a logical entity.
and what their purpose is? a) Coulomb counter comprises '12bit adc' and an 'N sample average/accumulation' logic helps to measure battery capacity Note: The charge and the discharge current of the battery is converted to voltage by an external resistor connected between GaugeSenseP and GaugeSenseN pins.
b) Battery temperature monitoring comprises a comparator which is enabled only by HW (charger state machine) helps to measure the thermal threshold Note: The accuracy of the battery temperature measurement depends of the accuracy of the thermistor used.
c) Charger provides 'Constant Current Constant Voltage' USB and Main(Wall) Charging support, it embeds: voltage detection, thermal protection, Constant voltage charging with programmable level, clock dithering and battery voltage monitoring
e.g. Correlation between charger and Btemp
if the battery temperature is higher than “MaxTemp °C, the charger does not start, but is enabled
if the battery temperature is between 0°C and “MaxTemp” °C charging is done in AB8500 Hardware control mode
charging is done in DB8500 Software control mode, if the battery: has a voltage higher than the “BattOK Threshold
If the battery temperature is between -10°C and 0°C: charging is done in AB8500 Hardware control mode
If the battery temperature is below -10°C, charging is done in AB8500 Hardware control mode
Ok, thanks for the explanation, this is starting to make a lot more sense. So the three blocks (fb, btemp, charger) are all separate mfd cells, each with their own register set in ab8500 and a separate driver in drivers/power, right?
Then there is the ab8500-charalg driver which I guess is implementing the Charger FSM you mention above, but it doesn't have any registers in the ab8500 but rather ties the other drivers together.
If this is true, I don't understand what makes the 'supplied-to' properties you list in the device tree binding board specific. Are they not always done the same way? If so, you could just leave them out.
What does indeed seem to be needed is a place to identify the battery type, but it's not clear if the btemp device is the best place for that (maybe it is). For this, I would suggest you give a list of possible batteries and require a property such as
st-ericsson,battery-type: A string identifier for the type of battery, which impacts how an operating system interpret the sensor readings. Possible values include: * "none" -- no battery connected * "li-ion-9100" -- Type 9100 Li-ION battery * <add any others that apply here>
Arnd
On Wednesday 12 September 2012 09:06 PM, Arnd Bergmann wrote:
On Wednesday 12 September 2012, Rajanikanth HV wrote:
On Tuesday 11 September 2012 04:52 PM, Arnd Bergmann wrote:
On Tuesday 11 September 2012, Rajanikanth HV wrote:
Consider: USB charging: ______________________ | | --(Vbus)-->| USB Charger with | | Charger FSM (h/w) | |______________________| | | | |(Vbat and other signals) | __|_____ to | | |(GaugeSense Charger FSM| | LION | Signal) _____________ | |Battery |----------->|FuelGauge blk| | |________| |{Coulomb Ctr}| | | ------------- | <Thermistor> | | | | (BatCtrl Signal) |_______|---------->[Btemp blk] | | [ADC] |__Btemp_Low |__Btemp_Med |__Btemp_High
Note: Charging algorithm is a logical entity.
Ok, thanks for the explanation, this is starting to make a lot more sense. So the three blocks (fb, btemp, charger) are all separate mfd cells, each with their own register set in ab8500 and a separate driver in drivers/power, right?
Correct, You can have a look at ab8500 spec: www.stericsson.com/developers/CD00291561_UM1031_AB8500_user_manual-rev5_CTDS_public.pdf
Then there is the ab8500-charalg driver which I guess is implementing the Charger FSM you mention above,
ab8500-charalg does not implements charger FSM, Charger FSM is a hardware block for which any functional info is not available in the spec. However, ab8500-charalg implements state m/c depicted in the figure (9) of spec, implementation can be found in the code: abx500_chargalg.c: centered around abx500_chargalg_algorithm(..)
Let me brief out what 'charger(Wall/USB)' and 'chargalg' driver does:
(a) Charger driver implements: - events specific to Wall(a/c) and USB Charger - power supply attributes handling and notification to power_supply core. Ref: enum power_supply_property linux/power_supply.h Note: subset of power_supply_property are handled - turn on/off charging led, AC and USB charging - Regulating Voltage and Current values for charging - voltage threshold check - Charging regulation based on btemp all this functionality has register dependency
(b) Charging algorithm: - Starts by collecting power_supply properties across power class devices which are 'bm' devices - Maintains the different charging states across ac and usb charging process pertaining to 'Vbus, main or Vbat', thermal, btemp etc., - Implements subset of power_suppply_class properties Note: Do not have direct register interface
but it doesn't have any registers yes, but make use of power supply properties updated by other bm devs while managing charging states.
in the ab8500 but rather ties the other drivers together.
If this is true, I don't understand what makes the 'supplied-to' properties you list in the device tree binding board specific. Are they not always done the same way? If so, you could just leave them out.
Precisely 'supplied-to' is not board specific, it was maintained as platform_data which i migrated to dt-node. It is meant to establish dependency across bm drivers based on power_supply property and runtime battery attributes. Basically, 'supplied-to' provides a way of exporting change in power_supply_property and runtime batter characteristics so that other bm devs shall make use or refer the updated values. Ref: external_power_changed(...) call back api. Note: all the bm drivers handles subset of power_supply property and battery attributes, ref: include/linux/power_supply.h and get_property(...) call back api across bm drivers.
What does indeed seem to be needed is a place to identify the battery type, but it's not clear if the btemp device is the best place for that (maybe it is).
I am not clear whether you are trying to correlate battery-type with supplied-to. however, battery type is identified based on the resistance value measured at batctrl pin which is expected to be in the allowable limit of ab8500 device. This resistance limit varies across battery types. This happens in btemp driver.
For this, I would suggest you give a list of
possible batteries and require a property such as
st-ericsson,battery-type: A string identifier for the type of battery, which impacts how an operating system interpret the sensor readings. Possible values include:
- "none" -- no battery connected
- "li-ion-9100" -- Type 9100 Li-ION battery
<add any others that apply here>
Can do this, not precisely as "st-ericsson,battery-type", it will be as battery-type = [unknown|NiMH|LION|...|]], reason being allowable battery type is based on technology, as you can see the possible types as: POWER_SUPPLY_TECHNOLOGY_UNKNOWN = 0, POWER_SUPPLY_TECHNOLOGY_NiMH, POWER_SUPPLY_TECHNOLOGY_LION, POWER_SUPPLY_TECHNOLOGY_LIPO, POWER_SUPPLY_TECHNOLOGY_LiFe, POWER_SUPPLY_TECHNOLOGY_NiCd, POWER_SUPPLY_TECHNOLOGY_LiMn Ref: include/linux/power_supply.h Note: doing this will impact my of_probe(...), may slightly bloat the code.
Arnd
On Thursday 13 September 2012, Rajanikanth HV wrote:
On Wednesday 12 September 2012 09:06 PM, Arnd Bergmann wrote:> >
If this is true, I don't understand what makes the 'supplied-to' properties you list in the device tree binding board specific. Are they not always done the same way? If so, you could just leave them out.
Precisely 'supplied-to' is not board specific, it was maintained as platform_data which i migrated to dt-node. It is meant to establish dependency across bm drivers based on power_supply property and runtime battery attributes. Basically, 'supplied-to' provides a way of exporting change in power_supply_property and runtime batter characteristics so that other bm devs shall make use or refer the updated values. Ref: external_power_changed(...) call back api. Note: all the bm drivers handles subset of power_supply property and battery attributes, ref: include/linux/power_supply.h and get_property(...) call back api across bm drivers.
Ok, so you want to just remove the property from the device tree, or do you want to establish a different method to specify these connections?
What does indeed seem to be needed is a place to identify the battery type, but it's not clear if the btemp device is the best place for that (maybe it is).
I am not clear whether you are trying to correlate battery-type with supplied-to. however, battery type is identified based on the resistance value measured at batctrl pin which is expected to be in the allowable limit of ab8500 device. This resistance limit varies across battery types. This happens in btemp driver.
I wasn't correlating them. I just mentioned that unlike the supplied-to property, the battery type property does seem to belong into the device tree.
For this, I would suggest you give a list of
possible batteries and require a property such as
st-ericsson,battery-type: A string identifier for the type of battery, which impacts how an operating system interpret the sensor readings. Possible values include:
- "none" -- no battery connected
- "li-ion-9100" -- Type 9100 Li-ION battery
<add any others that apply here>
Can do this, not precisely as "st-ericsson,battery-type", it will be as battery-type = [unknown|NiMH|LION|...|]], reason being allowable battery type is based on technology, as you can see the possible types as: POWER_SUPPLY_TECHNOLOGY_UNKNOWN = 0, POWER_SUPPLY_TECHNOLOGY_NiMH, POWER_SUPPLY_TECHNOLOGY_LION, POWER_SUPPLY_TECHNOLOGY_LIPO, POWER_SUPPLY_TECHNOLOGY_LiFe, POWER_SUPPLY_TECHNOLOGY_NiCd, POWER_SUPPLY_TECHNOLOGY_LiMn Ref: include/linux/power_supply.h Note: doing this will impact my of_probe(...), may slightly bloat the code.
Ok.
If you want to make the battery type a generic property, it's probably best to start a separate binding document for this in Documentation/devicetree/bindings/power-supply/common.txt and document a string for each of these.
If we expect the property to be needed only for ab8500, please use a vendor prefix like 'stericsson,'.
Arnd
(Thanks for Cc'ing me.)
On Thu, Sep 13, 2012 at 02:37:38PM +0000, Arnd Bergmann wrote: [...]
If this is true, I don't understand what makes the 'supplied-to' properties you list in the device tree binding board specific. Are they not always done the same way? If so, you could just leave them out.
Precisely 'supplied-to' is not board specific, it was maintained as platform_data which i migrated to dt-node. It is meant to establish dependency across bm drivers based on power_supply property and runtime battery attributes. Basically, 'supplied-to' provides a way of exporting change in power_supply_property and runtime batter characteristics so that other bm devs shall make use or refer the updated values. Ref: external_power_changed(...) call back api. Note: all the bm drivers handles subset of power_supply property and battery attributes, ref: include/linux/power_supply.h and get_property(...) call back api across bm drivers.
Ok, so you want to just remove the property from the device tree, or do you want to establish a different method to specify these connections?
Power supply subsystem's supplied_to describes not just how driver should notify other devices, supplied_to is more generic stuff, in terms that it describes power supply hierarchy. It's like a directed graph, e.g.:
<AC power> supplied_to <main battery> and <backup battery> <USB power> supplied_to <main battery> and <backup battery> <main battery> supplied_to <system> <backup battery> supplied_to <system> <cmos battery> supplied_to <southbridge pci device> <mice battery> supplied_to <mice wireless hid>
How things interact in linux are just implementations details. So, device tree is surely a perfect place to describe these things.
Although, in current bindings I see this:
+ ab8500-fg { + /* Other enery management module */ + supplied_to = "ab8500_chargalg", "ab8500_usb"; + num_supplicants = <2>; + };
Instead of addressing supplicants by name, it's better to address via phandles. And, of course, num_supplicants is not needed, it can be derived.
[...]
possible batteries and require a property such as
st-ericsson,battery-type: A string identifier for the type of battery, which impacts how an operating system interpret the sensor readings. Possible values include:
- "none" -- no battery connected
- "li-ion-9100" -- Type 9100 Li-ION battery
<add any others that apply here>
Can do this, not precisely as "st-ericsson,battery-type", it will be as battery-type = [unknown|NiMH|LION|...|]], reason being allowable battery type is based on technology, as you can see the possible types as: POWER_SUPPLY_TECHNOLOGY_UNKNOWN = 0, POWER_SUPPLY_TECHNOLOGY_NiMH, POWER_SUPPLY_TECHNOLOGY_LION, POWER_SUPPLY_TECHNOLOGY_LIPO, POWER_SUPPLY_TECHNOLOGY_LiFe, POWER_SUPPLY_TECHNOLOGY_NiCd, POWER_SUPPLY_TECHNOLOGY_LiMn Ref: include/linux/power_supply.h Note: doing this will impact my of_probe(...), may slightly bloat the code.
Ok.
If you want to make the battery type a generic property, it's probably best to start a separate binding document for this in Documentation/devicetree/bindings/power-supply/common.txt and document a string for each of these.
Fully agree. We need to document generic DT bindings for power supplies.
Thanks, Anton.
On Friday 14 September 2012, Anton Vorontsov wrote:
Power supply subsystem's supplied_to describes not just how driver should notify other devices, supplied_to is more generic stuff, in terms that it describes power supply hierarchy. It's like a directed graph, e.g.:
<AC power> supplied_to <main battery> and <backup battery> <USB power> supplied_to <main battery> and <backup battery><main battery> supplied_to <system> <backup battery> supplied_to <system> <cmos battery> supplied_to <southbridge pci device> <mice battery> supplied_to <mice wireless hid>
How things interact in linux are just implementations details. So, device tree is surely a perfect place to describe these things.
Although, in current bindings I see this:
ab8500-fg {/* Other enery management module */supplied_to = "ab8500_chargalg", "ab8500_usb";num_supplicants = <2>;};Instead of addressing supplicants by name, it's better to address via phandles. And, of course, num_supplicants is not needed, it can be derived.
Right. that's what I thought. The other comment I made initially is that it would be more in the spirit of the existing bindings to have the supply property in the opposite directory, if we need it, like (picking up your above example):
/ { /* power supply property in the root node is used by default */ power-supply = <&main-battery>, <&backup-battery>;
ac-power: power@... { ... };
usb-power: power@... { ... };
main-battery: battery@... { power-supply = <&ac-power>, <&usb-power}; ;
... };
It's the same information and absolutely equivalent as far as I can tell, but it feel more logical in the way we tend to describe things.
Arnd
On Friday 14 September 2012 01:39 PM, Arnd Bergmann wrote:
On Friday 14 September 2012, Anton Vorontsov wrote:
Power supply subsystem's supplied_to describes not just how driver should notify other devices, supplied_to is more generic stuff, in terms that it describes power supply hierarchy. It's like a directed graph, e.g.:
<AC power> supplied_to <main battery> and <backup battery> <USB power> supplied_to <main battery> and <backup battery><main battery> supplied_to <system> <backup battery> supplied_to <system> <cmos battery> supplied_to <southbridge pci device> <mice battery> supplied_to <mice wireless hid>
How things interact in linux are just implementations details. So, device tree is surely a perfect place to describe these things.
Although, in current bindings I see this:
ab8500-fg {/* Other enery management module */supplied_to = "ab8500_chargalg", "ab8500_usb";num_supplicants = <2>;};Instead of addressing supplicants by name, it's better to address via phandles. And, of course, num_supplicants is not needed, it can be derived.
Right. that's what I thought. The other comment I made initially is that it would be more in the spirit of the existing bindings to have the supply property in the opposite directory, if we need it, like (picking up your above example):
/ { /* power supply property in the root node is used by default */ power-supply = <&main-battery>, <&backup-battery>;
ac-power: power@... { ... };
usb-power: power@... { ... };
main-battery: battery@... { power-supply = <&ac-power>, <&usb-power}; ;
... };
It's the same information and absolutely equivalent as far as I can tell, but it feel more logical in the way we tend to describe things.
Arnd
phandle'd supplied-to will looks like:
usb: ab8500-usb {
};
battery : ab8500-bat-type { battery-name = "unknown|NiMH|LION|LIPO|LiFe|NiCd|LiMn"; thermistor_on_batctrl = <1>; };
chargalg: ab8500-chargalg { compatible = "stericsson,ab8500-chargalg"; interface-name = "ab8500_chargalg"; battery-info = <&ab8500-bat-type> supplied-to = <&ab8500_fg>; ... };
fuelguage: ab8500-fg { compatible = "stericsson,ab8500-fg"; interface-name = "ab8500_fg"; battery-info = <&ab8500-bat-type> supplied-to = <&ab8500_chargalg &ab8500_usb>; ... }; btemp: ab8500-btemp { compatible = "stericsson,ab8500-fg"; interface-name = "ab8500_btemp"; battery-info = <&ab8500-bat-type> supplied-to = <&ab8500_chargalg &ab8500_fg>; ... };
charger: ab8500-charger { compatible = "stericsson,ab8500-charger"; interface-name = "ab8500_charger"; battery-info = <&ab8500-bat-type> supplied-to = <&ab8500_chargalg &ab8500_fg &ab8500_btemp>; ... };
On Thursday 13 September 2012 08:07 PM, Arnd Bergmann wrote:
On Thursday 13 September 2012, Rajanikanth HV wrote:
On Wednesday 12 September 2012 09:06 PM, Arnd Bergmann wrote:> >
If this is true, I don't understand what makes the 'supplied-to' properties you list in the device tree binding board specific. Are they not always done the same way? If so, you could just leave them out.
Precisely 'supplied-to' is not board specific, it was maintained as platform_data which i migrated to dt-node. It is meant to establish dependency across bm drivers based on power_supply property and runtime battery attributes. Basically, 'supplied-to' provides a way of exporting change in power_supply_property and runtime batter characteristics so that other bm devs shall make use or refer the updated values. Ref: external_power_changed(...) call back api. Note: all the bm drivers handles subset of power_supply property and battery attributes, ref: include/linux/power_supply.h and get_property(...) call back api across bm drivers.
Ok, so you want to just remove the property from the device tree, or do you want to establish a different method to specify these connections?
What does indeed seem to be needed is a place to identify the battery type, but it's not clear if the btemp device is the best place for that (maybe it is).
I am not clear whether you are trying to correlate battery-type with supplied-to. however, battery type is identified based on the resistance value measured at batctrl pin which is expected to be in the allowable limit of ab8500 device. This resistance limit varies across battery types. This happens in btemp driver.
I wasn't correlating them. I just mentioned that unlike the supplied-to property, the battery type property does seem to belong into the device tree.
For this, I would suggest you give a list of
possible batteries and require a property such as
st-ericsson,battery-type: A string identifier for the type of battery, which impacts how an operating system interpret the sensor readings. Possible values include:
- "none" -- no battery connected
- "li-ion-9100" -- Type 9100 Li-ION battery
<add any others that apply here>
Can do this, not precisely as "st-ericsson,battery-type", it will be as battery-type = [unknown|NiMH|LION|...|]], reason being allowable battery type is based on technology, as you can see the possible types as: POWER_SUPPLY_TECHNOLOGY_UNKNOWN = 0, POWER_SUPPLY_TECHNOLOGY_NiMH, POWER_SUPPLY_TECHNOLOGY_LION, POWER_SUPPLY_TECHNOLOGY_LIPO, POWER_SUPPLY_TECHNOLOGY_LiFe, POWER_SUPPLY_TECHNOLOGY_NiCd, POWER_SUPPLY_TECHNOLOGY_LiMn Ref: include/linux/power_supply.h Note: doing this will impact my of_probe(...), may slightly bloat the code.
Ok.
If you want to make the battery type a generic property, it's probably best to start a separate binding document for this in Documentation/devicetree/bindings/power-supply/common.txt and document a string for each of these.
If we expect the property to be needed only for ab8500, please use a vendor prefix like 'stericsson,'.
it is fine to have battery-name in the battery-type property as said above and to have binding document, the basic requirement is to maintain the battery type information for a specified battery also within the same technology of battery method, battery parameters may vary. Presently battery-type and its dependent information is maintained in the drivers/power/ folder as per the arnd review sometimes back.
Arnd
On 09/10/2012 01:21 PM, Rajanikanth HV wrote: ...
-static int __devinit ab8500_btemp_probe(struct platform_device *pdev) +static int __devinit +btemp_of_probe(struct device *dev,
struct device_node *np,struct abx500_bm_plat_data *bm_pdata){
- int irq, i, ret = 0;
- u8 val;
- struct abx500_bm_plat_data *plat_data = pdev->dev.platform_data;
- struct ab8500_btemp *di;
- if (!plat_data) {
dev_err(&pdev->dev, "No platform data\n");return -EINVAL;
- u8 val;
- u32 pval;
- int i;
- int ext_thermister, lion_battery, ret = 0;
- const char *bm_dev_name;
- struct abx500_btemp_platform_data *btemp = bm_pdata->btemp;
- struct abx500_bm_data *bat;
- struct abx500_battery_type *btype;
- ret = of_property_read_u32(np, "num_supplicants", &pval);
- if (ret) {
dev_err(dev, "missing property num_supplicants\n");ret = -EINVAL;goto inval_pval;
- di = kzalloc(sizeof(*di), GFP_KERNEL);
- if (!di)
return -ENOMEM;- /* get parent data */
- di->dev = &pdev->dev;
- 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");
- btemp->num_supplicants = pval;
- btemp->supplied_to =
devm_kzalloc(dev, btemp->num_supplicants *sizeof(const char *), GFP_KERNEL);- if (btemp->supplied_to == NULL) {
dev_err(dev, "%s no mem for supplied_to\n", __func__);ret = -ENOMEM;goto inval_pval;- }
- for (val = 0; val < btemp->num_supplicants; ++val)
if (of_property_read_string_index(np, "supplied_to", val, &bm_dev_name) == 0)*(btemp->supplied_to + val) = (char *)bm_dev_name;else {dev_err(dev, "insufficient number of supplied_to data found\n");ret = -EINVAL;goto free_dev_mem;}- ret = of_property_read_u32(np, "thermister_on_batctrl", &pval);
- if (ret) {
dev_err(dev, "missing property thermister_on_batctrl\n");
goto free_device_info;
goto free_dev_mem;- }
- bm_pdata->battery = &ab8500_bm_data;
- bat = bm_pdata->battery;
- ext_thermister = 0;
- if (pval == 0) {
bat->n_btypes = 4;bat->bat_type = bat_type_ext_thermister;bat->adc_therm = ABx500_ADC_THERM_BATTEMP;ext_thermister = 1;- }
- ret = of_property_read_u32(np, "li_ion_9100", &pval);
- if (ret) {
dev_err(dev, "missing property li_ion_9100\n");ret = -EINVAL;goto free_dev_mem;- }
- lion_battery = 0;
- if (pval == 1) {
bat->no_maintenance = true;bat->chg_unknown_bat = true;bat->bat_type[BATTERY_UNKNOWN].charge_full_design = 2600;bat->bat_type[BATTERY_UNKNOWN].termination_vol = 4150;bat->bat_type[BATTERY_UNKNOWN].recharge_vol = 4130;bat->bat_type[BATTERY_UNKNOWN].normal_cur_lvl = 520;bat->bat_type[BATTERY_UNKNOWN].normal_vol_lvl = 4200;lion_battery = 1;- }
- /* select the battery resolution table */
- for (i = 0; i < bat->n_btypes; ++i) {
btype = (bat->bat_type + i);if (ext_thermister) {btype->batres_tbl =temp_to_batres_tbl_ext_thermister;} else if (lion_battery) {btype->batres_tbl =temp_to_batres_tbl_9100;} else {btype->batres_tbl =temp_to_batres_tbl_thermister;}- }
- return ret;
+free_dev_mem:
- devm_kfree(dev, btemp->supplied_to);
+inval_pval:
- return ret;
+}
+static int __devinit ab8500_btemp_probe(struct platform_device *pdev) +{
- u8 val;
- int i;
- int irq, ret = 0;
- struct abx500_bm_plat_data *pdata = pdev->dev.platform_data;
- struct device_node *np = pdev->dev.of_node;
- struct ab8500_btemp *di;
- di = devm_kzalloc(&pdev->dev, sizeof(*di), GFP_KERNEL);
- if (!di) {
dev_err(&pdev->dev, "%s no mem for ab8500_btemp\n", __func__);ret = -ENOMEM;
- /* get battery specific platform data */
- di->bat = plat_data->battery;
- if (!di->bat) {
dev_err(di->dev, "no battery platform data supplied\n");
if (np) {
if (!pdata) {pdata =devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);if (!pdata) {dev_err(&pdev->dev,"%s no mem for pdata\n", __func__);ret = -ENOMEM;goto err_no_mem;}pdata->btemp = devm_kzalloc(&pdev->dev,sizeof(*pdata->btemp), GFP_KERNEL);if (!pdata->btemp) {devm_kfree(&pdev->dev, pdata);dev_err(&pdev->dev,"%s no mem for pdata->btemp\n",__func__);ret = -ENOMEM;goto free_device_info;}}/* get battery specific platform data */ret = btemp_of_probe(&pdev->dev, np, pdata);if (ret) {devm_kfree(&pdev->dev, pdata->btemp);devm_kfree(&pdev->dev, pdata);goto free_device_info;}}
if (!pdata) {
dev_err(&pdev->dev,"%s no btemp platform data found\n", __func__);ret = -EINVAL; goto free_device_info; }
di->pdata = pdata->btemp;
di->bat = pdata->battery;
/* get parent data */
di->dev = &pdev->dev;
di->parent = dev_get_drvdata(pdev->dev.parent);
di->gpadc = ab8500_gpadc_get("ab8500-gpadc.0");
/* BTEMP supply */ di->btemp_psy.name = "ab8500_btemp";
@@ -1008,7 +1121,6 @@ static int __devinit ab8500_btemp_probe(struct platform_device *pdev) di->btemp_psy.external_power_changed = ab8500_btemp_external_power_changed;
- /* Create a work queue for the btemp */ di->btemp_wq = create_singlethread_workqueue("ab8500_btemp_wq");
@@ -1065,7 +1177,7 @@ static int __devinit ab8500_btemp_probe(struct platform_device *pdev) IRQF_SHARED | IRQF_NO_SUSPEND, ab8500_btemp_irq[i].name, di);
if (ret) {
if (ret < 0) { dev_err(di->dev, "failed to request %s IRQ %d: %d\n" , ab8500_btemp_irq[i].name, irq, ret); goto free_irq;@@ -1093,11 +1205,17 @@ free_irq: free_btemp_wq: destroy_workqueue(di->btemp_wq); free_device_info:
- kfree(di);
- devm_kfree(&pdev->dev, di);
+err_no_mem:
return ret; }
Same story as in fg driver, there is no need to call devm_kfree() on error in probe functions, and kfree(di) should be removed from ab8500_btemp_remove()
-- Francesco
-
Anton Vorontsov -
Arnd Bergmann -
Francesco Lavra -
Linus Walleij -
Rajanikanth HV -
Rajanikanth HV