diff mbox series

[RFC,2/6] pwm: core: Add option to config PWM duty/period with u64 data length

Message ID 20200724213659.273599-3-martin.botka1@gmail.com
State Not Applicable
Headers show
Series Add QCOM pwm-lpg and tri-led drivers | expand

Commit Message

Martin Botka July 24, 2020, 9:36 p.m. UTC 
From: Fenglin Wu <fenglinw@codeaurora.org>

Currently, PWM core driver provides interfaces for configuring PWM
period and duty length in nanoseconds with an integer data type, so
the max period can be only set to ~2.147 seconds. Add interfaces which
can set PWM period and duty with u64 data type to remove this
limitation.

Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
[konradybcio@gmail.com: Fast-forward from kernel 4.14 to 5.8]
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
Signed-off-by: Martin Botka <martin.botka1@gmail.com>
---
 drivers/pwm/core.c  | 30 +++++++++++------
 drivers/pwm/sysfs.c |  6 ++--
 include/linux/pwm.h | 79 +++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 95 insertions(+), 20 deletions(-)

Comments

Martin Botka July 25, 2020, 2:55 p.m. UTC | #1 
Hello,

As can be seen this divides llu by llu in few warnings and error.
At the time of sending i didn't realize it but this fails on 32 bit
architectures.

So i would like to ask how would you like this fixed ?
Using macro or some other way ?

Thank you.

Best regards,
Martin
Pavel Machek July 25, 2020, 3:24 p.m. UTC | #2 
Hi!

> As can be seen this divides llu by llu in few warnings and error.
> At the time of sending i didn't realize it but this fails on 32 bit
> architectures.
> 
> So i would like to ask how would you like this fixed ?
> Using macro or some other way ?

+#include <linux/math64.h>
-               gain_q23 = (gain_q23 * dmic->boost_gain) / 100;
+               gain_q23 = div_u64(gain_q23 * dmic->boost_gain, 100);

Best regards,
									Pavel
Martin Botka July 25, 2020, 3:30 p.m. UTC | #3 
> +#include <linux/math64.h>
> -               gain_q23 = (gain_q23 * dmic->boost_gain) / 100;
> +               gain_q23 = div_u64(gain_q23 * dmic->boost_gain, 100);

Ok so using a macro.

Thank you.
Andy Shevchenko July 26, 2020, 9:04 a.m. UTC | #4 
On Sat, Jul 25, 2020 at 12:40 AM Martin Botka <martin.botka1@gmail.com> wrote:
>
> From: Fenglin Wu <fenglinw@codeaurora.org>
>
> Currently, PWM core driver provides interfaces for configuring PWM
> period and duty length in nanoseconds with an integer data type, so
> the max period can be only set to ~2.147 seconds. Add interfaces which
> can set PWM period and duty with u64 data type to remove this
> limitation.

And all divisions go mad on 32-bit CPU, right?
Please, if you thought about it carefully, update a commit message to
clarify that.
Martin Botka July 26, 2020, 9:12 a.m. UTC | #5 
> And all divisions go mad on 32-bit CPU, right?
> Please, if you thought about it carefully, update a commit message to
> clarify that.

Hello,
This patch will be dropped in V2 since another series already made these u64.
See a9d887dc1c60ed67f2271d66560cdcf864c4a578 in linux-next.
I have not tested compiling that commit in linux-next on 32 bit arch
but if it fails i can replace this commit with fix for that.

Also  I'm not the author of this commit.
Konrad Dybcio fast forwarded it to 5.8 from 4.14.
Fenglin Wu is the author and also created that commit message.

Thank you.

Best regards
Martin
Martin Botka July 27, 2020, 7:29 a.m. UTC | #6 
Hello Uwe,

On Sat, Jul 25, 2020 at 09:12:23PM +0200, Martin Botka wrote:
>> > Note there is already a series that changes these values to u64. See
>> > a9d887dc1c60ed67f2271d66560cdcf864c4a578 in linux-next.
>>
>> Amazing. But isn't there the same issue with it as this one where this
>> would fail to build on 32 bit architecture?
>
> In theory all these cases are coped for. I didn't see any problems yet,
> so I still assume also the 32 bit archs are fine.

OK then all is fine. I will drop the patch in V2.

Also Uwe i just realized that you sent the original message and also
this reply only to me and not to anyone else.
Could you please send the messages also to everyone else ?

Thank you.

Best regards,
Martin
Martin Botka July 27, 2020, 7:32 a.m. UTC | #7 
> Could you please send the messages also to everyone else ?

Next time of course.
Uwe Kleine-König July 27, 2020, 7:52 a.m. UTC | #8 
Hello Martin,

