Moi, On Fri, Jan 24, 2025 at 03:19:32PM +0200, Tomi Valkeinen wrote: > Hi, > > On 15/01/2025 16:17, Sakari Ailus wrote: > > Moi, > > > > On Fri, Jan 10, 2025 at 11:14:07AM +0200, Tomi Valkeinen wrote: > > > From: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx> > > > > > > On the I2C bus for remote clients (sensors), by default the watchdog > > > timer expires in 1s. To allow for a quicker system bring-up time, TI > > > recommends to speed it up to 50us [1]. > > > > > > [1]: Section 7.3.1.1 - https://www.ti.com/lit/gpn/ds90ub953-q1 > > > > > > Signed-off-by: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx> > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> > > > --- > > > drivers/media/i2c/ds90ub953.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c > > > index 99a4852b9381..6c36980e8beb 100644 > > > --- a/drivers/media/i2c/ds90ub953.c > > > +++ b/drivers/media/i2c/ds90ub953.c > > > @@ -54,6 +54,10 @@ > > > #define UB953_REG_CLKOUT_CTRL0 0x06 > > > #define UB953_REG_CLKOUT_CTRL1 0x07 > > > +#define UB953_REG_I2C_CONTROL2 0x0a > > > +#define UB953_REG_I2C_CONTROL2_SDA_OUTPUT_SETUP_SHIFT 4 > > > +#define UB953_REG_I2C_CONTROL2_BUS_SPEEDUP BIT(1) > > > + > > > #define UB953_REG_SCL_HIGH_TIME 0x0b > > > #define UB953_REG_SCL_LOW_TIME 0x0c > > > @@ -1320,6 +1324,13 @@ static int ub953_hw_init(struct ub953_data *priv) > > > if (ret) > > > return ret; > > > + v = 0; > > > + v |= 1 << UB953_REG_I2C_CONTROL2_SDA_OUTPUT_SETUP_SHIFT; > > > > BIT()? Or at least 1U <<< ...;. > > It's a three-bit field, the value just happens to be 1. What's wrong with 1 > << SHIFT? Shifting a signed value leads to the sign bit being undefined on some architectures. > > > > > > + v |= UB953_REG_I2C_CONTROL2_BUS_SPEEDUP; > > > + ret = ub953_write(priv, UB953_REG_I2C_CONTROL2, v, NULL); > > > > I'd just do this without a temporary variable. If you prefer to keep it, do > > assign the first calculated value there first and remove the assignment to > > zero. > > I think we can do without. > > > > + if (ret) > > > + return ret; > > > > No need for this. > > No, but it keeps the code structure consistent and allows easy future/debug > modifications. Please still remove such redundancies. -- Terveisin, Sakari Ailus
