| Message ID | f7986df5d54b2bb84ee14e80d0c1225444608f32.1581533161.git.gurus@codeaurora.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Convert period and duty cycle to u64 | expand |
Hello, On Wed, Feb 12, 2020 at 10:54:08AM -0800, Guru Das Srinagesh wrote: > @@ -305,8 +305,8 @@ struct pwm_chip { > * @duty_cycle: duty cycle of the PWM signal (in nanoseconds) > */ > struct pwm_capture { > - unsigned int period; > - unsigned int duty_cycle; > + u64 period; > + u64 duty_cycle; > }; Is this last hunk a separate change? Otherwise looks fine. Best regards Uwe
On Thu, Feb 13, 2020 at 11:18:02AM +0100, Uwe Kleine-König wrote: > Hello, > > On Wed, Feb 12, 2020 at 10:54:08AM -0800, Guru Das Srinagesh wrote: > > @@ -305,8 +305,8 @@ struct pwm_chip { > > * @duty_cycle: duty cycle of the PWM signal (in nanoseconds) > > */ > > struct pwm_capture { > > - unsigned int period; > > - unsigned int duty_cycle; > > + u64 period; > > + u64 duty_cycle; > > }; > > Is this last hunk a separate change? > > Otherwise looks fine. Hi Uwe, No, this is very much a part of the change and not a separate one. If things look good to you, could you please add an Acked-by or Reviewed-by tag to the patches so that they get accepted by Thierry? Thank you. Guru Das.
Hello, On Thu, Feb 13, 2020 at 11:39:26AM -0800, Guru Das Srinagesh wrote: > On Thu, Feb 13, 2020 at 11:18:02AM +0100, Uwe Kleine-König wrote: > > Hello, > > > > On Wed, Feb 12, 2020 at 10:54:08AM -0800, Guru Das Srinagesh wrote: > > > @@ -305,8 +305,8 @@ struct pwm_chip { > > > * @duty_cycle: duty cycle of the PWM signal (in nanoseconds) > > > */ > > > struct pwm_capture { > > > - unsigned int period; > > > - unsigned int duty_cycle; > > > + u64 period; > > > + u64 duty_cycle; > > > }; > > > > Is this last hunk a separate change? > > > > Otherwise looks fine. > > No, this is very much a part of the change and not a separate one. Not sure we understand each other. I wondered if conversion of the pwm_capture stuff should be done separately. (OTOH I wonder if this is used at all and already considered deleting it.) Best regards Uwe
Hi Uwe, On Thu, Feb 13, 2020 at 09:28:04PM +0100, Uwe Kleine-König wrote: > Hello, > > On Thu, Feb 13, 2020 at 11:39:26AM -0800, Guru Das Srinagesh wrote: > > On Thu, Feb 13, 2020 at 11:18:02AM +0100, Uwe Kleine-König wrote: > > > Hello, > > > > > > On Wed, Feb 12, 2020 at 10:54:08AM -0800, Guru Das Srinagesh wrote: > > > > @@ -305,8 +305,8 @@ struct pwm_chip { > > > > * @duty_cycle: duty cycle of the PWM signal (in nanoseconds) > > > > */ > > > > struct pwm_capture { > > > > - unsigned int period; > > > > - unsigned int duty_cycle; > > > > + u64 period; > > > > + u64 duty_cycle; > > > > }; > > > > > > Is this last hunk a separate change? > > > > > > Otherwise looks fine. > > > > No, this is very much a part of the change and not a separate one. > > Not sure we understand each other. I wondered if conversion of the > pwm_capture stuff should be done separately. (OTOH I wonder if this is > used at all and already considered deleting it.) I see. Could you please elaborate on your concerns? I think this hunk's being in this patch makes sense as duty and period should be converted to u64 throughout the file in one go. Also, it looks like pwm_capture is being used by pwm-sti.c and pwm-stm32.c currently. Thank you. Guru Das.
On Thu, Feb 13, 2020 at 01:06:49PM -0800, Guru Das Srinagesh wrote: > On Thu, Feb 13, 2020 at 09:28:04PM +0100, Uwe Kleine-König wrote: > > On Thu, Feb 13, 2020 at 11:39:26AM -0800, Guru Das Srinagesh wrote: > > > On Thu, Feb 13, 2020 at 11:18:02AM +0100, Uwe Kleine-König wrote: > > > > Hello, > > > > > > > > On Wed, Feb 12, 2020 at 10:54:08AM -0800, Guru Das Srinagesh wrote: > > > > > @@ -305,8 +305,8 @@ struct pwm_chip { > > > > > * @duty_cycle: duty cycle of the PWM signal (in nanoseconds) > > > > > */ > > > > > struct pwm_capture { > > > > > - unsigned int period; > > > > > - unsigned int duty_cycle; > > > > > + u64 period; > > > > > + u64 duty_cycle; > > > > > }; > > > > > > > > Is this last hunk a separate change? > > > > > > > > Otherwise looks fine. > > > > > > No, this is very much a part of the change and not a separate one. > > > > Not sure we understand each other. I wondered if conversion of the > > pwm_capture stuff should be done separately. (OTOH I wonder if this is > > used at all and already considered deleting it.) > > I see. Could you please elaborate on your concerns? I think this hunk's > being in this patch makes sense as duty and period should be converted > to u64 throughout the file in one go. I guess that capturing isn't much used if at all. So keeping it as limited as it is today doesn't seem like a bad idea to me. (OK, you could also accidentally break it such that we can say in a few years time: This is broken since v5.6, obviously nobody cares and we remove it for good :-)) > Also, it looks like pwm_capture is being used by pwm-sti.c and > pwm-stm32.c currently. Yeah, these two drivers provide the needed callback. That doesn't necessarily mean there are active users. Also I'm convinced that there are implementation problems. For example the commit that added capture support for stm32 has in its commit log: Capture requires exclusive access (e.g. no pwm output running at the same time, to protect common prescaler). but the capture callback doesn't even check if the PWMs are in use (but I only looked quickly, so I might have missed something). Apart from that I think that the capturing stuff doesn't really fit into the PWM framework which is (apart from the capture callback and API function) about PWM *outputs* and most hardware's I know about either don't support capturing or it is located in a different IP than the PWM output. (And it is not even possible today to register a driver that can only capture but not drive a PWM output.) Best regards Uwe
On Thu, Feb 13, 2020 at 10:58:57PM +0100, Uwe Kleine-König wrote: > On Thu, Feb 13, 2020 at 01:06:49PM -0800, Guru Das Srinagesh wrote: > > On Thu, Feb 13, 2020 at 09:28:04PM +0100, Uwe Kleine-König wrote: > > > On Thu, Feb 13, 2020 at 11:39:26AM -0800, Guru Das Srinagesh wrote: > > > > On Thu, Feb 13, 2020 at 11:18:02AM +0100, Uwe Kleine-König wrote: > > > > > Hello, > > > > > > > > > > On Wed, Feb 12, 2020 at 10:54:08AM -0800, Guru Das Srinagesh wrote: > > > > > > @@ -305,8 +305,8 @@ struct pwm_chip { > > > > > > * @duty_cycle: duty cycle of the PWM signal (in nanoseconds) > > > > > > */ > > > > > > struct pwm_capture { > > > > > > - unsigned int period; > > > > > > - unsigned int duty_cycle; > > > > > > + u64 period; > > > > > > + u64 duty_cycle; > > > > > > }; > > > > > > > > > > Is this last hunk a separate change? > > > > > > > > > > Otherwise looks fine. > > > > > > > > No, this is very much a part of the change and not a separate one. > > > > > > Not sure we understand each other. I wondered if conversion of the > > > pwm_capture stuff should be done separately. (OTOH I wonder if this is > > > used at all and already considered deleting it.) > > > > I see. Could you please elaborate on your concerns? I think this hunk's > > being in this patch makes sense as duty and period should be converted > > to u64 throughout the file in one go. > > I guess that capturing isn't much used if at all. So keeping it as > limited as it is today doesn't seem like a bad idea to me. (OK, you > could also accidentally break it such that we can say in a few years > time: This is broken since v5.6, obviously nobody cares and we remove it > for good :-)) > > > Also, it looks like pwm_capture is being used by pwm-sti.c and > > pwm-stm32.c currently. > > Yeah, these two drivers provide the needed callback. That doesn't > necessarily mean there are active users. Also I'm convinced that there > are implementation problems. For example the commit that added capture > support for stm32 has in its commit log: > > Capture requires exclusive access (e.g. no pwm output running at > the same time, to protect common prescaler). > > but the capture callback doesn't even check if the PWMs are in use (but > I only looked quickly, so I might have missed something). > > Apart from that I think that the capturing stuff doesn't really fit into > the PWM framework which is (apart from the capture callback and API > function) about PWM *outputs* and most hardware's I know about either > don't support capturing or it is located in a different IP than the PWM > output. (And it is not even possible today to register a driver that can > only capture but not drive a PWM output.) Thanks for the explanation. So what would you recommend - dropping that hunk entirely or separating that out in a separate patch? Thank you. Guru Das.
Hello, On Thu, Feb 13, 2020 at 02:38:09PM -0800, Guru Das Srinagesh wrote: > Thanks for the explanation. So what would you recommend - dropping that > hunk entirely or separating that out in a separate patch? I'd drop that hunk. Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 5a7f659..81aa3c2 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -1163,8 +1163,8 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s) if (state.enabled) seq_puts(s, " enabled"); - seq_printf(s, " period: %u ns", state.period); - seq_printf(s, " duty: %u ns", state.duty_cycle); + seq_printf(s, " period: %llu ns", state.period); + seq_printf(s, " duty: %llu ns", state.duty_cycle); seq_printf(s, " polarity: %s", state.polarity ? "inverse" : "normal"); diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index 2389b86..3fb1610 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -42,7 +42,7 @@ static ssize_t period_show(struct device *child, pwm_get_state(pwm, &state); - return sprintf(buf, "%u\n", state.period); + return sprintf(buf, "%llu\n", state.period); } static ssize_t period_store(struct device *child, @@ -52,10 +52,10 @@ static ssize_t period_store(struct device *child, struct pwm_export *export = child_to_pwm_export(child); struct pwm_device *pwm = export->pwm; struct pwm_state state; - unsigned int val; + u64 val; int ret; - ret = kstrtouint(buf, 0, &val); + ret = kstrtou64(buf, 0, &val); if (ret) return ret; @@ -77,7 +77,7 @@ static ssize_t duty_cycle_show(struct device *child, pwm_get_state(pwm, &state); - return sprintf(buf, "%u\n", state.duty_cycle); + return sprintf(buf, "%llu\n", state.duty_cycle); } static ssize_t duty_cycle_store(struct device *child, @@ -212,7 +212,7 @@ static ssize_t capture_show(struct device *child, if (ret) return ret; - return sprintf(buf, "%u %u\n", result.period, result.duty_cycle); + return sprintf(buf, "%llu %llu\n", result.period, result.duty_cycle); } static DEVICE_ATTR_RW(period); diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 0ef808d..df83440 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -39,7 +39,7 @@ enum pwm_polarity { * current PWM hardware state. */ struct pwm_args { - unsigned int period; + u64 period; enum pwm_polarity polarity; }; @@ -56,8 +56,8 @@ enum { * @enabled: PWM enabled status */ struct pwm_state { - unsigned int period; - unsigned int duty_cycle; + u64 period; + u64 duty_cycle; enum pwm_polarity polarity; bool enabled; }; @@ -105,13 +105,13 @@ static inline bool pwm_is_enabled(const struct pwm_device *pwm) return state.enabled; } -static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period) +static inline void pwm_set_period(struct pwm_device *pwm, u64 period) { if (pwm) pwm->state.period = period; } -static inline unsigned int pwm_get_period(const struct pwm_device *pwm) +static inline u64 pwm_get_period(const struct pwm_device *pwm) { struct pwm_state state; @@ -126,7 +126,7 @@ static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty) pwm->state.duty_cycle = duty; } -static inline unsigned int pwm_get_duty_cycle(const struct pwm_device *pwm) +static inline u64 pwm_get_duty_cycle(const struct pwm_device *pwm) { struct pwm_state state; @@ -305,8 +305,8 @@ struct pwm_chip { * @duty_cycle: duty cycle of the PWM signal (in nanoseconds) */ struct pwm_capture { - unsigned int period; - unsigned int duty_cycle; + u64 period; + u64 duty_cycle; }; #if IS_ENABLED(CONFIG_PWM)
Because period and duty cycle are defined as ints with units of nanoseconds, the maximum time duration that can be set is limited to ~2.147 seconds. Change their definitions to u64 in the structs of the PWM framework so that higher durations may be set. Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org> --- drivers/pwm/core.c | 4 ++-- drivers/pwm/sysfs.c | 10 +++++----- include/linux/pwm.h | 16 ++++++++-------- 3 files changed, 15 insertions(+), 15 deletions(-)