From: Darbha Sriharsha <[email protected]>
Adding driver support for bq24735 charger chipset.
Signed-off-by: Darbha Sriharsha <[email protected]>
Signed-off-by: Rhyland Klein <[email protected]>
---
v2:
- gpio_request -> devm_gpio_request
.../bindings/power_supply/ti,bq24735.txt | 56 +++
drivers/power/Kconfig | 8 +
drivers/power/Makefile | 1 +
drivers/power/bq24735-charger.c | 384 ++++++++++++++++++++
include/linux/power/bq24735-charger.h | 68 ++++
5 files changed, 517 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power_supply/ti,bq24735.txt
create mode 100644 drivers/power/bq24735-charger.c
create mode 100644 include/linux/power/bq24735-charger.h
diff --git a/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt b/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt
new file mode 100644
index 0000000..19ea5a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt
@@ -0,0 +1,56 @@
+TI BQ24735 Charge Controller
+~~~~~~~~~~
+
+Required properties :
+ - compatible : "ti,bq24735"
+
+Optional properties :
+ - ti,ac-detect: This gpio is optionally used to read the ac adapter
+ presence.
+ - ti,charge-current : Used to control and set the charging current. This value
+ must follow the below guidelines:
+ bit 0 - 5: Not used
+ bit 6: 1 = Adds 64mA of charger current
+ bit 7: 1 = Adds 128mA of charger current
+ bit 8: 1 = Adds 256mA of charger current
+ bit 9: 1 = Adds 512mA of charger current
+ bit 10: 1 = Adds 1024mA of charger current
+ bit 11: 1 = Adds 2048mA of charger current
+ bit 12: 1 = Adds 4096mA of charger current
+ bit 13 - 15: Not used
+ Setting the value to < 128mA or > 8.128A terminates charging.
+ - ti,charge-voltage : Used to control and set the charging voltage. This value
+ must follow the below guidelines:
+ bit 0 - 3: Not used
+ bit 4: 1 = Adds 16mV of charger voltage
+ bit 5: 1 = Adds 32mV of charger voltage
+ bit 6: 1 = Adds 64mV of charger voltage
+ bit 7: 1 = Adds 128mV of charger voltage
+ bit 8: 1 = Adds 256mV of charger voltage
+ bit 9: 1 = Adds 512mV of charger voltage
+ bit 10: 1 = Adds 1024mV of charger voltage
+ bit 11: 1 = Adds 2048mV of charger voltage
+ bit 12: 1 = Adds 4096mV of charger voltage
+ bit 13: 1 = Adds 8192mV of charger voltage
+ bit 14: 1 = Adds 16384mV of charger voltage
+ bit 15: Not used
+ Setting the value to < 1.024V or > 19.2V terminates charging.
+ - ti,input-current: Used to control and set the charger input current. This
+ value must follow the below guidelines:
+ bit 0 - 6: Not used
+ bit 7: 1 = Adds 128mA of input current
+ bit 8: 1 = Adds 256mA of input current
+ bit 9: 1 = Adds 512mA of input current
+ bit 10: 1 = Adds 1024mA of input current
+ bit 11: 1 = Adds 2048mA of input current
+ bit 12: 1 = Adds 4086mA of input current
+ bit 13 - 15: Not used
+ Setting the value to < 128mA or > 8.064A terminates charging.
+
+Example:
+
+ bq24735@9 {
+ compatible = "ti,bq24735";
+ reg = < 0x9 >;
+ ti,ac-detect = <&gpio 72 0x1>;
+ }
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index e8970a6..380968d 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -340,6 +340,14 @@ config CHARGER_BQ24190
help
Say Y to enable support for the TI BQ24190 battery charger.
+config CHARGER_BQ24735
+ tristate "BQ24735 battery charger support"
+ depends on I2C && GPIOLIB
+ help
+ bq24735 is the battery charger chipset.
+ Say Y here to enable battery charging driver support for
+ bq24735
+
config CHARGER_SMB347
tristate "Summit Microelectronics SMB347 Battery Charger"
depends on I2C
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 4ae4533..e65487b 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
obj-$(CONFIG_CHARGER_BQ2415X) += bq2415x_charger.o
obj-$(CONFIG_CHARGER_BQ24190) += bq24190_charger.o
+obj-$(CONFIG_CHARGER_BQ24735) += bq24735-charger.o
obj-$(CONFIG_POWER_AVS) += avs/
obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
obj-$(CONFIG_CHARGER_TPS65090) += tps65090-charger.o
diff --git a/drivers/power/bq24735-charger.c b/drivers/power/bq24735-charger.c
new file mode 100644
index 0000000..8271558
--- /dev/null
+++ b/drivers/power/bq24735-charger.c
@@ -0,0 +1,384 @@
+/*
+ * drivers/power/bq24735-charger.c
+ *
+ * Battery charger driver for bq24735 from TI
+ *
+ * Copyright (c) 2013, NVIDIA CORPORATION. All rights reserved.
+ *
+ * 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;
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ */
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/power_supply.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+
+#include <linux/power/bq24735-charger.h>
+
+struct bq24735_charger {
+ struct power_supply charger;
+ struct device *dev;
+ struct i2c_client *client;
+ struct bq24735_platform *pdata;
+ int irq;
+};
+
+static enum power_supply_property bq24735_charger_properties[] = {
+ POWER_SUPPLY_PROP_ONLINE,
+};
+
+static int bq24735_write_word(struct i2c_client *client, int reg, u16 value)
+{
+ s32 ret;
+
+ ret = i2c_smbus_write_word_data(client, reg, le16_to_cpu(value));
+ if (ret < 0)
+ dev_err(&client->dev, "Failed in writing to 0x%x\n", reg);
+ return ret;
+}
+
+static int bq24735_read_word(struct i2c_client *client, int reg)
+{
+ s32 ret;
+
+ ret = i2c_smbus_read_word_data(client, reg);
+ if (ret < 0) {
+ dev_err(&client->dev, "Failed in reading from 0x%x\n", reg);
+ return ret;
+ }
+
+ return le16_to_cpu(ret);
+}
+
+static int bq24735_update_word(struct i2c_client *client, int reg,
+ u16 mask, u16 value)
+{
+ unsigned int tmp;
+ int ret;
+
+ ret = bq24735_read_word(client, reg);
+ if (ret < 0)
+ return ret;
+
+ tmp = ret & ~mask;
+ tmp |= value & mask;
+
+ ret = bq24735_write_word(client, reg, tmp);
+
+ return ret;
+}
+
+static int bq24735_config_charger(struct bq24735_charger *charger)
+{
+ int ret = 0;
+ u16 value = 0;
+ struct bq24735_platform *pdata = charger->pdata;
+
+ if (pdata->charge_current) {
+ value = pdata->charge_current & BQ24735_CHARGE_CURRENT_MASK;
+
+ ret = bq24735_write_word(charger->client,
+ BQ24735_CHARGE_CURRENT_REG, value);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (pdata->charge_voltage) {
+ value = pdata->charge_voltage & BQ24735_CHARGE_VOLTAGE_MASK;
+
+ ret = bq24735_write_word(charger->client,
+ BQ24735_CHARGE_VOLTAGE_REG, value);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (pdata->input_current) {
+ value = pdata->input_current & BQ24735_INPUT_CURRENT_MASK;
+
+ ret = bq24735_write_word(charger->client,
+ BQ24735_INPUT_CURRENT_REG, value);
+ if (ret < 0)
+ return ret;
+ }
+ return ret;
+}
+
+static irqreturn_t bq24735_charger_isr(int irq, void *devid)
+{
+ struct power_supply *charger = devid;
+
+ power_supply_changed(charger);
+
+ return IRQ_HANDLED;
+}
+
+static int bq24735_charger_get_property(struct power_supply *psy,
+ enum power_supply_property psp, union power_supply_propval *val)
+{
+ struct bq24735_charger *bq24735_charger;
+ struct bq24735_platform *pdata;
+
+ bq24735_charger = container_of(psy, struct bq24735_charger, charger);
+ pdata = bq24735_charger->pdata;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_ONLINE:
+ if (pdata->status_gpio) {
+ val->intval = gpio_get_value_cansleep(
+ pdata->status_gpio);
+ val->intval ^= pdata->gpio_active_low;
+ } else {
+ int ac = 0;
+
+ ac = bq24735_read_word(bq24735_charger->client,
+ BQ24735_CHG_OPT_REG);
+ val->intval = (ac & BQ24735_CHG_OPT_AC_PRESENT) ? 1 : 0;
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int bq24735_enable_charging(struct bq24735_charger *charger)
+{
+ int ret = 0;
+
+ ret = bq24735_update_word(charger->client, BQ24735_CHG_OPT_REG,
+ BQ24735_CHG_OPT_CHARGE_ENABLE,
+ BQ24735_ENABLE_CHARGING);
+ return ret;
+}
+
+#ifdef CONFIG_OF
+static struct bq24735_platform *bq24735_parse_dt_data(
+ struct i2c_client *client)
+{
+ struct bq24735_platform *pdata;
+ struct device_node *np = client->dev.of_node;
+ u32 val;
+ int ret;
+ enum of_gpio_flags flags;
+
+ pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_err(&client->dev,
+ "Memory alloc for bq24735 pdata failed\n");
+ return NULL;
+ }
+
+ pdata->status_gpio = of_get_named_gpio_flags(np, "ti,ac-detect",
+ 0, &flags);
+
+ if (flags & OF_GPIO_ACTIVE_LOW)
+ pdata->gpio_active_low = 1;
+
+ ret = of_property_read_u32(np, "ti,charge-current", &val);
+ if (!ret)
+ pdata->charge_current = val;
+
+ ret = of_property_read_u32(np, "ti,charge-voltage", &val);
+ if (!ret)
+ pdata->charge_voltage = val;
+
+ ret = of_property_read_u32(np, "ti,input-current", &val);
+ if (!ret)
+ pdata->input_current = val;
+
+ return pdata;
+}
+#else
+static inline struct bq24735_platform *bq24735_parse_dt_data(
+ struct i2c_client *client)
+{
+ return NULL;
+}
+#endif
+
+static int bq24735_charger_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ int ret = 0;
+ struct bq24735_charger *charger_device;
+ struct power_supply *charger;
+ char *name;
+
+ charger_device = devm_kzalloc(&client->dev, sizeof(*charger_device),
+ GFP_KERNEL);
+ if (!charger_device) {
+ dev_err(&client->dev, "failed to allocate memory status\n");
+ return -ENOMEM;
+ }
+
+ charger_device->pdata = client->dev.platform_data;
+
+ if (!charger_device->pdata && client->dev.of_node)
+ charger_device->pdata = bq24735_parse_dt_data(client);
+
+ if (!charger_device->pdata) {
+ dev_err(&client->dev, "no platform data provided\n");
+ return -EINVAL;
+ }
+
+ name = charger_device->pdata->name;
+ if (!name) {
+ name = kasprintf(GFP_KERNEL, "bq24735-%s",
+ dev_name(&client->dev));
+ if (!name) {
+ dev_err(&client->dev, "Failed to alloc device name\n");
+ return -ENOMEM;
+ }
+ }
+
+ charger_device->dev = &client->dev;
+ charger_device->client = client;
+
+ charger = &charger_device->charger;
+
+ charger->name = name;
+ charger->type = POWER_SUPPLY_TYPE_MAINS;
+ charger->properties = bq24735_charger_properties;
+ charger->num_properties = ARRAY_SIZE(bq24735_charger_properties);
+ charger->get_property = bq24735_charger_get_property;
+ charger->supplied_to = charger_device->pdata->supplied_to;
+ charger->num_supplicants = charger_device->pdata->num_supplicants;
+ charger->of_node = client->dev.of_node;
+
+ i2c_set_clientdata(client, charger_device);
+
+ ret = bq24735_read_word(charger_device->client,
+ BQ24735_MANUFACTURER_ID_REG);
+ if (ret != BQ24735_MANUFACTURER_ID) {
+ dev_err(charger_device->dev,
+ "manufacturer id mismatch..exiting driver..\n");
+ ret = -ENODEV;
+ goto err_free_name;
+ }
+
+ if (client->irq) {
+ ret = devm_request_threaded_irq(&client->dev, client->irq,
+ NULL, bq24735_charger_isr,
+ IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
+ IRQF_ONESHOT,
+ charger->name, charger);
+ if (ret) {
+ dev_err(&client->dev,
+ "Unable to register irq %d err %d\n",
+ client->irq, ret);
+ goto err_free_name;
+ }
+ charger_device->irq = client->irq;
+ }
+
+ if (charger_device->pdata->status_gpio) {
+ if (!gpio_is_valid(charger_device->pdata->status_gpio)) {
+ dev_err(&client->dev, "Invalid gpio pin\n");
+ ret = -EINVAL;
+ goto err_free_name;
+ }
+
+ ret = devm_gpio_request(&client->dev,
+ charger_device->pdata->status_gpio,
+ name);
+ if (ret) {
+ dev_err(&client->dev, "Failed gpio request: %d\n", ret);
+ goto err_free_name;
+ }
+ }
+
+ ret = bq24735_config_charger(charger_device);
+ if (ret < 0) {
+ dev_err(&client->dev, "failed in configuring charger");
+ goto err_free_name;
+ }
+
+ /* check for AC adapter presence */
+ ret = bq24735_read_word(charger_device->client, BQ24735_CHG_OPT_REG);
+ if (ret < 0)
+ goto err_free_name;
+ else if (ret & BQ24735_CHG_OPT_AC_PRESENT) {
+ /* enable charging */
+ ret = bq24735_enable_charging(charger_device);
+ if (ret < 0)
+ goto err_free_name;
+ }
+
+ ret = power_supply_register(&client->dev, charger);
+ if (ret < 0) {
+ dev_err(&client->dev, "Failed to register power supply: %d\n",
+ ret);
+ goto err_free_name;
+ }
+
+ return 0;
+
+err_free_name:
+ if (name && name != charger_device->pdata->name)
+ kfree(name);
+
+ return ret;
+}
+
+static int bq24735_charger_remove(struct i2c_client *client)
+{
+ struct bq24735_charger *charger_device = i2c_get_clientdata(client);
+
+ if (charger_device->irq)
+ devm_free_irq(charger_device->dev, charger_device->irq,
+ &charger_device->charger);
+
+ power_supply_unregister(&charger_device->charger);
+
+ if (charger_device->charger.name != charger_device->pdata->name)
+ kfree(charger_device->charger.name);
+
+ return 0;
+}
+
+static const struct i2c_device_id bq24735_charger_id[] = {
+ { "bq24735-charger", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, bq24735_charger_id);
+
+static const struct of_device_id bq24735_match_ids[] = {
+ { .compatible = "ti,bq24735", },
+ { /* end */ }
+};
+MODULE_DEVICE_TABLE(of, bq24735_match_ids);
+
+static struct i2c_driver bq24735_charger_driver = {
+ .driver = {
+ .name = "bq24735-charger",
+ .owner = THIS_MODULE,
+ .of_match_table = bq24735_match_ids,
+ },
+ .probe = bq24735_charger_probe,
+ .remove = bq24735_charger_remove,
+ .id_table = bq24735_charger_id,
+};
+
+module_i2c_driver(bq24735_charger_driver);
+
+MODULE_DESCRIPTION("bq24735 battery charging driver");
+MODULE_AUTHOR("Darbha Sriharsha <[email protected]>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/power/bq24735-charger.h b/include/linux/power/bq24735-charger.h
new file mode 100644
index 0000000..9663c57
--- /dev/null
+++ b/include/linux/power/bq24735-charger.h
@@ -0,0 +1,68 @@
+/*
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef __CHARGER_BQ24735_H_
+#define __CHARGER_BQ24735_H_
+
+#include <linux/types.h>
+#include <linux/power_supply.h>
+
+#define BQ24735_CHG_OPT_REG 0x12
+#define BQ24735_CHG_OPT_CHARGE_ENABLE (1 << 0)
+#define BQ24735_ENABLE_CHARGING 0
+#define BQ24735_DISABLE_CHARGING 1
+#define BQ24735_CHG_OPT_ACOC_THRESHOLD (1 << 1)
+#define BQ24735_CHG_OPT_BOOST_MODE (1 << 2)
+#define BQ24735_CHG_OPT_BOOST_ENABLE (1 << 3)
+#define BQ24735_CHG_OPT_AC_PRESENT (1 << 4)
+#define BQ24735_CHG_OPT_IOUT_SELECTION (1 << 5)
+#define BQ24735_CHG_OPT_LEARN_ENABLE (1 << 6)
+#define BQ24735_CHG_OPT_IFAULT_LOW (1 << 7)
+#define BQ24735_CHG_OPT_IFAULT_HIGH (1 << 8)
+#define BQ24735_CHG_OPT_EMI_SW_FREQ_EN (1 << 9)
+#define BQ24735_CHG_OPT_EMI_SW_FREQ_ADJ (1 << 10)
+#define BQ24735_CHG_OPT_BAT_DEPLETION (3 << 11)
+#define BQ24735_CHG_OPT_WATCHDOG_TIMER (3 << 13)
+#define BQ24735_CHG_OPT_ACOK_DEGLITCH (1 << 15)
+#define BQ24735_CHARGE_CURRENT_REG 0x14
+#define BQ24735_CHARGE_CURRENT_MASK 0x1fc0
+#define BQ24735_CHARGE_VOLTAGE_REG 0x15
+#define BQ24735_CHARGE_VOLTAGE_MASK 0x7ff0
+#define BQ24735_INPUT_CURRENT_REG 0x3f
+#define BQ24735_INPUT_CURRENT_MASK 0x1f80
+#define BQ24735_MANUFACTURER_ID_REG 0xfe
+#define BQ24735_DEVICE_ID_REG 0xff
+
+#define BQ24735_MANUFACTURER_ID 0x0040
+#define BQ24735_DEVICE_ID 0x000B
+
+
+struct bq24735_platform {
+ uint32_t charge_current;
+ uint32_t charge_voltage;
+ uint32_t input_current;
+
+ const char *name;
+
+ int status_gpio;
+ int gpio_active_low;
+
+ char **supplied_to;
+ size_t num_supplicants;
+};
+
+#endif /* __CHARGER_BQ24735_H_ */
--
1.7.9.5
On Thu, Sep 19, 2013 at 12:18:33PM -0400, Rhyland Klein wrote:
> From: Darbha Sriharsha <[email protected]>
>
> Adding driver support for bq24735 charger chipset.
This could be somewhat more descriptive. Merely repeating the subject
doesn't add useful information.
> diff --git a/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt b/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt
[...]
> +Optional properties :
> + - ti,ac-detect: This gpio is optionally used to read the ac adapter
> + presence.
"GPIO". And if this truly is a reference to a GPIO I think it should be
named something like: ti,ac-detect-gpio(s).
> + - ti,charge-current : Used to control and set the charging current. This value
> + must follow the below guidelines:
Perhaps the wording should be clearer here. Something like:
The charging current is specified by ORing together the bits
listed below and summing up their respective weights.
> + bit 0 - 5: Not used
> + bit 6: 1 = Adds 64mA of charger current
> + bit 7: 1 = Adds 128mA of charger current
> + bit 8: 1 = Adds 256mA of charger current
> + bit 9: 1 = Adds 512mA of charger current
> + bit 10: 1 = Adds 1024mA of charger current
> + bit 11: 1 = Adds 2048mA of charger current
> + bit 12: 1 = Adds 4096mA of charger current
Then again, these bits match the actual values, so it might just be
easier to simply state that the current must be a multiple of 64 mA
(and the below limits).
> + bit 13 - 15: Not used
> + Setting the value to < 128mA or > 8.128A terminates charging.
If my math doesn't deceive me, using the above you can't set the current
to > 8.128 A. In fact, adding in all the bits yields exactly 8128 mA.
> + - ti,charge-voltage : Used to control and set the charging voltage. This value
> + must follow the below guidelines:
> + bit 0 - 3: Not used
> + bit 4: 1 = Adds 16mV of charger voltage
> + bit 5: 1 = Adds 32mV of charger voltage
> + bit 6: 1 = Adds 64mV of charger voltage
> + bit 7: 1 = Adds 128mV of charger voltage
> + bit 8: 1 = Adds 256mV of charger voltage
> + bit 9: 1 = Adds 512mV of charger voltage
> + bit 10: 1 = Adds 1024mV of charger voltage
> + bit 11: 1 = Adds 2048mV of charger voltage
> + bit 12: 1 = Adds 4096mV of charger voltage
> + bit 13: 1 = Adds 8192mV of charger voltage
> + bit 14: 1 = Adds 16384mV of charger voltage
Same comments here.
> + bit 15: Not used
> + Setting the value to < 1.024V or > 19.2V terminates charging.
> + - ti,input-current: Used to control and set the charger input current. This
Nit: formatting here is inconsistent: sometimes there's a space on both
sides of the colon, here, there's no space on the left, but the space on
the right is actually a tab.
> + value must follow the below guidelines:
> + bit 0 - 6: Not used
> + bit 7: 1 = Adds 128mA of input current
> + bit 8: 1 = Adds 256mA of input current
> + bit 9: 1 = Adds 512mA of input current
> + bit 10: 1 = Adds 1024mA of input current
> + bit 11: 1 = Adds 2048mA of input current
> + bit 12: 1 = Adds 4086mA of input current
And here. Also, should 4086 really be 4096?
> + bit 13 - 15: Not used
> + Setting the value to < 128mA or > 8.064A terminates charging.
Again, none of the combinations of the valid bits are outside of the
range specified here. But I think it'd be easier to just specify the
valid range here and that it needs to be a multiple of 128 mA.
> +Example:
> +
> + bq24735@9 {
> + compatible = "ti,bq24735";
> + reg = < 0x9 >;
No space after < or before >.
> + ti,ac-detect = <&gpio 72 0x1>;
> + }
Since this doesn't list any of the ti,charge-current, ti,charge-voltage
or ti,input-current properties, perhaps the document should mention the
defaults?
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
[...]
> help
> Say Y to enable support for the TI BQ24190 battery charger.
>
> +config CHARGER_BQ24735
> + tristate "BQ24735 battery charger support"
> + depends on I2C && GPIOLIB
> + help
> + bq24735 is the battery charger chipset.
> + Say Y here to enable battery charging driver support for
> + bq24735
Perhaps keep this consistent with the BQ24190 entry above?
> diff --git a/drivers/power/bq24735-charger.c b/drivers/power/bq24735-charger.c
[...]
> new file mode 100644
> index 0000000..8271558
> --- /dev/null
> +++ b/drivers/power/bq24735-charger.c
> @@ -0,0 +1,384 @@
> +/*
> + * drivers/power/bq24735-charger.c
I'd leave this out. It's unnecessary.
> + *
> + * Battery charger driver for bq24735 from TI
Nit: this sounds like the driver is "from TI". Perhaps:
battery charger driver for TI BQ24735
?
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + */
> +#include <linux/init.h>
Nit: could use a space between the above two lines.
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/power_supply.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
Perhaps sort this list alphabetically?
> +struct bq24735_charger {
Perhaps just bq24735? _charger seems redundant.
> + struct power_supply charger;
> + struct device *dev;
This field is used in exactly two locations, where it would be easier to
just use the i2c_client.dev field instead.
> + struct i2c_client *client;
> + struct bq24735_platform *pdata;
The tendency has been to not borrow trouble by adding platform data
support unless you actually use it. So can this perhaps be dropped?
> + int irq;
This is the same as i2c_client.irq, so redundant and can be dropped.
> +static int bq24735_write_word(struct i2c_client *client, int reg, u16 value)
reg should probably be u8 because that's the type passed to
i2c_smbus_write_word_data().
> +{
> + s32 ret;
> +
> + ret = i2c_smbus_write_word_data(client, reg, le16_to_cpu(value));
> + if (ret < 0)
> + dev_err(&client->dev, "Failed in writing to 0x%x\n", reg);
> + return ret;
> +}
I'm not sure if we need that error here. This can probably be better
handled in callers. That way you can make this a one line function.
> +static int bq24735_read_word(struct i2c_client *client, int reg)
u8 for reg.
> +{
> + s32 ret;
> +
> + ret = i2c_smbus_read_word_data(client, reg);
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed in reading from 0x%x\n", reg);
> + return ret;
> + }
> +
> + return le16_to_cpu(ret);
> +}
Equally this can be made simpler:
{
s32 ret = i2c_smbus_read_word_data(client, reg);
return ret < 0 ? ret : le16_to_cpu(ret);
}
> +static int bq24735_update_word(struct i2c_client *client, int reg,
u8 for reg.
> + ret = bq24735_read_word(client, reg);
> + if (ret < 0)
> + return ret;
> +
> + tmp = ret & ~mask;
> + tmp |= value & mask;
> +
> + ret = bq24735_write_word(client, reg, tmp);
> +
> + return ret;
Just: return bq24735_write_word(...);
> +static int bq24735_config_charger(struct bq24735_charger *charger)
> +{
> + int ret = 0;
> + u16 value = 0;
I don't think you need to initialize these to 0 since you'll be
overwriting them immediately anyway.
> + struct bq24735_platform *pdata = charger->pdata;
> +
> + if (pdata->charge_current) {
[...]
> + }
> +
> + if (pdata->charge_voltage) {
[...]
> + }
> +
> + if (pdata->input_current) {
[...]
> + }
> + return ret;
If you don't initialize ret, then it might end up uninitialized here,
but since this is only reached when everything else succeeded, this can
probably just be "return 0;", can't it?
> +static irqreturn_t bq24735_charger_isr(int irq, void *devid)
> +{
> + struct power_supply *charger = devid;
> +
> + power_supply_changed(charger);
> +
> + return IRQ_HANDLED;
> +}
I may have missed this, but where does the interrupt come from? The
device tree binding didn't seem to specify an interrupts property. But
looking at what this ISR does, it seems like it handles interrupts from
the ti,ac-detect GPIO. But I don't see how that ends up in the
i2c_client.irq field.
> +static int bq24735_charger_get_property(struct power_supply *psy,
> + enum power_supply_property psp, union power_supply_propval *val)
Alignment of arguments isn't right here.
> +{
> + struct bq24735_charger *bq24735_charger;
Just "bq24735", or "charger" would be good names too, and shorter.
> + struct bq24735_platform *pdata;
> +
> + bq24735_charger = container_of(psy, struct bq24735_charger, charger);
Perhaps this should be a static inline wrapper: to_bq2475()?
> + pdata = bq24735_charger->pdata;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_ONLINE:
> + if (pdata->status_gpio) {
> + val->intval = gpio_get_value_cansleep(
> + pdata->status_gpio);
> + val->intval ^= pdata->gpio_active_low;
There's an extra space between ^= and pdata->.
> + } else {
> + int ac = 0;
> +
> + ac = bq24735_read_word(bq24735_charger->client,
> + BQ24735_CHG_OPT_REG);
> + val->intval = (ac & BQ24735_CHG_OPT_AC_PRESENT) ? 1 : 0;
You should probably check for ac < 0 before setting the value here.
> + }
Perhaps move this whole code for the online property into a separate
function. It's not really huge, but it'll remove one level of
indentation, which may result in less wrapping of statements across
several lines.
> +static int bq24735_enable_charging(struct bq24735_charger *charger)
> +{
> + int ret = 0;
No need to initialize this variable.
> +
> + ret = bq24735_update_word(charger->client, BQ24735_CHG_OPT_REG,
> + BQ24735_CHG_OPT_CHARGE_ENABLE,
> + BQ24735_ENABLE_CHARGING);
> + return ret;
Just "return bq24735_...(...);" will do.
> +#ifdef CONFIG_OF
> +static struct bq24735_platform *bq24735_parse_dt_data(
> + struct i2c_client *client)
> +{
[...]
> +}
> +#else
> +static inline struct bq24735_platform *bq24735_parse_dt_data(
> + struct i2c_client *client)
> +{
> + return NULL;
> +}
> +#endif
Can we please get rid of the whole #ifdefery here? We have this really
cool tool that allows the compiler to handle this really well and give
us complete compile coverage. See below.
> +static int bq24735_charger_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
Parameter alignment is not right.
> +{
> + int ret = 0;
No need to initialize this to 0.
> + struct bq24735_charger *charger_device;
> + struct power_supply *charger;
This is confusing, perhaps call the charger just "charger", and the
power supply "supply"?
> + char *name;
> +
> + charger_device = devm_kzalloc(&client->dev, sizeof(*charger_device),
> + GFP_KERNEL);
Parameter alignment again.
> + if (!charger_device) {
> + dev_err(&client->dev, "failed to allocate memory status\n");
No, we don't want to mention this explicitly. The allocator's output
should be quite verbose in case this really ever happens, so no need to
add to that.
> + return -ENOMEM;
> + }
> +
> + charger_device->pdata = client->dev.platform_data;
> +
> + if (!charger_device->pdata && client->dev.of_node)
If you use IS_ENABLED(CONFIG_OF) here, the compiler will see that it
evaluates to 0 if OF is not selected, in which case it will be clever
enough to see that bq24735_parse_dt_data() is not used and just discard
it (because it is static). Then the #ifdefery above is not needed and
you will get compile coverage whether or not OF has been selected. Which
is a good thing.
That said, I've mentioned before that you may want to not support the
non-DT at all since there's no immediate need, so this may not even be
an issue.
> + name = charger_device->pdata->name;
> + if (!name) {
> + name = kasprintf(GFP_KERNEL, "bq24735-%s",
> + dev_name(&client->dev));
Won't the device name already include bq24735 because of the driver
name?
> + if (!name) {
> + dev_err(&client->dev, "Failed to alloc device name\n");
Again, no need for the error message.
> + charger->supplied_to = charger_device->pdata->supplied_to;
> + charger->num_supplicants = charger_device->pdata->num_supplicants;
I think these are never filled in in the DT case.
> + ret = bq24735_read_word(charger_device->client,
You can just use client directly here.
> + BQ24735_MANUFACTURER_ID_REG);
> + if (ret != BQ24735_MANUFACTURER_ID) {
> + dev_err(charger_device->dev,
> + "manufacturer id mismatch..exiting driver..\n");
This should be reformatted. It's just weird.
> + ret = -ENODEV;
Perhaps differentiate between the original error (ret, instead of
overwriting it) and the case where the manufacturer ID doesn't match?
> + goto err_free_name;
> + }
> +
> + if (client->irq) {
> + ret = devm_request_threaded_irq(&client->dev, client->irq,
devm_request_threaded_irq() can be dangerous here. You seem to handle it
properly in remove, but the ISR could be run at any point from here on
in. And automatic removal will happen rather late.
The ISR could still be run at any point from here on in if you used the
non-devm variant, so it's probably safer to call this much later. Since
you'd call power_supply_changed() on an unregistered power_supply.
> + NULL, bq24735_charger_isr,
> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
> + IRQF_ONESHOT,
> + charger->name, charger);
Parameter indentation again.
> + if (ret) {
> + dev_err(&client->dev,
> + "Unable to register irq %d err %d\n",
"IRQ"
> + if (charger_device->pdata->status_gpio) {
> + if (!gpio_is_valid(charger_device->pdata->status_gpio)) {
Why not make the first if check for gpio_is_valid()? Also, 0 is a valid
GPIO number.
> + dev_err(&client->dev, "Invalid gpio pin\n");
"GPIO". And would it make sense to continue with degraded functionality
if no GPIO is specified? It seems like it given the initial check for a
non-zero GPIO.
> + ret = bq24735_config_charger(charger_device);
> + if (ret < 0) {
> + dev_err(&client->dev, "failed in configuring charger");
> + goto err_free_name;
> + }
> +
> + /* check for AC adapter presence */
> + ret = bq24735_read_word(charger_device->client, BQ24735_CHG_OPT_REG);
> + if (ret < 0)
> + goto err_free_name;
> + else if (ret & BQ24735_CHG_OPT_AC_PRESENT) {
> + /* enable charging */
> + ret = bq24735_enable_charging(charger_device);
> + if (ret < 0)
> + goto err_free_name;
> + }
I think you already had code for this (in the one property accessor?),
so perhaps it should be factored out.
Also I don't see where charging is disabled. Or enabled when AC power is
plugged after the device has been probed. How does that work?
> +err_free_name:
> + if (name && name != charger_device->pdata->name)
> + kfree(name);
kfree() can handle NULL pointers, so the check for name is unnecessary.
> diff --git a/include/linux/power/bq24735-charger.h b/include/linux/power/bq24735-charger.h
[...]
> +#ifndef __CHARGER_BQ24735_H_
> +#define __CHARGER_BQ24735_H_
I would hope that we can get rid of this file. As I already mentioned,
unless you're actually going to use platform data, there's little sense
in adding support for it.
> +#define BQ24735_CHG_OPT_REG 0x12
> +#define BQ24735_CHG_OPT_CHARGE_ENABLE (1 << 0)
> +#define BQ24735_ENABLE_CHARGING 0
> +#define BQ24735_DISABLE_CHARGING 1
I don't think these are really useful. The field is already named
CHARGE_ENABLE, so it should be pretty clear what you're supposed to put
in here. For that matter, I'm not a huge fan of the whole "update bits"
API because it encourages these things and they are just confusing.
> +#define BQ24735_CHG_OPT_ACOC_THRESHOLD (1 << 1)
> +#define BQ24735_CHG_OPT_BOOST_MODE (1 << 2)
> +#define BQ24735_CHG_OPT_BOOST_ENABLE (1 << 3)
> +#define BQ24735_CHG_OPT_AC_PRESENT (1 << 4)
> +#define BQ24735_CHG_OPT_IOUT_SELECTION (1 << 5)
> +#define BQ24735_CHG_OPT_LEARN_ENABLE (1 << 6)
> +#define BQ24735_CHG_OPT_IFAULT_LOW (1 << 7)
> +#define BQ24735_CHG_OPT_IFAULT_HIGH (1 << 8)
> +#define BQ24735_CHG_OPT_EMI_SW_FREQ_EN (1 << 9)
> +#define BQ24735_CHG_OPT_EMI_SW_FREQ_ADJ (1 << 10)
> +#define BQ24735_CHG_OPT_BAT_DEPLETION (3 << 11)
> +#define BQ24735_CHG_OPT_WATCHDOG_TIMER (3 << 13)
> +#define BQ24735_CHG_OPT_ACOK_DEGLITCH (1 << 15)
Most (if not all) of these fields are unused, so I'm not sure if it
makes sense to list them here.
> +#define BQ24735_CHARGE_CURRENT_REG 0x14
> +#define BQ24735_CHARGE_CURRENT_MASK 0x1fc0
> +#define BQ24735_CHARGE_VOLTAGE_REG 0x15
> +#define BQ24735_CHARGE_VOLTAGE_MASK 0x7ff0
> +#define BQ24735_INPUT_CURRENT_REG 0x3f
> +#define BQ24735_INPUT_CURRENT_MASK 0x1f80
> +#define BQ24735_MANUFACTURER_ID_REG 0xfe
> +#define BQ24735_DEVICE_ID_REG 0xff
I think I'd drop the _REG suffix. Also perhaps these register
definitions should go into the driver source file.
> +#define BQ24735_MANUFACTURER_ID 0x0040
> +#define BQ24735_DEVICE_ID 0x000B
I think these could actually be used literally, since you read out a
register and compare the value to this one, it is immediately clear from
the context that they are the reference manufacturer and device IDs that
you expect for this driver.
> +struct bq24735_platform {
> + uint32_t charge_current;
> + uint32_t charge_voltage;
> + uint32_t input_current;
> +
> + const char *name;
> +
> + int status_gpio;
> + int gpio_active_low;
This is somewhat unfortunate. Perhaps status_gpio_active_low. That makes
it clear that it relates to the status_gpio, but it's also rather long.
Fortunately there's some good work being done to the GPIO core that will
hopefully make this unnecessary in the future.
> +
> + char **supplied_to;
> + size_t num_supplicants;
> +};
If you don't implement platform data support, you can get rid of this.
Move the registers to the driver source file and you don't need this
header file at all anymore.
Thierry
On 9/19/2013 4:27 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Sep 19, 2013 at 12:18:33PM -0400, Rhyland Klein wrote:
>> From: Darbha Sriharsha <[email protected]>
>>
>> Adding driver support for bq24735 charger chipset.
...snip
>
>> + return -ENOMEM;
>> + }
>> +
>> + charger_device->pdata = client->dev.platform_data;
>> +
>> + if (!charger_device->pdata && client->dev.of_node)
>
> If you use IS_ENABLED(CONFIG_OF) here, the compiler will see that it
> evaluates to 0 if OF is not selected, in which case it will be clever
> enough to see that bq24735_parse_dt_data() is not used and just discard
> it (because it is static). Then the #ifdefery above is not needed and
> you will get compile coverage whether or not OF has been selected. Which
> is a good thing.
>
> That said, I've mentioned before that you may want to not support the
> non-DT at all since there's no immediate need, so this may not even be
> an issue.
The main reason I don't want to break non-DT support (or just not
implement it) is that this driver is going to be used in our downstream
kernels, and I prefer to minimize the patches they will have on top of
it so we don't diverge.
>
>> + name = charger_device->pdata->name;
>> + if (!name) {
>> + name = kasprintf(GFP_KERNEL, "bq24735-%s",
>> + dev_name(&client->dev));
>
> Won't the device name already include bq24735 because of the driver
> name?
In my experience this comes up with a name like "bq24735-5-0009". Thats
why I added the bq24735 in the beginning, so the name is more descriptive.
>
>> + if (!name) {
>> + dev_err(&client->dev, "Failed to alloc device name\n");
>
> Again, no need for the error message.
>
>> + charger->supplied_to = charger_device->pdata->supplied_to;
>> + charger->num_supplicants = charger_device->pdata->num_supplicants;
>
> I think these are never filled in in the DT case.
No. With DT there is a different mechanism for creating these linkages.
It uses phandles and so is doesn't overlap with these. This is here to
support non-DT init.
>
>> + ret = bq24735_read_word(charger_device->client,
>
> You can just use client directly here.
>
>> + BQ24735_MANUFACTURER_ID_REG);
>> + if (ret != BQ24735_MANUFACTURER_ID) {
>> + dev_err(charger_device->dev,
>> + "manufacturer id mismatch..exiting driver..\n");
>
> This should be reformatted. It's just weird.
>
>> + ret = -ENODEV;
>
> Perhaps differentiate between the original error (ret, instead of
> overwriting it) and the case where the manufacturer ID doesn't match?
>
>> + goto err_free_name;
>> + }
>> +
>> + if (client->irq) {
>> + ret = devm_request_threaded_irq(&client->dev, client->irq,
>
> devm_request_threaded_irq() can be dangerous here. You seem to handle it
> properly in remove, but the ISR could be run at any point from here on
> in. And automatic removal will happen rather late.
>
> The ISR could still be run at any point from here on in if you used the
> non-devm variant, so it's probably safer to call this much later. Since
> you'd call power_supply_changed() on an unregistered power_supply.
Thats a good point. I think using the non-devm version seems safer, will
switch to in in next version.
>
>> + NULL, bq24735_charger_isr,
>> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
>> + IRQF_ONESHOT,
>> + charger->name, charger);
>
> Parameter indentation again.
>
>> + if (ret) {
>> + dev_err(&client->dev,
>> + "Unable to register irq %d err %d\n",
>
> "IRQ"
>
>> + if (charger_device->pdata->status_gpio) {
>> + if (!gpio_is_valid(charger_device->pdata->status_gpio)) {
>
> Why not make the first if check for gpio_is_valid()? Also, 0 is a valid
> GPIO number.
Will do.
>
>> + dev_err(&client->dev, "Invalid gpio pin\n");
>
> "GPIO". And would it make sense to continue with degraded functionality
> if no GPIO is specified? It seems like it given the initial check for a
> non-zero GPIO.
Yes. I'll fix that up.
>
>> + ret = bq24735_config_charger(charger_device);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "failed in configuring charger");
>> + goto err_free_name;
>> + }
>> +
>> + /* check for AC adapter presence */
>> + ret = bq24735_read_word(charger_device->client, BQ24735_CHG_OPT_REG);
>> + if (ret < 0)
>> + goto err_free_name;
>> + else if (ret & BQ24735_CHG_OPT_AC_PRESENT) {
>> + /* enable charging */
>> + ret = bq24735_enable_charging(charger_device);
>> + if (ret < 0)
>> + goto err_free_name;
>> + }
>
> I think you already had code for this (in the one property accessor?),
> so perhaps it should be factored out.
Will do.
>
> Also I don't see where charging is disabled. Or enabled when AC power is
> plugged after the device has been probed. How does that work?
I believe charging is auto-disabled when the adapter is unplugged, but I
will verify and if that doesn't seem to be the case. This is something
that should likely be added to the ISR (enable/disable).
>
>> +err_free_name:
>> + if (name && name != charger_device->pdata->name)
>> + kfree(name);
>
> kfree() can handle NULL pointers, so the check for name is unnecessary.
ok.
>
>> diff --git a/include/linux/power/bq24735-charger.h b/include/linux/power/bq24735-charger.h
> [...]
>> +#ifndef __CHARGER_BQ24735_H_
>> +#define __CHARGER_BQ24735_H_
>
> I would hope that we can get rid of this file. As I already mentioned,
> unless you're actually going to use platform data, there's little sense
> in adding support for it.
>
>> +#define BQ24735_CHG_OPT_REG 0x12
>> +#define BQ24735_CHG_OPT_CHARGE_ENABLE (1 << 0)
>> +#define BQ24735_ENABLE_CHARGING 0
>> +#define BQ24735_DISABLE_CHARGING 1
>
> I don't think these are really useful. The field is already named
> CHARGE_ENABLE, so it should be pretty clear what you're supposed to put
> in here. For that matter, I'm not a huge fan of the whole "update bits"
> API because it encourages these things and they are just confusing.
The only thing about the enable bit is that isn't kind of inverted what
what you might expect. 1 is disabling. Thats why I think the bit
definitions for enable/disable make sense. What would you suggest to
replace the "update bits" API?
>
>> +#define BQ24735_CHG_OPT_ACOC_THRESHOLD (1 << 1)
>> +#define BQ24735_CHG_OPT_BOOST_MODE (1 << 2)
>> +#define BQ24735_CHG_OPT_BOOST_ENABLE (1 << 3)
>> +#define BQ24735_CHG_OPT_AC_PRESENT (1 << 4)
>> +#define BQ24735_CHG_OPT_IOUT_SELECTION (1 << 5)
>> +#define BQ24735_CHG_OPT_LEARN_ENABLE (1 << 6)
>> +#define BQ24735_CHG_OPT_IFAULT_LOW (1 << 7)
>> +#define BQ24735_CHG_OPT_IFAULT_HIGH (1 << 8)
>> +#define BQ24735_CHG_OPT_EMI_SW_FREQ_EN (1 << 9)
>> +#define BQ24735_CHG_OPT_EMI_SW_FREQ_ADJ (1 << 10)
>> +#define BQ24735_CHG_OPT_BAT_DEPLETION (3 << 11)
>> +#define BQ24735_CHG_OPT_WATCHDOG_TIMER (3 << 13)
>> +#define BQ24735_CHG_OPT_ACOK_DEGLITCH (1 << 15)
>
> Most (if not all) of these fields are unused, so I'm not sure if it
> makes sense to list them here.
I had considered the same. I will remove them.
>
>> +#define BQ24735_CHARGE_CURRENT_REG 0x14
>> +#define BQ24735_CHARGE_CURRENT_MASK 0x1fc0
>> +#define BQ24735_CHARGE_VOLTAGE_REG 0x15
>> +#define BQ24735_CHARGE_VOLTAGE_MASK 0x7ff0
>> +#define BQ24735_INPUT_CURRENT_REG 0x3f
>> +#define BQ24735_INPUT_CURRENT_MASK 0x1f80
>> +#define BQ24735_MANUFACTURER_ID_REG 0xfe
>> +#define BQ24735_DEVICE_ID_REG 0xff
>
> I think I'd drop the _REG suffix. Also perhaps these register
> definitions should go into the driver source file.
>
>> +#define BQ24735_MANUFACTURER_ID 0x0040
>> +#define BQ24735_DEVICE_ID 0x000B
>
> I think these could actually be used literally, since you read out a
> register and compare the value to this one, it is immediately clear from
> the context that they are the reference manufacturer and device IDs that
> you expect for this driver.
Ok.
>
>> +struct bq24735_platform {
>> + uint32_t charge_current;
>> + uint32_t charge_voltage;
>> + uint32_t input_current;
>> +
>> + const char *name;
>> +
>> + int status_gpio;
>> + int gpio_active_low;
>
> This is somewhat unfortunate. Perhaps status_gpio_active_low. That makes
> it clear that it relates to the status_gpio, but it's also rather long.
> Fortunately there's some good work being done to the GPIO core that will
> hopefully make this unnecessary in the future.
>
>> +
>> + char **supplied_to;
>> + size_t num_supplicants;
>> +};
>
> If you don't implement platform data support, you can get rid of this.
> Move the registers to the driver source file and you don't need this
> header file at all anymore.
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>
Thanks for the review, I'll take care of all the comments, and
investigate anything whose fix wasn't immediately apparent.
-rhyland
--
nvpublic
On Thu, Sep 19, 2013 at 04:45:11PM -0400, Rhyland Klein wrote:
> On 9/19/2013 4:27 PM, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> >
> > On Thu, Sep 19, 2013 at 12:18:33PM -0400, Rhyland Klein wrote:
> >> From: Darbha Sriharsha <[email protected]>
> >>
> >> Adding driver support for bq24735 charger chipset.
> ...snip
> >
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + charger_device->pdata = client->dev.platform_data;
> >> +
> >> + if (!charger_device->pdata && client->dev.of_node)
> >
> > If you use IS_ENABLED(CONFIG_OF) here, the compiler will see that it
> > evaluates to 0 if OF is not selected, in which case it will be clever
> > enough to see that bq24735_parse_dt_data() is not used and just discard
> > it (because it is static). Then the #ifdefery above is not needed and
> > you will get compile coverage whether or not OF has been selected. Which
> > is a good thing.
> >
> > That said, I've mentioned before that you may want to not support the
> > non-DT at all since there's no immediate need, so this may not even be
> > an issue.
>
> The main reason I don't want to break non-DT support (or just not
> implement it) is that this driver is going to be used in our downstream
> kernels, and I prefer to minimize the patches they will have on top of
> it so we don't diverge.
I was under the impression that our downstream kernels used DT for a lot
of devices already. This doesn't look like a very special binding, and I
don't see a reason why we'd have to use platform data in our downstream
kernels.
> >> + name = charger_device->pdata->name;
> >> + if (!name) {
> >> + name = kasprintf(GFP_KERNEL, "bq24735-%s",
> >> + dev_name(&client->dev));
> >
> > Won't the device name already include bq24735 because of the driver
> > name?
>
> In my experience this comes up with a name like "bq24735-5-0009". Thats
> why I added the bq24735 in the beginning, so the name is more descriptive.
Yes, you're right. Perhaps in that case it's even easier to just stick
with a static string such as "bq24735" or "bq24735-charger". It's likely
to be the only device of that type in a machine. If you want to include
the device name, perhaps something like "%s/bq24735" (5-0009/bq24735) is
clearer that 5-0009 is actually the bus-specific name.
> > Also I don't see where charging is disabled. Or enabled when AC power is
> > plugged after the device has been probed. How does that work?
>
> I believe charging is auto-disabled when the adapter is unplugged, but I
> will verify and if that doesn't seem to be the case. This is something
> that should likely be added to the ISR (enable/disable).
I can very well imagine that it's auto-disabled when the power supply is
unplugged, but probably more importantly charging should be reenabled
when the supply is plugged again.
> >> +#define BQ24735_CHG_OPT_REG 0x12
> >> +#define BQ24735_CHG_OPT_CHARGE_ENABLE (1 << 0)
> >> +#define BQ24735_ENABLE_CHARGING 0
> >> +#define BQ24735_DISABLE_CHARGING 1
> >
> > I don't think these are really useful. The field is already named
> > CHARGE_ENABLE, so it should be pretty clear what you're supposed to put
> > in here. For that matter, I'm not a huge fan of the whole "update bits"
> > API because it encourages these things and they are just confusing.
>
> The only thing about the enable bit is that isn't kind of inverted what
> what you might expect. 1 is disabling. Thats why I think the bit
> definitions for enable/disable make sense. What would you suggest to
> replace the "update bits" API?
Well, especially for single bits I find it much more intuitive to do
something like this:
value = read();
value |= ENABLE;
write(value);
or
value = read();
value &= ~ENABLE;
write(value);
And if the meaning of the bit is inverted, then you can just rename
ENABLE to DISABLE. "update bits" might be fine for fields wider than a
single bit, but even in those cases, I find something like the above
much easier to read. But perhaps that's just personal preference.
Thierry
On 9/20/2013 3:53 AM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Sep 19, 2013 at 04:45:11PM -0400, Rhyland Klein wrote:
>> On 9/19/2013 4:27 PM, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>>
>>> On Thu, Sep 19, 2013 at 12:18:33PM -0400, Rhyland Klein wrote:
>>>> From: Darbha Sriharsha <[email protected]>
>>>>
>>>> Adding driver support for bq24735 charger chipset.
>> ...snip
>>>
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + charger_device->pdata = client->dev.platform_data;
>>>> +
>>>> + if (!charger_device->pdata && client->dev.of_node)
>>>
>>> If you use IS_ENABLED(CONFIG_OF) here, the compiler will see that it
>>> evaluates to 0 if OF is not selected, in which case it will be clever
>>> enough to see that bq24735_parse_dt_data() is not used and just discard
>>> it (because it is static). Then the #ifdefery above is not needed and
>>> you will get compile coverage whether or not OF has been selected. Which
>>> is a good thing.
>>>
>>> That said, I've mentioned before that you may want to not support the
>>> non-DT at all since there's no immediate need, so this may not even be
>>> an issue.
>>
>> The main reason I don't want to break non-DT support (or just not
>> implement it) is that this driver is going to be used in our downstream
>> kernels, and I prefer to minimize the patches they will have on top of
>> it so we don't diverge.
>
> I was under the impression that our downstream kernels used DT for a lot
> of devices already. This doesn't look like a very special binding, and I
> don't see a reason why we'd have to use platform data in our downstream
> kernels.
Specifically, there is a platform that uses this part where the chip
itself isn't connected to an i2c bus (From what I understand). In that
case, they actually add callbacks into the platform data and then use
them to configure the charger.
>
>>>> + name = charger_device->pdata->name;
>>>> + if (!name) {
>>>> + name = kasprintf(GFP_KERNEL, "bq24735-%s",
>>>> + dev_name(&client->dev));
>>>
>>> Won't the device name already include bq24735 because of the driver
>>> name?
>>
>> In my experience this comes up with a name like "bq24735-5-0009". Thats
>> why I added the bq24735 in the beginning, so the name is more descriptive.
>
> Yes, you're right. Perhaps in that case it's even easier to just stick
> with a static string such as "bq24735" or "bq24735-charger". It's likely
> to be the only device of that type in a machine. If you want to include
> the device name, perhaps something like "%s/bq24735" (5-0009/bq24735) is
> clearer that 5-0009 is actually the bus-specific name.
I prefer to have the bus identifier included in some way just because in
theory there could be multiple chargers and this will make their names
unique in sysfs. I am fine with either way, I simply followed the model
I saw in other drivers (like the sbs-battery driver). In fact, many
driver seem to just use the dev_name(), but I think either way is fine.
Maybe Anton or David have a preference.
>
>>> Also I don't see where charging is disabled. Or enabled when AC power is
>>> plugged after the device has been probed. How does that work?
>>
>> I believe charging is auto-disabled when the adapter is unplugged, but I
>> will verify and if that doesn't seem to be the case. This is something
>> that should likely be added to the ISR (enable/disable).
>
> I can very well imagine that it's auto-disabled when the power supply is
> unplugged, but probably more importantly charging should be reenabled
> when the supply is plugged again.
>
>>>> +#define BQ24735_CHG_OPT_REG 0x12
>>>> +#define BQ24735_CHG_OPT_CHARGE_ENABLE (1 << 0)
>>>> +#define BQ24735_ENABLE_CHARGING 0
>>>> +#define BQ24735_DISABLE_CHARGING 1
>>>
>>> I don't think these are really useful. The field is already named
>>> CHARGE_ENABLE, so it should be pretty clear what you're supposed to put
>>> in here. For that matter, I'm not a huge fan of the whole "update bits"
>>> API because it encourages these things and they are just confusing.
>>
>> The only thing about the enable bit is that isn't kind of inverted what
>> what you might expect. 1 is disabling. Thats why I think the bit
>> definitions for enable/disable make sense. What would you suggest to
>> replace the "update bits" API?
>
> Well, especially for single bits I find it much more intuitive to do
> something like this:
>
> value = read();
> value |= ENABLE;
> write(value);
>
> or
>
> value = read();
> value &= ~ENABLE;
> write(value);
>
> And if the meaning of the bit is inverted, then you can just rename
> ENABLE to DISABLE. "update bits" might be fine for fields wider than a
> single bit, but even in those cases, I find something like the above
> much easier to read. But perhaps that's just personal preference.
I see your point I think your examples work fine for me. And since we
don't use any of the larger bit fields currently, we don't have to worry
about those cases right now.
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>
thanks again,
Rhyland
--
nvpublic
On 09/19/2013 10:18 AM, Rhyland Klein wrote:
> Adding driver support for bq24735 charger chipset.
> diff --git a/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt b/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt
> +Optional properties :
> + - ti,ac-detect: This gpio is optionally used to read the ac adapter
> + presence.
GPIO property names ought to end in "-gpios", i.e. "ti,ac-detect-gpios".
> + - ti,charge-current : Used to control and set the charging current. This value
> + must follow the below guidelines:
> + bit 0 - 5: Not used
> + bit 6: 1 = Adds 64mA of charger current
> + bit 7: 1 = Adds 128mA of charger current
> + bit 8: 1 = Adds 256mA of charger current
> + bit 9: 1 = Adds 512mA of charger current
> + bit 10: 1 = Adds 1024mA of charger current
> + bit 11: 1 = Adds 2048mA of charger current
> + bit 12: 1 = Adds 4096mA of charger current
> + bit 13 - 15: Not used
That's a little odd. Why not just put the number of mA directly into the
property unshifted?
> + Setting the value to < 128mA or > 8.128A terminates charging.
"terminates charging" is a driver action, not a description of HW. It's
fine to say that what min/max value should be specified in the property
for it to be valid, but not what action SW should take in response to that.
Those same two comments apply to other properties too.
> + - ti,input-current: Used to control and set the charger input current. This
> + value must follow the below guidelines:
What if this value is inconsistent with the charger's power consumption
derived from the other two properties? Is it really necessary to include
this property in the DT and hence create the potential for mismatch?
On 9/23/2013 5:53 PM, Stephen Warren wrote:
> On 09/19/2013 10:18 AM, Rhyland Klein wrote:
>> Adding driver support for bq24735 charger chipset.
>
>> diff --git a/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt b/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt
>
>> +Optional properties :
>> + - ti,ac-detect: This gpio is optionally used to read the ac adapter
>> + presence.
>
> GPIO property names ought to end in "-gpios", i.e. "ti,ac-detect-gpios".
V3 (still in the process of finishing up and testing, will have this change.
>
>> + - ti,charge-current : Used to control and set the charging current. This value
>> + must follow the below guidelines:
>> + bit 0 - 5: Not used
>> + bit 6: 1 = Adds 64mA of charger current
>> + bit 7: 1 = Adds 128mA of charger current
>> + bit 8: 1 = Adds 256mA of charger current
>> + bit 9: 1 = Adds 512mA of charger current
>> + bit 10: 1 = Adds 1024mA of charger current
>> + bit 11: 1 = Adds 2048mA of charger current
>> + bit 12: 1 = Adds 4096mA of charger current
>> + bit 13 - 15: Not used
>
> That's a little odd. Why not just put the number of mA directly into the
> property unshifted?
This is how the hw register is defined, its the literal number of mA.
This is cleaned up in the upcoming revision.
>
>> + Setting the value to < 128mA or > 8.128A terminates charging.
>
> "terminates charging" is a driver action, not a description of HW. It's
> fine to say that what min/max value should be specified in the property
> for it to be valid, but not what action SW should take in response to that.
This isn't sw. This is defined in the HW documentation as to what
happens if an invalid value is used.
>
> Those same two comments apply to other properties too.
>
>> + - ti,input-current: Used to control and set the charger input current. This
>> + value must follow the below guidelines:
>
> What if this value is inconsistent with the charger's power consumption
> derived from the other two properties? Is it really necessary to include
> this property in the DT and hence create the potential for mismatch?
>
This is a field which can be set in the device's registers. I think it
is being used in our downstream kernel. I maintained support for these 3
fields for the sake of completely defining the dt binding up front.
-rhyland
--
nvpublic
On 09/23/2013 04:01 PM, Rhyland Klein wrote:
> On 9/23/2013 5:53 PM, Stephen Warren wrote:
>> On 09/19/2013 10:18 AM, Rhyland Klein wrote:
>>> Adding driver support for bq24735 charger chipset.
>>
>>> diff --git a/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt b/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt
>>> + - ti,charge-current : Used to control and set the charging current. This value
>>> + must follow the below guidelines:
>>> + bit 0 - 5: Not used
>>> + bit 6: 1 = Adds 64mA of charger current
>>> + bit 7: 1 = Adds 128mA of charger current
>>> + bit 8: 1 = Adds 256mA of charger current
>>> + bit 9: 1 = Adds 512mA of charger current
>>> + bit 10: 1 = Adds 1024mA of charger current
>>> + bit 11: 1 = Adds 2048mA of charger current
>>> + bit 12: 1 = Adds 4096mA of charger current
>>> + bit 13 - 15: Not used
>>
>> That's a little odd. Why not just put the number of mA directly into the
>> property unshifted?
>
> This is how the hw register is defined, its the literal number of mA.
> This is cleaned up in the upcoming revision.
OK. If you still want to use the raw register encoding, which seems
reasonable, why not just say:
ti,charge-current: Value for charge current register as described in the
HW documentation.
>>> + Setting the value to < 128mA or > 8.128A terminates charging.
>>
>> "terminates charging" is a driver action, not a description of HW. It's
>> fine to say that what min/max value should be specified in the property
>> for it to be valid, but not what action SW should take in response to that.
>
> This isn't sw. This is defined in the HW documentation as to what
> happens if an invalid value is used.
OK, if the HW documentation already says that, I don't see the need for
the DT binding to also say it; I would suggest just removing that text.
On 9/23/2013 6:05 PM, Stephen Warren wrote:
> On 09/23/2013 04:01 PM, Rhyland Klein wrote:
>> On 9/23/2013 5:53 PM, Stephen Warren wrote:
>>> On 09/19/2013 10:18 AM, Rhyland Klein wrote:
>>>> Adding driver support for bq24735 charger chipset.
>>>
>>>> diff --git a/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt b/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt
>
>>>> + - ti,charge-current : Used to control and set the charging current. This value
>>>> + must follow the below guidelines:
>>>> + bit 0 - 5: Not used
>>>> + bit 6: 1 = Adds 64mA of charger current
>>>> + bit 7: 1 = Adds 128mA of charger current
>>>> + bit 8: 1 = Adds 256mA of charger current
>>>> + bit 9: 1 = Adds 512mA of charger current
>>>> + bit 10: 1 = Adds 1024mA of charger current
>>>> + bit 11: 1 = Adds 2048mA of charger current
>>>> + bit 12: 1 = Adds 4096mA of charger current
>>>> + bit 13 - 15: Not used
>>>
>>> That's a little odd. Why not just put the number of mA directly into the
>>> property unshifted?
>>
>> This is how the hw register is defined, its the literal number of mA.
>> This is cleaned up in the upcoming revision.
>
> OK. If you still want to use the raw register encoding, which seems
> reasonable, why not just say:
>
> ti,charge-current: Value for charge current register as described in the
> HW documentation.
I had written this for v3:
- ti,charge-current : Used to control and set the charging current. This
value must be between 128mA and 8.128A with a 64mA step resolution. The
POR value is 0x0000h. See spec for more details.
Do you think the range is unnecessary or is it fine?
>
>>>> + Setting the value to < 128mA or > 8.128A terminates charging.
>>>
>>> "terminates charging" is a driver action, not a description of HW. It's
>>> fine to say that what min/max value should be specified in the property
>>> for it to be valid, but not what action SW should take in response to that.
>>
>> This isn't sw. This is defined in the HW documentation as to what
>> happens if an invalid value is used.
>
> OK, if the HW documentation already says that, I don't see the need for
> the DT binding to also say it; I would suggest just removing that text.
>
Will remove in v3.
-rhyland
--
nvpublic
On 9/20/2013 3:53 AM, Thierry Reding wrote:
>>>> + name = charger_device->pdata->name;
>>>> + if (!name) {
>>>> + name = kasprintf(GFP_KERNEL, "bq24735-%s",
>>>> + dev_name(&client->dev));
>>>
>>> Won't the device name already include bq24735 because of the driver
>>> name?
>>
>> In my experience this comes up with a name like "bq24735-5-0009". Thats
>> why I added the bq24735 in the beginning, so the name is more descriptive.
>
> Yes, you're right. Perhaps in that case it's even easier to just stick
> with a static string such as "bq24735" or "bq24735-charger". It's likely
> to be the only device of that type in a machine. If you want to include
> the device name, perhaps something like "%s/bq24735" (5-0009/bq24735) is
> clearer that 5-0009 is actually the bus-specific name.
>
So it turns out if I try to put in a '/' or '\' they are translated to
'!' in sysfs, so the name comes out to be '5-0009!bq24735'. Perhaps
'bq24735@5-0009' to still signify that the 5-009 is more of an address
than chip name? What do you think Thierry?
-rhyland
--
nvpublic
On Tue, Sep 24, 2013 at 02:13:02PM -0400, Rhyland Klein wrote:
> On 9/20/2013 3:53 AM, Thierry Reding wrote:
> >>>> + name = charger_device->pdata->name;
> >>>> + if (!name) {
> >>>> + name = kasprintf(GFP_KERNEL, "bq24735-%s",
> >>>> + dev_name(&client->dev));
> >>>
> >>> Won't the device name already include bq24735 because of the driver
> >>> name?
> >>
> >> In my experience this comes up with a name like "bq24735-5-0009". Thats
> >> why I added the bq24735 in the beginning, so the name is more descriptive.
> >
> > Yes, you're right. Perhaps in that case it's even easier to just stick
> > with a static string such as "bq24735" or "bq24735-charger". It's likely
> > to be the only device of that type in a machine. If you want to include
> > the device name, perhaps something like "%s/bq24735" (5-0009/bq24735) is
> > clearer that 5-0009 is actually the bus-specific name.
> >
>
> So it turns out if I try to put in a '/' or '\' they are translated to
> '!' in sysfs, so the name comes out to be '5-0009!bq24735'. Perhaps
> 'bq24735@5-0009' to still signify that the 5-009 is more of an address
> than chip name? What do you think Thierry?
I hadn't thought about that. @ looks like a better separator indeed.
Thierry
On 09/24/2013 10:39 AM, Rhyland Klein wrote:
> On 9/23/2013 6:05 PM, Stephen Warren wrote:
>> On 09/23/2013 04:01 PM, Rhyland Klein wrote:
>>> On 9/23/2013 5:53 PM, Stephen Warren wrote:
>>>> On 09/19/2013 10:18 AM, Rhyland Klein wrote:
>>>>> Adding driver support for bq24735 charger chipset.
>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt b/Documentation/devicetree/bindings/power_supply/ti,bq24735.txt
>>
>>>>> + - ti,charge-current : Used to control and set the charging current. This value
>>>>> + must follow the below guidelines:
>>>>> + bit 0 - 5: Not used
>>>>> + bit 6: 1 = Adds 64mA of charger current
>>>>> + bit 7: 1 = Adds 128mA of charger current
>>>>> + bit 8: 1 = Adds 256mA of charger current
>>>>> + bit 9: 1 = Adds 512mA of charger current
>>>>> + bit 10: 1 = Adds 1024mA of charger current
>>>>> + bit 11: 1 = Adds 2048mA of charger current
>>>>> + bit 12: 1 = Adds 4096mA of charger current
>>>>> + bit 13 - 15: Not used
>>>>
>>>> That's a little odd. Why not just put the number of mA directly into the
>>>> property unshifted?
>>>
>>> This is how the hw register is defined, its the literal number of mA.
>>> This is cleaned up in the upcoming revision.
>>
>> OK. If you still want to use the raw register encoding, which seems
>> reasonable, why not just say:
>>
>> ti,charge-current: Value for charge current register as described in the
>> HW documentation.
>
> I had written this for v3:
>
> - ti,charge-current : Used to control and set the charging current. This
> value must be between 128mA and 8.128A with a 64mA step resolution. The
> POR value is 0x0000h. See spec for more details.
>
> Do you think the range is unnecessary or is it fine?
It's probably fine, although I expect unnecessary. I would mention the
exact register name from the spec, to make searching for it easier.