[PATCH] rtc: add ht1380 driver

[Sergei Antonov]

On Wed, 26 Oct 2022 at 02:35, Simon Glass <sjg at chromium.org> wrote:

> > +static void ht1380_half_period_delay(void)
> > +{
> > +       /* Delay for half a period. 1 us complies with the 500 KHz maximum
> > +          input serial clock limit given by the datasheet. */
>
> /*
>  * Delay for half...
>  * second line
>  */
>
> Please fix globally.

OK. I was confused by doc/develop/codingstyle.rst:
* The exception for net files to the `multi-line comment
<https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting>`_
applies only to Linux, not to U-Boot.
What is the U-Boot style of multi-line comments remains a mystery to me.

> > +static int ht1380_reset_on(struct ht1380_priv *priv)
>
> function comment - does it stay in reset state or automatically exit?

It stays in reset state. Until the counterpart function
ht1380_reset_off is called.

> > +       /* Correctness check: some registers are always > 0 */
> > +       if (!reg[MDAY] || !reg[MONTH] || !reg[WDAY])
> > +               goto exit;
> > +
> > +       tm->tm_sec = bcd2bin(reg[SEC]);
> > +       tm->tm_min = bcd2bin(reg[MIN]);
> > +       if (reg[HOUR] & 0x80) {
>
> I suggest having an enum or #define for 0x80 and the 0x20 below

Oh. I hoped so much for /* */ comments put after lines with 0x80 and 0x20.

> > +               /* AM-PM Mode, range is 01-12 */
> > +               tm->tm_hour = bcd2bin(reg[HOUR] & 0x1F) % 12;
> > +               if (reg[HOUR] & 0x20) {
> > +                       /* it is PM (otherwise AM) */
> > +                       tm->tm_hour += 12;
> > +               }
> > +       } else {
> > +               /* 24-hour Mode, range is 0-23 */
> > +               tm->tm_hour = bcd2bin(reg[HOUR]);
> > +       }
> > +       tm->tm_mday = bcd2bin(reg[MDAY]);
> > +       tm->tm_mon = bcd2bin(reg[MONTH]);
> > +       tm->tm_year = 2000 + bcd2bin(reg[YEAR]);
> > +       tm->tm_wday = bcd2bin(reg[WDAY]) - 1;
> > +       tm->tm_yday = 0;
> > +       tm->tm_isdst = 0;
> > +
> > +       ret = 0;
> > +
> > +exit:
> > +       ht1380_reset_on(priv);
> > +       return ret;
> > +}
> > +
> > +static int ht1380_write_protection_off(struct ht1380_priv *priv)
> > +{
> > +       int ret;
> > +       const int PROTECT = 0x8E;
>
> Define at top of file, e.g. in an enum

What about locality of definitions?

Thanks for your quick comments.