Re: [PATCH v9 2/2] leds: as3668: Driver for the ams Osram 4-channel i2c LED driver

From: Lukas Timmermann
Date: Mon Nov 10 2025 - 06:55:20 EST

Next message: Thadeu Lima de Souza Cascardo: "Re: [PATCH] ext4: validate xattrs to avoid OOB in ext4_find_inline_entry"
Previous message: Alice Ryhl: "Re: [PATCH 2/2] rust: device: make example buildable"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Am 23.10.25 um 16:18 schrieb Lee Jones:

On Tue, 14 Oct 2025, Lukas Timmermann wrote:

Since there were no existing drivers for the AS3668 or related devices,
a new driver was introduced in a separate file. Similar devices were
reviewed, but none shared enough characteristics to justify code reuse.
As a result, this driver is written specifically for the AS3668.

Signed-off-by: Lukas Timmermann <linux@timmermann.space>
---
MAINTAINERS | 1 +
drivers/leds/Kconfig | 13 +++
drivers/leds/Makefile | 1 +
drivers/leds/leds-as3668.c | 188 +++++++++++++++++++++++++++++++++++++
4 files changed, 203 insertions(+)
create mode 100644 drivers/leds/leds-as3668.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 091206c54c63..945d78fef380 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3511,6 +3511,7 @@ M: Lukas Timmermann <linux@timmermann.space>
L: linux-leds@xxxxxxxxxxxxxxx
S: Maintained
F: Documentation/devicetree/bindings/leds/ams,as3668.yaml
+F: drivers/leds/leds-as3668.c
ASAHI KASEI AK7375 LENS VOICE COIL DRIVER
M: Tianshu Qiu <tian.shu.qiu@xxxxxxxxx>
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a104cbb0a001..8cfb423ddf82 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -100,6 +100,19 @@ config LEDS_ARIEL
Say Y to if your machine is a Dell Wyse 3020 thin client.
+config LEDS_AS3668

LEDS_OSRAM_AMS_AS3668

+ tristate "LED support for AMS AS3668"

"Osram"

Thanks. Makes sense.

+ depends on LEDS_CLASS
+ depends on I2C
+ help
+ This option enables support for the AMS AS3668 LED controller.

"Osram"

Same.>> + The AS3668 provides up to four LED channels and is controlled via

