Add support for NXP's PCF85053A RTC chip.
Signed-off-by: Carlos Menin <[email protected]>
Reviewed-by: Sergio Prado <[email protected]>
---
drivers/rtc/Kconfig | 9 +
drivers/rtc/Makefile | 1 +
drivers/rtc/rtc-pcf85053a.c | 689 ++++++++++++++++++++++++++++++++++++
3 files changed, 699 insertions(+)
create mode 100644 drivers/rtc/rtc-pcf85053a.c
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 3814e0845e77..ab33940070d1 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -460,6 +460,15 @@ config RTC_DRV_PCF8523
This driver can also be built as a module. If so, the module
will be called rtc-pcf8523.
+config RTC_DRV_PCF85053A
+ tristate "NXP PCF85053A"
+ select REGMAP_I2C
+ help
+ If you say yes here you get support for the PCF85053A RTC chip
+
+ This driver can also be built as a module. If so, the module
+ will be called rtc-pcf85053a.
+
config RTC_DRV_PCF85063
tristate "NXP PCF85063"
select REGMAP_I2C
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 7b03c3abfd78..3f3a1ab8acb0 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -122,6 +122,7 @@ obj-$(CONFIG_RTC_DRV_PCAP) += rtc-pcap.o
obj-$(CONFIG_RTC_DRV_PCF2123) += rtc-pcf2123.o
obj-$(CONFIG_RTC_DRV_PCF2127) += rtc-pcf2127.o
obj-$(CONFIG_RTC_DRV_PCF50633) += rtc-pcf50633.o
+obj-$(CONFIG_RTC_DRV_PCF85053A) += rtc-pcf85053a.o
obj-$(CONFIG_RTC_DRV_PCF85063) += rtc-pcf85063.o
obj-$(CONFIG_RTC_DRV_PCF8523) += rtc-pcf8523.o
obj-$(CONFIG_RTC_DRV_PCF85363) += rtc-pcf85363.o
diff --git a/drivers/rtc/rtc-pcf85053a.c b/drivers/rtc/rtc-pcf85053a.c
new file mode 100644
index 000000000000..0474d90ebe52
--- /dev/null
+++ b/drivers/rtc/rtc-pcf85053a.c
@@ -0,0 +1,689 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/rtc.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/bcd.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/hwmon.h>
+
+#define ADDR_NVMEM 0x57
+
+#define REG_SECS 0x00
+#define REG_MINUTES 0x02
+#define REG_HOURS 0x04
+#define REG_WEEKDAYS 0x06
+#define REG_DAYS 0x07
+#define REG_MONTHS 0x08
+#define REG_YEARS 0x09
+
+#define REG_SECOND_ALM 0x01
+#define REG_MINUTE_ALM 0x03
+#define REG_HOUR_ALM 0x05
+
+#define REG_CTRL 0x0a
+#define REG_CTRL_ST BIT(7)
+#define REG_CTRL_DM BIT(6)
+#define REG_CTRL_HF BIT(5)
+#define REG_CTRL_DSM BIT(4)
+#define REG_CTRL_AIE BIT(3)
+#define REG_CTRL_OFIE BIT(2)
+#define REG_CTRL_CIE BIT(1)
+#define REG_CTRL_TWO BIT(0)
+
+#define REG_STATUS 0x0b
+#define REG_STATUS_AF BIT(7)
+#define REG_STATUS_OF BIT(6)
+#define REG_STATUS_RTCF BIT(5)
+#define REG_STATUS_CIF BIT(4)
+#define REG_STATUS_BVL GENMASK(2, 0)
+
+#define REG_CLKOUT 0x0c
+#define REG_CLKOUT_CKE BIT(7)
+#define REG_CLKOUT_CKD GENMASK(1, 0)
+
+#define REG_CTRL2 0x0d
+#define REG_CTRL2_MWO BIT(0)
+
+#define REG_SCRATCHPAD 0x0e
+
+#define REG_VERSION 0x0f
+#define REG_VENDOR 0x10
+#define REG_MODEL 0x11
+
+#define REG_OFFSET 0x12
+
+#define REG_OSCILLATOR 0x13
+#define REG_OSC_CLKIV BIT(7)
+#define REG_OSC_OFFM BIT(6)
+#define REG_OSC_LOWJ BIT(4)
+#define REG_OSC_OSCD GENMASK(3, 2)
+#define REG_OSC_CL GENMASK(1, 0)
+
+#define REG_ACCESS 0x14
+#define REG_ACCESS_XCLK BIT(7)
+
+#define REG_SEC_TS 0x15
+#define REG_MIN_TS 0x16
+#define REG_HOUR_TS 0x17
+#define REG_DAYWK_TS 0x18
+#define REG_DAYMON_TS 0x19
+#define REG_MON_TS 0x1a
+#define REG_YEAR_TS 0x1b
+
+#define REG_RCODE1 0x1c
+#define REG_RCODE2 0x1d
+
+#define OFFSET_STEP0 2170
+#define OFFSET_STEP1 2034
+
+struct pcf85053a {
+ struct rtc_device *rtc;
+ struct regmap *regmap;
+ struct regmap *regmap_nvmem;
+};
+
+struct pcf85053a_config {
+ struct regmap_config regmap;
+ struct regmap_config regmap_nvmem;
+};
+
+static int pcf85053a_read_offset(struct device *dev, long *offset)
+{
+ struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
+ long val;
+ u32 reg_offset, reg_oscillator;
+ int ret;
+
+ ret = regmap_read(pcf85053a->regmap, REG_OFFSET, ®_offset);
+ if (ret)
+ return -EIO;
+
+ ret = regmap_read(pcf85053a->regmap, REG_OSCILLATOR, ®_oscillator);
+ if (ret)
+ return -EIO;
+
+ val = sign_extend32(reg_offset, 7);
+
+ if (reg_oscillator & REG_OSC_OFFM)
+ *offset = val * OFFSET_STEP1;
+ else
+ *offset = val * OFFSET_STEP0;
+
+ return 0;
+}
+
+static int pcf85053a_set_offset(struct device *dev, long offset)
+{
+ struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
+ s8 mode0, mode1, reg_offset;
+ unsigned int ret, error0, error1;
+
+ if (offset > OFFSET_STEP0 * 127)
+ return -ERANGE;
+ if (offset < OFFSET_STEP0 * -128)
+ return -ERANGE;
+
+ ret = regmap_set_bits(pcf85053a->regmap, REG_ACCESS, REG_ACCESS_XCLK);
+ if (ret)
+ return -EIO;
+
+ mode0 = DIV_ROUND_CLOSEST(offset, OFFSET_STEP0);
+ mode1 = DIV_ROUND_CLOSEST(offset, OFFSET_STEP1);
+
+ error0 = abs(offset - (mode0 * OFFSET_STEP0));
+ error1 = abs(offset - (mode1 * OFFSET_STEP1));
+ if (error0 < error1) {
+ reg_offset = mode0;
+ ret = regmap_clear_bits(pcf85053a->regmap, REG_OSCILLATOR,
+ REG_OSC_OFFM);
+ } else {
+ reg_offset = mode1;
+ ret = regmap_set_bits(pcf85053a->regmap, REG_OSCILLATOR,
+ REG_OSC_OFFM);
+ }
+ if (ret)
+ return -EIO;
+
+ ret = regmap_write(pcf85053a->regmap, REG_OFFSET, reg_offset);
+
+ return ret;
+}
+
+static int pcf85053a_rtc_check_reliability(struct device *dev, u8 status_reg)
+{
+ int ret = 0;
+
+ if (status_reg & REG_STATUS_CIF) {
+ dev_warn(dev, "tamper detected,"
+ " date/time is not reliable\n");
+ ret = -EINVAL;
+ }
+
+ if (status_reg & REG_STATUS_OF) {
+ dev_warn(dev, "oscillator fail detected,"
+ " date/time is not reliable.\n");
+ ret = -EINVAL;
+ }
+
+ if (status_reg & REG_STATUS_RTCF) {
+ dev_warn(dev, "power loss detected,"
+ " date/time is not reliable.\n");
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static int pcf85053a_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+ struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
+ u8 buf[REG_STATUS + 1];
+ int ret, len = sizeof(buf);
+
+ ret = regmap_bulk_read(pcf85053a->regmap, REG_SECS, buf, len);
+ if (ret) {
+ dev_err(dev, "%s: error %d\n", __func__, ret);
+ return ret;
+ }
+
+ ret = pcf85053a_rtc_check_reliability(dev, buf[REG_STATUS]);
+ if (ret)
+ return ret;
+
+ tm->tm_year = buf[REG_YEARS];
+ /* adjust for 1900 base of rtc_time */
+ tm->tm_year += 100;
+
+ tm->tm_wday = (buf[REG_WEEKDAYS] - 1) & 7; /* 1 - 7 */
+ tm->tm_sec = buf[REG_SECS];
+ tm->tm_min = buf[REG_MINUTES];
+ tm->tm_hour = buf[REG_HOURS];
+ tm->tm_mday = buf[REG_DAYS];
+ tm->tm_mon = buf[REG_MONTHS] - 1; /* 1 - 12 */
+
+ return 0;
+}
+
+static int pcf85053a_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+ int ret;
+ struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
+ struct reg_sequence regs[] = {
+ REG_SEQ0(REG_SECS, tm->tm_sec),
+ REG_SEQ0(REG_MINUTES, tm->tm_min),
+ REG_SEQ0(REG_HOURS, tm->tm_hour),
+ REG_SEQ0(REG_WEEKDAYS, (tm->tm_wday + 1) & 0x7), /* 1 - 7 */
+ REG_SEQ0(REG_DAYS, tm->tm_mday),
+ REG_SEQ0(REG_MONTHS, tm->tm_mon + 1), /* 1 - 12 */
+ REG_SEQ0(REG_YEARS, tm->tm_year % 100),
+ };
+
+ /* tamper event will clear this bit */
+ ret = regmap_set_bits(pcf85053a->regmap, REG_CTRL, REG_CTRL_TWO);
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(pcf85053a->regmap, REG_CTRL, REG_CTRL_ST);
+ if (ret)
+ return ret;
+
+ ret = regmap_multi_reg_write(pcf85053a->regmap, regs, ARRAY_SIZE(regs));
+ if (ret)
+ return ret;
+
+ ret = regmap_clear_bits(pcf85053a->regmap, REG_CTRL, REG_CTRL_ST);
+ if (ret)
+ return ret;
+
+ ret = regmap_clear_bits(pcf85053a->regmap, REG_STATUS, REG_STATUS_OF);
+
+ return ret;
+}
+
+static int pcf85053a_bvl_to_mv(unsigned int bvl)
+{
+ long mv_table[] = {
+ 1700,
+ 1900,
+ 2100,
+ 2300,
+ 2500,
+ 2700,
+ 2900,
+ 3100,
+ };
+ return mv_table[bvl & 7];
+}
+
+static int pcf85053a_hwmon_read_in(struct device *dev, long *mV)
+{
+ struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
+ unsigned int status;
+ int ret;
+
+ ret = regmap_read(pcf85053a->regmap, REG_STATUS, &status);
+ if (ret)
+ return ret;
+
+ *mV = pcf85053a_bvl_to_mv(status & REG_STATUS_BVL);
+ return 0;
+}
+
+static umode_t pcf85053a_hwmon_is_visible(const void *data,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ if (type != hwmon_in)
+ return 0;
+
+ switch (attr) {
+ case hwmon_in_input:
+ return 0444;
+ default:
+ return 0;
+ }
+}
+
+static int pcf85053a_hwmon_read(struct device *dev,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ int ret;
+
+ switch (attr) {
+ case hwmon_in_input:
+ ret = pcf85053a_hwmon_read_in(dev, val);
+ break;
+ default:
+ ret = -EOPNOTSUPP;
+ break;
+ }
+
+ return ret;
+}
+
+static u32 pcf85053a_hwmon_in_config[] = {
+ HWMON_I_INPUT,
+ 0
+};
+
+static const struct hwmon_channel_info pcf85053a_hwmon_in = {
+ .type = hwmon_in,
+ .config = pcf85053a_hwmon_in_config,
+};
+
+static const struct hwmon_channel_info *pcf85053a_hwmon_info[] = {
+ &pcf85053a_hwmon_in,
+ 0
+};
+
+static const struct hwmon_ops pcf85053a_hwmon_ops = {
+ .is_visible = pcf85053a_hwmon_is_visible,
+ .read = pcf85053a_hwmon_read,
+};
+
+static const struct hwmon_chip_info pcf85053a_hwmon_chip_info = {
+ .ops = &pcf85053a_hwmon_ops,
+ .info = pcf85053a_hwmon_info,
+};
+
+static int pcf85053a_hwmon_register(struct device *dev, const char *name)
+{
+ struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
+ struct device *hwmon_dev;
+
+ hwmon_dev = devm_hwmon_device_register_with_info(dev, name, pcf85053a,
+ &pcf85053a_hwmon_chip_info,
+ 0);
+ if (IS_ERR(hwmon_dev)) {
+ dev_err(dev, "unable to register hwmon device: %ld\n",
+ PTR_ERR(hwmon_dev));
+ return PTR_ERR(hwmon_dev);
+ }
+
+ return 0;
+}
+
+static int pcf85053a_nvmem_read(void *priv, unsigned int offset, void *val,
+ size_t num)
+{
+ int ret;
+ struct pcf85053a *pcf85053a = priv;
+ struct regmap *regmap_nvmem = pcf85053a->regmap_nvmem;
+
+ ret = regmap_bulk_read(regmap_nvmem, offset, val, num);
+ if (ret)
+ pr_warn("%s: failed to read nvmem: %d\n", __func__, ret);
+
+ return ret;
+}
+
+static int pcf85053a_nvmem_write(void *priv, unsigned int offset, void *val,
+ size_t num)
+{
+ int ret;
+ struct pcf85053a *pcf85053a = priv;
+ struct regmap *regmap_nvmem = pcf85053a->regmap_nvmem;
+
+ /* tamper event will clear this bit */
+ ret = regmap_set_bits(pcf85053a->regmap, REG_CTRL2, REG_CTRL2_MWO);
+ if (ret) {
+ pr_warn("%s: failed to enable nvmem write: %d", __func__, ret);
+ return ret;
+ }
+
+ ret = regmap_bulk_write(regmap_nvmem, offset, val, num);
+ if (ret)
+ pr_warn("%s: failed to write nvmem: %d\n", __func__, ret);
+
+ return ret;
+}
+
+static ssize_t attr_flag_clear(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count,
+ u8 reg, u8 flag)
+{
+ struct pcf85053a *pcf85053a = dev_get_drvdata(dev->parent);
+ int ret;
+
+ (void)attr;
+ (void)buf;
+ (void)count;
+
+ ret = regmap_clear_bits(pcf85053a->regmap, reg, flag);
+ if (ret)
+ return -EIO;
+
+ return count;
+}
+
+static ssize_t attr_flag_read(struct device *dev,
+ struct device_attribute *attr,
+ char *buf,
+ u8 reg, u8 flag)
+{
+ struct pcf85053a *pcf85053a = dev_get_drvdata(dev->parent);
+ unsigned int status, val;
+ int ret;
+
+ (void)attr;
+ ret = regmap_read(pcf85053a->regmap, reg, &status);
+ if (ret)
+ return -EIO;
+
+ val = (status & flag) != 0;
+
+ return sprintf(buf, "%u\n", val);
+}
+
+/* flags that can be read or written to be cleared */
+#define PCF85053A_ATTR_FLAG_RWC(name, reg, flag) \
+ static ssize_t name ## _store( \
+ struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, \
+ size_t count) \
+ { \
+ return attr_flag_clear(dev, attr, buf, count, \
+ REG_ ## reg, REG_ ## reg ## _ ## flag); \
+ } \
+ static ssize_t name ## _show( \
+ struct device *dev, \
+ struct device_attribute *attr, \
+ char *buf) \
+ { \
+ return attr_flag_read(dev, attr, buf, \
+ REG_ ## reg, REG_ ## reg ## _ ## flag); \
+ } \
+ static DEVICE_ATTR_RW(name)
+
+PCF85053A_ATTR_FLAG_RWC(rtc_fail, STATUS, RTCF);
+PCF85053A_ATTR_FLAG_RWC(oscillator_fail, STATUS, OF);
+PCF85053A_ATTR_FLAG_RWC(rtc_clear, STATUS, CIF);
+
+static struct attribute *pcf85053a_attrs_flags[] = {
+ &dev_attr_rtc_fail.attr,
+ &dev_attr_oscillator_fail.attr,
+ &dev_attr_rtc_clear.attr,
+ 0,
+};
+
+static const struct attribute_group pcf85053a_attr_group = {
+ .name = "flags",
+ .attrs = pcf85053a_attrs_flags,
+};
+
+static const struct rtc_class_ops pcf85053a_rtc_ops = {
+ .read_offset = pcf85053a_read_offset,
+ .set_offset = pcf85053a_set_offset,
+ .read_time = pcf85053a_rtc_read_time,
+ .set_time = pcf85053a_rtc_set_time,
+};
+
+static const struct pcf85053a_config pcf85053a_config = {
+ .regmap = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = 0x1d,
+ },
+ .regmap_nvmem = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = 0xff,
+ },
+};
+
+static int pcf85053a_add_nvmem(struct i2c_client *client,
+ struct pcf85053a *pcf85053a)
+{
+ int ret;
+ const struct pcf85053a_config *config = &pcf85053a_config;
+ struct i2c_client *client_nvmem;
+ struct nvmem_config nvmem_cfg = {
+ .name = "pcf85053a_nvmem",
+ .reg_read = pcf85053a_nvmem_read,
+ .reg_write = pcf85053a_nvmem_write,
+ .type = NVMEM_TYPE_BATTERY_BACKED,
+ .size = 128,
+ };
+
+ client_nvmem = devm_i2c_new_dummy_device(&client->dev, client->adapter,
+ ADDR_NVMEM);
+ if (IS_ERR(client_nvmem)) {
+ dev_warn(&client->dev, "failed to create nvmem i2c device\n");
+ return -ENODEV;
+ }
+
+ pcf85053a->regmap_nvmem = devm_regmap_init_i2c(client_nvmem,
+ &config->regmap_nvmem);
+ if (IS_ERR(pcf85053a->regmap_nvmem)) {
+ dev_warn(&client->dev, "failed to init nvmem regmap\n");
+ return -EIO;
+ }
+
+ nvmem_cfg.priv = pcf85053a;
+ ret = devm_rtc_nvmem_register(pcf85053a->rtc, &nvmem_cfg);
+
+ return ret;
+}
+
+static void pcf85053a_set_load_capacitance(struct device *dev, u8 *reg_ctrl)
+{
+ int ret;
+ u32 val;
+ u8 regval;
+
+ ret = of_property_read_u32(dev->of_node, "quartz-load-femtofarads",
+ &val);
+ if (ret) {
+ dev_warn(dev, "failed to read quartz-load-femtofarads property,"
+ " assuming 12500");
+ val = 12500;
+ }
+
+ switch (val) {
+ case 7000:
+ regval = 0;
+ break;
+ case 6000:
+ regval = 1;
+ break;
+ default:
+ dev_warn(dev, "invalid quartz-load-femtofarads value: %u,"
+ " assuming 12500", val);
+ fallthrough;
+ case 12500:
+ regval = 2;
+ break;
+ }
+
+ *reg_ctrl |= regval;
+}
+
+static void pcf85053a_set_drive_control(struct device *dev, u8 *reg_ctrl)
+{
+ int ret;
+ const char *val;
+ u8 regval;
+
+ ret = of_property_read_string(dev->of_node, "nxp,quartz-drive-control",
+ &val);
+ if (ret) {
+ dev_warn(dev, "failed to read nxp,quartz-drive-control property,"
+ " assuming 'normal' drive");
+ val = "normal";
+ }
+
+ if (!strcmp(val, "normal")) {
+ regval = 0;
+ } else if (!strcmp(val, "low")) {
+ regval = 1;
+ } else if (!strcmp(val, "high")) {
+ regval = 2;
+ } else {
+ dev_warn(dev, "invalid nxp,quartz-drive-control value: %s,"
+ " assuming 'normal' drive", val);
+ regval = 0;
+ }
+
+ *reg_ctrl |= (regval << 2);
+}
+
+static void pcf85053a_set_low_jitter(struct device *dev, u8 *reg_ctrl)
+{
+ bool val;
+ u8 regval;
+
+ val = of_property_read_bool(dev->of_node, "nxp,low-jitter-mode");
+
+ regval = val ? 1 : 0;
+ *reg_ctrl |= (regval << 4);
+}
+
+static void pcf85053a_set_clk_inverted(struct device *dev, u8 *reg_ctrl)
+{
+ bool val;
+ u8 regval;
+
+ val = of_property_read_bool(dev->of_node, "nxp,clk-inverted");
+
+ regval = val ? 1 : 0;
+ *reg_ctrl |= (regval << 7);
+}
+
+static int pcf85053a_probe(struct i2c_client *client)
+{
+ int ret;
+ struct pcf85053a *pcf85053a;
+ const struct pcf85053a_config *config = &pcf85053a_config;
+ u8 reg_ctrl;
+
+ pcf85053a = devm_kzalloc(&client->dev, sizeof(*pcf85053a), GFP_KERNEL);
+ if (!pcf85053a) {
+ dev_err(&client->dev, "failed to allocate device: no memory");
+ return -ENOMEM;
+ }
+
+ pcf85053a->regmap = devm_regmap_init_i2c(client, &config->regmap);
+ if (IS_ERR(pcf85053a->regmap)) {
+ dev_err(&client->dev, "failed to allocate regmap: %ld\n",
+ PTR_ERR(pcf85053a->regmap));
+ return PTR_ERR(pcf85053a->regmap);
+ }
+
+ i2c_set_clientdata(client, pcf85053a);
+
+ pcf85053a->rtc = devm_rtc_allocate_device(&client->dev);
+ if (IS_ERR(pcf85053a->rtc)) {
+ dev_err(&client->dev, "failed to allocate rtc: %ld\n",
+ PTR_ERR(pcf85053a->rtc));
+ return PTR_ERR(pcf85053a->rtc);
+ }
+
+ pcf85053a->rtc->ops = &pcf85053a_rtc_ops;
+ pcf85053a->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
+ pcf85053a->rtc->range_max = RTC_TIMESTAMP_END_2099;
+
+ reg_ctrl = REG_CTRL_DM | REG_CTRL_HF | REG_CTRL_CIE;
+ pcf85053a_set_load_capacitance(&client->dev, ®_ctrl);
+ pcf85053a_set_drive_control(&client->dev, ®_ctrl);
+ pcf85053a_set_low_jitter(&client->dev, ®_ctrl);
+ pcf85053a_set_clk_inverted(&client->dev, ®_ctrl);
+
+ ret = regmap_write(pcf85053a->regmap, REG_CTRL, reg_ctrl);
+ if (ret) {
+ dev_err(&client->dev, "failed to configure rtc: %d\n", ret);
+ return ret;
+ }
+
+ ret = rtc_add_group(pcf85053a->rtc, &pcf85053a_attr_group);
+ if (ret) {
+ dev_err(&client->dev, "failed to add sysfs entry: %d\n", ret);
+ return ret;
+ }
+
+ ret = devm_rtc_register_device(pcf85053a->rtc);
+ if (ret) {
+ dev_err(&client->dev, "failed to register rtc: %d\n", ret);
+ return ret;
+ }
+
+ ret = pcf85053a_add_nvmem(client, pcf85053a);
+ if (ret) {
+ dev_err(&client->dev, "failed to register nvmem: %d\n", ret);
+ return ret;
+ }
+
+ ret = pcf85053a_hwmon_register(&client->dev, client->name);
+ if (ret)
+ dev_err(&client->dev, "failed to register hwmon: %d\n", ret);
+
+ return ret;
+}
+
+static const __maybe_unused struct of_device_id dev_ids[] = {
+ { .compatible = "nxp,pcf85053a", .data = &pcf85053a_config },
+ { },
+};
+MODULE_DEVICE_TABLE(of, dev_ids);
+
+static struct i2c_driver pcf85053a_driver = {
+ .driver = {
+ .name = "pcf85053a",
+ .of_match_table = of_match_ptr(dev_ids),
+ },
+ .probe_new = &pcf85053a_probe,
+};
+
+module_i2c_driver(pcf85053a_driver);
+
+MODULE_AUTHOR("Carlos Menin <[email protected]>");
+MODULE_DESCRIPTION("PCF85053A I2C RTC driver");
+MODULE_LICENSE("GPL");
--
2.34.1
Add YAML bindings for NXP's PCF85053A RTC chip.
Signed-off-by: Carlos Menin <[email protected]>
Reviewed-by: Sergio Prado <[email protected]>
---
.../bindings/rtc/nxp,pcf85053a.yaml | 71 +++++++++++++++++++
1 file changed, 71 insertions(+)
create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf85053a.yaml
diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85053a.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf85053a.yaml
new file mode 100644
index 000000000000..a1fc29dd30f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85053a.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/nxp,pcf85053a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP PCF85053A Real Time Clock
+
+maintainers:
+ - Carlos Menin <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - nxp,pcf85053a
+
+ reg:
+ maxItems: 1
+
+ quartz-load-femtofarads:
+ description:
+ The capacitive load of the crystal, expressed in femto Farad (fF).
+ enum: [6000, 7000, 12500]
+ default: 12500
+
+ nxp,quartz-drive-control:
+ description:
+ The oscillator is designed to be used with quartz with a series resistance
+ up to 100 kOhms. This covers the typical range of 32.768 kHz quartz
+ crystals. A low drive mode is available for low series resistance quartz
+ (up to 60 kOhms). This reduces the current consumption. For very high
+ series resistance quartz (up to 500 kOhms), there is a high drive mode.
+ Current consumption increases substantially in this mode.
+ enum: [low, normal, high]
+ default: normal
+
+ nxp,low-jitter-mode:
+ description:
+ If set to true, will decrease the CLK output jitter, at the cost of
+ increasing the current consumption.
+ type: boolean
+ default: false
+
+ nxp,clk-inverted:
+ description:
+ Invert clock output. Normally, the low jitter mode reduces jitter on the
+ rising edge of the output clock. If this property is set to true, then the
+ low jitter mode will affect the falling edge of the output clock.
+ type: boolean
+ default: false
+
+required:
+ - compatible
+ - reg
+
+allOf:
+ - $ref: rtc.yaml#
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ rtc@6f {
+ compatible = "nxp,pcf85053a";
+ reg = <0x6f>;
+ };
+ };
--
2.34.1
On 11/3/23 05:51, Carlos Menin wrote:
> Add support for NXP's PCF85053A RTC chip.
>
> Signed-off-by: Carlos Menin <[email protected]>
> Reviewed-by: Sergio Prado <[email protected]>
> ---
[ ... ]
> +static int pcf85053a_bvl_to_mv(unsigned int bvl)
> +{
> + long mv_table[] = {
> + 1700,
> + 1900,
> + 2100,
> + 2300,
> + 2500,
> + 2700,
> + 2900,
> + 3100,
How are those numbers determined ? The datasheet gives voltage ranges.
I'd have assumed that the center of those ranges is chosen, but for the
most part it is the maximum, except for 2900 which is a bit above center
and 3100 for "> 3.0V". Not that I care too much, but it seems to me that
using the center voltage for each range would be more consistent.
> +static int pcf85053a_hwmon_register(struct device *dev, const char *name)
> +{
> + struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
> + struct device *hwmon_dev;
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, name, pcf85053a,
> + &pcf85053a_hwmon_chip_info,
> + 0);
This won't compile if CONFIG_HWMON=n or if CONFIG_RTC_DRV_PCF85053A=y and
CONFIG_HWMON=m.
Guenter
Hello Carlos,
On 03/11/2023 09:51:05-0300, Carlos Menin wrote:
> +struct pcf85053a {
> + struct rtc_device *rtc;
> + struct regmap *regmap;
> + struct regmap *regmap_nvmem;
> +};
> +
> +struct pcf85053a_config {
> + struct regmap_config regmap;
> + struct regmap_config regmap_nvmem;
> +};
> +
> +static int pcf85053a_read_offset(struct device *dev, long *offset)
> +{
> + struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
> + long val;
> + u32 reg_offset, reg_oscillator;
> + int ret;
> +
> + ret = regmap_read(pcf85053a->regmap, REG_OFFSET, ®_offset);
> + if (ret)
> + return -EIO;
Why do you change the error returned by regmap?
> +
> + ret = regmap_read(pcf85053a->regmap, REG_OSCILLATOR, ®_oscillator);
> + if (ret)
> + return -EIO;
> +
> + val = sign_extend32(reg_offset, 7);
> +
> + if (reg_oscillator & REG_OSC_OFFM)
> + *offset = val * OFFSET_STEP1;
> + else
> + *offset = val * OFFSET_STEP0;
> +
> + return 0;
> +}
> +
> +static int pcf85053a_set_offset(struct device *dev, long offset)
> +{
> + struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
> + s8 mode0, mode1, reg_offset;
> + unsigned int ret, error0, error1;
> +
> + if (offset > OFFSET_STEP0 * 127)
> + return -ERANGE;
> + if (offset < OFFSET_STEP0 * -128)
> + return -ERANGE;
> +
> + ret = regmap_set_bits(pcf85053a->regmap, REG_ACCESS, REG_ACCESS_XCLK);
> + if (ret)
> + return -EIO;
> +
> + mode0 = DIV_ROUND_CLOSEST(offset, OFFSET_STEP0);
> + mode1 = DIV_ROUND_CLOSEST(offset, OFFSET_STEP1);
> +
> + error0 = abs(offset - (mode0 * OFFSET_STEP0));
> + error1 = abs(offset - (mode1 * OFFSET_STEP1));
> + if (error0 < error1) {
> + reg_offset = mode0;
> + ret = regmap_clear_bits(pcf85053a->regmap, REG_OSCILLATOR,
> + REG_OSC_OFFM);
> + } else {
> + reg_offset = mode1;
> + ret = regmap_set_bits(pcf85053a->regmap, REG_OSCILLATOR,
> + REG_OSC_OFFM);
> + }
> + if (ret)
> + return -EIO;
> +
> + ret = regmap_write(pcf85053a->regmap, REG_OFFSET, reg_offset);
> +
> + return ret;
> +}
> +
> +static int pcf85053a_rtc_check_reliability(struct device *dev, u8 status_reg)
> +{
> + int ret = 0;
> +
> + if (status_reg & REG_STATUS_CIF) {
> + dev_warn(dev, "tamper detected,"
> + " date/time is not reliable\n");
You should not split strings. Also, I don't think most of the messages
are actually useful as the end user doesn't have any specific action
after seeing it. You should probably drop them.
I don't think CIF means the time is not correct anymore.
> + ret = -EINVAL;
> + }
> +
> + if (status_reg & REG_STATUS_OF) {
> + dev_warn(dev, "oscillator fail detected,"
> + " date/time is not reliable.\n");
> + ret = -EINVAL;
> + }
> +
> + if (status_reg & REG_STATUS_RTCF) {
> + dev_warn(dev, "power loss detected,"
> + " date/time is not reliable.\n");
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +
> +static int pcf85053a_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
> + u8 buf[REG_STATUS + 1];
> + int ret, len = sizeof(buf);
> +
> + ret = regmap_bulk_read(pcf85053a->regmap, REG_SECS, buf, len);
> + if (ret) {
> + dev_err(dev, "%s: error %d\n", __func__, ret);
> + return ret;
> + }
> +
> + ret = pcf85053a_rtc_check_reliability(dev, buf[REG_STATUS]);
> + if (ret)
> + return ret;
> +
> + tm->tm_year = buf[REG_YEARS];
> + /* adjust for 1900 base of rtc_time */
> + tm->tm_year += 100;
> +
> + tm->tm_wday = (buf[REG_WEEKDAYS] - 1) & 7; /* 1 - 7 */
> + tm->tm_sec = buf[REG_SECS];
> + tm->tm_min = buf[REG_MINUTES];
> + tm->tm_hour = buf[REG_HOURS];
> + tm->tm_mday = buf[REG_DAYS];
> + tm->tm_mon = buf[REG_MONTHS] - 1; /* 1 - 12 */
Those comments are not useful.
> +
> + return 0;
> +}
> +
> +static ssize_t attr_flag_clear(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count,
> + u8 reg, u8 flag)
> +{
> + struct pcf85053a *pcf85053a = dev_get_drvdata(dev->parent);
> + int ret;
> +
> + (void)attr;
> + (void)buf;
> + (void)count;
> +
> + ret = regmap_clear_bits(pcf85053a->regmap, reg, flag);
> + if (ret)
> + return -EIO;
> +
> + return count;
> +}
> +
> +static ssize_t attr_flag_read(struct device *dev,
> + struct device_attribute *attr,
> + char *buf,
> + u8 reg, u8 flag)
> +{
> + struct pcf85053a *pcf85053a = dev_get_drvdata(dev->parent);
> + unsigned int status, val;
> + int ret;
> +
> + (void)attr;
> + ret = regmap_read(pcf85053a->regmap, reg, &status);
> + if (ret)
> + return -EIO;
> +
> + val = (status & flag) != 0;
> +
> + return sprintf(buf, "%u\n", val);
> +}
> +
> +/* flags that can be read or written to be cleared */
> +#define PCF85053A_ATTR_FLAG_RWC(name, reg, flag) \
> + static ssize_t name ## _store( \
> + struct device *dev, \
> + struct device_attribute *attr, \
> + const char *buf, \
> + size_t count) \
> + { \
> + return attr_flag_clear(dev, attr, buf, count, \
> + REG_ ## reg, REG_ ## reg ## _ ## flag); \
> + } \
> + static ssize_t name ## _show( \
> + struct device *dev, \
> + struct device_attribute *attr, \
> + char *buf) \
> + { \
> + return attr_flag_read(dev, attr, buf, \
> + REG_ ## reg, REG_ ## reg ## _ ## flag); \
> + } \
> + static DEVICE_ATTR_RW(name)
> +
> +PCF85053A_ATTR_FLAG_RWC(rtc_fail, STATUS, RTCF);
> +PCF85053A_ATTR_FLAG_RWC(oscillator_fail, STATUS, OF);
> +PCF85053A_ATTR_FLAG_RWC(rtc_clear, STATUS, CIF);
> +
> +static struct attribute *pcf85053a_attrs_flags[] = {
> + &dev_attr_rtc_fail.attr,
> + &dev_attr_oscillator_fail.attr,
> + &dev_attr_rtc_clear.attr,
> + 0,
> +};
Don't add undocumented sysfs files. Also, You must not allow userspace
to clear those flags without setting the time properly.
> +static void pcf85053a_set_drive_control(struct device *dev, u8 *reg_ctrl)
> +{
> + int ret;
> + const char *val;
> + u8 regval;
> +
> + ret = of_property_read_string(dev->of_node, "nxp,quartz-drive-control",
> + &val);
This property should rather be "nxp,quartz-drive".
> + if (ret) {
> + dev_warn(dev, "failed to read nxp,quartz-drive-control property,"
> + " assuming 'normal' drive");
> + val = "normal";
> + }
> +
> + if (!strcmp(val, "normal")) {
> + regval = 0;
> + } else if (!strcmp(val, "low")) {
> + regval = 1;
> + } else if (!strcmp(val, "high")) {
> + regval = 2;
> + } else {
> + dev_warn(dev, "invalid nxp,quartz-drive-control value: %s,"
> + " assuming 'normal' drive", val);
> + regval = 0;
> + }
> +
> + *reg_ctrl |= (regval << 2);
2 needs a define, what about using FIELD_PREP?
> +}
> +
> +static void pcf85053a_set_low_jitter(struct device *dev, u8 *reg_ctrl)
> +{
> + bool val;
> + u8 regval;
> +
> + val = of_property_read_bool(dev->of_node, "nxp,low-jitter-mode");
Bool properties don't work well with RTC because with this, there is now
way to enable the normal mode.
> +
> + regval = val ? 1 : 0;
> + *reg_ctrl |= (regval << 4);
4 also needs a define
> +}
> +
> +static void pcf85053a_set_clk_inverted(struct device *dev, u8 *reg_ctrl)
> +{
> + bool val;
> + u8 regval;
> +
> + val = of_property_read_bool(dev->of_node, "nxp,clk-inverted");
> +
> + regval = val ? 1 : 0;
> + *reg_ctrl |= (regval << 7);
Ditto
> +}
> +
> +static int pcf85053a_probe(struct i2c_client *client)
> +{
> + int ret;
> + struct pcf85053a *pcf85053a;
> + const struct pcf85053a_config *config = &pcf85053a_config;
> + u8 reg_ctrl;
> +
> + pcf85053a = devm_kzalloc(&client->dev, sizeof(*pcf85053a), GFP_KERNEL);
> + if (!pcf85053a) {
> + dev_err(&client->dev, "failed to allocate device: no memory");
> + return -ENOMEM;
> + }
> +
> + pcf85053a->regmap = devm_regmap_init_i2c(client, &config->regmap);
> + if (IS_ERR(pcf85053a->regmap)) {
> + dev_err(&client->dev, "failed to allocate regmap: %ld\n",
> + PTR_ERR(pcf85053a->regmap));
> + return PTR_ERR(pcf85053a->regmap);
> + }
> +
> + i2c_set_clientdata(client, pcf85053a);
> +
> + pcf85053a->rtc = devm_rtc_allocate_device(&client->dev);
> + if (IS_ERR(pcf85053a->rtc)) {
> + dev_err(&client->dev, "failed to allocate rtc: %ld\n",
> + PTR_ERR(pcf85053a->rtc));
> + return PTR_ERR(pcf85053a->rtc);
> + }
> +
> + pcf85053a->rtc->ops = &pcf85053a_rtc_ops;
> + pcf85053a->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> + pcf85053a->rtc->range_max = RTC_TIMESTAMP_END_2099;
> +
> + reg_ctrl = REG_CTRL_DM | REG_CTRL_HF | REG_CTRL_CIE;
CIE enables an interrupt but you never use interrupts.
> + pcf85053a_set_load_capacitance(&client->dev, ®_ctrl);
> + pcf85053a_set_drive_control(&client->dev, ®_ctrl);
> + pcf85053a_set_low_jitter(&client->dev, ®_ctrl);
> + pcf85053a_set_clk_inverted(&client->dev, ®_ctrl);
> +
> + ret = regmap_write(pcf85053a->regmap, REG_CTRL, reg_ctrl);
> + if (ret) {
> + dev_err(&client->dev, "failed to configure rtc: %d\n", ret);
> + return ret;
> + }
> +
> + ret = rtc_add_group(pcf85053a->rtc, &pcf85053a_attr_group);
> + if (ret) {
> + dev_err(&client->dev, "failed to add sysfs entry: %d\n", ret);
> + return ret;
> + }
> +
> + ret = devm_rtc_register_device(pcf85053a->rtc);
> + if (ret) {
> + dev_err(&client->dev, "failed to register rtc: %d\n", ret);
> + return ret;
> + }
> +
> + ret = pcf85053a_add_nvmem(client, pcf85053a);
> + if (ret) {
> + dev_err(&client->dev, "failed to register nvmem: %d\n", ret);
> + return ret;
probe must not fail after devm_rtc_register_device
> + }
> +
> + ret = pcf85053a_hwmon_register(&client->dev, client->name);
> + if (ret)
> + dev_err(&client->dev, "failed to register hwmon: %d\n", ret);
> +
> + return ret;
> +}
> +
> +static const __maybe_unused struct of_device_id dev_ids[] = {
> + { .compatible = "nxp,pcf85053a", .data = &pcf85053a_config },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, dev_ids);
> +
> +static struct i2c_driver pcf85053a_driver = {
> + .driver = {
> + .name = "pcf85053a",
> + .of_match_table = of_match_ptr(dev_ids),
> + },
> + .probe_new = &pcf85053a_probe,
> +};
> +
> +module_i2c_driver(pcf85053a_driver);
> +
> +MODULE_AUTHOR("Carlos Menin <[email protected]>");
> +MODULE_DESCRIPTION("PCF85053A I2C RTC driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On 03/11/2023 13:51, Carlos Menin wrote:
> Add YAML bindings for NXP's PCF85053A RTC chip.
>
> Signed-off-by: Carlos Menin <[email protected]>
> Reviewed-by: Sergio Prado <[email protected]>
Thank you for your patch. There is something to discuss/improve.
> + quartz-load-femtofarads:
> + description:
> + The capacitive load of the crystal, expressed in femto Farad (fF).
> + enum: [6000, 7000, 12500]
> + default: 12500
> +
> + nxp,quartz-drive-control:
> + description:
> + The oscillator is designed to be used with quartz with a series resistance
> + up to 100 kOhms. This covers the typical range of 32.768 kHz quartz
> + crystals. A low drive mode is available for low series resistance quartz
> + (up to 60 kOhms). This reduces the current consumption. For very high
> + series resistance quartz (up to 500 kOhms), there is a high drive mode.
> + Current consumption increases substantially in this mode.
> + enum: [low, normal, high]
> + default: normal
> +
> + nxp,low-jitter-mode:
> + description:
> + If set to true, will decrease the CLK output jitter, at the cost of
> + increasing the current consumption.
> + type: boolean
> + default: false
Drop this line.
> +
> + nxp,clk-inverted:
> + description:
> + Invert clock output. Normally, the low jitter mode reduces jitter on the
> + rising edge of the output clock. If this property is set to true, then the
> + low jitter mode will affect the falling edge of the output clock.
> + type: boolean
> + default: false
> +
> +required:
> + - compatible
> + - reg
> +
> +allOf:
> + - $ref: rtc.yaml#
> +
> +additionalProperties: false
This should be rather unevaluatedProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + rtc@6f {
> + compatible = "nxp,pcf85053a";
> + reg = <0x6f>;
Please extend the example to include all or almost all properties (which
make sense).
Best regards,
Krzysztof
On Fri, Nov 03, 2023 at 07:09:27AM -0700, Guenter Roeck wrote:
> On 11/3/23 05:51, Carlos Menin wrote:
> > Add support for NXP's PCF85053A RTC chip.
> >
> > Signed-off-by: Carlos Menin <[email protected]>
> > Reviewed-by: Sergio Prado <[email protected]>
> > ---
>
> [ ... ]
>
> > +static int pcf85053a_bvl_to_mv(unsigned int bvl)
> > +{
> > + long mv_table[] = {
> > + 1700,
> > + 1900,
> > + 2100,
> > + 2300,
> > + 2500,
> > + 2700,
> > + 2900,
> > + 3100,
>
> How are those numbers determined ? The datasheet gives voltage ranges.
> I'd have assumed that the center of those ranges is chosen, but for the
> most part it is the maximum, except for 2900 which is a bit above center
> and 3100 for "> 3.0V". Not that I care too much, but it seems to me that
> using the center voltage for each range would be more consistent.
>
I just used numbers that would result in the same step between levels
(200 mV) at the same time they would fit in the ranges, but I agree
that using the center of the ranges makes sense. In this case which
values would you suggest for <= 1.7 and > 3.0 ?
> > +static int pcf85053a_hwmon_register(struct device *dev, const char *name)
> > +{
> > + struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
> > + struct device *hwmon_dev;
> > +
> > + hwmon_dev = devm_hwmon_device_register_with_info(dev, name, pcf85053a,
> > + &pcf85053a_hwmon_chip_info,
> > + 0);
>
> This won't compile if CONFIG_HWMON=n or if CONFIG_RTC_DRV_PCF85053A=y and
> CONFIG_HWMON=m.
>
> Guenter
>
I will add dependencies in the Kconfig file.
Thanks for reviewing,
Carlos
On Fri, Nov 03, 2023 at 03:28:22PM +0100, Alexandre Belloni wrote:
> Hello Carlos,
>
> On 03/11/2023 09:51:05-0300, Carlos Menin wrote:
> > +struct pcf85053a {
> > + struct rtc_device *rtc;
> > + struct regmap *regmap;
> > + struct regmap *regmap_nvmem;
> > +};
> > +
> > +struct pcf85053a_config {
> > + struct regmap_config regmap;
> > + struct regmap_config regmap_nvmem;
> > +};
> > +
> > +static int pcf85053a_read_offset(struct device *dev, long *offset)
> > +{
> > + struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
> > + long val;
> > + u32 reg_offset, reg_oscillator;
> > + int ret;
> > +
> > + ret = regmap_read(pcf85053a->regmap, REG_OFFSET, ®_offset);
> > + if (ret)
> > + return -EIO;
>
> Why do you change the error returned by regmap?
>
I am not sure why I did that, should just return ret. Thanks for noting.
> > +
> > + ret = regmap_read(pcf85053a->regmap, REG_OSCILLATOR, ®_oscillator);
> > + if (ret)
> > + return -EIO;
> > +
> > + val = sign_extend32(reg_offset, 7);
> > +
> > + if (reg_oscillator & REG_OSC_OFFM)
> > + *offset = val * OFFSET_STEP1;
> > + else
> > + *offset = val * OFFSET_STEP0;
> > +
> > + return 0;
> > +}
> > +
> > +static int pcf85053a_set_offset(struct device *dev, long offset)
> > +{
> > + struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
> > + s8 mode0, mode1, reg_offset;
> > + unsigned int ret, error0, error1;
> > +
> > + if (offset > OFFSET_STEP0 * 127)
> > + return -ERANGE;
> > + if (offset < OFFSET_STEP0 * -128)
> > + return -ERANGE;
> > +
> > + ret = regmap_set_bits(pcf85053a->regmap, REG_ACCESS, REG_ACCESS_XCLK);
> > + if (ret)
> > + return -EIO;
> > +
> > + mode0 = DIV_ROUND_CLOSEST(offset, OFFSET_STEP0);
> > + mode1 = DIV_ROUND_CLOSEST(offset, OFFSET_STEP1);
> > +
> > + error0 = abs(offset - (mode0 * OFFSET_STEP0));
> > + error1 = abs(offset - (mode1 * OFFSET_STEP1));
> > + if (error0 < error1) {
> > + reg_offset = mode0;
> > + ret = regmap_clear_bits(pcf85053a->regmap, REG_OSCILLATOR,
> > + REG_OSC_OFFM);
> > + } else {
> > + reg_offset = mode1;
> > + ret = regmap_set_bits(pcf85053a->regmap, REG_OSCILLATOR,
> > + REG_OSC_OFFM);
> > + }
> > + if (ret)
> > + return -EIO;
> > +
> > + ret = regmap_write(pcf85053a->regmap, REG_OFFSET, reg_offset);
> > +
> > + return ret;
> > +}
> > +
> > +static int pcf85053a_rtc_check_reliability(struct device *dev, u8 status_reg)
> > +{
> > + int ret = 0;
> > +
> > + if (status_reg & REG_STATUS_CIF) {
> > + dev_warn(dev, "tamper detected,"
> > + " date/time is not reliable\n");
> You should not split strings. Also, I don't think most of the messages
> are actually useful as the end user doesn't have any specific action
> after seeing it. You should probably drop them.
>
About the usefullness of the message, do this apply to this CIF related
message or also to the other two flags?
> I don't think CIF means the time is not correct anymore.
>
You are right, it only clears the SRAM. I will remove this flag from
this check.
> > + ret = -EINVAL;
> > + }
> > +
> > + if (status_reg & REG_STATUS_OF) {
> > + dev_warn(dev, "oscillator fail detected,"
> > + " date/time is not reliable.\n");
> > + ret = -EINVAL;
> > + }
> > +
> > + if (status_reg & REG_STATUS_RTCF) {
> > + dev_warn(dev, "power loss detected,"
> > + " date/time is not reliable.\n");
> > + ret = -EINVAL;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int pcf85053a_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > + struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
> > + u8 buf[REG_STATUS + 1];
> > + int ret, len = sizeof(buf);
> > +
> > + ret = regmap_bulk_read(pcf85053a->regmap, REG_SECS, buf, len);
> > + if (ret) {
> > + dev_err(dev, "%s: error %d\n", __func__, ret);
> > + return ret;
> > + }
> > +
> > + ret = pcf85053a_rtc_check_reliability(dev, buf[REG_STATUS]);
> > + if (ret)
> > + return ret;
> > +
> > + tm->tm_year = buf[REG_YEARS];
> > + /* adjust for 1900 base of rtc_time */
> > + tm->tm_year += 100;
> > +
> > + tm->tm_wday = (buf[REG_WEEKDAYS] - 1) & 7; /* 1 - 7 */
> > + tm->tm_sec = buf[REG_SECS];
> > + tm->tm_min = buf[REG_MINUTES];
> > + tm->tm_hour = buf[REG_HOURS];
> > + tm->tm_mday = buf[REG_DAYS];
> > + tm->tm_mon = buf[REG_MONTHS] - 1; /* 1 - 12 */
>
> Those comments are not useful.
>
I Will improve them to explain why I am offsetting the register value.
> > +
> > + return 0;
> > +}
> > +
>
> > +static ssize_t attr_flag_clear(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count,
> > + u8 reg, u8 flag)
> > +{
> > + struct pcf85053a *pcf85053a = dev_get_drvdata(dev->parent);
> > + int ret;
> > +
> > + (void)attr;
> > + (void)buf;
> > + (void)count;
> > +
> > + ret = regmap_clear_bits(pcf85053a->regmap, reg, flag);
> > + if (ret)
> > + return -EIO;
> > +
> > + return count;
> > +}
> > +
> > +static ssize_t attr_flag_read(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf,
> > + u8 reg, u8 flag)
> > +{
> > + struct pcf85053a *pcf85053a = dev_get_drvdata(dev->parent);
> > + unsigned int status, val;
> > + int ret;
> > +
> > + (void)attr;
> > + ret = regmap_read(pcf85053a->regmap, reg, &status);
> > + if (ret)
> > + return -EIO;
> > +
> > + val = (status & flag) != 0;
> > +
> > + return sprintf(buf, "%u\n", val);
> > +}
>
>
>
> > +
> > +/* flags that can be read or written to be cleared */
> > +#define PCF85053A_ATTR_FLAG_RWC(name, reg, flag) \
> > + static ssize_t name ## _store( \
> > + struct device *dev, \
> > + struct device_attribute *attr, \
> > + const char *buf, \
> > + size_t count) \
> > + { \
> > + return attr_flag_clear(dev, attr, buf, count, \
> > + REG_ ## reg, REG_ ## reg ## _ ## flag); \
> > + } \
> > + static ssize_t name ## _show( \
> > + struct device *dev, \
> > + struct device_attribute *attr, \
> > + char *buf) \
> > + { \
> > + return attr_flag_read(dev, attr, buf, \
> > + REG_ ## reg, REG_ ## reg ## _ ## flag); \
> > + } \
> > + static DEVICE_ATTR_RW(name)
> > +
> > +PCF85053A_ATTR_FLAG_RWC(rtc_fail, STATUS, RTCF);
> > +PCF85053A_ATTR_FLAG_RWC(oscillator_fail, STATUS, OF);
> > +PCF85053A_ATTR_FLAG_RWC(rtc_clear, STATUS, CIF);
> > +
> > +static struct attribute *pcf85053a_attrs_flags[] = {
> > + &dev_attr_rtc_fail.attr,
> > + &dev_attr_oscillator_fail.attr,
> > + &dev_attr_rtc_clear.attr,
> > + 0,
> > +};
>
> Don't add undocumented sysfs files. Also, You must not allow userspace
> to clear those flags without setting the time properly.
>
Should the flags be cleared only by setting the time, or is there
another acceptable method? What would be the correct way to let
userspace read those flags?
> > +static void pcf85053a_set_drive_control(struct device *dev, u8 *reg_ctrl)
> > +{
> > + int ret;
> > + const char *val;
> > + u8 regval;
> > +
> > + ret = of_property_read_string(dev->of_node, "nxp,quartz-drive-control",
> > + &val);
>
> This property should rather be "nxp,quartz-drive".
>
> > + if (ret) {
> > + dev_warn(dev, "failed to read nxp,quartz-drive-control property,"
> > + " assuming 'normal' drive");
> > + val = "normal";
> > + }
> > +
> > + if (!strcmp(val, "normal")) {
> > + regval = 0;
> > + } else if (!strcmp(val, "low")) {
> > + regval = 1;
> > + } else if (!strcmp(val, "high")) {
> > + regval = 2;
> > + } else {
> > + dev_warn(dev, "invalid nxp,quartz-drive-control value: %s,"
> > + " assuming 'normal' drive", val);
> > + regval = 0;
> > + }
> > +
> > + *reg_ctrl |= (regval << 2);
>
> 2 needs a define, what about using FIELD_PREP?
>
> > +}
> > +
> > +static void pcf85053a_set_low_jitter(struct device *dev, u8 *reg_ctrl)
> > +{
> > + bool val;
> > + u8 regval;
> > +
> > + val = of_property_read_bool(dev->of_node, "nxp,low-jitter-mode");
>
> Bool properties don't work well with RTC because with this, there is now
> way to enable the normal mode.
>
Wouldn't the absence of the property enable normal mode? Or I am missing
something?
> > +
> > + regval = val ? 1 : 0;
> > + *reg_ctrl |= (regval << 4);
> 4 also needs a define
>
> > +}
> > +
> > +static void pcf85053a_set_clk_inverted(struct device *dev, u8 *reg_ctrl)
> > +{
> > + bool val;
> > + u8 regval;
> > +
> > + val = of_property_read_bool(dev->of_node, "nxp,clk-inverted");
> > +
> > + regval = val ? 1 : 0;
> > + *reg_ctrl |= (regval << 7);
>
> Ditto
> > +}
> > +
> > +static int pcf85053a_probe(struct i2c_client *client)
> > +{
> > + int ret;
> > + struct pcf85053a *pcf85053a;
> > + const struct pcf85053a_config *config = &pcf85053a_config;
> > + u8 reg_ctrl;
> > +
> > + pcf85053a = devm_kzalloc(&client->dev, sizeof(*pcf85053a), GFP_KERNEL);
> > + if (!pcf85053a) {
> > + dev_err(&client->dev, "failed to allocate device: no memory");
> > + return -ENOMEM;
> > + }
> > +
> > + pcf85053a->regmap = devm_regmap_init_i2c(client, &config->regmap);
> > + if (IS_ERR(pcf85053a->regmap)) {
> > + dev_err(&client->dev, "failed to allocate regmap: %ld\n",
> > + PTR_ERR(pcf85053a->regmap));
> > + return PTR_ERR(pcf85053a->regmap);
> > + }
> > +
> > + i2c_set_clientdata(client, pcf85053a);
> > +
> > + pcf85053a->rtc = devm_rtc_allocate_device(&client->dev);
> > + if (IS_ERR(pcf85053a->rtc)) {
> > + dev_err(&client->dev, "failed to allocate rtc: %ld\n",
> > + PTR_ERR(pcf85053a->rtc));
> > + return PTR_ERR(pcf85053a->rtc);
> > + }
> > +
> > + pcf85053a->rtc->ops = &pcf85053a_rtc_ops;
> > + pcf85053a->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> > + pcf85053a->rtc->range_max = RTC_TIMESTAMP_END_2099;
> > +
> > + reg_ctrl = REG_CTRL_DM | REG_CTRL_HF | REG_CTRL_CIE;
>
> CIE enables an interrupt but you never use interrupts.
>
> > + pcf85053a_set_load_capacitance(&client->dev, ®_ctrl);
> > + pcf85053a_set_drive_control(&client->dev, ®_ctrl);
> > + pcf85053a_set_low_jitter(&client->dev, ®_ctrl);
> > + pcf85053a_set_clk_inverted(&client->dev, ®_ctrl);
> > +
> > + ret = regmap_write(pcf85053a->regmap, REG_CTRL, reg_ctrl);
> > + if (ret) {
> > + dev_err(&client->dev, "failed to configure rtc: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = rtc_add_group(pcf85053a->rtc, &pcf85053a_attr_group);
> > + if (ret) {
> > + dev_err(&client->dev, "failed to add sysfs entry: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = devm_rtc_register_device(pcf85053a->rtc);
> > + if (ret) {
> > + dev_err(&client->dev, "failed to register rtc: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = pcf85053a_add_nvmem(client, pcf85053a);
> > + if (ret) {
> > + dev_err(&client->dev, "failed to register nvmem: %d\n", ret);
> > + return ret;
>
> probe must not fail after devm_rtc_register_device
>
> > + }
> > +
> > + ret = pcf85053a_hwmon_register(&client->dev, client->name);
> > + if (ret)
> > + dev_err(&client->dev, "failed to register hwmon: %d\n", ret);
> > +
> > + return ret;
> > +}
> > +
> > +static const __maybe_unused struct of_device_id dev_ids[] = {
> > + { .compatible = "nxp,pcf85053a", .data = &pcf85053a_config },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, dev_ids);
> > +
> > +static struct i2c_driver pcf85053a_driver = {
> > + .driver = {
> > + .name = "pcf85053a",
> > + .of_match_table = of_match_ptr(dev_ids),
> > + },
> > + .probe_new = &pcf85053a_probe,
> > +};
> > +
> > +module_i2c_driver(pcf85053a_driver);
> > +
> > +MODULE_AUTHOR("Carlos Menin <[email protected]>");
> > +MODULE_DESCRIPTION("PCF85053A I2C RTC driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Thanks for reviewing! I will implement changes to improve all noted
points in the next version of the patch.
Carlos
On 11/5/23 12:17, Carlos Menin wrote:
> On Fri, Nov 03, 2023 at 07:09:27AM -0700, Guenter Roeck wrote:
>> On 11/3/23 05:51, Carlos Menin wrote:
>>> Add support for NXP's PCF85053A RTC chip.
>>>
>>> Signed-off-by: Carlos Menin <[email protected]>
>>> Reviewed-by: Sergio Prado <[email protected]>
>>> ---
>>
>> [ ... ]
>>
>>> +static int pcf85053a_bvl_to_mv(unsigned int bvl)
>>> +{
>>> + long mv_table[] = {
>>> + 1700,
>>> + 1900,
>>> + 2100,
>>> + 2300,
>>> + 2500,
>>> + 2700,
>>> + 2900,
>>> + 3100,
>>
>> How are those numbers determined ? The datasheet gives voltage ranges.
>> I'd have assumed that the center of those ranges is chosen, but for the
>> most part it is the maximum, except for 2900 which is a bit above center
>> and 3100 for "> 3.0V". Not that I care too much, but it seems to me that
>> using the center voltage for each range would be more consistent.
>>
>
> I just used numbers that would result in the same step between levels
> (200 mV) at the same time they would fit in the ranges, but I agree
Ah, thanks for the explanation. Still, I find it a bit misleading.
> that using the center of the ranges makes sense. In this case which
> values would you suggest for <= 1.7 and > 3.0 ?
>
The datasheet says that the battery voltage must be between 1.55 V and 3.6 V.
With that in mind, and since the next voltages would be 1800 and 2850 if you
use center voltages, I'd probably use 1600 and 3100. You'd then have
1600, 1800, 2000, 2200, 2400, 2600, 2850, 3100
This gets close to using the same step size but also reflects voltages
more accurately.
>>> +static int pcf85053a_hwmon_register(struct device *dev, const char *name)
>>> +{
>>> + struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
>>> + struct device *hwmon_dev;
>>> +
>>> + hwmon_dev = devm_hwmon_device_register_with_info(dev, name, pcf85053a,
>>> + &pcf85053a_hwmon_chip_info,
>>> + 0);
>>
>> This won't compile if CONFIG_HWMON=n or if CONFIG_RTC_DRV_PCF85053A=y and
>> CONFIG_HWMON=m.
>>
>> Guenter
>>
>
> I will add dependencies in the Kconfig file.
>
You'll also need something like IS_REACHABLE() here unless you make the driver
depend on HWMON, which is probably not what you want.
Thanks,
Guenter
On 05/11/2023 17:49:33-0300, Carlos Menin wrote:
> > > +static int pcf85053a_rtc_check_reliability(struct device *dev, u8 status_reg)
> > > +{
> > > + int ret = 0;
> > > +
> > > + if (status_reg & REG_STATUS_CIF) {
> > > + dev_warn(dev, "tamper detected,"
> > > + " date/time is not reliable\n");
> > You should not split strings. Also, I don't think most of the messages
> > are actually useful as the end user doesn't have any specific action
> > after seeing it. You should probably drop them.
> >
>
> About the usefullness of the message, do this apply to this CIF related
> message or also to the other two flags?
This actually applies to all the messages of the driver, they will add
to the size of the kernel then slow the boot time so please have a
careful look at the usefulness of messages. You may consider swtching
them to dev_dbg.
> > > + tm->tm_year = buf[REG_YEARS];
> > > + /* adjust for 1900 base of rtc_time */
> > > + tm->tm_year += 100;
> > > +
> > > + tm->tm_wday = (buf[REG_WEEKDAYS] - 1) & 7; /* 1 - 7 */
> > > + tm->tm_sec = buf[REG_SECS];
> > > + tm->tm_min = buf[REG_MINUTES];
> > > + tm->tm_hour = buf[REG_HOURS];
> > > + tm->tm_mday = buf[REG_DAYS];
> > > + tm->tm_mon = buf[REG_MONTHS] - 1; /* 1 - 12 */
> >
> > Those comments are not useful.
> >
>
> I Will improve them to explain why I am offsetting the register value.
I don't think this is necessary, most of the drivers are doing it so
this is expected.
> > > +static struct attribute *pcf85053a_attrs_flags[] = {
> > > + &dev_attr_rtc_fail.attr,
> > > + &dev_attr_oscillator_fail.attr,
> > > + &dev_attr_rtc_clear.attr,
> > > + 0,
> > > +};
> >
> > Don't add undocumented sysfs files. Also, You must not allow userspace
> > to clear those flags without setting the time properly.
> >
>
> Should the flags be cleared only by setting the time, or is there
> another acceptable method? What would be the correct way to let
> userspace read those flags?
The RTC_VL_READ ioctl will allow reading the flags from userspace. For
the flags that monitor the validity of the time and date, they must only
be cleared when the time and date is known to be good s only when
setting the time.
> > > +}
> > > +
> > > +static void pcf85053a_set_low_jitter(struct device *dev, u8 *reg_ctrl)
> > > +{
> > > + bool val;
> > > + u8 regval;
> > > +
> > > + val = of_property_read_bool(dev->of_node, "nxp,low-jitter-mode");
> >
> > Bool properties don't work well with RTC because with this, there is now
> > way to enable the normal mode.
> >
>
> Wouldn't the absence of the property enable normal mode? Or I am missing
> something?
RTC have a greater lifetime than the linux system so you must have a way
to indicate that you don't want to change the configuration for example
if it has been set from the bootloader or at the factory.
Regards,
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com