RE: [PATCH 6.1.y-cip 09/13] rtc: isl1208: Add isl1208_set_xtoscb()

Biju Das
2023-08-17T13:43:59Z

Hi Pavel,

Thanks for the feedback.

> Subject: Re: [PATCH 6.1.y-cip 09/13] rtc: isl1208: Add isl1208_set_xtoscb()
>
> Hi!
>
> > > > If using an external crystal, the XTOSCB bit needs to be set at 0
> > > > to enable the crystal oscillator.
> > >
> > > > +++ b/drivers/rtc/rtc-isl1208.c
> > > > @@ -6,6 +6,7 @@
> > > > @@ -175,6 +176,20 @@ isl1208_i2c_validate_client(struct i2c_client
> > > *client)
> > > > return 0;
> > > > }
> > > >
> > > > +static int isl1208_set_xtoscb(struct i2c_client *client, int sr,
> > > > +int
> > > > +xtosb_val) {
> > > > + /* Do nothing if bit is already set to desired value */
> > > > + if ((sr & ISL1208_REG_SR_XTOSCB) == xtosb_val)
> > > > + return 0;
> > >
> > > XTOSCB bit is not bit 0, but xtosb_val is either 0 or 1. If it is 1,
> > > test will never succeed. I don't think that's intended.
> >
> > As mentioned in the comment, check is to avoid unnecessary writing to
> > I2C register if it is same value.
> >
> > 0 & 1 == 0 -->skip
> > 0 & 1 == 1 -->write
> > 1 & 1 == 0 --> write
> > 1 & 1 == 1 -->skip
> >
> > Is it expected result right?
>
> As far as I can see, you'll get
>
> #define ISL1208_REG_SR_XTOSCB (1<<6)
>
> xtosb_val = 1;
>
> 64 & 64 == 1
> 64 == 1
> false --> write
>
> when you should be skipping.

You are correct.

I need to do double negation to fix this.

/* Do nothing if bit is already set to desired value */
if (!!(sr & ISL1208_REG_SR_XTOSCB) == xtosb_val)
return 0;

Cheers,
Biju