Re: [EXTERNAL] Re: [PATCH v4 2/2] power: supply: bq256xx: Introduce the BQ256XX charger driver

2020-10-01 Thread Ricardo Rivera-Matos

Sebastian

On 9/30/20 6:47 PM, Sebastian Reichel wrote:

Hi,

You are leaking some resources, otherwise LGTM.

ACK


On Wed, Sep 23, 2020 at 10:24:16AM -0500, Ricardo Rivera-Matos wrote:

[...]
+static int bq256xx_hw_init(struct bq256xx_device *bq)
+{
+   struct power_supply_battery_info bat_info = { };
+   int wd_reg_val = BQ256XX_WATCHDOG_DIS;
+   int ret = 0;
+   int i;
+
+   for (i = 0; i < BQ256XX_NUM_WD_VAL; i++) {
+   if (bq->watchdog_timer > bq256xx_watchdog_time[i] &&
+   bq->watchdog_timer < bq256xx_watchdog_time[i + 1])
+   wd_reg_val = i;
+   }
+   ret = regmap_update_bits(bq->regmap, BQ256XX_CHARGER_CONTROL_1,
+BQ256XX_WATCHDOG_MASK, wd_reg_val <<
+   BQ256XX_WDT_BIT_SHIFT);
+
+   ret = power_supply_get_battery_info(bq->charger, _info);
+   if (ret) {
+   dev_warn(bq->dev, "battery info missing, default values will be 
applied\n");
+
+   bat_info.constant_charge_current_max_ua =
+   bq->chip_info->bq256xx_def_ichg;
+
+   bat_info.constant_charge_voltage_max_uv =
+   bq->chip_info->bq256xx_def_vbatreg;
+
+   bat_info.precharge_current_ua =
+   bq->chip_info->bq256xx_def_iprechg;
+
+   bat_info.charge_term_current_ua =
+   bq->chip_info->bq256xx_def_iterm;
+
+   bq->init_data.ichg_max =
+   bq->chip_info->bq256xx_max_ichg;
+
+   bq->init_data.vbatreg_max =
+   bq->chip_info->bq256xx_max_vbatreg;
+   } else {
+   bq->init_data.ichg_max =
+   bat_info.constant_charge_current_max_ua;
+
+   bq->init_data.vbatreg_max =
+   bat_info.constant_charge_voltage_max_uv;
+   }
+
+   ret = bq->chip_info->bq256xx_set_vindpm(bq, bq->init_data.vindpm);
+   if (ret)
+   goto err_out;
+
+   ret = bq->chip_info->bq256xx_set_iindpm(bq, bq->init_data.iindpm);
+   if (ret)
+   goto err_out;
+
+   ret = bq->chip_info->bq256xx_set_ichg(bq,
+   bat_info.constant_charge_current_max_ua);
+   if (ret)
+   goto err_out;
+
+   ret = bq->chip_info->bq256xx_set_iprechg(bq,
+   bat_info.precharge_current_ua);
+   if (ret)
+   goto err_out;
+
+   ret = bq->chip_info->bq256xx_set_vbatreg(bq,
+   bat_info.constant_charge_voltage_max_uv);
+   if (ret)
+   goto err_out;
+
+   ret = bq->chip_info->bq256xx_set_iterm(bq,
+   bat_info.charge_term_current_ua);
+   if (ret)
+   goto err_out;

You need to power_supply_put_battery_info().

ACK



+
+   return 0;
+
+err_out:
+   return ret;
+}
+
+static int bq256xx_parse_dt(struct bq256xx_device *bq)
+{
+   int ret = 0;
+
+   ret = device_property_read_u32(bq->dev, "ti,watchdog-timeout-ms",
+  >watchdog_timer);
+   if (ret)
+   bq->watchdog_timer = BQ256XX_WATCHDOG_DIS;
+
+   if (bq->watchdog_timer > BQ256XX_WATCHDOG_MAX ||
+   bq->watchdog_timer < BQ256XX_WATCHDOG_DIS)
+   return -EINVAL;
+
+   ret = device_property_read_u32(bq->dev,
+  "input-voltage-limit-microvolt",
+  >init_data.vindpm);
+   if (ret)
+   bq->init_data.vindpm = bq->chip_info->bq256xx_def_vindpm;
+
+   ret = device_property_read_u32(bq->dev,
+  "input-current-limit-microamp",
+  >init_data.iindpm);
+   if (ret)
+   bq->init_data.iindpm = bq->chip_info->bq256xx_def_iindpm;
+
+   return 0;
+}
+
+static int bq256xx_probe(struct i2c_client *client,
+const struct i2c_device_id *id)
+{
+   struct device *dev = >dev;
+   struct bq256xx_device *bq;
+   int ret;
+
+   bq = devm_kzalloc(dev, sizeof(*bq), GFP_KERNEL);
+   if (!bq)
+   return -ENOMEM;
+
+   bq->client = client;
+   bq->dev = dev;
+   bq->chip_info = _chip_info_tbl[id->driver_data];
+
+   mutex_init(>lock);
+
+   strncpy(bq->model_name, id->name, I2C_NAME_SIZE);
+
+   bq->regmap = devm_regmap_init_i2c(client,
+   bq->chip_info->bq256xx_regmap_config);
+
+   if (IS_ERR(bq->regmap)) {
+   dev_err(dev, "Failed to allocate register map\n");
+   return PTR_ERR(bq->regmap);
+   }
+
+   i2c_set_clientdata(client, bq);
+
+   ret = bq256xx_parse_dt(bq);
+   if (ret) {
+   dev_err(dev, "Failed to read device tree properties%d\n", 

Re: [PATCH v4 2/2] power: supply: bq256xx: Introduce the BQ256XX charger driver

2020-09-30 Thread Sebastian Reichel
Hi,

You are leaking some resources, otherwise LGTM.

On Wed, Sep 23, 2020 at 10:24:16AM -0500, Ricardo Rivera-Matos wrote:
> [...]
> +static int bq256xx_hw_init(struct bq256xx_device *bq)
> +{
> + struct power_supply_battery_info bat_info = { };
> + int wd_reg_val = BQ256XX_WATCHDOG_DIS;
> + int ret = 0;
> + int i;
> +
> + for (i = 0; i < BQ256XX_NUM_WD_VAL; i++) {
> + if (bq->watchdog_timer > bq256xx_watchdog_time[i] &&
> + bq->watchdog_timer < bq256xx_watchdog_time[i + 1])
> + wd_reg_val = i;
> + }
> + ret = regmap_update_bits(bq->regmap, BQ256XX_CHARGER_CONTROL_1,
> +  BQ256XX_WATCHDOG_MASK, wd_reg_val <<
> + BQ256XX_WDT_BIT_SHIFT);
> +
> + ret = power_supply_get_battery_info(bq->charger, _info);
> + if (ret) {
> + dev_warn(bq->dev, "battery info missing, default values will be 
> applied\n");
> +
> + bat_info.constant_charge_current_max_ua =
> + bq->chip_info->bq256xx_def_ichg;
> +
> + bat_info.constant_charge_voltage_max_uv =
> + bq->chip_info->bq256xx_def_vbatreg;
> +
> + bat_info.precharge_current_ua =
> + bq->chip_info->bq256xx_def_iprechg;
> +
> + bat_info.charge_term_current_ua =
> + bq->chip_info->bq256xx_def_iterm;
> +
> + bq->init_data.ichg_max =
> + bq->chip_info->bq256xx_max_ichg;
> +
> + bq->init_data.vbatreg_max =
> + bq->chip_info->bq256xx_max_vbatreg;
> + } else {
> + bq->init_data.ichg_max =
> + bat_info.constant_charge_current_max_ua;
> +
> + bq->init_data.vbatreg_max =
> + bat_info.constant_charge_voltage_max_uv;
> + }
> +
> + ret = bq->chip_info->bq256xx_set_vindpm(bq, bq->init_data.vindpm);
> + if (ret)
> + goto err_out;
> +
> + ret = bq->chip_info->bq256xx_set_iindpm(bq, bq->init_data.iindpm);
> + if (ret)
> + goto err_out;
> +
> + ret = bq->chip_info->bq256xx_set_ichg(bq,
> + bat_info.constant_charge_current_max_ua);
> + if (ret)
> + goto err_out;
> +
> + ret = bq->chip_info->bq256xx_set_iprechg(bq,
> + bat_info.precharge_current_ua);
> + if (ret)
> + goto err_out;
> +
> + ret = bq->chip_info->bq256xx_set_vbatreg(bq,
> + bat_info.constant_charge_voltage_max_uv);
> + if (ret)
> + goto err_out;
> +
> + ret = bq->chip_info->bq256xx_set_iterm(bq,
> + bat_info.charge_term_current_ua);
> + if (ret)
> + goto err_out;

You need to power_supply_put_battery_info().

> +
> + return 0;
> +
> +err_out:
> + return ret;
> +}
> +
> +static int bq256xx_parse_dt(struct bq256xx_device *bq)
> +{
> + int ret = 0;
> +
> + ret = device_property_read_u32(bq->dev, "ti,watchdog-timeout-ms",
> +>watchdog_timer);
> + if (ret)
> + bq->watchdog_timer = BQ256XX_WATCHDOG_DIS;
> +
> + if (bq->watchdog_timer > BQ256XX_WATCHDOG_MAX ||
> + bq->watchdog_timer < BQ256XX_WATCHDOG_DIS)
> + return -EINVAL;
> +
> + ret = device_property_read_u32(bq->dev,
> +"input-voltage-limit-microvolt",
> +>init_data.vindpm);
> + if (ret)
> + bq->init_data.vindpm = bq->chip_info->bq256xx_def_vindpm;
> +
> + ret = device_property_read_u32(bq->dev,
> +"input-current-limit-microamp",
> +>init_data.iindpm);
> + if (ret)
> + bq->init_data.iindpm = bq->chip_info->bq256xx_def_iindpm;
> +
> + return 0;
> +}
> +
> +static int bq256xx_probe(struct i2c_client *client,
> +  const struct i2c_device_id *id)
> +{
> + struct device *dev = >dev;
> + struct bq256xx_device *bq;
> + int ret;
> +
> + bq = devm_kzalloc(dev, sizeof(*bq), GFP_KERNEL);
> + if (!bq)
> + return -ENOMEM;
> +
> + bq->client = client;
> + bq->dev = dev;
> + bq->chip_info = _chip_info_tbl[id->driver_data];
> +
> + mutex_init(>lock);
> +
> + strncpy(bq->model_name, id->name, I2C_NAME_SIZE);
> +
> + bq->regmap = devm_regmap_init_i2c(client,
> + bq->chip_info->bq256xx_regmap_config);
> +
> + if (IS_ERR(bq->regmap)) {
> + dev_err(dev, "Failed to allocate register map\n");
> + return PTR_ERR(bq->regmap);
> + }
> +
> + i2c_set_clientdata(client, bq);
> +
> + ret = bq256xx_parse_dt(bq);
> + if (ret) {
> + dev_err(dev, "Failed to read device tree 

[PATCH v4 2/2] power: supply: bq256xx: Introduce the BQ256XX charger driver

2020-09-23 Thread Ricardo Rivera-Matos
The BQ256XX family of devices are highly integrated buck chargers
for single cell batteries.

Signed-off-by: Ricardo Rivera-Matos 
---
 drivers/power/supply/Kconfig   |   11 +
 drivers/power/supply/Makefile  |1 +
 drivers/power/supply/bq256xx_charger.c | 1769 
 3 files changed, 1781 insertions(+)
 create mode 100644 drivers/power/supply/bq256xx_charger.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 44d3c8512fb8..87d852914bc2 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -618,6 +618,17 @@ config CHARGER_BQ25890
help
  Say Y to enable support for the TI BQ25890 battery charger.
 
+config CHARGER_BQ256XX
+   tristate "TI BQ256XX battery charger driver"
+   depends on I2C
+   depends on GPIOLIB || COMPILE_TEST
+   select REGMAP_I2C
+   help
+ Say Y to enable support for the TI BQ256XX battery chargers. The
+ BQ256XX family of devices are highly-integrated, switch-mode battery
+ charge management and system power path management devices for single
+ cell Li-ion and Li-polymer batteries.
+
 config CHARGER_SMB347
tristate "Summit Microelectronics SMB347 Battery Charger"
depends on I2C
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index b9644663e435..e762442c7cc6 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -83,6 +83,7 @@ obj-$(CONFIG_CHARGER_BQ24190) += bq24190_charger.o
 obj-$(CONFIG_CHARGER_BQ24257)  += bq24257_charger.o
 obj-$(CONFIG_CHARGER_BQ24735)  += bq24735-charger.o
 obj-$(CONFIG_CHARGER_BQ25890)  += bq25890_charger.o
+obj-$(CONFIG_CHARGER_BQ256XX)  += bq256xx_charger.o
 obj-$(CONFIG_CHARGER_SMB347)   += smb347-charger.o
 obj-$(CONFIG_CHARGER_TPS65090) += tps65090-charger.o
 obj-$(CONFIG_CHARGER_TPS65217) += tps65217_charger.o
diff --git a/drivers/power/supply/bq256xx_charger.c 
b/drivers/power/supply/bq256xx_charger.c
new file mode 100644
index ..2b76e859a204
--- /dev/null
+++ b/drivers/power/supply/bq256xx_charger.c
@@ -0,0 +1,1769 @@
+// SPDX-License-Identifier: GPL-2.0
+// BQ256XX Battery Charger Driver
+// Copyright (C) 2020 Texas Instruments Incorporated - http://www.ti.com/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define BQ256XX_MANUFACTURER "Texas Instruments"
+
+#define BQ256XX_INPUT_CURRENT_LIMIT0x00
+#define BQ256XX_CHARGER_CONTROL_0  0x01
+#define BQ256XX_CHARGE_CURRENT_LIMIT   0x02
+#define BQ256XX_PRECHG_AND_TERM_CURR_LIM   0x03
+#define BQ256XX_BATTERY_VOLTAGE_LIMIT  0x04
+#define BQ256XX_CHARGER_CONTROL_1  0x05
+#define BQ256XX_CHARGER_CONTROL_2  0x06
+#define BQ256XX_CHARGER_CONTROL_3  0x07
+#define BQ256XX_CHARGER_STATUS_0   0x08
+#define BQ256XX_CHARGER_STATUS_1   0x09
+#define BQ256XX_CHARGER_STATUS_2   0x0a
+#define BQ256XX_PART_INFORMATION   0x0b
+#define BQ256XX_CHARGER_CONTROL_4  0x0c
+
+#define BQ256XX_IINDPM_MASKGENMASK(4, 0)
+#define BQ256XX_IINDPM_STEP_uA 10
+#define BQ256XX_IINDPM_OFFSET_uA   10
+#define BQ256XX_IINDPM_MIN_uA  10
+#define BQ256XX_IINDPM_MAX_uA  320
+#define BQ256XX_IINDPM_DEF_uA  240
+
+#define BQ256XX_VINDPM_MASKGENMASK(3, 0)
+#define BQ256XX_VINDPM_STEP_uV 10
+#define BQ256XX_VINDPM_OFFSET_uV   390
+#define BQ256XX_VINDPM_MIN_uV  390
+#define BQ256XX_VINDPM_MAX_uV  540
+#define BQ256XX_VINDPM_DEF_uV  450
+
+#define BQ256XX_VBATREG_MASK   GENMASK(7, 3)
+#define BQ2560X_VBATREG_STEP_uV32000
+#define BQ2560X_VBATREG_OFFSET_uV  3856000
+#define BQ2560X_VBATREG_MIN_uV 3856000
+#define BQ2560X_VBATREG_MAX_uV 4624000
+#define BQ2560X_VBATREG_DEF_uV 4208000
+#define BQ25601D_VBATREG_OFFSET_uV 3847000
+#define BQ25601D_VBATREG_MIN_uV3847000
+#define BQ25601D_VBATREG_MAX_uV4615000
+#define BQ25601D_VBATREG_DEF_uV4199000
+#define BQ2561X_VBATREG_STEP_uV1
+#define BQ25611D_VBATREG_MIN_uV3494000
+#define BQ25611D_VBATREG_MAX_uV451
+#define BQ25611D_VBATREG_DEF_uV419
+#define BQ25618_VBATREG_MIN_uV 3504000
+#define BQ25618_VBATREG_MAX_uV 450
+#define BQ25618_VBATREG_DEF_uV 420
+#define BQ256XX_VBATREG_BIT_SHIFT  3
+#define BQ2561X_VBATREG_THRESH 0x8
+#define BQ25611D_VBATREG_THRESH_uV 429
+#define BQ25618_VBATREG_THRESH_uV  430
+
+#define BQ256XX_ITERM_MASK GENMASK(3, 0)
+#define BQ256XX_ITERM_STEP_uA  6
+#define BQ256XX_ITERM_OFFSET_uA