| Message ID | 20241004-ub9xx-fixes-v1-12-e30a4633c786@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: i2c: ds90ub9xx: Misc fixes and improvements | expand |
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.
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; }
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(-)