2018-09-12 07:30:44

by Baolin Wang

[permalink] [raw]
Subject: [PATCH 1/2] dt-bindings: power: Add Spreadtrum SC27XX fuel gauge unit documentation

This patch adds the binding documentation for Spreadtrum SC27XX series PMICs
fuel gauge unit device, which is used to calculate the battery capacity.

Signed-off-by: Baolin Wang <[email protected]>
---
.../devicetree/bindings/power/supply/sc27xx-fg.txt | 55 ++++++++++++++++++++
1 file changed, 55 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt

diff --git a/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
new file mode 100644
index 0000000..7447bae
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
@@ -0,0 +1,55 @@
+Spreadtrum SC27XX PMICs Fuel Gauge Unit Power Supply Bindings
+
+Required properties:
+- compatible: Should be one of the following:
+ "sprd,sc2720-fgu",
+ "sprd,sc2721-fgu",
+ "sprd,sc2723-fgu",
+ "sprd,sc2730-fgu",
+ "sprd,sc2731-fgu".
+- reg: The address offset of fuel gauge unit.
+- bat-detect-gpio: GPIO for battery detection.
+- io-channels: Specify the IIO ADC channel to get temperature.
+- io-channel-names: Should be "bat-temp".
+- sprd,inner-resist: Specify the the battery inner resistance (mOhm).
+- sprd,ocv-cap-table: Provide the battery capacity percent with corresponding
+ open circuit voltage (ocv) of the battery, which is used to look up current
+ battery capacity according to current baterry ocv values.
+- monitored-battery: Phandle of battery characteristics devicetree node.
+ The fuel gauge uses the following battery properties:
+- charge-full-design-microamp-hours: battery design capacity.
+- constant-charge-voltage-max-microvolt: maximum constant input voltage.
+ See Documentation/devicetree/bindings/power/supply/battery.txt
+
+Example:
+
+ bat: battery {
+ compatible = "simple-battery";
+ charge-full-design-microamp-hours = <1900000>;
+ constant-charge-voltage-max-microvolt = <4350000>;
+ ......
+ };
+
+ sc2731_pmic: pmic@0 {
+ compatible = "sprd,sc2731";
+ reg = <0>;
+ spi-max-frequency = <26000000>;
+ interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ fgu@a00 {
+ compatible = "sprd,sc2731-fgu";
+ reg = <0xa00>;
+ bat-detect-gpio = <&pmic_eic 9 GPIO_ACTIVE_HIGH>;
+ io-channels = <&pmic_adc 5>;
+ io-channel-names = "bat-temp";
+ sprd,inner-resist = <250>;
+ sprd,ocv-cap-table = <4185 100>, <4113 95>, <4066 90>, <4022 85>, <3983 80>, <3949 75>, <3917 70>,
+ <3889 65>, <3864 60>, <3835 55>, <3805 50>, <3787 45>, <3777 40>, <3773 35>, <3770 30>,
+ <3765 25>, <3752 20>, <3724 15>, <3680 10>, <3605 5>, <3400 0>;
+ monitored-battery = <&bat>;
+ };
+ };
--
1.7.9.5



2018-09-12 07:30:45

by Baolin Wang

[permalink] [raw]
Subject: [PATCH 2/2] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

This patch adds the Spreadtrum SC27XX serial PMICs fuel gauge support,
which is used to calculate the battery capacity.

Original-by: Yuanjiang Yu <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/power/supply/Kconfig | 7 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/sc27xx_fuel_gauge.c | 705 ++++++++++++++++++++++++++++++
3 files changed, 713 insertions(+)
create mode 100644 drivers/power/supply/sc27xx_fuel_gauge.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index f27cf07..917f4b7 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -652,4 +652,11 @@ config CHARGER_SC2731
Say Y here to enable support for battery charging with SC2731
PMIC chips.

