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

Cássio Gabriel Monteiro Pires
2026-04-11T17:29:57Z

[-- Attachment #1.1: Type: text/plain, Size: 3870 bytes --]

On 4/11/26 13:17, Takashi Iwai wrote:
> 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,Takashi Iwai <tiwai@suse.com>
>>> 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.
>

Yes, seems to be a better solution.
I'll send a v2 patch with the proposed changes.

--
Thanks,
Cássio

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]