The conversion to CLK_FRAC_DIVIDER_POWER_OF_TWO_PS uses wrong flags
in the parameters and hence miscalculates the values in the clock
divider. Fix this by applying the flag to the proper parameter.
Fixes: 82f53f9ee577 ("clk: fractional-divider: Introduce POWER_OF_TWO_PS flag")
Reported-by: Alex Vinarskis <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/mfd/intel-lpss.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
index 177915845ba2..eff423f7dd28 100644
--- a/drivers/mfd/intel-lpss.c
+++ b/drivers/mfd/intel-lpss.c
@@ -309,8 +309,8 @@ static int intel_lpss_register_clock_divider(struct intel_lpss *lpss,
snprintf(name, sizeof(name), "%s-div", devname);
tmp = clk_register_fractional_divider(NULL, name, __clk_get_name(tmp),
+ 0, lpss->priv, 1, 15, 16, 15,
CLK_FRAC_DIVIDER_POWER_OF_TWO_PS,
- lpss->priv, 1, 15, 16, 15, 0,
NULL);
if (IS_ERR(tmp))
return PTR_ERR(tmp);
--
2.43.0.rc1.1.gbec44491f096
On Mon, 11 Dec 2023, Andy Shevchenko wrote:
> The conversion to CLK_FRAC_DIVIDER_POWER_OF_TWO_PS uses wrong flags
> in the parameters and hence miscalculates the values in the clock
> divider. Fix this by applying the flag to the proper parameter.
>
> Fixes: 82f53f9ee577 ("clk: fractional-divider: Introduce POWER_OF_TWO_PS flag")
> Reported-by: Alex Vinarskis <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/mfd/intel-lpss.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> index 177915845ba2..eff423f7dd28 100644
> --- a/drivers/mfd/intel-lpss.c
> +++ b/drivers/mfd/intel-lpss.c
> @@ -309,8 +309,8 @@ static int intel_lpss_register_clock_divider(struct intel_lpss *lpss,
>
> snprintf(name, sizeof(name), "%s-div", devname);
> tmp = clk_register_fractional_divider(NULL, name, __clk_get_name(tmp),
> + 0, lpss->priv, 1, 15, 16, 15,
> CLK_FRAC_DIVIDER_POWER_OF_TWO_PS,
> - lpss->priv, 1, 15, 16, 15, 0,
> NULL);
What an ugly interface. Intel-only too, right?
Have you also fixed this in: drivers/acpi/acpi_lpss.c
> if (IS_ERR(tmp))
> return PTR_ERR(tmp);
> --
> 2.43.0.rc1.1.gbec44491f096
>
--
Lee Jones [李琼斯]
On Wed, Dec 13, 2023 at 04:13:52PM +0000, Lee Jones wrote:
> On Mon, 11 Dec 2023, Andy Shevchenko wrote:
...
> > tmp = clk_register_fractional_divider(NULL, name, __clk_get_name(tmp),
> > + 0, lpss->priv, 1, 15, 16, 15,
> > CLK_FRAC_DIVIDER_POWER_OF_TWO_PS,
> > - lpss->priv, 1, 15, 16, 15, 0,
> > NULL);
>
> What an ugly interface. Intel-only too, right?
Nope, de facto way how custom clocks are being introduced.
See clk-provider.h for several similar APIs (that require an
additional, custom, flags to be supplied).
...
> Have you also fixed this in: drivers/acpi/acpi_lpss.c
Already pending in Rafael's tree, yes.
--
With Best Regards,
Andy Shevchenko
On Wed, 13 Dec 2023, Andy Shevchenko wrote:
> On Wed, Dec 13, 2023 at 04:13:52PM +0000, Lee Jones wrote:
> > On Mon, 11 Dec 2023, Andy Shevchenko wrote:
>
> ...
>
> > > tmp = clk_register_fractional_divider(NULL, name, __clk_get_name(tmp),
> > > + 0, lpss->priv, 1, 15, 16, 15,
> > > CLK_FRAC_DIVIDER_POWER_OF_TWO_PS,
> > > - lpss->priv, 1, 15, 16, 15, 0,
> > > NULL);
> >
> > What an ugly interface. Intel-only too, right?
>
> Nope, de facto way how custom clocks are being introduced.
> See clk-provider.h for several similar APIs (that require an
> additional, custom, flags to be supplied).
This call only has 2 call-sites, both Intel.
Anyway, just checking to ensure both are being fixed-up.
--
Lee Jones [李琼斯]
On Mon, 11 Dec 2023 13:14:41 +0200, Andy Shevchenko wrote:
> The conversion to CLK_FRAC_DIVIDER_POWER_OF_TWO_PS uses wrong flags
> in the parameters and hence miscalculates the values in the clock
> divider. Fix this by applying the flag to the proper parameter.
>
>
Applied, thanks!
[1/1] mfd: intel-lpss: Fix the fractional clock divider flags
commit: 03d790f04fb2507173913cad9c213272ac983a60
--
Lee Jones [李琼斯]
On Wed, Dec 13, 2023 at 04:18:54PM +0000, Lee Jones wrote:
> On Wed, 13 Dec 2023, Andy Shevchenko wrote:
> > On Wed, Dec 13, 2023 at 04:13:52PM +0000, Lee Jones wrote:
> > > On Mon, 11 Dec 2023, Andy Shevchenko wrote:
...
> > > > tmp = clk_register_fractional_divider(NULL, name, __clk_get_name(tmp),
> > > > + 0, lpss->priv, 1, 15, 16, 15,
> > > > CLK_FRAC_DIVIDER_POWER_OF_TWO_PS,
> > > > - lpss->priv, 1, 15, 16, 15, 0,
> > > > NULL);
> > >
> > > What an ugly interface. Intel-only too, right?
> >
> > Nope, de facto way how custom clocks are being introduced.
> > See clk-provider.h for several similar APIs (that require an
> > additional, custom, flags to be supplied).
>
> This call only has 2 call-sites, both Intel.
Yes, but the clock fractional divider is used wider.
And again, it's not related to Intel, as this how clock framework
does the custom clocks. I don't know how to say this clearer.
Whatever, thank you for review and applying this fix!
--
With Best Regards,
Andy Shevchenko
On Wed, 13 Dec 2023, Andy Shevchenko wrote:
> On Wed, Dec 13, 2023 at 04:18:54PM +0000, Lee Jones wrote:
> > On Wed, 13 Dec 2023, Andy Shevchenko wrote:
> > > On Wed, Dec 13, 2023 at 04:13:52PM +0000, Lee Jones wrote:
> > > > On Mon, 11 Dec 2023, Andy Shevchenko wrote:
>
> ...
>
> > > > > tmp = clk_register_fractional_divider(NULL, name, __clk_get_name(tmp),
> > > > > + 0, lpss->priv, 1, 15, 16, 15,
> > > > > CLK_FRAC_DIVIDER_POWER_OF_TWO_PS,
> > > > > - lpss->priv, 1, 15, 16, 15, 0,
> > > > > NULL);
> > > >
> > > > What an ugly interface. Intel-only too, right?
> > >
> > > Nope, de facto way how custom clocks are being introduced.
> > > See clk-provider.h for several similar APIs (that require an
> > > additional, custom, flags to be supplied).
> >
> > This call only has 2 call-sites, both Intel.
>
> Yes, but the clock fractional divider is used wider.
>
> And again, it's not related to Intel, as this how clock framework
> does the custom clocks. I don't know how to say this clearer.
I'm not sure how you can say that. Intel were the authors, hold the
_only_ copyright and are the _only_ users. If it were to be removed,
there is only a single entity that would even notice.
Anyway, it was just a passing comment. Not positive, not negative.
--
Lee Jones [李琼斯]
On Wed, Dec 13, 2023 at 05:46:05PM +0000, Lee Jones wrote:
> On Wed, 13 Dec 2023, Andy Shevchenko wrote:
> > On Wed, Dec 13, 2023 at 04:18:54PM +0000, Lee Jones wrote:
> > > On Wed, 13 Dec 2023, Andy Shevchenko wrote:
> > > > On Wed, Dec 13, 2023 at 04:13:52PM +0000, Lee Jones wrote:
> > > > > On Mon, 11 Dec 2023, Andy Shevchenko wrote:
...
> > > > > > tmp = clk_register_fractional_divider(NULL, name, __clk_get_name(tmp),
> > > > > > + 0, lpss->priv, 1, 15, 16, 15,
> > > > > > CLK_FRAC_DIVIDER_POWER_OF_TWO_PS,
> > > > > > - lpss->priv, 1, 15, 16, 15, 0,
> > > > > > NULL);
> > > > >
> > > > > What an ugly interface. Intel-only too, right?
> > > >
> > > > Nope, de facto way how custom clocks are being introduced.
> > > > See clk-provider.h for several similar APIs (that require an
> > > > additional, custom, flags to be supplied).
> > >
> > > This call only has 2 call-sites, both Intel.
> >
> > Yes, but the clock fractional divider is used wider.
> >
> > And again, it's not related to Intel, as this how clock framework
> > does the custom clocks. I don't know how to say this clearer.
>
> I'm not sure how you can say that. Intel were the authors, hold the
> _only_ copyright and are the _only_ users. If it were to be removed,
> there is only a single entity that would even notice.
_This_ API is indeed used by only Intel code right now, but the _design_
of the API is dictated by CCF, and not anyhow related to Intel.
> Anyway, it was just a passing comment. Not positive, not negative.
Okay!
--
With Best Regards,
Andy Shevchenko