Re: [PATCH v16 2/2] platform: Add initial synology microp driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] StatusLedHandler::brightness_set fails to turn off the LED when brightness is set to 0.
- [Low] The driver depends on a non-existent Kconfig symbol RUST_SERIAL_DEV_BUS_ABSTRACTIONS.
--

commit b10df269f564d88c0451b8cff4f399129c6cc317
Author: Markus Probst <markus.probst@posteo.de>

platform: Add initial synology microp driver

This commit adds an initial Synology Microp driver in Rust. It targets the
microcontroller found in Synology NAS devices to control various LEDs.

> diff --git a/drivers/platform/synology_microp/Kconfig b/drivers/platform/synology_microp/Kconfig
> new file mode 100644
> index 0000000000000..8878cfb7bcddb
> --- /dev/null
> +++ b/drivers/platform/synology_microp/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config SYNOLOGY_MICROP
> +	tristate "Synology Microp driver"
> +	depends on LEDS_CLASS && LEDS_CLASS_MULTICOLOR
> +	depends on RUST_SERIAL_DEV_BUS_ABSTRACTIONS

[Severity: Low]
Is RUST_SERIAL_DEV_BUS_ABSTRACTIONS defined anywhere in the kernel tree?

Referencing a non-existent config symbol in "depends on" makes it impossible
to satisfy the dependency, leaving the driver completely unselectable during
configuration.

[ ... ]
> diff --git a/drivers/platform/synology_microp/led.rs b/drivers/platform/synology_microp/led.rs
> new file mode 100644
> index 0000000000000..ce57adfd307f2
> --- /dev/null
> +++ b/drivers/platform/synology_microp/led.rs
[ ... ]
> +    fn brightness_set<'bound>(
> +        &self,
> +        dev: &'bound Self::Bus,
> +        classdev: &led::MultiColorDevice<'bound, Self>,
> +        brightness: u32,
> +    ) -> Result<()> {
> +        let mut blink = self.blink.lock();
> +        if brightness == 0 {
> +            *blink = false;
> +        }
> +
> +        let amber_brightness = classdev.subleds()[1].brightness();
> +
> +        let (color, subled_brightness) = if amber_brightness == 0 {
> +            (StatusLedColor::Green, classdev.subleds()[0].brightness())
> +        } else {
> +            (StatusLedColor::Amber, amber_brightness)
> +        };
> +
> +        Command::StatusLed(
> +            color,
> +            if subled_brightness == 0 {

[Severity: Medium]
When brightness is 0, will subled_brightness actually be 0 here?

The LED subsystem does not automatically recalculate the subled brightness
before invoking this callback. If the subled retains its previous non-zero
value, will subled_brightness == 0 evaluate to false, causing the driver to
incorrectly send State::On to the hardware instead of State::Off?

> +                State::Off
> +            } else if *blink {
> +                State::Blink
> +            } else {
> +                State::On
> +            },
> +        )
> +        .write(dev)
> +    }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-synology_microp_initial-v16-0-601e91c49e0d@posteo.de?part=2