Re: [PATCH v2 3/5] rtc-rv8803: add tamper function to sysfs for rv8901

[Manuel Traut <manuel.traut@mt.com>]

On Thu, Jan 16, 2025 at 02:15:30PM +0100, Markus Burri wrote:
> Add sysfs interface to enable the EVINx pins and read the time-stamp
> events from EVINX.
> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>
> ---
>  .../ABI/testing/sysfs-class-rtc-tamper        |  21 +
>  drivers/rtc/rtc-rv8803.c                      | 366 ++++++++++++++++++
>  2 files changed, 387 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-rtc-tamper
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-rtc-tamper b/Documentation/ABI/testing/sysfs-class-rtc-tamper
> new file mode 100644
> index 000000000..f035d0fa5
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-rtc-tamper
> @@ -0,0 +1,21 @@
> +What:		/sys/class/rtc/rtcX/tamper/enable
> +Date:		January 2025
> +KernelVersion:	6.13
> +Contact:	Markus Burri <markus.burri@mt.com>
> +Description:	(WO) Attribute to enable and disable the rtc tamper function.
> +		Write a '1' to enable tamper detection or a '0' to disable.
> +
> +What:		/sys/class/rtc/rtcX/tamper/read
> +Date:		January 2025
> +KernelVersion:	6.13
> +Contact:	Markus Burri <markus.burri@mt.com>
> +Description:	(RO) Attribute to read the stored timestamps form buffer FIFO.

Description:	(RO) Attribute to read tamper events.

> +		The timestamps are returned one by one

        Each read tries to retrieve a single entry from the hardware
        FIFO. Events that are not retrieved stay in the hardware FIFO.

> +		Format is 'seconds.milliseconds' since the epoch followed by the trigger events.
> +		The value of the event is the current pin value.
> +
> +		Example values:
> +		- "1234.567 EVIN1=1" for a trigger from EVIN1 changed from low to high
> +		- "1234.567 EVIN1=0 EVIN2=1 for a simultaneous trigger of EVIN1 changed to low and
> +		  EVIN2 changed to high.
> +
> diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c
> index 50fbae931..764e654c2 100644
> --- a/drivers/rtc/rtc-rv8803.c
> +++ b/drivers/rtc/rtc-rv8803.c
> @@ -10,6 +10,7 @@
>  #include <linux/bcd.h>
>  #include <linux/bitops.h>
>  #include <linux/bitfield.h>
> +#include <linux/delay.h>
>  #include <linux/log2.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
> @@ -58,6 +59,35 @@
>  #define RX8900_FLAG_SWOFF		BIT(2)
>  #define RX8900_FLAG_VDETOFF		BIT(3)
>  
> +#define RX8901_EVIN_EN			0x20
> +#define RX8901_EVIN1_CFG		0x21
> +#define RX8901_EVIN2_CFG		0x23
> +#define RX8901_EVIN3_CFG		0x25
> +#define RX8901_EVENTx_CFG_POL		GENMASK(1, 0)
> +#define RX8901_EVENTx_CFG_PUPD		GENMASK(4, 2)
> +
> +#define RX8901_EVIN1_FLT		0x22
> +#define RX8901_EVIN2_FLT		0x24
> +#define RX8901_EVIN3_FLT		0x26
> +
> +#define RX8901_BUF1_CFG1		0x27
> +
> +#define RX8901_BUF1_STAT		0x28
> +#define RX8901_BUFx_STAT_PTR		GENMASK(5, 0)
> +#define RX8901_WRCMD_CFG		0x41
> +#define RX8901_WRCMD_TRG		0x42
> +
> +#define RX8901_EVNT_INTE		0x43
> +
> +#define RX8901_BUF_INTF			0x46
> +#define RX8901_BUF_INTF_BUF1F		BIT(5)
> +
> +#define RX8901_EVNT_INTF		0x47
> +
> +#define NO_OF_EVIN			3
> +
> +#define EVIN_FILTER_MAX			40
> +
>  enum rv8803_type {
>  	rv_8803,
>  	rx_8803,
> @@ -66,6 +96,50 @@ enum rv8803_type {
>  	rx_8901,
>  };
>  
> +enum evin_pull_resistor {
> +	no = 0b000,
> +	pull_up_500k = 0b001,
> +	pull_up_1M = 0b010,
> +	pull_up_10M = 0b011,
> +	pull_down_500k = 0b100,
> +};
> +
> +enum evin_trigger {
> +	falling_edge = 0b00,
> +	rising_edge = 0b01,
> +	both_edges = 0b10,
> +};
> +
> +struct cfg_val_txt {
> +	char *txt;
> +	u8 val;
> +	bool hide;
> +};
> +
> +static const struct cfg_val_txt trg_status_txt[] = {
> +	{ "EVIN1", BIT(5) },
> +	{ "EVIN2", BIT(6) },
> +	{ "EVIN3", BIT(7) },
> +	{ "CMD", BIT(4) },
> +	{ "VBATL", BIT(3) },
> +	{ "VTMPL", BIT(2) },
> +	{ "VDDL", BIT(1) },
> +	{ "OSCSTP", BIT(0) },
> +	{ NULL }
> +};
> +
> +static const u8 evin_cfg_regs[] = {
> +	RX8901_EVIN1_CFG,
> +	RX8901_EVIN2_CFG,
> +	RX8901_EVIN3_CFG
> +};
> +
> +static const u8 evin_flt_regs[] = {
> +	RX8901_EVIN1_FLT,
> +	RX8901_EVIN2_FLT,
> +	RX8901_EVIN3_FLT
> +};
> +
>  struct rv8803_data {
>  	struct i2c_client *client;
>  	struct rtc_device *rtc;
> @@ -558,6 +632,292 @@ static int rv8803_nvram_read(void *priv, unsigned int offset,
>  	return 0;
>  }
>  
> +static int rv8803_ts_event_write_evin(int evin, struct rv8803_data *rv8803,
> +				      enum evin_pull_resistor pullup_down,
> +				      enum evin_trigger trigger, int filter)
> +{
> +	int ret;
>
> +	u8 reg_mask;
> +	struct i2c_client *client = rv8803->client;
> +
> +	/* according to data-sheet, "1" is not valid for filter */
> +	if (evin >= NO_OF_EVIN || filter == 1 || filter > EVIN_FILTER_MAX)
> +		return -EINVAL;
> +
> +	guard(mutex)(&rv8803->flags_lock);
> +
> +	/* set EVENTx pull-up edge trigger */
> +	ret = rv8803_read_reg(client, evin_cfg_regs[evin]);
> +	if (ret < 0)
> +		return ret;
> +	reg_mask = ret;
> +	reg_mask &= ~(RX8901_EVENTx_CFG_PUPD | RX8901_EVENTx_CFG_POL);
> +	reg_mask |= FIELD_PREP(RX8901_EVENTx_CFG_PUPD, pullup_down);
> +	reg_mask |= FIELD_PREP(RX8901_EVENTx_CFG_POL, trigger);
> +	ret = rv8803_write_reg(client, evin_cfg_regs[evin], reg_mask);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* set EVENTx noise filter */
> +	if (filter != -1) {
> +		ret = rv8803_write_reg(client, evin_flt_regs[evin], filter);

> +		if (ret < 0)
> +			return ret;
This check is not needed.

> +	}
> +
> +	return 0;
    return ret;
> +}
> +
> +static int rv8803_ts_enable_events(struct rv8803_data *rv8803, u8 enable_mask)
> +{
> +	int ret;
> +	u8 reg_mask;
> +	struct i2c_client *client = rv8803->client;
> +
> +	guard(mutex)(&rv8803->flags_lock);
> +
> +	/* event detection interrupt */
> +	ret = rv8803_read_reg(client, RV8803_CTRL);
> +	if (ret < 0)
> +		return ret;
> +
> +	reg_mask = ret & ~RV8803_CTRL_EIE;
> +	if (enable_mask)
> +		reg_mask |= RV8803_CTRL_EIE;
> +
> +	ret = rv8803_write_reg(client, RV8803_CTRL, reg_mask);
> +	if (ret)
> +		return ret;
> +
> +	/* events for configuration */
> +	ret = rv8803_write_reg(client, RX8901_EVIN_EN, enable_mask);


> +	if (ret < 0)
> +		return ret;

Not needed

> +	return 0;

    return ret;

> +}
> +
> +static ssize_t enable_store(struct device *dev, struct device_attribute *attr, const char *buf,
> +			    size_t count)
> +{
> +	int ret;

> +	int i;
can be declared in the scope of the for loop

> +	unsigned long tmo;
> +	u8 reg;
> +	u8 reg_mask;
> +	struct rv8803_data *rv8803 = dev_get_drvdata(dev->parent);
> +	struct i2c_client *client = rv8803->client;
> +
> +	/* EVINxCPEN | EVINxEN */;
> +	const u8 reg_mask_evin_en = GENMASK(5, 3) | GENMASK(2, 0);
> +
> +	bool enable = (strstr(buf, "1") == buf) ? true : false;
> +
> +	/* check if event detection status match requested mode */
> +	ret = rv8803_read_reg(client, RX8901_EVIN_EN);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* requested mode match current state -> nothing to do */
> +	if (ret == (enable ? reg_mask_evin_en : 0))
> +		return count;
> +
> +	dev_info(&client->dev, "%s time-stamp event detection\n",
> +		 (enable) ? "configure" : "disable");

dev_dbg

> +	/*
> +	 * 1. disable event detection interrupt
> +	 * 2. disable events for configuration
> +	 */
> +	ret = rv8803_ts_enable_events(rv8803, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* for disable no configuration is needed */
> +	if (!enable)
> +		return count;
> +
> +	/* 3. set EVENTx pull-up edge trigger and noise filter */
> +	for (i = 0; i < NO_OF_EVIN; ++i) {

	for (int i = 0; i < NO_OF_EVIN; ++i) {

> +		ret = rv8803_ts_event_write_evin(i, rv8803, pull_up_1M, falling_edge, 0);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	scoped_guard(mutex, &rv8803->flags_lock) {
> +		/* 4. enable EVENTx interrupt */
> +		ret = rv8803_read_reg(client, RX8901_EVNT_INTE);
> +		if (ret < 0)
> +			return ret;
> +		reg_mask = BIT(5) | BIT(6) | BIT(7); /* EVINxIEN 1-3 */
> +		reg = (ret & ~reg_mask) | reg_mask;
> +		ret = rv8803_write_reg(client, RX8901_EVNT_INTE, reg);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/*
> +	 * 5. set BUF1 inhibit and interrupt every 1 event
> +	 *    NOTE: BUF2-3 are not used in FIFO-mode
> +	 */
> +	ret = rv8803_write_reg(client, RX8901_BUF1_CFG1, 0x01);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* 6. clean and init for BUFx and event counter 1-3 and trigger cmd */
> +	reg = BIT(7) | GENMASK(6, 4);
> +	reg |= BIT(0); /* CMDTRGEN */
> +	ret = rv8803_write_reg(client, RX8901_WRCMD_CFG, reg);
> +	if (ret < 0)
> +		return ret;
> +	ret = rv8803_write_reg(client, RX8901_WRCMD_TRG, 0xFF);
> +	if (ret < 0)
> +		return ret;
> +	tmo = jiffies + msecs_to_jiffies(100); /* timeout 100ms */
> +	do {
> +		usleep_range(10, 2000);
> +		ret = rv8803_read_reg(client, RX8901_WRCMD_TRG);
> +		if (ret < 0)
> +			return ret;
> +		if (time_after(jiffies, tmo))
> +			return -EBUSY;
> +	} while (ret);
> +
> +	/*
> +	 * 7. enable event detection interrupt
> +	 * 8. / 10. enable events for configuration in FIFO mode
> +	 */
> +	ret = rv8803_ts_enable_events(rv8803, reg_mask_evin_en);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t read_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	int ret;
> +	int ev_idx;
> +	int num_events;
> +	unsigned long long time_s;
> +	int time_ms;
> +	int offset = 0;
> +	u8 reg_mask;
> +	u8 data[10];
> +	struct rtc_time tm;
> +
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	struct rv8803_data *rv8803 = dev_get_drvdata(dev->parent);
> +
> +	guard(mutex)(&rv8803->flags_lock);
> +
> +	/*
> +	 * For detailed description see datasheet:
> +	 *  - Reading Time Stamp Data (FIFO mode)
> +	 */
> +
> +	/* check interrupt source is from event 1-3 */
> +	ret = rv8803_read_reg(client, RX8901_EVNT_INTF);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* CHECK for EVF bit */
> +	if (ret & BIT(2)) {
> +		/* clear EVINxF 1-3 */
> +		reg_mask = BIT(5) | BIT(6) | BIT(7);
> +		ret = rv8803_write_reg(client, RX8901_EVNT_INTF, ret & ~reg_mask);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/* check interrupt source is from event 1-3 */
> +	ret = rv8803_read_reg(client, RX8901_BUF_INTF);
> +	if (ret < 0)
> +		return ret;
> +	if (ret & RX8901_BUF_INTF_BUF1F) {
> +		/* disable interrupts */
> +		ret = rv8803_read_reg(client, RV8803_CTRL);
> +		if (ret < 0)
> +			return ret;
> +		ret = rv8803_write_reg(client, RV8803_CTRL, ret & ~RV8803_CTRL_EIE);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* clear interrupt flag */
> +		ret = rv8803_read_reg(client, RX8901_BUF_INTF);
> +		if (ret < 0)
> +			return ret;
> +		ret = rv8803_write_reg(client, RX8901_BUF_INTF, ret & ~RX8901_BUF_INTF_BUF1F);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/* test if there are events available */
> +	ret = rv8803_read_reg(client, RX8901_BUF1_STAT);
> +	if (ret < 0)
> +		return ret;
> +	num_events = ret & RX8901_BUFx_STAT_PTR;
> +
> +	if (num_events) {

    if (!num_events)
        goto exit_reenable_irq;

would be better readable.

> +		ret = rv8803_read_regs(client, 0x60, ARRAY_SIZE(data), data);
> +		if (ret < 0)
> +			return ret;
> +
> +		tm.tm_sec = bcd2bin(data[2]);
> +		tm.tm_min = bcd2bin(data[3]);
> +		tm.tm_hour = bcd2bin(data[4]);
> +		tm.tm_mday = bcd2bin(data[5]);
> +		tm.tm_mon = bcd2bin(data[6]) - 1;
> +		tm.tm_year = bcd2bin(data[7]) + 100;
> +		tm.tm_wday = -1;
> +		tm.tm_yday = -1;
> +		tm.tm_isdst = -1;
> +
> +		ret = rtc_valid_tm(&tm);
> +		if (ret)
> +			return ret;
> +
> +		/* calculate 1/1024 -> ms */
> +		time_ms = (1000 * ((data[1] << 2) | (data[0] >> 6))) / 1024;
> +		time_s = rtc_tm_to_time64(&tm);
> +
> +		offset += snprintf(buf + offset, PAGE_SIZE - offset, "%llu.%03d", time_s, time_ms);
> +		for (ev_idx = 0; trg_status_txt[ev_idx].txt; ++ev_idx)
> +			if (data[9] & trg_status_txt[ev_idx].val)
> +				offset += snprintf(buf + offset, PAGE_SIZE - offset, " %s=%d",
> +						   trg_status_txt[ev_idx].txt,
> +						   !!(trg_status_txt[ev_idx].val & data[8]));
> +		offset += snprintf(buf + offset, PAGE_SIZE - offset, "\n");
> +
> +		/* according to the datasheet we have to wait for 1ms */

Is it fine that interrupts are disabled that long?

> +		usleep_range(1000, 2000);
> +	}
> +

exit_reenable_irq:

> +	/* re-enable interrupts */
> +	ret = rv8803_read_reg(client, RV8803_CTRL);

Is it fine, that interrupts are only enabled again, if all of the above code
doesn't return with a failure?

> +	if (ret < 0)
> +		return ret;
> +	ret = rv8803_write_reg(client, RV8803_CTRL, ret | RV8803_CTRL_EIE);
> +	if (ret < 0)
> +		return ret;
> +
> +	return offset;
> +}
> +
> +static DEVICE_ATTR_WO(enable);
> +static DEVICE_ATTR_RO(read);
> +
> +static struct attribute *rv8803_rtc_event_attrs[] = {
> +	&dev_attr_enable.attr,
> +	&dev_attr_read.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group rv8803_rtc_sysfs_event_files = {
> +	.name = "tamper",
> +	.attrs	= rv8803_rtc_event_attrs,
> +};
> +
>  static const struct rtc_class_ops rv8803_rtc_ops = {
>  	.read_time = rv8803_get_time,
>  	.set_time = rv8803_set_time,
> @@ -732,6 +1092,12 @@ static int rv8803_probe(struct i2c_client *client)
>  	if (err)
>  		return err;
>  
> +	if (rv8803->type == rx_8901) {
> +		err = rtc_add_group(rv8803->rtc, &rv8803_rtc_sysfs_event_files);
> +		if (err)
> +			return err;
> +	}
> +
>  	rv8803->rtc->ops = &rv8803_rtc_ops;
>  	rv8803->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
>  	rv8803->rtc->range_max = RTC_TIMESTAMP_END_2099;
> -- 
> 2.39.5
>