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

["Danilo Krummrich" <dakr@kernel.org>]

(Not a full review (let's work out the dependencies first), but there's one
thing that stood out to me.)

On Wed Oct 8, 2025 at 8:10 PM CEST, Markus Probst wrote:
> +struct Atmega1608Led {
> +    addr: Atmega1608LedAddress,
> +    id: Atmega1608LedId,
> +
> +    client: ARef<I2cClient>,
> +
> +    mode_lock: Arc<Mutex<()>>,

Mutex<()> raises an eyebrow, since a mutex that doesn't protect anything is
pointless. So, I assume it is protecting some data, but in an unsound way.

> +impl Atmega1608Led {
> +    fn update_mode(&self, mode: Atmega1608LedMode) -> Result<Atmega1608LedMode> {
> +        let _guard = self.mode_lock.lock();

What exactly does the mutex protect in the code below?

> +
> +        let mut current = self
> +            .client
> +            .read_byte_data(self.addr as u8)
> +            .inspect_err(|err| {
> +                dev_err!(
> +                    self.client.as_ref(),
> +                    "failed to read {:#2x}: {err:?}\n",
> +                    self.addr as u8
> +                );
> +            })?;
> +
> +        current =
> +            (current & !self.id.mask()) | (((mode as u8) << self.id.shift()) & self.id.mask());
> +
> +        self.client
> +            .write_byte_data(self.addr as u8, current)
> +            .inspect_err(|err| {
> +                dev_err!(
> +                    self.client.as_ref(),
> +                    "failed to write {:#2x}: {err:?}",
> +                    self.addr as u8
> +                );
> +            })?;
> +
> +        Ok(mode)
> +    }
> +}