rtc: add ht1380 driver

Support Holtek HT1380/HT1381 Serial Timekeeper Chip. It provides seconds
, minutes, hours,  day of the week, date, month and year information.

Datasheet:
https://www.holtek.com.tw/documents/10179/11842/ht1380_1v130.pdf

Signed-off-by: Sergei Antonov <saproj@gmail.com>
---

v2:
* The RESET pin is now to be described as ACTIVE_LOW in dts.

Changes suggested by Simon Glass:
* a more detailed driver description in Kconfig
* multi-line comments' style
* enum for 0x80 and the 0x20 at top of file
* lower-case hex constants
* function comments for ht1380_reset_on/off
* blank line before returns

PROTECT remains in a function scope for the sake of locality of definitions.

 drivers/rtc/Kconfig  |   8 +
 drivers/rtc/Makefile |   1 +
 drivers/rtc/ht1380.c | 337 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 346 insertions(+)
 create mode 100644 drivers/rtc/ht1380.c

Hi Sergei,

On Tue, 6 Dec 2022 at 23:07, Sergei Antonov <saproj@gmail.com> wrote:
>
> Support Holtek HT1380/HT1381 Serial Timekeeper Chip. It provides seconds
> , minutes, hours,  day of the week, date, month and year information.
>
> Datasheet:
> https://www.holtek.com.tw/documents/10179/11842/ht1380_1v130.pdf
>
> Signed-off-by: Sergei Antonov <saproj@gmail.com>
> ---
>
> v2:
> * The RESET pin is now to be described as ACTIVE_LOW in dts.
>
> Changes suggested by Simon Glass:
> * a more detailed driver description in Kconfig
> * multi-line comments' style
> * enum for 0x80 and the 0x20 at top of file
> * lower-case hex constants
> * function comments for ht1380_reset_on/off
> * blank line before returns
>
> PROTECT remains in a function scope for the sake of locality of definitions.
>
>  drivers/rtc/Kconfig  |   8 +
>  drivers/rtc/Makefile |   1 +
>  drivers/rtc/ht1380.c | 337 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 346 insertions(+)
>  create mode 100644 drivers/rtc/ht1380.c
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 23963271928a..eed48e35a578 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -220,4 +220,12 @@ config RTC_ZYNQMP
>           Say "yes" here to support the on chip real time clock
>           present on Xilinx ZynqMP SoC.
>
> +config RTC_HT1380
> +       bool "Enable Holtek HT1380/HT1381 RTC driver"
> +       depends on DM_RTC && DM_GPIO
> +       help
> +         Say "yes" here to get support for Holtek HT1380/HT1381
> +         Serial Timekeeper IC which provides seconds, minutes, hours,
> +         day of the week, date, month and year information.

Perhaps mention how it is connected, i.e. three GPIOs.

> +
>  endmenu
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 009dd9d28c95..f3164782b605 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_RTC_DS3231) += ds3231.o
>  obj-$(CONFIG_RTC_DS3232) += ds3232.o
>  obj-$(CONFIG_RTC_EMULATION) += emul_rtc.o
>  obj-$(CONFIG_RTC_FTRTC010) += ftrtc010.o
> +obj-$(CONFIG_RTC_HT1380) += ht1380.o
>  obj-$(CONFIG_SANDBOX) += i2c_rtc_emul.o
>  obj-$(CONFIG_RTC_IMXDI) += imxdi.o
>  obj-$(CONFIG_RTC_ISL1208) += isl1208.o
> diff --git a/drivers/rtc/ht1380.c b/drivers/rtc/ht1380.c
> new file mode 100644
> index 000000000000..25335227d893
> --- /dev/null
> +++ b/drivers/rtc/ht1380.c
> @@ -0,0 +1,337 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Holtek HT1380/HT1381 Serial Timekeeper Chip
> + *
> + * Communication with the chip is vendor-specific.
> + * It is done via 3 GPIO pins: reset, clock, and data.
> + * Describe in .dts this way:
> + *
> + * rtc {
> + *         compatible = "holtek,ht1380";
> + *         rst-gpio = <&gpio 19 GPIO_ACTIVE_LOW>;
> + *         clk-gpio = <&gpio 20 GPIO_ACTIVE_HIGH>;
> + *         dat-gpio = <&gpio 21 GPIO_ACTIVE_HIGH>;
> + * };

Is there a binding file for this?

I believe the standard name should be rst-gpios (i.e. plural)?

