Re: [PATCH 1/4] mfd: ab8500: add devicetree support for fuelgauge

From: Lee Jones
Date: Mon Oct 01 2012 - 05:49:39 EST

On Mon, 01 Oct 2012, Rajanikanth H.V wrote:

> From: "Rajanikanth H.V" <rajanikanth.hv@xxxxxxxxxxxxxx>
>
> - 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 <rajanikanth.hv@xxxxxxxxxxxxxx>
> ---
> 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 <linux/export.h>
> +#include <linux/power_supply.h>
> +#include <linux/of.h>
> +#include <linux/mfd/abx500.h>
> +#include <linux/mfd/abx500/ab8500.h>
> +#include <linux/mfd/abx500/ab8500-bm.h>
> +
> +/*
> + * 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;

<nit>

This spacing is uncharacteristic of Linux drivers.

Usually, struct declarations come first.

</nit>

> + /* 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));
> + }

<remove>

> + /* 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));

</remove>

... 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;

<remove>

> + dev_warn(dev, "missing property thermistor-on-batctrl\n");
> + thermistor = NTC_EXTERNAL;
> + }

</remove>

> + 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 <linux/platform_device.h>
> #include <linux/power_supply.h>
> #include <linux/kobject.h>
> -#include <linux/mfd/abx500/ab8500.h>
> -#include <linux/mfd/abx500.h>
> #include <linux/slab.h>
> -#include <linux/mfd/abx500/ab8500-bm.h>
> #include <linux/delay.h>
> -#include <linux/mfd/abx500/ab8500-gpadc.h>
> -#include <linux/mfd/abx500.h>
> #include <linux/time.h>
> #include <linux/completion.h>
> +#include <linux/mfd/abx500.h>
> +#include <linux/mfd/abx500/ab8500.h>
> +#include <linux/mfd/abx500/ab8500-bm.h>
> +#include <linux/mfd/abx500/ab8500-gpadc.h>
>
> #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) {
<snip>
} else {
<ERROR>
}
}

> + 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;
> + }
> + }

<remove>

> 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;
> }
</remove>

> - 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",},

<nit>

Spaces:

{ .compatible = "stericsson,ab8500-fg", },

</nit>

> + {},
> +};
> +
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/