[PATCH 2/2] drivers: rtc: add max313xx series rtc driver

[Simon Glass]

Hi Chris,

On Thu, 16 Mar 2023 at 16:16, Chris Packham <judge.packham at gmail.com> wrote:
>
> Adding support for Analog Devices MAX313XX series RTCs.
>
> This is ported from the Linux driver and adapted for use in u-boot.
> Notable differences are
> - handling of tm_year and tm_mon differ
> - clock source support is omitted
> - hwmon support for the MAX31328 and MAX31343 is omitted
> - rtc_ops->reset is added
>
> Signed-off-by: Chris Packham <judge.packham at gmail.com>
> ---
>
>  drivers/rtc/Kconfig    |   8 +
>  drivers/rtc/Makefile   |   1 +
>  drivers/rtc/max313xx.c | 442 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 451 insertions(+)
>  create mode 100644 drivers/rtc/max313xx.c

Reviewed-by: Simon Glass <sjg at chromium.org>

with requests below

>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 35b6ed4d7c72..49c260b5b190 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -134,6 +134,14 @@ config RTC_ISL1208
>           This driver supports reading and writing the RTC/calendar and detects
>           total power failures.
>
> +config RTC_MAX313XX
> +       bool "Analog Devices MAX313XX RTC driver"
> +       depends on DM_RTC
> +       depends on DM_I2C
> +       help
> +         If you say yes here you will get support for the
> +         Analog Devices MAX313XX series RTC family.

Can you mention the features the chip supports and anything notable
about the driver support/lack of support? E.g. does it have CMOS RAM?

> +
>  config RTC_PCF8563
>         tristate "Philips PCF8563"
>         help
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 447551e15aa2..adfa23f66702 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_RTC_HT1380) += ht1380.o
>  obj-$(CONFIG_SANDBOX) += i2c_rtc_emul.o
>  obj-$(CONFIG_RTC_ISL1208) += isl1208.o
>  obj-$(CONFIG_RTC_M41T62) += m41t62.o
> +obj-$(CONFIG_RTC_MAX313XX) += max313xx.o
>  obj-$(CONFIG_RTC_MC13XXX) += mc13xxx-rtc.o
>  obj-$(CONFIG_RTC_MC146818) += mc146818.o
>  obj-$(CONFIG_MCFRTC) += mcfrtc.o
> diff --git a/drivers/rtc/max313xx.c b/drivers/rtc/max313xx.c
> new file mode 100644
> index 000000000000..1aa430d121ee
> --- /dev/null
> +++ b/drivers/rtc/max313xx.c
> @@ -0,0 +1,442 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices MAX313XX series I2C RTC driver
> + *
> + * Copyright 2022 Analog Devices Inc.
> + */
> +#include <bcd.h>
> +#include <dm.h>
> +#include <dm/device_compat.h>
> +#include <i2c.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <rtc.h>

Please check header order...the dm/ and linux/ ones should go at the end

https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files

[..]

> +struct chip_desc {
> +       struct clkout_cfg *clkout;
> +       const char *clkout_name;
> +       u8 sec_reg;
> +       u8 alarm1_sec_reg;
> +
> +       u8 int_en_reg;
> +       u8 int_status_reg;
> +
> +       u8 ram_reg;
> +       u8 ram_size;
> +
> +       u8 temp_reg;
> +
> +       u8 trickle_reg;
> +
> +       u8 rst_reg;
> +       u8 rst_bit;

Please comment this struct

> +};
> +
> +struct max313xx {

The convention is to use a _priv suffix on this type. If this would
increase the diff from Linux, then it is OK as is.

> +       enum max313xx_ids id;
> +       const struct chip_desc *chip;
> +};
> +

[..]

[..]

> +
> +static int max313xx_trickle_charger_setup(struct udevice *dev)
> +{
> +       struct max313xx *rtc = dev_get_priv(dev);
> +       bool diode;
> +       int index, reg;
> +       u32 ohms;
> +       u32 chargeable;
> +       int ret;
> +
> +       if (dev_read_u32(dev, "trickle-resistor-ohms", &ohms) ||
> +           dev_read_u32(dev, "aux-voltage-chargeable", &chargeable))
> +               return 0;
> +
> +       switch (chargeable) {
> +       case 0:
> +               diode = false;
> +               break;
> +       case 1:
> +               diode = true;
> +               break;
> +       default:
> +               dev_err(dev, "unsupported aux-voltage-chargeable value\n");
> +               return -EINVAL;

consider deb_dbg() as code size is smaller

> +       }
> +
> +       if (!rtc->chip->trickle_reg) {
> +               dev_warn(dev, "device does not have trickle charger\n");
> +               return 0;

-ENOTSUPP ?

[..]

Regards,
Simon