> + *
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <rtc.h>
> +#include <bcd.h>
> +#include <asm/gpio.h>
> +#include <linux/delay.h>
> +
> +struct ht1380_priv {
> +       struct gpio_desc rst_desc;
> +       struct gpio_desc clk_desc;
> +       struct gpio_desc dat_desc;
> +};
> +
> +enum registers {
> +       SEC,
> +       MIN,
> +       HOUR,
> +       MDAY,
> +       MONTH,
> +       WDAY,
> +       YEAR,
> +       WP,
> +       N_REGS
> +};
> +
> +enum hour_mode {
> +       AMPM_MODE = 0x80, /* RTC is in AM/PM mode */
> +       PM_NOW = 0x20,    /* set if PM, clear if AM */
> +};
> +
> +static const int BURST = 0xbe;
> +static const int READ = 1;
> +
> +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.
> +        */
> +       udelay(1);
> +}
> +
> +static int ht1380_send_byte(struct ht1380_priv *priv, int byte)
> +{
> +       int ret;
> +
> +       for (int bit = 0; bit < 8; bit++) {
> +               ret = dm_gpio_set_value(&priv->dat_desc, byte >> bit & 1);
> +               if (ret)
> +                       break;
> +               ht1380_half_period_delay();
> +
> +               ret = dm_gpio_set_value(&priv->clk_desc, 1);
> +               if (ret)
> +                       break;
> +               ht1380_half_period_delay();
> +
> +               ret = dm_gpio_set_value(&priv->clk_desc, 0);
> +               if (ret)
> +                       break;
> +       }
> +
> +       return ret;
> +}
> +
> +/*
> + * Leave reset state. The transfer operation can then be started.
> + */
> +static int ht1380_reset_off(struct ht1380_priv *priv)
> +{
> +       const unsigned int T_CC = 4; /* us, Reset to Clock Setup */
> +       int ret;
> +
> +       /*
> +        * Leave RESET state.
> +        * Make sure we make the minimal delay required by the datasheet.
> +        */
> +       ret = dm_gpio_set_value(&priv->rst_desc, 0);
> +       udelay(T_CC);
> +
> +       return ret;
> +}
> +
> +/*
> + * Enter reset state. Completes the transfer operation.
> + */
> +static int ht1380_reset_on(struct ht1380_priv *priv)
> +{
> +       const unsigned int T_CWH = 4; /* us, Reset Inactive Time */
> +       int ret;
> +
> +       /*
> +        * Enter RESET state.
> +        * Make sure we make the minimal delay required by the datasheet.
> +        */
> +       ret = dm_gpio_set_value(&priv->rst_desc, 1);
> +       udelay(T_CWH);
> +
> +       return ret;
> +}
> +
> +static int ht1380_rtc_get(struct udevice *dev, struct rtc_time *tm)
> +{
> +       struct ht1380_priv *priv = dev_get_priv(dev);
> +       int ret, i, bit, reg[N_REGS];
> +
> +       ret = dm_gpio_set_value(&priv->clk_desc, 0);
> +       if (ret)
> +               return ret;
> +
> +       ret = dm_gpio_set_dir_flags(&priv->dat_desc, GPIOD_IS_OUT);
> +       if (ret)
> +               return ret;
> +
> +       ret = ht1380_reset_off(priv);
> +       if (ret)
> +               goto exit;
> +
> +       ret = ht1380_send_byte(priv, BURST + READ);
> +       if (ret)
> +               goto exit;
> +
> +       ret = dm_gpio_set_dir_flags(&priv->dat_desc, GPIOD_IS_IN);
> +       if (ret)
> +               goto exit;
> +

Is this some sort of I2C protocol?

> +       for (i = 0; i < N_REGS; i++) {
> +               reg[i] = 0;
> +
> +               for (bit = 0; bit < 8; bit++) {
> +                       ht1380_half_period_delay();
> +
> +                       ret = dm_gpio_set_value(&priv->clk_desc, 1);
> +                       if (ret)
> +                               goto exit;
> +                       ht1380_half_period_delay();
> +
> +                       reg[i] |= dm_gpio_get_value(&priv->dat_desc) << bit;
> +                       ret = dm_gpio_set_value(&priv->clk_desc, 0);
> +                       if (ret)
> +                               goto exit;
> +               }
> +       }
> +
> +       ret = -EINVAL;
> +
> +       /* Correctness check: some bits are always zero */
> +       if ((reg[MIN] & 0x80) || (reg[HOUR] & 0x40) || (reg[MDAY] & 0xc0) ||
> +           (reg[MONTH] & 0xe0) || (reg[WDAY] & 0xf8) || (reg[WP] & 0x7f))
> +               goto exit;

Drop extra brackets ?

> +
> +       /* Correctness check: some registers are always non-zero */
> +       if (!reg[MDAY] || !reg[MONTH] || !reg[WDAY])
> +               goto exit;
> +
> +       tm->tm_sec = bcd2bin(reg[SEC]);
> +       tm->tm_min = bcd2bin(reg[MIN]);
> +       if (reg[HOUR] & AMPM_MODE) {
> +               /* AM-PM Mode, range is 01-12 */
> +               tm->tm_hour = bcd2bin(reg[HOUR] & 0x1f) % 12;
> +               if (reg[HOUR] & PM_NOW) {
> +                       /* 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;
> +
> +       ret = ht1380_reset_off(priv);
> +       if (ret)
> +               return ret;
> +
> +       ret = ht1380_send_byte(priv, PROTECT);
> +       if (ret)
> +               return ret;
> +       ret = ht1380_send_byte(priv, 0); /* WP bit is 0 */
> +       if (ret)
> +               return ret;
> +
> +       return ht1380_reset_on(priv);
> +}
> +
> +static int ht1380_rtc_set(struct udevice *dev, const struct rtc_time *tm)
> +{
> +       struct ht1380_priv *priv = dev_get_priv(dev);
> +       int ret, i, reg[N_REGS];
> +
> +       ret = dm_gpio_set_value(&priv->clk_desc, 0);
> +       if (ret)
> +               return ret;
> +
> +       ret = dm_gpio_set_dir_flags(&priv->dat_desc, GPIOD_IS_OUT);
> +       if (ret)
> +               goto exit;
> +
> +       ret = ht1380_write_protection_off(priv);
> +       if (ret)
> +               goto exit;
> +
> +       reg[SEC] = bin2bcd(tm->tm_sec);
> +       reg[MIN] = bin2bcd(tm->tm_min);
> +       reg[HOUR] = bin2bcd(tm->tm_hour);
> +       reg[MDAY] = bin2bcd(tm->tm_mday);
> +       reg[MONTH] = bin2bcd(tm->tm_mon);
> +       reg[WDAY] = bin2bcd(tm->tm_wday) + 1;
> +       reg[YEAR] = bin2bcd(tm->tm_year - 2000);
> +       reg[WP] = 0x80; /* WP bit is 1 */
> +
> +       ret = ht1380_reset_off(priv);
> +       if (ret)
> +               goto exit;
> +
> +       ret = ht1380_send_byte(priv, BURST);
> +       for (i = 0; i < N_REGS && ret; i++)
> +               ret = ht1380_send_byte(priv, reg[i]);
> +
> +exit:
> +       ht1380_reset_on(priv);
> +
> +       return ret;
> +}
> +
> +static int ht1380_probe(struct udevice *dev)
> +{
> +       int ret;
> +       struct ht1380_priv *priv;
> +
> +       priv = dev_get_priv(dev);
> +       if (!priv)
> +               return -EINVAL;
> +
> +       ret = gpio_request_by_name(dev, "rst-gpio", 0,
> +                                  &priv->rst_desc, 0);

You can pass GPIOD_IS_OUT as the last param and drop the extra code below.

> +       if (ret)
> +               goto fail_rst;
> +
> +       ret = gpio_request_by_name(dev, "clk-gpio", 0,
> +                                  &priv->clk_desc, 0);
> +       if (ret)
> +               goto fail_clk;
> +
> +       ret = gpio_request_by_name(dev, "dat-gpio", 0,
> +                                  &priv->dat_desc, 0);
> +       if (ret)
> +               goto fail_dat;
> +
> +       ret = dm_gpio_set_dir_flags(&priv->clk_desc, GPIOD_IS_OUT);
> +       if (ret)
> +               goto fail;
> +
> +       ret = dm_gpio_set_dir_flags(&priv->rst_desc, GPIOD_IS_OUT);
> +       if (ret)
> +               goto fail;
> +
> +       ret = ht1380_reset_on(priv);
> +       if (ret)
> +               goto fail;
> +
> +       return 0;
> +
> +fail:
> +       dm_gpio_free(dev, &priv->dat_desc);
> +fail_dat:
> +       dm_gpio_free(dev, &priv->clk_desc);
> +fail_clk:
> +       dm_gpio_free(dev, &priv->rst_desc);
> +fail_rst:
> +       return ret;
> +}
> +
> +static int ht1380_remove(struct udevice *dev)
> +{
> +       struct ht1380_priv *priv = dev_get_priv(dev);
> +
> +       dm_gpio_free(dev, &priv->rst_desc);
> +       dm_gpio_free(dev, &priv->clk_desc);
> +       dm_gpio_free(dev, &priv->dat_desc);
> +
> +       return 0;
> +}
> +
> +static const struct rtc_ops ht1380_rtc_ops = {
> +       .get = ht1380_rtc_get,
> +       .set = ht1380_rtc_set,
> +};
> +
> +static const struct udevice_id ht1380_rtc_ids[] = {
> +       { .compatible = "holtek,ht1380" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(rtc_ht1380) = {
> +       .name = "rtc-ht1380",
> +       .id = UCLASS_RTC,
> +       .probe = ht1380_probe,
> +       .remove = ht1380_remove,
> +       .of_match = ht1380_rtc_ids,
> +       .ops = &ht1380_rtc_ops,
> +       .priv_auto = sizeof(struct ht1380_priv),
> +};
> --
> 2.34.1
>

Regards,
Simon

On Tue, Dec 06, 2022 at 01:06:59PM +0300, Sergei Antonov wrote:

> Support Holtek HT1380/HT1381 Serial Timekeeper Chip. It provides seconds
> , minutes, hours,  day of the week, date, month and year information.
> 
> Datasheet:
> https://www.holtek.com.tw/documents/10179/11842/ht1380_1v130.pdf
> 
> Signed-off-by: Sergei Antonov <saproj@gmail.com>

Please enable this on sandbox so that it gets build tested and not
removed later as unused / dead code, thanks.

On Wed, 7 Dec 2022 at 04:08, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sergei,
>
> On Tue, 6 Dec 2022 at 23:07, Sergei Antonov <saproj@gmail.com> wrote:
> >
> > Support Holtek HT1380/HT1381 Serial Timekeeper Chip. It provides seconds
> > , minutes, hours,  day of the week, date, month and year information.
> >
> > Datasheet:
> > https://www.holtek.com.tw/documents/10179/11842/ht1380_1v130.pdf
> >
> > Signed-off-by: Sergei Antonov <saproj@gmail.com>
> > ---
> >
> > v2:
> > * The RESET pin is now to be described as ACTIVE_LOW in dts.
> >
> > Changes suggested by Simon Glass:
> > * a more detailed driver description in Kconfig
> > * multi-line comments' style
> > * enum for 0x80 and the 0x20 at top of file
> > * lower-case hex constants
> > * function comments for ht1380_reset_on/off
> > * blank line before returns
> >
> > PROTECT remains in a function scope for the sake of locality of definitions.
> >
> >  drivers/rtc/Kconfig  |   8 +
> >  drivers/rtc/Makefile |   1 +
> >  drivers/rtc/ht1380.c | 337 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 346 insertions(+)
> >  create mode 100644 drivers/rtc/ht1380.c
> >
> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > index 23963271928a..eed48e35a578 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -220,4 +220,12 @@ config RTC_ZYNQMP
> >           Say "yes" here to support the on chip real time clock
> >           present on Xilinx ZynqMP SoC.
> >
> > +config RTC_HT1380
> > +       bool "Enable Holtek HT1380/HT1381 RTC driver"
> > +       depends on DM_RTC && DM_GPIO
> > +       help
> > +         Say "yes" here to get support for Holtek HT1380/HT1381
> > +         Serial Timekeeper IC which provides seconds, minutes, hours,
> > +         day of the week, date, month and year information.
>
> Perhaps mention how it is connected, i.e. three GPIOs.
>
> > +
> >  endmenu
> > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> > index 009dd9d28c95..f3164782b605 100644
> > --- a/drivers/rtc/Makefile
> > +++ b/drivers/rtc/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_RTC_DS3231) += ds3231.o
> >  obj-$(CONFIG_RTC_DS3232) += ds3232.o
> >  obj-$(CONFIG_RTC_EMULATION) += emul_rtc.o
> >  obj-$(CONFIG_RTC_FTRTC010) += ftrtc010.o
> > +obj-$(CONFIG_RTC_HT1380) += ht1380.o
> >  obj-$(CONFIG_SANDBOX) += i2c_rtc_emul.o
> >  obj-$(CONFIG_RTC_IMXDI) += imxdi.o
> >  obj-$(CONFIG_RTC_ISL1208) += isl1208.o
> > diff --git a/drivers/rtc/ht1380.c b/drivers/rtc/ht1380.c
> > new file mode 100644
> > index 000000000000..25335227d893
> > --- /dev/null
> > +++ b/drivers/rtc/ht1380.c
> > @@ -0,0 +1,337 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Holtek HT1380/HT1381 Serial Timekeeper Chip
> > + *
> > + * Communication with the chip is vendor-specific.
> > + * It is done via 3 GPIO pins: reset, clock, and data.
> > + * Describe in .dts this way:
> > + *
> > + * rtc {
> > + *         compatible = "holtek,ht1380";
> > + *         rst-gpio = <&gpio 19 GPIO_ACTIVE_LOW>;
> > + *         clk-gpio = <&gpio 20 GPIO_ACTIVE_HIGH>;
> > + *         dat-gpio = <&gpio 21 GPIO_ACTIVE_HIGH>;
> > + * };
>
> Is there a binding file for this?

Not in the U-Boot repo yet. I am going to submit a .dts file binding
to this driver. But I need to submit 2 more device drivers first. I
guess, submitting .dts should be the final step.

> I believe the standard name should be rst-gpios (i.e. plural)?

I grepped files in arch/arm/dts and it was not clear. There were more
plural names however. So I can change it to plural in v3.

> > + *
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <rtc.h>
> > +#include <bcd.h>
> > +#include <asm/gpio.h>
> > +#include <linux/delay.h>
> > +
> > +struct ht1380_priv {
> > +       struct gpio_desc rst_desc;
> > +       struct gpio_desc clk_desc;
> > +       struct gpio_desc dat_desc;
> > +};
> > +
> > +enum registers {
> > +       SEC,
> > +       MIN,
> > +       HOUR,
> > +       MDAY,
> > +       MONTH,
> > +       WDAY,
> > +       YEAR,
> > +       WP,
> > +       N_REGS
> > +};
> > +
> > +enum hour_mode {
> > +       AMPM_MODE = 0x80, /* RTC is in AM/PM mode */
> > +       PM_NOW = 0x20,    /* set if PM, clear if AM */
> > +};
> > +
> > +static const int BURST = 0xbe;
> > +static const int READ = 1;
> > +
> > +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.
> > +        */
> > +       udelay(1);
> > +}
> > +
> > +static int ht1380_send_byte(struct ht1380_priv *priv, int byte)
> > +{
> > +       int ret;
> > +
> > +       for (int bit = 0; bit < 8; bit++) {
> > +               ret = dm_gpio_set_value(&priv->dat_desc, byte >> bit & 1);
> > +               if (ret)
> > +                       break;
> > +               ht1380_half_period_delay();
> > +
> > +               ret = dm_gpio_set_value(&priv->clk_desc, 1);
> > +               if (ret)
> > +                       break;
> > +               ht1380_half_period_delay();
> > +
> > +               ret = dm_gpio_set_value(&priv->clk_desc, 0);
> > +               if (ret)
> > +                       break;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +/*
> > + * Leave reset state. The transfer operation can then be started.
> > + */
> > +static int ht1380_reset_off(struct ht1380_priv *priv)
> > +{
> > +       const unsigned int T_CC = 4; /* us, Reset to Clock Setup */
> > +       int ret;
> > +
> > +       /*
> > +        * Leave RESET state.
> > +        * Make sure we make the minimal delay required by the datasheet.
> > +        */
> > +       ret = dm_gpio_set_value(&priv->rst_desc, 0);
> > +       udelay(T_CC);
> > +
> > +       return ret;
> > +}
> > +
> > +/*
> > + * Enter reset state. Completes the transfer operation.
> > + */
> > +static int ht1380_reset_on(struct ht1380_priv *priv)
> > +{
> > +       const unsigned int T_CWH = 4; /* us, Reset Inactive Time */
> > +       int ret;
> > +
> > +       /*
> > +        * Enter RESET state.
> > +        * Make sure we make the minimal delay required by the datasheet.
> > +        */
> > +       ret = dm_gpio_set_value(&priv->rst_desc, 1);
> > +       udelay(T_CWH);
> > +
> > +       return ret;
> > +}
> > +
> > +static int ht1380_rtc_get(struct udevice *dev, struct rtc_time *tm)
> > +{
> > +       struct ht1380_priv *priv = dev_get_priv(dev);
> > +       int ret, i, bit, reg[N_REGS];
> > +
> > +       ret = dm_gpio_set_value(&priv->clk_desc, 0);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = dm_gpio_set_dir_flags(&priv->dat_desc, GPIOD_IS_OUT);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = ht1380_reset_off(priv);
> > +       if (ret)
> > +               goto exit;
> > +
> > +       ret = ht1380_send_byte(priv, BURST + READ);
> > +       if (ret)
> > +               goto exit;
> > +
> > +       ret = dm_gpio_set_dir_flags(&priv->dat_desc, GPIOD_IS_IN);
> > +       if (ret)
> > +               goto exit;
> > +
>
> Is this some sort of I2C protocol?

Like I2C it uses a pin for clock and a pin for data in/out. Unlike I2C
it does not use addressing. I am not sure whether this driver can
utilize some of the existing I2C code in U-Boot. Wrote my own bit
banging routines.

> > +       for (i = 0; i < N_REGS; i++) {
> > +               reg[i] = 0;
> > +
> > +               for (bit = 0; bit < 8; bit++) {
> > +                       ht1380_half_period_delay();
> > +
> > +                       ret = dm_gpio_set_value(&priv->clk_desc, 1);
> > +                       if (ret)
> > +                               goto exit;
> > +                       ht1380_half_period_delay();
> > +
> > +                       reg[i] |= dm_gpio_get_value(&priv->dat_desc) << bit;
> > +                       ret = dm_gpio_set_value(&priv->clk_desc, 0);
> > +                       if (ret)
> > +                               goto exit;
> > +               }
> > +       }
> > +
> > +       ret = -EINVAL;
> > +
> > +       /* Correctness check: some bits are always zero */
> > +       if ((reg[MIN] & 0x80) || (reg[HOUR] & 0x40) || (reg[MDAY] & 0xc0) ||
> > +           (reg[MONTH] & 0xe0) || (reg[WDAY] & 0xf8) || (reg[WP] & 0x7f))
> > +               goto exit;
>
> Drop extra brackets ?

OK. Will be done in v3. I put extra brackets expecting reviewers to
criticize code readability :).

> > +
> > +       /* Correctness check: some registers are always non-zero */
> > +       if (!reg[MDAY] || !reg[MONTH] || !reg[WDAY])
> > +               goto exit;
> > +
> > +       tm->tm_sec = bcd2bin(reg[SEC]);
> > +       tm->tm_min = bcd2bin(reg[MIN]);
> > +       if (reg[HOUR] & AMPM_MODE) {
> > +               /* AM-PM Mode, range is 01-12 */
> > +               tm->tm_hour = bcd2bin(reg[HOUR] & 0x1f) % 12;
> > +               if (reg[HOUR] & PM_NOW) {
> > +                       /* 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;
> > +
> > +       ret = ht1380_reset_off(priv);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = ht1380_send_byte(priv, PROTECT);
> > +       if (ret)
> > +               return ret;
> > +       ret = ht1380_send_byte(priv, 0); /* WP bit is 0 */
> > +       if (ret)
> > +               return ret;
> > +
> > +       return ht1380_reset_on(priv);
> > +}
> > +
> > +static int ht1380_rtc_set(struct udevice *dev, const struct rtc_time *tm)
> > +{
> > +       struct ht1380_priv *priv = dev_get_priv(dev);
> > +       int ret, i, reg[N_REGS];
> > +
> > +       ret = dm_gpio_set_value(&priv->clk_desc, 0);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = dm_gpio_set_dir_flags(&priv->dat_desc, GPIOD_IS_OUT);
> > +       if (ret)
> > +               goto exit;
> > +
> > +       ret = ht1380_write_protection_off(priv);
> > +       if (ret)
> > +               goto exit;
> > +
> > +       reg[SEC] = bin2bcd(tm->tm_sec);
> > +       reg[MIN] = bin2bcd(tm->tm_min);
> > +       reg[HOUR] = bin2bcd(tm->tm_hour);
> > +       reg[MDAY] = bin2bcd(tm->tm_mday);
> > +       reg[MONTH] = bin2bcd(tm->tm_mon);
> > +       reg[WDAY] = bin2bcd(tm->tm_wday) + 1;
> > +       reg[YEAR] = bin2bcd(tm->tm_year - 2000);
> > +       reg[WP] = 0x80; /* WP bit is 1 */
> > +
> > +       ret = ht1380_reset_off(priv);
> > +       if (ret)
> > +               goto exit;
> > +
> > +       ret = ht1380_send_byte(priv, BURST);
> > +       for (i = 0; i < N_REGS && ret; i++)
> > +               ret = ht1380_send_byte(priv, reg[i]);
> > +
> > +exit:
> > +       ht1380_reset_on(priv);
> > +
> > +       return ret;
> > +}
> > +
> > +static int ht1380_probe(struct udevice *dev)
> > +{
> > +       int ret;
> > +       struct ht1380_priv *priv;
> > +
> > +       priv = dev_get_priv(dev);
> > +       if (!priv)
> > +               return -EINVAL;
> > +
> > +       ret = gpio_request_by_name(dev, "rst-gpio", 0,
> > +                                  &priv->rst_desc, 0);
>
> You can pass GPIOD_IS_OUT as the last param and drop the extra code below.

Thanks! Did not know it.

> > +       if (ret)
> > +               goto fail_rst;
> > +
> > +       ret = gpio_request_by_name(dev, "clk-gpio", 0,
> > +                                  &priv->clk_desc, 0);
> > +       if (ret)
> > +               goto fail_clk;
> > +
> > +       ret = gpio_request_by_name(dev, "dat-gpio", 0,
> > +                                  &priv->dat_desc, 0);
> > +       if (ret)
> > +               goto fail_dat;
> > +
> > +       ret = dm_gpio_set_dir_flags(&priv->clk_desc, GPIOD_IS_OUT);
> > +       if (ret)
> > +               goto fail;
> > +
> > +       ret = dm_gpio_set_dir_flags(&priv->rst_desc, GPIOD_IS_OUT);
> > +       if (ret)
> > +               goto fail;
> > +
> > +       ret = ht1380_reset_on(priv);
> > +       if (ret)
> > +               goto fail;
> > +
> > +       return 0;
> > +
> > +fail:
> > +       dm_gpio_free(dev, &priv->dat_desc);
> > +fail_dat:
> > +       dm_gpio_free(dev, &priv->clk_desc);
> > +fail_clk:
> > +       dm_gpio_free(dev, &priv->rst_desc);
> > +fail_rst:
> > +       return ret;
> > +}
> > +
> > +static int ht1380_remove(struct udevice *dev)
> > +{
> > +       struct ht1380_priv *priv = dev_get_priv(dev);
> > +
> > +       dm_gpio_free(dev, &priv->rst_desc);
> > +       dm_gpio_free(dev, &priv->clk_desc);
> > +       dm_gpio_free(dev, &priv->dat_desc);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct rtc_ops ht1380_rtc_ops = {
> > +       .get = ht1380_rtc_get,
> > +       .set = ht1380_rtc_set,
> > +};
> > +
> > +static const struct udevice_id ht1380_rtc_ids[] = {
> > +       { .compatible = "holtek,ht1380" },
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(rtc_ht1380) = {
> > +       .name = "rtc-ht1380",
> > +       .id = UCLASS_RTC,
> > +       .probe = ht1380_probe,
> > +       .remove = ht1380_remove,
> > +       .of_match = ht1380_rtc_ids,
> > +       .ops = &ht1380_rtc_ops,
> > +       .priv_auto = sizeof(struct ht1380_priv),
> > +};
> > --
> > 2.34.1
> >
>
> Regards,
> Simon

Hi Sergei,

On Thu, 8 Dec 2022 at 06:09, Sergei Antonov <saproj@gmail.com> wrote:
>
> On Wed, 7 Dec 2022 at 04:08, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sergei,
> >
> > On Tue, 6 Dec 2022 at 23:07, Sergei Antonov <saproj@gmail.com> wrote:
> > >
> > > Support Holtek HT1380/HT1381 Serial Timekeeper Chip. It provides seconds
> > > , minutes, hours,  day of the week, date, month and year information.
> > >
> > > Datasheet:
> > > https://www.holtek.com.tw/documents/10179/11842/ht1380_1v130.pdf
> > >
> > > Signed-off-by: Sergei Antonov <saproj@gmail.com>
> > > ---
> > >
> > > v2:
> > > * The RESET pin is now to be described as ACTIVE_LOW in dts.
> > >
> > > Changes suggested by Simon Glass:
> > > * a more detailed driver description in Kconfig
> > > * multi-line comments' style
> > > * enum for 0x80 and the 0x20 at top of file
> > > * lower-case hex constants
> > > * function comments for ht1380_reset_on/off
> > > * blank line before returns
> > >
> > > PROTECT remains in a function scope for the sake of locality of definitions.
> > >
> > >  drivers/rtc/Kconfig  |   8 +
> > >  drivers/rtc/Makefile |   1 +
> > >  drivers/rtc/ht1380.c | 337 +++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 346 insertions(+)
> > >  create mode 100644 drivers/rtc/ht1380.c
> > >

[..]

> > > +static int ht1380_rtc_get(struct udevice *dev, struct rtc_time *tm)
> > > +{
> > > +       struct ht1380_priv *priv = dev_get_priv(dev);
> > > +       int ret, i, bit, reg[N_REGS];
> > > +
> > > +       ret = dm_gpio_set_value(&priv->clk_desc, 0);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       ret = dm_gpio_set_dir_flags(&priv->dat_desc, GPIOD_IS_OUT);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       ret = ht1380_reset_off(priv);
> > > +       if (ret)
> > > +               goto exit;
> > > +
> > > +       ret = ht1380_send_byte(priv, BURST + READ);
> > > +       if (ret)
> > > +               goto exit;
> > > +
> > > +       ret = dm_gpio_set_dir_flags(&priv->dat_desc, GPIOD_IS_IN);
> > > +       if (ret)
> > > +               goto exit;
> > > +
> >
> > Is this some sort of I2C protocol?
>
> Like I2C it uses a pin for clock and a pin for data in/out. Unlike I2C
> it does not use addressing. I am not sure whether this driver can
> utilize some of the existing I2C code in U-Boot. Wrote my own bit
> banging routines.

Yes you can use i2c, by setting the offset_len to 0, e.g. with:

   u-boot,i2c-offset-len = <0>;

>
> > > +       for (i = 0; i < N_REGS; i++) {
> > > +               reg[i] = 0;
> > > +
> > > +               for (bit = 0; bit < 8; bit++) {
> > > +                       ht1380_half_period_delay();
> > > +
> > > +                       ret = dm_gpio_set_value(&priv->clk_desc, 1);
> > > +                       if (ret)
> > > +                               goto exit;
> > > +                       ht1380_half_period_delay();
> > > +
> > > +                       reg[i] |= dm_gpio_get_value(&priv->dat_desc) << bit;
> > > +                       ret = dm_gpio_set_value(&priv->clk_desc, 0);
> > > +                       if (ret)
> > > +                               goto exit;
> > > +               }
> > > +       }
> > > +
> > > +       ret = -EINVAL;
> > > +
> > > +       /* Correctness check: some bits are always zero */
> > > +       if ((reg[MIN] & 0x80) || (reg[HOUR] & 0x40) || (reg[MDAY] & 0xc0) ||
> > > +           (reg[MONTH] & 0xe0) || (reg[WDAY] & 0xf8) || (reg[WP] & 0x7f))
> > > +               goto exit;
> >
> > Drop extra brackets ?
>
> OK. Will be done in v3. I put extra brackets expecting reviewers to
> criticize code readability :).

:)

Regards,
Simon

On Thu, 8 Dec 2022 at 00:52, Simon Glass <sjg@chromium.org> wrote:
> > > Is this some sort of I2C protocol?
> >
> > Like I2C it uses a pin for clock and a pin for data in/out. Unlike I2C
> > it does not use addressing. I am not sure whether this driver can
> > utilize some of the existing I2C code in U-Boot. Wrote my own bit
> > banging routines.
>
> Yes you can use i2c, by setting the offset_len to 0, e.g. with:
>
>    u-boot,i2c-offset-len = <0>;

I2C transmits/receives bytes most significant bit first (msb), but
this device needs lsb.

Hi Sergei,

On Fri, 9 Dec 2022 at 00:50, Sergei Antonov <saproj@gmail.com> wrote:
>
> On Thu, 8 Dec 2022 at 00:52, Simon Glass <sjg@chromium.org> wrote:
> > > > Is this some sort of I2C protocol?
> > >
> > > Like I2C it uses a pin for clock and a pin for data in/out. Unlike I2C
> > > it does not use addressing. I am not sure whether this driver can
> > > utilize some of the existing I2C code in U-Boot. Wrote my own bit
> > > banging routines.
> >
> > Yes you can use i2c, by setting the offset_len to 0, e.g. with:
> >
> >    u-boot,i2c-offset-len = <0>;
>
> I2C transmits/receives bytes most significant bit first (msb), but
> this device needs lsb.

Also it seems that it is not actually using the i2c protocol, even
with that change.

Regards,
Simon