| Message ID | b503833e0f58bd6dd9fe84d866124e7c457e099e.1583782035.git.gurus@codeaurora.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Convert period and duty cycle to u64 | expand |
On Mon, Mar 09, 2020 at 12:35:06PM -0700, Guru Das Srinagesh wrote: > Because period and duty cycle are defined in the PWM framework structs > as ints with units of nanoseconds, the maximum time duration that can be > set is limited to ~2.147 seconds. Redefining them as u64 values will > enable larger time durations to be set. > > As a first step, prepare drivers to handle the switch to u64 period and > duty_cycle by replacing division operations involving pwm period and duty cycle > with their 64-bit equivalents as appropriate. The actual switch to u64 period > and duty_cycle follows as a separate patch. > > Where the dividend is 64-bit but the divisor is 32-bit, use *_ULL > macros: > - DIV_ROUND_UP_ULL > - DIV_ROUND_CLOSEST_ULL > - div_u64 > > Where the divisor is 64-bit (dividend may be 32-bit or 64-bit), use > DIV64_* macros: > - DIV64_U64_ROUND_CLOSEST > - div64_u64 > There is no explanation why this is necessary. What is the use case ? It is hard to imagine a real-world use case with a duty cycle of more than 2 seconds. Guenter
Hello Guenter, On Mon, Mar 09, 2020 at 02:48:22PM -0700, Guenter Roeck wrote: > On Mon, Mar 09, 2020 at 12:35:06PM -0700, Guru Das Srinagesh wrote: > > Because period and duty cycle are defined in the PWM framework structs > > as ints with units of nanoseconds, the maximum time duration that can be > > set is limited to ~2.147 seconds. Redefining them as u64 values will > > enable larger time durations to be set. > > > > As a first step, prepare drivers to handle the switch to u64 period and > > duty_cycle by replacing division operations involving pwm period and duty cycle > > with their 64-bit equivalents as appropriate. The actual switch to u64 period > > and duty_cycle follows as a separate patch. > > > > Where the dividend is 64-bit but the divisor is 32-bit, use *_ULL > > macros: > > - DIV_ROUND_UP_ULL > > - DIV_ROUND_CLOSEST_ULL > > - div_u64 > > > > Where the divisor is 64-bit (dividend may be 32-bit or 64-bit), use > > DIV64_* macros: > > - DIV64_U64_ROUND_CLOSEST > > - div64_u64 > > There is no explanation why this is necessary. What is the use case ? > It is hard to imagine a real-world use case with a duty cycle of more > than 2 seconds. When my Laptop is in suspend there is an LED that blinks with a period of approximately 5 seconds. (To be fair, the brightness is more a sinus than a rectangle, but still.) Best regards Uwe
On 3/10/20 5:08 AM, Uwe Kleine-König wrote: > Hello Guenter, > > On Mon, Mar 09, 2020 at 02:48:22PM -0700, Guenter Roeck wrote: >> On Mon, Mar 09, 2020 at 12:35:06PM -0700, Guru Das Srinagesh wrote: >>> Because period and duty cycle are defined in the PWM framework structs >>> as ints with units of nanoseconds, the maximum time duration that can be >>> set is limited to ~2.147 seconds. Redefining them as u64 values will >>> enable larger time durations to be set. >>> >>> As a first step, prepare drivers to handle the switch to u64 period and >>> duty_cycle by replacing division operations involving pwm period and duty cycle >>> with their 64-bit equivalents as appropriate. The actual switch to u64 period >>> and duty_cycle follows as a separate patch. >>> >>> Where the dividend is 64-bit but the divisor is 32-bit, use *_ULL >>> macros: >>> - DIV_ROUND_UP_ULL >>> - DIV_ROUND_CLOSEST_ULL >>> - div_u64 >>> >>> Where the divisor is 64-bit (dividend may be 32-bit or 64-bit), use >>> DIV64_* macros: >>> - DIV64_U64_ROUND_CLOSEST >>> - div64_u64 >> >> There is no explanation why this is necessary. What is the use case ? >> It is hard to imagine a real-world use case with a duty cycle of more >> than 2 seconds. > > When my Laptop is in suspend there is an LED that blinks with a period > of approximately 5 seconds. (To be fair, the brightness is more a sinus > than a rectangle, but still.) > I don't see support in the LED subsystem to utilize the PWM framework directly for blinking. Plus, you say yourself that it isn't a _real_ use case, just a theoretic one. Either case, the reason / use case for this series should be explained in the summary patch. That is what it is for. That case is not made. Guenter
On Tue, Mar 10, 2020 at 08:05:58AM -0700, Guenter Roeck wrote: > I don't see support in the LED subsystem to utilize the PWM framework directly > for blinking. Plus, you say yourself that it isn't a _real_ use case, just a > theoretic one. An example use case is a mobile phone OEM that wishes to set a period of 5 seconds or more for, say, a low battery slow blinking indication - currently this is not possible. The PWM framework not having direct support for blinking should not be a concern if the PWM peripheral being controlled supports it via register writes. > Either case, the reason / use case for this series should be explained > in the summary patch. That is what it is for. That case is not made. Will upload a new patchset adding more details in the summary patch. Thank you. Guru Das.
On 3/10/20 3:24 PM, Guru Das Srinagesh wrote: > On Tue, Mar 10, 2020 at 08:05:58AM -0700, Guenter Roeck wrote: >> I don't see support in the LED subsystem to utilize the PWM framework directly >> for blinking. Plus, you say yourself that it isn't a _real_ use case, just a >> theoretic one. > > An example use case is a mobile phone OEM that wishes to set a period of > 5 seconds or more for, say, a low battery slow blinking indication - currently > this is not possible. The PWM framework not having direct support for > blinking should not be a concern if the PWM peripheral being controlled > supports it via register writes. > >> Either case, the reason / use case for this series should be explained >> in the summary patch. That is what it is for. That case is not made. > > Will upload a new patchset adding more details in the summary patch. > Well, let's assume that this is a real use case. Please also add information about alternatives considered, such as keeping the second-part of the period in a separate variable. Guenter
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index 42ffd2e..283423a 100644 --- a/drivers/hwmon/pwm-fan.c +++ b/drivers/hwmon/pwm-fan.c @@ -437,7 +437,7 @@ static int pwm_fan_resume(struct device *dev) return 0; pwm_get_args(ctx->pwm, &pargs); - duty = DIV_ROUND_UP(ctx->pwm_value * (pargs.period - 1), MAX_PWM); + duty = DIV_ROUND_UP_ULL(ctx->pwm_value * (pargs.period - 1), MAX_PWM); ret = pwm_config(ctx->pwm, duty, pargs.period); if (ret) return ret;
Because period and duty cycle are defined in the PWM framework structs as ints with units of nanoseconds, the maximum time duration that can be set is limited to ~2.147 seconds. Redefining them as u64 values will enable larger time durations to be set. As a first step, prepare drivers to handle the switch to u64 period and duty_cycle by replacing division operations involving pwm period and duty cycle with their 64-bit equivalents as appropriate. The actual switch to u64 period and duty_cycle follows as a separate patch. Where the dividend is 64-bit but the divisor is 32-bit, use *_ULL macros: - DIV_ROUND_UP_ULL - DIV_ROUND_CLOSEST_ULL - div_u64 Where the divisor is 64-bit (dividend may be 32-bit or 64-bit), use DIV64_* macros: - DIV64_U64_ROUND_CLOSEST - div64_u64 Cc: Kamil Debski <kamil@wypas.org> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Cc: Jean Delvare <jdelvare@suse.com> Cc: Guenter Roeck <linux@roeck-us.net> Cc: Liam Girdwood <lgirdwood@gmail.com> Cc: Mark Brown <broonie@kernel.org> Cc: linux-hwmon@vger.kernel.org Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org> --- drivers/hwmon/pwm-fan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)