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

Biju Das
2023-08-17T11:33:38Z

Hi Pavel Machek,

Thanks for the feedback.

> Subject: Re: [PATCH 6.1.y-cip 09/13] rtc: isl1208: Add isl1208_set_xtoscb()
>
> Hi!
>
> > As per the HW manual, set the XTOSCB bit as follows:
> >
> > If using an external clock signal, set the XTOSCB bit as 1 to disable
> > the crystal oscillator.
> >
> > 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?

Cheers,
Biju

>
> > + if (xtosb_val)
> > + sr |= ISL1208_REG_SR_XTOSCB;
> > + else
> > + sr &= ~ISL1208_REG_SR_XTOSCB;
> > +
> > + return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr);
>
> Best regards,
> Pavel
> --
> DENX Software Engineering GmbH, Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany