| Message ID | 20230720-fpdlink-additions-v2-6-b91b1eca2ad3@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: ds90ub9xx: Misc improvements | expand |
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; > +};
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
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.
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 <tomi.valkeinen@ideasonboard.com> > --- > 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 <laurent.pinchart@ideasonboard.com> > + ub953_write_clkout_regs(priv, &clkout_data); > > priv->clkout_clk_hw.init = &init; > >
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;
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 <tomi.valkeinen@ideasonboard.com> --- drivers/media/i2c/ds90ub953.c | 135 ++++++++++++++++++++++-------------------- 1 file changed, 70 insertions(+), 65 deletions(-)