[PATCH v5 0/2] tps6105x add devicetree and leds support

[PATCH v5 0/2] tps6105x add devicetree and leds support

[Sven Van Asbroeck]

v4 -> v5:
	Added Jacek Anaszewski's Acked-by tag on both patches.
	Added Rob Herring's Reviewed-by tag on devicetree patch.
	Lee Jones:
	- tweaked commit message s/led/LED/
	- use relative paths in Devicetree binding docs, line up ':'s

v3 -> v4:
	Removed tps6105 mfd patch - it was accepted (Mark Brown).
	
	Use the new LED registration API - suggested by Jacek Anaszewski.
	
	Updated led dt bindings to document function, color usage.

v2 -> v3:
	Removed tps6105x regulator patch - it was accepted (Mark Brown).
	
	Removed devicetree/platdata bindings for tps6105x led naming.
	I can test only with a 4.19 vendor kernel, which does not have the
	latest led naming infrastructure (function/color). Drop devicetree/
	fwnode/pdata led naming in favour of hard-coding to "tps6105x::torch",
	so the patch can be tested by me, yet remains acceptable to upstream.

v1 -> v2:
	Select chip operational mode by looking at subnode name, _not_ its
	compatible property. Suggested by Mark Brown.

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.

Sven Van Asbroeck (2):
  leds: tps6105x: add driver for mfd chip led mode
  dt-bindings: mfd: update TI tps6105x chip bindings

 .../devicetree/bindings/mfd/tps6105x.txt      | 47 ++++++++++-
 drivers/leds/Kconfig                          | 10 +++
 drivers/leds/Makefile                         |  1 +
 drivers/leds/leds-tps6105x.c                  | 83 +++++++++++++++++++
 4 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 drivers/leds/leds-tps6105x.c

-- 
2.17.1

[PATCH v5 1/2] 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.

Tree: next-20191118
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/leds/Kconfig         | 10 +++++
 drivers/leds/Makefile        |  1 +
 drivers/leds/leds-tps6105x.c | 83 ++++++++++++++++++++++++++++++++++++
 3 files changed, 94 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..ea2afaa3e3f0
