Re: [Qemu-devel] [PATCH 5/7] nios2: Add periodic timer emulation

[Dmitry Osipenko]

On 19.08.2016 02:24, Marek Vasut wrote:
> On 08/18/2016 11:49 AM, Dmitry Osipenko wrote:
>> On 17.08.2016 23:19, Marek Vasut wrote:
>>> On 08/16/2016 11:38 PM, Dmitry Osipenko wrote:
>>>
>>> [...]
>>>
>>>>>>> Well what is sane clock frequency for hardware which can have arbitrary
>>>>>>> frequency configured in ?
>>>>>>>
>>>>>>
>>>>>> You could set to the one that is used by "10M50 GHRD" patch for example.
>>>>>
>>>>> That doesn't sound right . I can set it to 1 (Hz) , that should make it
>>>>> slow enough to signalize that something is broken while providing valid
>>>>> number.
>>>>>
>>>>
>>>> I'm not sure about it. Explicitly showing that something is wrong would be 
>>>> better.
>>>>
>>>>> +static void altera_timer_realize(DeviceState *dev, Error **errp)
>>>>> +{
>>>>> +    AlteraTimer *t = ALTERA_TIMER(dev);
>>>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>>> +
>>>>> +    assert(t->freq_hz != 0);
>>>>
>>>> If you would prefer to keep error'ing out, then I can suggest to add some
>>>> verbose message instead of the assertion, like:
>>>>
>>>> if (!t->freq_hz) {
>>>>     error_setg(errp, "altera_timer: \"clock-frequency\" property " \
>>>>                      "must be provided");
>>>>     return;
>>>> }
>>>
>>> That's better, thanks.
>>>
>>> btw breaking strings is frowned upon in linux/u-boot as it makes it
>>> impossible to git grep for error message. Does the same apply to qemu?
>>>
>>
>> Actually, "altera_timer: " prefix isn't needed, as it should be already 
>> included
>> in the error message produced by by the error_setg(), so that string could be
>> squeezed into one line. And, of course, it could be rephrased more concisely.
> 
> Even better, prefix dropped. Thanks
> 
> So what about patches 1..5 ? Anything I should change there ?
> 

Unfortunately, I don't have a good sense of bad in those patches. I guess
maintainers are currently busy with a 2.7 release, and you are probably not the
only one in the review queue. Just wait for now, it could take a while.

-- 
Dmitry