diff mbox series

[v14,17/18] media: i2c: ds90ub953: Restructure clkout management

Message ID 20230616135922.442979-18-tomi.valkeinen@ideasonboard.com
State New
Headers
Series i2c-atr and FPDLink |

Commit Message

Tomi Valkeinen June 16, 2023, 1:59 p.m. UTC 
  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(-)

Comments

Andy Shevchenko June 16, 2023, 2:37 p.m. UTC | #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;
> +}
Tomi Valkeinen June 19, 2023, 10:14 a.m. UTC | #2 
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
Andy Shevchenko June 19, 2023, 10:44 a.m. UTC | #3 
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.)
Tomi Valkeinen June 19, 2023, 11:11 a.m. UTC | #4 
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
Andy Shevchenko June 19, 2023, 11:27 a.m. UTC | #5 
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 :-)
diff mbox series

Patch

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;