On Mon, Jul 27, 2020 at 09:29:19AM +0200, Martin Botka wrote:
> On Sat, Jul 25, 2020 at 09:12:23PM +0200, Martin Botka wrote:
> >> > Note there is already a series that changes these values to u64. See
> >> > a9d887dc1c60ed67f2271d66560cdcf864c4a578 in linux-next.
> >>
> >> Amazing. But isn't there the same issue with it as this one where this
> >> would fail to build on 32 bit architecture?
> >
> > In theory all these cases are coped for. I didn't see any problems yet,
> > so I still assume also the 32 bit archs are fine.
> 
> OK then all is fine. I will drop the patch in V2.
> 
> Also Uwe i just realized that you sent the original message and also
> this reply only to me and not to anyone else.
> Could you please send the messages also to everyone else ?

I hit "reply-to-all" and the mail only was sent to you because you wrote
to only me.

Also threading is somehow strange because your reply to my mail (with

	Message-Id: 20200727070411.ovkuwm76vuw3heo7@pengutronix.de

) has

	In-Reply-To: <CADQ2G_HkiAZx8OhfQ_jeizveMaB-QN9dfN6Tcwfk9XuF97rmOg@mail.gmail.com>

. So I assume all the strange things happened on your side until proved
otherwise. :-)

Best regards
Uwe
Martin Botka July 27, 2020, 7:58 a.m. UTC | #9 
Hello,

> I hit "reply-to-all" and the mail only was sent to you because you wrote
> to only me.

Yes my reply was only to you. But your original message was sent only to me too.
So when i clicked reply to all it was only you as you sent it only to me.

> Also threading is somehow strange because your reply to my mail

Yes Gmail would not allow me to reply to your message and also send it
to everyone so i had to reply to Andy's email which is why the
threading is broken there. Sorry for that.

> So I assume all the strange things happened on your side until proved
> otherwise. :-)

I think i just proved otherwise :)

Best Regards,
Martin
Uwe Kleine-König July 27, 2020, 8:34 a.m. UTC | #10 
Hello Martin,

On Mon, Jul 27, 2020 at 09:58:01AM +0200, Martin Botka wrote:
> > I hit "reply-to-all" and the mail only was sent to you because you wrote
> > to only me.
> 
> Yes my reply was only to you. But your original message was sent only to me too.
> So when i clicked reply to all it was only you as you sent it only to me.

