Re: [PATCH 1/2] ALSA: sscape: Cache per-card resources for board reinitialization

Takashi Iwai
2026-04-11T16:17:25Z

On Sat, 11 Apr 2026 16:33:54 +0200,
Cássio Gabriel Monteiro Pires wrote:
>
>
>
> On 4/11/26 05:07, Takashi Iwai wrote:
> > On Sat, 11 Apr 2026 06:54:29 +0200,
> > Cássio Gabriel wrote:
> >> +/*
> >> + * Restore the SoundScape's MIDI control state after the firmware
> >> + * upload has made the host interface available again.
> >> + */
> >> +static int sscape_restore_midi_state(struct soundscape *sscape)
> >> +{
> >> + int err = 1;
> >> +
> >> + guard(spinlock_irqsave)(&sscape->lock);
> >> + set_host_mode_unsafe(sscape->io_base);
> >> + err &= host_write_ctrl_unsafe(sscape->io_base, CMD_SET_MIDI_VOL, 100);
> >> + err &= host_write_ctrl_unsafe(sscape->io_base, sscape->midi_vol, 100);
> >> + err &= host_write_ctrl_unsafe(sscape->io_base, CMD_XXX_MIDI_VOL, 100);
> >> + err &= host_write_ctrl_unsafe(sscape->io_base, sscape->midi_vol, 100);
> >> + err &= host_write_ctrl_unsafe(sscape->io_base, CMD_SET_EXTMIDI, 100);
> >> + err &= host_write_ctrl_unsafe(sscape->io_base, 0, 100);
> >> + err &= host_write_ctrl_unsafe(sscape->io_base, CMD_ACK, 100);
> >> + set_midi_mode_unsafe(sscape->io_base);
> >> +
> >> + return err ? 0 : -EIO;
> >
> > The above means it checks only the error of the last write.
> > Is it intentional?
>
> It was meant to accumulate failures across the whole sequence, since
> host_write_ctrl_unsafe() returns 0/1 and the accumulator starts at 1.
>
> So an earlier failed write is meant to keep the final result failed, not
> only the last write. But I agree the current form is kinda easy to misread.
>
> I can rewrite to make the cumulative error handling explicit:
>
> static int sscape_restore_midi_state(struct soundscape *sscape)
> {
> bool ok = true;
>
> guard(spinlock_irqsave)(&sscape->lock);
> set_host_mode_unsafe(sscape->io_base);
>
> if (!host_write_ctrl_unsafe(sscape->io_base, CMD_SET_MIDI_VOL, 100))
> ok = false;
> if (!host_write_ctrl_unsafe(sscape->io_base, sscape->midi_vol, 100))
> ok = false;
> if (!host_write_ctrl_unsafe(sscape->io_base, CMD_XXX_MIDI_VOL, 100))
> ok = false;
> if (!host_write_ctrl_unsafe(sscape->io_base, sscape->midi_vol, 100))
> ok = false;
> if (!host_write_ctrl_unsafe(sscape->io_base, CMD_SET_EXTMIDI, 100))
> ok = false;
> if (!host_write_ctrl_unsafe(sscape->io_base, 0, 100))
> ok = false;
> if (!host_write_ctrl_unsafe(sscape->io_base, CMD_ACK, 100))
> ok = false;
>
> set_midi_mode_unsafe(sscape->io_base);
>
> return ok ? 0 : -EIO;
> }
>
> What you think?

Ah, I see, one of confusing parts is the definition of
host_write_*_unsafe() functions. They return true for success, not an
error, while the callers use a variable "err". IMO, it'd be better to
change them from int to bool and write the return condition in the
function comments, and use a different variable name like you
suggested.

And, looking at the original sscape_midi_put(), it's rather like:

change = host_write_ctrl_unsafe(s->io_base, CMD_SET_MIDI_VOL, 100)
&& host_write_ctrl_unsafe(s->io_base, new_val, 100)
&& host_write_ctrl_unsafe(s->io_base, CMD_XXX_MIDI_VOL, 100)
&& host_write_ctrl_unsafe(s->io_base, new_val, 100);

which is more straightforward, and it aborts when a sequence fails.
(Though, strictly speaking, sscape_midi_put() should return an error
in this case, but it's a different topic.)
Maybe this form can be used instead, too.

thanks,

Takashi