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

[Cássio Gabriel Monteiro Pires]

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@xxxxxxxx>
>>> 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: OpenPGP_signature.asc
Description: OpenPGP digital signature