From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753296AbdFVOzs (ORCPT ); Thu, 22 Jun 2017 10:55:48 -0400 Received: from mail-lf0-f54.google.com ([209.85.215.54]:35872 "EHLO mail-lf0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751156AbdFVOzp (ORCPT ); Thu, 22 Jun 2017 10:55:45 -0400 Subject: Re: [PATCH 2/3] irq: Track the interrupt timings To: Thomas Gleixner Cc: linux-kernel@vger.kernel.org, Peter Zijlstra , "Rafael J . Wysocki" , Vincent Guittot , Nicolas Pitre , Hannes Reinecke , Jens Axboe , Bjorn Helgaas References: <20170620212657.GB1812@mai> <1497994139-19816-1-git-send-email-daniel.lezcano@linaro.org> <1497994139-19816-2-git-send-email-daniel.lezcano@linaro.org> From: Daniel Lezcano Message-ID: Date: Thu, 22 Jun 2017 16:55:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Thomas, thanks for the review. On 22/06/2017 16:47, Thomas Gleixner wrote: > On Tue, 20 Jun 2017, Daniel Lezcano wrote: >> + >> +struct irq_timings { >> + u64 values[IRQ_TIMINGS_SIZE]; /* our circular buffer */ >> + unsigned int count; /* Number of interruptions since last inspection */ > > Groan. These tail comments are horrible. > > Please make the struct member names tabular aligned and add proper kernel > doc comments if you want to add useful documentations for the fields. [ ... ] Ok. >> + * The interrupt number and the timestamp are encoded into a single >> + * u64 variable to optimize the size. >> + * 48 bit time stamp and 16 bit IRQ number is way sufficient. >> + * Who cares an IRQ after 78 hours of idle time? >> + */ >> +static inline u64 irq_timing_encode(u64 timestamp, int irq) >> +{ >> + return (timestamp << 16) | irq; >> +} >> + >> +static inline void irq_timing_decode(u64 value, u64 *timestamp, int *irq) > > What's wrong with using a return value instead of void? Nothing wrong, as we are expecting two values I don't like the idea to have one returned and the other one passed as a pointer. It is a matter of taste. I can return the irq if you prefer. -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog