linux-kernel - Re: [PATCH v3 2/3] leds: tps6105x: add driver for mfd chip led mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <07b85f68-3668-e354-3379-13d32e84466f@gmail.com>
Date:   Wed, 20 Nov 2019 22:33:15 +0100
From:   Jacek Anaszewski <jacek.anaszewski@...il.com>
To:     Sven Van Asbroeck <thesven73@...il.com>,
        Lee Jones <lee.jones@...aro.org>, Pavel Machek <pavel@....cz>,
        Rob Herring <robh+dt@...nel.org>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Grigoryev Denis <grigoryev@...twel.ru>,
        Axel Lin <axel.lin@...ics.com>, Dan Murphy <dmurphy@...com>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-leds@...r.kernel.org
Subject: Re: [PATCH v3 2/3] leds: tps6105x: add driver for mfd chip led mode

Hi Sven,

On 11/20/19 3:44 PM, Sven Van Asbroeck wrote:
> 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>;
> 
> 		led {
> 		};
> 	};
> };

This is covered in DT bindings, it is redundant in the commit message.

> 
> Tree: next-20191118
> Signed-off-by: Sven Van Asbroeck <TheSven73@...il.com>
> ---
>  drivers/leds/Kconfig         | 10 ++++++
>  drivers/leds/Makefile        |  1 +
>  drivers/leds/leds-tps6105x.c | 67 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 78 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..44325251b3d6
> --- /dev/null
> +++ b/drivers/leds/leds-tps6105x.c
> @@ -0,0 +1,67 @@
> +// 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 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->regmap = tps6105x->regmap;
> +	priv->cdev.name = "tps6105x::torch";

Please remove above line.

> +	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;
>

And use new LED registration API like below:

struct led_init_data init_data = {
	.devicename = "tps6105x",
	.default_label = ":torch" };

return devm_led_classdev_register_ext(&pdev->dev, &priv->cdev, &init_data);

This way you will make the driver capable of using LED core mechanism
for composing LED names, which will allow to use function and color
properties in DT, and automatically override the defaults from the
driver. If both properties are missing in DT, then you will get the LED
named tps6105x::torch.

> +	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@...il.com>");
> +MODULE_LICENSE("GPL v2");
> 

-- 
Best regards,
Jacek Anaszewski