Separate clkout calculations and register writes into two functions:
ub953_calc_clkout_params and ub953_write_clkout_regs, and add a struct
ub953_clkout_data that is used to store the clkout parameters.
This simplifies the clkout management.
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/media/i2c/ds90ub953.c | 135 ++++++++++++++++++----------------
1 file changed, 70 insertions(+), 65 deletions(-)
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index f2e950a00d64..ff55740965eb 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -131,6 +131,13 @@ struct ub953_hw_data {
bool is_ub971;
};
+struct ub953_clkout_data {
+ u32 hs_div;
+ u32 m;
+ u32 n;
+ unsigned long rate;
+};
+
struct ub953_data {
const struct ub953_hw_data *hw_data;
@@ -906,6 +913,58 @@ static unsigned long ub953_calc_clkout_ub971(struct ub953_data *priv,
return res;
}
+static void ub953_calc_clkout_params(struct ub953_data *priv,
+ unsigned long target_rate,
+ struct ub953_clkout_data *clkout_data)
+{
+ struct device *dev = &priv->client->dev;
+ unsigned long clkout_rate;
+ u64 fc_rate;
+
+ fc_rate = ub953_get_fc_rate(priv);
+
+ if (priv->hw_data->is_ub971) {
+ u8 m, n;
+
+ clkout_rate = ub953_calc_clkout_ub971(priv, target_rate,
+ fc_rate, &m, &n);
+
+ clkout_data->m = m;
+ clkout_data->n = n;
+
+ dev_dbg(dev, "%s %llu * %u / (8 * %u) = %lu (requested %lu)",
+ __func__, fc_rate, m, n, clkout_rate, target_rate);
+ } else {
+ u8 hs_div, m, n;
+
+ clkout_rate = ub953_calc_clkout_ub953(priv, target_rate,
+ fc_rate, &hs_div, &m, &n);
+
+ clkout_data->hs_div = hs_div;
+ clkout_data->m = m;
+ clkout_data->n = n;
+
+ dev_dbg(dev, "%s %llu / %u * %u / %u = %lu (requested %lu)",
+ __func__, fc_rate, hs_div, m, n, clkout_rate,
+ target_rate);
+ }
+
+ clkout_data->rate = clkout_rate;
+}
+
+static void ub953_write_clkout_regs(struct ub953_data *priv,
+ struct ub953_clkout_data *clkout_data)
+{
+ if (priv->hw_data->is_ub971) {
+ ub953_write(priv, UB953_REG_CLKOUT_CTRL0, clkout_data->m);
+ ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_data->n);
+ } else {
+ ub953_write(priv, UB953_REG_CLKOUT_CTRL0,
+ (__ffs(clkout_data->hs_div) << 5) | clkout_data->m);
+ ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_data->n);
+ }
+}
+
static unsigned long ub953_clkout_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)
{
@@ -965,52 +1024,25 @@ static long ub953_clkout_round_rate(struct clk_hw *hw, unsigned long rate,
unsigned long *parent_rate)
{
struct ub953_data *priv = container_of(hw, struct ub953_data, clkout_clk_hw);
- struct device *dev = &priv->client->dev;
- unsigned long res;
- u64 fc_rate;
- u8 hs_div, m, n;
-
- fc_rate = ub953_get_fc_rate(priv);
+ struct ub953_clkout_data clkout_data;
- if (priv->hw_data->is_ub971) {
- res = ub953_calc_clkout_ub971(priv, rate, fc_rate, &m, &n);
+ ub953_calc_clkout_params(priv, rate, &clkout_data);
- dev_dbg(dev, "%s %llu * %u / (8 * %u) = %lu (requested %lu)",
- __func__, fc_rate, m, n, res, rate);
- } else {
- res = ub953_calc_clkout_ub953(priv, rate, fc_rate, &hs_div, &m, &n);
-
- dev_dbg(dev, "%s %llu / %u * %u / %u = %lu (requested %lu)",
- __func__, fc_rate, hs_div, m, n, res, rate);
- }
-
- return res;
+ return clkout_data.rate;
}
static int ub953_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
unsigned long parent_rate)
{
struct ub953_data *priv = container_of(hw, struct ub953_data, clkout_clk_hw);
- u64 fc_rate;
- u8 hs_div, m, n;
- unsigned long res;
+ struct ub953_clkout_data clkout_data;
- fc_rate = ub953_get_fc_rate(priv);
-
- if (priv->hw_data->is_ub971) {
- res = ub953_calc_clkout_ub971(priv, rate, fc_rate, &m, &n);
-
- ub953_write(priv, UB953_REG_CLKOUT_CTRL0, m);
- ub953_write(priv, UB953_REG_CLKOUT_CTRL1, n);
- } else {
- res = ub953_calc_clkout_ub953(priv, rate, fc_rate, &hs_div, &m, &n);
+ ub953_calc_clkout_params(priv, rate, &clkout_data);
- ub953_write(priv, UB953_REG_CLKOUT_CTRL0, (__ffs(hs_div) << 5) | m);
- ub953_write(priv, UB953_REG_CLKOUT_CTRL1, n);
- }
+ dev_dbg(&priv->client->dev, "%s %lu (requested %lu)\n", __func__,
+ clkout_data.rate, rate);
- dev_dbg(&priv->client->dev, "%s %lu (requested %lu)\n", __func__, res,
- rate);
+ ub953_write_clkout_regs(priv, &clkout_data);
return 0;
}
@@ -1021,32 +1053,6 @@ static const struct clk_ops ub953_clkout_ops = {
.set_rate = ub953_clkout_set_rate,
};
-static void ub953_init_clkout_ub953(struct ub953_data *priv)
-{
- u64 fc_rate;
- u8 hs_div, m, n;
-
- fc_rate = ub953_get_fc_rate(priv);
-
- ub953_calc_clkout_ub953(priv, 25000000, fc_rate, &hs_div, &m, &n);
-
- ub953_write(priv, UB953_REG_CLKOUT_CTRL0, (__ffs(hs_div) << 5) | m);
- ub953_write(priv, UB953_REG_CLKOUT_CTRL1, n);
-}
-
-static void ub953_init_clkout_ub971(struct ub953_data *priv)
-{
- u64 fc_rate;
- u8 m, n;
-
- fc_rate = ub953_get_fc_rate(priv);
-
- ub953_calc_clkout_ub971(priv, 25000000, fc_rate, &m, &n);
-
- ub953_write(priv, UB953_REG_CLKOUT_CTRL0, m);
- ub953_write(priv, UB953_REG_CLKOUT_CTRL1, n);
-}
-
static int ub953_register_clkout(struct ub953_data *priv)
{
struct device *dev = &priv->client->dev;
@@ -1055,16 +1061,15 @@ static int ub953_register_clkout(struct ub953_data *priv)
priv->hw_data->model, dev_name(dev)),
.ops = &ub953_clkout_ops,
};
+ struct ub953_clkout_data clkout_data;
int ret;
if (!init.name)
return -ENOMEM;
/* Initialize clkout to 25MHz by default */
- if (priv->hw_data->is_ub971)
- ub953_init_clkout_ub971(priv);
- else
- ub953_init_clkout_ub953(priv);
+ ub953_calc_clkout_params(priv, 25000000, &clkout_data);
+ ub953_write_clkout_regs(priv, &clkout_data);
priv->clkout_clk_hw.init = &init;
--
2.34.1
On Fri, Jun 16, 2023 at 04:59:21PM +0300, Tomi Valkeinen wrote:
> Separate clkout calculations and register writes into two functions:
> ub953_calc_clkout_params and ub953_write_clkout_regs, and add a struct
> ub953_clkout_data that is used to store the clkout parameters.
...
> +struct ub953_clkout_data {
> + u32 hs_div;
> + u32 m;
> + u32 n;
Please, use struxt u32_fract instead of m/n.
> + unsigned long rate;
> +};
...
> +static void ub953_calc_clkout_params(struct ub953_data *priv,
> + unsigned long target_rate,
> + struct ub953_clkout_data *clkout_data)
> +{
> + struct device *dev = &priv->client->dev;
> + unsigned long clkout_rate;
> + u64 fc_rate;
> +
> + fc_rate = ub953_get_fc_rate(priv);
> +
> + if (priv->hw_data->is_ub971) {
> + u8 m, n;
Ditto. struct u8_fract; (But probably needs to be added into math.h.
I'll Ack/Rb such a patch when one appears.
> + clkout_rate = ub953_calc_clkout_ub971(priv, target_rate,
> + fc_rate, &m, &n);
Can be also parameter to that function.
> + clkout_data->m = m;
> + clkout_data->n = n;
> +
> + dev_dbg(dev, "%s %llu * %u / (8 * %u) = %lu (requested %lu)",
> + __func__, fc_rate, m, n, clkout_rate, target_rate);
> + } else {
> + u8 hs_div, m, n;
Yeah, and so on...
> + clkout_rate = ub953_calc_clkout_ub953(priv, target_rate,
> + fc_rate, &hs_div, &m, &n);
> +
> + clkout_data->hs_div = hs_div;
> + clkout_data->m = m;
> + clkout_data->n = n;
> +
> + dev_dbg(dev, "%s %llu / %u * %u / %u = %lu (requested %lu)",
> + __func__, fc_rate, hs_div, m, n, clkout_rate,
> + target_rate);
> + }
> +
> + clkout_data->rate = clkout_rate;
> +}
--
With Best Regards,
Andy Shevchenko
On 16/06/2023 17:37, Andy Shevchenko wrote:
> On Fri, Jun 16, 2023 at 04:59:21PM +0300, Tomi Valkeinen wrote:
>> Separate clkout calculations and register writes into two functions:
>> ub953_calc_clkout_params and ub953_write_clkout_regs, and add a struct
>> ub953_clkout_data that is used to store the clkout parameters.
>
> ...
>
>> +struct ub953_clkout_data {
>> + u32 hs_div;
>> + u32 m;
>> + u32 n;
>
> Please, use struxt u32_fract instead of m/n.
I'm not sure how that helps. The documentation talks about m and n.
Using different terms will make it more difficult to compare the code
and the docs.
Tomi
On Mon, Jun 19, 2023 at 01:14:34PM +0300, Tomi Valkeinen wrote:
> On 16/06/2023 17:37, Andy Shevchenko wrote:
> > On Fri, Jun 16, 2023 at 04:59:21PM +0300, Tomi Valkeinen wrote:
> > > Separate clkout calculations and register writes into two functions:
> > > ub953_calc_clkout_params and ub953_write_clkout_regs, and add a struct
> > > ub953_clkout_data that is used to store the clkout parameters.
...
> > > +struct ub953_clkout_data {
> > > + u32 hs_div;
> > > + u32 m;
> > > + u32 n;
> >
> > Please, use struxt u32_fract instead of m/n.
>
> I'm not sure how that helps. The documentation talks about m and n. Using
> different terms will make it more difficult to compare the code and the
> docs.
You can always add a comment.
(For example in drivers/clk/clk-fractional-divider.c our documentation also
says about m/n, but most of the people understands that this is about
fractional divider and actually with properly spelled parameters it will
help others who are not experts in the CLK hardware.)
--
With Best Regards,
Andy Shevchenko
On 19/06/2023 13:44, Andy Shevchenko wrote:
> On Mon, Jun 19, 2023 at 01:14:34PM +0300, Tomi Valkeinen wrote:
>> On 16/06/2023 17:37, Andy Shevchenko wrote:
>>> On Fri, Jun 16, 2023 at 04:59:21PM +0300, Tomi Valkeinen wrote:
>>>> Separate clkout calculations and register writes into two functions:
>>>> ub953_calc_clkout_params and ub953_write_clkout_regs, and add a struct
>>>> ub953_clkout_data that is used to store the clkout parameters.
>
> ...
>
>>>> +struct ub953_clkout_data {
>>>> + u32 hs_div;
>>>> + u32 m;
>>>> + u32 n;
>>>
>>> Please, use struxt u32_fract instead of m/n.
>>
>> I'm not sure how that helps. The documentation talks about m and n. Using
>> different terms will make it more difficult to compare the code and the
>> docs.
>
> You can always add a comment.
>
> (For example in drivers/clk/clk-fractional-divider.c our documentation also
> says about m/n, but most of the people understands that this is about
> fractional divider and actually with properly spelled parameters it will
> help others who are not experts in the CLK hardware.)
Yes, I would agree with you if this was a generic piece of code, but I
don't see the reasoning as valid as this is specific to a piece of
hardware. Here, I think, matching the code to the HW documentation is
more important than possibly making the code more understandable to
people who are not familiar with the HW.
Especially as for non-English people seeing "numerator" and
"denominator" might require a check to figure out which one is which,
whereas m and n are (I would guess) a bit more universal (or maybe it's
just me).
Tomi
On Mon, Jun 19, 2023 at 02:11:09PM +0300, Tomi Valkeinen wrote:
> On 19/06/2023 13:44, Andy Shevchenko wrote:
> > On Mon, Jun 19, 2023 at 01:14:34PM +0300, Tomi Valkeinen wrote:
> > > On 16/06/2023 17:37, Andy Shevchenko wrote:
> > > > On Fri, Jun 16, 2023 at 04:59:21PM +0300, Tomi Valkeinen wrote:
...
> > > > > +struct ub953_clkout_data {
> > > > > + u32 hs_div;
> > > > > + u32 m;
> > > > > + u32 n;
> > > >
> > > > Please, use struxt u32_fract instead of m/n.
> > >
> > > I'm not sure how that helps. The documentation talks about m and n. Using
> > > different terms will make it more difficult to compare the code and the
> > > docs.
> >
> > You can always add a comment.
> >
> > (For example in drivers/clk/clk-fractional-divider.c our documentation also
> > says about m/n, but most of the people understands that this is about
> > fractional divider and actually with properly spelled parameters it will
> > help others who are not experts in the CLK hardware.)
>
> Yes, I would agree with you if this was a generic piece of code, but I don't
> see the reasoning as valid as this is specific to a piece of hardware. Here,
> I think, matching the code to the HW documentation is more important than
> possibly making the code more understandable to people who are not familiar
> with the HW.
>
> Especially as for non-English people seeing "numerator" and "denominator"
> might require a check to figure out which one is which, whereas m and n are
> (I would guess) a bit more universal (or maybe it's just me).
I think for unprepared user the English plain words are simpler,
or maybe it's just me.
At least we have a strong disagreement here, seems :-)
--
With Best Regards,
Andy Shevchenko