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 | 202 +++++++++++++++++++++++++++++++++++++
4 files changed, 217 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..0cfd3b68f90c
--- /dev/null
+++ b/drivers/leds/leds-as3668.c
@@ -0,0 +1,202 @@
+// 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_ID2_REG 0x3f
+#define AS3668_CHIP_ID1_EXPECTED_IDENTIFIER 0xa5
+#define AS3668_CHIP_ID2_SERIAL_MASK GENMASK(7, 4)
+#define AS3668_CHIP_ID2_REV_MASK GENMASK(3, 0)
+
+/* 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",
+ 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", ®);
+ if (err)
+ return dev_err_probe(dev, err, "'reg' property missing from %s\n",
+ 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",
+ 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_ident, chip_subident, chip_serial, chip_rev;
+
+ /* Check for sensible i2c address */
+ if (client->addr != AS3668_EXPECTED_I2C_ADDR)
+ 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 */
+ chip_ident = i2c_smbus_read_byte_data(client, AS3668_CHIP_ID1_REG);
+
+ if (chip_ident != AS3668_CHIP_ID1_EXPECTED_IDENTIFIER)
+ return dev_err_probe(&client->dev, -ENODEV,
+ "expected chip identifier 0x%02x, got 0x%02x\n",
+ AS3668_CHIP_ID1_EXPECTED_IDENTIFIER, chip_ident);
+
+ chip_subident = i2c_smbus_read_byte_data(client, AS3668_CHIP_ID2_REG);
+ chip_serial = FIELD_GET(AS3668_CHIP_ID2_SERIAL_MASK, chip_subident);
+ chip_rev = FIELD_GET(AS3668_CHIP_ID2_REV_MASK, chip_subident);
+
+ /* Print out information about the chip */
+ dev_dbg(&client->dev,
+ "chip_ident: 0x%02x | chip_subident: 0x%02x | chip_serial: 0x%02x | chip_rev: 0x%02x\n",
+ chip_ident, chip_subident, 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 err;
+
+ /* Set all four channel modes to 'on' */
+ 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, "error during hardware initialization\n");
+
+ return 0;
+}
+
+static void as3668_remove(struct i2c_client *client)
+{
+ int err = i2c_smbus_write_byte_data(client, AS3668_CURRX_CONTROL_REG, 0);
+
+ if (err)
+ dev_err(&client->dev, "couldn't deinit device\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.50.1On Fri, 08 Aug 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 | 202 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 217 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..0cfd3b68f90c
> --- /dev/null
> +++ b/drivers/leds/leds-as3668.c
> @@ -0,0 +1,202 @@
> +// 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
Can you tab out all of the values please.
> +#define AS3668_CHIP_ID2_REG 0x3f
> +#define AS3668_CHIP_ID1_EXPECTED_IDENTIFIER 0xa5
This is odd. What do you mean by expected?
What kind of ID is this? Board ID, platform ID, Chip ID?
Call it that instead.
> +#define AS3668_CHIP_ID2_SERIAL_MASK GENMASK(7, 4)
> +#define AS3668_CHIP_ID2_REV_MASK GENMASK(3, 0)
> +
> +/* Current Control */
> +
The X thing (below) is weirding me out.
> +#define AS3668_CURRX_CONTROL_REG 0x01
Drop the X.
> +#define AS3668_CURR1_REG 0x02
> +#define AS3668_CURR2_REG 0x03
> +#define AS3668_CURR3_REG 0x04
> +#define AS3668_CURR4_REG 0x05
Are these not also a 'CONTROL' regs?
If not, what kind of regs are they?
> +#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)
Drop the CURRX from each of these?
> +
> +struct as3668_led {
> + struct led_classdev cdev;
> + struct as3668 *chip;
> + struct fwnode_handle *fwnode;
> +
The new line seems unnecessary.
> + 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",
> + 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", ®);
> + if (err)
> + return dev_err_probe(dev, err, "'reg' property missing from %s\n",
> + 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",
> + 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_ident, chip_subident, chip_serial, chip_rev;
> +
> + /* Check for sensible i2c address */
I2C
"sensible" probably isn't the correct term here.
Actually, do we really need this comment? What does it add?
> + if (client->addr != AS3668_EXPECTED_I2C_ADDR)
> + return dev_err_probe(&client->dev, -EFAULT,
> + "expected i2c address 0x%02x, got 0x%02x\n",
I2C
If we already know the I2C address - why is it being set elsewhere?
> + AS3668_EXPECTED_I2C_ADDR, client->addr);
> +
> + /* Read identifier from chip */
> + chip_ident = i2c_smbus_read_byte_data(client, AS3668_CHIP_ID1_REG);
> +
> + if (chip_ident != AS3668_CHIP_ID1_EXPECTED_IDENTIFIER)
> + return dev_err_probe(&client->dev, -ENODEV,
> + "expected chip identifier 0x%02x, got 0x%02x\n",
> + AS3668_CHIP_ID1_EXPECTED_IDENTIFIER, chip_ident);
> +
> + chip_subident = i2c_smbus_read_byte_data(client, AS3668_CHIP_ID2_REG);
> + chip_serial = FIELD_GET(AS3668_CHIP_ID2_SERIAL_MASK, chip_subident);
> + chip_rev = FIELD_GET(AS3668_CHIP_ID2_REV_MASK, chip_subident);
> +
> + /* Print out information about the chip */
This is definitely superfluous.
> + dev_dbg(&client->dev,
> + "chip_ident: 0x%02x | chip_subident: 0x%02x | chip_serial: 0x%02x | chip_rev: 0x%02x\n",
> + chip_ident, chip_subident, chip_serial, chip_rev);
Does this have a role now that development is over?
Is the user going to care about all this stuff?
> +
> + 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' */
> + 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, "error during hardware initialization\n");
"Failed to write to the device"?
> +
> + return 0;
> +}
> +
> +static void as3668_remove(struct i2c_client *client)
> +{
> + int err = i2c_smbus_write_byte_data(client, AS3668_CURRX_CONTROL_REG, 0);
Do this after declaration please.
> +
> + if (err)
> + dev_err(&client->dev, "couldn't deinit device\n");
"deinit" is not a word.
Please expand slang and shortened words in comments and user-facing messages.
> +}
> +
> +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.1
>
>
--
Lee Jones [李琼斯]
On Tue, 02 Sep 2025, Lee Jones wrote:> On Fri, 08 Aug 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 | 202 +++++++++++++++++++++++++++++++++++++
>> 4 files changed, 217 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..0cfd3b68f90c
>> --- /dev/null
>> +++ b/drivers/leds/leds-as3668.c
>> @@ -0,0 +1,202 @@
>> +// 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
>
> Can you tab out all of the values please.
>
>> +#define AS3668_CHIP_ID2_REG 0x3f
>> +#define AS3668_CHIP_ID1_EXPECTED_IDENTIFIER 0xa5
>
> This is odd. What do you mean by expected?
>
> What kind of ID is this? Board ID, platform ID, Chip ID?
>
> Call it that instead.
Calling it just AS3668_CHIP_ID1 then?
It's the identifier of the chip model burned into silicon in the
CHIP_ID1 register. Checking it isn't critical in the first place.
It catches errors made in DT files but nothing else. You haven't
commented about that so i guess it's okay. Are drivers in the led
subsystem supposed to check that?>
>> +#define AS3668_CHIP_ID2_SERIAL_MASK GENMASK(7, 4)
>> +#define AS3668_CHIP_ID2_REV_MASK GENMASK(3, 0)
>> +
>> +/* Current Control */
>> +
>
> The X thing (below) is weirding me out.
>
>> +#define AS3668_CURRX_CONTROL_REG 0x01
>
> Drop the X.
The datasheet explictly calls this "CurrX control". It configures the
outputs for different modes like off/on and pwm or pattern generation
for all four channels simultaneously.
I could imagine AS3668_MODE_REG being more descriptive but we would
diverge from the datasheet. Is that acceptable?>
>> +#define AS3668_CURR1_REG 0x02
>> +#define AS3668_CURR2_REG 0x03
>> +#define AS3668_CURR3_REG 0x04
>> +#define AS3668_CURR4_REG 0x05
>
> Are these not also a 'CONTROL' regs?
>
> If not, what kind of regs are they?
>
>> +#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)
>
> Drop the CURRX from each of these?
Makes it more descriptive, I agree.
Wouldn't AS3668_MODE_CH1_MASK be even better?
But the datasheet calls this "curr1_mode" and so on.
As stated above, we would diverge from the datasheet.>
>> +
>> +struct as3668_led {
>> + struct led_classdev cdev;
>> + struct as3668 *chip;
>> + struct fwnode_handle *fwnode;
>> +
>
> The new line seems unnecessary.
>
>> + 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",
>> + 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", ®);
>> + if (err)
>> + return dev_err_probe(dev, err, "'reg' property missing from %s\n",
>> + 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",
>> + 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_ident, chip_subident, chip_serial, chip_rev;
>> +
>> + /* Check for sensible i2c address */
>
> I2C
>
> "sensible" probably isn't the correct term here.
>
> Actually, do we really need this comment? What does it add?
>
>> + if (client->addr != AS3668_EXPECTED_I2C_ADDR)
>> + return dev_err_probe(&client->dev, -EFAULT,
>> + "expected i2c address 0x%02x, got 0x%02x\n",
>
> I2C
>
> If we already know the I2C address - why is it being set elsewhere?
>
>> + AS3668_EXPECTED_I2C_ADDR, client->addr);
>> +
>> + /* Read identifier from chip */
>> + chip_ident = i2c_smbus_read_byte_data(client, AS3668_CHIP_ID1_REG);
>> +
>> + if (chip_ident != AS3668_CHIP_ID1_EXPECTED_IDENTIFIER)
>> + return dev_err_probe(&client->dev, -ENODEV,
>> + "expected chip identifier 0x%02x, got 0x%02x\n",
>> + AS3668_CHIP_ID1_EXPECTED_IDENTIFIER, chip_ident);
>> +
>> + chip_subident = i2c_smbus_read_byte_data(client, AS3668_CHIP_ID2_REG);
>> + chip_serial = FIELD_GET(AS3668_CHIP_ID2_SERIAL_MASK, chip_subident);
>> + chip_rev = FIELD_GET(AS3668_CHIP_ID2_REV_MASK, chip_subident);
>> +
>> + /* Print out information about the chip */
>
> This is definitely superfluous.
>
>> + dev_dbg(&client->dev,
>> + "chip_ident: 0x%02x | chip_subident: 0x%02x | chip_serial: 0x%02x | chip_rev: 0x%02x\n",
>> + chip_ident, chip_subident, chip_serial, chip_rev);
>
> Does this have a role now that development is over?
>
> Is the user going to care about all this stuff?
>
>> +
>> + 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' */
>> + 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, "error during hardware initialization\n");
>
> "Failed to write to the device"?
>
>> +
>> + return 0;
>> +}
>> +
>> +static void as3668_remove(struct i2c_client *client)
>> +{
>> + int err = i2c_smbus_write_byte_data(client, AS3668_CURRX_CONTROL_REG, 0);
>
> Do this after declaration please.
>
>> +
>> + if (err)
>> + dev_err(&client->dev, "couldn't deinit device\n");
>
> "deinit" is not a word.
>
> Please expand slang and shortened words in comments and user-facing messages.
>
>> +}
>> +
>> +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.1
>>
>>
>
On Tue, 02 Sep 2025, Lukas Timmermann wrote: > On Tue, 02 Sep 2025, Lee Jones wrote:> On Fri, 08 Aug 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 | 202 +++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 217 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..0cfd3b68f90c > > > --- /dev/null > > > +++ b/drivers/leds/leds-as3668.c > > > @@ -0,0 +1,202 @@ > > > +// 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 > > > > Can you tab out all of the values please. > > > > > +#define AS3668_CHIP_ID2_REG 0x3f > > > +#define AS3668_CHIP_ID1_EXPECTED_IDENTIFIER 0xa5 > > > > This is odd. What do you mean by expected? > > > > What kind of ID is this? Board ID, platform ID, Chip ID? > > > > Call it that instead. > Calling it just AS3668_CHIP_ID1 then? > It's the identifier of the chip model burned into silicon in the CHIP_ID1 > register. Checking it isn't critical in the first place. > It catches errors made in DT files but nothing else. You haven't commented > about that so i guess it's okay. Are drivers in the led subsystem supposed > to check that?> CHIP_ID1 is the register, but what does the number signify? What version of the chip? Does the chip have a name? What's the difference between the values in ID1 and ID2? > > > +#define AS3668_CHIP_ID2_SERIAL_MASK GENMASK(7, 4) > > > +#define AS3668_CHIP_ID2_REV_MASK GENMASK(3, 0) > > > + > > > +/* Current Control */ > > > + > > > > The X thing (below) is weirding me out. > > > > > +#define AS3668_CURRX_CONTROL_REG 0x01 > > > > Drop the X. > The datasheet explictly calls this "CurrX control". It configures the > outputs for different modes like off/on and pwm or pattern generation for > all four channels simultaneously. > I could imagine AS3668_MODE_REG being more descriptive but we would diverge > from the datasheet. Is that acceptable?> It is if you say it is. Some authors like to stick to the datasheet, which I would respect. Others think that datasheet authors / register namers are bonkers and the S/W should be much more friendly to readers. -- Lee Jones [李琼斯]
I'm sorry for the last message. It was sent incorrectly. Here is a fixed version. On Tue, 02 Sep 2025, Lee Jones wrote: > On Tue, 02 Sep 2025, Lukas Timmermann wrote: > >> On Tue, 02 Sep 2025, Lee Jones wrote:> On Fri, 08 Aug 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 | 202 +++++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 217 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..0cfd3b68f90c >>>> --- /dev/null >>>> +++ b/drivers/leds/leds-as3668.c >>>> @@ -0,0 +1,202 @@ >>>> +// 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 >>> >>> Can you tab out all of the values please. >>> >>>> +#define AS3668_CHIP_ID2_REG 0x3f >>>> +#define AS3668_CHIP_ID1_EXPECTED_IDENTIFIER 0xa5 >>> >>> This is odd. What do you mean by expected? >>> >>> What kind of ID is this? Board ID, platform ID, Chip ID? >>> >>> Call it that instead. >> Calling it just AS3668_CHIP_ID1 then? >> It's the identifier of the chip model burned into silicon in the CHIP_ID1 >> register. Checking it isn't critical in the first place. >> It catches errors made in DT files but nothing else. You haven't commented >> about that so i guess it's okay. Are drivers in the led subsystem supposed >> to check that?> > > CHIP_ID1 is the register, but what does the number signify? > > What version of the chip? Does the chip have a name? > > What's the difference between the values in ID1 and ID2? > CHIP_ID1 is a register which contains a fixed magic number so a driver can check if it's talking to the correct chip. There is no deeper meaning to the number as far as the datasheet is concerned. We simply check the number. It's supposed to be 10100101b according to the datasheet. CHIP_ID2 contains two fixed values. The upper four bits indicate a serial number of some sorts. The datasheet doesn't specify an expected value and doesn't state it's purpose. I suspect it to change between production runs. The lower four bits of the CHIP_ID2 register contains the revision of the chip. None of these values are particulary important for the function of the driver. My suggested driver does check CHIP_ID1 just to fail early. >>>> +#define AS3668_CHIP_ID2_SERIAL_MASK GENMASK(7, 4) >>>> +#define AS3668_CHIP_ID2_REV_MASK GENMASK(3, 0) >>>> + >>>> +/* Current Control */ >>>> + >>> >>> The X thing (below) is weirding me out. >>> >>>> +#define AS3668_CURRX_CONTROL_REG 0x01 >>> >>> Drop the X. >> The datasheet explictly calls this "CurrX control". It configures the >> outputs for different modes like off/on and pwm or pattern generation for >> all four channels simultaneously. >> I could imagine AS3668_MODE_REG being more descriptive but we would diverge >> from the datasheet. Is that acceptable?> > > It is if you say it is. Some authors like to stick to the datasheet, > which I would respect. Others think that datasheet authors / register > namers are bonkers and the S/W should be much more friendly to readers. > All the register names are taken from the datasheet. I considered renaming them but I think it would do more harm than good as the datasheet is publicly available and could be helpful for future changes.
© 2016 - 2025 Red Hat, Inc.