diff mbox series

[13/13] media: i2c: ds90ub953: Add error handling for i2c reads/writes

Message ID 20241004-ub9xx-fixes-v1-13-e30a4633c786@ideasonboard.com
State Superseded
Headers show
Series media: i2c: ds90ub9xx: Misc fixes and improvements | expand

Commit Message

Tomi Valkeinen Oct. 4, 2024, 2:46 p.m. UTC 
Add error handling for i2c reads/writes in various places.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub953.c | 46 ++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 14 deletions(-)

Comments

Andy Shevchenko Oct. 10, 2024, 2:06 p.m. UTC | #1 
On Fri, Oct 04, 2024 at 05:46:44PM +0300, Tomi Valkeinen wrote:
> Add error handling for i2c reads/writes in various places.

...

> +	ret = ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

This is just a more verbose version of

	return ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);

...

> -	ub953_write_clkout_regs(priv, &clkout_data);
> -
> -	return 0;
> +	return ub953_write_clkout_regs(priv, &clkout_data);

...and seems you use that pattern.
Tomi Valkeinen Nov. 8, 2024, 9:34 a.m. UTC | #2 
Hi,

On 10/10/2024 17:06, Andy Shevchenko wrote:
> On Fri, Oct 04, 2024 at 05:46:44PM +0300, Tomi Valkeinen wrote:
>> Add error handling for i2c reads/writes in various places.
> 
> ...
> 
>> +	ret = ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> This is just a more verbose version of
> 
> 	return ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);
> 
> ...
> 
>> -	ub953_write_clkout_regs(priv, &clkout_data);
>> -
>> -	return 0;
>> +	return ub953_write_clkout_regs(priv, &clkout_data);
> 
> ...and seems you use that pattern.

I use the pattern selectively =).

If the function has a bunch of

ret = foo()
if (ret)
	return ret;

blocks, I want to keep the pattern and thus I don't use "return foo();" 
as the last line.

Also, I think, I usually like to use "return foo();" only with small 
functions, as the function call becomes less visible with that format.

  Tomi
diff mbox series

Patch

diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index b6451811f906..f8f3e31f0077 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -401,8 +401,13 @@  static int ub953_gpiochip_probe(struct ub953_data *priv)
 	int ret;
 
 	/* Set all GPIOs to local input mode */
-	ub953_write(priv, UB953_REG_LOCAL_GPIO_DATA, 0);
-	ub953_write(priv, UB953_REG_GPIO_INPUT_CTRL, 0xf);
+	ret = ub953_write(priv, UB953_REG_LOCAL_GPIO_DATA, 0);
+	if (ret)
+		return ret;
+
+	ret = ub953_write(priv, UB953_REG_GPIO_INPUT_CTRL, 0xf);
+	if (ret)
+		return ret;
 
 	gc->label = dev_name(dev);
 	gc->parent = dev;
@@ -970,10 +975,11 @@  static void ub953_calc_clkout_params(struct ub953_data *priv,
 	clkout_data->rate = clkout_rate;
 }
 
-static void ub953_write_clkout_regs(struct ub953_data *priv,
-				    const struct ub953_clkout_data *clkout_data)
+static int ub953_write_clkout_regs(struct ub953_data *priv,
+				   const struct ub953_clkout_data *clkout_data)
 {
 	u8 clkout_ctrl0, clkout_ctrl1;
+	int ret;
 
 	if (priv->hw_data->is_ub971)
 		clkout_ctrl0 = clkout_data->m;
@@ -983,8 +989,15 @@  static void ub953_write_clkout_regs(struct ub953_data *priv,
 
 	clkout_ctrl1 = clkout_data->n;
 
-	ub953_write(priv, UB953_REG_CLKOUT_CTRL0, clkout_ctrl0);
-	ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);
+	ret = ub953_write(priv, UB953_REG_CLKOUT_CTRL0, clkout_ctrl0);
+	if (ret)
+		return ret;
+
+	ret = ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);
+	if (ret)
+		return ret;
+
+	return 0;
 }
 
 static unsigned long ub953_clkout_recalc_rate(struct clk_hw *hw,
@@ -1064,9 +1077,7 @@  static int ub953_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
 	dev_dbg(&priv->client->dev, "%s %lu (requested %lu)\n", __func__,
 		clkout_data.rate, rate);
 
-	ub953_write_clkout_regs(priv, &clkout_data);
-
-	return 0;
+	return ub953_write_clkout_regs(priv, &clkout_data);
 }
 
 static const struct clk_ops ub953_clkout_ops = {
@@ -1091,7 +1102,9 @@  static int ub953_register_clkout(struct ub953_data *priv)
 
 	/* Initialize clkout to 25MHz by default */
 	ub953_calc_clkout_params(priv, UB953_DEFAULT_CLKOUT_RATE, &clkout_data);
-	ub953_write_clkout_regs(priv, &clkout_data);
+	ret = ub953_write_clkout_regs(priv, &clkout_data);
+	if (ret)
+		return ret;
 
 	priv->clkout_clk_hw.init = &init;
 
@@ -1238,10 +1251,15 @@  static int ub953_hw_init(struct ub953_data *priv)
 	if (ret)
 		return dev_err_probe(dev, ret, "i2c init failed\n");
 
-	ub953_write(priv, UB953_REG_GENERAL_CFG,
-		    (priv->non_continous_clk ? 0 : UB953_REG_GENERAL_CFG_CONT_CLK) |
-		    ((priv->num_data_lanes - 1) << UB953_REG_GENERAL_CFG_CSI_LANE_SEL_SHIFT) |
-		    UB953_REG_GENERAL_CFG_CRC_TX_GEN_ENABLE);
+	v = 0;
+	v |= priv->non_continous_clk ? 0 : UB953_REG_GENERAL_CFG_CONT_CLK;
+	v |= (priv->num_data_lanes - 1) <<
+		UB953_REG_GENERAL_CFG_CSI_LANE_SEL_SHIFT;
+	v |= UB953_REG_GENERAL_CFG_CRC_TX_GEN_ENABLE;
+
+	ret = ub953_write(priv, UB953_REG_GENERAL_CFG, v);
+	if (ret)
+		return ret;
 
 	return 0;
 }