Re: [PATCH v5 2/2] power: supply: bq25790: Introduce the BQ25790 charger driver
On Tue, 2 Feb 2021 at 03:20, Ricardo Rivera-Matos wrote:
>
> From: Dan Murphy
>
> BQ25790 is a highly integrated switch-mode buck-boost charger
> for 1-4 cell Li-ion battery and Li-polymer battery.
>
> Signed-off-by: Ricardo Rivera-Matos
> Signed-off-by: Dan Murphy
Looks like wrong order of Sobs. Since Dan was the author, did you
really contribute here before him?
(...)
> +
> +static bool bq25790_state_changed(struct bq25790_device *bq,
> + struct bq25790_state *new_state)
> +{
> + struct bq25790_state old_state;
> +
> + mutex_lock(>lock);
> + old_state = bq->state;
> + mutex_unlock(>lock);
> +
> + return memcmp(_state, new_state,
> + sizeof(struct bq25790_state)) != 0;
> +}
> +
> +static irqreturn_t bq25790_irq_handler_thread(int irq, void *private)
> +{
> + struct bq25790_device *bq = private;
> + struct bq25790_state state;
> + int ret;
> +
> + ret = bq25790_get_state(bq, );
> + if (ret < 0)
> + goto irq_out;
> +
> + if (!bq25790_state_changed(bq, ))
You will be waking up user-space on every voltage or current change.
It was expressed on the lists that this is not desired and instead you
should notify only on change of important attributes (e.g. SoC, charge
status, cable status).
> + goto irq_out;
> +
> + mutex_lock(>lock);
> + bq->state = state;
> + mutex_unlock(>lock);
> +
> + power_supply_changed(bq->charger);
> +
> +irq_out:
> + return IRQ_HANDLED;
> +}
> +
(...)
> +
> +static int bq25790_parse_dt(struct bq25790_device *bq,
> + struct power_supply_config *psy_cfg, struct device *dev)
> +{
> + int ret = 0;
> +
> + psy_cfg->drv_data = bq;
> + psy_cfg->of_node = dev->of_node;
You parse here DT, so don't initialize power supply config in the same
time. It's mixing two things in the same function.
> +
> + ret = device_property_read_u32(bq->dev, "ti,watchdog-timeout-ms",
> + >watchdog_timer);
> + if (ret)
> + bq->watchdog_timer = BQ25790_WATCHDOG_DIS;
> +
> + if (bq->watchdog_timer > BQ25790_WATCHDOG_MAX ||
> + bq->watchdog_timer < BQ25790_WATCHDOG_DIS)
> + return -EINVAL;
> +
> + ret = device_property_read_u32(bq->dev,
> + "input-voltage-limit-microvolt",
> + >init_data.vlim);
> + if (ret)
> + bq->init_data.vlim = BQ25790_VINDPM_DEF_uV;
> +
> + if (bq->init_data.vlim > BQ25790_VINDPM_V_MAX_uV ||
> + bq->init_data.vlim < BQ25790_VINDPM_V_MIN_uV)
> + return -EINVAL;
> +
> + ret = device_property_read_u32(bq->dev,
> + "input-current-limit-microamp",
> + >init_data.ilim);
> + if (ret)
> + bq->init_data.ilim = BQ25790_IINDPM_DEF_uA;
> +
> + if (bq->init_data.ilim > BQ25790_IINDPM_I_MAX_uA ||
> + bq->init_data.ilim < BQ25790_IINDPM_I_MIN_uA)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int bq25790_probe(struct i2c_client *client,
> +const struct i2c_device_id *id)
> +{
> + struct device *dev = >dev;
> + struct bq25790_device *bq;
> + struct power_supply_config psy_cfg = { };
> +
> + int ret;
> +
> + bq = devm_kzalloc(dev, sizeof(*bq), GFP_KERNEL);
> + if (!bq)
> + return -ENOMEM;
> +
> + bq->client = client;
> + bq->dev = dev;
> +
> + mutex_init(>lock);
> +
> + strncpy(bq->model_name, id->name, I2C_NAME_SIZE);
> +
> + bq->regmap = devm_regmap_init_i2c(client, _regmap_config);
> +
Don't add blank line after every statement. All four blank lines above
should be removed.
> + 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 = bq25790_parse_dt(bq, _cfg, dev);
> + if (ret) {
> + dev_err(dev, "Failed to read device tree properties%d\n",
> ret);
> + return ret;
> + }
> +
> + ret = devm_add_action_or_reset(dev, bq25790_charger_reset, bq);
> + if (ret)
> + return ret;
> +
> + /* OTG reporting */
> + bq->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> + if (!IS_ERR_OR_NULL(bq->usb2_phy)) {
> + INIT_WORK(>usb_work, bq25790_usb_work);
> + bq->usb_nb.notifier_call = bq25790_usb_notifier;
> + usb_register_notifier(bq->usb2_phy, >usb_nb);
Where is the error checking? Where is cleanup in remove()?
> + }
> +
> + bq->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> + if (!IS_ERR_OR_NULL(bq->usb3_phy)) {
>
[PATCH v5 2/2] power: supply: bq25790: Introduce the BQ25790 charger driver
From: Dan Murphy
BQ25790 is a highly integrated switch-mode buck-boost charger
for 1-4 cell Li-ion battery and Li-polymer battery.
Signed-off-by: Ricardo Rivera-Matos
Signed-off-by: Dan Murphy
---
drivers/power/supply/Kconfig |8 +
drivers/power/supply/Makefile |1 +
drivers/power/supply/bq25790_charger.c | 1100
drivers/power/supply/bq25790_charger.h | 148
4 files changed, 1257 insertions(+)
create mode 100644 drivers/power/supply/bq25790_charger.c
create mode 100644 drivers/power/supply/bq25790_charger.h
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 006b95eca673..d0ecbe76b720 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -638,6 +638,14 @@ config CHARGER_BQ2515X
rail, ADC for battery and system monitoring, and push-button
controller.
+config CHARGER_BQ25790
+ tristate "TI BQ25790 battery charger driver"
+ depends on I2C
+ depends on GPIOLIB || COMPILE_TEST
+ select REGMAP_I2C
+ help
+ Say Y to enable support for the TI BQ25790 battery charger.
+
config CHARGER_BQ25890
tristate "TI BQ25890 battery charger driver"
depends on I2C
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 5e5fdbbef531..fa18d4fa9e6a 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -84,6 +84,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_BQ2515X) += bq2515x_charger.o
+obj-$(CONFIG_CHARGER_BQ25790) += bq25790_charger.o
obj-$(CONFIG_CHARGER_BQ25890) += bq25890_charger.o
obj-$(CONFIG_CHARGER_BQ25980) += bq25980_charger.o
obj-$(CONFIG_CHARGER_BQ256XX) += bq256xx_charger.o
diff --git a/drivers/power/supply/bq25790_charger.c
b/drivers/power/supply/bq25790_charger.c
new file mode 100644
index ..2a2305b985df
--- /dev/null
+++ b/drivers/power/supply/bq25790_charger.c
@@ -0,0 +1,1100 @@
+// SPDX-License-Identifier: GPL-2.0
+// BQ25790 driver
+// Copyright (C) 2020 Texas Instruments Incorporated - http://www.ti.com/
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#include "bq25790_charger.h"
+
+#define BQ25790_NUM_WD_VAL 8
+
+struct bq25790_init_data {
+ u32 ichg;
+ u32 ilim;
+ u32 vreg;
+ u32 iterm;
+ u32 iprechg;
+ u32 vlim;
+ u32 ichg_max;
+ u32 vreg_max;
+};
+
+struct bq25790_state {
+ bool online;
+ u8 chrg_status;
+ u8 chrg_type;
+ u8 health;
+ u8 chrg_fault;
+ u8 vsys_status;
+ u8 vbus_status;
+ u8 fault_0;
+ u8 fault_1;
+ u32 vbat_adc;
+ u32 vbus_adc;
+ u32 ibat_adc;
+};
+
+enum bq25790_id {
+ BQ25790,
+ BQ25792,
+};
+
+struct bq25790_device {
+ struct i2c_client *client;
+ struct device *dev;
+ struct power_supply *charger;
+ struct power_supply *battery;
+ struct mutex lock;
+
+ struct usb_phy *usb2_phy;
+ struct usb_phy *usb3_phy;
+ struct notifier_block usb_nb;
+ struct work_struct usb_work;
+ unsigned long usb_event;
+ struct regmap *regmap;
+
+ char model_name[I2C_NAME_SIZE];
+ int device_id;
+
+ struct bq25790_init_data init_data;
+ struct bq25790_state state;
+ int watchdog_timer;
+};
+
+static struct reg_default bq25790_reg_defs[] = {
+ {BQ25790_INPUT_V_LIM, 0x24},
+ {BQ25790_INPUT_I_LIM_MSB, 0x01},
+ {BQ25790_INPUT_I_LIM_LSB, 0x2c},
+ {BQ25790_PRECHRG_CTRL, 0xc3},
+ {BQ25790_TERM_CTRL, 0x5},
+ {BQ25790_VOTG_REG, 0xdc},
+ {BQ25790_IOTG_REG, 0x4b},
+ {BQ25790_TIMER_CTRL, 0x3d},
+ {BQ25790_CHRG_CTRL_0, 0xa2},
+ {BQ25790_CHRG_CTRL_1, 0x85},
+ {BQ25790_CHRG_CTRL_2, 0x40},
+ {BQ25790_CHRG_CTRL_3, 0x12},
+ {BQ25790_CHRG_CTRL_5, 0x16},
+ {BQ25790_MPPT_CTRL, 0xaa},
+ {BQ25790_TEMP_CTRL, 0xc0},
+ {BQ25790_NTC_CTRL_0, 0x7a},
+ {BQ25790_NTC_CTRL_1, 0x54},
+ {BQ25790_ICO_I_LIM, 0x0},
+ {BQ25790_CHRG_STAT_0, 0x0},
+ {BQ25790_CHRG_STAT_1, 0x0},
+ {BQ25790_CHRG_STAT_2, 0x0},
+ {BQ25790_CHRG_STAT_3, 0x0},
+ {BQ25790_CHRG_STAT_4, 0x0},
+ {BQ25790_FAULT_STAT_0, 0x0},
+ {BQ25790_FAULT_STAT_1, 0x0},
+ {BQ25790_CHRG_FLAG_0, 0x0},
+ {BQ25790_CHRG_FLAG_1, 0x0},
+ {BQ25790_CHRG_FLAG_2, 0x0},
+ {BQ25790_CHRG_FLAG_3, 0x0},
+ {BQ25790_FAULT_FLAG_0, 0x0},
+ {BQ25790_FAULT_FLAG_1, 0x0},
+ {BQ25790_CHRG_MSK_0, 0x0},
+ {BQ25790_CHRG_MSK_1, 0x0},
+ {BQ25790_CHRG_MSK_2, 0x0},
+ {BQ25790_CHRG_MSK_3, 0x0},
+ {BQ25790_FAULT_MSK_0, 0x0},
+ {BQ25790_FAULT_MSK_1, 0x0},
+ {BQ25790_ADC_CTRL, 0x30},
+
