Re: [PATCH 2/2] power_supply: Add support for TI BQ25890 charger chip

From: Krzysztof Kozlowski
Date: Tue May 19 2015 - 04:40:42 EST


2015-05-15 23:06 GMT+09:00 Laurentiu Palcu <laurentiu.palcu@xxxxxxxxx>:
> More details about the chip can be found here:
> http://www.ti.com/product/bq25890
>
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@xxxxxxxxx>
> ---
> drivers/power/Kconfig | 7 +
> drivers/power/Makefile | 1 +
> drivers/power/bq25890_charger.c | 998 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1006 insertions(+)
> create mode 100644 drivers/power/bq25890_charger.c

Hi,

Some comments (nothing serious) inline.

(...)

> +
> +static int bq25890_field_write(struct bq25890_device *bq,
> + enum bq25890_fields field_id, u8 val)
> +{
> + return regmap_field_write(bq->rmap_fields[field_id], val);
> +}
> +
> +static u8 bq25890_find_idx(u32 value, enum bq25890_table_ids id)
> +{
> + u8 idx;
> +
> + if (id >= TBL_TREG) {
> + const u32 *tbl = bq25890_tables[id].lt.tbl;
> + u32 tbl_size = bq25890_tables[id].lt.size;
> +
> + for (idx = 1; idx < tbl_size && tbl[idx] <= value; idx++)
> + ;
> + } else {
> + const struct bq25890_range *rtbl = &bq25890_tables[id].rt;
> + u8 rtbl_size;
> +
> + rtbl_size = (rtbl->max - rtbl->min) / rtbl->step + 1;
> +
> + for (idx = 1;
> + idx < rtbl_size && idx * rtbl->step + rtbl->min <= value;

Could you add parentheses around part of this expression? It is non
obvious to find the comparison statements.

> + idx++)
> + ;
> + }
> +
> + return idx - 1;
> +}
> +
> +static u32 bq25890_find_val(u8 idx, enum bq25890_table_ids id)
> +{
> + const struct bq25890_range *rtbl;
> +
> + /* lookup table? */
> + if (id >= TBL_TREG)
> + return bq25890_tables[id].lt.tbl[idx];
> +
> + /* range table */
> + rtbl = &bq25890_tables[id].rt;
> +
> + return(rtbl->min + idx * rtbl->step);

A nit: space between return and parenthesis would be nice.

> +}
> +
> +enum bq25890_status {
> + STATUS_NOT_CHARGING,
> + STATUS_PRE_CHARGING,
> + STATUS_FAST_CHARGING,
> + STATUS_TERMINATION_DONE,
> +};
> +
> +enum bq25890_chrg_fault {
> + CHRG_FAULT_NORMAL,
> + CHRG_FAULT_INPUT,
> + CHRG_FAULT_THERMAL_SHUTDOWN,
> + CHRG_FAULT_TIMER_EXPIRED,
> +};
> +
> +static int bq25890_power_supply_get_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + int ret;
> + struct bq25890_device *bq = power_supply_get_drvdata(psy);
> + struct bq25890_state state;
> +
> + mutex_lock(&bq->lock);
> + state = bq->state;
> + mutex_unlock(&bq->lock);
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_STATUS:
> + if (!state.online)
> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> + else if (state.chrg_status == STATUS_NOT_CHARGING)
> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> + else if (state.chrg_status == STATUS_PRE_CHARGING ||
> + state.chrg_status == STATUS_FAST_CHARGING)
> + val->intval = POWER_SUPPLY_STATUS_CHARGING;
> + else if (state.chrg_status == STATUS_TERMINATION_DONE)
> + val->intval = POWER_SUPPLY_STATUS_FULL;
> + else
> + val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> +
> + break;
> +
> + case POWER_SUPPLY_PROP_MANUFACTURER:
> + val->strval = BQ25890_MANUFACTURER;
> + break;
> +
> + case POWER_SUPPLY_PROP_ONLINE:
> + val->intval = state.online;
> + break;
> +
> + case POWER_SUPPLY_PROP_HEALTH:
> + if (!state.chrg_fault && !state.bat_fault && !state.boost_fault)
> + val->intval = POWER_SUPPLY_HEALTH_GOOD;
> + else if (state.bat_fault)
> + val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> + else if (state.chrg_fault == CHRG_FAULT_TIMER_EXPIRED)
> + val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
> + else if (state.chrg_fault == CHRG_FAULT_THERMAL_SHUTDOWN)
> + val->intval = POWER_SUPPLY_HEALTH_OVERHEAT;
> + else
> + val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> + break;
> +
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> + ret = bq25890_field_read(bq, F_ICHGR); /* read measured value */
> + if (ret < 0)
> + return ret;
> +
> + /* converted_val = ADC_val * 50mA (table 10.3.19) */
> + val->intval = ret * 50000;
> + break;
> +
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> + val->intval = bq25890_tables[TBL_ICHG].rt.max;
> + break;
> +
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> + if (!state.online) {
> + val->intval = 0;
> + break;
> + }
> +
> + ret = bq25890_field_read(bq, F_BATV); /* read measured value */
> + if (ret < 0)
> + return ret;
> +
> + /* converted_val = 2.304V + ADC_val * 20mV (table 10.3.15) */
> + val->intval = 2304000 + ret * 20000;
> + break;
> +
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> + val->intval = bq25890_tables[TBL_VREG].rt.max;
> + break;
> +
> + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
> + val->intval = bq25890_find_val(bq->init_data.iterm, TBL_ITERM);
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int bq25890_get_chip_state(struct bq25890_device *bq,
> + struct bq25890_state *state)
> +{
> + int i, ret;
> +
> + struct {
> + enum bq25890_fields id;
> + u8 *data;
> + } state_fields[] = {
> + {F_CHG_STAT, &state->chrg_status},
> + {F_PG_STAT, &state->online},
> + {F_VSYS_STAT, &state->vsys_status},
> + {F_BOOST_FAULT, &state->boost_fault},
> + {F_BAT_FAULT, &state->bat_fault},
> + {F_CHG_FAULT, &state->chrg_fault}
> + };
> +
> + for (i = 0; i < ARRAY_SIZE(state_fields); i++) {
> + ret = bq25890_field_read(bq, state_fields[i].id);
> + if (ret < 0)
> + return ret;
> +
> + *state_fields[i].data = ret;
> + }
> +
> + dev_dbg(bq->dev, "S:CHG/PG/VSYS=%d/%d/%d, F:CHG/BOOST/BAT=%d/%d/%d\n",
> + state->chrg_status, state->online, state->vsys_status,
> + state->chrg_fault, state->boost_fault, state->bat_fault);
> +
> + return 0;
> +}
> +
> +static bool bq25890_state_changed(struct bq25890_device *bq,
> + struct bq25890_state *new_state)
> +{
> + struct bq25890_state old_state;
> +
> + mutex_lock(&bq->lock);
> + old_state = bq->state;
> + mutex_unlock(&bq->lock);
> +
> + return (old_state.chrg_status != new_state->chrg_status ||
> + old_state.chrg_fault != new_state->chrg_fault ||
> + old_state.online != new_state->online ||
> + old_state.bat_fault != new_state->bat_fault ||
> + old_state.boost_fault != new_state->boost_fault ||
> + old_state.vsys_status != new_state->vsys_status);
> +}
> +
> +static void bq25890_handle_state_change(struct bq25890_device *bq,
> + struct bq25890_state *new_state)
> +{
> + int ret;
> + struct bq25890_state old_state;
> +
> + mutex_lock(&bq->lock);
> + old_state = bq->state;
> + mutex_unlock(&bq->lock);
> +
> + if (!new_state->online) { /* power removed */
> + /* disable ADC */
> + ret = bq25890_field_write(bq, F_CONV_START, 0);
> + if (ret < 0)
> + goto error;
> + } else if (!old_state.online) { /* power inserted */
> + /* enable ADC, to have control of charge current/voltage */
> + ret = bq25890_field_write(bq, F_CONV_START, 1);
> + if (ret < 0)
> + goto error;
> + }
> +
> + return;
> +
> +error:
> + dev_err(bq->dev, "%s: Error communicating with the chip.\n", __func__);

The __func__ is not needed here, there is only one kind of such message.

> +}
> +
> +static irqreturn_t bq25890_irq_handler_thread(int irq, void *private)
> +{
> + struct bq25890_device *bq = private;
> + int ret;
> + struct bq25890_state state;
> +
> + ret = bq25890_get_chip_state(bq, &state);
> + if (ret < 0)
> + goto handled;
> +
> + if (!bq25890_state_changed(bq, &state))
> + goto handled;
> +
> + bq25890_handle_state_change(bq, &state);
> +
> + mutex_lock(&bq->lock);
> + bq->state = state;
> + mutex_unlock(&bq->lock);
> +
> + power_supply_changed(bq->charger);
> +
> +handled:
> + return IRQ_HANDLED;
> +}
> +
> +static int bq25890_chip_reset(struct bq25890_device *bq)
> +{
> + int ret;
> +
> + ret = bq25890_field_write(bq, F_REG_RST, 1);
> + if (ret < 0)
> + return ret;
> +
> + do {
> + ret = bq25890_field_read(bq, F_REG_RST);
> + if (ret < 0)
> + return ret;
> +
> + usleep_range(5, 10);
> + } while (ret == 1);

Is it possible to loop here indefinetely?

> +
> + return 0;
> +}
> +
> +static int bq25890_hw_init(struct bq25890_device *bq)
> +{
> + int ret;
> + int i;
> + struct bq25890_state state;
> +
> + const struct {
> + enum bq25890_fields id;
> + u32 value;
> + } init_data[] = {
> + {F_ICHG, bq->init_data.ichg},
> + {F_VREG, bq->init_data.vreg},
> + {F_ITERM, bq->init_data.iterm},
> + {F_IPRECHG, bq->init_data.iprechg},
> + {F_SYSVMIN, bq->init_data.sysvmin},
> + {F_BOOSTV, bq->init_data.boostv},
> + {F_BOOSTI, bq->init_data.boosti},
> + {F_BOOSTF, bq->init_data.boostf},
> + {F_EN_ILIM, bq->init_data.ilim_en},
> + {F_TREG, bq->init_data.treg}
> + };
> +
> + ret = bq25890_chip_reset(bq);
> + if (ret < 0)
> + return ret;
> +
> + /* disable watchdog */
> + ret = bq25890_field_write(bq, F_WD, 0);
> + if (ret < 0)
> + return ret;
> +
> + /* initialize currents/voltages and other parameters */
> + for (i = 0; i < ARRAY_SIZE(init_data); i++) {
> + ret = bq25890_field_write(bq, init_data[i].id,
> + init_data[i].value);
> + if (ret < 0)
> + return ret;
> + }
> +
> + /* Configure ADC for continuous conversions. This does not enable it. */
> + ret = bq25890_field_write(bq, F_CONV_RATE, 1);
> + if (ret < 0)
> + return ret;
> +
> + ret = bq25890_get_chip_state(bq, &state);
> + if (ret < 0)
> + return ret;
> +
> + mutex_lock(&bq->lock);
> + bq->state = state;
> + mutex_unlock(&bq->lock);
> +
> + return 0;
> +}
> +
> +static enum power_supply_property bq25890_power_supply_props[] = {
> + POWER_SUPPLY_PROP_MANUFACTURER,
> + POWER_SUPPLY_PROP_STATUS,
> + POWER_SUPPLY_PROP_ONLINE,
> + POWER_SUPPLY_PROP_HEALTH,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
> + POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
> +};
> +
> +static char *bq25890_charger_supplied_to[] = {
> + "main-battery",
> +};
> +
> +static const struct power_supply_desc bq25890_power_supply_desc = {
> + .name = "bq25890-charger",
> + .type = POWER_SUPPLY_TYPE_USB,
> + .properties = bq25890_power_supply_props,
> + .num_properties = ARRAY_SIZE(bq25890_power_supply_props),
> + .get_property = bq25890_power_supply_get_property,
> +};
> +
> +static int bq25890_power_supply_init(struct bq25890_device *bq)
> +{
> + struct power_supply_config psy_cfg = { .drv_data = bq, };
> +
> + psy_cfg.supplied_to = bq25890_charger_supplied_to;
> + psy_cfg.num_supplicants = ARRAY_SIZE(bq25890_charger_supplied_to);
> +
> + bq->charger = power_supply_register(bq->dev, &bq25890_power_supply_desc,
> + &psy_cfg);
> + if (IS_ERR(bq->charger))
> + return PTR_ERR(bq->charger);
> +
> + return 0;

return PTR_ERR_OR_ZERO

> +}
> +
> +static void bq25890_usb_work(struct work_struct *data)
> +{
> + int ret;
> + struct bq25890_device *bq =
> + container_of(data, struct bq25890_device, usb_work);
> +
> + switch (bq->usb_event) {
> + case USB_EVENT_ID:
> + /* Enable boost mode */
> + ret = bq25890_field_write(bq, F_OTG_CFG, 1);
> + if (ret < 0)
> + goto error;
> + break;
> +
> + case USB_EVENT_NONE:
> + /* Disable boost mode */
> + ret = bq25890_field_write(bq, F_OTG_CFG, 0);
> + if (ret < 0)
> + goto error;
> +
> + power_supply_changed(bq->charger);
> + break;
> + }
> +
> + return;
> +
> +error:
> + dev_err(bq->dev, "%s - Error switching to boost/charger mode.\n",
> + __func__);

Again the __func__. It shouldn't be here.

> +}
> +
> +static int bq25890_usb_notifier(struct notifier_block *nb, unsigned long val,
> + void *priv)
> +{
> + struct bq25890_device *bq =
> + container_of(nb, struct bq25890_device, usb_nb);
> +
> + bq->usb_event = val;
> + schedule_work(&bq->usb_work);

If this shouldn't be executed on exactly this CPU then usage of
system_power_efficient_wq is encouraged.

> +
> + return NOTIFY_OK;
> +}
> +
> +static int bq25890_irq_probe(struct bq25890_device *bq)
> +{
> + int ret;
> + struct gpio_desc *irq;
> +
> + irq = devm_gpiod_get_index(bq->dev, BQ25890_IRQ_PIN, 0);
> + if (IS_ERR(irq)) {
> + dev_err(bq->dev, "could not probe irq pin\n");
> + return PTR_ERR(irq);
> + }
> +
> + ret = gpiod_direction_input(irq);
> + if (ret < 0)
> + return ret;
> +
> + return gpiod_to_irq(irq);
> +}
> +
> +static int bq25890_fw_read_u32_props(struct bq25890_device *bq)
> +{
> + int ret;
> + u32 property;
> + int i;
> + struct bq25890_init_data *init = &bq->init_data;
> + struct {
> + char *name;
> + bool optional;
> + enum bq25890_table_ids tbl_id;
> + u8 *conv_data; /* holds converted value from given property */
> + } props[] = {
> + /* required properties */
> + {"ti,charge-current", false, TBL_ICHG, &init->ichg},
> + {"ti,battery-regulation-voltage", false, TBL_VREG, &init->vreg},
> + {"ti,termination-current", false, TBL_ITERM, &init->iterm},
> + {"ti,precharge-current", false, TBL_ITERM, &init->iprechg},
> + {"ti,minimum-sys-voltage", false, TBL_SYSVMIN, &init->sysvmin},
> + {"ti,boost-voltage", false, TBL_BOOSTV, &init->boostv},
> + {"ti,boost-max-current", false, TBL_BOOSTI, &init->boosti},
> +
> + /* optional properties */
> + {"ti,thermal-regulation-threshold", true, TBL_TREG, &init->treg}
> + };
> +
> + /* initialize data for optional properties */
> + init->treg = 3; /* 120 degrees Celsius */
> +
> + for (i = 0; i < ARRAY_SIZE(props); i++) {
> + ret = device_property_read_u32(bq->dev, props[i].name,
> + &property);
> + if (ret < 0) {
> + if (props[i].optional)
> + continue;
> +
> + return ret;
> + }
> +
> + *props[i].conv_data = bq25890_find_idx(property,
> + props[i].tbl_id);
> + }
> +
> + return 0;
> +}
> +
> +static int bq25890_fw_probe(struct bq25890_device *bq)
> +{
> + int ret;
> + struct bq25890_init_data *init = &bq->init_data;
> +
> + ret = bq25890_fw_read_u32_props(bq);
> + if (ret < 0)
> + return ret;
> +
> + init->ilim_en = device_property_read_bool(bq->dev, "ti,use-ilim-pin");
> + init->boostf = device_property_read_bool(bq->dev, "ti,boost-low-freq");
> +
> + return 0;
> +}
> +
> +static int bq25890_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> + struct device *dev = &client->dev;
> + struct bq25890_device *bq;
> + int ret;
> + int i;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> + dev_err(dev, "No support for SMBUS_BYTE_DATA\n");
> + return -ENODEV;
> + }
> +
> + bq = devm_kzalloc(dev, sizeof(*bq), GFP_KERNEL);
> + if (!bq)
> + return -ENOMEM;
> +
> + bq->client = client;
> + bq->dev = dev;
> +
> + mutex_init(&bq->lock);
> +
> + bq->rmap = devm_regmap_init_i2c(client, &bq25890_regmap_config);
> + if (IS_ERR(bq->rmap)) {
> + dev_err(dev, "failed to allocate register map\n");
> + return PTR_ERR(bq->rmap);
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(bq25890_reg_fields); i++) {
> + const struct reg_field *reg_fields = bq25890_reg_fields;
> +
> + bq->rmap_fields[i] = devm_regmap_field_alloc(dev, bq->rmap,
> + reg_fields[i]);
> + if (IS_ERR(bq->rmap_fields[i])) {
> + dev_err(dev, "cannot allocate regmap field\n");
> + return PTR_ERR(bq->rmap_fields[i]);
> + }
> + }
> +
> + i2c_set_clientdata(client, bq);
> +
> + bq->chip_id = bq25890_field_read(bq, F_PN);
> + if (bq->chip_id < 0) {
> + dev_err(dev, "Cannot read chip ID.\n");
> + return ret;
> + }
> +
> + if (bq->chip_id != BQ25890_ID) {
> + dev_err(dev, "Chip with ID=%d, not supported!\n", bq->chip_id);
> + return -ENODEV;
> + }
> +
> + if (!dev->platform_data) {
> + ret = bq25890_fw_probe(bq);
> + if (ret < 0) {
> + dev_err(dev, "Cannot read device properties.\n");
> + return ret;
> + }
> + } else {
> + return -ENODEV;
> + }
> +
> + ret = bq25890_hw_init(bq);
> + if (ret < 0) {
> + dev_err(dev, "Cannot initialize the chip.\n");
> + return ret;
> + }
> +
> + if (client->irq <= 0)
> + client->irq = bq25890_irq_probe(bq);
> +
> + if (client->irq < 0) {
> + dev_err(dev, "no irq resource found\n");

A nit: stick to one convention - capitilize first letter of error message.

> + return client->irq;
> + }
> +
> + /* OTG reporting */
> + bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> + if (!IS_ERR_OR_NULL(bq->usb_phy)) {
> + INIT_WORK(&bq->usb_work, bq25890_usb_work);
> + bq->usb_nb.notifier_call = bq25890_usb_notifier;
> + usb_register_notifier(bq->usb_phy, &bq->usb_nb);
> + }
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + bq25890_irq_handler_thread,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + BQ25890_IRQ_PIN, bq);
> + if (ret)
> + goto irq_fail;
> +
> + ret = bq25890_power_supply_init(bq);
> + if (ret < 0) {
> + dev_err(dev, "Failed to register power supply\n");
> + goto irq_fail;
> + }
> +
> + return 0;
> +
> +irq_fail:
> + if (!IS_ERR_OR_NULL(bq->usb_phy))
> + usb_unregister_notifier(bq->usb_phy, &bq->usb_nb);
> +
> + return ret;
> +}
> +
> +static int bq25890_remove(struct i2c_client *client)
> +{
> + struct bq25890_device *bq = i2c_get_clientdata(client);
> +
> + if (!IS_ERR_OR_NULL(bq->usb_phy))
> + usb_unregister_notifier(bq->usb_phy, &bq->usb_nb);
> +
> + power_supply_unregister(bq->charger);

I would prefer cleaning in reversed order of probe, so first would be
power_supply_unregister(). I think this is expected in usual case.
Often when I encounter such code I wonder - is this non-standard order
of cleanup is important?

> +
> + /* reset all registers to default values */
> + bq25890_chip_reset(bq);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int bq25890_suspend(struct device *dev)
> +{
> + struct bq25890_device *bq = dev_get_drvdata(dev);
> +
> + /*
> + * If charger is removed, while in suspend, make sure ADC is diabled
> + * since it consumes slightly more power.
> + */
> + return bq25890_field_write(bq, F_CONV_START, 0);
> +}
> +
> +static int bq25890_resume(struct device *dev)
> +{
> + int ret;
> + struct bq25890_state state;
> + struct bq25890_device *bq = dev_get_drvdata(dev);
> +
> + ret = bq25890_get_chip_state(bq, &state);
> + if (ret < 0)
> + return ret;
> +
> + mutex_lock(&bq->lock);
> + bq->state = state;
> + mutex_unlock(&bq->lock);
> +
> + /* Re-enable ADC only if charger is plugged in. */
> + if (bq->state.online) {

Shouldn't you be here accessing local variable state?

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/