| 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: <20250902120944.GJ2163762@google.com> Date: Tue, 2 Sep 2025 13:09:44 +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 v8 2/2] leds: as3668: Driver for the ams Osram 4-channel i2c LED driver 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@...mermann.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@...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..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@...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 > > > +#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 [李琼斯]
Powered by blists - more mailing lists