Message-ID: <9056176d97168a8a9506ccb46540721b1c30942a.camel@posteo.de>
Date: Sun, 23 Nov 2025 22:45:22 +0000
From: Markus Probst <markus.probst@...teo.de>
To: Pavel Machek <pavel@....cz>
Cc: Lee Jones <lee@...nel.org>, Danilo Krummrich <dakr@...nel.org>, Miguel
 Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Igor Korotin
	 <igor.korotin.linux@...il.com>, Lorenzo Stoakes
 <lorenzo.stoakes@...cle.com>,  Vlastimil Babka	 <vbabka@...e.cz>, "Liam R.
 Howlett" <Liam.Howlett@...cle.com>, Uladzislau Rezki	 <urezki@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	bjorn3_gh@...tonmail.com, Benno Lossin <lossin@...nel.org>, Andreas
 Hindborg	 <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross	 <tmgross@...ch.edu>, Daniel Almeida
 <daniel.almeida@...labora.com>, 	linux-leds@...r.kernel.org,
 rust-for-linux@...r.kernel.org, 	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] leds: add driver for synology atmega1608 controlled
 LEDs

On Sun, 2025-11-23 at 21:00 +0100, Pavel Machek wrote:
> Hi!
> 
> > The Atmega1608 is a microcontroller used by synology devices to control
> > leds via the i2c bus. It can handle up to 24 leds.
> 
> Ok, but driver is not really for atmega1608, but for whatever code is
> running there, right?
> 
> I would not mind that code being opensource.
> 
> I would not mind if we had standard interface to these so the next
> person can reuse the protocol.
Me neither. But Synology is a company which seems to like proprietary
software. They have there own Linux-based (4.4.x kernel) operating
system, which is proprietary. Those devices are not intended for
installing other operating systems and doing so will certainly void the
warranty, but that doesn't stop me from doing it anyway.
> 
> 
> > +++ b/drivers/leds/Kconfig
> > @@ -323,6 +323,15 @@ config LEDS_WRAP
> >  	help
> >  	  This option enables support for the PCEngines WRAP programmable LEDs.
> >  
> > +config LEDS_ATMEGA1608
> > +	tristate "LED Support for Atmega1608 used in Synology devices"
> 
> There's nothing Atmega specific, right?
Not that I am aware of. The driver interacts with code running on the
Atmega1608.

> 
> "LED Support for Synology devices"?
I don't think thats the only driver they use for controlling disk leds
(although it seems the only synology specific they have added to their
kernel. They also seem to use LP3943 for some of their models,
according to their kernel config, which is already in the upstream
kernel), so it could mislead users into thinking this driver will be
useful on every synology device.

But I agree the config naming could be better.

> 
> > +	depends on LEDS_CLASS
> > +	depends on I2C
> > +	depends on RUST
> > +	help
> > +	  This option enables support for the Atmega1608 microcontroller used
> > +	  as led controller in synology devices.
> 
> led->LED
> synology->Synology
> 
> > +++ b/drivers/leds/leds_atmega1608.rs
> > @@ -0,0 +1,377 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Led Driver for Synology devices using Atmega1608 as Led Controller.
> 
> Led->LED
> 
> > +//! Atmega1608 is a microcontroller from Microchip Technology, it is used
> > +//! as a led controller in synology nas devices.
> 
> led->LED
> 
> (Please fix globally).
> 
> > +//! # Limitations
> > +//!
> > +//! DIM0 (and DIM1) are shared across all leds, meaning if DIM0 is used by
> > +//! multiple leds, these multiple leds cannot have different brightness
> > +//! levels. The same does apply for the hardware accelerated blinking.
> > +//!
> > +//! In other words, for all 24 leds there can either only be one brightness other
> > +//! than 0 and 255, or one hardware accelerated blinking delay.
> > +//!
> > +//! Furthermore the off and on delay in hardware accelerated blinking cannot
> > +//! have different values and have to be equal. We solve this by calculating
> > +//! the average of those numbers and use it as delay for both. The delay
> > +//! cannot be larger than 765 ms (255*3).
> > +//!
> > +//! While hardware accelerated blinking is on, the led cycles between
> > +//! the current brightness for the mode and full brightness. Because of this
> > +//! behaviour, we hardcode the brightness value of 128 if hardware accelerated
> > +//! blinking is used.
> 
> Maybe we should just pretend that hw acceleration does not exist? Or
> pretend it can only do single blinking rate.
> 
> > +
> > +kernel::module_i2c_driver! {
> > +    type: Atmega1608LedDriver,
> > +    name: "atmega1608",
> > +    authors: ["Markus Probst <markus.probst@...teo.de>"],
> > +    description: "Led Driver for Synology devices using Atmega1608 as Led Controller",
> > +    license: "GPL v2",
> > +}
> 
> This should be called synologyLedDriver or something. We have driver
> for Thinkpad LEDs. We don't care what kind of hardware it runs on.
> 
> 
> > +impl LedHandler for Atmega1608Led {
> > +    const BLOCKING: bool = true;
> > +    const BLINK: bool = true;
> > +    const MAX_BRIGHTNESS: u32 = 255;
> > +
> > +    fn brightness_set(&self, brightness: u32) -> Result<()> {
> > +        let brightness = u8::try_from(brightness).unwrap_or(255);
> > +
> > +        let mode = self.update_mode(match brightness {
> > +            0 => Atmega1608LedMode::Off,
> > +            255 | 254 => Atmega1608LedMode::On,
> > +            _ => Atmega1608LedMode::Dim0,
> > +        })?;
> 
> Umm... so the hardware can only do on / off and dim. And dim is
> strange.
> 
> Certainly MAX_BRIGHTNESS should not be 255, but 1 (or 2 if you want to
> support dim mode).
> 
> One idea: Use hardware diming to have three levels. So MAX_BRIGHTNESS
> = 2. Then ignore hardware accelerated blinking. And you should have
> working and simpler driver.
Those LEDs are labeled "Disk 1", "Disk 2", "Disk 3", "Disk 4" ..., I
think blinking is more important here than brightness. The main purpose
of those leds is to show disk activity (blink) and issues (different
color). Pretending it can only do a single blinking rate seems to be
the way to go.

Another reason why brightness is less important here:

There are other leds: Status, Power, USB, Alert which are handled by a
different driver (communication through uart). I am also currently
working on supporting these (the driver would be in the drivers/mfd
tree as it takes care of other things too. I have a working prototyp,
but its far from being ready for the kernel).

Synology devices have a global brightness setting, having an impact on
all the leds mentioned above (not just via the atmega1608).
On my Synology NAS device (DS923+), its controlled via the '0x2e' i2c
data address (outside of the atmega1608 range) on the same i2c bus, but
that will propably differ per device (they have an xml config on each
device, for how to control the brightness). I think this should be
handled by userspace via i2c-dev (default is MAX brightness) and not by
the kernel.



Also I noticed, that there isn't a "per-disk led trigger", which did
actually suprise me. Just need to search "NAS" on the web and I find
enough devices (not just from Synology) having a per-disk led, which
proabably run Linux. There is "disk-activity", but thats for all the
disks. Is there another way for linking the disk activity of one
specific disk to a specific led that I missed? Otherwise I would write
(in C) a led trigger for this purpose.

Thanks
- Markus Probst

> 
> Thanks and best regards,
> 								Pavel

Powered by blists - more mailing lists