| Message ID | 20220125141549.747889-4-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: Improve clock range handling | expand |
Quoting Maxime Ripard (2022-01-25 06:15:42) > The code in clk_set_rate_range() will, if the current rate is outside of > the new range, will force it to the minimum or maximum. This is > equivalent to using clamp, while being less readable. Let's switch to > using clamp instead. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/clk/clk.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 7bb5ae0fb688..150d1bc0985b 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -2365,11 +2365,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max) > * this corner case when determining the rate > */ > > - if (rate < min) > - rate = min; > - else > - rate = max; > - > + rate = clamp(clk->core->req_rate, min, max); This isn't equivalent. The else arm is taken if rate >= min and rate is set to max, whereas clamp() will leave the rate unchanged if rate >= min && rate < max.
On Fri, Feb 18, 2022 at 02:34:20PM -0800, Stephen Boyd wrote: > Quoting Maxime Ripard (2022-01-25 06:15:42) > > The code in clk_set_rate_range() will, if the current rate is outside of > > the new range, will force it to the minimum or maximum. This is > > equivalent to using clamp, while being less readable. Let's switch to > > using clamp instead. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/clk/clk.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index 7bb5ae0fb688..150d1bc0985b 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -2365,11 +2365,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max) > > * this corner case when determining the rate > > */ > > > > - if (rate < min) > > - rate = min; > > - else > > - rate = max; > > - > > + rate = clamp(clk->core->req_rate, min, max); > > This isn't equivalent. The else arm is taken if rate >= min and rate is > set to max, whereas clamp() will leave the rate unchanged if rate >= min > && rate < max. This can't happen, since we're in an if block that is (rate < min || rate > max), so at this point if rate is not less than min, it is greater than rate. Thus, it's equivalent to clamp. Still, the commit message could be better, I'll rephrase it. Maxime
Quoting Maxime Ripard (2022-02-21 08:30:01) > On Fri, Feb 18, 2022 at 02:34:20PM -0800, Stephen Boyd wrote: > > Quoting Maxime Ripard (2022-01-25 06:15:42) > > > The code in clk_set_rate_range() will, if the current rate is outside of > > > the new range, will force it to the minimum or maximum. This is > > > equivalent to using clamp, while being less readable. Let's switch to > > > using clamp instead. > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > --- > > > drivers/clk/clk.c | 6 +----- > > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > index 7bb5ae0fb688..150d1bc0985b 100644 > > > --- a/drivers/clk/clk.c > > > +++ b/drivers/clk/clk.c > > > @@ -2365,11 +2365,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max) > > > * this corner case when determining the rate > > > */ > > > > > > - if (rate < min) > > > - rate = min; > > > - else > > > - rate = max; > > > - > > > + rate = clamp(clk->core->req_rate, min, max); > > > > This isn't equivalent. The else arm is taken if rate >= min and rate is > > set to max, whereas clamp() will leave the rate unchanged if rate >= min > > && rate < max. > > This can't happen, since we're in an if block that is (rate < min || > rate > max), so at this point if rate is not less than min, it is > greater than rate. Thus, it's equivalent to clamp. > > Still, the commit message could be better, I'll rephrase it. Perfect! Should probably add a comment above the clamp as well just in case someone decides to move it out of that if block.
On Thu, Feb 24, 2022 at 02:44:20PM -0800, Stephen Boyd wrote: > Quoting Maxime Ripard (2022-02-21 08:30:01) > > On Fri, Feb 18, 2022 at 02:34:20PM -0800, Stephen Boyd wrote: > > > Quoting Maxime Ripard (2022-01-25 06:15:42) > > > > The code in clk_set_rate_range() will, if the current rate is outside of > > > > the new range, will force it to the minimum or maximum. This is > > > > equivalent to using clamp, while being less readable. Let's switch to > > > > using clamp instead. > > > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > > --- > > > > drivers/clk/clk.c | 6 +----- > > > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > > index 7bb5ae0fb688..150d1bc0985b 100644 > > > > --- a/drivers/clk/clk.c > > > > +++ b/drivers/clk/clk.c > > > > @@ -2365,11 +2365,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max) > > > > * this corner case when determining the rate > > > > */ > > > > > > > > - if (rate < min) > > > > - rate = min; > > > > - else > > > > - rate = max; > > > > - > > > > + rate = clamp(clk->core->req_rate, min, max); > > > > > > This isn't equivalent. The else arm is taken if rate >= min and rate is > > > set to max, whereas clamp() will leave the rate unchanged if rate >= min > > > && rate < max. > > > > This can't happen, since we're in an if block that is (rate < min || > > rate > max), so at this point if rate is not less than min, it is > > greater than rate. Thus, it's equivalent to clamp. > > > > Still, the commit message could be better, I'll rephrase it. > > Perfect! Should probably add a comment above the clamp as well just in > case someone decides to move it out of that if block. The last version has a better commit message. We're actually moving that out of the if in the next patch. I'm not sure we really need a comment for this though: The existing code does: if (rate < min || rate > max) if (rate < min) rate = min; else rate = max; So if the rate is below min, it's set to min, if it's between min and max, it's unaffected, and if it's above max, it's set to max. With this patch, the code does: if (rate < min || rate > max) rate = clamp(rate, min, max) So if the rate is below min, it's set to min, if it's between min and max, it's unaffected, and if it's above max, it's set to max. It's equivalent. After the next patch, the code will do: rate = clamp(rate, min, max) So if the rate is below min, it's set to min, if it's between min and max, it's unaffected, and if it's above max, it's set to max. They are all equivalent. Maxime
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 7bb5ae0fb688..150d1bc0985b 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2365,11 +2365,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max) * this corner case when determining the rate */ - if (rate < min) - rate = min; - else - rate = max; - + rate = clamp(clk->core->req_rate, min, max); ret = clk_core_set_rate_nolock(clk->core, rate); if (ret) { /* rollback the changes */
The code in clk_set_rate_range() will, if the current rate is outside of the new range, will force it to the minimum or maximum. This is equivalent to using clamp, while being less readable. Let's switch to using clamp instead. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/clk/clk.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)