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

Pavel Machek
2023-08-17T13:27:19Z

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

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.

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]