From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Stultz Subject: Re: [PATCH V12 3/4] ptp: Added a clock driver for the IXP46x. Date: Wed, 23 Mar 2011 14:43:00 -0700 Message-ID: <1300916580.848.51.camel@work-vm> References: Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Richard Cochran Cc: linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, netdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, Alan Cox , Arnd Bergmann , Christoph Lameter , David Miller , Krzysztof Halasa , Peter Zijlstra , Rodolfo Giometti , Thomas Gleixner , Benjamin Herrenschmidt , Mike Frysinger , Paul Mackerras , Russell King List-Id: linux-api@vger.kernel.org On Mon, 2011-02-28 at 08:58 +0100, Richard Cochran wrote: > This patch adds a driver for the hardware time stamping unit found on the > IXP465. The basic clock operations and an external trigger are implemented. > > Signed-off-by: Richard Cochran > Acked-by: John Stultz My ack is still fine, but found a few more nits to consider on another pass... > +static int match(struct sk_buff *skb, u16 uid_hi, u32 uid_lo, u16 seq) [snip] > +static void do_rx_timestamp(struct port *port, struct sk_buff *skb) [snip] > +static u64 sys_time_read(struct ixp46x_ts_regs *regs) [snip] > +static void sys_time_write(struct ixp46x_ts_regs *regs, u64 ns) I know these are static, but these function names are sort of generic names, and make grep/cscoping for similar sounding functions a little noisy. Maybe add a ipx_ prefix just to be more obvious? thanks -john From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH V12 3/4] ptp: Added a clock driver for the IXP46x. From: John Stultz To: Richard Cochran In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Date: Wed, 23 Mar 2011 14:43:00 -0700 Message-ID: <1300916580.848.51.camel@work-vm> Mime-Version: 1.0 Cc: Rodolfo Giometti , Arnd Bergmann , Peter Zijlstra , linux-api@vger.kernel.org, devicetree-discuss@lists.ozlabs.org, linux-kernel@vger.kernel.org, Russell King , Paul Mackerras , Thomas Gleixner , Alan Cox , netdev@vger.kernel.org, Mike Frysinger , Christoph Lameter , linuxppc-dev@lists.ozlabs.org, David Miller , linux-arm-kernel@lists.infradead.org, Krzysztof Halasa List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Mon, 2011-02-28 at 08:58 +0100, Richard Cochran wrote: > This patch adds a driver for the hardware time stamping unit found on the > IXP465. The basic clock operations and an external trigger are implemented. > > Signed-off-by: Richard Cochran > Acked-by: John Stultz My ack is still fine, but found a few more nits to consider on another pass... > +static int match(struct sk_buff *skb, u16 uid_hi, u32 uid_lo, u16 seq) [snip] > +static void do_rx_timestamp(struct port *port, struct sk_buff *skb) [snip] > +static u64 sys_time_read(struct ixp46x_ts_regs *regs) [snip] > +static void sys_time_write(struct ixp46x_ts_regs *regs, u64 ns) I know these are static, but these function names are sort of generic names, and make grep/cscoping for similar sounding functions a little noisy. Maybe add a ipx_ prefix just to be more obvious? thanks -john From mboxrd@z Thu Jan 1 00:00:00 1970 From: john.stultz@linaro.org (John Stultz) Date: Wed, 23 Mar 2011 14:43:00 -0700 Subject: [PATCH V12 3/4] ptp: Added a clock driver for the IXP46x. In-Reply-To: References: Message-ID: <1300916580.848.51.camel@work-vm> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, 2011-02-28 at 08:58 +0100, Richard Cochran wrote: > This patch adds a driver for the hardware time stamping unit found on the > IXP465. The basic clock operations and an external trigger are implemented. > > Signed-off-by: Richard Cochran > Acked-by: John Stultz My ack is still fine, but found a few more nits to consider on another pass... > +static int match(struct sk_buff *skb, u16 uid_hi, u32 uid_lo, u16 seq) [snip] > +static void do_rx_timestamp(struct port *port, struct sk_buff *skb) [snip] > +static u64 sys_time_read(struct ixp46x_ts_regs *regs) [snip] > +static void sys_time_write(struct ixp46x_ts_regs *regs, u64 ns) I know these are static, but these function names are sort of generic names, and make grep/cscoping for similar sounding functions a little noisy. Maybe add a ipx_ prefix just to be more obvious? thanks -john