| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250723093108.GQ11056@google.com>
Date: Wed, 23 Jul 2025 10:31:08 +0100
From: Lee Jones <lee@...nel.org>
To: Lukas Timmermann <linux@...mermann.space>
Cc: pavel@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, linux-leds@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/2] leds: as3668: Driver for the ams Osram 4-channel
i2c LED driver
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@...mermann.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@...mermann.space>
> L: linux-leds@...r.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@...el.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@...mermann.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", ®);
> + 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@...mermann.space>");
> +MODULE_DESCRIPTION("AS3668 LED driver");
> +MODULE_LICENSE("GPL");
> --
> 2.50.0
>
--
Lee Jones [李琼斯]
Powered by blists - more mailing lists