[v2,6/8] media: i2c: ds90ub953: Restructure clkout management

  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(-)
  

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;
>  
>