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@xxxxxxxxxx>
---
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
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?
+
+ 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@xxxxxxxxxx>
+ */
+
+#include <linux/gpio/driver.h>
Not needed I think?
+#include <linux/mfd/tps68470.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/leds.h>
Alphabetical order?
+
+#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.
+
+ 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.
+
+ 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?
+ 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.
+ }
+ 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");
