[PATCH 5/5] rtc: ds1307: Limit clock starting retries

[Andrey Smirnov]

On Tue, Jun 7, 2016 at 1:09 PM, Trent Piepho <tpiepho at kymetacorp.com> wrote:
> If the driver detects the clock is stopped, via clock halted control
> bit or oscillator stopped status flag, it will try to start the clock
> and then reread the control registers and retry the probe process.
>
> It will continue to retry until the clock starts, possibly forever if
> the clock crystal is not installed.  While the driver's dogged
> determination to start the clock is admirable, at some point you have
> to say enough is enough, this clock won't go, and get one with more
Typo?  -----------------------------------------------------------------^

> important things, like booting.
>
> This limits the number of tries to start the clock to three.

This is definitely a good change. Might I suggest a slightly different
implementation? Something to the effect of:

------------------------8<----------------------------------------------------

for (retries = 0; retries < 3; retries++) {
    clock_halted = oscillator_failed = false;

    /* read RTC registers */
    tmp = ds1307_read_block_data(client, 0, nreg, buf);
    if (tmp != nreg) {
        dev_dbg(&client->dev, "read error %d\n", tmp);
        err = -EIO;
        goto exit;
    }

    /* clock halted?  turn it on, so clock can tick. */
    if (ds1307->has_alarms) {
         if (buf[DS1337_REG_CONTROL] & DS1337_BIT_nEOSC) {
              buf[DS1337_REG_CONTROL] &= ~DS1337_BIT_nEOSC;
              i2c_smbus_write_byte_data(client,
                                                            DS1337_REG_CONTROL,

buf[DS1337_REG_CONTROL]);
              clock_halted = true;
        }
    } else {
        if (buf[DS1307_REG_SECS] & DS1307_BIT_CH) {
            buf[DS1307_REG_SECS] = 0;
            i2c_smbus_write_byte_data(client,
                                                          DS1307_REG_SECS,
                                                          buf[DS1307_REG_SECS]);
            clock_halted = true;
        }
    }

    if (clock_halted) {
          dev_warn(&client->dev,
                           "chip's oscillator is disabled. "
                           "Re-enabling it.\n");
          continue;
    }


     /* Oscillator fault?  clear flag and print warning */
     switch (ds1307->osf) {
     case OSF_1338:
         if (buf[DS1307_REG_CONTROL] & DS1338_BIT_OSF) {
             buf[DS1307_REG_CONTROL] &= ~DS1338_BIT_OSF;
             i2c_smbus_write_byte_data(client,
                                                           DS1307_REG_CONTROL,

buf[DS1307_REG_CONTROL]);
             oscillator_failed = true;
         }
         break;
     case OSF_1337:
         if (buf[DS1337_REG_STATUS] & DS1337_BIT_OSF) {
             buf[DS1337_REG_STATUS] &= ~DS1337_BIT_OSF;
             i2c_smbus_write_byte_data(client,
                                                           DS1337_REG_STATUS,

buf[DS1337_REG_STATUS]);
              oscillator_failed = true;
         }
         break;
     default: ;
     }

     if (oscillator_failed) {
          dev_warn(&client->dev,
                           "chip's oscillator failed. "
                           "Clearing and re-reading status.\n");
          continue;
     }

     break;
};

if (oscillator_failed || clock_halted) {
    dev_warn(&client->dev,
                     "RTC's time information is incorrect. "
                     "Please set time\n")
}

if (!retries)
    dev_err(&client->dev,
                 "Failed to start clock, placing in correct once per
day mode!\n");


------------------------>8----------------------------------------------------

Note, however that this would restore original error handling behavior
-- which you changed in 3/5 -- where all registers are re-read after
each type of failure is detected.