| Message ID | 20250611083151.22150-1-linux@timmermann.space |
|---|---|
| Headers | show |
| Series | Support for Osram as3668 LED driver | expand |
Le 11/06/2025 à 10:31, Lukas Timmermann a écrit : > 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> Hi, first, I should that you should wait longer before sending each new version, so that you can collect more feedback. > --- > MAINTAINERS | 1 + > drivers/leds/Kconfig | 13 +++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-as3668.c | 204 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 219 insertions(+) > create mode 100644 drivers/leds/leds-as3668.c ... > +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) { > + dev_err(dev, "unable to read device tree led reg, err %d\n", err); as3668_dt_init() is only called from the probe. Sometimes maintainers prefer using "return dev_err_probe()" in such a case, to have less verbose code. (I don't know if it is the case for the leds subsystem) > + return err; > + } > + > + if (reg < 0 || reg > AS3668_MAX_LEDS) { > + dev_err(dev, "unsupported led reg %d\n", reg); > + return -EOPNOTSUPP; Same. > + } > + > + 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) { > + dev_err(dev, "failed to register %d LED\n", reg); > + return err; Same. > + } > + } > + > + return 0; > +} > + > +static int as3668_probe(struct i2c_client *client) > +{ > + 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); > + Unneeded new line. > + if (!as3668) > + return -ENOMEM; > + > + as3668->client = client; > + int err = as3668_dt_init(as3668); Would be better, IMHO, if err was declared at the top of the function. > + Unneeded new line. > + if (err) { > + dev_err(&client->dev, "failed to initialize device, err %d\n", err); return dev_err_probe() to be consistent with the code above. > + return err; > + } > + > + /* 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; > +} ... CJ
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 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 | 204 ++++++++++++++++++ 5 files changed, 299 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/ams,as3668.yaml create mode 100644 drivers/leds/leds-as3668.c