+ the I2C bus. This driver offers basic brightness control for each
+ channel, without support for blinking or other advanced features.
+
+ To compile this driver as a module, choose M here: the module
+ will be called leds-as3668.
+
config LEDS_AW200XX
tristate "LED support for Awinic AW20036/AW20054/AW20072/AW20108"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 2f170d69dcbf..983811384fec 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_LEDS_ADP5520) += leds-adp5520.o
obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o
obj-$(CONFIG_LEDS_APU) += leds-apu.o
obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o
+obj-$(CONFIG_LEDS_AS3668) += leds-as3668.o
obj-$(CONFIG_LEDS_AW200XX) += leds-aw200xx.o
obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o
obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
diff --git a/drivers/leds/leds-as3668.c b/drivers/leds/leds-as3668.c
new file mode 100644
index 000000000000..2b7b776fe2f5
--- /dev/null
+++ b/drivers/leds/leds-as3668.c
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Osram AMS AS3668 LED Driver IC
+ *
+ * Copyright (C) 2025 Lukas Timmermann <linux@timmermann.space>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/uleds.h>
+
+#define AS3668_MAX_LEDS 4
+#define AS3668_EXPECTED_I2C_ADDR 0x42
+
+/* Chip Ident */
+
+#define AS3668_CHIP_ID1_REG 0x3e
+#define AS3668_CHIP_ID 0xa5
+
+/* Current Control */
+
+#define AS3668_CURRX_CONTROL_REG 0x01
+#define AS3668_CURR1_REG 0x02
+#define AS3668_CURR2_REG 0x03
+#define AS3668_CURR3_REG 0x04
+#define AS3668_CURR4_REG 0x05
+#define AS3668_CURRX_MODE_ON 0x1
+#define AS3668_CURRX_CURR1_MASK GENMASK(1, 0)
+#define AS3668_CURRX_CURR2_MASK GENMASK(3, 2)
+#define AS3668_CURRX_CURR3_MASK GENMASK(5, 4)
+#define AS3668_CURRX_CURR4_MASK GENMASK(7, 6)
+
+struct as3668_led {
+ struct led_classdev cdev;
+ struct as3668 *chip;
+ struct fwnode_handle *fwnode;
+ int led_id;
+};
+
+struct as3668 {
+ struct i2c_client *client;
+ struct as3668_led leds[AS3668_MAX_LEDS];
+};
+
+static enum led_brightness as3668_brightness_get(struct led_classdev *cdev)
+{
+ struct as3668_led *led = container_of(cdev, struct as3668_led, cdev);
+
+ return i2c_smbus_read_byte_data(led->chip->client, AS3668_CURR1_REG + led->led_id);
+}
+
+static void as3668_brightness_set(struct led_classdev *cdev, enum led_brightness brightness)
+{
+ struct as3668_led *led = container_of(cdev, struct as3668_led, cdev);
+
+ int err = i2c_smbus_write_byte_data(led->chip->client,
+ AS3668_CURR1_REG + led->led_id,
+ brightness);
+
+ if (err)
+ dev_err(&led->chip->client->dev, "error writing to reg 0x%02x, returned %d\n",

The user isn't going to care about this stuff.

"Failed to set brightness: %d"

+ AS3668_CURR1_REG + led->led_id, err);
+}
+
+static int as3668_dt_init(struct as3668 *as3668)
+{
+ struct device *dev = &as3668->client->dev;
+ struct as3668_led *led;
+ struct led_init_data init_data = {};
+ int err;
+ u32 reg;
+
+ for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
+ err = of_property_read_u32(child, "reg", &reg);
+ if (err)
+ return dev_err_probe(dev, err, "'reg' property missing from %s\n",

"Failed to read 'reg' property"

Thanks

+ child->name);
+
+ if (reg < 0 || reg > AS3668_MAX_LEDS)
+ return dev_err_probe(dev, -EOPNOTSUPP,
+ "'reg' property in %s is out of scope: %d\n",

"Unsupported LED: %d"

I understand now that should be user facing messages... Thanks.

+ child->name, reg);
+
+ led = &as3668->leds[reg];
+ led->fwnode = of_fwnode_handle(child);
+
+ led->led_id = reg;
+ led->chip = as3668;
+
+ led->cdev.max_brightness = U8_MAX;
+ led->cdev.brightness_get = as3668_brightness_get;
+ led->cdev.brightness_set = as3668_brightness_set;
+
+ init_data.fwnode = led->fwnode;
+ init_data.default_label = ":";
+
+ err = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
+ if (err)
+ return dev_err_probe(dev, err, "failed to register LED %d\n", reg);
+ }
+
+ return 0;
+}
+
+static int as3668_probe(struct i2c_client *client)
+{
+ struct as3668 *as3668;
+ int err;
+ u8 chip_id;
+
+ if (client->addr != AS3668_EXPECTED_I2C_ADDR)

Expected is weird.

Why are we trying to catch-out the consumer?

If you already know what the I2C address is, just use that.

Okay, I will do that instead.
I aim to fail early in my code and double check everything.
Drivers shouldn't error check the device tree, if I understand you correctly.

+ return dev_err_probe(&client->dev, -EFAULT,
+ "expected i2c address 0x%02x, got 0x%02x\n",
+ AS3668_EXPECTED_I2C_ADDR, client->addr);
+
+ /* Read identifier from chip */

This comment is superfluous IMHO.

The register name should tell us everything.

+ chip_id = i2c_smbus_read_byte_data(client, AS3668_CHIP_ID1_REG);
+

Remove this line.

+ if (chip_id != AS3668_CHIP_ID)
+ return dev_err_probe(&client->dev, -ENODEV,
+ "expected chip id 0x%02x, got 0x%02x\n",

"ID"

+ AS3668_CHIP_ID, chip_id);
+
+ as3668 = devm_kzalloc(&client->dev, sizeof(*as3668), GFP_KERNEL);
+ if (!as3668)
+ return -ENOMEM;
+
+ as3668->client = client;
+
+ err = as3668_dt_init(as3668);
+ if (err)
+ return err;
+
+ /* Set all four channel modes to 'on' */

Even if a specific LED wasn't requested?

Are you sure that this doesn't have any drawbacks (power perhaps)?

After reading through downstream code and it's datasheet, this actually might result in higher power consumption than switching it off.
I suppose we could enable and disable a specific channel when setting the brightness. I will add that in my next patch version.

+ err = i2c_smbus_write_byte_data(client, AS3668_CURRX_CONTROL_REG,
+ FIELD_PREP(AS3668_CURRX_CURR1_MASK, AS3668_CURRX_MODE_ON) |
+ FIELD_PREP(AS3668_CURRX_CURR2_MASK, AS3668_CURRX_MODE_ON) |
+ FIELD_PREP(AS3668_CURRX_CURR3_MASK, AS3668_CURRX_MODE_ON) |
+ FIELD_PREP(AS3668_CURRX_CURR4_MASK, AS3668_CURRX_MODE_ON));
+
+ /* Set initial currents to 0mA */
+ err |= i2c_smbus_write_byte_data(client, AS3668_CURR1_REG, 0);
+ err |= i2c_smbus_write_byte_data(client, AS3668_CURR2_REG, 0);
+ err |= i2c_smbus_write_byte_data(client, AS3668_CURR3_REG, 0);
+ err |= i2c_smbus_write_byte_data(client, AS3668_CURR4_REG, 0);
+
+ if (err)
+ return dev_err_probe(&client->dev, -EIO, "failed to write to the device\n");
+
+ return 0;
+}
+
+static void as3668_remove(struct i2c_client *client)
+{
+ int err;

'\n' here.

Okay

+ err = i2c_smbus_write_byte_data(client, AS3668_CURRX_CONTROL_REG, 0);
+ if (err)
+ dev_err(&client->dev, "couldn't remove the device\n");

This does not remove the device.

"Failed to turn off the LEDs"

Obviously, now that I see it. Thanks

+}
+
+static const struct i2c_device_id as3668_idtable[] = {
+ { "as3668" },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, as3668_idtable);
+
+static const struct of_device_id as3668_match_table[] = {
+ { .compatible = "ams,as3668" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, as3668_match_table);
+
+static struct i2c_driver as3668_driver = {
+ .driver = {
+ .name = "leds_as3668",
+ .of_match_table = as3668_match_table,
+ },
+ .probe = as3668_probe,
+ .remove = as3668_remove,
+ .id_table = as3668_idtable,
+};
+module_i2c_driver(as3668_driver);
+
+MODULE_AUTHOR("Lukas Timmermann <linux@timmermann.space>");
+MODULE_DESCRIPTION("AS3668 LED driver");
+MODULE_LICENSE("GPL");
--
2.51.0

Next message: Thadeu Lima de Souza Cascardo: "Re: [PATCH] ext4: validate xattrs to avoid OOB in ext4_find_inline_entry"
Previous message: Alice Ryhl: "Re: [PATCH 2/2] rust: device: make example buildable"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]