Hi Trent Piepho,
Thanks for the feedback.
> Subject: Re: [PATCH v6 10/11] rtc: isl1208: Add isl1208_set_xtoscb()
>
> On Friday, June 2, 2023 7:24:25 AM PDT Biju Das wrote:
> >
> > +static int isl1208_set_xtoscb(struct i2c_client *client, int sr, int
> xtosb_val) +{
> > + 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); }
>
> Since the contents of this register are preserved by battery power, it
> will normally already have the correct value.
Agreed. What about the cold boot and environment where there is no RTC support in
bootloader?
>
> Setting it this way adds an extra I2C transaction to every driver init to
> set the register to the value it's already at.
[3]
> It would be better to check if the bit is not set correctly, and then
> only set it and write to the register if it is not.
Agreed.
>
> You can do that like this:
>
> /* Strangely, xtosb_val of 0 means to set the bit and 1 means to clear
> it!
OK, But as per HW manual, see [1],
0 means :- Enable internal crystal oscillator
1 means :- Disable internal crystal oscillator
<snippet>
CRYSTAL OSCILLATOR ENABLE BIT (XTOSCB)
This bit enables/disables the internal crystal oscillator. When
the XTOSCB is set to "1", the oscillator is disabled, and the X1
pin allows for an external 32kHz signal to drive the RTC. The
XTOSCB bit is set to "0" on power-up.
</snippet>
[1] https://www.renesas.com/us/en/document/dst/isl1208-datasheet
> * Hopefully, that is really what you want to do. Seems backward of what
> * I would expect.
> */
Agreed.
> 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 (!(st & ISL1208_REG_SR_XTOSCB) == xtosb_val)
> return 0;
It is (st & ISL1208_REG_SR_XTOSCB) == xtosb_val) right??
BIT(2) = 0 and xtosb_val =0 --> return 0
BIT(2) = 1 and xtosb_val =1 --> return 0
BIT(2) = 0 and xtosb_val = 1 --> sr ^= ISL1208_REG_SR_XTOSCB
BIT(2) = 1 and xtosb_val =0 --> sr ^= ISL1208_REG_SR_XTOSCB
> sr ^= ISL1208_REG_SR_XTOSCB;
> return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr); }
>
>
> > static int
> > isl1208_probe(struct i2c_client *client) {
> > - int rc = 0;
> > struct isl1208_state *isl1208;
> > int evdet_irq = -1;
> > + int xtosb_val = 1;
>
> So you assume XTOSCB should be unset by default? Prior behavior of the
> driver was to preserve this bit. Maybe it was set the bootloader to the
> correct value? This would break such a setup.
Cold boot, reset value is '0'. We should not assume any bootloader
settings. Currently I don't have bootloader support for RTC in my environment
and depending on dt settings parse the details to find it is connected to
external crystal or Clock source.
Normally u-boot device tree is based on kernel device tree.
As you said above[3],
Parse the details from kernel and see, if there is mismatch and then only override.
>
> > + rc = isl1208_clk_present(client, "xin");
> > + if (rc < 0)
> > + return rc;
> > +
> > + if (!rc) {
> > + rc = isl1208_clk_present(client, "clkin");
>
> Why do you support two names for the same clock? I don't see this
> discussed in any of the threads.
Please see [2].
+ Use xin, if connected to an external crystal.
+ Use clkin, if connected to an external clock signal.
[2] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230602142426.438375-7-biju.das.jz@xxxxxxxxxxxxxx/
Cheers,
Biju
