On 02/08/2018 10:16 AM, Philippe Mathieu-Daudé wrote:
> On 02/08/2018 10:01 AM, Peter Maydell wrote:
>> On 8 February 2018 at 12:15, Philippe Mathieu-Daudé <f4...@amsat.org> 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).
Now I see Fabrice comment "FT2 sets output freq with this, go figure"
and agree with him.
I like to think this is a bug in Fast Tracker 2, so Peter suggestion
about using LOG_GUEST_ERROR here might be clever.
>
> 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.
>
