Add error handling to ub913_hw_init() using a new helper function,
ub913_update_bits().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub913.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index 150d6641516f..8b540b360e79 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -146,6 +146,19 @@ static int ub913_write(const struct ub913_data *priv, u8 reg, u8 val)
 	return ret;
 }
 
+static int ub913_update_bits(const struct ub913_data *priv, u8 reg, u8 mask,
+			     u8 val)
+{
+	int ret;
+
+	ret = regmap_update_bits(priv->regmap, reg, mask, val);
+	if (ret < 0)
+		dev_err(&priv->client->dev,
+			"Cannot update register 0x%02x %d!\n", reg, ret);
+
+	return ret;
+}
+
 /*
  * GPIO chip
  */
@@ -733,10 +746,13 @@ static int ub913_hw_init(struct ub913_data *priv)
 	if (ret)
 		return dev_err_probe(dev, ret, "i2c master init failed\n");
 
-	ub913_read(priv, UB913_REG_GENERAL_CFG, &v);
-	v &= ~UB913_REG_GENERAL_CFG_PCLK_RISING;
-	v |= priv->pclk_polarity_rising ? UB913_REG_GENERAL_CFG_PCLK_RISING : 0;
-	ub913_write(priv, UB913_REG_GENERAL_CFG, v);
+	ret = ub913_update_bits(priv, UB913_REG_GENERAL_CFG,
+				UB913_REG_GENERAL_CFG_PCLK_RISING,
+				priv->pclk_polarity_rising ?
+					UB913_REG_GENERAL_CFG_PCLK_RISING :
+					0);
+	if (ret)
+		return ret;
 
 	return 0;
 }

-- 
2.43.0

On Fri, Oct 04, 2024 at 05:46:43PM +0300, Tomi Valkeinen wrote:
> Add error handling to ub913_hw_init() using a new helper function,
> ub913_update_bits().

...

> +	ret = ub913_update_bits(priv, UB913_REG_GENERAL_CFG,
> +				UB913_REG_GENERAL_CFG_PCLK_RISING,
> +				priv->pclk_polarity_rising ?
> +					UB913_REG_GENERAL_CFG_PCLK_RISING :
> +					0);

So, you can use regmap_set_bits() / regmap_clear_bits() instead of this
ternary. It also gives one parameter less to the regmap calls.

> +	if (ret)
> +		return ret;

-- 
With Best Regards,
Andy Shevchenko

Hi Andy,

On 10/10/2024 17:04, Andy Shevchenko wrote:
> On Fri, Oct 04, 2024 at 05:46:43PM +0300, Tomi Valkeinen wrote:
>> Add error handling to ub913_hw_init() using a new helper function,
>> ub913_update_bits().
> 
> ...
> 
>> +	ret = ub913_update_bits(priv, UB913_REG_GENERAL_CFG,
>> +				UB913_REG_GENERAL_CFG_PCLK_RISING,
>> +				priv->pclk_polarity_rising ?
>> +					UB913_REG_GENERAL_CFG_PCLK_RISING :
>> +					0);
> 
> So, you can use regmap_set_bits() / regmap_clear_bits() instead of this
> ternary. It also gives one parameter less to the regmap calls.

True... But is it better?

if (priv->pclk_polarity_rising)
	ret = regmap_set_bits(priv->regmap, UB913_REG_GENERAL_CFG,
			      UB913_REG_GENERAL_CFG_PCLK_RISING);
else
	ret = regmap_clear_bits(priv->regmap, UB913_REG_GENERAL_CFG,
				UB913_REG_GENERAL_CFG_PCLK_RISING);

The call itself is more readable there, but then again, as we're setting 
the value of a bit, I dislike having if/else with two calls for a single 
assignment.

Using FIELD_PREP is perhaps a bit better than the ternary:

ret = ub913_update_bits(priv, UB913_REG_GENERAL_CFG,
			UB913_REG_GENERAL_CFG_PCLK_RISING,
			FIELD_PREP(UB913_REG_GENERAL_CFG_PCLK_RISING,
				   priv->pclk_polarity_rising));

I think I'd like best a function to set/clear a bitmask with a boolean:

ret = regmap_toggle_bits(priv->regmap, UB913_REG_GENERAL_CFG,
			 UB913_REG_GENERAL_CFG_PCLK_RISING,
			 priv->pclk_polarity_rising);

For now, I think I'll go with the FIELD_PREP() version. It's perhaps a 
bit better than the ternary.

  Tomi

On Fri, Nov 08, 2024 at 11:34:09AM +0200, Tomi Valkeinen wrote:
> On 10/10/2024 17:04, Andy Shevchenko wrote:
> > On Fri, Oct 04, 2024 at 05:46:43PM +0300, Tomi Valkeinen wrote:
> > > Add error handling to ub913_hw_init() using a new helper function,
> > > ub913_update_bits().

...

> > > +	ret = ub913_update_bits(priv, UB913_REG_GENERAL_CFG,
> > > +				UB913_REG_GENERAL_CFG_PCLK_RISING,
> > > +				priv->pclk_polarity_rising ?
> > > +					UB913_REG_GENERAL_CFG_PCLK_RISING :
> > > +					0);
> > 
> > So, you can use regmap_set_bits() / regmap_clear_bits() instead of this
> > ternary. It also gives one parameter less to the regmap calls.
> 
> True... But is it better?

In my opinion yes, because it's clearer on what's going on.
It has no (semi-)hidden choice, so code wise it most likely
will be the same at the end. So we are speaking only about
C-level of readability.

> if (priv->pclk_polarity_rising)
> 	ret = regmap_set_bits(priv->regmap, UB913_REG_GENERAL_CFG,
> 			      UB913_REG_GENERAL_CFG_PCLK_RISING);
> else
> 	ret = regmap_clear_bits(priv->regmap, UB913_REG_GENERAL_CFG,
> 				UB913_REG_GENERAL_CFG_PCLK_RISING);
> 
> The call itself is more readable there, but then again, as we're setting the
> value of a bit, I dislike having if/else with two calls for a single
> assignment.

FTR, there was an attempt to add _assign() in similar way how it's done with
bitops (set/clear/assign) to regmap, but had been rejected by Mark. I don't
remember detail why, though.

> Using FIELD_PREP is perhaps a bit better than the ternary:
> 
> ret = ub913_update_bits(priv, UB913_REG_GENERAL_CFG,
> 			UB913_REG_GENERAL_CFG_PCLK_RISING,
> 			FIELD_PREP(UB913_REG_GENERAL_CFG_PCLK_RISING,
> 				   priv->pclk_polarity_rising));
> 
> I think I'd like best a function to set/clear a bitmask with a boolean:
> 
> ret = regmap_toggle_bits(priv->regmap, UB913_REG_GENERAL_CFG,
> 			 UB913_REG_GENERAL_CFG_PCLK_RISING,
> 			 priv->pclk_polarity_rising);
> 
> For now, I think I'll go with the FIELD_PREP() version. It's perhaps a bit
> better than the ternary.

Okay.

-- 
With Best Regards,
Andy Shevchenko