From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38457) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1baqnA-0005D4-5N for qemu-devel@nongnu.org; Fri, 19 Aug 2016 16:53:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1baqn6-0005kH-2r for qemu-devel@nongnu.org; Fri, 19 Aug 2016 16:53:27 -0400 Received: from mail-lf0-x243.google.com ([2a00:1450:4010:c07::243]:36746) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1baqn5-0005jo-R2 for qemu-devel@nongnu.org; Fri, 19 Aug 2016 16:53:24 -0400 Received: by mail-lf0-x243.google.com with SMTP id 33so3536647lfw.3 for ; Fri, 19 Aug 2016 13:53:23 -0700 (PDT) References: <1469708878-9453-1-git-send-email-marex@denx.de> <1469708878-9453-5-git-send-email-marex@denx.de> <8441a477-57c4-7deb-6531-98ab125cc3b3@gmail.com> <40332f61-6523-9c59-6c35-ed59aeb6fef4@denx.de> <7e76da71-3037-bb9b-dfaf-2146a0a465b0@denx.de> <4123941b-73a0-fec3-4af8-ada701832f48@gmail.com> <0f952ce0-e9f3-5335-4fcc-899cb6f70ad0@denx.de> <9227bfa4-a356-5960-5723-e06c4e0f3337@gmail.com> <1e89db9a-b440-cdf7-a000-e072a1789014@denx.de> From: Dmitry Osipenko Message-ID: <0702a4f9-3c22-92ea-82d9-8e8b3c4d75b5@gmail.com> Date: Fri, 19 Aug 2016 23:53:19 +0300 MIME-Version: 1.0 In-Reply-To: <1e89db9a-b440-cdf7-a000-e072a1789014@denx.de> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/7] nios2: Add periodic timer emulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marek Vasut , qemu-devel@nongnu.org Cc: Jeff Da Silva , Chris Wulff , Sandra Loosemore , Yves Vandervennet , Ley Foon Tan , Juro Bystricky 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