From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A350E32A3E5; Thu, 23 Oct 2025 14:18:49 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1761229129; cv=none; b=e7UKnl9nmFfNgwa2qQW0Ztym/WeeAZydlVAil20mJsBm1ij3frAjx7HUrtA76gFTTPVPi6y/ocv7Fj77XPwod/hImlIPYBnxBa3/Z+9HkxVh2y10dkPul0i64Uk4b0XwYnCCk2HBrzhXufDb/2devdZ8DhsnI1cm+RR3pXvtFSw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1761229129; c=relaxed/simple; bh=sxjYuJAOiSgP8WWUNYz5Ch+weQq8LvobXN7bvmNfSNI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=lAou+Lb4BiZAJSyJn1QCbnlS0qBGdOSk1oA3Va8jCidaFavEiY4HzTKImQu1mfhTiJcwvnu/CEDXGYmPORDgwl+22iJ/f5e50jVzs9qDEsJyieTmE8YZ6MPr+OsgdfCjf8zr9+9unHyiZKQiIKc/XPuG8uFpVlhLlMK/JpLZdzQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=JcJSoOdh; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="JcJSoOdh" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 81A44C4CEE7; Thu, 23 Oct 2025 14:18:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1761229129; bh=sxjYuJAOiSgP8WWUNYz5Ch+weQq8LvobXN7bvmNfSNI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=JcJSoOdh0Vx1za9H9FvdB2ekZo4wxoUh4/x8cs0uAPN/jDiXq+cpqupAsz0BmXkj3 b3m6AyW5SnjYJVMIKp3I/fgpm4KfcMG6yJob19yEnuA+Dmn5CYQfHnwzbFGxOAXQwF RlEgz+foQ8WmE/+3gjOpF0OzW2J7bQ2uuiVaazRSz4AqZv/Nkp6gSYRXpGT646uwY3 yoQyTPCBIFoeCez7ju6hyuj/R3xPuW6CadMdWNtoPFAK/hWiBXRdXEzz7JIJsD+VnX ggy6arGsFrCKGe6OzbH8tJGbhX5gtfoZXDckcvJiOWS1f1jnnEMUP2yIPQsTP/Nqnf S4Pjx5NcV2r1g== Date: Thu, 23 Oct 2025 15:18:44 +0100 From: Lee Jones To: Lukas Timmermann Cc: pavel@kernel.org, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, linux-leds@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v9 2/2] leds: as3668: Driver for the ams Osram 4-channel i2c LED driver Message-ID: <20251023141844.GP475031@google.com> References: <20251014152604.852487-1-linux@timmermann.space> <20251014152604.852487-3-linux@timmermann.space> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20251014152604.852487-3-linux@timmermann.space> On Tue, 14 Oct 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 > --- > MAINTAINERS | 1 + > drivers/leds/Kconfig | 13 +++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-as3668.c | 188 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 203 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 > 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 > 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 LEDS_OSRAM_AMS_AS3668 > + tristate "LED support for AMS AS3668" "Osram" > + depends on LEDS_CLASS > + depends on I2C > + help > + This option enables support for the AMS AS3668 LED controller. "Osram" > + 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..2b7b776fe2f5 > --- /dev/null > +++ b/drivers/leds/leds-as3668.c > @@ -0,0 +1,188 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Osram AMS AS3668 LED Driver IC > + * > + * Copyright (C) 2025 Lukas Timmermann > + */ > + > +#include > +#include > +#include > +#include > +#include > + > +#define AS3668_MAX_LEDS 4 > +#define AS3668_EXPECTED_I2C_ADDR 0x42 > + > +/* Chip Ident */ > + > +#define AS3668_CHIP_ID1_REG 0x3e > +#define AS3668_CHIP_ID 0xa5 > + > +/* 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", The user isn't going to care about this stuff. "Failed to set brightness: %d" > + 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", "Failed to read 'reg' property" > + 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", "Unsupported LED: %d" > + 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_id; > + > + if (client->addr != AS3668_EXPECTED_I2C_ADDR) Expected is weird. Why are we trying to catch-out the consumer? If you already know what the I2C address is, just use that. > + 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 */ This comment is superfluous IMHO. The register name should tell us everything. > + chip_id = i2c_smbus_read_byte_data(client, AS3668_CHIP_ID1_REG); > + Remove this line. > + if (chip_id != AS3668_CHIP_ID) > + return dev_err_probe(&client->dev, -ENODEV, > + "expected chip id 0x%02x, got 0x%02x\n", "ID" > + 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 'on' */ Even if a specific LED wasn't requested? Are you sure that this doesn't have any drawbacks (power perhaps)? > + 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, "failed to write to the device\n"); > + > + return 0; > +} > + > +static void as3668_remove(struct i2c_client *client) > +{ > + int err; '\n' here. > + err = i2c_smbus_write_byte_data(client, AS3668_CURRX_CONTROL_REG, 0); > + if (err) > + dev_err(&client->dev, "couldn't remove the device\n"); This does not remove the device. "Failed to turn off the LEDs" > +} > + > +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 "); > +MODULE_DESCRIPTION("AS3668 LED driver"); > +MODULE_LICENSE("GPL"); > -- > 2.51.0 > -- Lee Jones [李琼斯]