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

[Dan Murphy <dmurphy@ti.com>]

Sven

On 11/19/19 9:46 AM, Sven Van Asbroeck wrote:
> This driver adds support for the led operational mode of the
> tps6105x mfd device.
How many LEDs does this device support?
>
> Example usage, devicetree:
>
> i2c0 {
> 	tps61052@33 {
> 		compatible = "ti,tps61052";
> 		reg = <0x33>;
>
> 		led {
> 			label = "tps-led";
> 		};
> 	};
> };
>
> Example usage, platform data in machine layer:
>
>   #include <linux/mfd/tps6105x.h>
>
>   struct tps6105x_platform_data pdata = {
>           .mode = TPS6105X_MODE_TORCH,
>           .led_label = "tps-led",
>   };
>
> 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 | 87 ++++++++++++++++++++++++++++++++++++
>   include/linux/mfd/tps6105x.h |  1 +
>   4 files changed, 99 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..87dbe4846df6
> --- /dev/null
> +++ b/drivers/leds/leds-tps6105x.c
> @@ -0,0 +1,87 @@
> +// 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 const char *label_from_dt(struct device *dev)
> +{
> +	struct device_node *led =
> +		of_get_child_by_name(dev->parent->of_node, "led");
Prefer device_* calls as opposed to of_* calls.
> +	const char *label;
> +
> +	if (!led) {
> +		dev_err(dev, "led node not found");
> +		return NULL;
> +	}
> +	if (of_property_read_string(led, "label", &label))
same as above
> +		label = NULL;
> +	of_node_put(led);
> +
> +	return label;
> +}
> +
> +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;
> +	const char *label;
> +	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;
> +	}
> +
> +	label = pdata->led_label ?: label_from_dt(&pdev->dev);

Since this is a new driver do we really have to continue to use the 
pdata for the init

data?  Can't we just get the label from the DT node now like other drivers?

Dan