From mboxrd@z Thu Jan 1 00:00:00 1970 From: boris.brezillon@free-electrons.com (Boris Brezillon) Date: Mon, 20 Jul 2015 12:12:52 +0200 Subject: [RFC PATCH 06/15] pwm: define a new pwm_state struct In-Reply-To: <20150720100925.GX29614@ulmo> References: <1435738921-25027-1-git-send-email-boris.brezillon@free-electrons.com> <1435738921-25027-7-git-send-email-boris.brezillon@free-electrons.com> <20150720080458.GG29614@ulmo> <20150720120116.2358b829@bbrezillon> <20150720100925.GX29614@ulmo> Message-ID: <20150720121252.559d286a@bbrezillon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, 20 Jul 2015 12:09:26 +0200 Thierry Reding wrote: > On Mon, Jul 20, 2015 at 12:01:16PM +0200, Boris Brezillon wrote: > > On Mon, 20 Jul 2015 10:04:59 +0200 > > Thierry Reding wrote: > > > > > On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote: > > > [...] > > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > > > [...] > > > > +struct pwm_state { > > > > + unsigned int period; /* in nanoseconds */ > > > > + unsigned int duty_cycle; /* in nanoseconds */ > > > > + enum pwm_polarity polarity; > > > > +}; > > > > > > No need for the extra padding here. > > > > What do you mean by "extra padding" ? > > I just reused the indentation used in the pwm_device struct. > > Yeah, I have a local patch to fix that up. I find it useless to pad > things like this, and it has the downside that it will become totally > inconsistent (or cause a lot of churn by reformatting) if ever you add a > field that extends beyond the padding. Single spaces don't have any such > drawbacks and, in my opinion, look just as good. I prefer the single space approach too, so I won't complain ;-). > > > Would you prefer something like that ? > > > > struct pwm_state { > > unsigned int period; /* in nanoseconds */ > > unsigned int duty_cycle; /* in nanoseconds */ > > enum pwm_polarity polarity; > > }; > > Yeah. I'd say even the comments would be more suited in a kerneldoc- > style comment: > > /** > * struct pwm_state - state of a PWM channel > * @period: PWM period (in nanoseconds) > * @duty_cycle: PWM duty cycle (in nanoseconds) > * @polarity: PWM polarity > */ > struct pwm_state { > unsigned int period; > unsigned int duty_cycle; > enum pwm_polarity polarity; > }; > > This is something that users will need to deal with, so eventually > somebody might look at this via some DocBook generated HTML or PDF. I agree. -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct Date: Mon, 20 Jul 2015 12:12:52 +0200 Message-ID: <20150720121252.559d286a@bbrezillon> References: <1435738921-25027-1-git-send-email-boris.brezillon@free-electrons.com> <1435738921-25027-7-git-send-email-boris.brezillon@free-electrons.com> <20150720080458.GG29614@ulmo> <20150720120116.2358b829@bbrezillon> <20150720100925.GX29614@ulmo> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: Received: from down.free-electrons.com ([37.187.137.238]:47183 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753441AbbGTKM4 (ORCPT ); Mon, 20 Jul 2015 06:12:56 -0400 In-Reply-To: <20150720100925.GX29614@ulmo> Sender: linux-leds-owner@vger.kernel.org List-Id: linux-leds@vger.kernel.org To: Thierry Reding Cc: linux-pwm@vger.kernel.org, Mark Brown , Liam Girdwood , Bryan Wu , Richard Purdie , Jacek Anaszewski , linux-leds@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Jean-Christophe Plagniol-Villard , Tomi Valkeinen , linux-fbdev@vger.kernel.org, Stephen Warren , Alexandre Courbot , linux-tegra@vger.kernel.org, Maxime Ripard , Jingoo Han , Lee Jones , Doug Anderson On Mon, 20 Jul 2015 12:09:26 +0200 Thierry Reding wrote: > On Mon, Jul 20, 2015 at 12:01:16PM +0200, Boris Brezillon wrote: > > On Mon, 20 Jul 2015 10:04:59 +0200 > > Thierry Reding wrote: > > > > > On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote: > > > [...] > > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > > > [...] > > > > +struct pwm_state { > > > > + unsigned int period; /* in nanoseconds */ > > > > + unsigned int duty_cycle; /* in nanoseconds */ > > > > + enum pwm_polarity polarity; > > > > +}; > > > > > > No need for the extra padding here. > > > > What do you mean by "extra padding" ? > > I just reused the indentation used in the pwm_device struct. > > Yeah, I have a local patch to fix that up. I find it useless to pad > things like this, and it has the downside that it will become totally > inconsistent (or cause a lot of churn by reformatting) if ever you add a > field that extends beyond the padding. Single spaces don't have any such > drawbacks and, in my opinion, look just as good. I prefer the single space approach too, so I won't complain ;-). > > > Would you prefer something like that ? > > > > struct pwm_state { > > unsigned int period; /* in nanoseconds */ > > unsigned int duty_cycle; /* in nanoseconds */ > > enum pwm_polarity polarity; > > }; > > Yeah. I'd say even the comments would be more suited in a kerneldoc- > style comment: > > /** > * struct pwm_state - state of a PWM channel > * @period: PWM period (in nanoseconds) > * @duty_cycle: PWM duty cycle (in nanoseconds) > * @polarity: PWM polarity > */ > struct pwm_state { > unsigned int period; > unsigned int duty_cycle; > enum pwm_polarity polarity; > }; > > This is something that users will need to deal with, so eventually > somebody might look at this via some DocBook generated HTML or PDF. I agree. -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Date: Mon, 20 Jul 2015 10:12:52 +0000 Subject: Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct Message-Id: <20150720121252.559d286a@bbrezillon> List-Id: References: <1435738921-25027-1-git-send-email-boris.brezillon@free-electrons.com> <1435738921-25027-7-git-send-email-boris.brezillon@free-electrons.com> <20150720080458.GG29614@ulmo> <20150720120116.2358b829@bbrezillon> <20150720100925.GX29614@ulmo> In-Reply-To: <20150720100925.GX29614@ulmo> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-arm-kernel@lists.infradead.org On Mon, 20 Jul 2015 12:09:26 +0200 Thierry Reding wrote: > On Mon, Jul 20, 2015 at 12:01:16PM +0200, Boris Brezillon wrote: > > On Mon, 20 Jul 2015 10:04:59 +0200 > > Thierry Reding wrote: > > > > > On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote: > > > [...] > > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > > > [...] > > > > +struct pwm_state { > > > > + unsigned int period; /* in nanoseconds */ > > > > + unsigned int duty_cycle; /* in nanoseconds */ > > > > + enum pwm_polarity polarity; > > > > +}; > > > > > > No need for the extra padding here. > > > > What do you mean by "extra padding" ? > > I just reused the indentation used in the pwm_device struct. > > Yeah, I have a local patch to fix that up. I find it useless to pad > things like this, and it has the downside that it will become totally > inconsistent (or cause a lot of churn by reformatting) if ever you add a > field that extends beyond the padding. Single spaces don't have any such > drawbacks and, in my opinion, look just as good. I prefer the single space approach too, so I won't complain ;-). > > > Would you prefer something like that ? > > > > struct pwm_state { > > unsigned int period; /* in nanoseconds */ > > unsigned int duty_cycle; /* in nanoseconds */ > > enum pwm_polarity polarity; > > }; > > Yeah. I'd say even the comments would be more suited in a kerneldoc- > style comment: > > /** > * struct pwm_state - state of a PWM channel > * @period: PWM period (in nanoseconds) > * @duty_cycle: PWM duty cycle (in nanoseconds) > * @polarity: PWM polarity > */ > struct pwm_state { > unsigned int period; > unsigned int duty_cycle; > enum pwm_polarity polarity; > }; > > This is something that users will need to deal with, so eventually > somebody might look at this via some DocBook generated HTML or PDF. I agree. -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com