Re: [Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement

[Philippe Mathieu-Daudé]

On 02/08/2018 10:01 AM, Peter Maydell wrote:
> On 8 February 2018 at 12:15, Philippe Mathieu-Daudé <address@hidden> wrote:
>> Hi Daniel,
>>
>> On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote:
>>> This patch adds a break in the switch() statement of complete(),
>>> value 0x42:
>>>
>>>     case 0x42:              /* FT2 sets output freq with this, go figure */
>>>         qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
>>>                       " should\n");
>>>         break; <-------
>>>     case 0x41:
>>
>> It seems this is an intentional fallthrough, I understand cmd 0x42 is
>> expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415).
> 
> Yes, I agree; I wrote a bit about this in this thread:
> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02081.html

Oh, very useful link!

> (though my guess is that actually 0x42 is supposed to do exactly
> what 0x41 does, and that the LOG_UNIMP should maybe just be removed).

I now understand 0x42 sets the dsp input sampling freq, the model seems
to be designed with output in mind, then added input support (using same
freq as output).

So imho the simpler/safer fix would be:

  case 0x42:
      if (dsp_get_hilo(s) != s->freq) {
          qemu_log_mask(LOG_UNIMP,
                        "input sampling freq different than "
                        "output not implemented");
      }
      /* fallthrough */
  case 0x41:
      ...

and the correct fix would be split s->freq in {s->freq_in, s->freq_out}
but nobody ever required this during at least 14 years.