This patchset adds support for the TPS65090-charger. This charger is
registered as a subdevice of the TPS65090 PMIC.
This series includes a fix for the tps65090 header file where all
the interrupts were shifted by 1.
v3:
- fixed up the tps65090-charger driver to fix build errors/warnings
v2:
- cleaned up tps65090 driver per comments (mostly var naming)
- flushed out commit description for driver
- reordered probe so irq is registered after power_supply and cleaned up 1st
Rhyland Klein (4):
mfd: tps65090: Fix enum in header file
mfd: tps65090: Add resources for charger
power_supply: tps65090-charger: Add binding doc
power: tps65090: Add support for tps65090-charger
.../devicetree/bindings/power_supply/tps65090.txt | 23 ++
drivers/mfd/tps65090.c | 10 +
drivers/power/Kconfig | 7 +
drivers/power/Makefile | 1 +
drivers/power/tps65090-charger.c | 315 ++++++++++++++++++++
include/linux/mfd/tps65090.h | 6 +
6 files changed, 362 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power_supply/tps65090.txt
create mode 100644 drivers/power/tps65090-charger.c
--
1.7.9.5
The enum is missing the definition for the first bit, which makes all
the rest off by one. Add definition for the TPS65090_IRQ_INTERRUPT bit
which at 0.
Signed-off-by: Rhyland Klein <[email protected]>
---
v3:
- no changes since v2
v2:
- no changes since v1
include/linux/mfd/tps65090.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/mfd/tps65090.h b/include/linux/mfd/tps65090.h
index 6694cf4..9ce231f 100644
--- a/include/linux/mfd/tps65090.h
+++ b/include/linux/mfd/tps65090.h
@@ -27,6 +27,7 @@
/* TPS65090 IRQs */
enum {
+ TPS65090_IRQ_INTERRUPT,
TPS65090_IRQ_VAC_STATUS_CHANGE,
TPS65090_IRQ_VSYS_STATUS_CHANGE,
TPS65090_IRQ_BAT_STATUS_CHANGE,
--
1.7.9.5
This change adds the binding documentation for the tps65090-charger.
Signed-off-by: Rhyland Klein <[email protected]>
---
v3:
- no changes since v2
v2:
- no changes since v1
.../devicetree/bindings/power_supply/tps65090.txt | 23 ++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power_supply/tps65090.txt
diff --git a/Documentation/devicetree/bindings/power_supply/tps65090.txt b/Documentation/devicetree/bindings/power_supply/tps65090.txt
new file mode 100644
index 0000000..56370c7
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_supply/tps65090.txt
@@ -0,0 +1,23 @@
+TPS65090 Frontend PMU with Switchmode Charger
+
+Required Properties:
+-compatible: "ti,tps65090"
+-reg: I2C slave address
+-interrupts: the interrupt output to which this device connects
+
+Optional Properties:
+-ti,enable-low-current-chrg: Enables charging when a low current is detected
+ while the default logic is to stop charging.
+
+Example:
+
+ tps65090@48 {
+ compatible = "ti,tps65090";
+ reg = <0x48>;
+ interrupts = <0 88 0x4>;
+
+ ti,enable-low-current-chrg;
+
+ regulators {
+ ...
+ };
--
1.7.9.5
This patch adds support for the tps65090 charger driver. This driver is
responsible for controlling the charger aspect of the tps65090 mfd.
Currently, this mainly consists of turning on and off the charger, but
some features of the charger can be supported through this driver
including:
Enable Auto Recharge based on Battery voltage
Fast Charge Safety Timer
Maximum battery discharge current
Maximum battery adapter current
Enable External Charge
Disable charging termination based on low charger current (supported)
Once the driver is accepted, later patches can add support for the
features above which are not yet supported.
Based on work by:
Syed Rafiuddin <[email protected]>
Laxman Dewangan <[email protected]>
Signed-off-by: Rhyland Klein <[email protected]>
---
v3:
- fixed build errors/warnings around devm_free_irq and unused irq label
v2:
- updated commit description with more info
- cleaned up misc comments (var names, spacing, etc)
- reordered probe around irq so it is cleaner
drivers/power/Kconfig | 7 +
drivers/power/Makefile | 1 +
drivers/power/tps65090-charger.c | 315 ++++++++++++++++++++++++++++++++++++++
include/linux/mfd/tps65090.h | 5 +
4 files changed, 328 insertions(+)
create mode 100644 drivers/power/tps65090-charger.c
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 9e00c38..0c5d336 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -340,6 +340,13 @@ config CHARGER_SMB347
Say Y to include support for Summit Microelectronics SMB347
Battery Charger.
+config CHARGER_TPS65090
+ tristate "TPS65090 battery charger driver"
+ depends on MFD_TPS65090
+ help
+ Say Y here to enable support for battery charging with TPS65090
+ PMIC chips.
+
config AB8500_BM
bool "AB8500 Battery Management Driver"
depends on AB8500_CORE && AB8500_GPADC
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 3f66436..8720987 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -53,4 +53,5 @@ obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
obj-$(CONFIG_CHARGER_BQ2415X) += bq2415x_charger.o
obj-$(CONFIG_POWER_AVS) += avs/
obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
+obj-$(CONFIG_CHARGER_TPS65090) += tps65090-charger.o
obj-$(CONFIG_POWER_RESET) += reset/
diff --git a/drivers/power/tps65090-charger.c b/drivers/power/tps65090-charger.c
new file mode 100644
index 0000000..0c66c66
--- /dev/null
+++ b/drivers/power/tps65090-charger.c
@@ -0,0 +1,315 @@
+/*
+ * Battery charger driver for TI's tps65090
+ *
+ * Copyright (c) 2013, NVIDIA CORPORATION. All rights reserved.
+
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+
+ * This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/mfd/tps65090.h>
+
+#define TPS65090_REG_INTR_STS 0x00
+#define TPS65090_REG_CG_CTRL0 0x04
+#define TPS65090_REG_CG_CTRL1 0x05
+#define TPS65090_REG_CG_CTRL2 0x06
+#define TPS65090_REG_CG_CTRL3 0x07
+#define TPS65090_REG_CG_CTRL4 0x08
+#define TPS65090_REG_CG_CTRL5 0x09
+#define TPS65090_REG_CG_STATUS1 0x0a
+#define TPS65090_REG_CG_STATUS2 0x0b
+
+#define TPS65090_CHARGER_ENABLE BIT(0)
+#define TPS65090_VACG BIT(1)
+#define TPS65090_NOITERM BIT(5)
+
+struct tps65090_charger {
+ struct device *dev;
+ int ac_online;
+ int prev_ac_online;
+ int irq;
+ struct power_supply ac;
+ struct tps65090_platform_data *pdata;
+};
+
+static enum power_supply_property tps65090_ac_props[] = {
+ POWER_SUPPLY_PROP_ONLINE,
+};
+
+static int tps65090_low_chrg_current(struct tps65090_charger *charger)
+{
+ int ret;
+
+ ret = tps65090_write(charger->dev->parent, TPS65090_REG_CG_CTRL5,
+ TPS65090_NOITERM);
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
+ __func__, TPS65090_REG_CG_CTRL5);
+ return ret;
+ }
+ return 0;
+}
+
+static int tps65090_enable_charging(struct tps65090_charger *charger,
+ uint8_t enable)
+{
+ int ret;
+ uint8_t ctrl0 = 0;
+
+ ret = tps65090_read(charger->dev->parent, TPS65090_REG_CG_CTRL0,
+ &ctrl0);
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
+ __func__, TPS65090_REG_CG_CTRL0);
+ return ret;
+ }
+
+ ret = tps65090_write(charger->dev->parent, TPS65090_REG_CG_CTRL0,
+ (ctrl0 | TPS65090_CHARGER_ENABLE));
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
+ __func__, TPS65090_REG_CG_CTRL0);
+ return ret;
+ }
+ return 0;
+}
+
+static int tps65090_config_charger(struct tps65090_charger *charger)
+{
+ int ret;
+
+ if (charger->pdata->enable_low_current_chrg) {
+ ret = tps65090_low_chrg_current(charger);
+ if (ret < 0) {
+ dev_err(charger->dev,
+ "error configuring low charge current\n");
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int tps65090_ac_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct tps65090_charger *charger = container_of(psy,
+ struct tps65090_charger, ac);
+
+ if (psp == POWER_SUPPLY_PROP_ONLINE) {
+ val->intval = charger->ac_online;
+ charger->prev_ac_online = charger->ac_online;
+ return 0;
+ }
+ return -EINVAL;
+}
+
+static irqreturn_t tps65090_charger_isr(int irq, void *dev_id)
+{
+ struct tps65090_charger *charger = dev_id;
+ int ret;
+ uint8_t status1 = 0;
+ uint8_t intrsts = 0;
+
+ ret = tps65090_read(charger->dev->parent, TPS65090_REG_CG_STATUS1,
+ &status1);
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): Error in reading reg 0x%x\n",
+ __func__, TPS65090_REG_CG_STATUS1);
+ return IRQ_HANDLED;
+ }
+ msleep(75);
+ ret = tps65090_read(charger->dev->parent, TPS65090_REG_INTR_STS,
+ &intrsts);
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): Error in reading reg 0x%x\n",
+ __func__, TPS65090_REG_INTR_STS);
+ return IRQ_HANDLED;
+ }
+
+ if (intrsts & TPS65090_VACG) {
+ ret = tps65090_enable_charging(charger, 1);
+ if (ret < 0)
+ return IRQ_HANDLED;
+ charger->ac_online = 1;
+ } else {
+ charger->ac_online = 0;
+ }
+
+ if (charger->prev_ac_online != charger->ac_online)
+ power_supply_changed(&charger->ac);
+
+ return IRQ_HANDLED;
+}
+
+#if defined(CONFIG_OF)
+
+#include <linux/of_device.h>
+
+static struct tps65090_platform_data *
+ tps65090_parse_dt_charger_data(struct platform_device *pdev)
+{
+ struct tps65090_platform_data *pdata;
+ struct device_node *np = pdev->dev.parent->of_node;
+ unsigned int prop;
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_err(&pdev->dev, "Memory alloc for tps65090_pdata failed\n");
+ return NULL;
+ }
+
+ prop = of_property_read_bool(np, "ti,enable-low-current-chrg");
+ pdata->enable_low_current_chrg = prop;
+
+ pdata->irq_base = -1;
+
+ return pdata;
+
+}
+#else
+static struct tps65090_platform_data *
+ tps65090_parse_dt_charger_data(struct platform_device *pdev)
+{
+ return NULL;
+}
+#endif
+
+static int tps65090_charger_probe(struct platform_device *pdev)
+{
+ struct tps65090 *tps65090_mfd = dev_get_drvdata(pdev->dev.parent);
+ struct tps65090_charger *cdata;
+ struct tps65090_platform_data *pdata;
+ uint8_t status1 = 0;
+ int ret;
+ int irq;
+
+ pdata = dev_get_platdata(pdev->dev.parent);
+
+ if (!pdata && tps65090_mfd->dev->of_node)
+ pdata = tps65090_parse_dt_charger_data(pdev);
+
+ if (!pdata) {
+ dev_err(&pdev->dev, "%s():no platform data available\n",
+ __func__);
+ return -ENODEV;
+ }
+
+ cdata = devm_kzalloc(&pdev->dev, sizeof(*cdata), GFP_KERNEL);
+ if (!cdata) {
+ dev_err(&pdev->dev, "failed to allocate memory status\n");
+ return -ENOMEM;
+ }
+
+ dev_set_drvdata(&pdev->dev, cdata);
+
+ cdata->dev = &pdev->dev;
+ cdata->pdata = pdata;
+
+ cdata->ac.name = "tps65090-ac";
+ cdata->ac.type = POWER_SUPPLY_TYPE_MAINS;
+ cdata->ac.get_property = tps65090_ac_get_property;
+ cdata->ac.properties = tps65090_ac_props;
+ cdata->ac.num_properties = ARRAY_SIZE(tps65090_ac_props);
+ cdata->ac.supplied_to = pdata->supplied_to;
+ cdata->ac.num_supplicants = pdata->num_supplicants;
+
+ ret = power_supply_register(&pdev->dev, &cdata->ac);
+ if (ret) {
+ dev_err(&pdev->dev, "failed: power supply register\n");
+ return ret;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq <= 0) {
+ dev_warn(&pdev->dev, "Unable to get charger irq = %d\n", irq);
+ ret = irq;
+ goto fail_unregister_supply;
+ }
+
+ cdata->irq = irq;
+
+ ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+ tps65090_charger_isr, 0, "tps65090-charger", cdata);
+ if (ret) {
+ dev_err(cdata->dev, "Unable to register irq %d err %d\n", irq,
+ ret);
+ goto fail_free_irq;
+ }
+
+ ret = tps65090_config_charger(cdata);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "charger config failed, err %d\n", ret);
+ goto fail_free_irq;
+ }
+
+ /* Check for charger presence */
+ ret = tps65090_read(cdata->dev->parent, TPS65090_REG_CG_STATUS1,
+ &status1);
+ if (ret < 0) {
+ dev_err(cdata->dev, "%s(): Error in reading reg 0x%x", __func__,
+ TPS65090_REG_CG_STATUS1);
+ goto fail_free_irq;
+ }
+
+ if (status1 != 0) {
+ ret = tps65090_enable_charging(cdata, 1);
+ if (ret < 0) {
+ dev_err(cdata->dev, "error enabling charger\n");
+ goto fail_free_irq;
+ }
+ cdata->ac_online = 1;
+ power_supply_changed(&cdata->ac);
+ }
+
+ return 0;
+
+fail_free_irq:
+ devm_free_irq(cdata->dev, irq, cdata);
+fail_unregister_supply:
+ power_supply_unregister(&cdata->ac);
+
+ return ret;
+}
+
+static int tps65090_charger_remove(struct platform_device *pdev)
+{
+ struct tps65090_charger *cdata = dev_get_drvdata(&pdev->dev);
+
+ devm_free_irq(cdata->dev, cdata->irq, cdata);
+ power_supply_unregister(&cdata->ac);
+
+ return 0;
+}
+
+static struct platform_driver tps65090_charger_driver = {
+ .driver = {
+ .name = "tps65090-charger",
+ .owner = THIS_MODULE,
+ },
+ .probe = tps65090_charger_probe,
+ .remove = tps65090_charger_remove,
+};
+module_platform_driver(tps65090_charger_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Syed Rafiuddin <[email protected]>");
+MODULE_DESCRIPTION("tps65090 battery charger driver");
diff --git a/include/linux/mfd/tps65090.h b/include/linux/mfd/tps65090.h
index 9ce231f..3f43069 100644
--- a/include/linux/mfd/tps65090.h
+++ b/include/linux/mfd/tps65090.h
@@ -87,6 +87,11 @@ struct tps65090_regulator_plat_data {
struct tps65090_platform_data {
int irq_base;
+
+ char **supplied_to;
+ size_t num_supplicants;
+ int enable_low_current_chrg;
+
struct tps65090_regulator_plat_data *reg_pdata[TPS65090_REGULATOR_MAX];
};
--
1.7.9.5
Add irq resources to pass to the charger mfd sub dev so
the charger can listen for interrupts.
Signed-off-by: Rhyland Klein <[email protected]>
---
v3:
- no changes since v2
v2:
- no changes since v1
drivers/mfd/tps65090.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
index 98edb5be..88846ae 100644
--- a/drivers/mfd/tps65090.c
+++ b/drivers/mfd/tps65090.c
@@ -56,12 +56,22 @@
#define TPS65090_INT2_MASK_OVERLOAD_FET6 6
#define TPS65090_INT2_MASK_OVERLOAD_FET7 7
+static struct resource charger_resources[] = {
+ {
+ .start = TPS65090_IRQ_VAC_STATUS_CHANGE,
+ .end = TPS65090_IRQ_VAC_STATUS_CHANGE,
+ .flags = IORESOURCE_IRQ,
+ }
+};
+
static struct mfd_cell tps65090s[] = {
{
.name = "tps65090-pmic",
},
{
.name = "tps65090-charger",
+ .num_resources = ARRAY_SIZE(charger_resources),
+ .resources = &charger_resources[0],
},
};
--
1.7.9.5
On 03/12/2013 04:08 PM, Rhyland Klein wrote:
> This change adds the binding documentation for the tps65090-charger.
> diff --git a/Documentation/devicetree/bindings/power_supply/tps65090.txt b/Documentation/devicetree/bindings/power_supply/tps65090.txt
> +Example:
> +
> + tps65090@48 {
> + compatible = "ti,tps65090";
> + reg = <0x48>;
> + interrupts = <0 88 0x4>;
> +
> + ti,enable-low-current-chrg;
> +
> + regulators {
> + ...
> + };
I'm a little confused by this binding.
What goes in the regulators sub-node; is that specified by another
binding file in bindings/regulator/tps65090.txt?
I would expect one of the following:
1) A single binding file that describes absolutely everything in the
chip. In this case, the main TPS65909 node wouldn't have child nodes for
the MFD components, although the regulators sub-node, which in turn
contains children does still make sense.
2) A separate binding for each component block, and perhaps also some
top-level binding that indicates which child bindings can "plug into"
it. In this case, I'd expect each block to be represented as a sub-node
in DT. The overall regulator component might then still have a
regulators child DT node itself, to represent each regulator's
configuration. In this scenario, each binding document describes the
entirety of a single node.
I think what you've got here is a hybrid; a single top-level node, but
different binding documents defining the various properties that are
relevant to each component block in the device. That seems odd to me.
On 3/12/2013 7:10 PM, Stephen Warren wrote:
> On 03/12/2013 04:08 PM, Rhyland Klein wrote:
>> This change adds the binding documentation for the tps65090-charger.
>> diff --git a/Documentation/devicetree/bindings/power_supply/tps65090.txt b/Documentation/devicetree/bindings/power_supply/tps65090.txt
>> +Example:
>> +
>> + tps65090@48 {
>> + compatible = "ti,tps65090";
>> + reg = <0x48>;
>> + interrupts = <0 88 0x4>;
>> +
>> + ti,enable-low-current-chrg;
>> +
>> + regulators {
>> + ...
>> + };
> I'm a little confused by this binding.
>
> What goes in the regulators sub-node; is that specified by another
> binding file in bindings/regulator/tps65090.txt?
>
> I would expect one of the following:
>
> 1) A single binding file that describes absolutely everything in the
> chip. In this case, the main TPS65909 node wouldn't have child nodes for
> the MFD components, although the regulators sub-node, which in turn
> contains children does still make sense.
>
> 2) A separate binding for each component block, and perhaps also some
> top-level binding that indicates which child bindings can "plug into"
> it. In this case, I'd expect each block to be represented as a sub-node
> in DT. The overall regulator component might then still have a
> regulators child DT node itself, to represent each regulator's
> configuration. In this scenario, each binding document describes the
> entirety of a single node.
>
> I think what you've got here is a hybrid; a single top-level node, but
> different binding documents defining the various properties that are
> relevant to each component block in the device. That seems odd to me.
Yes we started this discussion before and were discussing the proper
arrangement of
documentation when dealing with devices like these. This is where the
drivers/ directory
naming in the binding docs might diverge a bit as it might make less
sense to have
a binding doc for each child component of an mfd.
I was thinking about moving this driver towards #1 above, and using a
child node for
the charger. I would then also move the regulators to a child node, and
its structure would
be very similar to the Palmas driver/dt representation. My only concern
was that, from
what I understood, separating out the child node implied that the child
functionality
could/might be used somewhere else. Say in this case, that the charger
functionality might
be duplicated in another pmic from ti. I don't know how much that is the
case with the
tps65090 and so I am unsure if child nodes are the correct way to go.
As for #2, This would also be fine with me, as logically we are talking
about a single chip. I
this the only concern here is where to place a single binding document
in the bindings
directory where it makes sense. Putting regulator documentation under
charger or vice
versa doesn't make sense. And then for some devices, they might also
have an rtc, gpio
controller, interrupt controller, etc.. If each of them had a driver and
their own dt
information, I don't know where a single core place for all that
documentation would be
right now.
Hence, I was hoping to continue this dicussion and see if we can decide
on the most logical
choice, whatever that may be.
-rhyland
--
nvpublic
On 03/13/2013 01:25 PM, Rhyland Klein wrote:
> On 3/12/2013 7:10 PM, Stephen Warren wrote:
>> On 03/12/2013 04:08 PM, Rhyland Klein wrote:
>>> This change adds the binding documentation for the tps65090-charger.
>>> diff --git
>>> a/Documentation/devicetree/bindings/power_supply/tps65090.txt
>>> b/Documentation/devicetree/bindings/power_supply/tps65090.txt
>>> +Example:
>>> +
>>> + tps65090@48 {
>>> + compatible = "ti,tps65090";
>>> + reg = <0x48>;
>>> + interrupts = <0 88 0x4>;
>>> +
>>> + ti,enable-low-current-chrg;
>>> +
>>> + regulators {
>>> + ...
>>> + };
>> I'm a little confused by this binding.
>>
>> What goes in the regulators sub-node; is that specified by another
>> binding file in bindings/regulator/tps65090.txt?
>>
>> I would expect one of the following:
>>
>> 1) A single binding file that describes absolutely everything in the
>> chip. In this case, the main TPS65909 node wouldn't have child nodes for
>> the MFD components, although the regulators sub-node, which in turn
>> contains children does still make sense.
>>
>> 2) A separate binding for each component block, and perhaps also some
>> top-level binding that indicates which child bindings can "plug into"
>> it. In this case, I'd expect each block to be represented as a sub-node
>> in DT. The overall regulator component might then still have a
>> regulators child DT node itself, to represent each regulator's
>> configuration. In this scenario, each binding document describes the
>> entirety of a single node.
>>
>> I think what you've got here is a hybrid; a single top-level node, but
>> different binding documents defining the various properties that are
>> relevant to each component block in the device. That seems odd to me.
>
> Yes we started this discussion before and were discussing the proper
> arrangement of
(your message was wrapped far over 80 columns, please restrict to
something like 72-74 or so).
> documentation when dealing with devices like these. This is where the
> drivers/ directory
> naming in the binding docs might diverge a bit as it might make less
> sense to have
> a binding doc for each child component of an mfd.
I think your discussion swaps #1 and #2 relative to my list above? It
certainly makes more sense if I read it that way.
> I was thinking about moving this driver towards #1 above, and using a
> child node for
> the charger. I would then also move the regulators to a child node, and
> its structure would
> be very similar to the Palmas driver/dt representation. My only concern
> was that, from
> what I understood, separating out the child node implied that the child
> functionality
> could/might be used somewhere else. Say in this case, that the charger
> functionality might
> be duplicated in another pmic from ti.
Having separate nodes certainly makes it easy to /allow/ that. I don't
think it /requires/ that TI release (or plan to release) another chip
containing that IP block.
> I don't know how much that is the case with the
> tps65090 and so I am unsure if child nodes are the correct way to go.
Thinking more about this, separate child nodes for separate IP blocks do
start to make some sense. They definitely do allow easy
binding/driver/... re-use if future chips come out with the same
sub-block. Similarly, they neatly isolate each sub-block's binding from
the others, so (a) avoid any conflicts between what the different IP
blocks want to put in their nodes, like a list of regulators vs. using
sub-nodes for other purposes, such as an integrate I2C mux with I2C bus
child nodes, etc, and (b) the bindings can be defined separately thus
avoiding the "where do we put the binding file" issue.
It's certainly more work, but now I tend to think separate nodes are the
way to go.
> As for #2, This would also be fine with me, as logically we are talking
> about a single chip. I
> this the only concern here is where to place a single binding document
> in the bindings
> directory where it makes sense. Putting regulator documentation under
> charger or vice
> versa doesn't make sense. And then for some devices, they might also
> have an rtc, gpio
> controller, interrupt controller, etc.. If each of them had a driver and
> their own dt
> information, I don't know where a single core place for all that
> documentation would be
> right now.
>
> Hence, I was hoping to continue this dicussion and see if we can decide
> on the most logical
> choice, whatever that may be.
On 3/13/2013 4:41 PM, Stephen Warren wrote:
> On 03/13/2013 01:25 PM, Rhyland Klein wrote:
>> On 3/12/2013 7:10 PM, Stephen Warren wrote:
>>> On 03/12/2013 04:08 PM, Rhyland Klein wrote:
>>>> This change adds the binding documentation for the tps65090-charger.
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/power_supply/tps65090.txt
>>>> b/Documentation/devicetree/bindings/power_supply/tps65090.txt
>>>> +Example:
>>>> +
>>>> + tps65090@48 {
>>>> + compatible = "ti,tps65090";
>>>> + reg = <0x48>;
>>>> + interrupts = <0 88 0x4>;
>>>> +
>>>> + ti,enable-low-current-chrg;
>>>> +
>>>> + regulators {
>>>> + ...
>>>> + };
>>> I'm a little confused by this binding.
>>>
>>> What goes in the regulators sub-node; is that specified by another
>>> binding file in bindings/regulator/tps65090.txt?
>>>
>>> I would expect one of the following:
>>>
>>> 1) A single binding file that describes absolutely everything in the
>>> chip. In this case, the main TPS65909 node wouldn't have child nodes for
>>> the MFD components, although the regulators sub-node, which in turn
>>> contains children does still make sense.
>>>
>>> 2) A separate binding for each component block, and perhaps also some
>>> top-level binding that indicates which child bindings can "plug into"
>>> it. In this case, I'd expect each block to be represented as a sub-node
>>> in DT. The overall regulator component might then still have a
>>> regulators child DT node itself, to represent each regulator's
>>> configuration. In this scenario, each binding document describes the
>>> entirety of a single node.
>>>
>>> I think what you've got here is a hybrid; a single top-level node, but
>>> different binding documents defining the various properties that are
>>> relevant to each component block in the device. That seems odd to me.
>>
>> Yes we started this discussion before and were discussing the proper
>> arrangement of
>
> (your message was wrapped far over 80 columns, please restrict to
> something like 72-74 or so).
Apparently line wrapping only works properly on thunderbird if you force
it to plain text (which of course I think I had...) all better now.
>
>> documentation when dealing with devices like these. This is where the
>> drivers/ directory
>> naming in the binding docs might diverge a bit as it might make less
>> sense to have
>> a binding doc for each child component of an mfd.
>
> I think your discussion swaps #1 and #2 relative to my list above? It
> certainly makes more sense if I read it that way.
Yep you're right, I swapped them.
>
>> I was thinking about moving this driver towards #1 above, and using a
>> child node for
>> the charger. I would then also move the regulators to a child node, and
>> its structure would
>> be very similar to the Palmas driver/dt representation. My only concern
>> was that, from
>> what I understood, separating out the child node implied that the child
>> functionality
>> could/might be used somewhere else. Say in this case, that the charger
>> functionality might
>> be duplicated in another pmic from ti.
>
> Having separate nodes certainly makes it easy to /allow/ that. I don't
> think it /requires/ that TI release (or plan to release) another chip
> containing that IP block.
>
>> I don't know how much that is the case with the
>> tps65090 and so I am unsure if child nodes are the correct way to go.
>
> Thinking more about this, separate child nodes for separate IP blocks do
> start to make some sense. They definitely do allow easy
> binding/driver/... re-use if future chips come out with the same
> sub-block. Similarly, they neatly isolate each sub-block's binding from
> the others, so (a) avoid any conflicts between what the different IP
> blocks want to put in their nodes, like a list of regulators vs. using
> sub-nodes for other purposes, such as an integrate I2C mux with I2C bus
> child nodes, etc, and (b) the bindings can be defined separately thus
> avoiding the "where do we put the binding file" issue.
>
> It's certainly more work, but now I tend to think separate nodes are the
> way to go.
I agree, I think this is definitely a much cleaner way of doing it, then
each component gets its own documentation file and is clean.
-rhyland
--
nvpublic
On Tue, Mar 12, 2013 at 06:08:05PM -0400, Rhyland Klein wrote:
> This patchset adds support for the TPS65090-charger. This charger is
> registered as a subdevice of the TPS65090 PMIC.
>
> This series includes a fix for the tps65090 header file where all
> the interrupts were shifted by 1.
Thanks for the patches, Rhyland! 3/4 and 4/4 applied to the
battery-2.6.git tree.
Anton
On 3/18/2013 10:24 PM, Anton Vorontsov wrote:
> On Tue, Mar 12, 2013 at 06:08:05PM -0400, Rhyland Klein wrote:
>> This patchset adds support for the TPS65090-charger. This charger is
>> registered as a subdevice of the TPS65090 PMIC.
>>
>> This series includes a fix for the tps65090 header file where all
>> the interrupts were shifted by 1.
>
> Thanks for the patches, Rhyland! 3/4 and 4/4 applied to the
> battery-2.6.git tree.
>
> Anton
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
I was planning on posting a slightly changed v4 sometime soon which took
care of some concerns on the Documentation and added a compatibility
entry for the charger. Do you want me to send that still and you can
replace v3 with v4?
-rhyland
--
nvpublic
On Tue, Mar 19, 2013 at 11:55:33AM -0400, Rhyland Klein wrote:
> On 3/18/2013 10:24 PM, Anton Vorontsov wrote:
> > On Tue, Mar 12, 2013 at 06:08:05PM -0400, Rhyland Klein wrote:
> >> This patchset adds support for the TPS65090-charger. This charger is
> >> registered as a subdevice of the TPS65090 PMIC.
> >>
> >> This series includes a fix for the tps65090 header file where all
> >> the interrupts were shifted by 1.
> >
> > Thanks for the patches, Rhyland! 3/4 and 4/4 applied to the
> > battery-2.6.git tree.
> >
> I was planning on posting a slightly changed v4 sometime soon which took
> care of some concerns on the Documentation and added a compatibility
> entry for the charger. Do you want me to send that still and you can
> replace v3 with v4?
I would rather avoid replacing, i.e. I'd prefer if you send an update on
top of the v3 (incremental patch). If the changes were inspired by
someone, feel free to add a Suggested-by: tag. ;-)
Thanks!
Anton
On 3/19/2013 11:59 AM, Anton Vorontsov wrote:
> On Tue, Mar 19, 2013 at 11:55:33AM -0400, Rhyland Klein wrote:
>> On 3/18/2013 10:24 PM, Anton Vorontsov wrote:
>>> On Tue, Mar 12, 2013 at 06:08:05PM -0400, Rhyland Klein wrote:
>>>> This patchset adds support for the TPS65090-charger. This charger is
>>>> registered as a subdevice of the TPS65090 PMIC.
>>>>
>>>> This series includes a fix for the tps65090 header file where all
>>>> the interrupts were shifted by 1.
>>>
>>> Thanks for the patches, Rhyland! 3/4 and 4/4 applied to the
>>> battery-2.6.git tree.
>>>
>> I was planning on posting a slightly changed v4 sometime soon which took
>> care of some concerns on the Documentation and added a compatibility
>> entry for the charger. Do you want me to send that still and you can
>> replace v3 with v4?
>
> I would rather avoid replacing, i.e. I'd prefer if you send an update on
> top of the v3 (incremental patch). If the changes were inspired by
> someone, feel free to add a Suggested-by: tag. ;-)
>
> Thanks!
>
> Anton
>
Will Do! the changes won't be big I don't think. Thanks!
-rhyland
--
nvpublic
On 3/12/2013 6:08 PM, Rhyland Klein wrote:
> Add irq resources to pass to the charger mfd sub dev so
> the charger can listen for interrupts.
>
> Signed-off-by: Rhyland Klein <[email protected]>
> ---
> v3:
> - no changes since v2
> v2:
> - no changes since v1
>
> drivers/mfd/tps65090.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
> index 98edb5be..88846ae 100644
> --- a/drivers/mfd/tps65090.c
> +++ b/drivers/mfd/tps65090.c
> @@ -56,12 +56,22 @@
> #define TPS65090_INT2_MASK_OVERLOAD_FET6 6
> #define TPS65090_INT2_MASK_OVERLOAD_FET7 7
>
> +static struct resource charger_resources[] = {
> + {
> + .start = TPS65090_IRQ_VAC_STATUS_CHANGE,
> + .end = TPS65090_IRQ_VAC_STATUS_CHANGE,
> + .flags = IORESOURCE_IRQ,
> + }
> +};
> +
> static struct mfd_cell tps65090s[] = {
> {
> .name = "tps65090-pmic",
> },
> {
> .name = "tps65090-charger",
> + .num_resources = ARRAY_SIZE(charger_resources),
> + .resources = &charger_resources[0],
> },
> };
>
>
Samuel, Anton merged patches 3 & 4 of this series. Have you had a chance
to review patches 1 & 2 ?
-rhyland
--
nvpublic