mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Andrey Smirnov <andrew.smirnov@gmail.com>
To: Trent Piepho <tpiepho@kymetacorp.com>
Cc: barebox <barebox@lists.infradead.org>
Subject: Re: [PATCH 3/5] rtc: ds1307: Clean up of chip variant support
Date: Sun, 12 Jun 2016 16:54:54 -0700	[thread overview]
Message-ID: <CAHQ1cqH=zMbwqE9DaCQdNytn2zg4AMT6DyzCJnhN6f9r1PJsyQ@mail.gmail.com> (raw)
In-Reply-To: <1465329985.15779.162.camel@rtred1test09.kymeta.local>

On Tue, Jun 7, 2016 at 1:06 PM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> Gets rid of multiple case statements and chip type checks by
> attempting to use a more unified approach to chip differences.  Flag
> bits are added to the state struct for specific differences.
>
> Combines the checks for OSF and CH bits into a single block for all
> chips.
>
> Add some flags the indicate chip features.  Use these instead of a
> bunch of case statements in different places.
>
> Do a single read of chips registers in probe instead of multiple ones
> and place register in matching location in the buffer.  Fix bug where
> DOSF bit was set in incorrect buffer location.
>
> Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
> ---
>  drivers/rtc/rtc-ds1307.c | 159 +++++++++++++++++++++++++++--------------------
>  1 file changed, 90 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 5ed64bf..1526273 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -98,10 +98,26 @@ enum ds_type {
>  /* Most registers for any device */
>  #define DS13xx_REG_COUNT       16
>
> -
>  struct ds1307 {
>         struct rtc_device       rtc;
>         enum ds_type            type;
> +       /*
> +        * Flags to code chip differences that this code handles.
> +        * has_alarms:  Chip has alarms, also signifies:
> +        *      Control register has a different address and format
> +        *      Seconds register does not have a 'CH' bit
> +        *      Month register has century bit
> +        * osf:         Oscillator Stop Flag location
> +        *      0 = none
> +        *      1..3 = DS13(38|40|37)_BIT_OSF
> +        */
> +       unsigned                has_alarms:1;
> +#define OSF_NONE               0
> +#define OSF_1338               1
> +#define OSF_1340               2
> +#define OSF_1337               3
> +       unsigned                osf:2;
> +
>         struct i2c_client       *client;
>  };
>
> @@ -257,16 +273,10 @@ static int ds1307_set_time(struct rtc_device *rtcdev, struct rtc_time *t)
>         tmp = t->tm_year - 100;
>         buf[DS1307_REG_YEAR] = bin2bcd(tmp);
>
> -       switch (ds1307->type) {
> -       case ds_1337:
> -       case ds_1341:
> +       if (ds1307->has_alarms) {
>                 buf[DS1307_REG_MONTH] |= DS1337_BIT_CENTURY;
> -               break;
> -       default:
> -               break;
>         }
>
> -
>         dev_dbg(dev, "%s: %7ph\n", "write", buf);
>
>         result = ds1307_write_block_data(ds1307->client, DS13xx_REG_TIME_START,
> @@ -289,6 +299,8 @@ static int ds1307_probe(struct device_d *dev)
>         struct ds1307           *ds1307;
>         int                     err = -ENODEV;
>         int                     tmp;
> +       int                     fault;
> +       int                     nreg;
>         unsigned char           buf[DS13xx_REG_COUNT];
>         unsigned long driver_data;
>         const struct device_node *np = dev->device_node;
> @@ -303,23 +315,35 @@ static int ds1307_probe(struct device_d *dev)
>         ds1307->type = driver_data;
>
>         switch (ds1307->type) {
> +       case ds_1307:
> +               nreg = DS1307_REG_CONTROL + 1;
> +               break;
> +       case ds_1338:
> +               nreg = DS1307_REG_CONTROL + 1;
> +               ds1307->osf = OSF_1338;
> +               break;
>         case ds_1337:
>         case ds_1341:
> -               /* get registers that the "rtc" read below won't read... */
> -               tmp = ds1307_read_block_data(ds1307->client,
> -                                            DS1337_REG_CONTROL, 2, buf);
> -
> -               if (tmp != 2) {
> -                       dev_dbg(&client->dev, "read error %d\n", tmp);
> -                       err = -EIO;
> -                       goto exit;
> -               }
> -
> -               /* oscillator off?  turn it on, so clock can tick. */
> -               if (buf[0] & DS1337_BIT_nEOSC)
> -                       buf[0] &= ~DS1337_BIT_nEOSC;
> +               nreg = DS1337_REG_STATUS + 1;
> +               ds1307->osf = OSF_1337;
> +               ds1307->has_alarms = 1;
> +               break;
> +       default:
> +               dev_err(&client->dev, "Unknown device type %lu\n", driver_data);
> +               err = -ENODEV;
> +               goto exit;
> +       }
>
> +read_rtc:
> +       /* 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;
> +       }
>
> +       if (ds1307->has_alarms) {
>                 /*
>                   Make sure no alarm interrupts or square wave signals
>                   are produced by the chip while we are in
> @@ -328,56 +352,36 @@ static int ds1307_probe(struct device_d *dev)
>                   wave generation), but disabling each individual
>                   alarm interrupt source
>                  */
> -               buf[0] |= DS1337_BIT_INTCN;
> -               buf[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
> +               buf[DS1337_REG_CONTROL] |= DS1337_BIT_INTCN;
> +               buf[DS1337_REG_CONTROL] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
>
>                 i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
> -                                         buf[0]);
> +                                         buf[DS1337_REG_CONTROL]);
>
>                 if (ds1307->type == ds_1341) {
>                         /*
>                          * For the above to be true, DS1341 also has to have
>                          * ECLK bit set to 0
>                          */
> -                       buf[1] &= ~DS1341_BIT_ECLK;
> +                       buf[DS1337_REG_STATUS] &= ~DS1341_BIT_ECLK;
>
>                         /*
>                          * Let's set additionale RTC bits to
>                          * facilitate maximum power saving.
>                          */
> -                       buf[0] |=  DS1341_BIT_DOSF;
> -                       buf[0] &= ~DS1341_BIT_EGFIL;
> +                       buf[DS1337_REG_STATUS] |=  DS1341_BIT_DOSF;
> +                       buf[DS1337_REG_CONTROL] &= ~DS1341_BIT_EGFIL;
>
>                         i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
> -                                                 buf[0]);
> +                                                 buf[DS1337_REG_CONTROL]);
>                         i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
> -                                                 buf[1]);
> +                                                 buf[DS1337_REG_STATUS]);
>                 }
>
> -
> -               /* oscillator fault?  clear flag, and warn */
> -               if (buf[1] & DS1337_BIT_OSF) {
> -                       i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
> -                               buf[1] & ~DS1337_BIT_OSF);
> -                       dev_warn(&client->dev, "SET TIME!\n");
> -               }
> -
> -       default:
> -               break;
> -       }
> -
> -read_rtc:
> -       /* read RTC registers */
> -       tmp = ds1307_read_block_data(client, 0, 8, buf);
> -       if (tmp != 8) {
> -               dev_dbg(&client->dev, "read error %d\n", tmp);
> -               err = -EIO;
> -               goto exit;
>         }
>
>         /* Configure clock using OF data if available */
> -       if (IS_ENABLED(CONFIG_OFDEVICE) && np &&
> -           ds1307->type != ds_1337 && ds1307->type != ds_1341) {
> +       if (IS_ENABLED(CONFIG_OFDEVICE) && np && !ds1307->has_alarms) {
>                 u8 control = buf[DS1307_REG_CONTROL];
>                 u32 rate = 0;
>
> @@ -416,32 +420,49 @@ read_rtc:
>         }
>
>         /* Check for clock halted conditions and start clock */
> -       tmp = buf[DS1307_REG_SECS];
> -       switch (ds1307->type) {
> -       case ds_1307:
> -               /* clock halted?  turn it on, so clock can tick. */
> -               if (tmp & DS1307_BIT_CH) {
> -                       i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
> -                       dev_warn(&client->dev, "SET TIME!\n");
> -                       goto read_rtc;
> +       fault = 0;
> +
> +       /* 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]);
> +                       fault = 1;
>                 }
> -               break;
> -       case ds_1338:
> -               /* clock halted?  turn it on, so clock can tick. */
> -               if (tmp & DS1307_BIT_CH)
> -                       i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
> +       } 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]);
> +                       fault = 1;
> +               }
> +       }
>
> -               /* oscillator fault?  clear flag, and warn */
> +       /* 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]
> -                                       & ~DS1338_BIT_OSF);
> -                       dev_warn(&client->dev, "SET TIME!\n");
> -                       goto read_rtc;
> +                                                 buf[DS1307_REG_CONTROL]);
> +                       fault = 1;
>                 }
>                 break;
> -       default:
> +       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]);
> +                       fault = 1;
> +               }
>                 break;
> +       default: ;
> +       }
> +
> +       if (fault) {
> +               dev_warn(&client->dev, "SET TIME!\n");
> +               goto read_rtc;
>         }

Now that you only have only one "goto" place, wouldn't it make things
more readable if you convert this to a loop? This should also make the
diff of 5/5 slightly smaller.

>
>         if (buf[DS1307_REG_HOUR] & DS1307_BIT_12HR) {
> --
> 2.7.0.25.gfc10eb5.dirty
>
>

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

  reply	other threads:[~2016-06-12 23:55 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-07 20:01 [PATCH 0/5] Fix problems with DS1307 RTC driver Trent Piepho
2016-06-07 20:03 ` [PATCH 1/5] rtc: ds1307: Keep DT based RTC configuration from breaking DS1337/41 Trent Piepho
2016-06-07 20:04 ` [PATCH 2/5] rtc: ds1307: Remove unused and unneeded driver code Trent Piepho
2016-06-07 20:06 ` [PATCH 3/5] rtc: ds1307: Clean up of chip variant support Trent Piepho
2016-06-12 23:54   ` Andrey Smirnov [this message]
2016-06-07 20:09 ` [PATCH 4/5] rtc: ds1307: Allow configuring DS1337 type chips Trent Piepho
2016-06-12 22:29   ` Andrey Smirnov
2016-06-07 20:09 ` [PATCH 5/5] rtc: ds1307: Limit clock starting retries Trent Piepho
2016-06-12 23:51   ` Andrey Smirnov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHQ1cqH=zMbwqE9DaCQdNytn2zg4AMT6DyzCJnhN6f9r1PJsyQ@mail.gmail.com' \
    --to=andrew.smirnov@gmail.com \
    --cc=barebox@lists.infradead.org \
    --cc=tpiepho@kymetacorp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox