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

[Takashi Iwai]

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