+config FUEL_GAUGE_SC27XX
+ tristate "Spreadtrum SC27XX fuel gauge driver"
+ depends on MFD_SC27XX_PMIC || COMPILE_TEST
+ help
+ Say Y here to enable support for fuel gauge with SC27XX
+ PMIC chips.
+
endif # POWER_SUPPLY
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 767105b..b731c2a 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -86,3 +86,4 @@ obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o
obj-$(CONFIG_CHARGER_CROS_USBPD) += cros_usbpd-charger.o
obj-$(CONFIG_CHARGER_SC2731) += sc2731_charger.o
+obj-$(CONFIG_FUEL_GAUGE_SC27XX) += sc27xx_fuel_gauge.o
diff --git a/drivers/power/supply/sc27xx_fuel_gauge.c b/drivers/power/supply/sc27xx_fuel_gauge.c
new file mode 100644
index 0000000..4df0f47
--- /dev/null
+++ b/drivers/power/supply/sc27xx_fuel_gauge.c
@@ -0,0 +1,705 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Spreadtrum Communications Inc.
+
+#include <linux/gpio/consumer.h>
+#include <linux/iio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/regmap.h>
+
+/* PMIC global control registers definition */
+#define SC27XX_MODULE_EN0 0xc08
+#define SC27XX_CLK_EN0 0xc18
+#define SC27XX_FGU_EN BIT(7)
+#define SC27XX_FGU_RTC_EN BIT(6)
+
+/* FGU registers definition */
+#define SC27XX_FGU_START 0x0
+#define SC27XX_FGU_CONFIG 0x4
+#define SC27XX_FGU_ADC_CONFIG 0x8
+#define SC27XX_FGU_STATUS 0xc
+#define SC27XX_FGU_INT_EN 0x10
+#define SC27XX_FGU_INT_CLR 0x14
+#define SC27XX_FGU_INT_STS 0x1c
+#define SC27XX_FGU_VOLTAGE 0x20
+#define SC27XX_FGU_OCV 0x24
+#define SC27XX_FGU_POCV 0x28
+#define SC27XX_FGU_CURRENT 0x2c
+#define SC27XX_FGU_CLBCNT_SETH 0x50
+#define SC27XX_FGU_CLBCNT_SETL 0x54
+#define SC27XX_FGU_CLBCNT_VALH 0x68
+#define SC27XX_FGU_CLBCNT_VALL 0x6c
+#define SC27XX_FGU_CLBCNT_QMAXL 0x74
+
+#define SC27XX_WRITE_SELCLB_EN BIT(0)
+#define SC27XX_FGU_CLBCNT_MASK GENMASK(15, 0)
+#define SC27XX_FGU_CLBCNT_SHIFT 16
+
+#define SC27XX_FGU_1000MV_ADC 686
+#define SC27XX_FGU_1000MA_ADC 1372
+#define SC27XX_FGU_CUR_BASIC_ADC 8192
+#define SC27XX_FGU_SAMPLE_HZ 2
+
+struct sc27xx_fgu_capacity_table {
+ int ocv;
+ int capacity;
+};
+
+/*
+ * struct sc27xx_fgu_data: describe the FGU device
+ * @regmap: regmap for register access
+ * @dev: platform device
+ * @battery: battery power supply
+ * @base: the base offset for the controller
+ * @lock: protect the structure
+ * @gpiod: GPIO for battery detection
+ * @channel: IIO channel to get battery temperature
+ * @inner_resist: the battery inner resistance in mOhm
+ * @total_cap: the total capacity of the battery in mAh
+ * @init_cap: the initial capacity of the battery in mAh
+ * @init_clbcnt: the initial coulomb counter
+ * @max_volt: the maximum constant input voltage in millivolt
+ * @table_len: the capacity table length
+ * @cap_table: capacity table with corresponding ocv
+ */
+struct sc27xx_fgu_data {
+ struct regmap *regmap;
+ struct device *dev;
+ struct power_supply *battery;
+ u32 base;
+ struct mutex lock;
+ struct gpio_desc *gpiod;
+ struct iio_channel *channel;
+ bool bat_present;
+ int inner_resist;
+ int total_cap;
+ int init_cap;
+ int init_clbcnt;
+ int max_volt;
+ int table_len;
+ struct sc27xx_fgu_capacity_table *cap_table;
+};
+
+static const char * const sc27xx_charger_supply_name[] = {
+ "sc2731_charger",
+ "sc2720_charger",
+ "sc2721_charger",
+ "sc2723_charger",
+};
+
+static int sc27xx_fgu_adc_to_current(int adc)
+{
+ return (adc * 1000) / SC27XX_FGU_1000MA_ADC;
+}
+
+static int sc27xx_fgu_adc_to_voltage(int adc)
+{
+ return (adc * 1000) / SC27XX_FGU_1000MV_ADC;
+}
+
+static int sc27xx_fgu_ocv_to_capacity(struct sc27xx_fgu_data *data, int ocv)
+{
+ struct sc27xx_fgu_capacity_table *tab = data->cap_table;
+ int n = data->table_len;
+ int i, cap, tmp;
+
+ /*
+ * Find the position in the table for current battery OCV value,
+ * then use these two points to calculate battery capacity
+ * according to the linear method.
+ */
+ for (i = 0; i < n; i++)
+ if (ocv > tab[i].ocv)
+ break;
+
+ if (i > 0 && i < n) {
+ tmp = (tab[i - 1].capacity - tab[i].capacity) *
+ (ocv - tab[i].ocv) * 2;
+ tmp /= tab[i - 1].ocv - tab[i].ocv;
+ tmp = (tmp + 1) / 2;
+ cap = tmp + tab[i].capacity;
+ } else if (i == 0) {
+ cap = tab[0].capacity;
+ } else {
+ cap = tab[n - 1].capacity;
+ }
+
+ return cap;
+}
+
+/*
+ * When system boots on, we can not read battery capacity from coulomb
+ * registers, since now the coulomb registers are invalid. So we should
+ * calculate the battery open circuit voltage, and get current battery
+ * capacity according to the capacity table.
+ */
+static int sc27xx_fgu_get_boot_capacity(struct sc27xx_fgu_data *data, int *cap)
+{
+ int volt, cur, oci, ocv, ret;
+
+ /*
+ * After system booting on, the SC27XX_FGU_CLBCNT_QMAXL register saved
+ * the first sampled open circuit current.
+ */
+ ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_QMAXL,
+ &cur);
+ if (ret)
+ return ret;
+
+ cur <<= 1;
+ oci = sc27xx_fgu_adc_to_current(cur - SC27XX_FGU_CUR_BASIC_ADC);
+
+ /*
+ * Should get the OCV from SC27XX_FGU_POCV register at the system
+ * beginning. It is ADC values reading from registers which need to
+ * convert the corresponding voltage.
+ */
+ ret = regmap_read(data->regmap, data->base + SC27XX_FGU_POCV, &volt);
+ if (ret)
+ return ret;
+
+ volt = sc27xx_fgu_adc_to_voltage(volt);
+ ocv = volt - (oci * data->inner_resist) / 1000;
+
+ /*
+ * Parse the capacity table to look up the correct capacity percent
+ * according to current battery's corresponding OCV values.
+ */
+ *cap = sc27xx_fgu_ocv_to_capacity(data, ocv);
+
+ return 0;
+}
+
+static int sc27xx_fgu_set_clbcnt(struct sc27xx_fgu_data *data, int clbcnt)
+{
+ int ret;
+
+ clbcnt *= SC27XX_FGU_SAMPLE_HZ;
+
+ ret = regmap_update_bits(data->regmap,
+ data->base + SC27XX_FGU_CLBCNT_SETL,
+ SC27XX_FGU_CLBCNT_MASK, clbcnt);
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(data->regmap,
+ data->base + SC27XX_FGU_CLBCNT_SETH,
+ SC27XX_FGU_CLBCNT_MASK,
+ clbcnt >> SC27XX_FGU_CLBCNT_SHIFT);
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(data->regmap, data->base + SC27XX_FGU_START,
+ SC27XX_WRITE_SELCLB_EN,
+ SC27XX_WRITE_SELCLB_EN);
+}
+
+static int sc27xx_fgu_get_clbcnt(struct sc27xx_fgu_data *data, int *clb_cnt)
+{
+ int ccl, cch, ret;
+
+ ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_VALL,
+ &ccl);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_VALH,
+ &cch);
+ if (ret)
+ return ret;
+
+ *clb_cnt = ccl & SC27XX_FGU_CLBCNT_MASK;
+ *clb_cnt |= (cch & SC27XX_FGU_CLBCNT_MASK) << SC27XX_FGU_CLBCNT_SHIFT;
+ *clb_cnt /= SC27XX_FGU_SAMPLE_HZ;
+
+ return 0;
+}
+
+static int sc27xx_fgu_get_capacity(struct sc27xx_fgu_data *data, int *cap)
+{
+ int ret, cur_clbcnt, delta_clbcnt, delta_cap, temp;
+
+ /* Get current coulomb counters firstly */
+ ret = sc27xx_fgu_get_clbcnt(data, &cur_clbcnt);
+ if (ret)
+ return ret;
+
+ delta_clbcnt = cur_clbcnt - data->init_clbcnt;
+
+ /*
+ * Convert coulomb counter to delta capacity (mAh), and set multiplier
+ * as 100 to improve the precision.
+ */
+ temp = DIV_ROUND_CLOSEST(delta_clbcnt, 360);
+ temp = sc27xx_fgu_adc_to_current(temp);
+
+ /*
+ * Convert to capacity percent of the battery total capacity,
+ * and multiplier is 100 too.
+ */
+ delta_cap = DIV_ROUND_CLOSEST(temp * 100, data->total_cap);
+ *cap = delta_cap + data->init_cap;
+
+ return 0;
+}
+
+static int sc27xx_fgu_get_vbat_vol(struct sc27xx_fgu_data *data, int *val)
+{
+ int ret, vol;
+
+ ret = regmap_read(data->regmap, data->base + SC27XX_FGU_VOLTAGE, &vol);
+ if (ret)
+ return ret;
+
+ /*
+ * It is ADC values reading from registers which need to convert to
+ * corresponding voltage values.
+ */
+ *val = sc27xx_fgu_adc_to_voltage(vol);
+
+ return 0;
+}
+
+static int sc27xx_fgu_get_current(struct sc27xx_fgu_data *data, int *val)
+{
+ int ret, cur;
+
+ ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CURRENT, &cur);
+ if (ret)
+ return ret;
+
+ /*
+ * It is ADC values reading from registers which need to convert to
+ * corresponding current values.
+ */
+ *val = sc27xx_fgu_adc_to_current(cur - SC27XX_FGU_CUR_BASIC_ADC);
+
+ return 0;
+}
+
+static int sc27xx_fgu_get_vbat_ocv(struct sc27xx_fgu_data *data, int *val)
+{
+ int vol, cur, ret;
+
+ ret = sc27xx_fgu_get_vbat_vol(data, &vol);
+ if (ret)
+ return ret;
+
+ ret = sc27xx_fgu_get_current(data, &cur);
+ if (ret)
+ return ret;
+
+ *val = vol - (cur * data->inner_resist) / 1000;
+
+ return 0;
+}
+
+static int sc27xx_fgu_get_temp(struct sc27xx_fgu_data *data, int *temp)
+{
+ return iio_read_channel_processed(data->channel, temp);
+}
+
+static int sc27xx_fgu_get_health(struct sc27xx_fgu_data *data, int *health)
+{
+ int ret, vol;
+
+ ret = sc27xx_fgu_get_vbat_vol(data, &vol);
+ if (ret)
+ return ret;
+
+ if (vol > data->max_volt)
+ *health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+ else
+ *health = POWER_SUPPLY_HEALTH_GOOD;
+
+ return 0;
+}
+
+static int sc27xx_fgu_get_status(struct sc27xx_fgu_data *data, int *status)
+{
+ union power_supply_propval val;
+ struct power_supply *psy;
+ int i, ret = -EINVAL;
+
+ for (i = 0; i < ARRAY_SIZE(sc27xx_charger_supply_name); i++) {
+ psy = power_supply_get_by_name(sc27xx_charger_supply_name[i]);
+ if (!psy)
+ continue;
+
+ ret = power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS,
+ &val);
+ power_supply_put(psy);
+ if (ret)
+ return ret;
+
+ *status = val.intval;
+ }
+
+ return ret;
+}
+
+static int sc27xx_fgu_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct sc27xx_fgu_data *data = power_supply_get_drvdata(psy);
+ int ret = 0;
+ int value;
+
+ mutex_lock(&data->lock);
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_STATUS:
+ ret = sc27xx_fgu_get_status(data, &value);
+ if (ret)
+ goto error;
+
+ val->intval = value;
+ break;
+
+ case POWER_SUPPLY_PROP_HEALTH:
+ ret = sc27xx_fgu_get_health(data, &value);
+ if (ret)
+ goto error;
+
+ val->intval = value;
+ break;
+
+ case POWER_SUPPLY_PROP_PRESENT:
+ val->intval = data->bat_present;
+ break;
+
+ case POWER_SUPPLY_PROP_TEMP:
+ ret = sc27xx_fgu_get_temp(data, &value);
+ if (ret)
+ goto error;
+
+ val->intval = value;
+ break;
+
+ case POWER_SUPPLY_PROP_TECHNOLOGY:
+ val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
+ break;
+
+ case POWER_SUPPLY_PROP_CAPACITY:
+ ret = sc27xx_fgu_get_capacity(data, &value);
+ if (ret)
+ goto error;
+
+ val->intval = value;
+ break;
+
+ case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ ret = sc27xx_fgu_get_vbat_vol(data, &value);
+ if (ret)
+ goto error;
+
+ val->intval = value * 1000;
+ break;
+
+ case POWER_SUPPLY_PROP_VOLTAGE_OCV:
+ ret = sc27xx_fgu_get_vbat_ocv(data, &value);
+ if (ret)
+ goto error;
+
+ val->intval = value * 1000;
+ break;
+
+ case POWER_SUPPLY_PROP_CURRENT_NOW:
+ case POWER_SUPPLY_PROP_CURRENT_AVG:
+ ret = sc27xx_fgu_get_current(data, &value);
+ if (ret)
+ goto error;
+
+ val->intval = value * 1000;
+ break;
+
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+error:
+ mutex_unlock(&data->lock);
+ return ret;
+}
+
+static void sc27xx_fgu_external_power_changed(struct power_supply *psy)
+{
+ struct sc27xx_fgu_data *data = power_supply_get_drvdata(psy);
+
+ power_supply_changed(data->battery);
+}
+
+static enum power_supply_property sc27xx_fgu_props[] = {
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_HEALTH,
+ POWER_SUPPLY_PROP_PRESENT,
+ POWER_SUPPLY_PROP_TEMP,
+ POWER_SUPPLY_PROP_TECHNOLOGY,
+ POWER_SUPPLY_PROP_CAPACITY,
+ POWER_SUPPLY_PROP_VOLTAGE_NOW,
+ POWER_SUPPLY_PROP_VOLTAGE_OCV,
+ POWER_SUPPLY_PROP_CURRENT_NOW,
+ POWER_SUPPLY_PROP_CURRENT_AVG,
+};
+
+static const struct power_supply_desc sc27xx_fgu_desc = {
+ .name = "sc27xx-fgu",
+ .type = POWER_SUPPLY_TYPE_BATTERY,
+ .properties = sc27xx_fgu_props,
+ .num_properties = ARRAY_SIZE(sc27xx_fgu_props),
+ .get_property = sc27xx_fgu_get_property,
+ .external_power_changed = sc27xx_fgu_external_power_changed,
+};
+
+static irqreturn_t sc27xx_fgu_bat_detection(int irq, void *dev_id)
+{
+ struct sc27xx_fgu_data *data = dev_id;
+ int state;
+
+ mutex_lock(&data->lock);
+
+ state = gpiod_get_value_cansleep(data->gpiod);
+ if (state < 0) {
+ dev_err(data->dev, "failed to get gpio state\n");
+ mutex_unlock(&data->lock);
+ return IRQ_RETVAL(state);
+ }
+
+ if (state)
+ data->bat_present = true;
+ else
+ data->bat_present = false;
+
+ mutex_unlock(&data->lock);
+
+ return IRQ_HANDLED;
+}
+
+static void sc27xx_fgu_disable(void *_data)
+{
+ struct sc27xx_fgu_data *data = _data;
+
+ regmap_update_bits(data->regmap, SC27XX_CLK_EN0, SC27XX_FGU_RTC_EN, 0);
+ regmap_update_bits(data->regmap, SC27XX_MODULE_EN0, SC27XX_FGU_EN, 0);
+}
+
+static int sc27xx_fgu_cap_to_clbcnt(struct sc27xx_fgu_data *data, int capacity)
+{
+ /*
+ * Get current capacity (mAh) = battery total capacity (mAh) *
+ * current capacity percent (capacity / 100).
+ */
+ int cur_cap = DIV_ROUND_CLOSEST(data->total_cap * capacity, 100);
+
+ /*
+ * Convert current capacity (mAh) to coulomb counter according to the
+ * formula: 1 mAh =3.6 coulomb.
+ */
+ return DIV_ROUND_CLOSEST(cur_cap * 36, 10);
+}
+
+static int sc27xx_fgu_hw_init(struct sc27xx_fgu_data *data)
+{
+ struct power_supply_battery_info info = { };
+ int ret;
+
+ ret = power_supply_get_battery_info(data->battery, &info);
+ if (ret) {
+ dev_err(data->dev, "failed to get battery information\n");
+ return ret;
+ }
+
+ data->total_cap = info.charge_full_design_uah / 1000;
+ data->max_volt = info.constant_charge_voltage_max_uv / 1000;
+
+ /* Enable the FGU module */
+ ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN0,
+ SC27XX_FGU_EN, SC27XX_FGU_EN);
+ if (ret) {
+ dev_err(data->dev, "failed to enable fgu\n");
+ return ret;
+ }
+
+ /* Enable the FGU RTC clock to make it work */
+ ret = regmap_update_bits(data->regmap, SC27XX_CLK_EN0,
+ SC27XX_FGU_RTC_EN, SC27XX_FGU_RTC_EN);
+ if (ret) {
+ dev_err(data->dev, "failed to enable fgu RTC clock\n");
+ goto disable_fgu;
+ }
+
+ /*
+ * Get the boot battery capacity when system powers on, which is used to
+ * initialize the coulomb counter. After that, we can read the coulomb
+ * counter to measure the battery capacity.
+ */
+ ret = sc27xx_fgu_get_boot_capacity(data, &data->init_cap);
+ if (ret) {
+ dev_err(data->dev, "failed to get boot capacity\n");
+ goto disable_clk;
+ }
+
+ /*
+ * Convert battery capacity to the corresponding initial coulomb counter
+ * and set into coulomb counter registers.
+ */
+ data->init_clbcnt = sc27xx_fgu_cap_to_clbcnt(data, data->init_cap);
+ ret = sc27xx_fgu_set_clbcnt(data, data->init_clbcnt);
+ if (ret) {
+ dev_err(data->dev, "failed to initialize coulomb counter\n");
+ goto disable_clk;
+ }
+
+ return 0;
+
+disable_clk:
+ regmap_update_bits(data->regmap, SC27XX_CLK_EN0, SC27XX_FGU_RTC_EN, 0);
+disable_fgu:
+ regmap_update_bits(data->regmap, SC27XX_MODULE_EN0, SC27XX_FGU_EN, 0);
+
+ return ret;
+}
+
+static int sc27xx_fgu_parse_dt(struct sc27xx_fgu_data *data,
+ struct device_node *np)
+{
+ const __be32 *list;
+ int i, len, size, ret;
+
+ ret = of_property_read_u32(np, "reg", &data->base);
+ if (ret) {
+ dev_err(data->dev, "failed to get fgu address\n");
+ return ret;
+ }
+
+ data->gpiod = devm_gpiod_get_optional(data->dev, "bat-detect", GPIOD_IN);
+ if (IS_ERR(data->gpiod)) {
+ dev_err(data->dev, "failed to get battery detection GPIO\n");
+ return PTR_ERR(data->gpiod);
+ }
+
+ data->channel = devm_iio_channel_get(data->dev, "bat-temp");
+ if (IS_ERR(data->channel)) {
+ dev_err(data->dev, "failed to get IIO channel\n");
+ return PTR_ERR(data->channel);
+ }
+
+ /* Get the battery inner resistance */
+ ret = of_property_read_u32(np, "sprd,inner-resist", &data->inner_resist);
+ if (ret) {
+ dev_err(data->dev, "failed to get battery inner resistance\n");
+ return ret;
+ }
+
+ /* Get battery ocv-capacity table */
+ list = of_get_property(np, "sprd,ocv-cap-table", &size);
+ if (!list || !size) {
+ dev_err(data->dev, "failed to get ocv-capacity table\n");
+ return -EINVAL;
+ }
+
+ len = size / 8;
+ data->table_len = len;
+ data->cap_table = devm_kzalloc(data->dev,
+ sizeof(struct sc27xx_fgu_capacity_table) * len,
+ GFP_KERNEL);
+ if (!data->cap_table)
+ return -ENOMEM;
+
+ for (i = 0; i < len; i++) {
+ data->cap_table[i].ocv = be32_to_cpu(*list++);
+ data->cap_table[i].capacity = be32_to_cpu(*list++);
+ }
+
+ return 0;
+}
+
+static int sc27xx_fgu_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct power_supply_config fgu_cfg = { };
+ struct sc27xx_fgu_data *data;
+ int ret, irq;
+
+ data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!data->regmap) {
+ dev_err(&pdev->dev, "failed to get regmap\n");
+ return -ENODEV;
+ }
+
+ mutex_init(&data->lock);
+ data->dev = &pdev->dev;
+ data->bat_present = true;
+
+ ret = sc27xx_fgu_parse_dt(data, np);
+ if (ret)
+ return ret;
+
+ fgu_cfg.drv_data = data;
+ fgu_cfg.of_node = np;
+ data->battery = devm_power_supply_register(&pdev->dev, &sc27xx_fgu_desc,
+ &fgu_cfg);
+ if (IS_ERR(data->battery)) {
+ dev_err(&pdev->dev, "failed to register power supply\n");
+ return PTR_ERR(data->battery);
+ }
+
+ ret = sc27xx_fgu_hw_init(data);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to initialize fgu hardware\n");
+ return ret;
+ }
+
+ ret = devm_add_action(&pdev->dev, sc27xx_fgu_disable, data);
+ if (ret) {
+ sc27xx_fgu_disable(data);
+ dev_err(&pdev->dev, "failed to add fgu disable action\n");
+ return ret;
+ }
+
+ irq = gpiod_to_irq(data->gpiod);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "failed to translate GPIO to IRQ\n");
+ return irq;
+ }
+
+ ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+ sc27xx_fgu_bat_detection,
+ IRQF_ONESHOT | IRQF_TRIGGER_RISING |
+ IRQF_TRIGGER_FALLING,
+ pdev->name, data);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to request IRQ\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id sc27xx_fgu_of_match[] = {
+ { .compatible = "sprd,sc2731-fgu", },
+ { }
+};
+
+static struct platform_driver sc27xx_fgu_driver = {
+ .probe = sc27xx_fgu_probe,
+ .driver = {
+ .name = "sc27xx-fgu",
+ .of_match_table = sc27xx_fgu_of_match,
+ }
+};
+
+module_platform_driver(sc27xx_fgu_driver);
+
+MODULE_DESCRIPTION("Spreadtrum SC27XX PMICs Fual Gauge Unit Driver");
+MODULE_LICENSE("GPL v2");
--
1.7.9.5


