| Message ID | 20230310095635.813262-4-hpa@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | leds: tps68470: LED driver for TPS68470 | expand |
Hi, On Fri, Mar 10, 2023 at 6:43 PM Dan Scally <dan.scally@ideasonboard.com> wrote: > > Hi Kate - thanks for the v2 > > On 10/03/2023 09:56, Kate Hsuan wrote: > > There are two LED controllers, LEDA indicator LED and LEDB flash LED for > > tps68470. LEDA can be enabled by setting TPS68470_ILEDCTL_ENA. Moreover, > > tps68470 provides four levels of power status for LEDB. If the > > properties called "ti,ledb-current" can be found, the current will be > > set according to the property values. These two LEDs can be controlled > > through the LED class of sysfs (tps68470-leda and tps68470-ledb). > > > > Signed-off-by: Kate Hsuan <hpa@redhat.com> > > --- > > drivers/leds/Kconfig | 12 +++ > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-tps68470.c | 182 +++++++++++++++++++++++++++++++++++ > > 3 files changed, 195 insertions(+) > > create mode 100644 drivers/leds/leds-tps68470.c > > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > > index 9dbce09eabac..fd26036b3c61 100644 > > --- a/drivers/leds/Kconfig > > +++ b/drivers/leds/Kconfig > > @@ -827,6 +827,18 @@ config LEDS_TPS6105X > > It is a single boost converter primarily for white LEDs and > > audio amplifiers. > > > > +config LEDS_TPS68470 > > + tristate "LED support for TI TPS68470" > > + depends on LEDS_CLASS > > + depends on INTEL_SKL_INT3472 > > + help > > + This driver supports TPS68470 PMIC with LED chip. > > + It provide two LED controllers, including an indicator LED > > + and a flash LED. > > s/provide/provides. Also maybe "It provides two LED controllers, with the ability to drive 2 I'll revise the description in the v3 patch. > indicator LEDs and 2 flash LEDs". I actually got the WLED part working now finally so I'll send > patches on top of this series if that's ok? Sounds good! That would be great! Thank you > > > + > > + To compile this driver as a module, choose M and it will be > > + called leds-tps68470 > > + > > config LEDS_IP30 > > tristate "LED support for SGI Octane machines" > > depends on LEDS_CLASS > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > index d30395d11fd8..b284bc0daa98 100644 > > --- a/drivers/leds/Makefile > > +++ b/drivers/leds/Makefile > > @@ -84,6 +84,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA) += leds-turris-omnia.o > > obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o > > obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o > > obj-$(CONFIG_LEDS_WRAP) += leds-wrap.o > > +obj-$(CONFIG_LEDS_TPS68470) += leds-tps68470.o > > > > # LED SPI Drivers > > obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o > > diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c > > new file mode 100644 > > index 000000000000..98bb56153690 > > --- /dev/null > > +++ b/drivers/leds/leds-tps68470.c > > @@ -0,0 +1,182 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * LED driver for TPS68470 PMIC > > + * > > + * Copyright (C) 2023 Red Hat > > + * > > + * Authors: > > + * Kate Hsuan <hpa@redhat.com> > > + */ > > + > > +#include <linux/gpio/driver.h> > > Not needed I think? removed > > > +#include <linux/mfd/tps68470.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/leds.h> > > Alphabetical order? Okay. > > + > > +#define lcdev_to_led(led_cdev) \ > > + container_of(led_cdev, struct tps68470_led, lcdev) > > + > > +#define led_to_tps68470(led, index) \ > > + container_of(led, struct tps68470_device, leds[index]) > > + > > +enum tps68470_led_ids { > > + TPS68470_ILED_A, > > + TPS68470_ILED_B, > > + TPS68470_NUM_LEDS > > +}; > > + > > +static const char *tps68470_led_names[] = { > > + [TPS68470_ILED_A] = "tps68470-iled_a", > > + [TPS68470_ILED_B] = "tps68470-iled_b", > > +}; > > + > > +struct tps68470_led { > > + unsigned int led_id; > > + struct led_classdev lcdev; > > +}; > > + > > +struct tps68470_device { > > + struct device *dev; > > + struct regmap *regmap; > > + struct tps68470_led leds[TPS68470_NUM_LEDS]; > > +}; > > + > > +enum ctrlb_current { > > + CTRLB_2MA = 0, > > + CTRLB_4MA = 1, > > + CTRLB_8MA = 2, > > + CTRLB_16MA = 3, > > +}; > > + > > +static int tps68470_brightness_set(struct led_classdev *led_cdev, enum led_brightness brightness) > > +{ > > + struct tps68470_led *led = lcdev_to_led(led_cdev); > > + struct tps68470_device *tps68470 = led_to_tps68470(led, led->led_id); > > + struct regmap *regmap = tps68470->regmap > This would work fine as is...but I would maybe add something like > > if (state > LED_ON) > return -EINVAL; > > So that brightness values of > 1 aren't just silently accepted...or does the LED core already > prevent that? If so it's fine. I think it is unnecessary. The LED framework already handles this. Since we already set "max_brightness" for the device, the framework will check the "brightness" value and make sure the value isn't greater than "max_brightness". > > > + > > + switch (led->led_id) { > > + case TPS68470_ILED_A: > > + return regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, TPS68470_ILEDCTL_ENA, > > + brightness ? TPS68470_ILEDCTL_ENA : 0); > > + case TPS68470_ILED_B: > > + return regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, TPS68470_ILEDCTL_ENB, > > + brightness ? TPS68470_ILEDCTL_ENB : 0); > > + } > > + return -EINVAL; > > +} > > + > > +static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev) > > +{ > > + struct tps68470_led *led = lcdev_to_led(led_cdev); > > + struct tps68470_device *tps68470 = led_to_tps68470(led, led->led_id); > > + struct regmap *regmap = tps68470->regmap; > > + int ret = 0; > > + int value = 0; > > + > > + ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value); > > + if (ret) > > + goto error; > > Just return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n") here imo. Okay. > > + > > + switch (led->led_id) { > > + case TPS68470_ILED_A: > > + value = value & TPS68470_ILEDCTL_ENA; > > + break; > > + case TPS68470_ILED_B: > > + value = value & TPS68470_ILEDCTL_ENB; > > + break; > > + } > > + > > + return value ? LED_ON : LED_OFF; > > + > > +error: > > + dev_err(led_cdev->dev, "Failed on reading register\n"); > > + return -EINVAL; > > +} > > + > > +static int tps68470_leds_probe(struct platform_device *pdev) > > +{ > > + int i = 0; > > + int ret = 0; > > + unsigned int curr; > > + struct tps68470_device *tps68470; > > + struct tps68470_led *led; > > + struct led_classdev *lcdev; > > + > > + tps68470 = devm_kzalloc(&pdev->dev, sizeof(struct tps68470_device), > > + GFP_KERNEL); > > No -ENOMEM check here? > I'll add a return value check here in the v3 patch. > > + tps68470->dev = &pdev->dev; > > + tps68470->regmap = dev_get_drvdata(pdev->dev.parent); > > + > > + for (i = 0; i < TPS68470_NUM_LEDS; i++) { > > + led = &tps68470->leds[i]; > > + lcdev = &led->lcdev; > > + > > + led->led_id = i; > > + > > + lcdev->name = devm_kasprintf(tps68470->dev, GFP_KERNEL, "%s::%s", > > + tps68470_led_names[i], LED_FUNCTION_INDICATOR); > > + if (!lcdev->name) > > + return -ENOMEM; > > + > > + lcdev->max_brightness = 1; > > + lcdev->brightness = 0; > > + lcdev->brightness_set_blocking = tps68470_brightness_set; > > + lcdev->brightness_get = tps68470_brightness_get; > > + lcdev->dev = &pdev->dev; > > + > > + ret = devm_led_classdev_register(tps68470->dev, lcdev); > > + if (ret) { > > + dev_err_probe(tps68470->dev, ret, > > + "error registering led\n"); > > + goto err_exit; > > + } > > + } > > + > > + /* configure LEDB current if the properties can be got */ > > + if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) { > > + switch (curr) { > > + case 2: > > + curr = CTRLB_2MA; > > + break; > > + case 4: > > + curr = CTRLB_4MA; > > + break; > > + case 8: > > + curr = CTRLB_8MA; > > + break; > > + case 16: > > + curr = CTRLB_16MA; > > + break; > > + default: > > + dev_err(&pdev->dev, "Invalid LEDB curr value: %d\n", curr); > > + return -EINVAL; > > There's no jump to err_exit here...I think that this whole section should go above the registration > of the LEDS...and probably also into its own function. Okay. > > > + } > > + ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_ILEDCTL, > > + TPS68470_ILEDCTL_CTRLB, curr); > > + } > > + > > +err_exit: > > + if (ret) { > > + for (i = 0; i < TPS68470_NUM_LEDS; i++) { > > + if (tps68470->leds[i].lcdev.name) > > + devm_led_classdev_unregister(&pdev->dev, > > + &tps68470->leds[i].lcdev); > > + } > > + } > > + > > + return ret; > > +} > > +static struct platform_driver tps68470_led_driver = { > > + .driver = { > > + .name = "tps68470-led", > > + }, > > + .probe = tps68470_leds_probe, > > +}; > > + > > +module_platform_driver(tps68470_led_driver); > > + > > +MODULE_ALIAS("platform:tps68470-led"); > > +MODULE_DESCRIPTION("LED driver for TPS68470 PMIC"); > > +MODULE_LICENSE("GPL v2"); > Thank you
Hi Kate - sorry just noticed one more thing... On 10/03/2023 09:56, Kate Hsuan wrote: > There are two LED controllers, LEDA indicator LED and LEDB flash LED for > tps68470. LEDA can be enabled by setting TPS68470_ILEDCTL_ENA. Moreover, > tps68470 provides four levels of power status for LEDB. If the > properties called "ti,ledb-current" can be found, the current will be > set according to the property values. These two LEDs can be controlled > through the LED class of sysfs (tps68470-leda and tps68470-ledb). > > Signed-off-by: Kate Hsuan <hpa@redhat.com> > --- > drivers/leds/Kconfig | 12 +++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-tps68470.c | 182 +++++++++++++++++++++++++++++++++++ > 3 files changed, 195 insertions(+) > create mode 100644 drivers/leds/leds-tps68470.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 9dbce09eabac..fd26036b3c61 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -827,6 +827,18 @@ config LEDS_TPS6105X > It is a single boost converter primarily for white LEDs and > audio amplifiers. > > +config LEDS_TPS68470 > + tristate "LED support for TI TPS68470" > + depends on LEDS_CLASS > + depends on INTEL_SKL_INT3472 > + help > + This driver supports TPS68470 PMIC with LED chip. > + It provide two LED controllers, including an indicator LED > + and a flash LED. > + > + To compile this driver as a module, choose M and it will be > + called leds-tps68470 > + > config LEDS_IP30 > tristate "LED support for SGI Octane machines" > depends on LEDS_CLASS > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index d30395d11fd8..b284bc0daa98 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -84,6 +84,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA) += leds-turris-omnia.o > obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o > obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o > obj-$(CONFIG_LEDS_WRAP) += leds-wrap.o > +obj-$(CONFIG_LEDS_TPS68470) += leds-tps68470.o Alphabetical order here too please :)
On Mon, Mar 20, 2023 at 10:24 PM Dan Scally <dan.scally@ideasonboard.com> wrote: > > Hi Kate - sorry just noticed one more thing... > > On 10/03/2023 09:56, Kate Hsuan wrote: > > There are two LED controllers, LEDA indicator LED and LEDB flash LED for > > tps68470. LEDA can be enabled by setting TPS68470_ILEDCTL_ENA. Moreover, > > tps68470 provides four levels of power status for LEDB. If the > > properties called "ti,ledb-current" can be found, the current will be > > set according to the property values. These two LEDs can be controlled > > through the LED class of sysfs (tps68470-leda and tps68470-ledb). > > > > Signed-off-by: Kate Hsuan <hpa@redhat.com> > > --- > > drivers/leds/Kconfig | 12 +++ > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-tps68470.c | 182 +++++++++++++++++++++++++++++++++++ > > 3 files changed, 195 insertions(+) > > create mode 100644 drivers/leds/leds-tps68470.c > > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > > index 9dbce09eabac..fd26036b3c61 100644 > > --- a/drivers/leds/Kconfig > > +++ b/drivers/leds/Kconfig > > @@ -827,6 +827,18 @@ config LEDS_TPS6105X > > It is a single boost converter primarily for white LEDs and > > audio amplifiers. > > > > +config LEDS_TPS68470 > > + tristate "LED support for TI TPS68470" > > + depends on LEDS_CLASS > > + depends on INTEL_SKL_INT3472 > > + help > > + This driver supports TPS68470 PMIC with LED chip. > > + It provide two LED controllers, including an indicator LED > > + and a flash LED. > > + > > + To compile this driver as a module, choose M and it will be > > + called leds-tps68470 > > + > > config LEDS_IP30 > > tristate "LED support for SGI Octane machines" > > depends on LEDS_CLASS > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > index d30395d11fd8..b284bc0daa98 100644 > > --- a/drivers/leds/Makefile > > +++ b/drivers/leds/Makefile > > @@ -84,6 +84,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA) += leds-turris-omnia.o > > obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o > > obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o > > obj-$(CONFIG_LEDS_WRAP) += leds-wrap.o > > +obj-$(CONFIG_LEDS_TPS68470) += leds-tps68470.o > > > Alphabetical order here too please :) > Thank you. I updated this. :)
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 9dbce09eabac..fd26036b3c61 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -827,6 +827,18 @@ config LEDS_TPS6105X It is a single boost converter primarily for white LEDs and audio amplifiers. +config LEDS_TPS68470 + tristate "LED support for TI TPS68470" + depends on LEDS_CLASS + depends on INTEL_SKL_INT3472 + help + This driver supports TPS68470 PMIC with LED chip. + It provide two LED controllers, including an indicator LED + and a flash LED. + + To compile this driver as a module, choose M and it will be + called leds-tps68470 + config LEDS_IP30 tristate "LED support for SGI Octane machines" depends on LEDS_CLASS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index d30395d11fd8..b284bc0daa98 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -84,6 +84,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA) += leds-turris-omnia.o obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o obj-$(CONFIG_LEDS_WRAP) += leds-wrap.o +obj-$(CONFIG_LEDS_TPS68470) += leds-tps68470.o # LED SPI Drivers obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c new file mode 100644 index 000000000000..98bb56153690 --- /dev/null +++ b/drivers/leds/leds-tps68470.c @@ -0,0 +1,182 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * LED driver for TPS68470 PMIC + * + * Copyright (C) 2023 Red Hat + * + * Authors: + * Kate Hsuan <hpa@redhat.com> + */ + +#include <linux/gpio/driver.h> +#include <linux/mfd/tps68470.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/leds.h> + +#define lcdev_to_led(led_cdev) \ + container_of(led_cdev, struct tps68470_led, lcdev) + +#define led_to_tps68470(led, index) \ + container_of(led, struct tps68470_device, leds[index]) + +enum tps68470_led_ids { + TPS68470_ILED_A, + TPS68470_ILED_B, + TPS68470_NUM_LEDS +}; + +static const char *tps68470_led_names[] = { + [TPS68470_ILED_A] = "tps68470-iled_a", + [TPS68470_ILED_B] = "tps68470-iled_b", +}; + +struct tps68470_led { + unsigned int led_id; + struct led_classdev lcdev; +}; + +struct tps68470_device { + struct device *dev; + struct regmap *regmap; + struct tps68470_led leds[TPS68470_NUM_LEDS]; +}; + +enum ctrlb_current { + CTRLB_2MA = 0, + CTRLB_4MA = 1, + CTRLB_8MA = 2, + CTRLB_16MA = 3, +}; + +static int tps68470_brightness_set(struct led_classdev *led_cdev, enum led_brightness brightness) +{ + struct tps68470_led *led = lcdev_to_led(led_cdev); + struct tps68470_device *tps68470 = led_to_tps68470(led, led->led_id); + struct regmap *regmap = tps68470->regmap; + + switch (led->led_id) { + case TPS68470_ILED_A: + return regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, TPS68470_ILEDCTL_ENA, + brightness ? TPS68470_ILEDCTL_ENA : 0); + case TPS68470_ILED_B: + return regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, TPS68470_ILEDCTL_ENB, + brightness ? TPS68470_ILEDCTL_ENB : 0); + } + return -EINVAL; +} + +static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev) +{ + struct tps68470_led *led = lcdev_to_led(led_cdev); + struct tps68470_device *tps68470 = led_to_tps68470(led, led->led_id); + struct regmap *regmap = tps68470->regmap; + int ret = 0; + int value = 0; + + ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value); + if (ret) + goto error; + + switch (led->led_id) { + case TPS68470_ILED_A: + value = value & TPS68470_ILEDCTL_ENA; + break; + case TPS68470_ILED_B: + value = value & TPS68470_ILEDCTL_ENB; + break; + } + + return value ? LED_ON : LED_OFF; + +error: + dev_err(led_cdev->dev, "Failed on reading register\n"); + return -EINVAL; +} + +static int tps68470_leds_probe(struct platform_device *pdev) +{ + int i = 0; + int ret = 0; + unsigned int curr; + struct tps68470_device *tps68470; + struct tps68470_led *led; + struct led_classdev *lcdev; + + tps68470 = devm_kzalloc(&pdev->dev, sizeof(struct tps68470_device), + GFP_KERNEL); + tps68470->dev = &pdev->dev; + tps68470->regmap = dev_get_drvdata(pdev->dev.parent); + + for (i = 0; i < TPS68470_NUM_LEDS; i++) { + led = &tps68470->leds[i]; + lcdev = &led->lcdev; + + led->led_id = i; + + lcdev->name = devm_kasprintf(tps68470->dev, GFP_KERNEL, "%s::%s", + tps68470_led_names[i], LED_FUNCTION_INDICATOR); + if (!lcdev->name) + return -ENOMEM; + + lcdev->max_brightness = 1; + lcdev->brightness = 0; + lcdev->brightness_set_blocking = tps68470_brightness_set; + lcdev->brightness_get = tps68470_brightness_get; + lcdev->dev = &pdev->dev; + + ret = devm_led_classdev_register(tps68470->dev, lcdev); + if (ret) { + dev_err_probe(tps68470->dev, ret, + "error registering led\n"); + goto err_exit; + } + } + + /* configure LEDB current if the properties can be got */ + if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) { + switch (curr) { + case 2: + curr = CTRLB_2MA; + break; + case 4: + curr = CTRLB_4MA; + break; + case 8: + curr = CTRLB_8MA; + break; + case 16: + curr = CTRLB_16MA; + break; + default: + dev_err(&pdev->dev, "Invalid LEDB curr value: %d\n", curr); + return -EINVAL; + } + ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_ILEDCTL, + TPS68470_ILEDCTL_CTRLB, curr); + } + +err_exit: + if (ret) { + for (i = 0; i < TPS68470_NUM_LEDS; i++) { + if (tps68470->leds[i].lcdev.name) + devm_led_classdev_unregister(&pdev->dev, + &tps68470->leds[i].lcdev); + } + } + + return ret; +} +static struct platform_driver tps68470_led_driver = { + .driver = { + .name = "tps68470-led", + }, + .probe = tps68470_leds_probe, +}; + +module_platform_driver(tps68470_led_driver); + +MODULE_ALIAS("platform:tps68470-led"); +MODULE_DESCRIPTION("LED driver for TPS68470 PMIC"); +MODULE_LICENSE("GPL v2");
There are two LED controllers, LEDA indicator LED and LEDB flash LED for tps68470. LEDA can be enabled by setting TPS68470_ILEDCTL_ENA. Moreover, tps68470 provides four levels of power status for LEDB. If the properties called "ti,ledb-current" can be found, the current will be set according to the property values. These two LEDs can be controlled through the LED class of sysfs (tps68470-leda and tps68470-ledb). Signed-off-by: Kate Hsuan <hpa@redhat.com> --- drivers/leds/Kconfig | 12 +++ drivers/leds/Makefile | 1 + drivers/leds/leds-tps68470.c | 182 +++++++++++++++++++++++++++++++++++ 3 files changed, 195 insertions(+) create mode 100644 drivers/leds/leds-tps68470.c