[PATCH v10 0/2] Support for Osram as3668 LED driver

[PATCH v10 0/2] Support for Osram as3668 LED driver

[Lukas Timmermann]

This patch adds basic support for the as3668 driver IC via I2C interface. 
The IC is capable of driving four individual LEDs up to 25.5mA per 
channel. Hardware blinking would be theoretically possible, but this chip
only supports a few set on/off-delays which makes using that feature 
unfeasable, therefore my driver doesn't offer that capability. 
It's intended applications is in mobile devices such as phones, 
tablets and cameras. This driver was tested and is working on 
a samsung-manta which is running postmarketOS with a near mainline kernel.
The register names and values are taken from the official datasheet which
can be found here:
https://www.mouser.com/datasheet/2/588/AS3668_DS000196_1-00-1512816.pdf

Please note: This is my first suggested patch to the kernel. 
I've read the docs in regards to the led subsystem, 
coding style and submission of patches, 
but I'm still a bit unsure about the general workflow. 

I will try my best.

Changes in v10:
- Reworded some error messages to be more clear. (@Lee)
- Removed newline. (@Lee)
- Added enabling and disabling of channels during brightness set. (@Lee)
- Rearranged and renamed constants.
- Link to v9: https://lore.kernel.org/all/20251014152604.852487-1-linux@timmermann.space/
Changes in v9:
- Tabbed out values. (@Lee)
- Removed newlines. (@Lee)
- Renamed chip id constant and variables to be more clear. (@Lee)
- Removed unnecessary comments. (@Lee)
- Removed unnecessary debug message. (@Lee)
- Separate declaration and assignment of err variable. (@Lee)
- Link to v8: https://lore.kernel.org/all/20250808213143.146732-1-linux@timmermann.space/
Changes in v8:
- Rearranged constants.
- Removed more newlines.
- Changed error messages to be more clear.
- Renamed variables.
- Removed revision check.
- Removed extra i2c read&write functions.
- Made initalisation code more readable with bitmasks.
- The code now wraps around before 100 chars instead of 80.
- Link to v7: https://lore.kernel.org/all/20250708141114.134950-1-linux@timmermann.space/
Changes in v7:
- Simplified multiple error messages. They now use dev_err_probe().
- Removed some newlines.
- Link to v6: https://lore.kernel.org/all/20250611083151.22150-1-linux@timmermann.space/
Changes in v6:
- Fixed missing error handling during init
- Fixed missing newline in error messages
- Fixed size calculation for memory allocation
- Fixed error handling for memory allocation
- Link to v5: https://lore.kernel.org/lkml/20250608231854.75668-1-linux@timmermann.space/
Changes in v5:
- Fixed debug and error messages using wrong format specifiers.
- Fixed missing include bitwise.h.
- Changed commit message for dt file to fit expected style.
- Link to v4: https://lore.kernel.org/lkml/20250607215049.29259-1-linux@timmermann.space/
Changes in v4:
- Fixed some mistakes made in the dt file pointed out in v3.
- Swapped dt and driver in patch series. DT now comes first.
- Fixed errors in Kconfig due to last minute changes.
- Added dt file into MAINTAINERS file.
- Link to v3: https://lore.kernel.org/lkml/20250604225838.102910-2-linux@timmermann.space/
Changes in v3:
- Fixed an extra whitespace in the dt bindings documentation.
- Sent patch to all related lists and maintainers.
- Link to v2: https://lore.kernel.org/lkml/20250531120715.302870-4-linux@timmermann.space/
Changes in v2:
- Fixed reading led subnodes in dt incorrectly, 
  which caused wrong numbering and a segfault when removing the driver module
- Fixed calling of_property_read_u8 with an int, causing a compiler error
- Added more error checking during writes to the i2c bus
- Link to v1: https://lore.kernel.org/linux-leds/20250530184219.78085-3-linux@timmermann.space/

Signed-off-by: Lukas Timmermann <linux@timmermann.space>

Lukas Timmermann (2):
  dt-bindings: leds: Add new as3668 support
  leds: as3668: Driver for the ams Osram 4-channel i2c LED driver

 .../devicetree/bindings/leds/ams,as3668.yaml  |  74 ++++++
 MAINTAINERS                                   |   7 +
 drivers/leds/Kconfig                          |  13 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-as3668.c                    | 222 ++++++++++++++++++
 5 files changed, 317 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/ams,as3668.yaml
 create mode 100644 drivers/leds/leds-as3668.c

-- 
2.51.2

[PATCH v10 1/2] dt-bindings: leds: Add new as3668 support

[Lukas Timmermann]

The bindings are incomplete, as the GPIO/Audio Input pin
is still undocumented. The hardware used for testing this patch series
does not allow modification, so the mentioned pin has been omitted.

Signed-off-by: Lukas Timmermann <linux@timmermann.space>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/leds/ams,as3668.yaml  | 74 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/ams,as3668.yaml

diff --git a/Documentation/devicetree/bindings/leds/ams,as3668.yaml b/Documentation/devicetree/bindings/leds/ams,as3668.yaml
new file mode 100644
index 000000000000..d1d73782da55
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/ams,as3668.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/ams,as3668.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Osram 4-channel i2c LED driver
+
+maintainers:
+  - Lukas Timmermann <linux@timmermann.space>
+
+description:
+  This IC can drive up to four separate LEDs.
+  Having four channels suggests it could be used with a single RGBW LED.
+
+properties:
+  compatible:
+    const: ams,as3668
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^led@[0-3]$":
+    type: object
+    $ref: common.yaml#
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      led-controller@42 {
+        compatible = "ams,as3668";
+        reg = <0x42>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led@0 {
+          reg = <0x0>;
+          function = LED_FUNCTION_STATUS;
+          color = <LED_COLOR_ID_RED>;
+        };
+
+        led@1 {
+          reg = <0x1>;
+          function = LED_FUNCTION_STATUS;
+          color = <LED_COLOR_ID_GREEN>;
+        };
+      };
+    };
+
diff --git a/MAINTAINERS b/MAINTAINERS
index c59316109e3f..091206c54c63 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3506,6 +3506,12 @@ L:	linux-leds@vger.kernel.org
 S:	Maintained
 F:	drivers/leds/flash/leds-as3645a.c
 
+AS3668 LED DRIVER
+M:	Lukas Timmermann <linux@timmermann.space>
+L:	linux-leds@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/leds/ams,as3668.yaml
+
 ASAHI KASEI AK7375 LENS VOICE COIL DRIVER
 M:	Tianshu Qiu <tian.shu.qiu@intel.com>
 L:	linux-media@vger.kernel.org
-- 
2.51.2

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

[Lukas Timmermann]

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 | 222 +++++++++++++++++++++++++++++++++++++
 4 files changed, 237 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@vger.kernel.org
 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@intel.com>
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a104cbb0a001..ec37d55ac14e 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_OSRAM_AMS_AS3668
+	tristate "LED support for Osram AMS AS3668"
+	depends on LEDS_CLASS
+	depends on I2C
+	help
+	  This option enables support for the Osram AMS AS3668 LED controller.
+	  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..8c43429f2856
--- /dev/null
+++ b/drivers/leds/leds-as3668.c
@@ -0,0 +1,222 @@
+// 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
+
+/* Chip Ident */
+
+#define AS3668_CHIP_ID1_REG		0x3e
+#define AS3668_CHIP_ID			0xa5
+
+/* Current Control */
+
+#define AS3668_CURR_MODE_REG		0x01
+#define AS3668_CURR_MODE_OFF		0x0
+#define AS3668_CURR_MODE_ON		0x1
+#define AS3668_CURR1_MODE_MASK		GENMASK(1, 0)
+#define AS3668_CURR2_MODE_MASK		GENMASK(3, 2)
+#define AS3668_CURR3_MODE_MASK		GENMASK(5, 4)
+#define AS3668_CURR4_MODE_MASK		GENMASK(7, 6)
+#define AS3668_CURR1_REG		0x02
+#define AS3668_CURR2_REG		0x03
+#define AS3668_CURR3_REG		0x04
+#define AS3668_CURR4_REG		0x05
+
+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 void as3668_channel_mode_set(struct as3668 *as3668, int led_id, u8 mode)
+{
+	int err;
+	u8 reg;
+
+	reg = i2c_smbus_read_byte_data(as3668->client, AS3668_CURR_MODE_REG);
+	if (reg < 0) {
+		dev_err(&as3668->client->dev, "failed to read channel modes\n");
+		return;
+	}
+
+	switch (led_id) {
+	case 0:
+		reg &= ~AS3668_CURR1_MODE_MASK;
+		reg |= FIELD_PREP(AS3668_CURR1_MODE_MASK, mode);
+		break;
+	case 1:
+		reg &= ~AS3668_CURR2_MODE_MASK;
+		reg |= FIELD_PREP(AS3668_CURR2_MODE_MASK, mode);
+		break;
+	case 2:
+		reg &= ~AS3668_CURR3_MODE_MASK;
+		reg |= FIELD_PREP(AS3668_CURR3_MODE_MASK, mode);
+		break;
+	case 3:
+		reg &= ~AS3668_CURR4_MODE_MASK;
+		reg |= FIELD_PREP(AS3668_CURR4_MODE_MASK, mode);
+		break;
+	default:
+		return;
+	}
+
+	err = i2c_smbus_write_byte_data(as3668->client, AS3668_CURR_MODE_REG, reg);
+	if (err)
+		dev_err(&as3668->client->dev, "failed to set channel modes\n");
+}
+
+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;
+
+	if (brightness == 0)
+		as3668_channel_mode_set(led->chip, led->led_id, AS3668_CURR_MODE_OFF);
+	else
+		as3668_channel_mode_set(led->chip, led->led_id, AS3668_CURR_MODE_ON);
+
+	err = i2c_smbus_write_byte_data(led->chip->client,
+					AS3668_CURR1_REG + led->led_id,
+					brightness);
+
+	if (err)
+		dev_err(&led->chip->client->dev, "failed to set brightness: %d\n", 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, "failed to read 'reg' property");
+
+		if (reg < 0 || reg > AS3668_MAX_LEDS)
+			return dev_err_probe(dev, -EOPNOTSUPP,
+					     "unsupported LED: %d\n", 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;
+
+	chip_id = i2c_smbus_read_byte_data(client, AS3668_CHIP_ID1_REG);
+	if (chip_id != AS3668_CHIP_ID)
+		return dev_err_probe(&client->dev, -ENODEV,
+				     "expected chip ID 0x%02x, got 0x%02x\n",
+				     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 'off' */
+	err = i2c_smbus_write_byte_data(client, AS3668_CURR_MODE_REG,
+					FIELD_PREP(AS3668_CURR1_MODE_MASK, AS3668_CURR_MODE_OFF) |
+					FIELD_PREP(AS3668_CURR2_MODE_MASK, AS3668_CURR_MODE_OFF) |
+					FIELD_PREP(AS3668_CURR3_MODE_MASK, AS3668_CURR_MODE_OFF) |
+					FIELD_PREP(AS3668_CURR4_MODE_MASK, AS3668_CURR_MODE_OFF));
+
+	/* 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;
+
+	err = i2c_smbus_write_byte_data(client, AS3668_CURR_MODE_REG, 0);
+	if (err)
+		dev_err(&client->dev, "failed to turn off the LEDs\n");
+}
+
+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.2

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

[Lukas Timmermann]

On Mon, Nov 17, 2025 at 03:00:08AM +0100, 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 | 222 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 237 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@vger.kernel.org
>  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@intel.com>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index a104cbb0a001..ec37d55ac14e 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_OSRAM_AMS_AS3668
I've modified this line as requested in patch series v9. After comparing
this with other configuration options in drivers/leds/Kconfig, this
seems out of place. Shouldn't we keep this consistent?
> +	tristate "LED support for Osram AMS AS3668"
> +	depends on LEDS_CLASS
> +	depends on I2C
> +	help
> +	  This option enables support for the Osram AMS AS3668 LED controller.
> +	  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..8c43429f2856
> --- /dev/null
> +++ b/drivers/leds/leds-as3668.c
> @@ -0,0 +1,222 @@
> +// 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
> +
> +/* Chip Ident */
> +
> +#define AS3668_CHIP_ID1_REG		0x3e
> +#define AS3668_CHIP_ID			0xa5
> +
> +/* Current Control */
> +
> +#define AS3668_CURR_MODE_REG		0x01
> +#define AS3668_CURR_MODE_OFF		0x0
> +#define AS3668_CURR_MODE_ON		0x1
> +#define AS3668_CURR1_MODE_MASK		GENMASK(1, 0)
> +#define AS3668_CURR2_MODE_MASK		GENMASK(3, 2)
> +#define AS3668_CURR3_MODE_MASK		GENMASK(5, 4)
> +#define AS3668_CURR4_MODE_MASK		GENMASK(7, 6)
> +#define AS3668_CURR1_REG		0x02
> +#define AS3668_CURR2_REG		0x03
> +#define AS3668_CURR3_REG		0x04
> +#define AS3668_CURR4_REG		0x05
> +
> +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 void as3668_channel_mode_set(struct as3668 *as3668, int led_id, u8 mode)
> +{
> +	int err;
> +	u8 reg;
> +
> +	reg = i2c_smbus_read_byte_data(as3668->client, AS3668_CURR_MODE_REG);
> +	if (reg < 0) {
> +		dev_err(&as3668->client->dev, "failed to read channel modes\n");
> +		return;
> +	}
> +
> +	switch (led_id) {
> +	case 0:
> +		reg &= ~AS3668_CURR1_MODE_MASK;
> +		reg |= FIELD_PREP(AS3668_CURR1_MODE_MASK, mode);
> +		break;
> +	case 1:
> +		reg &= ~AS3668_CURR2_MODE_MASK;
> +		reg |= FIELD_PREP(AS3668_CURR2_MODE_MASK, mode);
> +		break;
> +	case 2:
> +		reg &= ~AS3668_CURR3_MODE_MASK;
> +		reg |= FIELD_PREP(AS3668_CURR3_MODE_MASK, mode);
> +		break;
> +	case 3:
> +		reg &= ~AS3668_CURR4_MODE_MASK;
> +		reg |= FIELD_PREP(AS3668_CURR4_MODE_MASK, mode);
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	err = i2c_smbus_write_byte_data(as3668->client, AS3668_CURR_MODE_REG, reg);
> +	if (err)
> +		dev_err(&as3668->client->dev, "failed to set channel modes\n");
> +}
> +
> +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;
> +
> +	if (brightness == 0)
> +		as3668_channel_mode_set(led->chip, led->led_id, AS3668_CURR_MODE_OFF);
> +	else
> +		as3668_channel_mode_set(led->chip, led->led_id, AS3668_CURR_MODE_ON);
> +
> +	err = i2c_smbus_write_byte_data(led->chip->client,
> +					AS3668_CURR1_REG + led->led_id,
> +					brightness);
> +
> +	if (err)
> +		dev_err(&led->chip->client->dev, "failed to set brightness: %d\n", 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, "failed to read 'reg' property");
> +
> +		if (reg < 0 || reg > AS3668_MAX_LEDS)
> +			return dev_err_probe(dev, -EOPNOTSUPP,
> +					     "unsupported LED: %d\n", 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;
> +
> +	chip_id = i2c_smbus_read_byte_data(client, AS3668_CHIP_ID1_REG);
> +	if (chip_id != AS3668_CHIP_ID)
> +		return dev_err_probe(&client->dev, -ENODEV,
> +				     "expected chip ID 0x%02x, got 0x%02x\n",
> +				     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 'off' */
> +	err = i2c_smbus_write_byte_data(client, AS3668_CURR_MODE_REG,
> +					FIELD_PREP(AS3668_CURR1_MODE_MASK, AS3668_CURR_MODE_OFF) |
> +					FIELD_PREP(AS3668_CURR2_MODE_MASK, AS3668_CURR_MODE_OFF) |
> +					FIELD_PREP(AS3668_CURR3_MODE_MASK, AS3668_CURR_MODE_OFF) |
> +					FIELD_PREP(AS3668_CURR4_MODE_MASK, AS3668_CURR_MODE_OFF));
> +
> +	/* 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;
> +
> +	err = i2c_smbus_write_byte_data(client, AS3668_CURR_MODE_REG, 0);
> +	if (err)
> +		dev_err(&client->dev, "failed to turn off the LEDs\n");
> +}
> +
> +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.2
> 
> 
--
Best regards,
Lukas Timmermann

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

[Lee Jones]

On Mon, 17 Nov 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 | 222 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 237 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@vger.kernel.org
>  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@intel.com>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index a104cbb0a001..ec37d55ac14e 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_OSRAM_AMS_AS3668
> +	tristate "LED support for Osram AMS AS3668"
> +	depends on LEDS_CLASS
> +	depends on I2C
> +	help
> +	  This option enables support for the Osram AMS AS3668 LED controller.
> +	  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..8c43429f2856
> --- /dev/null
> +++ b/drivers/leds/leds-as3668.c
> @@ -0,0 +1,222 @@
> +// 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
> +
> +/* Chip Ident */
> +
> +#define AS3668_CHIP_ID1_REG		0x3e
> +#define AS3668_CHIP_ID			0xa5
> +
> +/* Current Control */
> +
> +#define AS3668_CURR_MODE_REG		0x01
> +#define AS3668_CURR_MODE_OFF		0x0
> +#define AS3668_CURR_MODE_ON		0x1
> +#define AS3668_CURR1_MODE_MASK		GENMASK(1, 0)
> +#define AS3668_CURR2_MODE_MASK		GENMASK(3, 2)
> +#define AS3668_CURR3_MODE_MASK		GENMASK(5, 4)
> +#define AS3668_CURR4_MODE_MASK		GENMASK(7, 6)
> +#define AS3668_CURR1_REG		0x02
> +#define AS3668_CURR2_REG		0x03
> +#define AS3668_CURR3_REG		0x04
> +#define AS3668_CURR4_REG		0x05
> +
> +struct as3668_led {
> +	struct led_classdev cdev;
> +	struct as3668 *chip;
> +	struct fwnode_handle *fwnode;
> +	int led_id;

If you stored AS3668_CURR{X}_MODE_MASK and AS3668_CURR1_REG + {X} in
here, you could omit led_id from here and save on a bunch of parameter
passing and additional handling (i.e. removal of the switch(), etc).

> +};
> +
> +struct as3668 {
> +	struct i2c_client *client;
> +	struct as3668_led leds[AS3668_MAX_LEDS];
> +};
> +
> +static void as3668_channel_mode_set(struct as3668 *as3668, int led_id, u8 mode)
> +{
> +	int err;
> +	u8 reg;
> +
> +	reg = i2c_smbus_read_byte_data(as3668->client, AS3668_CURR_MODE_REG);

Does CURR have anything to do with Current (amps)?

Either way 'reg' can be named better.

> +	if (reg < 0) {
> +		dev_err(&as3668->client->dev, "failed to read channel modes\n");
> +		return;
> +	}
> +
> +	switch (led_id) {
> +	case 0:
> +		reg &= ~AS3668_CURR1_MODE_MASK;
> +		reg |= FIELD_PREP(AS3668_CURR1_MODE_MASK, mode);
> +		break;
> +	case 1:
> +		reg &= ~AS3668_CURR2_MODE_MASK;
> +		reg |= FIELD_PREP(AS3668_CURR2_MODE_MASK, mode);
> +		break;
> +	case 2:
> +		reg &= ~AS3668_CURR3_MODE_MASK;
> +		reg |= FIELD_PREP(AS3668_CURR3_MODE_MASK, mode);
> +		break;
> +	case 3:
> +		reg &= ~AS3668_CURR4_MODE_MASK;
> +		reg |= FIELD_PREP(AS3668_CURR4_MODE_MASK, mode);
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	err = i2c_smbus_write_byte_data(as3668->client, AS3668_CURR_MODE_REG, reg);

Either it's an error or it's not.  Why isn't it being propagated?

> +}
> +
> +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;
> +
> +	if (brightness == 0)
> +		as3668_channel_mode_set(led->chip, led->led_id, AS3668_CURR_MODE_OFF);
> +	else
> +		as3668_channel_mode_set(led->chip, led->led_id, AS3668_CURR_MODE_ON);

If you take my advice further up, you can drop all of this for:

	as3668_channel_mode_set(led, !!brightness);

> +
> +	err = i2c_smbus_write_byte_data(led->chip->client,
> +					AS3668_CURR1_REG + led->led_id,
> +					brightness);
> +
> +	if (err)
> +		dev_err(&led->chip->client->dev, "failed to set brightness: %d\n", err);

cdev->dev

> +}
> +
> +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, "failed to read 'reg' property");
> +
> +		if (reg < 0 || reg > AS3668_MAX_LEDS)
> +			return dev_err_probe(dev, -EOPNOTSUPP,

This should be -EINVAL.

> +					     "unsupported LED: %d\n", 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;
> +
> +	chip_id = i2c_smbus_read_byte_data(client, AS3668_CHIP_ID1_REG);
> +	if (chip_id != AS3668_CHIP_ID)
> +		return dev_err_probe(&client->dev, -ENODEV,
> +				     "expected chip ID 0x%02x, got 0x%02x\n",
> +				     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 'off' */
> +	err = i2c_smbus_write_byte_data(client, AS3668_CURR_MODE_REG,
> +					FIELD_PREP(AS3668_CURR1_MODE_MASK, AS3668_CURR_MODE_OFF) |
> +					FIELD_PREP(AS3668_CURR2_MODE_MASK, AS3668_CURR_MODE_OFF) |
> +					FIELD_PREP(AS3668_CURR3_MODE_MASK, AS3668_CURR_MODE_OFF) |
> +					FIELD_PREP(AS3668_CURR4_MODE_MASK, AS3668_CURR_MODE_OFF));
> +
> +	/* 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");

Failed to set zero initial current levels

> +	return 0;
> +}
> +
> +static void as3668_remove(struct i2c_client *client)
> +{
> +	int err;
> +
> +	err = i2c_smbus_write_byte_data(client, AS3668_CURR_MODE_REG, 0);
> +	if (err)
> +		dev_err(&client->dev, "failed to turn off the LEDs\n");

This is probably not useful to the user.

Just make an attempt to turn them off, then leave.

> +}
> +
> +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.2
> 

-- 
Lee Jones [李琼斯]

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

[Lee Jones]

On Thu, 20 Nov 2025, Lukas Timmermann wrote:

> On Mon, Nov 17, 2025 at 03:00:08AM +0100, 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 | 222 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 237 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@vger.kernel.org
> >  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@intel.com>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index a104cbb0a001..ec37d55ac14e 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_OSRAM_AMS_AS3668
> I've modified this line as requested in patch series v9. After comparing
> this with other configuration options in drivers/leds/Kconfig, this
> seems out of place. Shouldn't we keep this consistent?

There are a few other examples in there, so consistency is less of an
issue.  I personally think it provides a better picture of the device if
the manufacture is mentioned as well.

But this is not a blocking point.  Take your preference.

-- 
Lee Jones [李琼斯]

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

[Lukas Timmermann]

On Thu, Nov 20, 2025 at 12:07:04PM +0000, Lee Jones wrote:
> On Mon, 17 Nov 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 | 222 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 237 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@vger.kernel.org
> >  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@intel.com>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index a104cbb0a001..ec37d55ac14e 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_OSRAM_AMS_AS3668
> > +	tristate "LED support for Osram AMS AS3668"
> > +	depends on LEDS_CLASS
> > +	depends on I2C
> > +	help
> > +	  This option enables support for the Osram AMS AS3668 LED controller.
> > +	  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..8c43429f2856
> > --- /dev/null
> > +++ b/drivers/leds/leds-as3668.c
> > @@ -0,0 +1,222 @@
> > +// 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
> > +
> > +/* Chip Ident */
> > +
> > +#define AS3668_CHIP_ID1_REG		0x3e
> > +#define AS3668_CHIP_ID			0xa5
> > +
> > +/* Current Control */
> > +
> > +#define AS3668_CURR_MODE_REG		0x01
> > +#define AS3668_CURR_MODE_OFF		0x0
> > +#define AS3668_CURR_MODE_ON		0x1
> > +#define AS3668_CURR1_MODE_MASK		GENMASK(1, 0)
> > +#define AS3668_CURR2_MODE_MASK		GENMASK(3, 2)
> > +#define AS3668_CURR3_MODE_MASK		GENMASK(5, 4)
> > +#define AS3668_CURR4_MODE_MASK		GENMASK(7, 6)
> > +#define AS3668_CURR1_REG		0x02
> > +#define AS3668_CURR2_REG		0x03
> > +#define AS3668_CURR3_REG		0x04
> > +#define AS3668_CURR4_REG		0x05
> > +
> > +struct as3668_led {
> > +	struct led_classdev cdev;
> > +	struct as3668 *chip;
> > +	struct fwnode_handle *fwnode;
> > +	int led_id;
> 
> If you stored AS3668_CURR{X}_MODE_MASK and AS3668_CURR1_REG + {X} in
> here, you could omit led_id from here and save on a bunch of parameter
> passing and additional handling (i.e. removal of the switch(), etc).
> 
I will check that out, thanks.
> > +};
> > +
> > +struct as3668 {
> > +	struct i2c_client *client;
> > +	struct as3668_led leds[AS3668_MAX_LEDS];
> > +};
> > +
> > +static void as3668_channel_mode_set(struct as3668 *as3668, int led_id, u8 mode)
> > +{
> > +	int err;
> > +	u8 reg;
> > +
> > +	reg = i2c_smbus_read_byte_data(as3668->client, AS3668_CURR_MODE_REG);
> 
> Does CURR have anything to do with Current (amps)?
> 
> Either way 'reg' can be named better.
> 
No. We read the register in order to modify just parts of its value. We then
write the modified state back to the register. I guess we could call it
'mode'.
> > +	if (reg < 0) {
> > +		dev_err(&as3668->client->dev, "failed to read channel modes\n");
> > +		return;
> > +	}
> > +
> > +	switch (led_id) {
> > +	case 0:
> > +		reg &= ~AS3668_CURR1_MODE_MASK;
> > +		reg |= FIELD_PREP(AS3668_CURR1_MODE_MASK, mode);
> > +		break;
> > +	case 1:
> > +		reg &= ~AS3668_CURR2_MODE_MASK;
> > +		reg |= FIELD_PREP(AS3668_CURR2_MODE_MASK, mode);
> > +		break;
> > +	case 2:
> > +		reg &= ~AS3668_CURR3_MODE_MASK;
> > +		reg |= FIELD_PREP(AS3668_CURR3_MODE_MASK, mode);
> > +		break;
> > +	case 3:
> > +		reg &= ~AS3668_CURR4_MODE_MASK;
> > +		reg |= FIELD_PREP(AS3668_CURR4_MODE_MASK, mode);
> > +		break;
> > +	default:
> > +		return;
> > +	}
> > +
> > +	err = i2c_smbus_write_byte_data(as3668->client, AS3668_CURR_MODE_REG, reg);
> 
> Either it's an error or it's not.  Why isn't it being propagated?
> 
My patch had a dev_err() call here. It's missing in your citation.
Was using dev_err() here wrong?
> > +}
> > +
> > +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;
> > +
> > +	if (brightness == 0)
> > +		as3668_channel_mode_set(led->chip, led->led_id, AS3668_CURR_MODE_OFF);
> > +	else
> > +		as3668_channel_mode_set(led->chip, led->led_id, AS3668_CURR_MODE_ON);
> 
> If you take my advice further up, you can drop all of this for:
> 
> 	as3668_channel_mode_set(led, !!brightness);
> 
Understood. That is indeed nicer.
> > +
> > +	err = i2c_smbus_write_byte_data(led->chip->client,
> > +					AS3668_CURR1_REG + led->led_id,
> > +					brightness);
> > +
> > +	if (err)
> > +		dev_err(&led->chip->client->dev, "failed to set brightness: %d\n", err);
> 
> cdev->dev
> 
That makes sense. Thank you.
> > +}
> > +
> > +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, "failed to read 'reg' property");
> > +
> > +		if (reg < 0 || reg > AS3668_MAX_LEDS)
> > +			return dev_err_probe(dev, -EOPNOTSUPP,
> 
> This should be -EINVAL.
> 
Acknowledged.
> > +					     "unsupported LED: %d\n", 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;
> > +
> > +	chip_id = i2c_smbus_read_byte_data(client, AS3668_CHIP_ID1_REG);
> > +	if (chip_id != AS3668_CHIP_ID)
> > +		return dev_err_probe(&client->dev, -ENODEV,
> > +				     "expected chip ID 0x%02x, got 0x%02x\n",
> > +				     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 'off' */
> > +	err = i2c_smbus_write_byte_data(client, AS3668_CURR_MODE_REG,
> > +					FIELD_PREP(AS3668_CURR1_MODE_MASK, AS3668_CURR_MODE_OFF) |
> > +					FIELD_PREP(AS3668_CURR2_MODE_MASK, AS3668_CURR_MODE_OFF) |
> > +					FIELD_PREP(AS3668_CURR3_MODE_MASK, AS3668_CURR_MODE_OFF) |
> > +					FIELD_PREP(AS3668_CURR4_MODE_MASK, AS3668_CURR_MODE_OFF));
> > +
> > +	/* 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");
> 
> Failed to set zero initial current levels
> 
More descriptive. Thanks.
> > +	return 0;
> > +}
> > +
> > +static void as3668_remove(struct i2c_client *client)
> > +{
> > +	int err;
> > +
> > +	err = i2c_smbus_write_byte_data(client, AS3668_CURR_MODE_REG, 0);
> > +	if (err)
> > +		dev_err(&client->dev, "failed to turn off the LEDs\n");
> 
> This is probably not useful to the user.
> 
> Just make an attempt to turn them off, then leave.
> 
Okay. That wasn't in v1, but I guess I went the other way and added to
much error handling now. I will leave it out in v11. 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.2
> > 
> 
> -- 
> Lee Jones [李琼斯]
> 
--
Best regards,
Lukas Timmermann

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

[Lukas Timmermann]

On Thu, Nov 20, 2025 at 01:31:49PM +0000, Lee Jones wrote:
> On Thu, 20 Nov 2025, Lukas Timmermann wrote:
> 
> > On Mon, Nov 17, 2025 at 03:00:08AM +0100, 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 | 222 +++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 237 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@vger.kernel.org
> > >  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@intel.com>
> > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > index a104cbb0a001..ec37d55ac14e 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_OSRAM_AMS_AS3668
> > I've modified this line as requested in patch series v9. After comparing
> > this with other configuration options in drivers/leds/Kconfig, this
> > seems out of place. Shouldn't we keep this consistent?
> 
> There are a few other examples in there, so consistency is less of an
> issue.  I personally think it provides a better picture of the device if
> the manufacture is mentioned as well.
> 
> But this is not a blocking point.  Take your preference.
> 
> -- 
> Lee Jones [李琼斯]
> 
I agree that including the manufacturer is more descriptive. Thanks for
explaining that to me, I didn't notice the other entries with
manufacturers. Also thanks for the continued patience with my newbie patches.

Best regards,
Lukas Timmermann

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

[Lee Jones]

On Fri, 21 Nov 2025, Lukas Timmermann wrote:

> On Thu, Nov 20, 2025 at 12:07:04PM +0000, Lee Jones wrote:
> > On Mon, 17 Nov 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 | 222 +++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 237 insertions(+)
> > >  create mode 100644 drivers/leds/leds-as3668.c

[...] 

> > > +	if (reg < 0) {
> > > +		dev_err(&as3668->client->dev, "failed to read channel modes\n");
> > > +		return;
> > > +	}
> > > +
> > > +	switch (led_id) {
> > > +	case 0:
> > > +		reg &= ~AS3668_CURR1_MODE_MASK;
> > > +		reg |= FIELD_PREP(AS3668_CURR1_MODE_MASK, mode);
> > > +		break;
> > > +	case 1:
> > > +		reg &= ~AS3668_CURR2_MODE_MASK;
> > > +		reg |= FIELD_PREP(AS3668_CURR2_MODE_MASK, mode);
> > > +		break;
> > > +	case 2:
> > > +		reg &= ~AS3668_CURR3_MODE_MASK;
> > > +		reg |= FIELD_PREP(AS3668_CURR3_MODE_MASK, mode);
> > > +		break;
> > > +	case 3:
> > > +		reg &= ~AS3668_CURR4_MODE_MASK;
> > > +		reg |= FIELD_PREP(AS3668_CURR4_MODE_MASK, mode);
> > > +		break;
> > > +	default:
> > > +		return;
> > > +	}
> > > +
> > > +	err = i2c_smbus_write_byte_data(as3668->client, AS3668_CURR_MODE_REG, reg);
> > 
> > Either it's an error or it's not.  Why isn't it being propagated?
> > 
> My patch had a dev_err() call here. It's missing in your citation.
> Was using dev_err() here wrong?

Yes.  It's preferable that you propagate (return) the error.

If you don't do that, it's just a warning.

-- 
Lee Jones [李琼斯]