linux-kernel - Re: [PATCH 4/4] leds: add driver for synology atmega1608 controlled LEDs

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aSNnx6LNA/zMO4Gw@duo.ucw.cz>
Date: Sun, 23 Nov 2025 21:00:07 +0100
From: Pavel Machek <pavel@....cz>
To: Markus Probst <markus.probst@...teo.de>
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

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.


> +++ 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?

"LED Support for Synology devices"?

> +	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.

Thanks and best regards,
								Pavel
-- 
I don't work for Nazis and criminals, and neither should you.
Boycott Putin, Trump, Netanyahu and Musk!

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)