Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct

Thierry Reding
2015-07-20T10:09:26Z

[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]

On Mon, Jul 20, 2015 at 12:01:16PM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:04:59 +0200
> Thierry Reding <thierry.reding@gmail.com> 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.

> 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.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]