Hi Chris, On Thu, 16 Mar 2023 at 16:16, Chris Packham <judge.pack...@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.pack...@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 <s...@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
