Re: [PATCH 12/13] media: i2c: ds90ub913: Add error handling to ub913_hw_init()

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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