Hi Andreas, I went on a unplanned leave and I came back to office recently. I will go through your comments and get back to you.
> Subject: Re: [RFC PATCH] power: bq24261_charger: Add support for TI BQ24261 > charger > > Hi, > > On Tue, Aug 18, 2015 at 11:19:35PM +0530, Ramakrishna Pallala wrote: > > Add new charger driver support for BQ24261 charger IC. > > > > Signed-off-by: Ramakrishna Pallala <ramakrishna.pall...@intel.com> > > --- > > drivers/power/Kconfig | 6 + > > drivers/power/Makefile | 1 + > > drivers/power/bq24261_charger.c | 1127 > +++++++++++++++++++++++++++++++++ > > include/linux/power/bq24261_charger.h | 26 + > > 4 files changed, 1160 insertions(+) > > create mode 100644 drivers/power/bq24261_charger.c create mode > > 100644 include/linux/power/bq24261_charger.h . . . > Thanks Ram for submitting your driver. I think it's a good base to merge my > (unplished) work supporting the bq24261M and bq24262. > > Laurentiu and I were having an offline discussion whether to make the above > constant charge current / voltage writable through sysfs on the > bq24257_charger.c driver as I'm merging my bq24250/bq24251 work there. > While helpful for debugging and to empower certain user-space applications > those properties also are somewhat dangerous to expose at the same time if an > unskilled user gets hold of them... > > (Thanks Laurentiu for digging up below thread) > http://marc.info/?l=linux-pm&m=143080413218161&w=2 > > In this context I was thinking, what about introducing a DT property for this > and > other charger drivers called "batt_param_write_enable" that by default is OFF > and controls the writability of above two parameters? I think this would add > one > layer of safety while at the same time allowing more flexibility for where > it's > needed. > > (more comments below) > > > + > > +static enum power_supply_property bq24261_usb_props[] = { > > + POWER_SUPPLY_PROP_PRESENT, > > + POWER_SUPPLY_PROP_ONLINE, > > + POWER_SUPPLY_PROP_TYPE, > > + POWER_SUPPLY_PROP_HEALTH, > > + POWER_SUPPLY_PROP_STATUS, > > + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX, > > + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX, > > + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT, > > + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE, > > + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, > > + POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT, > > + POWER_SUPPLY_PROP_TEMP_MAX, > > + POWER_SUPPLY_PROP_TEMP_MIN, > > +}; > > + > > +static char *bq24261_charger_supplied_to[] = { > > + "main-battery", > > +}; > > + > > +static struct power_supply_desc bq24261_charger_desc = { > > + .name = DEV_NAME, > > + .type = POWER_SUPPLY_TYPE_USB, > > + .properties = bq24261_usb_props, > > + .num_properties = ARRAY_SIZE(bq24261_usb_props), > > + .get_property = bq24261_usb_get_property, > > + .set_property = bq24261_usb_set_property, > > + .property_is_writeable = bq24261_property_is_writeable, > > +}; > > + > > +static void bq24261_wdt_reset_worker(struct work_struct *work) { > > + > > + struct bq24261_charger *chip = container_of(work, > > + struct bq24261_charger, wdt_work.work); > > + int ret; > > + > > + ret = bq24261_reset_wdt_timer(chip); > > + if (ret) > > + dev_err(&chip->client->dev, "WDT timer reset error(%d)\n", > ret); > > + > > + schedule_delayed_work(&chip->wdt_work, WDT_RESET_DELAY); } > > + > > +static void bq24261_irq_worker(struct work_struct *work) { > > + struct bq24261_charger *chip = > > + container_of(work, struct bq24261_charger, irq_work); > > + int ret; > > + > > + /* > > + * Lock to ensure that interrupt register readings are done > > + * and processed sequentially. The interrupt Fault registers > > + * are read on clear and without sequential processing double > > + * fault interrupts or fault recovery cannot be handlled propely > > + */ > > + > > + mutex_lock(&chip->lock); > > + > > + ret = bq24261_read_reg(chip->client, BQ24261_STAT_CTRL0_ADDR); > > + if (ret < 0) { > > + dev_err(&chip->client->dev, > > + "Error (%d) in reading BQ24261_STAT_CTRL0_ADDR\n", > ret); > > + goto irq_out; > > + } > > + > > + if (!chip->cable.boost) { > > + bq24261_handle_status(chip, ret); > > + bq24261_handle_health(chip, ret); > > + power_supply_changed(chip->psy_usb); > > + } > > + > > +irq_out: > > + mutex_unlock(&chip->lock); > > +} > > + > > +static irqreturn_t bq24261_thread_handler(int id, void *data) { > > + struct bq24261_charger *chip = (struct bq24261_charger *)data; > > + > > + queue_work(system_highpri_wq, &chip->irq_work); > > + return IRQ_HANDLED; > > +} > > + > > +static void bq24261_fault_mon_work(struct work_struct *work) { > > + struct bq24261_charger *chip = container_of(work, > > + struct bq24261_charger, fault_mon_work.work); > > + int ret; > > + > > + if ((chip->chg_health == POWER_SUPPLY_HEALTH_OVERVOLTAGE) || > > + (chip->chg_health == POWER_SUPPLY_HEALTH_DEAD)) { > > + > > + mutex_lock(&chip->lock); > > + ret = bq24261_read_reg(chip->client, > BQ24261_STAT_CTRL0_ADDR); > > + if (ret < 0) { > > + dev_err(&chip->client->dev, > > + "Status register read failed(%d)\n", ret); > > + goto fault_mon_out; > > + } > > + > > + if ((ret & BQ24261_STAT_MASK) == BQ24261_STAT_READY) { > > + dev_info(&chip->client->dev, > > + "Charger fault recovered\n"); > > + bq24261_handle_status(chip, ret); > > + bq24261_handle_health(chip, ret); > > + power_supply_changed(chip->psy_usb); > > + } > > +fault_mon_out: > > + mutex_unlock(&chip->lock); > > + } > > +} > > + > > +static void bq24261_boost_control(struct bq24261_charger *chip, bool > > +enable) { > > + int ret; > > + > > + if (enable) > > + ret = bq24261_write_reg(chip->client, > BQ24261_STAT_CTRL0_ADDR, > > + BQ24261_TMR_RST | > BQ24261_ENABLE_BOOST); > > + else > > + ret = bq24261_write_reg(chip->client, > > + BQ24261_STAT_CTRL0_ADDR, > 0x0); > > + > > + if (ret < 0) > > + dev_err(&chip->client->dev, > > + "stat cntl0 reg access error(%d)\n", ret); } > > + > > +static void bq24261_extcon_event_work(struct work_struct *work) { > > + struct bq24261_charger *chip = > > + container_of(work, struct bq24261_charger, > cable.work); > > + int ret, current_limit = 0; > > + bool old_connected = chip->cable.connected; > > + > > + /* Determine cable/charger type */ > > + if (extcon_get_cable_state(chip->cable.sdp.edev, > > + "SLOW-CHARGER") > 0) { > > + chip->cable.connected = true; > > + current_limit = ILIM_500MA; > > + chip->cable.chg_type = POWER_SUPPLY_TYPE_USB; > > + dev_dbg(&chip->client->dev, "USB SDP charger is connected"); > > + } else if (extcon_get_cable_state(chip->cable.cdp.edev, > > + "CHARGE-DOWNSTREAM") > 0) { > > + chip->cable.connected = true; > > + current_limit = ILIM_1500MA; > > + chip->cable.chg_type = POWER_SUPPLY_TYPE_USB_CDP; > > + dev_dbg(&chip->client->dev, "USB CDP charger is connected"); > > + } else if (extcon_get_cable_state(chip->cable.dcp.edev, > > + "FAST-CHARGER") > 0) { > > + chip->cable.connected = true; > > + current_limit = ILIM_1500MA; > > + chip->cable.chg_type = POWER_SUPPLY_TYPE_USB_DCP; > > + dev_dbg(&chip->client->dev, "USB DCP charger is connected"); > > + } else if (extcon_get_cable_state(chip->cable.otg.edev, > > + "USB-Host") > 0) { > > + chip->cable.boost = true; > > + chip->cable.connected = true; > > + dev_dbg(&chip->client->dev, "USB-Host cable is connected"); > > + } else { > > + if (old_connected) > > + dev_dbg(&chip->client->dev, "USB Cable > disconnected"); > > + chip->cable.connected = false; > > + chip->cable.boost = false; > > + chip->cable.chg_type = POWER_SUPPLY_TYPE_USB; > > + } > > + > > + /* Cable status changed */ > > + if (old_connected == chip->cable.connected) > > + return; > > + > > + mutex_lock(&chip->lock); > > + if (chip->cable.connected && !chip->cable.boost) { > > + chip->inlmt = current_limit; > > + /* Set up charging */ > > + ret = bq24261_set_cc(chip, chip->cc); > > + if (ret < 0) > > + dev_err(&chip->client->dev, "set CC failed(%d)", ret); > > + ret = bq24261_set_cv(chip, chip->cv); > > + if (ret < 0) > > + dev_err(&chip->client->dev, "set CV failed(%d)", ret); > > + ret = bq24261_set_inlmt(chip, chip->inlmt); > > + if (ret < 0) > > + dev_err(&chip->client->dev, "set ILIM failed(%d)", ret); > > + ret = bq24261_enable_charger(chip, true); > > + if (ret < 0) > > + dev_err(&chip->client->dev, > > + "enable charger failed(%d)", ret); > > + ret = bq24261_enable_charging(chip, true); > > + if (ret < 0) > > + dev_err(&chip->client->dev, > > + "enable charging failed(%d)", ret); > > + > > + chip->is_charging_enabled = true; > > + chip->present = true; > > + chip->online = true; > > + schedule_delayed_work(&chip->wdt_work, 0); > > + } else if (chip->cable.connected && chip->cable.boost) { > > + /* Enable VBUS for Host Mode */ > > + bq24261_boost_control(chip, true); > > + schedule_delayed_work(&chip->wdt_work, 0); > > + } else { > > + dev_info(&chip->client->dev, "Cable disconnect event\n"); > > + cancel_delayed_work_sync(&chip->wdt_work); > > + cancel_delayed_work_sync(&chip->fault_mon_work); > > + bq24261_boost_control(chip, false); > > + ret = bq24261_enable_charging(chip, false); > > + if (ret < 0) > > + dev_err(&chip->client->dev, > > + "charger disable failed(%d)", ret); > > + > > + chip->is_charging_enabled = false; > > + chip->present = false; > > + chip->online = false; > > + chip->inlmt = 0; > > + } > > + bq24261_charger_desc.type = chip->cable.chg_type; > > + mutex_unlock(&chip->lock); > > + power_supply_changed(chip->psy_usb); > > +} > > + > > +static int bq24261_handle_extcon_events(struct notifier_block *nb, > > + unsigned long event, void *param) { > > + struct bq24261_charger *chip = > > + container_of(nb, struct bq24261_charger, cable.nb); > > + > > + dev_dbg(&chip->client->dev, "external connector event(%ld)\n", > > +event); > > + > > + schedule_work(&chip->cable.work); > > + return NOTIFY_OK; > > +} > > + > > +static int bq24261_extcon_register(struct bq24261_charger *chip) { > > + int ret; > > + > > + INIT_WORK(&chip->cable.work, bq24261_extcon_event_work); > > + chip->cable.nb.notifier_call = bq24261_handle_extcon_events; > > + > > + ret = extcon_register_interest(&chip->cable.sdp, NULL, > > + "SLOW-CHARGER", &chip->cable.nb); > > + if (ret < 0) { > > + dev_warn(&chip->client->dev, > > + "extcon SDP registration failed(%d)\n", ret); > > + goto sdp_reg_failed; > > + } > > + > > + ret = extcon_register_interest(&chip->cable.cdp, NULL, > > + "CHARGE-DOWNSTREAM", &chip->cable.nb); > > + if (ret < 0) { > > + dev_warn(&chip->client->dev, > > + "extcon CDP registration failed(%d)\n", ret); > > + goto cdp_reg_failed; > > + } > > + > > + ret = extcon_register_interest(&chip->cable.dcp, NULL, > > + "FAST-CHARGER", &chip->cable.nb); > > + if (ret < 0) { > > + dev_warn(&chip->client->dev, > > + "extcon DCP registration failed(%d)\n", ret); > > + goto dcp_reg_failed; > > + } > > + > > + ret = extcon_register_interest(&chip->cable.otg, NULL, > > + "USB-Host", &chip->cable.nb); > > + if (ret < 0) { > > + dev_warn(&chip->client->dev, > > + "extcon USB-Host registration failed(%d)\n", ret); > > + goto otg_reg_failed; > > + } > > + > > + return 0; > > + > > +otg_reg_failed: > > + extcon_unregister_interest(&chip->cable.dcp); > > +dcp_reg_failed: > > + extcon_unregister_interest(&chip->cable.cdp); > > +cdp_reg_failed: > > + extcon_unregister_interest(&chip->cable.sdp); > > +sdp_reg_failed: > > + return -EPROBE_DEFER; > > +} > > + > > +static int bq24261_get_model(struct i2c_client *client, > > + enum bq2426x_model *model) > > +{ > > + int ret; > > + > > + ret = bq24261_read_reg(client, BQ24261_VENDOR_REV_ADDR); > > + if (ret < 0) > > + return ret; > > + > > + if ((ret & BQ24261_VENDOR_MASK) != VENDOR_BQ2426X) > > + return -EINVAL; > > + > > + switch (ret & BQ24261_REV_MASK) { > > + case REV_BQ24261: > > + *model = BQ24261; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +static int bq24261_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent); > > + struct power_supply_config charger_cfg = {}; > > + struct bq24261_charger *chip; > > + int ret; > > + enum bq2426x_model model; > > + > > + adapter = to_i2c_adapter(client->dev.parent); > > + > > + if (!client->dev.platform_data && !id) { > > + dev_err(&client->dev, "platform data is null"); > > + return -EFAULT; > > + } > > + > > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) { > > + dev_err(&client->dev, > > + "I2C adapter %s doesn'tsupport BYTE DATA transfer\n", > > + adapter->name); > > + return -EIO; > > + } > > + > > + ret = bq24261_get_model(client, &model); > > + if (ret < 0) { > > + dev_err(&client->dev, "chip detection error (%d)\n", ret); > > + return -ENODEV; > > + } > > + > > + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL); > > + if (!chip) > > + return -ENOMEM; > > + > > + chip->client = client; > > + if (client->dev.platform_data) > > + chip->pdata = client->dev.platform_data; > > + else > > + chip->pdata = (struct bq24261_platform_data *)id->driver_data; > > + i2c_set_clientdata(client, chip); > > + mutex_init(&chip->lock); > > + chip->model = model; > > + > > + /* Initialize charger parameters */ > > + chip->cc = chip->pdata->def_cc; > > + chip->cv = chip->pdata->def_cv; > > + chip->iterm = chip->pdata->iterm; > > + chip->chg_status = BQ24261_CHRGR_STAT_UNKNOWN; > > + chip->chg_health = POWER_SUPPLY_HEALTH_UNKNOWN; > > + > > + charger_cfg.drv_data = chip; > > + charger_cfg.supplied_to = bq24261_charger_supplied_to; > > + charger_cfg.num_supplicants = > ARRAY_SIZE(bq24261_charger_supplied_to); > > + chip->psy_usb = power_supply_register(&client->dev, > > + &bq24261_charger_desc, &charger_cfg); > > + if (IS_ERR(chip->psy_usb)) { > > + dev_err(&client->dev, > > + "power supply registration failed(%d)\n", ret); > > + return ret; > > + } > > + > > + INIT_DELAYED_WORK(&chip->wdt_work, bq24261_wdt_reset_worker); > > + INIT_DELAYED_WORK(&chip->fault_mon_work, > bq24261_fault_mon_work); > > + > > + ret = bq24261_extcon_register(chip); > > + if (ret < 0) > > + goto extcon_reg_failed; > > + > > + if (chip->client->irq) { > > + ret = request_threaded_irq(chip->client->irq, > > + NULL, bq24261_thread_handler, > > + IRQF_SHARED | IRQF_NO_SUSPEND, > > + DEV_NAME, chip); > > + if (ret) { > > + dev_err(&client->dev, > > + "irq request failed (%d)\n", ret); > > + goto irq_reg_failed; > > + } > > + INIT_WORK(&chip->irq_work, bq24261_irq_worker); > > + } > > + > > + /* Check for charger connecetd boot case */ > > + schedule_work(&chip->cable.work); > > + > > + return 0; > > + > > +irq_reg_failed: > > + extcon_unregister_interest(&chip->cable.sdp); > > + extcon_unregister_interest(&chip->cable.cdp); > > + extcon_unregister_interest(&chip->cable.dcp); > > + extcon_unregister_interest(&chip->cable.otg); > > +extcon_reg_failed: > > + power_supply_unregister(chip->psy_usb); > > + return ret; > > +} > > + > > +static int bq24261_remove(struct i2c_client *client) { > > + struct bq24261_charger *chip = i2c_get_clientdata(client); > > + > > + free_irq(client->irq, chip); > > + flush_scheduled_work(); > > + extcon_unregister_interest(&chip->cable.sdp); > > + extcon_unregister_interest(&chip->cable.cdp); > > + extcon_unregister_interest(&chip->cable.dcp); > > + extcon_unregister_interest(&chip->cable.otg); > > + power_supply_unregister(chip->psy_usb); > > + return 0; > > +} > > + > > +static int bq24261_suspend(struct device *dev) { > > + struct bq24261_charger *chip = dev_get_drvdata(dev); > > + > > + dev_dbg(&chip->client->dev, "bq24261 suspend\n"); > > + return 0; > > +} > > + > > +static int bq24261_resume(struct device *dev) { > > + struct bq24261_charger *chip = dev_get_drvdata(dev); > > + > > + dev_dbg(&chip->client->dev, "bq24261 resume\n"); > > + return 0; > > +} > > + > > +static SIMPLE_DEV_PM_OPS(bq24261_pm_ops, bq24261_suspend, > > + bq24261_resume); > > + > > +static const struct i2c_device_id bq24261_id[] = { > > + {DEV_NAME, 0}, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(i2c, bq24261_id); > > + > > +static struct i2c_driver bq24261_driver = { > > + .driver = { > > + .name = DEV_NAME, > > + .pm = &bq24261_pm_ops, > > + }, > > + .probe = bq24261_probe, > > + .remove = bq24261_remove, > > + .id_table = bq24261_id, > > I just noticed this driver doesn't yet support DT which is probably something > that > should be added. When I start merging my work I will certainly need to do that > but I'm curious if there are plans from your end to add this as well (and I > had > seen Laurentiu's bq24257_charger.c driver uses ACPI for this which seems to be > some kind of superset of DT > - forgive my simplification I'm not too familiar with ACPI yet). > > -- > Andreas Dannenberg > Texas Instruments > > > +}; > > + > > +module_i2c_driver(bq24261_driver); > > + > > +MODULE_AUTHOR("Jenny TC <jenny...@intel.com>"); > > +MODULE_AUTHOR("Ramakrishna Pallala <ramakrishna.pall...@intel.com>"); > > +MODULE_DESCRIPTION("BQ24261 Charger Driver"); MODULE_LICENSE("GPL > > +v2"); > > diff --git a/include/linux/power/bq24261_charger.h > > b/include/linux/power/bq24261_charger.h > > new file mode 100644 > > index 0000000..656db58 > > --- /dev/null > > +++ b/include/linux/power/bq24261_charger.h > > @@ -0,0 +1,26 @@ > > +/* > > + * bq24261_charger.h: platform data structure for bq24261 driver > > + * > > + * Copyright (C) 2014 Intel Corporation > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * as published by the Free Software Foundation; version 2 > > + * of the License. > > + */ > > + > > +#ifndef __BQ24261_CHARGER_H__ > > +#define __BQ24261_CHARGER_H__ > > + > > +struct bq24261_platform_data { > > + int def_cc; > > + int def_cv; > > + int iterm; > > + int max_cc; > > + int max_cv; > > + int min_temp; > > + int max_temp; > > + bool thermal_sensing; > > +}; > > + > > +#endif > > -- > > 1.7.9.5 Thanks, Ram -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
