[PATCH v1 0/4] tps6105x add devicetree and leds support

[PATCH v1 0/4] tps6105x add devicetree and leds support

[Sven Van Asbroeck]

I needed led operation for this mfd chip, so I added a very simple
driver for this.

My platform (arm imx6q) is devicetree-based, so I added optional
devicetree support for this chip and its sub-drivers.

Interestingly, the main mfd driver had a dt 'compatible' binding, but
could not operate without platform data?

Sven Van Asbroeck (4):
  tps6105x: add optional devicetree support
  regulator: tps6105x: add optional devicetree support
  leds: tps6105x: add driver for mfd chip led mode
  dt-bindings: mfd: update TI tps6105x chip bindings

 .../devicetree/bindings/mfd/tps6105x.txt      | 32 ++++++++-
 drivers/leds/Kconfig                          | 10 +++
 drivers/leds/Makefile                         |  1 +
 drivers/leds/leds-tps6105x.c                  | 68 +++++++++++++++++++
 drivers/mfd/tps6105x.c                        | 36 +++++++++-
 drivers/regulator/tps6105x-regulator.c        | 10 ++-
 6 files changed, 152 insertions(+), 5 deletions(-)
 create mode 100644 drivers/leds/leds-tps6105x.c

-- 
2.17.1

[PATCH v1 1/4] tps6105x: add optional devicetree support

[Sven Van Asbroeck]

This driver currently requires platform data to specify the
operational mode and regulator init data (in case of regulator
mode).

Optionally specify the operational mode by looking at the
'compatible' property of the devicetree child node.

Example: put chip in regulator mode:

i2c0 {
	tps61052@33 {
		compatible = "ti,tps61052";
		reg = <0x33>;

		regulator {
			compatible = "ti,tps6105x-regulator";
		};
	};
};

