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
