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