2018-09-16 20:04:25

by Sebastian Reichel

[permalink] [raw]
Subject: Re: [PATCH 2/2] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

Hi,

Looks mostly good. I have a couple of comments in addition to the
ones from the binding about using battery_info for the OCV ->
capacity mapping.

On Wed, Sep 12, 2018 at 03:29:39PM +0800, Baolin Wang wrote:
> This patch adds the Spreadtrum SC27XX serial PMICs fuel gauge support,
> which is used to calculate the battery capacity.
>
> Original-by: Yuanjiang Yu <[email protected]>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> drivers/power/supply/Kconfig | 7 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/sc27xx_fuel_gauge.c | 705 ++++++++++++++++++++++++++++++
> 3 files changed, 713 insertions(+)
> create mode 100644 drivers/power/supply/sc27xx_fuel_gauge.c
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index f27cf07..917f4b7 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -652,4 +652,11 @@ config CHARGER_SC2731
> Say Y here to enable support for battery charging with SC2731
> PMIC chips.
>
> +config FUEL_GAUGE_SC27XX
> + tristate "Spreadtrum SC27XX fuel gauge driver"
> + depends on MFD_SC27XX_PMIC || COMPILE_TEST
> + help
> + Say Y here to enable support for fuel gauge with SC27XX
> + PMIC chips.
> +
> endif # POWER_SUPPLY
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 767105b..b731c2a 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -86,3 +86,4 @@ obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
> obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o
> obj-$(CONFIG_CHARGER_CROS_USBPD) += cros_usbpd-charger.o
> obj-$(CONFIG_CHARGER_SC2731) += sc2731_charger.o
> +obj-$(CONFIG_FUEL_GAUGE_SC27XX) += sc27xx_fuel_gauge.o
> diff --git a/drivers/power/supply/sc27xx_fuel_gauge.c b/drivers/power/supply/sc27xx_fuel_gauge.c
> new file mode 100644
> index 0000000..4df0f47
> --- /dev/null
> +++ b/drivers/power/supply/sc27xx_fuel_gauge.c
> @@ -0,0 +1,705 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 Spreadtrum Communications Inc.
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +
> +/* PMIC global control registers definition */
> +#define SC27XX_MODULE_EN0 0xc08
> +#define SC27XX_CLK_EN0 0xc18
> +#define SC27XX_FGU_EN BIT(7)
> +#define SC27XX_FGU_RTC_EN BIT(6)
> +
> +/* FGU registers definition */
> +#define SC27XX_FGU_START 0x0
> +#define SC27XX_FGU_CONFIG 0x4
> +#define SC27XX_FGU_ADC_CONFIG 0x8
> +#define SC27XX_FGU_STATUS 0xc
> +#define SC27XX_FGU_INT_EN 0x10
> +#define SC27XX_FGU_INT_CLR 0x14
> +#define SC27XX_FGU_INT_STS 0x1c
> +#define SC27XX_FGU_VOLTAGE 0x20
> +#define SC27XX_FGU_OCV 0x24
> +#define SC27XX_FGU_POCV 0x28
> +#define SC27XX_FGU_CURRENT 0x2c
> +#define SC27XX_FGU_CLBCNT_SETH 0x50
> +#define SC27XX_FGU_CLBCNT_SETL 0x54
> +#define SC27XX_FGU_CLBCNT_VALH 0x68
> +#define SC27XX_FGU_CLBCNT_VALL 0x6c
> +#define SC27XX_FGU_CLBCNT_QMAXL 0x74
> +
> +#define SC27XX_WRITE_SELCLB_EN BIT(0)
> +#define SC27XX_FGU_CLBCNT_MASK GENMASK(15, 0)
> +#define SC27XX_FGU_CLBCNT_SHIFT 16
> +
> +#define SC27XX_FGU_1000MV_ADC 686
> +#define SC27XX_FGU_1000MA_ADC 1372
> +#define SC27XX_FGU_CUR_BASIC_ADC 8192
> +#define SC27XX_FGU_SAMPLE_HZ 2
> +
> +struct sc27xx_fgu_capacity_table {
> + int ocv;
> + int capacity;
> +};
> +
> +/*
> + * struct sc27xx_fgu_data: describe the FGU device
> + * @regmap: regmap for register access
> + * @dev: platform device
> + * @battery: battery power supply
> + * @base: the base offset for the controller
> + * @lock: protect the structure
> + * @gpiod: GPIO for battery detection
> + * @channel: IIO channel to get battery temperature
> + * @inner_resist: the battery inner resistance in mOhm
> + * @total_cap: the total capacity of the battery in mAh
> + * @init_cap: the initial capacity of the battery in mAh
> + * @init_clbcnt: the initial coulomb counter
> + * @max_volt: the maximum constant input voltage in millivolt
> + * @table_len: the capacity table length
> + * @cap_table: capacity table with corresponding ocv
> + */
> +struct sc27xx_fgu_data {
> + struct regmap *regmap;
> + struct device *dev;
> + struct power_supply *battery;
> + u32 base;
> + struct mutex lock;
> + struct gpio_desc *gpiod;
> + struct iio_channel *channel;
> + bool bat_present;
> + int inner_resist;
> + int total_cap;
> + int init_cap;
> + int init_clbcnt;
> + int max_volt;
> + int table_len;
> + struct sc27xx_fgu_capacity_table *cap_table;
> +};
> +
> +static const char * const sc27xx_charger_supply_name[] = {
> + "sc2731_charger",
> + "sc2720_charger",
> + "sc2721_charger",
> + "sc2723_charger",
> +};
> +
> +static int sc27xx_fgu_adc_to_current(int adc)
> +{
> + return (adc * 1000) / SC27XX_FGU_1000MA_ADC;
> +}
> +
> +static int sc27xx_fgu_adc_to_voltage(int adc)
> +{
> + return (adc * 1000) / SC27XX_FGU_1000MV_ADC;
> +}
> +
> +static int sc27xx_fgu_ocv_to_capacity(struct sc27xx_fgu_data *data, int ocv)
> +{
> + struct sc27xx_fgu_capacity_table *tab = data->cap_table;
> + int n = data->table_len;
> + int i, cap, tmp;
> +
> + /*
> + * Find the position in the table for current battery OCV value,
> + * then use these two points to calculate battery capacity
> + * according to the linear method.
> + */
> + for (i = 0; i < n; i++)
> + if (ocv > tab[i].ocv)
> + break;
> +
> + if (i > 0 && i < n) {
> + tmp = (tab[i - 1].capacity - tab[i].capacity) *
> + (ocv - tab[i].ocv) * 2;
> + tmp /= tab[i - 1].ocv - tab[i].ocv;
> + tmp = (tmp + 1) / 2;
> + cap = tmp + tab[i].capacity;
> + } else if (i == 0) {
> + cap = tab[0].capacity;
> + } else {
> + cap = tab[n - 1].capacity;
> + }
> +
> + return cap;
> +}
> +
> +/*
> + * When system boots on, we can not read battery capacity from coulomb
> + * registers, since now the coulomb registers are invalid. So we should
> + * calculate the battery open circuit voltage, and get current battery
> + * capacity according to the capacity table.
> + */
> +static int sc27xx_fgu_get_boot_capacity(struct sc27xx_fgu_data *data, int *cap)
> +{
> + int volt, cur, oci, ocv, ret;
> +
> + /*
> + * After system booting on, the SC27XX_FGU_CLBCNT_QMAXL register saved
> + * the first sampled open circuit current.
> + */
> + ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_QMAXL,
> + &cur);
> + if (ret)
> + return ret;
> +
> + cur <<= 1;
> + oci = sc27xx_fgu_adc_to_current(cur - SC27XX_FGU_CUR_BASIC_ADC);
> +
> + /*
> + * Should get the OCV from SC27XX_FGU_POCV register at the system
> + * beginning. It is ADC values reading from registers which need to
> + * convert the corresponding voltage.
> + */
> + ret = regmap_read(data->regmap, data->base + SC27XX_FGU_POCV, &volt);
> + if (ret)
> + return ret;
> +
> + volt = sc27xx_fgu_adc_to_voltage(volt);
> + ocv = volt - (oci * data->inner_resist) / 1000;
> +
> + /*
> + * Parse the capacity table to look up the correct capacity percent
> + * according to current battery's corresponding OCV values.
> + */
> + *cap = sc27xx_fgu_ocv_to_capacity(data, ocv);
> +
> + return 0;
> +}
> +
> +static int sc27xx_fgu_set_clbcnt(struct sc27xx_fgu_data *data, int clbcnt)
> +{
> + int ret;
> +
> + clbcnt *= SC27XX_FGU_SAMPLE_HZ;
> +
> + ret = regmap_update_bits(data->regmap,
> + data->base + SC27XX_FGU_CLBCNT_SETL,
> + SC27XX_FGU_CLBCNT_MASK, clbcnt);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(data->regmap,
> + data->base + SC27XX_FGU_CLBCNT_SETH,
> + SC27XX_FGU_CLBCNT_MASK,
> + clbcnt >> SC27XX_FGU_CLBCNT_SHIFT);
> + if (ret)
> + return ret;
> +
> + return regmap_update_bits(data->regmap, data->base + SC27XX_FGU_START,
> + SC27XX_WRITE_SELCLB_EN,
> + SC27XX_WRITE_SELCLB_EN);
> +}
> +
> +static int sc27xx_fgu_get_clbcnt(struct sc27xx_fgu_data *data, int *clb_cnt)
> +{
> + int ccl, cch, ret;
> +
> + ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_VALL,
> + &ccl);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_VALH,
> + &cch);
> + if (ret)
> + return ret;
> +
> + *clb_cnt = ccl & SC27XX_FGU_CLBCNT_MASK;
> + *clb_cnt |= (cch & SC27XX_FGU_CLBCNT_MASK) << SC27XX_FGU_CLBCNT_SHIFT;
> + *clb_cnt /= SC27XX_FGU_SAMPLE_HZ;
> +
> + return 0;
> +}
> +
> +static int sc27xx_fgu_get_capacity(struct sc27xx_fgu_data *data, int *cap)
> +{
> + int ret, cur_clbcnt, delta_clbcnt, delta_cap, temp;
> +
> + /* Get current coulomb counters firstly */
> + ret = sc27xx_fgu_get_clbcnt(data, &cur_clbcnt);
> + if (ret)
> + return ret;
> +
> + delta_clbcnt = cur_clbcnt - data->init_clbcnt;
> +
> + /*
> + * Convert coulomb counter to delta capacity (mAh), and set multiplier
> + * as 100 to improve the precision.
> + */
> + temp = DIV_ROUND_CLOSEST(delta_clbcnt, 360);
> + temp = sc27xx_fgu_adc_to_current(temp);
> +
> + /*
> + * Convert to capacity percent of the battery total capacity,
> + * and multiplier is 100 too.
> + */
> + delta_cap = DIV_ROUND_CLOSEST(temp * 100, data->total_cap);
> + *cap = delta_cap + data->init_cap;
> +
> + return 0;
> +}
> +
> +static int sc27xx_fgu_get_vbat_vol(struct sc27xx_fgu_data *data, int *val)
> +{
> + int ret, vol;
> +
> + ret = regmap_read(data->regmap, data->base + SC27XX_FGU_VOLTAGE, &vol);
> + if (ret)
> + return ret;
> +
> + /*
> + * It is ADC values reading from registers which need to convert to
> + * corresponding voltage values.
> + */
> + *val = sc27xx_fgu_adc_to_voltage(vol);
> +
> + return 0;
> +}
> +
> +static int sc27xx_fgu_get_current(struct sc27xx_fgu_data *data, int *val)
> +{
> + int ret, cur;
> +
> + ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CURRENT, &cur);
> + if (ret)
> + return ret;
> +
> + /*
> + * It is ADC values reading from registers which need to convert to
> + * corresponding current values.
> + */
> + *val = sc27xx_fgu_adc_to_current(cur - SC27XX_FGU_CUR_BASIC_ADC);
> +
> + return 0;
> +}
> +
> +static int sc27xx_fgu_get_vbat_ocv(struct sc27xx_fgu_data *data, int *val)
> +{
> + int vol, cur, ret;
> +
> + ret = sc27xx_fgu_get_vbat_vol(data, &vol);
> + if (ret)
> + return ret;
> +
> + ret = sc27xx_fgu_get_current(data, &cur);
> + if (ret)
> + return ret;
> +
> + *val = vol - (cur * data->inner_resist) / 1000;

You multiply this with 1000 directly after this function to get back
to uV. Just drop the division and the multiplication.

> +
> + return 0;
> +}
> +
> +static int sc27xx_fgu_get_temp(struct sc27xx_fgu_data *data, int *temp)
> +{
> + return iio_read_channel_processed(data->channel, temp);
> +}
> +
> +static int sc27xx_fgu_get_health(struct sc27xx_fgu_data *data, int *health)
> +{
> + int ret, vol;
> +
> + ret = sc27xx_fgu_get_vbat_vol(data, &vol);
> + if (ret)
> + return ret;
> +
> + if (vol > data->max_volt)
> + *health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> + else
> + *health = POWER_SUPPLY_HEALTH_GOOD;
> +
> + return 0;
> +}
> +
> +static int sc27xx_fgu_get_status(struct sc27xx_fgu_data *data, int *status)
> +{
> + union power_supply_propval val;
> + struct power_supply *psy;
> + int i, ret = -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(sc27xx_charger_supply_name); i++) {
> + psy = power_supply_get_by_name(sc27xx_charger_supply_name[i]);
> + if (!psy)
> + continue;
> +
> + ret = power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS,
> + &val);
> + power_supply_put(psy);
> + if (ret)
> + return ret;
> +
> + *status = val.intval;
> + }
> +
> + return ret;
> +}
> +
> +static int sc27xx_fgu_get_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct sc27xx_fgu_data *data = power_supply_get_drvdata(psy);
> + int ret = 0;
> + int value;
> +
> + mutex_lock(&data->lock);
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_STATUS:
> + ret = sc27xx_fgu_get_status(data, &value);
> + if (ret)
> + goto error;
> +
> + val->intval = value;
> + break;
> +
> + case POWER_SUPPLY_PROP_HEALTH:
> + ret = sc27xx_fgu_get_health(data, &value);
> + if (ret)
> + goto error;
> +
> + val->intval = value;
> + break;
> +
> + case POWER_SUPPLY_PROP_PRESENT:
> + val->intval = data->bat_present;
> + break;
> +
> + case POWER_SUPPLY_PROP_TEMP:
> + ret = sc27xx_fgu_get_temp(data, &value);
> + if (ret)
> + goto error;
> +
> + val->intval = value;
> + break;
> +
> + case POWER_SUPPLY_PROP_TECHNOLOGY:
> + val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> + break;
> +
> + case POWER_SUPPLY_PROP_CAPACITY:
> + ret = sc27xx_fgu_get_capacity(data, &value);
> + if (ret)
> + goto error;
> +
> + val->intval = value;
> + break;
> +
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + ret = sc27xx_fgu_get_vbat_vol(data, &value);
> + if (ret)
> + goto error;
> +
> + val->intval = value * 1000;
> + break;
> +
> + case POWER_SUPPLY_PROP_VOLTAGE_OCV:
> + ret = sc27xx_fgu_get_vbat_ocv(data, &value);
> + if (ret)
> + goto error;
> +
> + val->intval = value * 1000;
> + break;
> +
> + case POWER_SUPPLY_PROP_CURRENT_NOW:
> + case POWER_SUPPLY_PROP_CURRENT_AVG:
> + ret = sc27xx_fgu_get_current(data, &value);
> + if (ret)
> + goto error;
> +
> + val->intval = value * 1000;
> + break;
> +
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> +error:
> + mutex_unlock(&data->lock);
> + return ret;
> +}
> +
> +static void sc27xx_fgu_external_power_changed(struct power_supply *psy)
> +{
> + struct sc27xx_fgu_data *data = power_supply_get_drvdata(psy);
> +
> + power_supply_changed(data->battery);
> +}
> +
> +static enum power_supply_property sc27xx_fgu_props[] = {
> + POWER_SUPPLY_PROP_STATUS,
> + POWER_SUPPLY_PROP_HEALTH,
> + POWER_SUPPLY_PROP_PRESENT,
> + POWER_SUPPLY_PROP_TEMP,
> + POWER_SUPPLY_PROP_TECHNOLOGY,
> + POWER_SUPPLY_PROP_CAPACITY,
> + POWER_SUPPLY_PROP_VOLTAGE_NOW,
> + POWER_SUPPLY_PROP_VOLTAGE_OCV,
> + POWER_SUPPLY_PROP_CURRENT_NOW,
> + POWER_SUPPLY_PROP_CURRENT_AVG,
> +};
> +
> +static const struct power_supply_desc sc27xx_fgu_desc = {
> + .name = "sc27xx-fgu",
> + .type = POWER_SUPPLY_TYPE_BATTERY,
> + .properties = sc27xx_fgu_props,
> + .num_properties = ARRAY_SIZE(sc27xx_fgu_props),
> + .get_property = sc27xx_fgu_get_property,
> + .external_power_changed = sc27xx_fgu_external_power_changed,
> +};
> +
> +static irqreturn_t sc27xx_fgu_bat_detection(int irq, void *dev_id)
> +{
> + struct sc27xx_fgu_data *data = dev_id;
> + int state;
> +
> + mutex_lock(&data->lock);
> +
> + state = gpiod_get_value_cansleep(data->gpiod);
> + if (state < 0) {
> + dev_err(data->dev, "failed to get gpio state\n");
> + mutex_unlock(&data->lock);
> + return IRQ_RETVAL(state);
> + }
> +
> + if (state)
> + data->bat_present = true;
> + else
> + data->bat_present = false;
> +
> + mutex_unlock(&data->lock);

You want to call power_supply_changed() here.

> + return IRQ_HANDLED;
> +}
> +
> +static void sc27xx_fgu_disable(void *_data)
> +{
> + struct sc27xx_fgu_data *data = _data;
> +
> + regmap_update_bits(data->regmap, SC27XX_CLK_EN0, SC27XX_FGU_RTC_EN, 0);
> + regmap_update_bits(data->regmap, SC27XX_MODULE_EN0, SC27XX_FGU_EN, 0);
> +}
> +
> +static int sc27xx_fgu_cap_to_clbcnt(struct sc27xx_fgu_data *data, int capacity)
> +{
> + /*
> + * Get current capacity (mAh) = battery total capacity (mAh) *
> + * current capacity percent (capacity / 100).
> + */
> + int cur_cap = DIV_ROUND_CLOSEST(data->total_cap * capacity, 100);
> +
> + /*
> + * Convert current capacity (mAh) to coulomb counter according to the
> + * formula: 1 mAh =3.6 coulomb.
> + */
> + return DIV_ROUND_CLOSEST(cur_cap * 36, 10);
> +}
> +
> +static int sc27xx_fgu_hw_init(struct sc27xx_fgu_data *data)
> +{
> + struct power_supply_battery_info info = { };
> + int ret;
> +
> + ret = power_supply_get_battery_info(data->battery, &info);
> + if (ret) {
> + dev_err(data->dev, "failed to get battery information\n");
> + return ret;
> + }
> +
> + data->total_cap = info.charge_full_design_uah / 1000;
> + data->max_volt = info.constant_charge_voltage_max_uv / 1000;
> +
> + /* Enable the FGU module */
> + ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN0,
> + SC27XX_FGU_EN, SC27XX_FGU_EN);
> + if (ret) {
> + dev_err(data->dev, "failed to enable fgu\n");
> + return ret;
> + }
> +
> + /* Enable the FGU RTC clock to make it work */
> + ret = regmap_update_bits(data->regmap, SC27XX_CLK_EN0,
> + SC27XX_FGU_RTC_EN, SC27XX_FGU_RTC_EN);
> + if (ret) {
> + dev_err(data->dev, "failed to enable fgu RTC clock\n");
> + goto disable_fgu;
> + }
> +
> + /*
> + * Get the boot battery capacity when system powers on, which is used to
> + * initialize the coulomb counter. After that, we can read the coulomb
> + * counter to measure the battery capacity.
> + */
> + ret = sc27xx_fgu_get_boot_capacity(data, &data->init_cap);
> + if (ret) {
> + dev_err(data->dev, "failed to get boot capacity\n");
> + goto disable_clk;
> + }
> +
> + /*
> + * Convert battery capacity to the corresponding initial coulomb counter
> + * and set into coulomb counter registers.
> + */
> + data->init_clbcnt = sc27xx_fgu_cap_to_clbcnt(data, data->init_cap);
> + ret = sc27xx_fgu_set_clbcnt(data, data->init_clbcnt);
> + if (ret) {
> + dev_err(data->dev, "failed to initialize coulomb counter\n");
> + goto disable_clk;
> + }
> +
> + return 0;
> +
> +disable_clk:
> + regmap_update_bits(data->regmap, SC27XX_CLK_EN0, SC27XX_FGU_RTC_EN, 0);
> +disable_fgu:
> + regmap_update_bits(data->regmap, SC27XX_MODULE_EN0, SC27XX_FGU_EN, 0);
> +
> + return ret;
> +}
> +
> +static int sc27xx_fgu_parse_dt(struct sc27xx_fgu_data *data,
> + struct device_node *np)
> +{
> + const __be32 *list;
> + int i, len, size, ret;
> +
> + ret = of_property_read_u32(np, "reg", &data->base);
> + if (ret) {
> + dev_err(data->dev, "failed to get fgu address\n");
> + return ret;
> + }
> +
> + data->gpiod = devm_gpiod_get_optional(data->dev, "bat-detect", GPIOD_IN);
> + if (IS_ERR(data->gpiod)) {
> + dev_err(data->dev, "failed to get battery detection GPIO\n");
> + return PTR_ERR(data->gpiod);
> + }

According to the binding (and the remaining code!) this gpio is not
optional.

-- Sebastian

> + data->channel = devm_iio_channel_get(data->dev, "bat-temp");
> + if (IS_ERR(data->channel)) {
> + dev_err(data->dev, "failed to get IIO channel\n");
> + return PTR_ERR(data->channel);
> + }
> +
> + /* Get the battery inner resistance */
> + ret = of_property_read_u32(np, "sprd,inner-resist", &data->inner_resist);
> + if (ret) {
> + dev_err(data->dev, "failed to get battery inner resistance\n");
> + return ret;
> + }
> +
> + /* Get battery ocv-capacity table */
> + list = of_get_property(np, "sprd,ocv-cap-table", &size);
> + if (!list || !size) {
> + dev_err(data->dev, "failed to get ocv-capacity table\n");
> + return -EINVAL;
> + }
> +
> + len = size / 8;
> + data->table_len = len;
> + data->cap_table = devm_kzalloc(data->dev,
> + sizeof(struct sc27xx_fgu_capacity_table) * len,
> + GFP_KERNEL);
> + if (!data->cap_table)
> + return -ENOMEM;
> +
> + for (i = 0; i < len; i++) {
> + data->cap_table[i].ocv = be32_to_cpu(*list++);
> + data->cap_table[i].capacity = be32_to_cpu(*list++);
> + }
> +
> + return 0;
> +}
> +
> +static int sc27xx_fgu_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct power_supply_config fgu_cfg = { };
> + struct sc27xx_fgu_data *data;
> + int ret, irq;
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!data->regmap) {
> + dev_err(&pdev->dev, "failed to get regmap\n");
> + return -ENODEV;
> + }
> +
> + mutex_init(&data->lock);
> + data->dev = &pdev->dev;
> + data->bat_present = true;
> +
> + ret = sc27xx_fgu_parse_dt(data, np);
> + if (ret)
> + return ret;
> +
> + fgu_cfg.drv_data = data;
> + fgu_cfg.of_node = np;
> + data->battery = devm_power_supply_register(&pdev->dev, &sc27xx_fgu_desc,
> + &fgu_cfg);
> + if (IS_ERR(data->battery)) {
> + dev_err(&pdev->dev, "failed to register power supply\n");
> + return PTR_ERR(data->battery);
> + }
> +
> + ret = sc27xx_fgu_hw_init(data);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to initialize fgu hardware\n");
> + return ret;
> + }
> +
> + ret = devm_add_action(&pdev->dev, sc27xx_fgu_disable, data);
> + if (ret) {
> + sc27xx_fgu_disable(data);
> + dev_err(&pdev->dev, "failed to add fgu disable action\n");
> + return ret;
> + }
> +
> + irq = gpiod_to_irq(data->gpiod);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "failed to translate GPIO to IRQ\n");
> + return irq;
> + }
> +
> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> + sc27xx_fgu_bat_detection,
> + IRQF_ONESHOT | IRQF_TRIGGER_RISING |
> + IRQF_TRIGGER_FALLING,
> + pdev->name, data);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to request IRQ\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id sc27xx_fgu_of_match[] = {
> + { .compatible = "sprd,sc2731-fgu", },
> + { }
> +};
> +
> +static struct platform_driver sc27xx_fgu_driver = {
> + .probe = sc27xx_fgu_probe,
> + .driver = {
> + .name = "sc27xx-fgu",
> + .of_match_table = sc27xx_fgu_of_match,
> + }
> +};
> +
> +module_platform_driver(sc27xx_fgu_driver);
> +
> +MODULE_DESCRIPTION("Spreadtrum SC27XX PMICs Fual Gauge Unit Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.7.9.5
>


Attachments:
(No filename) (21.84 kB)
signature.asc (849.00 B)
Download all attachments

2018-09-16 20:05:09

by Sebastian Reichel

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: Add Spreadtrum SC27XX fuel gauge unit documentation

Hi,

On Wed, Sep 12, 2018 at 03:29:38PM +0800, Baolin Wang wrote:
> This patch adds the binding documentation for Spreadtrum SC27XX series PMICs
> fuel gauge unit device, which is used to calculate the battery capacity.
>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> .../devicetree/bindings/power/supply/sc27xx-fg.txt | 55 ++++++++++++++++++++
> 1 file changed, 55 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
>
> diff --git a/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
> new file mode 100644
> index 0000000..7447bae
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
> @@ -0,0 +1,55 @@
> +Spreadtrum SC27XX PMICs Fuel Gauge Unit Power Supply Bindings
> +
> +Required properties:
> +- compatible: Should be one of the following:
> + "sprd,sc2720-fgu",
> + "sprd,sc2721-fgu",
> + "sprd,sc2723-fgu",
> + "sprd,sc2730-fgu",
> + "sprd,sc2731-fgu".
> +- reg: The address offset of fuel gauge unit.
> +- bat-detect-gpio: GPIO for battery detection.
> +- io-channels: Specify the IIO ADC channel to get temperature.
> +- io-channel-names: Should be "bat-temp".
> +- sprd,inner-resist: Specify the the battery inner resistance (mOhm).

This should be a property of the battery.without the sprd, prefix.

> +- sprd,ocv-cap-table: Provide the battery capacity percent with corresponding
> + open circuit voltage (ocv) of the battery, which is used to look up current
> + battery capacity according to current baterry ocv values.

This should also be part of the battery binding. I just reviewed a
patchset for the Qualcomm Battery Monitoring System, which needs the
same functionality. The Qualcomm binding is more advanced, but
should also support this simpler case. Thus I think it makes sense
to use its description as base for adding support for this feature
to Documentation/devicetree/bindings/power/supply/battery.txt

-- Sebastian

> +- monitored-battery: Phandle of battery characteristics devicetree node.
> + The fuel gauge uses the following battery properties:
> +- charge-full-design-microamp-hours: battery design capacity.
> +- constant-charge-voltage-max-microvolt: maximum constant input voltage.
> + See Documentation/devicetree/bindings/power/supply/battery.txt
> +
> +Example:
> +
> + bat: battery {
> + compatible = "simple-battery";
> + charge-full-design-microamp-hours = <1900000>;
> + constant-charge-voltage-max-microvolt = <4350000>;
> + ......
> + };
> +
> + sc2731_pmic: pmic@0 {
> + compatible = "sprd,sc2731";
> + reg = <0>;
> + spi-max-frequency = <26000000>;
> + interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + fgu@a00 {
> + compatible = "sprd,sc2731-fgu";
> + reg = <0xa00>;
> + bat-detect-gpio = <&pmic_eic 9 GPIO_ACTIVE_HIGH>;
> + io-channels = <&pmic_adc 5>;
> + io-channel-names = "bat-temp";
> + sprd,inner-resist = <250>;
> + sprd,ocv-cap-table = <4185 100>, <4113 95>, <4066 90>, <4022 85>, <3983 80>, <3949 75>, <3917 70>,
> + <3889 65>, <3864 60>, <3835 55>, <3805 50>, <3787 45>, <3777 40>, <3773 35>, <3770 30>,
> + <3765 25>, <3752 20>, <3724 15>, <3680 10>, <3605 5>, <3400 0>;
> + monitored-battery = <&bat>;
> + };
> + };
> --
> 1.7.9.5
>


Attachments:
(No filename) (3.43 kB)
signature.asc (849.00 B)
Download all attachments

2018-09-17 03:43:52

by Baolin Wang

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: Add Spreadtrum SC27XX fuel gauge unit documentation

Hi Sebastian,

On 16 September 2018 at 21:57, Sebastian Reichel
<[email protected]> wrote:
> Hi,
>
> On Wed, Sep 12, 2018 at 03:29:38PM +0800, Baolin Wang wrote:
>> This patch adds the binding documentation for Spreadtrum SC27XX series PMICs
>> fuel gauge unit device, which is used to calculate the battery capacity.
>>
>> Signed-off-by: Baolin Wang <[email protected]>
>> ---
>> .../devicetree/bindings/power/supply/sc27xx-fg.txt | 55 ++++++++++++++++++++
>> 1 file changed, 55 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
>> new file mode 100644
>> index 0000000..7447bae
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
>> @@ -0,0 +1,55 @@
>> +Spreadtrum SC27XX PMICs Fuel Gauge Unit Power Supply Bindings
>> +
>> +Required properties:
>> +- compatible: Should be one of the following:
>> + "sprd,sc2720-fgu",
>> + "sprd,sc2721-fgu",
>> + "sprd,sc2723-fgu",
>> + "sprd,sc2730-fgu",
>> + "sprd,sc2731-fgu".
>> +- reg: The address offset of fuel gauge unit.
>> +- bat-detect-gpio: GPIO for battery detection.
>> +- io-channels: Specify the IIO ADC channel to get temperature.
>> +- io-channel-names: Should be "bat-temp".
>> +- sprd,inner-resist: Specify the the battery inner resistance (mOhm).
>
> This should be a property of the battery.without the sprd, prefix.

Right. But I did not find one proper property of the battery, so I
will add one new standard property of battery named
'inner-resistance-microohm' in next version. Is it OK for you?

>
>> +- sprd,ocv-cap-table: Provide the battery capacity percent with corresponding
>> + open circuit voltage (ocv) of the battery, which is used to look up current
>> + battery capacity according to current baterry ocv values.
>
> This should also be part of the battery binding. I just reviewed a
> patchset for the Qualcomm Battery Monitoring System, which needs the
> same functionality. The Qualcomm binding is more advanced, but
> should also support this simpler case. Thus I think it makes sense
> to use its description as base for adding support for this feature
> to Documentation/devicetree/bindings/power/supply/battery.txt

That's great. But could you give me the link of Qualcomm binding,
which I can change my bindings. Thanks for your comments.

--
Baolin Wang
Best Regards

2018-09-17 04:02:16

by Baolin Wang

[permalink] [raw]
Subject: Re: [PATCH 2/2] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

Hi Sebastian,

On 16 September 2018 at 22:35, Sebastian Reichel
<[email protected]> wrote:
> Hi,
>
> Looks mostly good. I have a couple of comments in addition to the
> ones from the binding about using battery_info for the OCV ->
> capacity mapping.

OK.

>> +
>> +static int sc27xx_fgu_get_vbat_vol(struct sc27xx_fgu_data *data, int *val)
>> +{
>> + int ret, vol;
>> +
>> + ret = regmap_read(data->regmap, data->base + SC27XX_FGU_VOLTAGE, &vol);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * It is ADC values reading from registers which need to convert to
>> + * corresponding voltage values.
>> + */
>> + *val = sc27xx_fgu_adc_to_voltage(vol);
>> +
>> + return 0;
>> +}
>> +
>> +static int sc27xx_fgu_get_current(struct sc27xx_fgu_data *data, int *val)
>> +{
>> + int ret, cur;
>> +
>> + ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CURRENT, &cur);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * It is ADC values reading from registers which need to convert to
>> + * corresponding current values.
>> + */
>> + *val = sc27xx_fgu_adc_to_current(cur - SC27XX_FGU_CUR_BASIC_ADC);
>> +
>> + return 0;
>> +}
>> +
>> +static int sc27xx_fgu_get_vbat_ocv(struct sc27xx_fgu_data *data, int *val)
>> +{
>> + int vol, cur, ret;
>> +
>> + ret = sc27xx_fgu_get_vbat_vol(data, &vol);
>> + if (ret)
>> + return ret;
>> +
>> + ret = sc27xx_fgu_get_current(data, &cur);
>> + if (ret)
>> + return ret;
>> +
>> + *val = vol - (cur * data->inner_resist) / 1000;
>
> You multiply this with 1000 directly after this function to get back
> to uV. Just drop the division and the multiplication.

But the vol's unit is mV, so I can change to:

*val = vol * 1000 - cur * data->inner_resist;

>> +
>> + return 0;
>> +}
>> +
>> +static int sc27xx_fgu_get_temp(struct sc27xx_fgu_data *data, int *temp)
>> +{
>> + return iio_read_channel_processed(data->channel, temp);
>> +}
>> +
>> +static int sc27xx_fgu_get_health(struct sc27xx_fgu_data *data, int *health)
>> +{
>> + int ret, vol;
>> +
>> + ret = sc27xx_fgu_get_vbat_vol(data, &vol);
>> + if (ret)
>> + return ret;
>> +
>> + if (vol > data->max_volt)
>> + *health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>> + else
>> + *health = POWER_SUPPLY_HEALTH_GOOD;
>> +
>> + return 0;
>> +}
>> +
>> +static int sc27xx_fgu_get_status(struct sc27xx_fgu_data *data, int *status)
>> +{
>> + union power_supply_propval val;
>> + struct power_supply *psy;
>> + int i, ret = -EINVAL;
>> +
>> + for (i = 0; i < ARRAY_SIZE(sc27xx_charger_supply_name); i++) {
>> + psy = power_supply_get_by_name(sc27xx_charger_supply_name[i]);
>> + if (!psy)
>> + continue;
>> +
>> + ret = power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS,
>> + &val);
>> + power_supply_put(psy);
>> + if (ret)
>> + return ret;
>> +
>> + *status = val.intval;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int sc27xx_fgu_get_property(struct power_supply *psy,
>> + enum power_supply_property psp,
>> + union power_supply_propval *val)
>> +{
>> + struct sc27xx_fgu_data *data = power_supply_get_drvdata(psy);
>> + int ret = 0;
>> + int value;
>> +
>> + mutex_lock(&data->lock);
>> +
>> + switch (psp) {
>> + case POWER_SUPPLY_PROP_STATUS:
>> + ret = sc27xx_fgu_get_status(data, &value);
>> + if (ret)
>> + goto error;
>> +
>> + val->intval = value;
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_HEALTH:
>> + ret = sc27xx_fgu_get_health(data, &value);
>> + if (ret)
>> + goto error;
>> +
>> + val->intval = value;
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_PRESENT:
>> + val->intval = data->bat_present;
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_TEMP:
>> + ret = sc27xx_fgu_get_temp(data, &value);
>> + if (ret)
>> + goto error;
>> +
>> + val->intval = value;
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_TECHNOLOGY:
>> + val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_CAPACITY:
>> + ret = sc27xx_fgu_get_capacity(data, &value);
>> + if (ret)
>> + goto error;
>> +
>> + val->intval = value;
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> + ret = sc27xx_fgu_get_vbat_vol(data, &value);
>> + if (ret)
>> + goto error;
>> +
>> + val->intval = value * 1000;
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_VOLTAGE_OCV:
>> + ret = sc27xx_fgu_get_vbat_ocv(data, &value);
>> + if (ret)
>> + goto error;
>> +
>> + val->intval = value * 1000;
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_CURRENT_NOW:
>> + case POWER_SUPPLY_PROP_CURRENT_AVG:
>> + ret = sc27xx_fgu_get_current(data, &value);
>> + if (ret)
>> + goto error;
>> +
>> + val->intval = value * 1000;
>> + break;
>> +
>> + default:
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> +error:
>> + mutex_unlock(&data->lock);
>> + return ret;
>> +}
>> +
>> +static void sc27xx_fgu_external_power_changed(struct power_supply *psy)
>> +{
>> + struct sc27xx_fgu_data *data = power_supply_get_drvdata(psy);
>> +
>> + power_supply_changed(data->battery);
>> +}
>> +
>> +static enum power_supply_property sc27xx_fgu_props[] = {
>> + POWER_SUPPLY_PROP_STATUS,
>> + POWER_SUPPLY_PROP_HEALTH,
>> + POWER_SUPPLY_PROP_PRESENT,
>> + POWER_SUPPLY_PROP_TEMP,
>> + POWER_SUPPLY_PROP_TECHNOLOGY,
>> + POWER_SUPPLY_PROP_CAPACITY,
>> + POWER_SUPPLY_PROP_VOLTAGE_NOW,
>> + POWER_SUPPLY_PROP_VOLTAGE_OCV,
>> + POWER_SUPPLY_PROP_CURRENT_NOW,
>> + POWER_SUPPLY_PROP_CURRENT_AVG,
>> +};
>> +
>> +static const struct power_supply_desc sc27xx_fgu_desc = {
>> + .name = "sc27xx-fgu",
>> + .type = POWER_SUPPLY_TYPE_BATTERY,
>> + .properties = sc27xx_fgu_props,
>> + .num_properties = ARRAY_SIZE(sc27xx_fgu_props),
>> + .get_property = sc27xx_fgu_get_property,
>> + .external_power_changed = sc27xx_fgu_external_power_changed,
>> +};
>> +
>> +static irqreturn_t sc27xx_fgu_bat_detection(int irq, void *dev_id)
>> +{
>> + struct sc27xx_fgu_data *data = dev_id;
>> + int state;
>> +
>> + mutex_lock(&data->lock);
>> +
>> + state = gpiod_get_value_cansleep(data->gpiod);
>> + if (state < 0) {
>> + dev_err(data->dev, "failed to get gpio state\n");
>> + mutex_unlock(&data->lock);
>> + return IRQ_RETVAL(state);
>> + }
>> +
>> + if (state)
>> + data->bat_present = true;
>> + else
>> + data->bat_present = false;
>> +
>> + mutex_unlock(&data->lock);
>
> You want to call power_supply_changed() here.

Ah, right. Missed this.

>
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static void sc27xx_fgu_disable(void *_data)
>> +{
>> + struct sc27xx_fgu_data *data = _data;
>> +
>> + regmap_update_bits(data->regmap, SC27XX_CLK_EN0, SC27XX_FGU_RTC_EN, 0);
>> + regmap_update_bits(data->regmap, SC27XX_MODULE_EN0, SC27XX_FGU_EN, 0);
>> +}
>> +
>> +static int sc27xx_fgu_cap_to_clbcnt(struct sc27xx_fgu_data *data, int capacity)
>> +{
>> + /*
>> + * Get current capacity (mAh) = battery total capacity (mAh) *
>> + * current capacity percent (capacity / 100).
>> + */
>> + int cur_cap = DIV_ROUND_CLOSEST(data->total_cap * capacity, 100);
>> +
>> + /*
>> + * Convert current capacity (mAh) to coulomb counter according to the
>> + * formula: 1 mAh =3.6 coulomb.
>> + */
>> + return DIV_ROUND_CLOSEST(cur_cap * 36, 10);
>> +}
>> +
>> +static int sc27xx_fgu_hw_init(struct sc27xx_fgu_data *data)
>> +{
>> + struct power_supply_battery_info info = { };
>> + int ret;
>> +
>> + ret = power_supply_get_battery_info(data->battery, &info);
>> + if (ret) {
>> + dev_err(data->dev, "failed to get battery information\n");
>> + return ret;
>> + }
>> +
>> + data->total_cap = info.charge_full_design_uah / 1000;
>> + data->max_volt = info.constant_charge_voltage_max_uv / 1000;
>> +
>> + /* Enable the FGU module */
>> + ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN0,
>> + SC27XX_FGU_EN, SC27XX_FGU_EN);
>> + if (ret) {
>> + dev_err(data->dev, "failed to enable fgu\n");
>> + return ret;
>> + }
>> +
>> + /* Enable the FGU RTC clock to make it work */
>> + ret = regmap_update_bits(data->regmap, SC27XX_CLK_EN0,
>> + SC27XX_FGU_RTC_EN, SC27XX_FGU_RTC_EN);
>> + if (ret) {
>> + dev_err(data->dev, "failed to enable fgu RTC clock\n");
>> + goto disable_fgu;
>> + }
>> +
>> + /*
>> + * Get the boot battery capacity when system powers on, which is used to
>> + * initialize the coulomb counter. After that, we can read the coulomb
>> + * counter to measure the battery capacity.
>> + */
>> + ret = sc27xx_fgu_get_boot_capacity(data, &data->init_cap);
>> + if (ret) {
>> + dev_err(data->dev, "failed to get boot capacity\n");
>> + goto disable_clk;
>> + }
>> +
>> + /*
>> + * Convert battery capacity to the corresponding initial coulomb counter
>> + * and set into coulomb counter registers.
>> + */
>> + data->init_clbcnt = sc27xx_fgu_cap_to_clbcnt(data, data->init_cap);
>> + ret = sc27xx_fgu_set_clbcnt(data, data->init_clbcnt);
>> + if (ret) {
>> + dev_err(data->dev, "failed to initialize coulomb counter\n");
>> + goto disable_clk;
>> + }
>> +
>> + return 0;
>> +
>> +disable_clk:
>> + regmap_update_bits(data->regmap, SC27XX_CLK_EN0, SC27XX_FGU_RTC_EN, 0);
>> +disable_fgu:
>> + regmap_update_bits(data->regmap, SC27XX_MODULE_EN0, SC27XX_FGU_EN, 0);
>> +
>> + return ret;
>> +}
>> +
>> +static int sc27xx_fgu_parse_dt(struct sc27xx_fgu_data *data,
>> + struct device_node *np)
>> +{
>> + const __be32 *list;
>> + int i, len, size, ret;
>> +
>> + ret = of_property_read_u32(np, "reg", &data->base);
>> + if (ret) {
>> + dev_err(data->dev, "failed to get fgu address\n");
>> + return ret;
>> + }
>> +
>> + data->gpiod = devm_gpiod_get_optional(data->dev, "bat-detect", GPIOD_IN);
>> + if (IS_ERR(data->gpiod)) {
>> + dev_err(data->dev, "failed to get battery detection GPIO\n");
>> + return PTR_ERR(data->gpiod);
>> + }
>
> According to the binding (and the remaining code!) this gpio is not
> optional.

Yes, they are not optional. If we can not get the detection GPIO, we
will return errors. So am I missing something else?

Thanks for your comments.

>
> -- Sebastian
>
>> + data->channel = devm_iio_channel_get(data->dev, "bat-temp");
>> + if (IS_ERR(data->channel)) {
>> + dev_err(data->dev, "failed to get IIO channel\n");
>> + return PTR_ERR(data->channel);
>> + }
>> +
>> + /* Get the battery inner resistance */
>> + ret = of_property_read_u32(np, "sprd,inner-resist", &data->inner_resist);
>> + if (ret) {
>> + dev_err(data->dev, "failed to get battery inner resistance\n");
>> + return ret;
>> + }
>> +
>> + /* Get battery ocv-capacity table */
>> + list = of_get_property(np, "sprd,ocv-cap-table", &size);
>> + if (!list || !size) {
>> + dev_err(data->dev, "failed to get ocv-capacity table\n");
>> + return -EINVAL;
>> + }
>> +
>> + len = size / 8;
>> + data->table_len = len;
>> + data->cap_table = devm_kzalloc(data->dev,
>> + sizeof(struct sc27xx_fgu_capacity_table) * len,
>> + GFP_KERNEL);
>> + if (!data->cap_table)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < len; i++) {
>> + data->cap_table[i].ocv = be32_to_cpu(*list++);
>> + data->cap_table[i].capacity = be32_to_cpu(*list++);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int sc27xx_fgu_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct power_supply_config fgu_cfg = { };
>> + struct sc27xx_fgu_data *data;
>> + int ret, irq;
>> +
>> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> + if (!data->regmap) {
>> + dev_err(&pdev->dev, "failed to get regmap\n");
>> + return -ENODEV;
>> + }
>> +
>> + mutex_init(&data->lock);
>> + data->dev = &pdev->dev;
>> + data->bat_present = true;
>> +
>> + ret = sc27xx_fgu_parse_dt(data, np);
>> + if (ret)
>> + return ret;
>> +
>> + fgu_cfg.drv_data = data;
>> + fgu_cfg.of_node = np;
>> + data->battery = devm_power_supply_register(&pdev->dev, &sc27xx_fgu_desc,
>> + &fgu_cfg);
>> + if (IS_ERR(data->battery)) {
>> + dev_err(&pdev->dev, "failed to register power supply\n");
>> + return PTR_ERR(data->battery);
>> + }
>> +
>> + ret = sc27xx_fgu_hw_init(data);
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to initialize fgu hardware\n");
>> + return ret;
>> + }
>> +
>> + ret = devm_add_action(&pdev->dev, sc27xx_fgu_disable, data);
>> + if (ret) {
>> + sc27xx_fgu_disable(data);
>> + dev_err(&pdev->dev, "failed to add fgu disable action\n");
>> + return ret;
>> + }
>> +
>> + irq = gpiod_to_irq(data->gpiod);
>> + if (irq < 0) {
>> + dev_err(&pdev->dev, "failed to translate GPIO to IRQ\n");
>> + return irq;
>> + }
>> +
>> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
>> + sc27xx_fgu_bat_detection,
>> + IRQF_ONESHOT | IRQF_TRIGGER_RISING |
>> + IRQF_TRIGGER_FALLING,
>> + pdev->name, data);
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to request IRQ\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id sc27xx_fgu_of_match[] = {
>> + { .compatible = "sprd,sc2731-fgu", },
>> + { }
>> +};
>> +
>> +static struct platform_driver sc27xx_fgu_driver = {
>> + .probe = sc27xx_fgu_probe,
>> + .driver = {
>> + .name = "sc27xx-fgu",
>> + .of_match_table = sc27xx_fgu_of_match,
>> + }
>> +};
>> +
>> +module_platform_driver(sc27xx_fgu_driver);
>> +
>> +MODULE_DESCRIPTION("Spreadtrum SC27XX PMICs Fual Gauge Unit Driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.7.9.5
>>



--
Baolin Wang
Best Regards

2018-09-17 20:45:33

by Baolin Wang

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: Add Spreadtrum SC27XX fuel gauge unit documentation

Hi Rob,

On 17 September 2018 at 11:26, Rob Herring <[email protected]> wrote:
> On Wed, Sep 12, 2018 at 03:29:38PM +0800, Baolin Wang wrote:
>> This patch adds the binding documentation for Spreadtrum SC27XX series PMICs
>> fuel gauge unit device, which is used to calculate the battery capacity.
>>
>> Signed-off-by: Baolin Wang <[email protected]>
>> ---
>> .../devicetree/bindings/power/supply/sc27xx-fg.txt | 55 ++++++++++++++++++++
>> 1 file changed, 55 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
>> new file mode 100644
>> index 0000000..7447bae
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
>> @@ -0,0 +1,55 @@
>> +Spreadtrum SC27XX PMICs Fuel Gauge Unit Power Supply Bindings
>> +
>> +Required properties:
>> +- compatible: Should be one of the following:
>> + "sprd,sc2720-fgu",
>> + "sprd,sc2721-fgu",
>> + "sprd,sc2723-fgu",
>> + "sprd,sc2730-fgu",
>> + "sprd,sc2731-fgu".
>> +- reg: The address offset of fuel gauge unit.
>> +- bat-detect-gpio: GPIO for battery detection.
>
> We already have sbs,battery-detect-gpios, let's drop the 'sbs,' and copy
> that.

Sure.

>
>> +- io-channels: Specify the IIO ADC channel to get temperature.
>> +- io-channel-names: Should be "bat-temp".
>> +- sprd,inner-resist: Specify the the battery inner resistance (mOhm).
>
> If this is a property of the battery, then why a sprd vendor prefix?
> This should perhaps be a common property.
>
> Minimally, it needs a unit suffix as defined in property-units.txt.

Right. As Sebastian pointed out previously we should add one common
property to describe the battery inner resistance.

>
>> +- sprd,ocv-cap-table: Provide the battery capacity percent with corresponding
>> + open circuit voltage (ocv) of the battery, which is used to look up current
>> + battery capacity according to current baterry ocv values.
>
> Sounds like another battery property.

Yes.

>
>> +- monitored-battery: Phandle of battery characteristics devicetree node.
>
> If the battery is represented in another node, why are properties of the
> battery in this node.

Sure. I can remove these properties which are belonging to the battery
node. Thanks.

>
>> + The fuel gauge uses the following battery properties:
>> +- charge-full-design-microamp-hours: battery design capacity.
>> +- constant-charge-voltage-max-microvolt: maximum constant input voltage.
>> + See Documentation/devicetree/bindings/power/supply/battery.txt
>> +
>> +Example:
>> +
>> + bat: battery {
>> + compatible = "simple-battery";
>> + charge-full-design-microamp-hours = <1900000>;
>> + constant-charge-voltage-max-microvolt = <4350000>;
>> + ......
>> + };
>> +
>> + sc2731_pmic: pmic@0 {
>> + compatible = "sprd,sc2731";
>> + reg = <0>;
>> + spi-max-frequency = <26000000>;
>> + interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-controller;
>> + #interrupt-cells = <2>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + fgu@a00 {
>> + compatible = "sprd,sc2731-fgu";
>> + reg = <0xa00>;
>> + bat-detect-gpio = <&pmic_eic 9 GPIO_ACTIVE_HIGH>;
>> + io-channels = <&pmic_adc 5>;
>> + io-channel-names = "bat-temp";
>> + sprd,inner-resist = <250>;
>> + sprd,ocv-cap-table = <4185 100>, <4113 95>, <4066 90>, <4022 85>, <3983 80>, <3949 75>, <3917 70>,
>> + <3889 65>, <3864 60>, <3835 55>, <3805 50>, <3787 45>, <3777 40>, <3773 35>, <3770 30>,
>> + <3765 25>, <3752 20>, <3724 15>, <3680 10>, <3605 5>, <3400 0>;
>> + monitored-battery = <&bat>;
>> + };
>> + };
>> --
>> 1.7.9.5
>>
>



--
Baolin Wang
Best Regards

2018-09-20 00:39:19

by Sebastian Reichel

[permalink] [raw]
Subject: Re: [PATCH 2/2] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

Hi,

On Mon, Sep 17, 2018 at 12:01:51PM +0800, Baolin Wang wrote:
[...]
> >> +static int sc27xx_fgu_parse_dt(struct sc27xx_fgu_data *data,
> >> + struct device_node *np)
> >> +{
> >> + const __be32 *list;
> >> + int i, len, size, ret;
> >> +
> >> + ret = of_property_read_u32(np, "reg", &data->base);
> >> + if (ret) {
> >> + dev_err(data->dev, "failed to get fgu address\n");
> >> + return ret;
> >> + }
> >> +
> >> + data->gpiod = devm_gpiod_get_optional(data->dev, "bat-detect", GPIOD_IN);
> >> + if (IS_ERR(data->gpiod)) {
> >> + dev_err(data->dev, "failed to get battery detection GPIO\n");
> >> + return PTR_ERR(data->gpiod);
> >> + }
> >
> > According to the binding (and the remaining code!) this gpio is not
> > optional.
>
> Yes, they are not optional. If we can not get the detection GPIO, we
> will return errors. So am I missing something else?
>
> Thanks for your comments.

devm_gpiod_get_optional => devm_gpiod_get

The _optional variant will return NULL if the GPIO is not specified in DT.
The variant without _optional will return an error instead.

-- Sebastian


Attachments:
(No filename) (1.20 kB)
signature.asc (849.00 B)
Download all attachments

2018-09-20 00:43:17

by Baolin Wang

[permalink] [raw]
Subject: Re: [PATCH 2/2] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

Hi,

On 20 September 2018 at 08:36, Sebastian Reichel
<[email protected]> wrote:
> Hi,
>
> On Mon, Sep 17, 2018 at 12:01:51PM +0800, Baolin Wang wrote:
> [...]
>> >> +static int sc27xx_fgu_parse_dt(struct sc27xx_fgu_data *data,
>> >> + struct device_node *np)
>> >> +{
>> >> + const __be32 *list;
>> >> + int i, len, size, ret;
>> >> +
>> >> + ret = of_property_read_u32(np, "reg", &data->base);
>> >> + if (ret) {
>> >> + dev_err(data->dev, "failed to get fgu address\n");
>> >> + return ret;
>> >> + }
>> >> +
>> >> + data->gpiod = devm_gpiod_get_optional(data->dev, "bat-detect", GPIOD_IN);
>> >> + if (IS_ERR(data->gpiod)) {
>> >> + dev_err(data->dev, "failed to get battery detection GPIO\n");
>> >> + return PTR_ERR(data->gpiod);
>> >> + }
>> >
>> > According to the binding (and the remaining code!) this gpio is not
>> > optional.
>>
>> Yes, they are not optional. If we can not get the detection GPIO, we
>> will return errors. So am I missing something else?
>>
>> Thanks for your comments.
>
> devm_gpiod_get_optional => devm_gpiod_get
>
> The _optional variant will return NULL if the GPIO is not specified in DT.
> The variant without _optional will return an error instead.

Ah, you are definitely correct. I missed _optional and thanks for
pointing this out.

--
Baolin Wang
Best Regards

2018-09-20 00:47:08

by Sebastian Reichel

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: Add Spreadtrum SC27XX fuel gauge unit documentation

Hi,

On Mon, Sep 17, 2018 at 11:43:30AM +0800, Baolin Wang wrote:
> Hi Sebastian,
>
> On 16 September 2018 at 21:57, Sebastian Reichel
> <[email protected]> wrote:
> > Hi,
> >
> > On Wed, Sep 12, 2018 at 03:29:38PM +0800, Baolin Wang wrote:
> >> This patch adds the binding documentation for Spreadtrum SC27XX series PMICs
> >> fuel gauge unit device, which is used to calculate the battery capacity.
> >>
> >> Signed-off-by: Baolin Wang <[email protected]>
> >> ---
> >> .../devicetree/bindings/power/supply/sc27xx-fg.txt | 55 ++++++++++++++++++++
> >> 1 file changed, 55 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
> >> new file mode 100644
> >> index 0000000..7447bae
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
> >> @@ -0,0 +1,55 @@
> >> +Spreadtrum SC27XX PMICs Fuel Gauge Unit Power Supply Bindings
> >> +
> >> +Required properties:
> >> +- compatible: Should be one of the following:
> >> + "sprd,sc2720-fgu",
> >> + "sprd,sc2721-fgu",
> >> + "sprd,sc2723-fgu",
> >> + "sprd,sc2730-fgu",
> >> + "sprd,sc2731-fgu".
> >> +- reg: The address offset of fuel gauge unit.
> >> +- bat-detect-gpio: GPIO for battery detection.
> >> +- io-channels: Specify the IIO ADC channel to get temperature.
> >> +- io-channel-names: Should be "bat-temp".
> >> +- sprd,inner-resist: Specify the the battery inner resistance (mOhm).
> >
> > This should be a property of the battery.without the sprd, prefix.
>
> Right. But I did not find one proper property of the battery, so I
> will add one new standard property of battery

Thanks.

> named 'inner-resistance-microohm' in next version. Is it OK for you?

The proper suffix is -micro-ohms according to
Documentation/devicetree/bindings/property-units.txt

Also the proper English term is internal resistance as far
as I know (and Wikipedia also names it this way), so I
suggest to name the property 'internal-resistance-micro-ohms'.

> >> +- sprd,ocv-cap-table: Provide the battery capacity percent with corresponding
> >> + open circuit voltage (ocv) of the battery, which is used to look up current
> >> + battery capacity according to current baterry ocv values.
> >
> > This should also be part of the battery binding. I just reviewed a
> > patchset for the Qualcomm Battery Monitoring System, which needs the
> > same functionality. The Qualcomm binding is more advanced, but
> > should also support this simpler case. Thus I think it makes sense
> > to use its description as base for adding support for this feature
> > to Documentation/devicetree/bindings/power/supply/battery.txt
>
> That's great. But could you give me the link of Qualcomm binding,
> which I can change my bindings. Thanks for your comments.

https://patchwork.kernel.org/patch/10464633/

-- Sebastian


Attachments:
(No filename) (3.00 kB)
signature.asc (849.00 B)
Download all attachments

2018-09-20 00:53:41

by Baolin Wang

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: power: Add Spreadtrum SC27XX fuel gauge unit documentation

Hi Sebastian,

On 20 September 2018 at 08:46, Sebastian Reichel
<[email protected]> wrote:
> Hi,
>
> On Mon, Sep 17, 2018 at 11:43:30AM +0800, Baolin Wang wrote:
>> Hi Sebastian,
>>
>> On 16 September 2018 at 21:57, Sebastian Reichel
>> <[email protected]> wrote:
>> > Hi,
>> >
>> > On Wed, Sep 12, 2018 at 03:29:38PM +0800, Baolin Wang wrote:
>> >> This patch adds the binding documentation for Spreadtrum SC27XX series PMICs
>> >> fuel gauge unit device, which is used to calculate the battery capacity.
>> >>
>> >> Signed-off-by: Baolin Wang <[email protected]>
>> >> ---
>> >> .../devicetree/bindings/power/supply/sc27xx-fg.txt | 55 ++++++++++++++++++++
>> >> 1 file changed, 55 insertions(+)
>> >> create mode 100644 Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
>> >> new file mode 100644
>> >> index 0000000..7447bae
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
>> >> @@ -0,0 +1,55 @@
>> >> +Spreadtrum SC27XX PMICs Fuel Gauge Unit Power Supply Bindings
>> >> +
>> >> +Required properties:
>> >> +- compatible: Should be one of the following:
>> >> + "sprd,sc2720-fgu",
>> >> + "sprd,sc2721-fgu",
>> >> + "sprd,sc2723-fgu",
>> >> + "sprd,sc2730-fgu",
>> >> + "sprd,sc2731-fgu".
>> >> +- reg: The address offset of fuel gauge unit.
>> >> +- bat-detect-gpio: GPIO for battery detection.
>> >> +- io-channels: Specify the IIO ADC channel to get temperature.
>> >> +- io-channel-names: Should be "bat-temp".
>> >> +- sprd,inner-resist: Specify the the battery inner resistance (mOhm).
>> >
>> > This should be a property of the battery.without the sprd, prefix.
>>
>> Right. But I did not find one proper property of the battery, so I
>> will add one new standard property of battery
>
> Thanks.
>
>> named 'inner-resistance-microohm' in next version. Is it OK for you?
>
> The proper suffix is -micro-ohms according to
> Documentation/devicetree/bindings/property-units.txt
>
> Also the proper English term is internal resistance as far
> as I know (and Wikipedia also names it this way), so I
> suggest to name the property 'internal-resistance-micro-ohms'.

Sure.

>
>> >> +- sprd,ocv-cap-table: Provide the battery capacity percent with corresponding
>> >> + open circuit voltage (ocv) of the battery, which is used to look up current
>> >> + battery capacity according to current baterry ocv values.
>> >
>> > This should also be part of the battery binding. I just reviewed a
>> > patchset for the Qualcomm Battery Monitoring System, which needs the
>> > same functionality. The Qualcomm binding is more advanced, but
>> > should also support this simpler case. Thus I think it makes sense
>> > to use its description as base for adding support for this feature
>> > to Documentation/devicetree/bindings/power/supply/battery.txt
>>
>> That's great. But could you give me the link of Qualcomm binding,
>> which I can change my bindings. Thanks for your comments.
>
> https://patchwork.kernel.org/patch/10464633/

Thanks.

--
Baolin Wang
Best Regards