From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sylwester Nawrocki Date: Mon, 08 Apr 2013 15:20:35 +0000 Subject: Re: [PATCH v6 1/7] media: V4L2: add temporary clock helpers Message-Id: <5162E043.9050103@samsung.com> List-Id: References: <1363382873-20077-1-git-send-email-g.liakhovetski@gmx.de> <1363382873-20077-2-git-send-email-g.liakhovetski@gmx.de> <5147934D.2040908@gmail.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Guennadi Liakhovetski Cc: Sylwester Nawrocki , linux-media@vger.kernel.org, Laurent Pinchart , Hans Verkuil , Sylwester Nawrocki , linux-sh@vger.kernel.org, Magnus Damm , Sakari Ailus , Prabhakar Lad On 04/08/2013 12:36 PM, Guennadi Liakhovetski wrote: > On Mon, 18 Mar 2013, Sylwester Nawrocki wrote: > > [snip] > >>> +unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk) >>> +{ >>> + if (!clk->ops->get_rate) >>> + return -ENOSYS; >> >> I guess we should just WARN if this callback is null and return 0 >> or return value type of this function needs to be 'long'. Otherwise >> we'll get insanely large frequency value by casting this error code >> to unsigned long. > > Comparing to the CCF: AFAICS, they do the same, you're supposed to use > IS_ERR_VALUE() on the clock rate, obtained from clk_get_rate(). Hmm, that might work. Nevertheless I consider that a pretty horrible pattern. I couldn't find any references to IS_ERR_VALUE in the clock code though. Only that 0 is returned when clk is NULL. Regards, Sylwester From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mailout1.w1.samsung.com ([210.118.77.11]:43075 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936123Ab3DHPUk (ORCPT ); Mon, 8 Apr 2013 11:20:40 -0400 Message-id: <5162E043.9050103@samsung.com> Date: Mon, 08 Apr 2013 17:20:35 +0200 From: Sylwester Nawrocki MIME-version: 1.0 To: Guennadi Liakhovetski Cc: Sylwester Nawrocki , linux-media@vger.kernel.org, Laurent Pinchart , Hans Verkuil , Sylwester Nawrocki , linux-sh@vger.kernel.org, Magnus Damm , Sakari Ailus , Prabhakar Lad Subject: Re: [PATCH v6 1/7] media: V4L2: add temporary clock helpers References: <1363382873-20077-1-git-send-email-g.liakhovetski@gmx.de> <1363382873-20077-2-git-send-email-g.liakhovetski@gmx.de> <5147934D.2040908@gmail.com> In-reply-to: Content-type: text/plain; charset=ISO-8859-1 Content-transfer-encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: On 04/08/2013 12:36 PM, Guennadi Liakhovetski wrote: > On Mon, 18 Mar 2013, Sylwester Nawrocki wrote: > > [snip] > >>> +unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk) >>> +{ >>> + if (!clk->ops->get_rate) >>> + return -ENOSYS; >> >> I guess we should just WARN if this callback is null and return 0 >> or return value type of this function needs to be 'long'. Otherwise >> we'll get insanely large frequency value by casting this error code >> to unsigned long. > > Comparing to the CCF: AFAICS, they do the same, you're supposed to use > IS_ERR_VALUE() on the clock rate, obtained from clk_get_rate(). Hmm, that might work. Nevertheless I consider that a pretty horrible pattern. I couldn't find any references to IS_ERR_VALUE in the clock code though. Only that 0 is returned when clk is NULL. Regards, Sylwester