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

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 3880 bytes --]

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@posteo.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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]