| Message ID | 1435738921-25027-7-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Thierry
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. Would you prefer something like that ? struct pwm_state { unsigned int period; /* in nanoseconds */ unsigned int duty_cycle; /* in nanoseconds */ enum pwm_polarity polarity; };
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
On Mon, 20 Jul 2015 12:09:26 +0200 Thierry Reding <thierry.reding@gmail.com> 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 <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. 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.
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 7ffad2b..a6bc8e6 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -431,8 +431,8 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns) if (err) return err; - pwm->duty_cycle = duty_ns; - pwm->period = period_ns; + pwm->state.duty_cycle = duty_ns; + pwm->state.period = period_ns; return 0; } @@ -462,7 +462,7 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity) if (err) return err; - pwm->polarity = polarity; + pwm->state.polarity = polarity; return 0; } diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 72a21d5..bed7234 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -79,6 +79,12 @@ enum { PWMF_EXPORTED = 1 << 2, }; +struct pwm_state { + unsigned int period; /* in nanoseconds */ + unsigned int duty_cycle; /* in nanoseconds */ + enum pwm_polarity polarity; +}; + struct pwm_device { const char *label; unsigned long flags; @@ -87,9 +93,7 @@ struct pwm_device { struct pwm_chip *chip; void *chip_data; - unsigned int period; /* in nanoseconds */ - unsigned int duty_cycle; /* in nanoseconds */ - enum pwm_polarity polarity; + struct pwm_state state; }; static inline bool pwm_is_enabled(const struct pwm_device *pwm) @@ -100,7 +104,7 @@ static inline bool pwm_is_enabled(const struct pwm_device *pwm) static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period) { if (pwm) - pwm->period = period; + pwm->state.period = period; } static inline void pwm_set_default_period(struct pwm_device *pwm, unsigned int period) @@ -110,7 +114,7 @@ static inline void pwm_set_default_period(struct pwm_device *pwm, unsigned int p static inline unsigned int pwm_get_period(const struct pwm_device *pwm) { - return pwm ? pwm->period : 0; + return pwm ? pwm->state.period : 0; } static inline unsigned int pwm_get_default_period(const struct pwm_device *pwm) @@ -121,12 +125,12 @@ static inline unsigned int pwm_get_default_period(const struct pwm_device *pwm) static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty) { if (pwm) - pwm->duty_cycle = duty; + pwm->state.duty_cycle = duty; } static inline unsigned int pwm_get_duty_cycle(const struct pwm_device *pwm) { - return pwm ? pwm->duty_cycle : 0; + return pwm ? pwm->state.duty_cycle : 0; } /* @@ -141,7 +145,7 @@ static inline void pwm_set_default_polarity(struct pwm_device *pwm, enum pwm_pol static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm) { - return pwm ? pwm->polarity : PWM_POLARITY_NORMAL; + return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL; } /**
The PWM state, represented by its period, duty_cycle and polarity, is currently directly stored in the PWM device. Declare a pwm_state structure embedding those field so that we can later use this struct to atomically update all the PWM parameters at once. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/pwm/core.c | 6 +++--- include/linux/pwm.h | 20 ++++++++++++-------- 2 files changed, 15 insertions(+), 11 deletions(-)