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. >>>>>>> For the IIC, I wonder what that'd look like -- probably >>>>>>> qemu_set_irq(parent, 0); ? >>>>>>> >>>>>> >>>>>> No, IRQ's would be "deasserted" by resetting CPUClass state, of which >>>>>> QEMU takes >>>>>> care itself. No action needed by the IIC. >>>>> >>>>> I see, thanks :) >>>>> >>>>> btw any chance someone can look at the other patches too ? >>>>> >>>>> >>>> >>>> >>> >>> >> >> > > -- Dmitry