--- /dev/null
+++ b/drivers/leds/leds-tps6105x.c
@@ -0,0 +1,83 @@
+// 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;
+	struct fwnode_handle *fwnode;
+};
+
+static void tps6105x_handle_put(void *data)
+{
+	struct tps6105x_priv *priv = data;
+
+	fwnode_handle_put(priv->fwnode);
+}
+
+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 led_init_data init_data = { };
+	struct tps6105x_priv *priv;
+	int ret;
+
+	/* This instance is not set for torch 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->fwnode = device_get_next_child_node(pdev->dev.parent, NULL);
+	ret = devm_add_action_or_reset(&pdev->dev, tps6105x_handle_put, priv);
+	if (ret)
+		return ret;
+	priv->regmap = tps6105x->regmap;
+	priv->cdev.brightness_set_blocking = tps6105x_brightness_set;
+	priv->cdev.max_brightness = 7;
+	init_data.devicename = "tps6105x";
+	init_data.default_label = ":torch";
+	init_data.fwnode = priv->fwnode;
+
+	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_ext(&pdev->dev, &priv->cdev,
+					      &init_data);
+}
+
+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 v5 2/2] dt-bindings: mfd: update TI tps6105x chip bindings

[Sven Van Asbroeck]

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

Tree: next-20191118
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 .../devicetree/bindings/mfd/tps6105x.txt      | 47 ++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/tps6105x.txt b/Documentation/devicetree/bindings/mfd/tps6105x.txt
index 93602c7a19c8..d15763740a3f 100644
--- a/Documentation/devicetree/bindings/mfd/tps6105x.txt
+++ b/Documentation/devicetree/bindings/mfd/tps6105x.txt
@@ -7,11 +7,56 @@ Required properties:
 - compatible:		"ti,tps61050" or "ti,tps61052"
 - reg:			Specifies the I2C slave address
 
-Example:
+Optional sub-node:
+
+This subnode selects the chip's operational mode.
+There can be at most one single available subnode.
+
+- regulator: presence of this sub-node puts the chip in regulator mode.
+	see Documentation/devicetree/bindings/regulator/regulator.txt
+
+- led: presence of this sub-node puts the chip in led mode.
+	Optional properties:
+	- function : see ../leds/common.txt
+	- color    : see ../leds/common.txt
+	- label    : see ../leds/common.txt
+			(deprecated)
+
+Example (GPIO operation only):
+
+i2c0 {
+	tps61052@33 {
+		compatible = "ti,tps61052";
+		reg = <0x33>;
+	};
+};
+
+Example (GPIO + regulator operation):
 
 i2c0 {
 	tps61052@33 {
 		compatible = "ti,tps61052";
 		reg = <0x33>;
+
+		regulator {
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			regulator-always-on;
+		};
+	};
+};
+
+Example (GPIO + led operation):
+
+#include <dt-bindings/leds/common.h>
+
+i2c0 {
+	tps61052@33 {
+		compatible = "ti,tps61052";
+		reg = <0x33>;
+
+		led {
+			color = <LED_COLOR_ID_WHITE>;
+		};
 	};
 };
-- 
2.17.1

Re: [PATCH v5 1/2] leds: tps6105x: add driver for mfd chip led mode

[Dan Murphy]

Sven

On 12/9/19 8:02 AM, Sven Van Asbroeck wrote:
> This driver adds support for the led operational mode of the
s/led/LED
> tps6105x mfd device.
s/mfd/MFD
>
> Tree: next-20191118
> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
> ---
>   drivers/leds/Kconfig         | 10 +++++
>   drivers/leds/Makefile        |  1 +
>   drivers/leds/leds-tps6105x.c | 83 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 94 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.
s/led/LED
> +	  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..ea2afaa3e3f0
> --- /dev/null
> +++ b/drivers/leds/leds-tps6105x.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +

Do you need a copyright?


> +#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;
> +	struct fwnode_handle *fwnode;
> +};
> +
> +static void tps6105x_handle_put(void *data)
> +{
> +	struct tps6105x_priv *priv = data;
> +
> +	fwnode_handle_put(priv->fwnode);
> +}
> +
> +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 led_init_data init_data = { };
> +	struct tps6105x_priv *priv;
> +	int ret;
> +
> +	/* This instance is not set for torch 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->fwnode = device_get_next_child_node(pdev->dev.parent, NULL);

Probably need to check for NULL on the return


> +	ret = devm_add_action_or_reset(&pdev->dev, tps6105x_handle_put, priv);
> +	if (ret)
> +		return ret;
> +	priv->regmap = tps6105x->regmap;
> +	priv->cdev.brightness_set_blocking = tps6105x_brightness_set;
> +	priv->cdev.max_brightness = 7;
> +	init_data.devicename = "tps6105x";
> +	init_data.default_label = ":torch";
> +	init_data.fwnode = priv->fwnode;
> +
> +	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);
Checkpatch should have warned about alignment here
> +	if (ret)
> +		return ret;
> +
> +	return devm_led_classdev_register_ext(&pdev->dev, &priv->cdev,
> +					      &init_data);
> +}
> +
> +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");

Re: [PATCH v5 1/2] leds: tps6105x: add driver for mfd chip led mode

[Sven Van Asbroeck]

Thank you for the review, Dan. Some remarks below.

On Mon, Dec 9, 2019 at 9:12 AM Dan Murphy <dmurphy@ti.com> wrote:
>
> > +     priv->fwnode = device_get_next_child_node(pdev->dev.parent, NULL);
>
> Probably need to check for NULL on the return
>

The driver will work without crashes or oopses even when this returns NULL:
- struct led_init_data . fwnode is optional (can be NULL)
- fwnode_handle_put() ignores NULL arguments

By not checking for NULL here, non-devicetree users can still select
led mode through platform data on the parent mfd driver, and things
will "just work".

Could I persuade you to keep this behaviour?
Perhaps I should put a comment in to clarify?

> > +     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);
> Checkpatch should have warned about alignment here

I used 5.4's checkpatch.pl, but somehow it doesn't warn :(
Will fix that up.

Re: [PATCH v5 2/2] dt-bindings: mfd: update TI tps6105x chip bindings

[Lee Jones]

On Mon, 09 Dec 2019, Sven Van Asbroeck wrote:

> The driver has been extended to optionally get its operational
> mode, regulator init data and LED naming from the devicetree.
> 
> Tree: next-20191118

Please refrain from putting this in the commit message.

> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
> ---

... if you really want to put that in, place it here, so it doesn't
become part of the kernel's Git history.

>  .../devicetree/bindings/mfd/tps6105x.txt      | 47 ++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/tps6105x.txt b/Documentation/devicetree/bindings/mfd/tps6105x.txt
> index 93602c7a19c8..d15763740a3f 100644
> --- a/Documentation/devicetree/bindings/mfd/tps6105x.txt
> +++ b/Documentation/devicetree/bindings/mfd/tps6105x.txt
> @@ -7,11 +7,56 @@ Required properties:
>  - compatible:		"ti,tps61050" or "ti,tps61052"
>  - reg:			Specifies the I2C slave address
>  
> -Example:
> +Optional sub-node:
> +
> +This subnode selects the chip's operational mode.
> +There can be at most one single available subnode.
> +
> +- regulator: presence of this sub-node puts the chip in regulator mode.
> +	see Documentation/devicetree/bindings/regulator/regulator.txt

Relative paths are preferred.

> +- led: presence of this sub-node puts the chip in led mode.
> +	Optional properties:
> +	- function : see ../leds/common.txt
> +	- color    : see ../leds/common.txt
> +	- label    : see ../leds/common.txt

Yes, like this.

> +			(deprecated)
> +
> +Example (GPIO operation only):
> +
> +i2c0 {
> +	tps61052@33 {
> +		compatible = "ti,tps61052";
> +		reg = <0x33>;
> +	};
> +};
> +
> +Example (GPIO + regulator operation):
>  
>  i2c0 {
>  	tps61052@33 {
>  		compatible = "ti,tps61052";
>  		reg = <0x33>;
> +
> +		regulator {
> +			regulator-min-microvolt = <5000000>;
> +			regulator-max-microvolt = <5000000>;
> +			regulator-always-on;
> +		};
> +	};
> +};
> +
> +Example (GPIO + led operation):
> +
> +#include <dt-bindings/leds/common.h>
> +
> +i2c0 {
> +	tps61052@33 {
> +		compatible = "ti,tps61052";
> +		reg = <0x33>;
> +
> +		led {
> +			color = <LED_COLOR_ID_WHITE>;
> +		};
>  	};
>  };

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog