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

[PATCH v7 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.

Please note: This is my first suggested patch to the kernel. 
checkpatch.pl runs without warnings or errors. 
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 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                    | 195 ++++++++++++++++++
 5 files changed, 290 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/ams,as3668.yaml
 create mode 100644 drivers/leds/leds-as3668.c

-- 
2.50.0

[PATCH v7 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.50.0

[PATCH v7 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 | 195 +++++++++++++++++++++++++++++++++++++
 4 files changed, 210 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..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
+	tristate "LED support for AMS AS3668"
+	depends on LEDS_CLASS
+	depends on I2C
+	help
+	  This option enables support for the 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..fbf29a6d0296
--- /dev/null
+++ b/drivers/leds/leds-as3668.c
@@ -0,0 +1,195 @@
+// 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 Registers */
+#define AS3668_CHIP_ID1 0x3e
+#define AS3668_CHIP_ID2 0x3f
+
+#define AS3668_CHIP_ID2_SERIAL_MASK GENMASK(7, 4)
+#define AS3668_CHIP_ID2_REV_MASK GENMASK(3, 0)
+
+#define AS3668_CURRX_CONTROL 0x01
+#define AS3668_CURR1 0x02
+#define AS3668_CURR2 0x03
+#define AS3668_CURR3 0x04
+#define AS3668_CURR4 0x05
+
+/* Constants */
+#define AS3668_CHIP_IDENT 0xa5
+#define AS3668_CHIP_REV1 0x01
+
+struct as3668_led {
+	struct led_classdev cdev;
+	struct as3668 *chip;
+	struct fwnode_handle *fwnode;
+
+	int num;
+};
+
+struct as3668 {
+	struct i2c_client *client;
+	struct as3668_led leds[AS3668_MAX_LEDS];
+};
+
+static int as3668_read_value(struct i2c_client *client, u8 reg)
+{
+	return i2c_smbus_read_byte_data(client, reg);
+}
+
+static int as3668_write_value(struct i2c_client *client, u8 reg, u8 value)
+{
+	int err = i2c_smbus_write_byte_data(client, reg, value);
+
+	if (err)
+		dev_err(&client->dev, "error writing to reg 0x%02x, returned %d\n", reg, err);
+
+	return err;
+}
+
+static enum led_brightness as3668_brightness_get(struct led_classdev *cdev)
+{
+	struct as3668_led *led = container_of(cdev, struct as3668_led, cdev);
+
+	return as3668_read_value(led->chip->client, AS3668_CURR1 + led->num);
+}
+
+static void as3668_brightness_set(struct led_classdev *cdev, enum led_brightness brightness)
+{
+	struct as3668_led *led = container_of(cdev, struct as3668_led, cdev);
+
+	as3668_write_value(led->chip->client, AS3668_CURR1 + led->num, brightness);
+}
+
+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, "unable to read device tree led reg\n");
+
+		if (reg < 0 || reg > AS3668_MAX_LEDS)
+			return dev_err_probe(dev, -EOPNOTSUPP, "unsupported led reg %d\n", reg);
+
+		led = &as3668->leds[reg];
+		led->fwnode = of_fwnode_handle(child);
+
+		led->num = 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 %d LED\n", reg);
+	}
+
+	return 0;
+}
+
+static int as3668_probe(struct i2c_client *client)
+{
+	int err;
+	u8 chip_id1, chip_id2, chip_serial, chip_rev;
+	struct as3668 *as3668;
+
+	/* Check for sensible i2c address */
+	if (client->addr != 0x42)
+		return dev_err_probe(&client->dev, -EFAULT,
+				     "unexpected address for as3668 device\n");
+
+	/* Read identifier from chip */
+	chip_id1 = as3668_read_value(client, AS3668_CHIP_ID1);
+
+	if (chip_id1 != AS3668_CHIP_IDENT)
+		return dev_err_probe(&client->dev, -ENODEV,
+				"chip reported wrong id: 0x%02x\n", chip_id1);
+
+	/* Check the revision */
+	chip_id2 = as3668_read_value(client, AS3668_CHIP_ID2);
+	chip_serial = FIELD_GET(AS3668_CHIP_ID2_SERIAL_MASK, chip_id2);
+	chip_rev = FIELD_GET(AS3668_CHIP_ID2_REV_MASK, chip_id2);
+
+	if (chip_rev != AS3668_CHIP_REV1)
+		dev_warn(&client->dev, "unexpected chip revision\n");
+
+	/* Print out information about the chip */
+	dev_dbg(&client->dev,
+		"chip_id: 0x%02x | chip_id2: 0x%02x | chip_serial: 0x%02x | chip_rev: 0x%02x\n",
+		chip_id1, chip_id2, chip_serial, chip_rev);
+
+	as3668 = devm_kzalloc(&client->dev, sizeof(*as3668), GFP_KERNEL);
+	if (!as3668)
+		return -ENOMEM;
+
+	as3668->client = client;
+	err = as3668_dt_init(as3668);
+	if (err)
+		return dev_err_probe(&client->dev, err, "failed to initialize device\n");
+
+	/* Initialize the chip */
+	as3668_write_value(client, AS3668_CURRX_CONTROL, 0x55);
+	as3668_write_value(client, AS3668_CURR1, 0x00);
+	as3668_write_value(client, AS3668_CURR2, 0x00);
+	as3668_write_value(client, AS3668_CURR3, 0x00);
+	as3668_write_value(client, AS3668_CURR4, 0x00);
+
+	return 0;
+}
+
+static void as3668_remove(struct i2c_client *client)
+{
+	as3668_write_value(client, AS3668_CURRX_CONTROL, 0x0);
+}
+
+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.50.0

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

[Lee Jones]

On Tue, 08 Jul 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 | 195 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 210 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..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
> +	tristate "LED support for AMS AS3668"
> +	depends on LEDS_CLASS
> +	depends on I2C
> +	help
> +	  This option enables support for the 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..fbf29a6d0296
> --- /dev/null
> +++ b/drivers/leds/leds-as3668.c
> @@ -0,0 +1,195 @@
> +// 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 Registers */

This should be obvious in the nomenclature.

> +#define AS3668_CHIP_ID1 0x3e

AS3668_CHIP_ID1_REG

> +#define AS3668_CHIP_ID2 0x3f
> +
> +#define AS3668_CHIP_ID2_SERIAL_MASK GENMASK(7, 4)
> +#define AS3668_CHIP_ID2_REV_MASK GENMASK(3, 0)
> +
> +#define AS3668_CURRX_CONTROL 0x01
> +#define AS3668_CURR1 0x02
> +#define AS3668_CURR2 0x03
> +#define AS3668_CURR3 0x04
> +#define AS3668_CURR4 0x05
> +
> +/* Constants */
> +#define AS3668_CHIP_IDENT 0xa5

What's the difference between ID and IDENT?

> +#define AS3668_CHIP_REV1 0x01

How many REVs can one chip have?

> +struct as3668_led {
> +	struct led_classdev cdev;
> +	struct as3668 *chip;
> +	struct fwnode_handle *fwnode;
> +
> +	int num;

We can do better than 'num'.

> +};
> +
> +struct as3668 {
> +	struct i2c_client *client;
> +	struct as3668_led leds[AS3668_MAX_LEDS];
> +};
> +
> +static int as3668_read_value(struct i2c_client *client, u8 reg)
> +{
> +	return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static int as3668_write_value(struct i2c_client *client, u8 reg, u8 value)
> +{
> +	int err = i2c_smbus_write_byte_data(client, reg, value);
> +
> +	if (err)
> +		dev_err(&client->dev, "error writing to reg 0x%02x, returned %d\n", reg, err);
> +
> +	return err;
> +}

These look like abstractions for the sake of abstractions.

Just use the i2c_smbus_*() calls directly.

> +static enum led_brightness as3668_brightness_get(struct led_classdev *cdev)
> +{
> +	struct as3668_led *led = container_of(cdev, struct as3668_led, cdev);
> +
> +	return as3668_read_value(led->chip->client, AS3668_CURR1 + led->num);
> +}
> +
> +static void as3668_brightness_set(struct led_classdev *cdev, enum led_brightness brightness)
> +{
> +	struct as3668_led *led = container_of(cdev, struct as3668_led, cdev);
> +
> +	as3668_write_value(led->chip->client, AS3668_CURR1 + led->num, brightness);
> +}
> +
> +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, "unable to read device tree led reg\n");

"'reg' property missing from %s\n", child->name

> +
> +		if (reg < 0 || reg > AS3668_MAX_LEDS)
> +			return dev_err_probe(dev, -EOPNOTSUPP, "unsupported led reg %d\n", reg);

"'reg' property in %s is out of scope: %d\n", child->name, reg

> +		led = &as3668->leds[reg];
> +		led->fwnode = of_fwnode_handle(child);
> +
> +		led->num = 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 %d LED\n", reg);

Swap "%d" and "LED"

> +	}
> +
> +	return 0;
> +}
> +
> +static int as3668_probe(struct i2c_client *client)
> +{
> +	int err;
> +	u8 chip_id1, chip_id2, chip_serial, chip_rev;
> +	struct as3668 *as3668;

Nit: structs at the top, then filter down in size order.

> +	/* Check for sensible i2c address */
> +	if (client->addr != 0x42)

No magic numbers - define this please.

> +		return dev_err_probe(&client->dev, -EFAULT,
> +				     "unexpected address for as3668 device\n");

Unwrap this - you can use up to 100-chars in LEDs.

"Expected I2C address %x, got %x", <DEFINE>, client->addr"

> +	/* Read identifier from chip */
> +	chip_id1 = as3668_read_value(client, AS3668_CHIP_ID1);
> +
> +	if (chip_id1 != AS3668_CHIP_IDENT)
> +		return dev_err_probe(&client->dev, -ENODEV,
> +				"chip reported wrong id: 0x%02x\n", chip_id1);

Unlikely.  This too is unexpected, as above.

> +	/* Check the revision */
> +	chip_id2 = as3668_read_value(client, AS3668_CHIP_ID2);

Is child_id2 not for another chip?

This is ambiguous, please improve the variable nomenclature.

> +	chip_serial = FIELD_GET(AS3668_CHIP_ID2_SERIAL_MASK, chip_id2);
> +	chip_rev = FIELD_GET(AS3668_CHIP_ID2_REV_MASK, chip_id2);
> +
> +	if (chip_rev != AS3668_CHIP_REV1)
> +		dev_warn(&client->dev, "unexpected chip revision\n");

Values please.

> +	/* Print out information about the chip */
> +	dev_dbg(&client->dev,
> +		"chip_id: 0x%02x | chip_id2: 0x%02x | chip_serial: 0x%02x | chip_rev: 0x%02x\n",
> +		chip_id1, chip_id2, chip_serial, chip_rev);
> +
> +	as3668 = devm_kzalloc(&client->dev, sizeof(*as3668), GFP_KERNEL);
> +	if (!as3668)
> +		return -ENOMEM;
> +
> +	as3668->client = client;

\n

> +	err = as3668_dt_init(as3668);
> +	if (err)
> +		return dev_err_probe(&client->dev, err, "failed to initialize device\n");

No need for 2 error messages.

> +	/* Initialize the chip */
> +	as3668_write_value(client, AS3668_CURRX_CONTROL, 0x55);

No magic numbers.

> +	as3668_write_value(client, AS3668_CURR1, 0x00);
> +	as3668_write_value(client, AS3668_CURR2, 0x00);
> +	as3668_write_value(client, AS3668_CURR3, 0x00);
> +	as3668_write_value(client, AS3668_CURR4, 0x00);
> +
> +	return 0;
> +}
> +
> +static void as3668_remove(struct i2c_client *client)
> +{
> +	as3668_write_value(client, AS3668_CURRX_CONTROL, 0x0);
> +}
> +
> +static const struct i2c_device_id as3668_idtable[] = {
> +	{"as3668"},

Spaces after the '{' and before the '}'.

> +	{}
> +};
> +

Remove this line.

> +MODULE_DEVICE_TABLE(i2c, as3668_idtable);
> +
> +static const struct of_device_id as3668_match_table[] = {
> +	{.compatible = "ams,as3668"},

Spaces after the '{' and before the '}'.

> +	{}
> +};
> +

Remove this line.

> +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,

Remove all of the odd tabbing from in here.

A single space is fine.

> +};
> +

Remove.

> +module_i2c_driver(as3668_driver);
> +
> +MODULE_AUTHOR("Lukas Timmermann <linux@timmermann.space>");
> +MODULE_DESCRIPTION("AS3668 LED driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.50.0
> 

-- 
Lee Jones [李琼斯]

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

[Lukas Timmermann]

On Sun, 23 Jul 2025, Lee Jones wrote:
 >
 > How many REVs can one chip have?
 > Would be 4-bit/16. I thought I do a little check about the revision 
and print a warning message to inform about the probably untested 
revision. Or is that not necessary?
Removing the REV constant results in an if-statement similar to
if(rev == 1). Is this considered a magic number?>> +static int 
as3668_read_value(struct i2c_client *client, u8 reg)
 >> +{
 >> +	return i2c_smbus_read_byte_data(client, reg);
 >> +}
 >> +
 >> +static int as3668_write_value(struct i2c_client *client, u8 reg, u8 
value)
 >> +{
 >> +	int err = i2c_smbus_write_byte_data(client, reg, value);
 >> +
 >> +	if (err)
 >> +		dev_err(&client->dev, "error writing to reg 0x%02x, returned 
%d\n", reg, err);
 >> +
 >> +	return err;
 >> +}
 >
 > These look like abstractions for the sake of abstractions.
 >
 > Just use the i2c_smbus_*() calls directly.Should I omit the write 
function as well? I do some error handling there. Is it okay to err |= 
write() the returned error codes in init or should I handle every 
possible write error by itself?>> +	/* Read identifier from chip */
 >> +	chip_id1 = as3668_read_value(client, AS3668_CHIP_ID1);
 >> +
 >> +	if (chip_id1 != AS3668_CHIP_IDENT)
 >> +		return dev_err_probe(&client->dev, -ENODEV,
 >> +				"chip reported wrong id: 0x%02x\n", chip_id1);
 >
 > Unlikely.  This too is unexpected, as above.
 > Error message not descriptive, understood. Changing that to 
"unexpected ..." as above. Or am I misunderstanding and the check should 
be omitted entirely?>> +	/* Check the revision */
 >> +	chip_id2 = as3668_read_value(client, AS3668_CHIP_ID2);
 >
 > Is child_id2 not for another chip?
 >
 > This is ambiguous, please improve the variable nomenclature.
 > chip_id2 is directly related to the defined register CHIP_ID2 which 
name is taken from the datasheet of the AS3668. (Not sure if I'm allowed 
to link it)
Should we diverge from the datasheet in case of naming?
Or is only chip_id2 to be renamed, even tho it holds the values of 
CHIP_ID2? I would consider chip_ident for chip_id1 and chip_subident for 
chip_id2. chip_subident would break down into chip_rev and chip_serial.
Of course reading chip_id2 would be unnecessary if I omit checking the 
revision in the first place (see above).>> +	err = as3668_dt_init(as3668);
 >> +	if (err)
 >> +		return dev_err_probe(&client->dev, err, "failed to initialize 
device\n");
 >
 > No need for 2 error messages.
 > Doing if(error) return error; then...?

Thanks for the input. I'm willing to learn. :)

Lukas Timmermann

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

[Lukas Timmermann]

Formatting was malformed in the last message, sorry. Next try:

>> +#define AS3668_CHIP_REV1 0x01
> 
> How many REVs can one chip have?
> 
Would be 4-bit/16. I thought I do a little check about the revision and 
print a warning message to inform about the probably untested revision. 
Or is that not necessary?
Removing the REV constant results in an if-statement similar to
if(rev == 1). Is this considered a magic number?
>> +static int as3668_read_value(struct i2c_client *client, u8 reg)
>> +{
>> +	return i2c_smbus_read_byte_data(client, reg);
>> +}
>> +
>> +static int as3668_write_value(struct i2c_client *client, u8 reg, u8 value)
>> +{
>> +	int err = i2c_smbus_write_byte_data(client, reg, value);
>> +
>> +	if (err)
>> +		dev_err(&client->dev, "error writing to reg 0x%02x, returned %d\n", reg, err);
>> +
>> +	return err;
>> +}
> 
> These look like abstractions for the sake of abstractions.
> 
> Just use the i2c_smbus_*() calls directly.
> 
Should I omit the write function as well? I do some error handling 
there. Is it okay to err |= write() the returned error codes in init or 
should I handle every possible write error by itself?

>> +	/* Read identifier from chip */
>> +	chip_id1 = as3668_read_value(client, AS3668_CHIP_ID1);
>> +
>> +	if (chip_id1 != AS3668_CHIP_IDENT)
>> +		return dev_err_probe(&client->dev, -ENODEV,
>> +				"chip reported wrong id: 0x%02x\n", chip_id1);
> 
> Unlikely.  This too is unexpected, as above.
> 
Error message not descriptive, understood. Changing that to "unexpected 
..." as above. Or am I misunderstanding and the check should be omitted 
entirely?

>> +	/* Check the revision */
>> +	chip_id2 = as3668_read_value(client, AS3668_CHIP_ID2);
> 
> Is child_id2 not for another chip?
> 
> This is ambiguous, please improve the variable nomenclature.
> 
chip_id2 is directly related to the defined register CHIP_ID2 which name 
is taken from the datasheet of the AS3668. (Not sure if I'm allowed to 
link it)
Should we diverge from the datasheet in case of naming?
Or is only chip_id2 to be renamed, even tho it holds the values of 
CHIP_ID2? I would consider chip_ident for chip_id1 and chip_subident for 
chip_id2. chip_subident would break down into chip_rev and chip_serial.
Of course reading chip_id2 would be unnecessary if I omit checking the 
revision in the first place (see above).

>> +	err = as3668_dt_init(as3668);
>> +	if (err)
>> +		return dev_err_probe(&client->dev, err, "failed to initialize device\n");
> 
> No need for 2 error messages.
> 
Doing if(error) return error; then...?

Thanks for the input. I'm willing to learn. 🙂

Lukas Timmermann

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

[Lee Jones]

On Sun, 27 Jul 2025, Lukas Timmermann wrote:

> Formatting was malformed in the last message, sorry. Next try:
> 
> > > +#define AS3668_CHIP_REV1 0x01
> > 
> > How many REVs can one chip have?
> > 
> Would be 4-bit/16. I thought I do a little check about the revision and
> print a warning message to inform about the probably untested revision. Or
> is that not necessary?
> Removing the REV constant results in an if-statement similar to
> if(rev == 1). Is this considered a magic number?

I would omit this until there is another revision.

> > > +static int as3668_read_value(struct i2c_client *client, u8 reg)
> > > +{
> > > +	return i2c_smbus_read_byte_data(client, reg);
> > > +}
> > > +
> > > +static int as3668_write_value(struct i2c_client *client, u8 reg, u8 value)
> > > +{
> > > +	int err = i2c_smbus_write_byte_data(client, reg, value);
> > > +
> > > +	if (err)
> > > +		dev_err(&client->dev, "error writing to reg 0x%02x, returned %d\n", reg, err);
> > > +
> > > +	return err;
> > > +}
> > 
> > These look like abstractions for the sake of abstractions.
> > 
> > Just use the i2c_smbus_*() calls directly.
> > 
> Should I omit the write function as well? I do some error handling there. Is
> it okay to err |= write() the returned error codes in init or should I
> handle every possible write error by itself?

The handling in write() is standard error handling.

It doesn't justify another function.

> > > +	/* Read identifier from chip */
> > > +	chip_id1 = as3668_read_value(client, AS3668_CHIP_ID1);
> > > +
> > > +	if (chip_id1 != AS3668_CHIP_IDENT)
> > > +		return dev_err_probe(&client->dev, -ENODEV,
> > > +				"chip reported wrong id: 0x%02x\n", chip_id1);
> > 
> > Unlikely.  This too is unexpected, as above.
> > 
> Error message not descriptive, understood. Changing that to "unexpected ..."
> as above. Or am I misunderstanding and the check should be omitted entirely?

No, that's fine.

> > > +	/* Check the revision */
> > > +	chip_id2 = as3668_read_value(client, AS3668_CHIP_ID2);
> > 
> > Is child_id2 not for another chip?
> > 
> > This is ambiguous, please improve the variable nomenclature.
> > 
> chip_id2 is directly related to the defined register CHIP_ID2 which name is
> taken from the datasheet of the AS3668. (Not sure if I'm allowed to link it)
> Should we diverge from the datasheet in case of naming?
> Or is only chip_id2 to be renamed, even tho it holds the values of CHIP_ID2?
> I would consider chip_ident for chip_id1 and chip_subident for chip_id2.
> chip_subident would break down into chip_rev and chip_serial.
> Of course reading chip_id2 would be unnecessary if I omit checking the
> revision in the first place (see above).

I would encourage people to match up defines with the datasheet.

Variables should instead be easy to read / maintain.

> > > +	err = as3668_dt_init(as3668);
> > > +	if (err)
> > > +		return dev_err_probe(&client->dev, err, "failed to initialize device\n");
> > 
> > No need for 2 error messages.
> > 
> Doing if(error) return error; then...?

Right.

-- 
Lee Jones [李琼斯]