From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753113Ab2JAKAL (ORCPT ); Mon, 1 Oct 2012 06:00:11 -0400 Received: from eu1sys200aog108.obsmtp.com ([207.126.144.125]:54909 "EHLO eu1sys200aog108.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753079Ab2JAKAI (ORCPT ); Mon, 1 Oct 2012 06:00:08 -0400 Message-ID: <50696991.2080103@stericsson.com> Date: Mon, 1 Oct 2012 15:29:45 +0530 From: Rajanikanth HV Organization: st.com User-Agent: Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20120827 Thunderbird/15.0 MIME-Version: 1.0 To: Lee Jones Cc: "francescolavra.fl@gmail.com" , "arnd@arndb.de" , "anton.vorontsov@linaro.org" , Linus WALLEIJ , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "linaro-dev@lists.linaro.org" , "patches@linaro.org" , STEricsson_nomadik_linux Subject: Re: [PATCH 1/4] mfd: ab8500: add devicetree support for fuelgauge References: <1349064513-31301-1-git-send-email-rajanikanth.hv@stericsson.com> <1349064513-31301-2-git-send-email-rajanikanth.hv@stericsson.com> <20121001094929.GB6682@gmail.com> In-Reply-To: <20121001094929.GB6682@gmail.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org did you have a look at arnd and anton comments regarding 'supplied-to' and boolean property On Monday 01 October 2012 03:19 PM, Lee Jones wrote: > On Mon, 01 Oct 2012, Rajanikanth H.V wrote: > >> From: "Rajanikanth H.V" >> >> - This patch adds device tree support for fuelguage driver >> - optimize bm devices platform_data usage and of_probe(...) >> Note: of_probe() routine for battery managed devices is made >> common across all bm drivers. >> >> Signed-off-by: Rajanikanth H.V >> --- >> Documentation/devicetree/bindings/mfd/ab8500.txt | 8 +- >> .../devicetree/bindings/power_supply/ab8500/fg.txt | 86 +++ >> arch/arm/boot/dts/dbx5x0.dtsi | 22 +- >> drivers/mfd/ab8500-core.c | 1 + >> drivers/power/Makefile | 2 +- >> drivers/power/ab8500_bmdata.c | 549 ++++++++++++++++++++ >> drivers/power/ab8500_btemp.c | 4 +- >> drivers/power/ab8500_charger.c | 4 +- >> drivers/power/ab8500_fg.c | 76 ++- >> drivers/power/abx500_chargalg.c | 4 +- >> include/linux/mfd/abx500.h | 37 +- >> include/linux/mfd/abx500/ab8500-bm.h | 7 + >> 12 files changed, 744 insertions(+), 56 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/power_supply/ab8500/fg.txt >> create mode 100644 drivers/power/ab8500_bmdata.c >> >> diff --git a/Documentation/devicetree/bindings/mfd/ab8500.txt b/Documentation/devicetree/bindings/mfd/ab8500.txt >> index ce83c8d..762dc11 100644 >> --- a/Documentation/devicetree/bindings/mfd/ab8500.txt >> +++ b/Documentation/devicetree/bindings/mfd/ab8500.txt >> @@ -24,7 +24,13 @@ ab8500-bm : : : Battery Manager >> ab8500-btemp : : : Battery Temperature >> ab8500-charger : : : Battery Charger >> ab8500-codec : : : Audio Codec >> -ab8500-fg : : : Fuel Gauge >> +ab8500-fg : : vddadc : Fuel Gauge >> + : NCONV_ACCU : : Accumulate N Sample Conversion >> + : BATT_OVV : : Battery Over Voltage >> + : LOW_BAT_F : : LOW threshold battery voltage >> + : CC_INT_CALIB : : Counter Counter Internal Calibration > > I think you mean: Coulomb Counter. > >> + : CCEOC : : Coulomb Counter End of Conversion >> + : : : > > Random empty entry. > >> ab8500-gpadc : HW_CONV_END : vddadc : Analogue to Digital Converter >> SW_CONV_END : : >> ab8500-gpio : : : GPIO Controller >> diff --git a/Documentation/devicetree/bindings/power_supply/ab8500/fg.txt b/Documentation/devicetree/bindings/power_supply/ab8500/fg.txt >> new file mode 100644 >> index 0000000..caa33b0 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power_supply/ab8500/fg.txt >> @@ -0,0 +1,86 @@ >> +=== AB8500 Fuel Gauge Driver === >> + >> +AB8500 is a mixed signal multimedia and power management >> +device comprising: power and energy-management-module, >> +wall-charger, usb-charger, audio codec, general purpose adc, >> +tvout, clock management and sim card interface. >> + >> +Fuel-guage support is part of energy-management-module, the other > > Spelling. > >> +components of this module are: >> +main-charger, usb-combo-charger and Battery temperature monitoring. >> + >> +The properties below describes the node for fuel guage driver. > > Spelling. > >> + >> +Required Properties: >> +- compatible = "stericsson,ab8500-fg" >> +- interface-name: >> + Name of the controller/driver which is part of energy-management-module >> +- supplied-to: > > Still not sure about this property, or your justification for use. > >> + This property shall have dependent nodes which represent other >> + energy-management-module. > > Plural? > >> + This is a logical binding w.r.t power supply events > > Proper English please, no slang. > >> + across energy-management-module drivers where-in, the > > Ill placed comma? > >> + runtime battery properties are shared along with uevent >> + notification. > > Plural? > >> + ref: di->fg.external_power_changed = >> + ab8500_fg_external_power_changed; >> + ab8500_fg.c >> + >> + Need for this property: >> + energy-management-module driver updates power-supply properties >> + which are subset of events listed in 'enum power_supply_property', >> + ref: power_supply.h file >> + 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 >> + di->fg_psy.external_power_changed >> + >> + example: >> + ab8500-fg { >> + /* dependent energy management modules */ >> + supplied-to = <&ab8500_chargalg &ab8500_usb>; >> + }; >> + >> + ab8500_battery_info: ab8500_bat_type { >> + battery-type = <2>; >> + thermistor-on-batctrl = <1>; > > You have this as a bool here, and ... >> + }; >> + >> +Other dependent node for fuel-gauge is: >> + ab8500_battery_info: ab8500_bat_type { >> + }; >> + This node will provide information on 'thermistor interface' and >> + 'battery technology type' used. >> + >> +Properties of this node are: >> +thermistor-on-batctrl: >> + A boolean value indicating thermistor interface to battery >> + >> + Note: >> + 'btemp' and 'batctrl' are the pins interfaced for battery temperature >> + measurement, 'btemp' signal is used when NTC(negative temperature >> + coefficient) resister is interfaced external to battery whereas >> + 'batctrl' pin is used when NTC resister is internal to battery. >> + >> + e.g: >> + ab8500_battery_info: ab8500_bat_type { >> + thermistor-on-batctrl; > > ... a standard property here. I suggest you drop the bool value. > >> + }; >> + indiactes: NTC resister is internal to battery, 'batctrl' is used >> + for thermal measurement. >> + >> + The absence of property 'thermal-on-batctrl' indicates >> + NTC resister is external to battery and 'btemp' signal is used >> + for thermal measurement. >> + >> +battery-type: >> + This shall be the battery manufacturing technology type, >> + allowed types are: >> + "UNKNOWN" "NiMH" "LION" "LIPO" "LiFe" "NiCd" "LiMn" >> + e.g: >> + ab8500_battery_info: ab8500_bat_type { >> + battery-name = "LION"; >> + } >> + >> diff --git a/arch/arm/boot/dts/dbx5x0.dtsi b/arch/arm/boot/dts/dbx5x0.dtsi >> index 748ba7a..bd22c56 100644 >> --- a/arch/arm/boot/dts/dbx5x0.dtsi >> +++ b/arch/arm/boot/dts/dbx5x0.dtsi >> @@ -352,8 +352,28 @@ >> vddadc-supply = <&ab8500_ldo_tvout_reg>; >> }; >> >> - ab8500-usb { >> + ab8500_battery_info: ab8500_bat_type { >> + battery-name = "LION"; > > All new properties have to be documented. > > Vendor specific properties should be prepended with the vendor name, so > either write a generic binding document for all to use or prefix with > 'stericsson,". > >> + thermistor-on-batctrl; >> + }; >> + >> + ab8500_chargalg: ab8500_chalg { >> + compatible = "stericsson,ab8500-chargalg"; >> + interface-name = "ab8500_chargalg"; > > Same with all of your new properties (I'll stop mentioning them now). > >> + battery-info = <&ab8500_battery_info>; >> + supplied-to = <&ab8500_fuel_gauge>; > > Weren't you going to reverse this logic to be more inline with how > the reset of Device Tree works? > >> + }; >> + >> + ab8500_fuel_gauge: ab8500_fg { >> + compatible = "stericsson,ab8500-fg"; >> + interface-name = "ab8500_fg"; >> + battery-info = <&ab8500_battery_info>; >> + supplied-to = <&ab8500_chargalg &ab8500_usb>; > > As above. > >> + }; >> + >> + ab8500_usb: ab8500_usb_if { > > What does 'if' mean? > >> compatible = "stericsson,ab8500-usb"; >> + interface-name = "ab8500_usb"; > > Why is this required? > >> interrupts = < 90 0x4 >> 96 0x4 >> 14 0x4 >> diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c >> index 1667c77..6c3d7c2 100644 >> --- a/drivers/mfd/ab8500-core.c >> +++ b/drivers/mfd/ab8500-core.c >> @@ -1051,6 +1051,7 @@ static struct mfd_cell __devinitdata ab8500_bm_devs[] = { >> }, >> { >> .name = "ab8500-fg", >> + .of_compatible = "stericsson,ab8500-fg", >> .num_resources = ARRAY_SIZE(ab8500_fg_resources), >> .resources = ab8500_fg_resources, >> }, >> diff --git a/drivers/power/Makefile b/drivers/power/Makefile >> index ee58afb..2c58d4e 100644 >> --- a/drivers/power/Makefile >> +++ b/drivers/power/Makefile >> @@ -34,7 +34,7 @@ obj-$(CONFIG_BATTERY_S3C_ADC) += s3c_adc_battery.o >> obj-$(CONFIG_CHARGER_PCF50633) += pcf50633-charger.o >> obj-$(CONFIG_BATTERY_JZ4740) += jz4740-battery.o >> obj-$(CONFIG_BATTERY_INTEL_MID) += intel_mid_battery.o >> -obj-$(CONFIG_AB8500_BM) += ab8500_charger.o ab8500_btemp.o ab8500_fg.o abx500_chargalg.o >> +obj-$(CONFIG_AB8500_BM) += ab8500_bmdata.o ab8500_charger.o ab8500_fg.o ab8500_btemp.o abx500_chargalg.o >> obj-$(CONFIG_CHARGER_ISP1704) += isp1704_charger.o >> obj-$(CONFIG_CHARGER_MAX8903) += max8903_charger.o >> obj-$(CONFIG_CHARGER_TWL4030) += twl4030_charger.o >> diff --git a/drivers/power/ab8500_bmdata.c b/drivers/power/ab8500_bmdata.c >> new file mode 100644 >> index 0000000..d0def3b >> --- /dev/null >> +++ b/drivers/power/ab8500_bmdata.c >> @@ -0,0 +1,549 @@ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* >> + * These are the defined batteries that uses a NTC and ID resistor placed >> + * inside of the battery pack. >> + * Note that the res_to_temp table must be strictly sorted by falling resistance >> + * values to work. >> + */ >> +static struct abx500_res_to_temp temp_tbl_A_thermistor[] = { >> + {-5, 53407}, >> + { 0, 48594}, >> + { 5, 43804}, >> + {10, 39188}, >> + {15, 34870}, >> + {20, 30933}, >> + {25, 27422}, >> + {30, 24347}, >> + {35, 21694}, >> + {40, 19431}, >> + {45, 17517}, >> + {50, 15908}, >> + {55, 14561}, >> + {60, 13437}, >> + {65, 12500}, >> +}; >> +static struct abx500_res_to_temp temp_tbl_B_thermistor[] = { >> + {-5, 165418}, >> + { 0, 159024}, >> + { 5, 151921}, >> + {10, 144300}, >> + {15, 136424}, >> + {20, 128565}, >> + {25, 120978}, >> + {30, 113875}, >> + {35, 107397}, >> + {40, 101629}, >> + {45, 96592}, >> + {50, 92253}, >> + {55, 88569}, >> + {60, 85461}, >> + {65, 82869}, >> +}; >> +static struct abx500_v_to_cap cap_tbl_A_thermistor[] = { >> + {4171, 100}, >> + {4114, 95}, >> + {4009, 83}, >> + {3947, 74}, >> + {3907, 67}, >> + {3863, 59}, >> + {3830, 56}, >> + {3813, 53}, >> + {3791, 46}, >> + {3771, 33}, >> + {3754, 25}, >> + {3735, 20}, >> + {3717, 17}, >> + {3681, 13}, >> + {3664, 8}, >> + {3651, 6}, >> + {3635, 5}, >> + {3560, 3}, >> + {3408, 1}, >> + {3247, 0}, >> +}; >> +static struct abx500_v_to_cap cap_tbl_B_thermistor[] = { >> + {4161, 100}, >> + {4124, 98}, >> + {4044, 90}, >> + {4003, 85}, >> + {3966, 80}, >> + {3933, 75}, >> + {3888, 67}, >> + {3849, 60}, >> + {3813, 55}, >> + {3787, 47}, >> + {3772, 30}, >> + {3751, 25}, >> + {3718, 20}, >> + {3681, 16}, >> + {3660, 14}, >> + {3589, 10}, >> + {3546, 7}, >> + {3495, 4}, >> + {3404, 2}, >> + {3250, 0}, >> +}; >> + >> +static struct abx500_v_to_cap cap_tbl[] = { >> + {4186, 100}, >> + {4163, 99}, >> + {4114, 95}, >> + {4068, 90}, >> + {3990, 80}, >> + {3926, 70}, >> + {3898, 65}, >> + {3866, 60}, >> + {3833, 55}, >> + {3812, 50}, >> + {3787, 40}, >> + {3768, 30}, >> + {3747, 25}, >> + {3730, 20}, >> + {3705, 15}, >> + {3699, 14}, >> + {3684, 12}, >> + {3672, 9}, >> + {3657, 7}, >> + {3638, 6}, >> + {3556, 4}, >> + {3424, 2}, >> + {3317, 1}, >> + {3094, 0}, >> +}; >> + >> +/* >> + * Note that the res_to_temp table must be strictly sorted by falling >> + * resistance values to work. >> + */ >> +static struct abx500_res_to_temp temp_tbl[] = { >> + {-5, 214834}, >> + { 0, 162943}, >> + { 5, 124820}, >> + {10, 96520}, >> + {15, 75306}, >> + {20, 59254}, >> + {25, 47000}, >> + {30, 37566}, >> + {35, 30245}, >> + {40, 24520}, >> + {45, 20010}, >> + {50, 16432}, >> + {55, 13576}, >> + {60, 11280}, >> + {65, 9425}, >> +}; >> + >> +/* >> + * Note that the batres_vs_temp table must be strictly sorted by falling >> + * temperature values to work. >> + */ >> +struct batres_vs_temp temp_to_batres_tbl_thermistor[] = { >> + { 40, 120}, >> + { 30, 135}, >> + { 20, 165}, >> + { 10, 230}, >> + { 00, 325}, >> + {-10, 445}, >> + {-20, 595}, >> +}; >> + >> +/* >> + * Note that the batres_vs_temp table must be strictly sorted by falling >> + * temperature values to work. >> + */ >> +struct batres_vs_temp temp_to_batres_tbl_ext_thermistor[] = { >> + { 60, 300}, >> + { 30, 300}, >> + { 20, 300}, >> + { 10, 300}, >> + { 00, 300}, >> + {-10, 300}, >> + {-20, 300}, >> +}; >> + >> +/* battery resistance table for LI ION 9100 battery */ >> +struct batres_vs_temp temp_to_batres_tbl_9100[] = { >> + { 60, 180}, >> + { 30, 180}, >> + { 20, 180}, >> + { 10, 180}, >> + { 00, 180}, >> + {-10, 180}, >> + {-20, 180}, >> +}; >> + >> +struct abx500_battery_type bat_type_thermistor[] = { >> +[BATTERY_UNKNOWN] = { >> + /* First element always represent the UNKNOWN battery */ >> + .name = POWER_SUPPLY_TECHNOLOGY_UNKNOWN, >> + .resis_high = 0, >> + .resis_low = 0, >> + .battery_resistance = 300, >> + .charge_full_design = 612, >> + .nominal_voltage = 3700, >> + .termination_vol = 4050, >> + .termination_curr = 200, >> + .recharge_vol = 3990, >> + .normal_cur_lvl = 400, >> + .normal_vol_lvl = 4100, >> + .maint_a_cur_lvl = 400, >> + .maint_a_vol_lvl = 4050, >> + .maint_a_chg_timer_h = 60, >> + .maint_b_cur_lvl = 400, >> + .maint_b_vol_lvl = 4000, >> + .maint_b_chg_timer_h = 200, >> + .low_high_cur_lvl = 300, >> + .low_high_vol_lvl = 4000, >> + .n_temp_tbl_elements = ARRAY_SIZE(temp_tbl), >> + .r_to_t_tbl = temp_tbl, >> + .n_v_cap_tbl_elements = ARRAY_SIZE(cap_tbl), >> + .v_to_cap_tbl = cap_tbl, >> + .n_batres_tbl_elements = ARRAY_SIZE(temp_to_batres_tbl_thermistor), >> + .batres_tbl = temp_to_batres_tbl_thermistor, >> +}, >> +{ >> + .name = POWER_SUPPLY_TECHNOLOGY_LIPO, >> + .resis_high = 53407, >> + .resis_low = 12500, >> + .battery_resistance = 300, >> + .charge_full_design = 900, >> + .nominal_voltage = 3600, >> + .termination_vol = 4150, >> + .termination_curr = 80, >> + .recharge_vol = 4130, >> + .normal_cur_lvl = 700, >> + .normal_vol_lvl = 4200, >> + .maint_a_cur_lvl = 600, >> + .maint_a_vol_lvl = 4150, >> + .maint_a_chg_timer_h = 60, >> + .maint_b_cur_lvl = 600, >> + .maint_b_vol_lvl = 4100, >> + .maint_b_chg_timer_h = 200, >> + .low_high_cur_lvl = 300, >> + .low_high_vol_lvl = 4000, >> + .n_temp_tbl_elements = ARRAY_SIZE(temp_tbl_A_thermistor), >> + .r_to_t_tbl = temp_tbl_A_thermistor, >> + .n_v_cap_tbl_elements = ARRAY_SIZE(cap_tbl_A_thermistor), >> + .v_to_cap_tbl = cap_tbl_A_thermistor, >> + .n_batres_tbl_elements = ARRAY_SIZE(temp_to_batres_tbl_thermistor), >> + .batres_tbl = temp_to_batres_tbl_thermistor, >> + >> +}, >> +{ >> + .name = POWER_SUPPLY_TECHNOLOGY_LIPO, >> + .resis_high = 165418, >> + .resis_low = 82869, >> + .battery_resistance = 300, >> + .charge_full_design = 900, >> + .nominal_voltage = 3600, >> + .termination_vol = 4150, >> + .termination_curr = 80, >> + .recharge_vol = 4130, >> + .normal_cur_lvl = 700, >> + .normal_vol_lvl = 4200, >> + .maint_a_cur_lvl = 600, >> + .maint_a_vol_lvl = 4150, >> + .maint_a_chg_timer_h = 60, >> + .maint_b_cur_lvl = 600, >> + .maint_b_vol_lvl = 4100, >> + .maint_b_chg_timer_h = 200, >> + .low_high_cur_lvl = 300, >> + .low_high_vol_lvl = 4000, >> + .n_temp_tbl_elements = ARRAY_SIZE(temp_tbl_B_thermistor), >> + .r_to_t_tbl = temp_tbl_B_thermistor, >> + .n_v_cap_tbl_elements = ARRAY_SIZE(cap_tbl_B_thermistor), >> + .v_to_cap_tbl = cap_tbl_B_thermistor, >> + .n_batres_tbl_elements = ARRAY_SIZE(temp_to_batres_tbl_thermistor), >> + .batres_tbl = temp_to_batres_tbl_thermistor, >> +}, >> +}; >> + >> +struct abx500_battery_type bat_type_ext_thermistor[] = { >> +[BATTERY_UNKNOWN] = { >> + /* First element always represent the UNKNOWN battery */ >> + .name = POWER_SUPPLY_TECHNOLOGY_UNKNOWN, >> + .resis_high = 0, >> + .resis_low = 0, >> + .battery_resistance = 300, >> + .charge_full_design = 612, >> + .nominal_voltage = 3700, >> + .termination_vol = 4050, >> + .termination_curr = 200, >> + .recharge_vol = 3990, >> + .normal_cur_lvl = 400, >> + .normal_vol_lvl = 4100, >> + .maint_a_cur_lvl = 400, >> + .maint_a_vol_lvl = 4050, >> + .maint_a_chg_timer_h = 60, >> + .maint_b_cur_lvl = 400, >> + .maint_b_vol_lvl = 4000, >> + .maint_b_chg_timer_h = 200, >> + .low_high_cur_lvl = 300, >> + .low_high_vol_lvl = 4000, >> + .n_temp_tbl_elements = ARRAY_SIZE(temp_tbl), >> + .r_to_t_tbl = temp_tbl, >> + .n_v_cap_tbl_elements = ARRAY_SIZE(cap_tbl), >> + .v_to_cap_tbl = cap_tbl, >> + .n_batres_tbl_elements = ARRAY_SIZE(temp_to_batres_tbl_thermistor), >> + .batres_tbl = temp_to_batres_tbl_thermistor, >> +}, >> +/* >> + * These are the batteries that doesn't have an internal NTC resistor to measure >> + * its temperature. The temperature in this case is measure with a NTC placed >> + * near the battery but on the PCB. >> + */ >> +{ >> + .name = POWER_SUPPLY_TECHNOLOGY_LIPO, >> + .resis_high = 76000, >> + .resis_low = 53000, >> + .battery_resistance = 300, >> + .charge_full_design = 900, >> + .nominal_voltage = 3700, >> + .termination_vol = 4150, >> + .termination_curr = 100, >> + .recharge_vol = 4130, >> + .normal_cur_lvl = 700, >> + .normal_vol_lvl = 4200, >> + .maint_a_cur_lvl = 600, >> + .maint_a_vol_lvl = 4150, >> + .maint_a_chg_timer_h = 60, >> + .maint_b_cur_lvl = 600, >> + .maint_b_vol_lvl = 4100, >> + .maint_b_chg_timer_h = 200, >> + .low_high_cur_lvl = 300, >> + .low_high_vol_lvl = 4000, >> + .n_temp_tbl_elements = ARRAY_SIZE(temp_tbl), >> + .r_to_t_tbl = temp_tbl, >> + .n_v_cap_tbl_elements = ARRAY_SIZE(cap_tbl), >> + .v_to_cap_tbl = cap_tbl, >> + .n_batres_tbl_elements = ARRAY_SIZE(temp_to_batres_tbl_thermistor), >> + .batres_tbl = temp_to_batres_tbl_thermistor, >> +}, >> +{ >> + .name = POWER_SUPPLY_TECHNOLOGY_LION, >> + .resis_high = 30000, >> + .resis_low = 10000, >> + .battery_resistance = 300, >> + .charge_full_design = 950, >> + .nominal_voltage = 3700, >> + .termination_vol = 4150, >> + .termination_curr = 100, >> + .recharge_vol = 4130, >> + .normal_cur_lvl = 700, >> + .normal_vol_lvl = 4200, >> + .maint_a_cur_lvl = 600, >> + .maint_a_vol_lvl = 4150, >> + .maint_a_chg_timer_h = 60, >> + .maint_b_cur_lvl = 600, >> + .maint_b_vol_lvl = 4100, >> + .maint_b_chg_timer_h = 200, >> + .low_high_cur_lvl = 300, >> + .low_high_vol_lvl = 4000, >> + .n_temp_tbl_elements = ARRAY_SIZE(temp_tbl), >> + .r_to_t_tbl = temp_tbl, >> + .n_v_cap_tbl_elements = ARRAY_SIZE(cap_tbl), >> + .v_to_cap_tbl = cap_tbl, >> + .n_batres_tbl_elements = ARRAY_SIZE(temp_to_batres_tbl_thermistor), >> + .batres_tbl = temp_to_batres_tbl_thermistor, >> +}, >> +{ >> + .name = POWER_SUPPLY_TECHNOLOGY_LION, >> + .resis_high = 95000, >> + .resis_low = 76001, >> + .battery_resistance = 300, >> + .charge_full_design = 950, >> + .nominal_voltage = 3700, >> + .termination_vol = 4150, >> + .termination_curr = 100, >> + .recharge_vol = 4130, >> + .normal_cur_lvl = 700, >> + .normal_vol_lvl = 4200, >> + .maint_a_cur_lvl = 600, >> + .maint_a_vol_lvl = 4150, >> + .maint_a_chg_timer_h = 60, >> + .maint_b_cur_lvl = 600, >> + .maint_b_vol_lvl = 4100, >> + .maint_b_chg_timer_h = 200, >> + .low_high_cur_lvl = 300, >> + .low_high_vol_lvl = 4000, >> + .n_temp_tbl_elements = ARRAY_SIZE(temp_tbl), >> + .r_to_t_tbl = temp_tbl, >> + .n_v_cap_tbl_elements = ARRAY_SIZE(cap_tbl), >> + .v_to_cap_tbl = cap_tbl, >> + .n_batres_tbl_elements = ARRAY_SIZE(temp_to_batres_tbl_thermistor), >> + .batres_tbl = temp_to_batres_tbl_thermistor, >> +}, >> +}; >> + >> +static const struct abx500_bm_capacity_levels cap_levels = { >> + .critical = 2, >> + .low = 10, >> + .normal = 70, >> + .high = 95, >> + .full = 100, >> +}; >> + >> +static const struct abx500_fg_parameters fg = { >> + .recovery_sleep_timer = 10, >> + .recovery_total_time = 100, >> + .init_timer = 1, >> + .init_discard_time = 5, >> + .init_total_time = 40, >> + .high_curr_time = 60, >> + .accu_charging = 30, >> + .accu_high_curr = 30, >> + .high_curr_threshold = 50, >> + .lowbat_threshold = 3100, >> + .battok_falling_th_sel0 = 2860, >> + .battok_raising_th_sel1 = 2860, >> + .user_cap_limit = 15, >> + .maint_thres = 97, >> +}; >> + >> +static const struct abx500_maxim_parameters maxi_params = { >> + .ena_maxi = true, >> + .chg_curr = 910, >> + .wait_cycles = 10, >> + .charger_curr_step = 100, >> +}; >> + >> +static const struct abx500_bm_charger_parameters chg = { >> + .usb_volt_max = 5500, >> + .usb_curr_max = 1500, >> + .ac_volt_max = 7500, >> + .ac_curr_max = 1500, >> +}; >> + >> +struct abx500_bm_data ab8500_bm_data = { >> + .temp_under = 3, >> + .temp_low = 8, >> + .temp_high = 43, >> + .temp_over = 48, >> + .main_safety_tmr_h = 4, >> + .temp_interval_chg = 20, >> + .temp_interval_nochg = 120, >> + .usb_safety_tmr_h = 4, >> + .bkup_bat_v = BUP_VCH_SEL_2P6V, >> + .bkup_bat_i = BUP_ICH_SEL_150UA, >> + .no_maintenance = false, >> + .adc_therm = ABx500_ADC_THERM_BATCTRL, >> + .chg_unknown_bat = false, >> + .enable_overshoot = false, >> + .fg_res = 100, >> + .cap_levels = &cap_levels, >> + .bat_type = bat_type_thermistor, >> + .n_btypes = 3, >> + .batt_id = 0, >> + .interval_charging = 5, >> + .interval_not_charging = 120, >> + .temp_hysteresis = 3, >> + .gnd_lift_resistance = 34, >> + .maxi = &maxi_params, >> + .chg_params = &chg, >> + .fg_params = &fg, >> +}; >> + >> +int __devinit >> +bmdevs_of_probe(struct device *dev, >> + struct device_node *np, >> + struct abx500_bm_plat_data *pdata) >> +{ >> + int i, ret = 0, thermistor = NTC_INTERNAL; >> + const __be32 *ph; >> + const char *bat_tech; >> + struct abx500_bm_data *bat; >> + struct abx500_battery_type *btype; >> + struct device_node *np_bat_supply; >> + struct abx500_bmdevs_plat_data *plat_data = pdata->bmdev_pdata; > > > > This spacing is uncharacteristic of Linux drivers. > > Usually, struct declarations come first. > > > >> + /* get phandle to 'supplied-to' node */ > > I thought you were going to reverse this? > >> + ph = of_get_property(np, "supplied-to", &plat_data->num_supplicants); >> + if (ph == NULL) { > > if (!ph) { > >> + dev_err(dev, "no supplied_to property specified\n"); >> + return -EINVAL; >> + } >> + plat_data->num_supplicants /= sizeof(int); >> + plat_data->supplied_to = >> + devm_kzalloc(dev, plat_data->num_supplicants * >> + sizeof(const char *), GFP_KERNEL); >> + if (plat_data->supplied_to == NULL) { >> + dev_err(dev, "%s no mem for supplied-to\n", __func__); >> + return -ENOMEM; >> + } >> + for (i = 0; i < plat_data->num_supplicants; ++i) { >> + np_bat_supply = of_find_node_by_phandle(be32_to_cpup(ph) + i); > > Use: of_parse_phandle(np, "supplied-to", i) instead. > >> + if (np_bat_supply == NULL) { > > if (!np_bat_supply) { > >> + dev_err(dev, "invalid supplied_to property\n"); >> + return -EINVAL; >> + } >> + ret = of_property_read_string(np_bat_supply, "interface-name", >> + (const char **)(plat_data->supplied_to + i)); >> + if (ret < 0) { >> + of_node_put(np_bat_supply); >> + dev_err(dev, "supply/interface name not found\n"); >> + return ret; >> + } >> + dev_dbg(dev, "%s power supply interface_name:%s\n", >> + __func__, *(plat_data->supplied_to + i)); >> + } > > > >> + /* get phandle to 'battery-info' node */ >> + ph = of_get_property(np, "battery-info", NULL); >> + if (ph == NULL) { >> + dev_err(dev, "missing property battery-info\n"); >> + return -EINVAL; >> + } >> + np_bat_supply = of_find_node_by_phandle(be32_to_cpup(ph)); > > > > ... and replace with: np_bat_supply = of_parse_phandle(np, "battery-info", 0) instead. > >> + if (np_bat_supply == NULL) { > > if (!np_bat_supply) { > > I'll not mention this again. > >> + dev_err(dev, "invalid battery-info node\n"); >> + return -EINVAL; >> + } >> + if (of_property_read_bool(np_bat_supply, >> + "thermistor-on-batctrl") == false){ > > Replace with: > if (of_get_property(np_bat_supply, "thermistor-on-batctr", NULL)) > np_bat_supply = true; > > > >> + dev_warn(dev, "missing property thermistor-on-batctrl\n"); >> + thermistor = NTC_EXTERNAL; >> + } > > > >> + pdata->battery = &ab8500_bm_data; >> + bat = pdata->battery; > > Why not: bat = &ab8500_bm_data > > Or just use ab8500_bm_data in its own right? > >> + if (thermistor == NTC_EXTERNAL) { >> + bat->n_btypes = 4; >> + bat->bat_type = bat_type_ext_thermistor; >> + bat->adc_therm = ABx500_ADC_THERM_BATTEMP; >> + } >> + ret = of_property_read_string(np_bat_supply, "battery-name", &bat_tech); >> + if (ret < 0) { >> + dev_warn(dev, "missing property battery-name/type\n"); >> + bat_tech = "UNKNOWN"; >> + } >> + of_node_put(np_bat_supply); >> + if (strcmp(bat_tech, "LION") == 0) { >> + 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; >> + } >> + /* select the battery resolution table */ >> + for (i = 0; i < bat->n_btypes; ++i) { >> + btype = (bat->bat_type + i); >> + if (thermistor == NTC_EXTERNAL) { >> + btype->batres_tbl = >> + temp_to_batres_tbl_ext_thermistor; >> + } else if (strcmp(bat_tech, "LION") == 0) { > > Isn't strncmp safer, since you know the size of the comparison? > >> + btype->batres_tbl = >> + temp_to_batres_tbl_9100; >> + } else { >> + btype->batres_tbl = >> + temp_to_batres_tbl_thermistor; >> + } >> + } >> + return 0; >> +} >> +EXPORT_SYMBOL(bmdevs_of_probe); > > Why are you exporting? > >> diff --git a/drivers/power/ab8500_btemp.c b/drivers/power/ab8500_btemp.c >> index bba3cca..8e427e7 100644 >> --- a/drivers/power/ab8500_btemp.c >> +++ b/drivers/power/ab8500_btemp.c >> @@ -93,7 +93,7 @@ struct ab8500_btemp { >> struct ab8500 *parent; >> struct ab8500_gpadc *gpadc; >> struct ab8500_fg *fg; >> - struct abx500_btemp_platform_data *pdata; >> + struct abx500_bmdevs_plat_data *pdata; >> struct abx500_bm_data *bat; >> struct power_supply btemp_psy; >> struct ab8500_btemp_events events; >> @@ -982,7 +982,7 @@ static int __devinit ab8500_btemp_probe(struct platform_device *pdev) >> di->gpadc = ab8500_gpadc_get("ab8500-gpadc.0"); >> >> /* get btemp specific platform data */ >> - di->pdata = plat_data->btemp; >> + di->pdata = plat_data->bmdev_pdata; >> if (!di->pdata) { >> dev_err(di->dev, "no btemp platform data supplied\n"); >> ret = -EINVAL; >> diff --git a/drivers/power/ab8500_charger.c b/drivers/power/ab8500_charger.c >> index d4f0c98..5ff0d83 100644 >> --- a/drivers/power/ab8500_charger.c >> +++ b/drivers/power/ab8500_charger.c >> @@ -220,7 +220,7 @@ struct ab8500_charger { >> bool autopower; >> struct ab8500 *parent; >> struct ab8500_gpadc *gpadc; >> - struct abx500_charger_platform_data *pdata; >> + struct abx500_bmdevs_plat_data *pdata; >> struct abx500_bm_data *bat; >> struct ab8500_charger_event_flags flags; >> struct ab8500_charger_usb_state usb_state; >> @@ -2555,7 +2555,7 @@ static int __devinit ab8500_charger_probe(struct platform_device *pdev) >> spin_lock_init(&di->usb_state.usb_lock); >> >> /* get charger specific platform data */ >> - di->pdata = plat_data->charger; >> + di->pdata = plat_data->bmdev_pdata; >> if (!di->pdata) { >> dev_err(di->dev, "no charger platform data supplied\n"); >> ret = -EINVAL; >> diff --git a/drivers/power/ab8500_fg.c b/drivers/power/ab8500_fg.c >> index bf02225..96741b8 100644 >> --- a/drivers/power/ab8500_fg.c >> +++ b/drivers/power/ab8500_fg.c >> @@ -22,15 +22,14 @@ >> #include >> #include >> #include >> -#include >> -#include >> #include >> -#include >> #include >> -#include >> -#include >> #include >> #include >> +#include >> +#include >> +#include >> +#include >> >> #define MILLI_TO_MICRO 1000 >> #define FG_LSB_IN_MA 1627 >> @@ -212,7 +211,7 @@ struct ab8500_fg { >> struct ab8500_fg_avg_cap avg_cap; >> struct ab8500 *parent; >> struct ab8500_gpadc *gpadc; >> - struct abx500_fg_platform_data *pdata; >> + struct abx500_bmdevs_plat_data *pdata; >> struct abx500_bm_data *bat; >> struct power_supply fg_psy; >> struct workqueue_struct *fg_wq; >> @@ -544,14 +543,14 @@ cc_err: >> ret = abx500_set_register_interruptible(di->dev, >> AB8500_GAS_GAUGE, AB8500_GASG_CC_NCOV_ACCU, >> SEC_TO_SAMPLE(10)); >> - if (ret) >> + if (ret < 0) > > I don't 'think' this change is required. abx500_set_register_interruptible > will only return !0 on error. > >> goto fail; >> >> /* Start the CC */ >> ret = abx500_set_register_interruptible(di->dev, AB8500_RTC, >> AB8500_RTC_CC_CONF_REG, >> (CC_DEEP_SLEEP_ENA | CC_PWR_UP_ENA)); >> - if (ret) >> + if (ret < 0) >> goto fail; >> } else { >> di->turn_off_fg = false; >> @@ -2429,7 +2428,6 @@ static int __devexit ab8500_fg_remove(struct platform_device *pdev) >> flush_scheduled_work(); >> power_supply_unregister(&di->fg_psy); >> platform_set_drvdata(pdev, NULL); >> - kfree(di); >> return ret; >> } >> >> @@ -2446,18 +2444,47 @@ static int __devinit ab8500_fg_probe(struct platform_device *pdev) >> { >> int i, irq; >> int ret = 0; >> - struct abx500_bm_plat_data *plat_data = pdev->dev.platform_data; >> + struct abx500_bm_plat_data *plat_data >> + = pdev->dev.platform_data; >> + struct device_node *np = pdev->dev.of_node; >> struct ab8500_fg *di; >> >> + di = devm_kzalloc(&pdev->dev, sizeof(*di), GFP_KERNEL); >> + if (!di) { >> + dev_err(&pdev->dev, "%s no mem for ab8500_fg\n", __func__); >> + return -ENOMEM; >> + } >> + if (np) { >> + if (!plat_data) { > > Change these around. > > if (!plat_data) { > if (np) { > > } else { > > } > } > >> + plat_data = >> + devm_kzalloc(&pdev->dev, sizeof(*plat_data), >> + GFP_KERNEL); >> + if (!plat_data) { >> + dev_err(&pdev->dev, >> + "%s no mem for plat_data\n", __func__); >> + return -ENOMEM; >> + } >> + plat_data->bmdev_pdata = devm_kzalloc(&pdev->dev, >> + sizeof(*plat_data->bmdev_pdata), GFP_KERNEL); >> + if (!plat_data->bmdev_pdata) { >> + dev_err(&pdev->dev, >> + "%s no mem for pdata->fg\n", >> + __func__); >> + return -ENOMEM; >> + } >> + } >> + ret = bmdevs_of_probe(&pdev->dev, np, plat_data); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "failed to get platform data\n"); >> + return ret; >> + } >> + } > > > >> if (!plat_data) { >> - dev_err(&pdev->dev, "No platform data\n"); >> + dev_err(&pdev->dev, >> + "%s no fg platform data found\n", __func__); >> return -EINVAL; >> } > > >> - di = kzalloc(sizeof(*di), GFP_KERNEL); >> - if (!di) >> - return -ENOMEM; >> - >> mutex_init(&di->cc_lock); >> >> /* get parent data */ >> @@ -2466,19 +2493,17 @@ static int __devinit ab8500_fg_probe(struct platform_device *pdev) >> di->gpadc = ab8500_gpadc_get("ab8500-gpadc.0"); >> >> /* get fg specific platform data */ >> - di->pdata = plat_data->fg; >> + di->pdata = plat_data->bmdev_pdata; >> if (!di->pdata) { >> dev_err(di->dev, "no fg platform data supplied\n"); >> - ret = -EINVAL; >> - goto free_device_info; >> + return -EINVAL; >> } >> >> /* get battery specific platform data */ >> di->bat = plat_data->battery; >> if (!di->bat) { >> dev_err(di->dev, "no battery platform data supplied\n"); >> - ret = -EINVAL; >> - goto free_device_info; >> + return -EINVAL; >> } >> >> di->fg_psy.name = "ab8500_fg"; >> @@ -2506,7 +2531,7 @@ static int __devinit ab8500_fg_probe(struct platform_device *pdev) >> di->fg_wq = create_singlethread_workqueue("ab8500_fg_wq"); >> if (di->fg_wq == NULL) { >> dev_err(di->dev, "failed to create work queue\n"); >> - goto free_device_info; >> + return -ENOMEM; >> } >> >> /* Init work for running the fg algorithm instantly */ >> @@ -2605,12 +2630,14 @@ free_irq: >> } >> free_inst_curr_wq: >> destroy_workqueue(di->fg_wq); >> -free_device_info: >> - kfree(di); >> - >> return ret; >> } >> >> +static const struct of_device_id ab8500_fg_match[] = { >> + {.compatible = "stericsson,ab8500-fg",}, > > > > Spaces: > > { .compatible = "stericsson,ab8500-fg", }, > > > >> + {}, >> +}; >> + >> static struct platform_driver ab8500_fg_driver = { >> .probe = ab8500_fg_probe, >> .remove = __devexit_p(ab8500_fg_remove), >> @@ -2619,6 +2646,7 @@ static struct platform_driver ab8500_fg_driver = { >> .driver = { >> .name = "ab8500-fg", >> .owner = THIS_MODULE, >> + .of_match_table = ab8500_fg_match, >> }, >> }; >> >> diff --git a/drivers/power/abx500_chargalg.c b/drivers/power/abx500_chargalg.c >> index 804b88c..ba548e4 100644 >> --- a/drivers/power/abx500_chargalg.c >> +++ b/drivers/power/abx500_chargalg.c >> @@ -231,7 +231,7 @@ struct abx500_chargalg { >> struct abx500_chargalg_charger_info chg_info; >> struct abx500_chargalg_battery_data batt_data; >> struct abx500_chargalg_suspension_status susp_status; >> - struct abx500_chargalg_platform_data *pdata; >> + struct abx500_bmdevs_plat_data *pdata; >> struct abx500_bm_data *bat; >> struct power_supply chargalg_psy; >> struct ux500_charger *ac_chg; >> @@ -1814,7 +1814,7 @@ static int __devinit abx500_chargalg_probe(struct platform_device *pdev) >> di->dev = &pdev->dev; >> >> plat_data = pdev->dev.platform_data; >> - di->pdata = plat_data->chargalg; >> + di->pdata = plat_data->bmdev_pdata; >> di->bat = plat_data->battery; >> >> /* chargalg supply */ >> diff --git a/include/linux/mfd/abx500.h b/include/linux/mfd/abx500.h >> index 1318ca6..286f8ac 100644 >> --- a/include/linux/mfd/abx500.h >> +++ b/include/linux/mfd/abx500.h >> @@ -382,39 +382,30 @@ struct abx500_bm_data { >> int gnd_lift_resistance; >> const struct abx500_maxim_parameters *maxi; >> const struct abx500_bm_capacity_levels *cap_levels; >> - const struct abx500_battery_type *bat_type; >> + struct abx500_battery_type *bat_type; >> const struct abx500_bm_charger_parameters *chg_params; >> const struct abx500_fg_parameters *fg_params; >> }; >> >> -struct abx500_chargalg_platform_data { >> - char **supplied_to; >> - size_t num_supplicants; >> +struct abx500_bmdevs_plat_data { >> + char **supplied_to; >> + size_t num_supplicants; >> + bool autopower_cfg; >> }; >> >> -struct abx500_charger_platform_data { >> - char **supplied_to; >> - size_t num_supplicants; >> - bool autopower_cfg; >> -}; >> - >> -struct abx500_btemp_platform_data { >> - char **supplied_to; >> - size_t num_supplicants; >> +struct abx500_bm_plat_data { >> + struct abx500_bm_data *battery; >> + struct abx500_bmdevs_plat_data *bmdev_pdata; >> }; >> >> -struct abx500_fg_platform_data { >> - char **supplied_to; >> - size_t num_supplicants; >> +enum { >> + NTC_EXTERNAL = 0, >> + NTC_INTERNAL, >> }; >> >> -struct abx500_bm_plat_data { >> - struct abx500_bm_data *battery; >> - struct abx500_charger_platform_data *charger; >> - struct abx500_btemp_platform_data *btemp; >> - struct abx500_fg_platform_data *fg; >> - struct abx500_chargalg_platform_data *chargalg; >> -}; >> +int bmdevs_of_probe(struct device *dev, >> + struct device_node *np, >> + struct abx500_bm_plat_data *pdata); >> >> int abx500_set_register_interruptible(struct device *dev, u8 bank, u8 reg, >> u8 value); >> diff --git a/include/linux/mfd/abx500/ab8500-bm.h b/include/linux/mfd/abx500/ab8500-bm.h >> index 44310c9..d15b7f1 100644 >> --- a/include/linux/mfd/abx500/ab8500-bm.h >> +++ b/include/linux/mfd/abx500/ab8500-bm.h >> @@ -422,6 +422,13 @@ struct ab8500_chargalg_platform_data { >> struct ab8500_btemp; >> struct ab8500_gpadc; >> struct ab8500_fg; >> + >> +extern struct abx500_bm_data ab8500_bm_data; >> +extern struct abx500_battery_type bat_type_ext_thermistor[]; >> +extern struct batres_vs_temp temp_to_batres_tbl_ext_thermistor[]; >> +extern struct batres_vs_temp temp_to_batres_tbl_9100[]; >> +extern struct batres_vs_temp temp_to_batres_tbl_thermistor[]; >> + >> #ifdef CONFIG_AB8500_BM >> void ab8500_fg_reinit(void); >> void ab8500_charger_usb_state_changed(u8 bm_usb_state, u16 mA); >> -- >> 1.7.9.5 >> > > -- > Lee Jones > Linaro ST-Ericsson Landing Team Lead > Linaro.org │ Open source software for ARM SoCs > Follow Linaro: Facebook | Twitter | Blog >