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 ad479923d2b4..3a19c6dedd2f 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 Thu, Jul 20, 2023 at 01:30:37PM +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.
>
> This simplifies the clkout management.
...
> +struct ub953_clkout_data {
> + u32 hs_div;
> + u32 m;
> + u32 n;
I don't think it makes driver worse. The V4L2 UAPI has similar struct which is
used widely, hence I see no issues in using u32_fract here.
OTOH I'm not a maintainer, so...
> + unsigned long rate;
> +};
--
With Best Regards,
Andy Shevchenko
On Fri, Jul 21, 2023 at 04:23:54PM +0300, Tomi Valkeinen wrote:
> On 21/07/2023 13:29, Andy Shevchenko wrote:
> > On Thu, Jul 20, 2023 at 01:30:37PM +0300, Tomi Valkeinen wrote:
...
> > > +struct ub953_clkout_data {
> > > + u32 hs_div;
> > > + u32 m;
> > > + u32 n;
> >
> > I don't think it makes driver worse. The V4L2 UAPI has similar struct which is
> > used widely, hence I see no issues in using u32_fract here.
>
> I think it makes sense to use u32_fract in common code. My argument for not
> using it here is:
>
> - There is no actual functionality that u32_fract brings, so it's really
> only about field naming
> - m and n matches the terms in the HW documentation, making it easier to
> compare the code and the docs
> - This is private to the driver
> - I'm (currently) the most likely person to edit the driver, and I would
> have to check which one that numerator/denominator was again when looking at
> this part of the code (but maybe I would learn eventually)
>
> So, in my view, the change doesn't really have any pros but does have cons.
>
> That said, it's not a biggie. If others chime in and say it's a good idea to
> use u32_fract, I'm fine doing that change.
Thank you for a good summary of your point of view.
I agree that others, esp. maintainers, can decide
on how to proceed with this suggestion.
--
With Best Regards,
Andy Shevchenko
On 21/07/2023 13:29, Andy Shevchenko wrote:
> On Thu, Jul 20, 2023 at 01:30:37PM +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.
>>
>> This simplifies the clkout management.
>
> ...
>
>> +struct ub953_clkout_data {
>> + u32 hs_div;
>> + u32 m;
>> + u32 n;
>
> I don't think it makes driver worse. The V4L2 UAPI has similar struct which is
> used widely, hence I see no issues in using u32_fract here.
I think it makes sense to use u32_fract in common code. My argument for
not using it here is:
- There is no actual functionality that u32_fract brings, so it's really
only about field naming
- m and n matches the terms in the HW documentation, making it easier to
compare the code and the docs
- This is private to the driver
- I'm (currently) the most likely person to edit the driver, and I would
have to check which one that numerator/denominator was again when
looking at this part of the code (but maybe I would learn eventually)
So, in my view, the change doesn't really have any pros but does have cons.
That said, it's not a biggie. If others chime in and say it's a good
idea to use u32_fract, I'm fine doing that change.
Tomi
Hi Tomi,
Thank you for the patch.
On Thu, Jul 20, 2023 at 01:30:37PM +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.
>
> 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 ad479923d2b4..3a19c6dedd2f 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)
const
> +{
> + 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);
This line is common to both branches, you could move it after the
conditional part.
> + } 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);
While at it, a macro to replace the numerical constant could be nice.
Reviewed-by: Laurent Pinchart <[email protected]>
> + ub953_write_clkout_regs(priv, &clkout_data);
>
> priv->clkout_clk_hw.init = &init;
>
>
--
Regards,
Laurent Pinchart