Oh indeed. Bummer, and I was so sure to always reply to all :-|

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index f3aa44106962..82411e3ccbbb 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -511,12 +511,12 @@  static void pwm_apply_state_debug(struct pwm_device *pwm,
 	    last->period > s2.period &&
 	    last->period <= state->period)
 		dev_warn(chip->dev,
-			 ".apply didn't pick the best available period (requested: %u, applied: %u, possible: %u)\n",
+			 ".apply didn't pick the best available period (requested: %llu, applied: %llu, possible: %llu)\n",
 			 state->period, s2.period, last->period);
 
 	if (state->enabled && state->period < s2.period)
 		dev_warn(chip->dev,
-			 ".apply is supposed to round down period (requested: %u, applied: %u)\n",
+			 ".apply is supposed to round down period (requested: %llu, applied: %llu)\n",
 			 state->period, s2.period);
 
 	if (state->enabled &&
@@ -525,14 +525,14 @@  static void pwm_apply_state_debug(struct pwm_device *pwm,
 	    last->duty_cycle > s2.duty_cycle &&
 	    last->duty_cycle <= state->duty_cycle)
 		dev_warn(chip->dev,
-			 ".apply didn't pick the best available duty cycle (requested: %u/%u, applied: %u/%u, possible: %u/%u)\n",
+			 ".apply didn't pick the best available duty cycle (requested: %llu/%llu, applied: %llu/%llu, possible: %llu/%llu)\n",
 			 state->duty_cycle, state->period,
 			 s2.duty_cycle, s2.period,
 			 last->duty_cycle, last->period);
 
 	if (state->enabled && state->duty_cycle < s2.duty_cycle)
 		dev_warn(chip->dev,
-			 ".apply is supposed to round down duty_cycle (requested: %u/%u, applied: %u/%u)\n",
+			 ".apply is supposed to round down duty_cycle (requested: %llu/%llu, applied: %llu/%llu)\n",
 			 state->duty_cycle, state->period,
 			 s2.duty_cycle, s2.period);
 
@@ -559,7 +559,7 @@  static void pwm_apply_state_debug(struct pwm_device *pwm,
 	    (s1.enabled && s1.period != last->period) ||
 	    (s1.enabled && s1.duty_cycle != last->duty_cycle)) {
 		dev_err(chip->dev,
-			".apply is not idempotent (ena=%d pol=%d %u/%u) -> (ena=%d pol=%d %u/%u)\n",
+			".apply is not idempotent (ena=%d pol=%d %llu/%llu) -> (ena=%d pol=%d %llu/%llu)\n",
 			s1.enabled, s1.polarity, s1.duty_cycle, s1.period,
 			last->enabled, last->polarity, last->duty_cycle,
 			last->period);
@@ -655,9 +655,19 @@  int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 
 		if (state->period != pwm->state.period ||
 		    state->duty_cycle != pwm->state.duty_cycle) {
-			err = chip->ops->config(pwm->chip, pwm,
-						state->duty_cycle,
-						state->period);
+			if (pwm->chip->ops->config_extend) {
+					err = pwm->chip->ops->config_extend(pwm->chip,
+								pwm, state->duty_cycle,
+								state->period);
+			} else {
+				if (state->period > UINT_MAX)
+					pr_warn("period %llu duty_cycle %llu will be truncated\n",
+								state->period,
+								state->duty_cycle);
+				err = pwm->chip->ops->config(pwm->chip, pwm,
+								state->duty_cycle,
+								state->period);
+			}
 			if (err)
 				return err;
 
@@ -1310,8 +1320,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 4ee1e81db0bc..f9d7ebfb48f4 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,
@@ -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 ssize_t output_type_show(struct device *child,
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 10a102efadc4..ab235007287f 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;
 };
 
@@ -66,9 +66,9 @@  enum pwm_output_type {
  * @cycles_per_duty: number of PWM period cycles an entry stays at
  */
 struct pwm_output_pattern {
-	unsigned int *duty_pattern;
+	u64 *duty_pattern;
 	unsigned int num_entries;
-	unsigned int cycles_per_duty;
+	u64 cycles_per_duty;
 };
 
 /*
@@ -79,8 +79,8 @@  struct pwm_output_pattern {
  * @enabled: PWM enabled status
  */
 struct pwm_state {
-	unsigned int period;
-	unsigned int duty_cycle;
+	u64 period;
+	u64 duty_cycle;
 	enum pwm_polarity polarity;
 	enum pwm_output_type output_type;
 	struct pwm_output_pattern *output_pattern;
@@ -138,12 +138,30 @@  static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
 		pwm->state.period = period;
 }
 
+static inline void pwm_set_period_extend(struct pwm_device *pwm, u64 period)
+{
+	if (pwm)
+		pwm->state.period = period;
+}
+
 static inline unsigned int pwm_get_period(const struct pwm_device *pwm)
 {
 	struct pwm_state state;
 
 	pwm_get_state(pwm, &state);
 
+	if (state.period > UINT_MAX)
+		pr_warn("PWM period %llu is truncated\n", state.period);
+
+	return (unsigned int)state.period;
+}
+
+static inline u64 pwm_get_period_extend(const struct pwm_device *pwm)
+{
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
 	return state.period;
 }
 
@@ -153,12 +171,30 @@  static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty)
 		pwm->state.duty_cycle = duty;
 }
 
+static inline void pwm_set_duty_cycle_extend(struct pwm_device *pwm, u64 duty)
+{
+	if (pwm)
+		pwm->state.duty_cycle = duty;
+}
+
 static inline unsigned int pwm_get_duty_cycle(const struct pwm_device *pwm)
 {
 	struct pwm_state state;
 
 	pwm_get_state(pwm, &state);
 
+	if (state.duty_cycle > UINT_MAX)
+		pr_warn("PWM duty cycle %llu is truncated\n", state.duty_cycle);
+
+	return (unsigned int)state.duty_cycle;
+}
+
+static inline u64 pwm_get_duty_cycle_extend(const struct pwm_device *pwm)
+{
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
 	return state.duty_cycle;
 }
 
@@ -296,6 +332,8 @@  pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int duty_cycle,
  *	       registered.
  * @owner: helps prevent removal of modules exporting active PWMs
  * @config: configure duty cycles and period length for this PWM
+ * @config_extend: configure duty cycles and period length for this
+ *	       PWM with u64 data type
  * @set_polarity: configure the polarity of this PWM
  * @enable: enable PWM output toggling
  * @disable: disable PWM output toggling
@@ -317,6 +355,8 @@  struct pwm_ops {
 	/* Only used by legacy drivers */
 	int (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
 		      int duty_ns, int period_ns);
+	int (*config_extend)(struct pwm_chip *chip, struct pwm_device *pwm,
+		      u64 duty_ns, u64 period_ns);
 	int (*set_polarity)(struct pwm_chip *chip, struct pwm_device *pwm,
 			    enum pwm_polarity polarity);
 	int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
@@ -362,8 +402,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)
@@ -415,6 +455,31 @@  static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
 	return pwm_apply_state(pwm, &state);
 }
 
+/*
+ * pwm_config_extend() - change PWM period and duty length with u64 data type
+ * @pwm: PWM device
+ * @duty_ns: "on" time (in nanoseconds)
+ * @period_ns: duration (in nanoseconds) of one cycle
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+static inline int pwm_config_extend(struct pwm_device *pwm, u64 duty_ns,
+			     u64 period_ns)
+{
+	struct pwm_state state;
+
+	if (!pwm)
+		return -EINVAL;
+
+	pwm_get_state(pwm, &state);
+	if (state.duty_cycle == duty_ns && state.period == period_ns)
+		return 0;
+
+	state.duty_cycle = duty_ns;
+	state.period = period_ns;
+	return pwm_apply_state(pwm, &state);
+}
+
 /**
  * pwm_enable() - start a PWM output toggling
  * @pwm: PWM device