| Message ID | 20230321153718.1355511-4-hpa@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | leds: tps68470: LED driver for TPS68470 | expand |
Hi, On 3/21/23 16:37, 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> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/leds/Kconfig | 12 +++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-tps68470.c | 185 +++++++++++++++++++++++++++++++++++ > 3 files changed, 198 insertions(+) > create mode 100644 drivers/leds/leds-tps68470.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 9dbce09eabac..ab9073b2cfef 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 provides two LED controllers, with the ability to drive 2 > + indicator LEDs and 2 flash LEDs. > + > + 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..515a69953e73 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -80,6 +80,7 @@ obj-$(CONFIG_LEDS_TCA6507) += leds-tca6507.o > obj-$(CONFIG_LEDS_TI_LMU_COMMON) += leds-ti-lmu-common.o > obj-$(CONFIG_LEDS_TLC591XX) += leds-tlc591xx.o > obj-$(CONFIG_LEDS_TPS6105X) += leds-tps6105x.o > +obj-$(CONFIG_LEDS_TPS68470) += leds-tps68470.o > 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 > diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c > new file mode 100644 > index 000000000000..35aeb5db89c8 > --- /dev/null > +++ b/drivers/leds/leds-tps68470.c > @@ -0,0 +1,185 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * LED driver for TPS68470 PMIC > + * > + * Copyright (C) 2023 Red Hat > + * > + * Authors: > + * Kate Hsuan <hpa@redhat.com> > + */ > + > +#include <linux/leds.h> > +#include <linux/mfd/tps68470.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/property.h> > +#include <linux/regmap.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) > + return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n"); > + > + 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; > +} > + > + > +static int tps68470_ledb_current_init(struct platform_device *pdev, > + struct tps68470_device *tps68470) > +{ > + int ret = 0; > + unsigned int curr; > + > + /* configure LEDB current if the properties can be got */ > + if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) { > + if (curr > CTRLB_16MA) { > + dev_err(&pdev->dev, > + "Invalid LEDB current value: %d\n", > + curr); > + return -EINVAL; > + } > + ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_ILEDCTL, > + TPS68470_ILEDCTL_CTRLB, curr); > + } > + return ret; > +} > + > +static int tps68470_leds_probe(struct platform_device *pdev) > +{ > + int i = 0; > + int ret = 0; > + struct tps68470_device *tps68470; > + struct tps68470_led *led; > + struct led_classdev *lcdev; > + > + tps68470 = devm_kzalloc(&pdev->dev, sizeof(struct tps68470_device), > + GFP_KERNEL); > + if (!tps68470) > + return -ENOMEM; > + > + 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; > + } > + > + if (i == TPS68470_ILED_B) { > + ret = tps68470_ledb_current_init(pdev, tps68470); > + if (ret) > + goto err_exit; > + } > + } > + > +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");
Hi Kate On 21/03/2023 15:37, 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> > --- This looks good to me now: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > drivers/leds/Kconfig | 12 +++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-tps68470.c | 185 +++++++++++++++++++++++++++++++++++ > 3 files changed, 198 insertions(+) > create mode 100644 drivers/leds/leds-tps68470.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 9dbce09eabac..ab9073b2cfef 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 provides two LED controllers, with the ability to drive 2 > + indicator LEDs and 2 flash LEDs. > + > + 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..515a69953e73 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -80,6 +80,7 @@ obj-$(CONFIG_LEDS_TCA6507) += leds-tca6507.o > obj-$(CONFIG_LEDS_TI_LMU_COMMON) += leds-ti-lmu-common.o > obj-$(CONFIG_LEDS_TLC591XX) += leds-tlc591xx.o > obj-$(CONFIG_LEDS_TPS6105X) += leds-tps6105x.o > +obj-$(CONFIG_LEDS_TPS68470) += leds-tps68470.o > 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 > diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c > new file mode 100644 > index 000000000000..35aeb5db89c8 > --- /dev/null > +++ b/drivers/leds/leds-tps68470.c > @@ -0,0 +1,185 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * LED driver for TPS68470 PMIC > + * > + * Copyright (C) 2023 Red Hat > + * > + * Authors: > + * Kate Hsuan <hpa@redhat.com> > + */ > + > +#include <linux/leds.h> > +#include <linux/mfd/tps68470.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/property.h> > +#include <linux/regmap.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) > + return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n"); > + > + 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; > +} > + > + > +static int tps68470_ledb_current_init(struct platform_device *pdev, > + struct tps68470_device *tps68470) > +{ > + int ret = 0; > + unsigned int curr; > + > + /* configure LEDB current if the properties can be got */ > + if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) { > + if (curr > CTRLB_16MA) { > + dev_err(&pdev->dev, > + "Invalid LEDB current value: %d\n", > + curr); > + return -EINVAL; > + } > + ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_ILEDCTL, > + TPS68470_ILEDCTL_CTRLB, curr); > + } > + return ret; > +} > + > +static int tps68470_leds_probe(struct platform_device *pdev) > +{ > + int i = 0; > + int ret = 0; > + struct tps68470_device *tps68470; > + struct tps68470_led *led; > + struct led_classdev *lcdev; > + > + tps68470 = devm_kzalloc(&pdev->dev, sizeof(struct tps68470_device), > + GFP_KERNEL); > + if (!tps68470) > + return -ENOMEM; > + > + 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; > + } > + > + if (i == TPS68470_ILED_B) { > + ret = tps68470_ledb_current_init(pdev, tps68470); > + if (ret) > + goto err_exit; > + } > + } > + > +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");
Hi! > 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). If the LED can have four different currents, should it have 4 brightness levels? > +++ 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 provides two LED controllers, with the ability to drive 2 > + indicator LEDs and 2 flash LEDs. > + > + To compile this driver as a module, choose M and it will be > + called leds-tps68470 . at end of line. > +static const char *tps68470_led_names[] = { > + [TPS68470_ILED_A] = "tps68470-iled_a", > + [TPS68470_ILED_B] = "tps68470-iled_b", No. See Documentation/leds/well-known-leds.txt . We want the LEDs to be named after their function. > +static int tps68470_ledb_current_init(struct platform_device *pdev, > + struct tps68470_device *tps68470) > +{ > + int ret = 0; > + unsigned int curr; > + > + /* configure LEDB current if the properties can be got */ english? > + if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) { > + if (curr > CTRLB_16MA) { We'll need device tree binding documentation, right? Best regards, Pavel
Hi Pavel, On 3/23/23 12:15, Pavel Machek wrote: > Hi! > >> 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). > > If the LED can have four different currents, should it have 4 > brightness levels? No this was already discussed with an earlier version. This is in indicator LED output. The current setting is a one time boot configure thing after which the indicator LED is either on or off. > >> +++ 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 provides two LED controllers, with the ability to drive 2 >> + indicator LEDs and 2 flash LEDs. >> + >> + To compile this driver as a module, choose M and it will be >> + called leds-tps68470 > > . at end of line. > >> +static const char *tps68470_led_names[] = { >> + [TPS68470_ILED_A] = "tps68470-iled_a", >> + [TPS68470_ILED_B] = "tps68470-iled_b", > > No. See Documentation/leds/well-known-leds.txt . We want the LEDs to > be named after their function. > >> +static int tps68470_ledb_current_init(struct platform_device *pdev, >> + struct tps68470_device *tps68470) >> +{ >> + int ret = 0; >> + unsigned int curr; >> + >> + /* configure LEDB current if the properties can be got */ > > english? > >> + if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) { >> + if (curr > CTRLB_16MA) { > > We'll need device tree binding documentation, right? For now this PMIC is only used for the camera in some x86/acpi designs, so no we don't need dt binding documentation (the dt binding maintainers have explicitly asked to not add dt binding documentation for things not actually used with dt). Or I guess we should simply add this to the platform_data which one of Daniel's later patches introduces and drop the initializing of the LEDB current setting from the initial driver addition. I think that that (moving this to the later added platform_data) would be best. Since now after Daniel's patches we have a mix of platform_data + the 1 device-property for this. Daniel, what do you think about moving the LEDB current setting to your "[PATCH 2/8] leds: tps68470: Init LED registers using platform data" patch ? Regards, Hans
On Thu 2023-03-23 12:24:05, Hans de Goede wrote: > Hi Pavel, > > On 3/23/23 12:15, Pavel Machek wrote: > > Hi! > > > >> 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). > > > > If the LED can have four different currents, should it have 4 > > brightness levels? > > No this was already discussed with an earlier version. This is in > indicator LED output. The current setting is a one time boot configure > thing after which the indicator LED is either on or off. Current levels are exponential in that driver. That will result in rather nice four level. Surely LED does not care if you set it during boot or later? BR, Pavel
Hi, On 3/23/23 12:26, Pavel Machek wrote: > On Thu 2023-03-23 12:24:05, Hans de Goede wrote: >> Hi Pavel, >> >> On 3/23/23 12:15, Pavel Machek wrote: >>> Hi! >>> >>>> 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). >>> >>> If the LED can have four different currents, should it have 4 >>> brightness levels? >> >> No this was already discussed with an earlier version. This is in >> indicator LED output. The current setting is a one time boot configure >> thing after which the indicator LED is either on or off. > > Current levels are exponential in that driver. That will result in > rather nice four level. Surely LED does not care if you set it during > boot or later? Well for one there is no guarantee the LED can continuously handle the maximum configurable LED current and as you rightly point out elsewhere in the thread we don't want to be blowing up hw. Regards, Hans
Hi, On 3/23/23 12:29, Hans de Goede wrote: > Hi, > > On 3/23/23 12:26, Pavel Machek wrote: >> On Thu 2023-03-23 12:24:05, Hans de Goede wrote: >>> Hi Pavel, >>> >>> On 3/23/23 12:15, Pavel Machek wrote: >>>> Hi! >>>> >>>>> 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). >>>> >>>> If the LED can have four different currents, should it have 4 >>>> brightness levels? >>> >>> No this was already discussed with an earlier version. This is in >>> indicator LED output. The current setting is a one time boot configure >>> thing after which the indicator LED is either on or off. >> >> Current levels are exponential in that driver. That will result in >> rather nice four level. Surely LED does not care if you set it during >> boot or later? > > Well for one there is no guarantee the LED can continuously handle > the maximum configurable LED current and as you rightly point out > elsewhere in the thread we don't want to be blowing up hw. Also this LED output as mentioned is especially intended for use with on/off triggers and those don't use/set different brightness levels. So it really is best to set the LED current once to the current which we want when the LED is on to indicate whatever it is intended to be indicating. Regards, Hans
On Thu 2023-03-23 12:29:29, Hans de Goede wrote: > Hi, > > On 3/23/23 12:26, Pavel Machek wrote: > > On Thu 2023-03-23 12:24:05, Hans de Goede wrote: > >> Hi Pavel, > >> > >> On 3/23/23 12:15, Pavel Machek wrote: > >>> Hi! > >>> > >>>> 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). > >>> > >>> If the LED can have four different currents, should it have 4 > >>> brightness levels? > >> > >> No this was already discussed with an earlier version. This is in > >> indicator LED output. The current setting is a one time boot configure > >> thing after which the indicator LED is either on or off. > > > > Current levels are exponential in that driver. That will result in > > rather nice four level. Surely LED does not care if you set it during > > boot or later? > > Well for one there is no guarantee the LED can continuously handle > the maximum configurable LED current and as you rightly point out > elsewhere in the thread we don't want to be blowing up hw. hw can support 16mA -> you expose 0, 2mA, 4mA, 8mA, 16mA levels. hw can support 4mA -> you expose 0, 2mA, 4mA. Triggers will work with these. Best regards, Pavel
Hi, On 3/23/23 12:36, Pavel Machek wrote: > On Thu 2023-03-23 12:29:29, Hans de Goede wrote: >> Hi, >> >> On 3/23/23 12:26, Pavel Machek wrote: >>> On Thu 2023-03-23 12:24:05, Hans de Goede wrote: >>>> Hi Pavel, >>>> >>>> On 3/23/23 12:15, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>> 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). >>>>> >>>>> If the LED can have four different currents, should it have 4 >>>>> brightness levels? >>>> >>>> No this was already discussed with an earlier version. This is in >>>> indicator LED output. The current setting is a one time boot configure >>>> thing after which the indicator LED is either on or off. >>> >>> Current levels are exponential in that driver. That will result in >>> rather nice four level. Surely LED does not care if you set it during >>> boot or later? >> >> Well for one there is no guarantee the LED can continuously handle >> the maximum configurable LED current and as you rightly point out >> elsewhere in the thread we don't want to be blowing up hw. > > hw can support 16mA -> you expose 0, 2mA, 4mA, 8mA, 16mA levels. > > hw can support 4mA -> you expose 0, 2mA, 4mA. This is just not how this hw is intended to be used. If you look at the registers there are on/off bits for LEDA and LEDB in a single register and then there is a current-limit only for LEDB, where as LEDA has a fixed current. So clearly the intention here is on/off use with a fixed current and dimming something like a privacy-LED on a laptop camera makes no sense and will never be used since we disable userspace access of the LED when it is used as a privacy-LED. Regards, Hans
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 9dbce09eabac..ab9073b2cfef 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 provides two LED controllers, with the ability to drive 2 + indicator LEDs and 2 flash LEDs. + + 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..515a69953e73 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -80,6 +80,7 @@ obj-$(CONFIG_LEDS_TCA6507) += leds-tca6507.o obj-$(CONFIG_LEDS_TI_LMU_COMMON) += leds-ti-lmu-common.o obj-$(CONFIG_LEDS_TLC591XX) += leds-tlc591xx.o obj-$(CONFIG_LEDS_TPS6105X) += leds-tps6105x.o +obj-$(CONFIG_LEDS_TPS68470) += leds-tps68470.o 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 diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c new file mode 100644 index 000000000000..35aeb5db89c8 --- /dev/null +++ b/drivers/leds/leds-tps68470.c @@ -0,0 +1,185 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * LED driver for TPS68470 PMIC + * + * Copyright (C) 2023 Red Hat + * + * Authors: + * Kate Hsuan <hpa@redhat.com> + */ + +#include <linux/leds.h> +#include <linux/mfd/tps68470.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/property.h> +#include <linux/regmap.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) + return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n"); + + 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; +} + + +static int tps68470_ledb_current_init(struct platform_device *pdev, + struct tps68470_device *tps68470) +{ + int ret = 0; + unsigned int curr; + + /* configure LEDB current if the properties can be got */ + if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) { + if (curr > CTRLB_16MA) { + dev_err(&pdev->dev, + "Invalid LEDB current value: %d\n", + curr); + return -EINVAL; + } + ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_ILEDCTL, + TPS68470_ILEDCTL_CTRLB, curr); + } + return ret; +} + +static int tps68470_leds_probe(struct platform_device *pdev) +{ + int i = 0; + int ret = 0; + struct tps68470_device *tps68470; + struct tps68470_led *led; + struct led_classdev *lcdev; + + tps68470 = devm_kzalloc(&pdev->dev, sizeof(struct tps68470_device), + GFP_KERNEL); + if (!tps68470) + return -ENOMEM; + + 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; + } + + if (i == TPS68470_ILED_B) { + ret = tps68470_ledb_current_init(pdev, tps68470); + if (ret) + goto err_exit; + } + } + +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 | 185 +++++++++++++++++++++++++++++++++++ 3 files changed, 198 insertions(+) create mode 100644 drivers/leds/leds-tps68470.c