* Re: [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470
2023-03-21 15:37 ` [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470 Kate Hsuan
@ 2023-03-22 16:46 ` Hans de Goede
2023-03-23 7:36 ` Dan Scally
2023-03-23 11:15 ` Pavel Machek
2 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2023-03-22 16:46 UTC (permalink / raw)
To: Kate Hsuan, Pavel Machek, Lee Jones, linux-leds,
platform-driver-x86, Daniel Scally, Mark Gross
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");
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470
2023-03-21 15:37 ` [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470 Kate Hsuan
2023-03-22 16:46 ` Hans de Goede
@ 2023-03-23 7:36 ` Dan Scally
2023-03-23 11:15 ` Pavel Machek
2 siblings, 0 replies; 17+ messages in thread
From: Dan Scally @ 2023-03-23 7:36 UTC (permalink / raw)
To: Kate Hsuan, Pavel Machek, Lee Jones, linux-leds,
platform-driver-x86, Hans de Goede, Daniel Scally, Mark Gross
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");
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470
2023-03-21 15:37 ` [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470 Kate Hsuan
2023-03-22 16:46 ` Hans de Goede
2023-03-23 7:36 ` Dan Scally
@ 2023-03-23 11:15 ` Pavel Machek
2023-03-23 11:24 ` Hans de Goede
2 siblings, 1 reply; 17+ messages in thread
From: Pavel Machek @ 2023-03-23 11:15 UTC (permalink / raw)
To: Kate Hsuan
Cc: Lee Jones, linux-leds, platform-driver-x86, Hans de Goede,
Daniel Scally, Mark Gross
[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]
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
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470
2023-03-23 11:15 ` Pavel Machek
@ 2023-03-23 11:24 ` Hans de Goede
2023-03-23 11:26 ` Pavel Machek
0 siblings, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2023-03-23 11:24 UTC (permalink / raw)
To: Pavel Machek, Kate Hsuan
Cc: Lee Jones, linux-leds, platform-driver-x86, Daniel Scally,
Mark Gross
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470
2023-03-23 11:24 ` Hans de Goede
@ 2023-03-23 11:26 ` Pavel Machek
2023-03-23 11:29 ` Hans de Goede
0 siblings, 1 reply; 17+ messages in thread
From: Pavel Machek @ 2023-03-23 11:26 UTC (permalink / raw)
To: Hans de Goede
Cc: Kate Hsuan, Lee Jones, linux-leds, platform-driver-x86,
Daniel Scally, Mark Gross
[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]
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
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470
2023-03-23 11:26 ` Pavel Machek
@ 2023-03-23 11:29 ` Hans de Goede
2023-03-23 11:31 ` Hans de Goede
2023-03-23 11:36 ` Pavel Machek
0 siblings, 2 replies; 17+ messages in thread
From: Hans de Goede @ 2023-03-23 11:29 UTC (permalink / raw)
To: Pavel Machek
Cc: Kate Hsuan, Lee Jones, linux-leds, platform-driver-x86,
Daniel Scally, Mark Gross
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470
2023-03-23 11:29 ` Hans de Goede
@ 2023-03-23 11:31 ` Hans de Goede
2023-03-23 11:36 ` Pavel Machek
1 sibling, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2023-03-23 11:31 UTC (permalink / raw)
To: Pavel Machek
Cc: Kate Hsuan, Lee Jones, linux-leds, platform-driver-x86,
Daniel Scally, Mark Gross
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470
2023-03-23 11:29 ` Hans de Goede
2023-03-23 11:31 ` Hans de Goede
@ 2023-03-23 11:36 ` Pavel Machek
2023-03-23 12:36 ` Hans de Goede
1 sibling, 1 reply; 17+ messages in thread
From: Pavel Machek @ 2023-03-23 11:36 UTC (permalink / raw)
To: Hans de Goede
Cc: Kate Hsuan, Lee Jones, linux-leds, platform-driver-x86,
Daniel Scally, Mark Gross
[-- Attachment #1: Type: text/plain, Size: 1657 bytes --]
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
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470
2023-03-23 11:36 ` Pavel Machek
@ 2023-03-23 12:36 ` Hans de Goede
0 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2023-03-23 12:36 UTC (permalink / raw)
To: Pavel Machek
Cc: Kate Hsuan, Lee Jones, linux-leds, platform-driver-x86,
Daniel Scally, Mark Gross
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
^ permalink raw reply [flat|nested] 17+ messages in thread