Tree: next-20191118
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/mfd/tps6105x.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/tps6105x.c b/drivers/mfd/tps6105x.c
index 6ac3607a79c2..401bd093b4ea 100644
--- a/drivers/mfd/tps6105x.c
+++ b/drivers/mfd/tps6105x.c
@@ -71,6 +71,7 @@ static struct mfd_cell tps6105x_gpio_cell = {
 
 static struct mfd_cell tps6105x_leds_cell = {
 	.name = "tps6105x-leds",
+	.of_compatible = "ti,tps6105x-leds",
 };
 
 static struct mfd_cell tps6105x_flash_cell = {
@@ -79,6 +80,7 @@ static struct mfd_cell tps6105x_flash_cell = {
 
 static struct mfd_cell tps6105x_regulator_cell = {
 	.name = "tps6105x-regulator",
+	.of_compatible = "ti,tps6105x-regulator",
 };
 
 static int tps6105x_add_device(struct tps6105x *tps6105x,
@@ -91,6 +93,32 @@ static int tps6105x_add_device(struct tps6105x *tps6105x,
 			       PLATFORM_DEVID_AUTO, cell, 1, NULL, 0, NULL);
 }
 
+static struct tps6105x_platform_data *tps6105x_parse_dt(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct tps6105x_platform_data *pdata;
+	struct device_node *child;
+
+	if (!np)
+		return ERR_PTR(-EINVAL);
+	if (of_get_available_child_count(np) > 1) {
+		dev_err(dev, "cannot support multiple operational modes");
+		return ERR_PTR(-EINVAL);
+	}
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+	pdata->mode = TPS6105X_MODE_SHUTDOWN;
+	for_each_available_child_of_node(np, child) {
+		if (of_device_is_compatible(child, "ti,tps6105x-regulator"))
+			pdata->mode = TPS6105X_MODE_VOLTAGE;
+		else if (of_device_is_compatible(child, "ti,tps6105x-leds"))
+			pdata->mode = TPS6105X_MODE_TORCH;
+	}
+
+	return pdata;
+}
+
 static int tps6105x_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -99,9 +127,11 @@ static int tps6105x_probe(struct i2c_client *client,
 	int ret;
 
 	pdata = dev_get_platdata(&client->dev);
-	if (!pdata) {
-		dev_err(&client->dev, "missing platform data\n");
-		return -ENODEV;
+	if (!pdata)
+		pdata = tps6105x_parse_dt(&client->dev);
+	if (IS_ERR(pdata)) {
+		dev_err(&client->dev, "No platform data or DT found");
+		return PTR_ERR(pdata);
 	}
 
 	tps6105x = devm_kmalloc(&client->dev, sizeof(*tps6105x), GFP_KERNEL);
-- 
2.17.1

Re: [PATCH v1 1/4] tps6105x: add optional devicetree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

On Mon, Nov 18, 2019 at 11:53:57AM -0500, Sven Van Asbroeck wrote:

> 	tps61052@33 {
> 		compatible = "ti,tps61052";
> 		reg = <0x33>;
> 
> 		regulator {
> 			compatible = "ti,tps6105x-regulator";
> 		};

We shouldn't need a compatible here, the MFD should just instantiate any
children it has.  This also doesn't document any actual regulators on
the device.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/4] tps6105x: add optional devicetree support

[Sven Van Asbroeck]

On Mon, Nov 18, 2019 at 12:01 PM Mark Brown <broonie@kernel.org> wrote:
>
> We shouldn't need a compatible here, the MFD should just instantiate any
> children it has.

If the child node doesn't have a compatible, how would the driver be
able to work
out the operational mode? The chip can only be in a single operational mode
at a time. So the child node has 'led' or 'regulator' compatible.

Or is there a more elegant method I've overlooked?

> This also doesn't document any actual regulators on
> the device.

The chip contains a single optional regulator only. I documented its
dt bindings later in the patchset.

Re: [PATCH v1 1/4] tps6105x: add optional devicetree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

On Mon, Nov 18, 2019 at 12:17:50PM -0500, Sven Van Asbroeck wrote:
> On Mon, Nov 18, 2019 at 12:01 PM Mark Brown <broonie@kernel.org> wrote:

> > We shouldn't need a compatible here, the MFD should just instantiate any
> > children it has.

> If the child node doesn't have a compatible, how would the driver be
> able to work
> out the operational mode? The chip can only be in a single operational mode
> at a time. So the child node has 'led' or 'regulator' compatible.

> Or is there a more elegant method I've overlooked?

So this is one device that has two separate modes?  This sounds like you
need a property specifying how the device is wired up, or possibly just
different compatibles at the root of the device though that's not quite
idiomatic.  Splitting this up with different devices is a bit of a Linux
specific implementation detail.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/4] tps6105x: add optional devicetree support

[Sven Van Asbroeck]

On Mon, Nov 18, 2019 at 12:45 PM Mark Brown <broonie@kernel.org> wrote:
>
> So this is one device that has two separate modes?  This sounds like you
> need a property specifying how the device is wired up, or possibly just
> different compatibles at the root of the device though that's not quite
> idiomatic.  Splitting this up with different devices is a bit of a Linux
> specific implementation detail.

Yes, that does make sense and sounds elegant. However, bear with me:

This mfd chip can be wired up as one of the following:
- gpio only
- gpio + regulator
- gpio + led
- gpio + flash

So I need a way to indicate this in the dt.

Imagine I do this with a text/value property, like this:

i2c0 {
        tps61052@33 {
                compatible = "ti,tps61050";
                reg = <0x33>;
                mode = "regulator"; // or
                mode = <TPS6105X_REGULATOR_MODE>;
        };
};

in this case, there is no elegant way to specify the regulator properties in
the devicetree. Except by grabbing a reference to a subnode perhaps. And then
I'd have to somehow make sure that the sub driver's device->of_node points
at this subnode, which the mfd core doesn't do automatically.

Now imagine I use a reference property:

i2c0 {
        tps61052@33 {
                compatible = "ti,tps61050";
                reg = <0x33>;
                mode = <&tps_reg>;

                tps_reg: regulator {
                        regulator-min-microvolt = <5000000>;
                        regulator-max-microvolt = <5000000>;
                        regulator-always-on;
                };
        };
};

However for this to work, I need to make sure the regulator subdriver gets a
valid dev->of_node, which the mfd core doesn't do automatically. So I'd have
to follow the 'mode' node, check that its compatible is correct, and then
manually assign the of_node to the mfd child driver (not sure how to even
do that).

So I arrived at the following:

i2c0 {
        tps61052@33 {
                compatible = "ti,tps61050";
                reg = <0x33>;

                regulator {
                        compatible = "ti,tps6105x-regulator";
                        regulator-min-microvolt = <5000000>;
                        regulator-max-microvolt = <5000000>;
                        regulator-always-on;
                };
        };
};

In this case, I only need to verify that there is at most one single subnode.
And when I specify of_compatible = "ti,tps6105x-regulator" in the mfd cell,
the mfd core will automatically assign the child driver's of_node. Nice 'n
elegant?

Would you be able to suggest a way forward?

Re: [PATCH v1 1/4] tps6105x: add optional devicetree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

On Mon, Nov 18, 2019 at 01:13:24PM -0500, Sven Van Asbroeck wrote:

> This mfd chip can be wired up as one of the following:
> - gpio only
> - gpio + regulator
> - gpio + led
> - gpio + flash

Is the regulator bit of this perhaps a voltage regulator and a current
regulator packaged together mainly for use powering LEDs?  That's a
hardware design I've seen before...

> in this case, there is no elegant way to specify the regulator properties in
> the devicetree. Except by grabbing a reference to a subnode perhaps. And then
> I'd have to somehow make sure that the sub driver's device->of_node points
> at this subnode, which the mfd core doesn't do automatically.

Just point the regulator framework at the MFD's DT node - the children
of the MFD can look at the parent device happily, there's several
existing MFDs do this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/4] tps6105x: add optional devicetree support

[Sven Van Asbroeck]

On Mon, Nov 18, 2019 at 3:34 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Nov 18, 2019 at 01:13:24PM -0500, Sven Van Asbroeck wrote:
>
> Is the regulator bit of this perhaps a voltage regulator and a current
> regulator packaged together mainly for use powering LEDs?  That's a
> hardware design I've seen before...

Yes, you nailed it :)

>
> Just point the regulator framework at the MFD's DT node - the children
> of the MFD can look at the parent device happily, there's several
> existing MFDs do this.

Ok, I think I get it now. Will prepare v2.

[PATCH v1 2/4] regulator: tps6105x: add optional devicetree support

[Sven Van Asbroeck]

If regulator init data is not specified in the parent mfd
device's platform data, attempt to retrieve it from the
devicetree node.

Example:

i2c0 {
	tps61052@33 {
		compatible = "ti,tps61052";
		reg = <0x33>;

		regulator {
			compatible = "ti,tps6105x-regulator";
			regulator-min-microvolt = <5000000>;
			regulator-max-microvolt = <5000000>;
			regulator-always-on;
		};
	};
};

Tree: next-20191118
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/regulator/tps6105x-regulator.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/tps6105x-regulator.c b/drivers/regulator/tps6105x-regulator.c
index 06059a94f7c6..9bc4e869fc4c 100644
--- a/drivers/regulator/tps6105x-regulator.c
+++ b/drivers/regulator/tps6105x-regulator.c
@@ -18,6 +18,7 @@
 #include <linux/regulator/driver.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/tps6105x.h>
+#include <linux/regulator/of_regulator.h>
 
 static const unsigned int tps6105x_voltages[] = {
 	4500000,
@@ -58,6 +59,7 @@ static int tps6105x_regulator_probe(struct platform_device *pdev)
 {
 	struct tps6105x *tps6105x = dev_get_platdata(&pdev->dev);
 	struct tps6105x_platform_data *pdata = tps6105x->pdata;
+	struct regulator_init_data *init_data = pdata->regulator_data;
 	struct regulator_config config = { };
 	int ret;
 
@@ -68,8 +70,14 @@ static int tps6105x_regulator_probe(struct platform_device *pdev)
 		return 0;
 	}
 
+	if (!init_data)
+		init_data = of_get_regulator_init_data(
+					&pdev->dev, pdev->dev.of_node,
+					&tps6105x_regulator_desc);
+	if (!init_data)
+		return -EINVAL;
 	config.dev = &tps6105x->client->dev;
-	config.init_data = pdata->regulator_data;
+	config.init_data = init_data;
 	config.driver_data = tps6105x;
 	config.regmap = tps6105x->regmap;
 
-- 
2.17.1

[PATCH v1 3/4] leds: tps6105x: add driver for mfd chip led mode

[Sven Van Asbroeck]

This driver adds support for the led operational mode of the
tps6105x mfd device.

Example usage, devicetree:

i2c0 {
	tps61052@33 {
	compatible = "ti,tps61052";
	reg = <0x33>;

		tps-led {
			compatible = "ti,tps6105x-leds";
		};
	};
};

Example usage, platform data in machine layer:

 #include <linux/mfd/tps6105x.h>

 struct tps6105x_platform_data pdata = {
         .mode = TPS6105X_MODE_TORCH,
 };

Tree: next-20191118
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/leds/Kconfig         | 10 ++++++
 drivers/leds/Makefile        |  1 +
 drivers/leds/leds-tps6105x.c | 68 ++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)
 create mode 100644 drivers/leds/leds-tps6105x.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 4b68520ac251..7c7ceaa824a2 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -836,6 +836,16 @@ config LEDS_LM36274
 	  Say Y to enable the LM36274 LED driver for TI LMU devices.
 	  This supports the LED device LM36274.
 
+config LEDS_TPS6105X
+	tristate "LED support for TI TPS6105X"
+	depends on LEDS_CLASS
+	depends on TPS6105X
+	default y if TPS6105X
+	help
+	  This driver supports TPS61050/TPS61052 led chips.
+	  It is a single boost converter primarily for white LEDs and
+	  audio amplifiers.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 2da39e896ce8..d7e1107753fb 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
 obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= leds-ti-lmu-common.o
 obj-$(CONFIG_LEDS_LM3697)		+= leds-lm3697.o
 obj-$(CONFIG_LEDS_LM36274)		+= leds-lm36274.o
+obj-$(CONFIG_LEDS_TPS6105X)		+= leds-tps6105x.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
diff --git a/drivers/leds/leds-tps6105x.c b/drivers/leds/leds-tps6105x.c
new file mode 100644
index 000000000000..c68fc5b180ed
--- /dev/null
+++ b/drivers/leds/leds-tps6105x.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/tps6105x.h>
+#include <linux/regmap.h>
+
+struct tps6105x_priv {
+	struct regmap *regmap;
+	struct led_classdev cdev;
+};
+
+static int tps6105x_brightness_set(struct led_classdev *cdev,
+				  enum led_brightness brightness)
+{
+	struct tps6105x_priv *priv = container_of(cdev, struct tps6105x_priv,
+							cdev);
+
+	return regmap_update_bits(priv->regmap, TPS6105X_REG_0,
+		TPS6105X_REG0_TORCHC_MASK,
+		brightness << TPS6105X_REG0_TORCHC_SHIFT);
+}
+
+static int tps6105x_led_probe(struct platform_device *pdev)
+{
+	struct tps6105x *tps6105x = dev_get_platdata(&pdev->dev);
+	struct tps6105x_platform_data *pdata = tps6105x->pdata;
+	struct device_node *np = pdev->dev.of_node;
+	struct tps6105x_priv *priv;
+	int ret;
+
+	/* This instance is not set for led mode so bail out */
+	if (pdata->mode != TPS6105X_MODE_TORCH) {
+		dev_info(&pdev->dev,
+			"chip not in torch mode, exit probe");
+		return -EINVAL;
+	}
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	priv->regmap = tps6105x->regmap;
+	priv->cdev.name = np ? np->name : "tps6105x";
+	priv->cdev.brightness_set_blocking = tps6105x_brightness_set;
+	priv->cdev.max_brightness = 7;
+
+	ret = regmap_update_bits(tps6105x->regmap, TPS6105X_REG_0,
+		TPS6105X_REG0_MODE_MASK | TPS6105X_REG0_TORCHC_MASK,
+		TPS6105X_REG0_MODE_TORCH << TPS6105X_REG0_MODE_SHIFT);
+	if (ret)
+		return ret;
+
+	return devm_led_classdev_register(&pdev->dev, &priv->cdev);
+}
+
+static struct platform_driver led_driver = {
+	.probe = tps6105x_led_probe,
+	.driver = {
+		.name = "tps6105x-leds",
+	},
+};
+
+module_platform_driver(led_driver);
+
+MODULE_DESCRIPTION("TPS6105x led driver");
+MODULE_AUTHOR("Sven Van Asbroeck <TheSven73@gmail.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH v1 4/4] dt-bindings: mfd: update TI tps6105x chip bindings

[Sven Van Asbroeck]

The driver has been extended to optionally get its operational
mode and regulator init data from the devicetree.

Tree: next-20191118
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 .../devicetree/bindings/mfd/tps6105x.txt      | 32 ++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/tps6105x.txt b/Documentation/devicetree/bindings/mfd/tps6105x.txt
index 93602c7a19c8..7c690812336a 100644
--- a/Documentation/devicetree/bindings/mfd/tps6105x.txt
+++ b/Documentation/devicetree/bindings/mfd/tps6105x.txt
@@ -7,7 +7,21 @@ Required properties:
 - compatible:		"ti,tps61050" or "ti,tps61052"
 - reg:			Specifies the I2C slave address
 
-Example:
+Optional subnode:
+
+The subnode selects the chip's operational mode.
+There can be at most one single subnode.
+
+- regulator
+	Required properties:
+		- compatible: "ti,tps6105x-regulator"
+	see Documentation/devicetree/bindings/regulator/regulator.txt
+
+- led
+	Required properties:
+		- compatible: "ti,tps6105x-leds"
+
+Example (GPIO operation only):
 
 i2c0 {
 	tps61052@33 {
@@ -15,3 +29,19 @@ i2c0 {
 		reg = <0x33>;
 	};
 };
+
+Example (GPIO + regulator operation):
+
+i2c0 {
+	tps61052@33 {
+		compatible = "ti,tps61052";
+		reg = <0x33>;
+
+		regulator {
+			compatible = "ti,tps6105x-regulator";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			regulator-always-on;
+		};
+	};
+};
-- 
2.17.1