pwm: th1520: fix `CLIPPY=1` warning
Subsystems (5)
Miguel Ojeda
maintainerPATCH 1/1 pwm: th1520: fix `CLIPPY=1` warning
The Rust kernel code should be kept CLIPPY=1-clean [1].
Clippy reports:
error: this pattern reimplements `Option::unwrap_or`
--> drivers/pwm/pwm_th1520.rs:64:5
|
64 | / (match ns.checked_mul(rate_hz) {
65 | | Some(product) => product,
66 | | None => u64::MAX,
67 | | }) / NSEC_PER_SEC_U64
| |______^ help: replace with: `ns.checked_mul(rate_hz).unwrap_or(u64::MAX)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#manual_unwrap_or
= note: `-D clippy::manual-unwrap-or` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::manual_unwrap_or)]`Applying the suggestion then triggers:
error: manual saturating arithmetic
--> drivers/pwm/pwm_th1520.rs:64:5
|
64 | ns.checked_mul(rate_hz).unwrap_or(u64::MAX) / NSEC_PER_SEC_U64
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `saturating_mul`: `ns.saturating_mul(rate_hz)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#manual_saturating_arithmetic
= note: `-D clippy::manual-saturating-arithmetic` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::manual_saturating_arithmetic)]`Thus fix it by using saturating arithmatic, which simplifies the code as well.
---
It would be nice to clean this up, so that Mark may start enforcing it in linux-next -- thanks!
Completely untested, so please beware. Also, I am not sure if I am missing something here, since saturation arithmetic is already used a few lines below already.
drivers/pwm/pwm_th1520.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/pwm/pwm_th1520.rs +1 -4
@@ -61,10 +61,7 @@ const fn th1520_pwm_fp(n: u32) -> usize {fn ns_to_cycles (ns:u64 , rate_hz:u64 ) ->u64 {const NSEC_PER_SEC_U64 :u64 = time::NSEC_PER_SEC as u64 ; - (match ns.checked_mul (rate_hz) { -Some (product) => product, -None => u64::MAX , - }) /NSEC_PER_SEC_U64 + ns.saturating_mul (rate_hz) /NSEC_PER_SEC_U64 }fn cycles_to_ns (cycles:u64 , rate_hz:u64 ) ->u64 {
base-commit: e3b32dcb9f23e3c3927ef3eec6a5842a988fb574 --
2.52.0
Quoted 32 lines · Miguel Ojeda
The Rust kernel code should be kept
CLIPPY=1-clean [1].Clippy reports:
error: this pattern reimplements `Option::unwrap_or` --> drivers/pwm/pwm_th1520.rs:64:5 | 64 | / (match ns.checked_mul(rate_hz) { 65 | | Some(product) => product, 66 | | None => u64::MAX, 67 | | }) / NSEC_PER_SEC_U64 | |______^ help: replace with: `ns.checked_mul(rate_hz).unwrap_or(u64::MAX)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#manual_unwrap_or = note: `-D clippy::manual-unwrap-or` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_unwrap_or)]`Applying the suggestion then triggers:
error: manual saturating arithmetic --> drivers/pwm/pwm_th1520.rs:64:5 | 64 | ns.checked_mul(rate_hz).unwrap_or(u64::MAX) / NSEC_PER_SEC_U64 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `saturating_mul`: `ns.saturating_mul(rate_hz)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#manual_saturating_arithmetic = note: `-D clippy::manual-saturating-arithmetic` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_saturating_arithmetic)]`Thus fix it by using saturating arithmatic, which simplifies the code as well.
Quoted 6 lines
Fixes e03724aac758 ("pwm: Add Rust driver for T-HEAD TH1520 SoC")Signed-off-by Miguel Ojeda <[email protected]>---
It would be nice to clean this up, so that Mark may start enforcing it in linux-next -- thanks!
Indeed, the patch itself LGTM.
Quoted 30 lines · Miguel Ojeda
The Rust kernel code should be kept
CLIPPY=1-clean [1].Clippy reports:
error: this pattern reimplements `Option::unwrap_or` --> drivers/pwm/pwm_th1520.rs:64:5 | 64 | / (match ns.checked_mul(rate_hz) { 65 | | Some(product) => product, 66 | | None => u64::MAX, 67 | | }) / NSEC_PER_SEC_U64 | |______^ help: replace with: `ns.checked_mul(rate_hz).unwrap_or(u64::MAX)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#manual_unwrap_or = note: `-D clippy::manual-unwrap-or` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_unwrap_or)]`Applying the suggestion then triggers:
error: manual saturating arithmetic --> drivers/pwm/pwm_th1520.rs:64:5 | 64 | ns.checked_mul(rate_hz).unwrap_or(u64::MAX) / NSEC_PER_SEC_U64 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `saturating_mul`: `ns.saturating_mul(rate_hz)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#manual_saturating_arithmetic = note: `-D clippy::manual-saturating-arithmetic` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_saturating_arithmetic)]`
Quoted 6 lines
Thus fix it by using saturating arithmatic, which simplifies the code as well.
Fixes e03724aac758 ("pwm: Add Rust driver for T-HEAD TH1520 SoC")Signed-off-by Miguel Ojeda <[email protected]>
Pinging about this...
Adding Michal and Uwe to Cc in case it helps (the driver is under "RISC-V THEAD SoC SUPPORT" so they don't get paged).
Thanks!
Cheers,
Miguel
Hello Miguel,
Quoted 29 lines · Miguel Ojeda
The Rust kernel code should be kept
CLIPPY=1-clean [1].Clippy reports:
error: this pattern reimplements `Option::unwrap_or` --> drivers/pwm/pwm_th1520.rs:64:5 | 64 | / (match ns.checked_mul(rate_hz) { 65 | | Some(product) => product, 66 | | None => u64::MAX, 67 | | }) / NSEC_PER_SEC_U64 | |______^ help: replace with: `ns.checked_mul(rate_hz).unwrap_or(u64::MAX)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#manual_unwrap_or = note: `-D clippy::manual-unwrap-or` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_unwrap_or)]`Applying the suggestion then triggers:
error: manual saturating arithmetic --> drivers/pwm/pwm_th1520.rs:64:5 | 64 | ns.checked_mul(rate_hz).unwrap_or(u64::MAX) / NSEC_PER_SEC_U64 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `saturating_mul`: `ns.saturating_mul(rate_hz)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#manual_saturating_arithmetic = note: `-D clippy::manual-saturating-arithmetic` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_saturating_arithmetic)]`
Quoted 7 lines
Thus fix it by using saturating arithmatic, which simplifies the code as well.
Fixes e03724aac758 ("pwm: Add Rust driver for T-HEAD TH1520 SoC")Signed-off-by Miguel Ojeda <[email protected]>---
Looks good to me. I'll wait for Michal to add his blessing and then when picking it up tend to drop the Fixes line. Or do we also care about CLIPPY-cleanness in stable?
Best regards
Uwe
Alice Ryhl
reviewerQuoted 36 lines · Uwe Kleine-König
Hello Miguel,
Quoted 32 lines · Miguel Ojeda
The Rust kernel code should be kept
CLIPPY=1-clean [1].Clippy reports:
error: this pattern reimplements `Option::unwrap_or` --> drivers/pwm/pwm_th1520.rs:64:5 | 64 | / (match ns.checked_mul(rate_hz) { 65 | | Some(product) => product, 66 | | None => u64::MAX, 67 | | }) / NSEC_PER_SEC_U64 | |______^ help: replace with: `ns.checked_mul(rate_hz).unwrap_or(u64::MAX)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#manual_unwrap_or = note: `-D clippy::manual-unwrap-or` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_unwrap_or)]`Applying the suggestion then triggers:
error: manual saturating arithmetic --> drivers/pwm/pwm_th1520.rs:64:5 | 64 | ns.checked_mul(rate_hz).unwrap_or(u64::MAX) / NSEC_PER_SEC_U64 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `saturating_mul`: `ns.saturating_mul(rate_hz)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#manual_saturating_arithmetic = note: `-D clippy::manual-saturating-arithmetic` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_saturating_arithmetic)]`Thus fix it by using saturating arithmatic, which simplifies the code as well.
Quoted 8 lines
Quoted 4 lines
Fixes e03724aac758 ("pwm: Add Rust driver for T-HEAD TH1520 SoC")Signed-off-by Miguel Ojeda <[email protected]>---
Looks good to me. I'll wait for Michal to add his blessing and then when picking it up tend to drop the Fixes line. Or do we also care about CLIPPY-cleanness in stable?
I think we do care about that.
Alice
Quoted 35 lines · Uwe Kleine-König
Hello Miguel,
Quoted 32 lines · Miguel Ojeda
The Rust kernel code should be kept
CLIPPY=1-clean [1].Clippy reports:
error: this pattern reimplements `Option::unwrap_or` --> drivers/pwm/pwm_th1520.rs:64:5 | 64 | / (match ns.checked_mul(rate_hz) { 65 | | Some(product) => product, 66 | | None => u64::MAX, 67 | | }) / NSEC_PER_SEC_U64 | |______^ help: replace with: `ns.checked_mul(rate_hz).unwrap_or(u64::MAX)` | = help: for further information visit https://protect2.fireeye.com/v1/url?k=4a8755d5-151b7cff-4a86de9a-000babe598f7-51498316c06f2766&q=1&e=b527c3ac-a0b8-4074-9bc4-c3497f7913a5&u=https%3A%2F%2Frust-lang.github.io%2Frust-clippy%2Frust-1.92.0%2Findex.html%23manual_unwrap_or = note: `-D clippy::manual-unwrap-or` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_unwrap_or)]`Applying the suggestion then triggers:
error: manual saturating arithmetic --> drivers/pwm/pwm_th1520.rs:64:5 | 64 | ns.checked_mul(rate_hz).unwrap_or(u64::MAX) / NSEC_PER_SEC_U64 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `saturating_mul`: `ns.saturating_mul(rate_hz)` | = help: for further information visit https://protect2.fireeye.com/v1/url?k=7fd7ef65-204bc64f-7fd6642a-000babe598f7-c6165289323e3bed&q=1&e=b527c3ac-a0b8-4074-9bc4-c3497f7913a5&u=https%3A%2F%2Frust-lang.github.io%2Frust-clippy%2Frust-1.92.0%2Findex.html%23manual_saturating_arithmetic = note: `-D clippy::manual-saturating-arithmetic` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_saturating_arithmetic)]`Thus fix it by using saturating arithmatic, which simplifies the code as well.
Quoted 8 lines
Quoted 4 lines
Fixes e03724aac758 ("pwm: Add Rust driver for T-HEAD TH1520 SoC")Signed-off-by Miguel Ojeda <[email protected]>---
Looks good to me. I'll wait for Michal to add his blessing and then when picking it up tend to drop the Fixes line. Or do we also care about CLIPPY-cleanness in stable?
Hmm I'm pretty sure at the time of the writing building with CLIPPY didn't trigger this warning, so I guess it's either a newer version of CLIPPY that I used or in the meantime CLIPPY was updated.
Anyway
Quoted 3 lines
Best regards
Uwe
Best regards,
Signature · 1 line
Michal Wilczynski <[email protected]>
Quoted 4 lines · Uwe Kleine-König
Looks good to me. I'll wait for Michal to add his blessing and then when picking it up tend to drop the Fixes line. Or do we also care about CLIPPY-cleanness in stable?
Yeah, so far we do. At least, back then we decided we would try our best to keep them clean, and I build the stable and LTS kernels with Clippy enabled (and I think some downstream users may do development based on those, so I hope that effort helps).
Worst case, we can always drop that requirement.
In any case, some of these patches, even if not considered Fixes (e.g. a new lint), sometimes they may need to go "urgently" into -fixes branches rather than -next ones, i.e. even without such a tag; and we can also still send them to stable -- what I usually give the stable team is a line like:
Cc: [email protected] # Needed in 6.12.y and later (Rust is
pinned in older LTSs).
I hope that helps!
Cheers,
Miguel
Quoted 4 lines · Uwe Kleine-König
Looks good to me. I'll wait for Michal to add his blessing and then when picking it up tend to drop the Fixes line. Or do we also care about CLIPPY-cleanness in stable?
Do you plan on picking this one up? Michal already reviewed it.
Thanks!
Cheers,
Miguel
Quoted 8 lines · Miguel Ojeda
Quoted 4 lines · Uwe Kleine-König
Looks good to me. I'll wait for Michal to add his blessing and then when picking it up tend to drop the Fixes line. Or do we also care about CLIPPY-cleanness in stable?
Do you plan on picking this one up? Michal already reviewed it.
Hmm, I guess I missed that because that patch isn't on patchwork. I don't even find it in the list of patches that don't require action. (e.g.
https://patchwork.ozlabs.org/project/linux-pwm/list/?submitter=83457&state=%2A&archive=both,
or
https://patchwork.ozlabs.org/project/linux-pwm/list/?series=&submitter=&state=*&q=CLIPPY%3D1&archive=both&delegate=
)
Applied with s/arithmatic/arithmetic/ in the commit log to
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next now.
Sorry for the delay
Uwe
Quoted 8 lines · Uwe Kleine-König
Hmm, I guess I missed that because that patch isn't on patchwork. I don't even find it in the list of patches that don't require action. (e.g.
https://patchwork.ozlabs.org/project/linux-pwm/list/?submitter=83457&state=%2A&archive=both,
or
https://patchwork.ozlabs.org/project/linux-pwm/list/?series=&submitter=&state=*&q=CLIPPY%3D1&archive=both&delegate=
)
Ah, yeah, I track things manually in a spreadsheet (which is a bit insane... :), but this one was noticeable mostly because it appears in CI runs etc.
Quoted 3 lines
Applied with s/arithmatic/arithmetic/ in the commit log to
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next now.
Thanks!
Cheers,
Miguel
Benno Lossin
reviewerQuoted 29 lines · Miguel Ojeda
The Rust kernel code should be kept
CLIPPY=1-clean [1].Clippy reports:
error: this pattern reimplements `Option::unwrap_or` --> drivers/pwm/pwm_th1520.rs:64:5 | 64 | / (match ns.checked_mul(rate_hz) { 65 | | Some(product) => product, 66 | | None => u64::MAX, 67 | | }) / NSEC_PER_SEC_U64 | |______^ help: replace with: `ns.checked_mul(rate_hz).unwrap_or(u64::MAX)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#manual_unwrap_or = note: `-D clippy::manual-unwrap-or` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_unwrap_or)]`Applying the suggestion then triggers:
error: manual saturating arithmetic --> drivers/pwm/pwm_th1520.rs:64:5 | 64 | ns.checked_mul(rate_hz).unwrap_or(u64::MAX) / NSEC_PER_SEC_U64 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `saturating_mul`: `ns.saturating_mul(rate_hz)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#manual_saturating_arithmetic = note: `-D clippy::manual-saturating-arithmetic` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_saturating_arithmetic)]`
Quoted 6 lines
Thus fix it by using saturating arithmatic, which simplifies the code as well.
Fixes e03724aac758 ("pwm: Add Rust driver for T-HEAD TH1520 SoC")Signed-off-by Miguel Ojeda <[email protected]>
Just encountered this during my testing, could we get this picked up please?
Cheers,
Benno
Quoted 12 lines
---
It would be nice to clean this up, so that Mark may start enforcing it in linux-next -- thanks!Completely untested, so please beware. Also, I am not sure if I am missing something here, since saturation arithmetic is already used a few lines below already.
Cc Mark Brown <[email protected]>drivers/pwm/pwm_th1520.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)