devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] rtc: isl1208: fixes, documentation and isl1219 support
@ 2018-01-23 12:17 Michael Grzeschik
       [not found] ` <20180123121801.4214-1-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
  2018-01-23 12:18 ` [PATCH 3/4] rtc: isl1208: enable interrupt after context preparation Michael Grzeschik
  0 siblings, 2 replies; 23+ messages in thread
From: Michael Grzeschik @ 2018-01-23 12:17 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni
  Cc: linux-rtc, linux-kernel, devicetree, linux, jdelvare, kernel,
	Denis.Osterland

This series includes fixes regarding the time setup and interrupt
preparation. It also adds devicetree binding documentation.

It also adds support for the isl1219 device that comes with tamper
detection support. For this we also add devicetree binding documentation
and instantiate the function via hwmon including the new
intrusion[0-*]_timestamp interface.


Denis Osterland (2):
  rtc: isl1208: Fix unintended clear of SR bits
  rtc: isl1208: Add device tree binding documentation

Michael Grzeschik (2):
  rtc: isl1208: enable interrupt after context preparation
  rtc: isl1208: add support for isl1219 with hwmon for tamper detection

 .../devicetree/bindings/rtc/isil,isl1208.txt       |  51 +++++
 Documentation/hwmon/sysfs-interface                |   7 +
 drivers/rtc/rtc-isl1208.c                          | 227 ++++++++++++++++++---
 3 files changed, 256 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl1208.txt

-- 
2.11.0

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 1/4] rtc: isl1208: Fix unintended clear of SR bits
       [not found] ` <20180123121801.4214-1-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2018-01-23 12:17   ` Michael Grzeschik
       [not found]     ` <20180123121801.4214-2-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
  2018-01-23 12:17   ` [PATCH 2/4] rtc: isl1208: Add device tree binding documentation Michael Grzeschik
  2018-01-23 12:18   ` [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection Michael Grzeschik
  2 siblings, 1 reply; 23+ messages in thread
From: Michael Grzeschik @ 2018-01-23 12:17 UTC (permalink / raw)
  To: a.zummo-BfzFCNDTiLLj+vYz1yj4TQ,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: linux-rtc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, linux-0h96xk9xTtrk1uMJSBkQmQ,
	jdelvare-IBi9RG/b67k, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
	Denis.Osterland-85mDkmTllUAAvxtiuMwx3w

From: Denis Osterland <Denis.Osterland-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>

After successful
sr = isl1208_i2c_set_regs(client, 0, regs, ISL1208_RTC_SECTION_LEN);
sr will be 0.
As a result
sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR,
			sr & ~ISL1208_REG_SR_WRTC);
is equal to
sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR, 0);
which clears all flags in SR.

Add an additional read of SR, to have value of SR in sr again.

Signed-off-by: Denis Osterland <Denis.Osterland-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/rtc/rtc-isl1208.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index 8dd299c6a1f33..c8b4953482296 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -459,6 +459,11 @@ isl1208_i2c_set_time(struct i2c_client *client, struct rtc_time const *tm)
 	}
 
 	/* clear WRTC again */
+	sr = isl1208_i2c_get_sr(client);
+	if (sr < 0) {
+		dev_err(&client->dev, "%s: reading SR failed\n", __func__);
+		return sr;
+	}
 	sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR,
 				       sr & ~ISL1208_REG_SR_WRTC);
 	if (sr < 0) {
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 2/4] rtc: isl1208: Add device tree binding documentation
       [not found] ` <20180123121801.4214-1-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
  2018-01-23 12:17   ` [PATCH 1/4] rtc: isl1208: Fix unintended clear of SR bits Michael Grzeschik
@ 2018-01-23 12:17   ` Michael Grzeschik
  2018-01-29 23:34     ` Rob Herring
  2018-01-30 10:06     ` Alexandre Belloni
  2018-01-23 12:18   ` [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection Michael Grzeschik
  2 siblings, 2 replies; 23+ messages in thread
From: Michael Grzeschik @ 2018-01-23 12:17 UTC (permalink / raw)
  To: a.zummo-BfzFCNDTiLLj+vYz1yj4TQ,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: linux-rtc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, linux-0h96xk9xTtrk1uMJSBkQmQ,
	jdelvare-IBi9RG/b67k, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
	Denis.Osterland-85mDkmTllUAAvxtiuMwx3w

From: Denis Osterland <Denis.Osterland-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>

Wrote documentation for ISL1208, ISL1218 device tree
binding with short examples.

Signed-off-by: Denis Osterland <Denis.Osterland-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 .../devicetree/bindings/rtc/intersil,isl1208.txt   | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl1208.txt

diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt b/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
new file mode 100644
index 0000000000000..a54e99feae1ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
@@ -0,0 +1,35 @@
+Intersil ISL1208, ISL1218 I2C RTC/Alarm chip
+
+ISL1208 is a trivial I2C device (it has simple device tree bindings,
+consisting of a compatible field, an address and possibly an interrupt
+line).
+
+Required properties supported by the device:
+
+ - "compatible": must be one of
+	"isil,isl1208"
+	"isil,isl1218"
+ - "reg": I2C bus address of the device
+
+Optional properties:
+
+ - "interrupt-parent", "interrupts", "interrupts-extended":
+   for passing the interrupt line of the SoC connected to #IRQ pin
+   of the RTC chip.
+
+
+Example isl1208 node without #IRQ pin connected:
+
+	isl1208: isl1208@68 {
+		compatible = "isil,isl1208";
+		reg = <0x68>;
+	};
+
+Example isl1208 node with #IRQ pin connected to SoC gpio1 pin 12:
+
+	isl1208: isl1208@68 {
+		compatible = "isil,isl1208";
+		reg = <0x68>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
+	};
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 3/4] rtc: isl1208: enable interrupt after context preparation
  2018-01-23 12:17 [PATCH 0/4] rtc: isl1208: fixes, documentation and isl1219 support Michael Grzeschik
       [not found] ` <20180123121801.4214-1-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2018-01-23 12:18 ` Michael Grzeschik
  2018-01-30 10:34   ` Alexandre Belloni
  1 sibling, 1 reply; 23+ messages in thread
From: Michael Grzeschik @ 2018-01-23 12:18 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni
  Cc: linux-rtc, linux-kernel, devicetree, linux, jdelvare, kernel,
	Denis.Osterland

The interrupt handler got enabled very early. If the interrupt cause is
triggering immediately before the context is fully prepared. This can
lead to undefined behaviour. Therefor we move the interrupt enable code
to the end of the probe function.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
---
 drivers/rtc/rtc-isl1208.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index c8b4953482296..a13a4ba79004d 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -635,23 +635,6 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (isl1208_i2c_validate_client(client) < 0)
 		return -ENODEV;
 
-	if (client->irq > 0) {
-		rc = devm_request_threaded_irq(&client->dev, client->irq, NULL,
-					       isl1208_rtc_interrupt,
-					       IRQF_SHARED | IRQF_ONESHOT,
-					       isl1208_driver.driver.name,
-					       client);
-		if (!rc) {
-			device_init_wakeup(&client->dev, 1);
-			enable_irq_wake(client->irq);
-		} else {
-			dev_err(&client->dev,
-				"Unable to request irq %d, no alarm support\n",
-				client->irq);
-			client->irq = 0;
-		}
-	}
-
 	rtc = devm_rtc_device_register(&client->dev, isl1208_driver.driver.name,
 				  &isl1208_rtc_ops,
 				  THIS_MODULE);
@@ -674,6 +657,23 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (rc)
 		return rc;
 
+	if (client->irq > 0) {
+		rc = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+					       isl1208_rtc_interrupt,
+					       IRQF_SHARED | IRQF_ONESHOT,
+					       isl1208_driver.driver.name,
+					       client);
+		if (!rc) {
+			device_init_wakeup(&client->dev, 1);
+			enable_irq_wake(client->irq);
+		} else {
+			dev_err(&client->dev,
+				"Unable to request irq %d, no alarm support\n",
+				client->irq);
+			client->irq = 0;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
       [not found] ` <20180123121801.4214-1-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
  2018-01-23 12:17   ` [PATCH 1/4] rtc: isl1208: Fix unintended clear of SR bits Michael Grzeschik
  2018-01-23 12:17   ` [PATCH 2/4] rtc: isl1208: Add device tree binding documentation Michael Grzeschik
@ 2018-01-23 12:18   ` Michael Grzeschik
       [not found]     ` <20180123121801.4214-5-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
  2 siblings, 1 reply; 23+ messages in thread
From: Michael Grzeschik @ 2018-01-23 12:18 UTC (permalink / raw)
  To: a.zummo-BfzFCNDTiLLj+vYz1yj4TQ,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: linux-rtc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, linux-0h96xk9xTtrk1uMJSBkQmQ,
	jdelvare-IBi9RG/b67k, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
	Denis.Osterland-85mDkmTllUAAvxtiuMwx3w

We add support for the ISL1219 chip that got an integrated tamper
detection function. This patch implements the feature by using an hwmon
interface.

The ISL1219 can also describe the timestamp of the intrusion
event. For this we add the documentation of the new interface
intrusion[0-*]_timestamp.

The devicetree documentation for the ISL1219 device tree
binding is added with an short example.

Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Denis Osterland <Denis.Osterland-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
---
 .../rtc/{intersil,isl1208.txt => isil,isl1208.txt} |  18 +-
 Documentation/hwmon/sysfs-interface                |   7 +
 drivers/rtc/rtc-isl1208.c                          | 190 +++++++++++++++++++--
 3 files changed, 201 insertions(+), 14 deletions(-)
 rename Documentation/devicetree/bindings/rtc/{intersil,isl1208.txt => isil,isl1208.txt} (57%)

diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
similarity index 57%
rename from Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
rename to Documentation/devicetree/bindings/rtc/isil,isl1208.txt
index a54e99feae1ca..d549699e1cfc4 100644
--- a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
+++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
@@ -1,14 +1,21 @@
-Intersil ISL1208, ISL1218 I2C RTC/Alarm chip
+Intersil ISL1208, ISL1218, ISL1219 I2C RTC/Alarm chip
 
 ISL1208 is a trivial I2C device (it has simple device tree bindings,
 consisting of a compatible field, an address and possibly an interrupt
 line).
 
+ISL1219 supports tamper detection user space representation through
+case intrusion hwmon sensor.
+ISL1219 has additional pins EVIN and #EVDET for tamper detection.
+I2C devices support only one irq. #IRQ and #EVDET are open-drain active low,
+so it is possible layout them to one SoC pin with pull-up.
+
 Required properties supported by the device:
 
  - "compatible": must be one of
 	"isil,isl1208"
 	"isil,isl1218"
+	"isil,isl1219"
  - "reg": I2C bus address of the device
 
 Optional properties:
@@ -33,3 +40,12 @@ Example isl1208 node with #IRQ pin connected to SoC gpio1 pin 12:
 		interrupt-parent = <&gpio1>;
 		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
 	};
+
+Example isl1219 node with #IRQ pin and #EVDET pin connected to SoC gpio1 pin 12:
+
+	isl1219: isl1219@68 {
+		compatible = "intersil,isl1219";
+		reg = <0x68>;
+		interrupts-extended = <&gpio1 12 IRQ_TYPE_EDGE_FALLING>;
+	};
+
diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
index fc337c317c673..a12b3c2b2a18c 100644
--- a/Documentation/hwmon/sysfs-interface
+++ b/Documentation/hwmon/sysfs-interface
@@ -702,6 +702,13 @@ intrusion[0-*]_alarm
 		the user. This is done by writing 0 to the file. Writing
 		other values is unsupported.
 
+intrusion[0-*]_timestamp
+		Chassis intrusion detection
+		YYYY-MM-DD HH:MM:SS UTC (ts.sec): intrusion detected
+		RO
+		The corresponding timestamp on which the intrustion
+		was detected.
+
 intrusion[0-*]_beep
 		Chassis intrusion beep
 		0: disable
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index a13a4ba79004d..6e4d24614d98b 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -14,6 +14,8 @@
 #include <linux/i2c.h>
 #include <linux/bcd.h>
 #include <linux/rtc.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
 
 /* Register map */
 /* rtc section */
@@ -33,6 +35,7 @@
 #define ISL1208_REG_SR_ARST    (1<<7)	/* auto reset */
 #define ISL1208_REG_SR_XTOSCB  (1<<6)	/* crystal oscillator */
 #define ISL1208_REG_SR_WRTC    (1<<4)	/* write rtc */
+#define ISL1208_REG_SR_EVT     (1<<3)	/* event */
 #define ISL1208_REG_SR_ALM     (1<<2)	/* alarm */
 #define ISL1208_REG_SR_BAT     (1<<1)	/* battery */
 #define ISL1208_REG_SR_RTCF    (1<<0)	/* rtc fail */
@@ -57,8 +60,29 @@
 #define ISL1208_REG_USR2 0x13
 #define ISL1208_USR_SECTION_LEN 2
 
+/* event section */
+#define ISL1208_REG_SCT 0x14
+#define ISL1208_REG_MNT 0x15
+#define ISL1208_REG_HRT 0x16
+#define ISL1208_REG_DTT 0x17
+#define ISL1208_REG_MOT 0x18
+#define ISL1208_REG_YRT 0x19
+#define ISL1208_EVT_SECTION_LEN 6
+
 static struct i2c_driver isl1208_driver;
 
+/* ISL1208 various variants */
+enum {
+	TYPE_ISL1208 = 0,
+	TYPE_ISL1218,
+	TYPE_ISL1219,
+};
+
+struct isl1208 {
+	struct rtc_device *rtc;
+	struct device *hwmon;
+};
+
 /* block read */
 static int
 isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
@@ -80,8 +104,8 @@ isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
 	};
 	int ret;
 
-	BUG_ON(reg > ISL1208_REG_USR2);
-	BUG_ON(reg + len > ISL1208_REG_USR2 + 1);
+	WARN_ON(reg > ISL1208_REG_YRT);
+	WARN_ON(reg + len > ISL1208_REG_YRT + 1);
 
 	ret = i2c_transfer(client->adapter, msgs, 2);
 	if (ret > 0)
@@ -104,8 +128,8 @@ isl1208_i2c_set_regs(struct i2c_client *client, u8 reg, u8 const buf[],
 	};
 	int ret;
 
-	BUG_ON(reg > ISL1208_REG_USR2);
-	BUG_ON(reg + len > ISL1208_REG_USR2 + 1);
+	WARN_ON(reg > ISL1208_REG_YRT);
+	WARN_ON(reg + len > ISL1208_REG_YRT + 1);
 
 	i2c_buf[0] = reg;
 	memcpy(&i2c_buf[1], &buf[0], len);
@@ -493,12 +517,128 @@ isl1208_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	return isl1208_i2c_set_alarm(to_i2c_client(dev), alarm);
 }
 
+static ssize_t isl1208_hwmon_show_tamper(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = dev_get_drvdata(dev);
+	int sr;
+
+	sr = isl1208_i2c_get_sr(client);
+	if (sr < 0) {
+		dev_err(dev, "%s: reading SR failed\n", __func__);
+		return sr;
+	}
+
+	if (sr & ISL1208_REG_SR_EVT)
+		return sprintf(buf, "1\n");
+
+	return sprintf(buf, "0\n");
+};
+
+static ssize_t isl1208_hwmon_clear_caseopen(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct i2c_client *client = dev_get_drvdata(dev);
+	unsigned long val;
+	int sr;
+
+	if (kstrtoul(buf, 10, &val) || val != 0)
+		return -EINVAL;
+
+	sr = isl1208_i2c_get_sr(client);
+	if (sr < 0) {
+		dev_err(dev, "%s: reading SR failed\n", __func__);
+		return sr;
+	}
+
+	sr &= ~ISL1208_REG_SR_EVT;
+
+	sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr);
+	if (sr < 0)
+		dev_err(dev, "%s: writing SR failed\n",
+			__func__);
+
+	return count;
+};
+
+static ssize_t isl1208_hwmon_show_tamper_timestamp(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = dev_get_drvdata(dev);
+	u8 regs[ISL1208_EVT_SECTION_LEN] = { 0, };
+	struct timespec64 tv64;
+	struct rtc_time tm;
+	int sr;
+
+	sr = isl1208_i2c_get_sr(client);
+	if (sr < 0) {
+		dev_err(dev, "%s: reading SR failed\n", __func__);
+		return sr;
+	}
+
+	if (!(sr & ISL1208_REG_SR_EVT))
+		return 0;
+
+	sr = isl1208_i2c_read_regs(client, ISL1208_REG_SCT, regs,
+				   ISL1208_EVT_SECTION_LEN);
+	if (sr < 0) {
+		dev_err(dev, "%s: reading event section failed\n",
+			__func__);
+		return 0;
+	}
+
+	/* MSB of each alarm register is an enable bit */
+	tm.tm_sec = bcd2bin(regs[ISL1208_REG_SCT - ISL1208_REG_SCT] & 0x7f);
+	tm.tm_min = bcd2bin(regs[ISL1208_REG_MNT - ISL1208_REG_SCT] & 0x7f);
+	tm.tm_hour = bcd2bin(regs[ISL1208_REG_HRT - ISL1208_REG_SCT] & 0x3f);
+	tm.tm_mday = bcd2bin(regs[ISL1208_REG_DTT - ISL1208_REG_SCT] & 0x3f);
+	tm.tm_mon =
+		bcd2bin(regs[ISL1208_REG_MOT - ISL1208_REG_SCT] & 0x1f) - 1;
+	tm.tm_year = bcd2bin(regs[ISL1208_REG_YRT - ISL1208_REG_SCT]) + 100;
+
+	tv64.tv_sec = rtc_tm_to_time64(&tm);
+
+	return sprintf(buf,
+		"%d-%02d-%02d %02d:%02d:%02d UTC (%lld)\n",
+		tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+		tm.tm_hour, tm.tm_min, tm.tm_sec,
+		(long long) tv64.tv_sec);
+};
+
+static SENSOR_DEVICE_ATTR(intrusion0_alarm, 0644, isl1208_hwmon_show_tamper,
+			isl1208_hwmon_clear_caseopen, 0);
+
+static SENSOR_DEVICE_ATTR(intrusion0_timestamp, 0444,
+		isl1208_hwmon_show_tamper_timestamp, NULL, 0);
+
+static struct attribute *isl1208_hwmon_attrs[] = {
+	&sensor_dev_attr_intrusion0_alarm.dev_attr.attr,
+	&sensor_dev_attr_intrusion0_timestamp.dev_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(isl1208_hwmon);
+
+static void isl1208_hwmon_register(struct i2c_client *client)
+{
+	struct isl1208 *isl1208 = i2c_get_clientdata(client);
+
+	isl1208->hwmon = devm_hwmon_device_register_with_groups(&client->dev,
+						client->name,
+						client, isl1208_hwmon_groups);
+	if (IS_ERR(isl1208->hwmon)) {
+		dev_warn(&client->dev,
+			"unable to register hwmon device %ld\n",
+			PTR_ERR(isl1208->hwmon));
+	}
+}
+
 static irqreturn_t
 isl1208_rtc_interrupt(int irq, void *data)
 {
 	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
 	struct i2c_client *client = data;
-	struct rtc_device *rtc = i2c_get_clientdata(client);
+	struct isl1208 *isl1208 = i2c_get_clientdata(client);
 	int handled = 0, sr, err;
 
 	/*
@@ -521,7 +661,7 @@ isl1208_rtc_interrupt(int irq, void *data)
 	if (sr & ISL1208_REG_SR_ALM) {
 		dev_dbg(&client->dev, "alarm!\n");
 
-		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
+		rtc_update_irq(isl1208->rtc, 1, RTC_IRQF | RTC_AF);
 
 		/* Clear the alarm */
 		sr &= ~ISL1208_REG_SR_ALM;
@@ -538,6 +678,13 @@ isl1208_rtc_interrupt(int irq, void *data)
 			return err;
 	}
 
+	if (isl1208->hwmon && (sr & ISL1208_REG_SR_EVT)) {
+		sysfs_notify(&isl1208->hwmon->kobj, NULL,
+			sensor_dev_attr_intrusion0_alarm.dev_attr.attr.name);
+		dev_warn(&client->dev, "event detected");
+		handled = 1;
+	}
+
 	return handled ? IRQ_HANDLED : IRQ_NONE;
 }
 
@@ -627,7 +774,7 @@ static int
 isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	int rc = 0;
-	struct rtc_device *rtc;
+	struct isl1208 *isl1208;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
@@ -635,13 +782,19 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (isl1208_i2c_validate_client(client) < 0)
 		return -ENODEV;
 
-	rtc = devm_rtc_device_register(&client->dev, isl1208_driver.driver.name,
+	isl1208 = devm_kzalloc(&client->dev, sizeof(struct isl1208),
+				GFP_KERNEL);
+	if (!isl1208)
+		return -ENOMEM;
+
+	isl1208->rtc = devm_rtc_device_register(&client->dev,
+				  isl1208_driver.driver.name,
 				  &isl1208_rtc_ops,
 				  THIS_MODULE);
-	if (IS_ERR(rtc))
-		return PTR_ERR(rtc);
+	if (IS_ERR(isl1208->rtc))
+		return PTR_ERR(isl1208->rtc);
 
-	i2c_set_clientdata(client, rtc);
+	i2c_set_clientdata(client, isl1208);
 
 	rc = isl1208_i2c_get_sr(client);
 	if (rc < 0) {
@@ -657,6 +810,15 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (rc)
 		return rc;
 
+	if (id->driver_data == TYPE_ISL1219) {
+		rc = i2c_smbus_write_byte_data(client, ISL1208_REG_09, 0x10);
+		if (rc < 0) {
+			dev_err(&client->dev, "could not enable tamper detection\n");
+			return rc;
+		}
+		isl1208_hwmon_register(client);
+	}
+
 	if (client->irq > 0) {
 		rc = devm_request_threaded_irq(&client->dev, client->irq, NULL,
 					       isl1208_rtc_interrupt,
@@ -686,8 +848,9 @@ isl1208_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id isl1208_id[] = {
-	{ "isl1208", 0 },
-	{ "isl1218", 0 },
+	{ "isl1208", TYPE_ISL1208 },
+	{ "isl1218", TYPE_ISL1218 },
+	{ "isl1219", TYPE_ISL1219 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, isl1208_id);
@@ -695,6 +858,7 @@ MODULE_DEVICE_TABLE(i2c, isl1208_id);
 static const struct of_device_id isl1208_of_match[] = {
 	{ .compatible = "isil,isl1208" },
 	{ .compatible = "isil,isl1218" },
+	{ .compatible = "isil,isl1219" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, isl1208_of_match);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
       [not found]     ` <20180123121801.4214-5-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2018-01-23 18:22       ` Guenter Roeck
  2018-01-24  9:03         ` Michael Grzeschik
  2018-01-29 23:41       ` Rob Herring
  1 sibling, 1 reply; 23+ messages in thread
From: Guenter Roeck @ 2018-01-23 18:22 UTC (permalink / raw)
  To: Michael Grzeschik
  Cc: a.zummo-BfzFCNDTiLLj+vYz1yj4TQ,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	linux-rtc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, jdelvare-IBi9RG/b67k,
	kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
	Denis.Osterland-85mDkmTllUAAvxtiuMwx3w

On Tue, Jan 23, 2018 at 01:18:01PM +0100, Michael Grzeschik wrote:
> We add support for the ISL1219 chip that got an integrated tamper
> detection function. This patch implements the feature by using an hwmon
> interface.
> 
> The ISL1219 can also describe the timestamp of the intrusion
> event. For this we add the documentation of the new interface
> intrusion[0-*]_timestamp.
> 
> The devicetree documentation for the ISL1219 device tree
> binding is added with an short example.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Denis Osterland <Denis.Osterland-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
> ---
>  .../rtc/{intersil,isl1208.txt => isil,isl1208.txt} |  18 +-
>  Documentation/hwmon/sysfs-interface                |   7 +
>  drivers/rtc/rtc-isl1208.c                          | 190 +++++++++++++++++++--
>  3 files changed, 201 insertions(+), 14 deletions(-)
>  rename Documentation/devicetree/bindings/rtc/{intersil,isl1208.txt => isil,isl1208.txt} (57%)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> similarity index 57%
> rename from Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> rename to Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> index a54e99feae1ca..d549699e1cfc4 100644
> --- a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> +++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> @@ -1,14 +1,21 @@
> -Intersil ISL1208, ISL1218 I2C RTC/Alarm chip
> +Intersil ISL1208, ISL1218, ISL1219 I2C RTC/Alarm chip
>  
>  ISL1208 is a trivial I2C device (it has simple device tree bindings,
>  consisting of a compatible field, an address and possibly an interrupt
>  line).
>  
> +ISL1219 supports tamper detection user space representation through
> +case intrusion hwmon sensor.
> +ISL1219 has additional pins EVIN and #EVDET for tamper detection.
> +I2C devices support only one irq. #IRQ and #EVDET are open-drain active low,
> +so it is possible layout them to one SoC pin with pull-up.
> +
>  Required properties supported by the device:
>  
>   - "compatible": must be one of
>  	"isil,isl1208"
>  	"isil,isl1218"
> +	"isil,isl1219"
>   - "reg": I2C bus address of the device
>  
>  Optional properties:
> @@ -33,3 +40,12 @@ Example isl1208 node with #IRQ pin connected to SoC gpio1 pin 12:
>  		interrupt-parent = <&gpio1>;
>  		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
>  	};
> +
> +Example isl1219 node with #IRQ pin and #EVDET pin connected to SoC gpio1 pin 12:
> +
> +	isl1219: isl1219@68 {
> +		compatible = "intersil,isl1219";
> +		reg = <0x68>;
> +		interrupts-extended = <&gpio1 12 IRQ_TYPE_EDGE_FALLING>;
> +	};
> +
> diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> index fc337c317c673..a12b3c2b2a18c 100644
> --- a/Documentation/hwmon/sysfs-interface
> +++ b/Documentation/hwmon/sysfs-interface
> @@ -702,6 +702,13 @@ intrusion[0-*]_alarm
>  		the user. This is done by writing 0 to the file. Writing
>  		other values is unsupported.
>  
> +intrusion[0-*]_timestamp
> +		Chassis intrusion detection
> +		YYYY-MM-DD HH:MM:SS UTC (ts.sec): intrusion detected
> +		RO
> +		The corresponding timestamp on which the intrustion
> +		was detected.
> +

Sneaky. Nack. You don't just add attributes to the ABI because you want it,
without serious discussion, and much less so hidden in an RTC driver
(and even less as unparseable attribute).

In addition to that, I consider the attribute unnecessary. The intrusion
already generates an event which should be sufficient for all practical
purposes.

Guenter

>  intrusion[0-*]_beep
>  		Chassis intrusion beep
>  		0: disable
> diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> index a13a4ba79004d..6e4d24614d98b 100644
> --- a/drivers/rtc/rtc-isl1208.c
> +++ b/drivers/rtc/rtc-isl1208.c
> @@ -14,6 +14,8 @@
>  #include <linux/i2c.h>
>  #include <linux/bcd.h>
>  #include <linux/rtc.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
>  
>  /* Register map */
>  /* rtc section */
> @@ -33,6 +35,7 @@
>  #define ISL1208_REG_SR_ARST    (1<<7)	/* auto reset */
>  #define ISL1208_REG_SR_XTOSCB  (1<<6)	/* crystal oscillator */
>  #define ISL1208_REG_SR_WRTC    (1<<4)	/* write rtc */
> +#define ISL1208_REG_SR_EVT     (1<<3)	/* event */
>  #define ISL1208_REG_SR_ALM     (1<<2)	/* alarm */
>  #define ISL1208_REG_SR_BAT     (1<<1)	/* battery */
>  #define ISL1208_REG_SR_RTCF    (1<<0)	/* rtc fail */
> @@ -57,8 +60,29 @@
>  #define ISL1208_REG_USR2 0x13
>  #define ISL1208_USR_SECTION_LEN 2
>  
> +/* event section */
> +#define ISL1208_REG_SCT 0x14
> +#define ISL1208_REG_MNT 0x15
> +#define ISL1208_REG_HRT 0x16
> +#define ISL1208_REG_DTT 0x17
> +#define ISL1208_REG_MOT 0x18
> +#define ISL1208_REG_YRT 0x19
> +#define ISL1208_EVT_SECTION_LEN 6
> +
>  static struct i2c_driver isl1208_driver;
>  
> +/* ISL1208 various variants */
> +enum {
> +	TYPE_ISL1208 = 0,
> +	TYPE_ISL1218,
> +	TYPE_ISL1219,
> +};
> +
> +struct isl1208 {
> +	struct rtc_device *rtc;
> +	struct device *hwmon;
> +};
> +
>  /* block read */
>  static int
>  isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
> @@ -80,8 +104,8 @@ isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
>  	};
>  	int ret;
>  
> -	BUG_ON(reg > ISL1208_REG_USR2);
> -	BUG_ON(reg + len > ISL1208_REG_USR2 + 1);
> +	WARN_ON(reg > ISL1208_REG_YRT);
> +	WARN_ON(reg + len > ISL1208_REG_YRT + 1);
>  
>  	ret = i2c_transfer(client->adapter, msgs, 2);
>  	if (ret > 0)
> @@ -104,8 +128,8 @@ isl1208_i2c_set_regs(struct i2c_client *client, u8 reg, u8 const buf[],
>  	};
>  	int ret;
>  
> -	BUG_ON(reg > ISL1208_REG_USR2);
> -	BUG_ON(reg + len > ISL1208_REG_USR2 + 1);
> +	WARN_ON(reg > ISL1208_REG_YRT);
> +	WARN_ON(reg + len > ISL1208_REG_YRT + 1);
>  
>  	i2c_buf[0] = reg;
>  	memcpy(&i2c_buf[1], &buf[0], len);
> @@ -493,12 +517,128 @@ isl1208_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>  	return isl1208_i2c_set_alarm(to_i2c_client(dev), alarm);
>  }
>  
> +static ssize_t isl1208_hwmon_show_tamper(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = dev_get_drvdata(dev);
> +	int sr;
> +
> +	sr = isl1208_i2c_get_sr(client);
> +	if (sr < 0) {
> +		dev_err(dev, "%s: reading SR failed\n", __func__);
> +		return sr;
> +	}
> +
> +	if (sr & ISL1208_REG_SR_EVT)
> +		return sprintf(buf, "1\n");
> +
> +	return sprintf(buf, "0\n");
> +};
> +
> +static ssize_t isl1208_hwmon_clear_caseopen(struct device *dev,
> +		struct device_attribute *attr,
> +		const char *buf, size_t count)
> +{
> +	struct i2c_client *client = dev_get_drvdata(dev);
> +	unsigned long val;
> +	int sr;
> +
> +	if (kstrtoul(buf, 10, &val) || val != 0)
> +		return -EINVAL;
> +
> +	sr = isl1208_i2c_get_sr(client);
> +	if (sr < 0) {
> +		dev_err(dev, "%s: reading SR failed\n", __func__);
> +		return sr;
> +	}
> +
> +	sr &= ~ISL1208_REG_SR_EVT;
> +
> +	sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr);
> +	if (sr < 0)
> +		dev_err(dev, "%s: writing SR failed\n",
> +			__func__);
> +
> +	return count;
> +};
> +
> +static ssize_t isl1208_hwmon_show_tamper_timestamp(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = dev_get_drvdata(dev);
> +	u8 regs[ISL1208_EVT_SECTION_LEN] = { 0, };
> +	struct timespec64 tv64;
> +	struct rtc_time tm;
> +	int sr;
> +
> +	sr = isl1208_i2c_get_sr(client);
> +	if (sr < 0) {
> +		dev_err(dev, "%s: reading SR failed\n", __func__);
> +		return sr;
> +	}
> +
> +	if (!(sr & ISL1208_REG_SR_EVT))
> +		return 0;
> +
> +	sr = isl1208_i2c_read_regs(client, ISL1208_REG_SCT, regs,
> +				   ISL1208_EVT_SECTION_LEN);
> +	if (sr < 0) {
> +		dev_err(dev, "%s: reading event section failed\n",
> +			__func__);
> +		return 0;
> +	}
> +
> +	/* MSB of each alarm register is an enable bit */
> +	tm.tm_sec = bcd2bin(regs[ISL1208_REG_SCT - ISL1208_REG_SCT] & 0x7f);
> +	tm.tm_min = bcd2bin(regs[ISL1208_REG_MNT - ISL1208_REG_SCT] & 0x7f);
> +	tm.tm_hour = bcd2bin(regs[ISL1208_REG_HRT - ISL1208_REG_SCT] & 0x3f);
> +	tm.tm_mday = bcd2bin(regs[ISL1208_REG_DTT - ISL1208_REG_SCT] & 0x3f);
> +	tm.tm_mon =
> +		bcd2bin(regs[ISL1208_REG_MOT - ISL1208_REG_SCT] & 0x1f) - 1;
> +	tm.tm_year = bcd2bin(regs[ISL1208_REG_YRT - ISL1208_REG_SCT]) + 100;
> +
> +	tv64.tv_sec = rtc_tm_to_time64(&tm);
> +
> +	return sprintf(buf,
> +		"%d-%02d-%02d %02d:%02d:%02d UTC (%lld)\n",
> +		tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
> +		tm.tm_hour, tm.tm_min, tm.tm_sec,
> +		(long long) tv64.tv_sec);
> +};
> +
> +static SENSOR_DEVICE_ATTR(intrusion0_alarm, 0644, isl1208_hwmon_show_tamper,
> +			isl1208_hwmon_clear_caseopen, 0);
> +
> +static SENSOR_DEVICE_ATTR(intrusion0_timestamp, 0444,
> +		isl1208_hwmon_show_tamper_timestamp, NULL, 0);
> +
> +static struct attribute *isl1208_hwmon_attrs[] = {
> +	&sensor_dev_attr_intrusion0_alarm.dev_attr.attr,
> +	&sensor_dev_attr_intrusion0_timestamp.dev_attr.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(isl1208_hwmon);
> +
> +static void isl1208_hwmon_register(struct i2c_client *client)
> +{
> +	struct isl1208 *isl1208 = i2c_get_clientdata(client);
> +
> +	isl1208->hwmon = devm_hwmon_device_register_with_groups(&client->dev,
> +						client->name,
> +						client, isl1208_hwmon_groups);
> +	if (IS_ERR(isl1208->hwmon)) {
> +		dev_warn(&client->dev,
> +			"unable to register hwmon device %ld\n",
> +			PTR_ERR(isl1208->hwmon));
> +	}
> +}
> +
>  static irqreturn_t
>  isl1208_rtc_interrupt(int irq, void *data)
>  {
>  	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
>  	struct i2c_client *client = data;
> -	struct rtc_device *rtc = i2c_get_clientdata(client);
> +	struct isl1208 *isl1208 = i2c_get_clientdata(client);
>  	int handled = 0, sr, err;
>  
>  	/*
> @@ -521,7 +661,7 @@ isl1208_rtc_interrupt(int irq, void *data)
>  	if (sr & ISL1208_REG_SR_ALM) {
>  		dev_dbg(&client->dev, "alarm!\n");
>  
> -		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
> +		rtc_update_irq(isl1208->rtc, 1, RTC_IRQF | RTC_AF);
>  
>  		/* Clear the alarm */
>  		sr &= ~ISL1208_REG_SR_ALM;
> @@ -538,6 +678,13 @@ isl1208_rtc_interrupt(int irq, void *data)
>  			return err;
>  	}
>  
> +	if (isl1208->hwmon && (sr & ISL1208_REG_SR_EVT)) {
> +		sysfs_notify(&isl1208->hwmon->kobj, NULL,
> +			sensor_dev_attr_intrusion0_alarm.dev_attr.attr.name);
> +		dev_warn(&client->dev, "event detected");
> +		handled = 1;
> +	}
> +
>  	return handled ? IRQ_HANDLED : IRQ_NONE;
>  }
>  
> @@ -627,7 +774,7 @@ static int
>  isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
>  	int rc = 0;
> -	struct rtc_device *rtc;
> +	struct isl1208 *isl1208;
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>  		return -ENODEV;
> @@ -635,13 +782,19 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	if (isl1208_i2c_validate_client(client) < 0)
>  		return -ENODEV;
>  
> -	rtc = devm_rtc_device_register(&client->dev, isl1208_driver.driver.name,
> +	isl1208 = devm_kzalloc(&client->dev, sizeof(struct isl1208),
> +				GFP_KERNEL);
> +	if (!isl1208)
> +		return -ENOMEM;
> +
> +	isl1208->rtc = devm_rtc_device_register(&client->dev,
> +				  isl1208_driver.driver.name,
>  				  &isl1208_rtc_ops,
>  				  THIS_MODULE);
> -	if (IS_ERR(rtc))
> -		return PTR_ERR(rtc);
> +	if (IS_ERR(isl1208->rtc))
> +		return PTR_ERR(isl1208->rtc);
>  
> -	i2c_set_clientdata(client, rtc);
> +	i2c_set_clientdata(client, isl1208);
>  
>  	rc = isl1208_i2c_get_sr(client);
>  	if (rc < 0) {
> @@ -657,6 +810,15 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	if (rc)
>  		return rc;
>  
> +	if (id->driver_data == TYPE_ISL1219) {
> +		rc = i2c_smbus_write_byte_data(client, ISL1208_REG_09, 0x10);
> +		if (rc < 0) {
> +			dev_err(&client->dev, "could not enable tamper detection\n");
> +			return rc;
> +		}
> +		isl1208_hwmon_register(client);
> +	}
> +
>  	if (client->irq > 0) {
>  		rc = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>  					       isl1208_rtc_interrupt,
> @@ -686,8 +848,9 @@ isl1208_remove(struct i2c_client *client)
>  }
>  
>  static const struct i2c_device_id isl1208_id[] = {
> -	{ "isl1208", 0 },
> -	{ "isl1218", 0 },
> +	{ "isl1208", TYPE_ISL1208 },
> +	{ "isl1218", TYPE_ISL1218 },
> +	{ "isl1219", TYPE_ISL1219 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, isl1208_id);
> @@ -695,6 +858,7 @@ MODULE_DEVICE_TABLE(i2c, isl1208_id);
>  static const struct of_device_id isl1208_of_match[] = {
>  	{ .compatible = "isil,isl1208" },
>  	{ .compatible = "isil,isl1218" },
> +	{ .compatible = "isil,isl1219" },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, isl1208_of_match);
> -- 
> 2.11.0
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
  2018-01-23 18:22       ` Guenter Roeck
@ 2018-01-24  9:03         ` Michael Grzeschik
  2018-01-24 12:10           ` Michael Grzeschik
       [not found]           ` <20180124090333.r5o2mzpm4q536w4r-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
  0 siblings, 2 replies; 23+ messages in thread
From: Michael Grzeschik @ 2018-01-24  9:03 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: a.zummo, alexandre.belloni, linux-rtc, linux-kernel, devicetree,
	jdelvare, kernel, Denis.Osterland

[-- Attachment #1: Type: text/plain, Size: 14479 bytes --]

On Tue, Jan 23, 2018 at 10:22:54AM -0800, Guenter Roeck wrote:
> On Tue, Jan 23, 2018 at 01:18:01PM +0100, Michael Grzeschik wrote:
> > We add support for the ISL1219 chip that got an integrated tamper
> > detection function. This patch implements the feature by using an hwmon
> > interface.
> > 
> > The ISL1219 can also describe the timestamp of the intrusion
> > event. For this we add the documentation of the new interface
> > intrusion[0-*]_timestamp.
> > 
> > The devicetree documentation for the ISL1219 device tree
> > binding is added with an short example.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
> > ---
> >  .../rtc/{intersil,isl1208.txt => isil,isl1208.txt} |  18 +-
> >  Documentation/hwmon/sysfs-interface                |   7 +
> >  drivers/rtc/rtc-isl1208.c                          | 190 +++++++++++++++++++--
> >  3 files changed, 201 insertions(+), 14 deletions(-)
> >  rename Documentation/devicetree/bindings/rtc/{intersil,isl1208.txt => isil,isl1208.txt} (57%)
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> > similarity index 57%
> > rename from Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> > rename to Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> > index a54e99feae1ca..d549699e1cfc4 100644
> > --- a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> > +++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> > @@ -1,14 +1,21 @@
> > -Intersil ISL1208, ISL1218 I2C RTC/Alarm chip
> > +Intersil ISL1208, ISL1218, ISL1219 I2C RTC/Alarm chip
> >  
> >  ISL1208 is a trivial I2C device (it has simple device tree bindings,
> >  consisting of a compatible field, an address and possibly an interrupt
> >  line).
> >  
> > +ISL1219 supports tamper detection user space representation through
> > +case intrusion hwmon sensor.
> > +ISL1219 has additional pins EVIN and #EVDET for tamper detection.
> > +I2C devices support only one irq. #IRQ and #EVDET are open-drain active low,
> > +so it is possible layout them to one SoC pin with pull-up.
> > +
> >  Required properties supported by the device:
> >  
> >   - "compatible": must be one of
> >  	"isil,isl1208"
> >  	"isil,isl1218"
> > +	"isil,isl1219"
> >   - "reg": I2C bus address of the device
> >  
> >  Optional properties:
> > @@ -33,3 +40,12 @@ Example isl1208 node with #IRQ pin connected to SoC gpio1 pin 12:
> >  		interrupt-parent = <&gpio1>;
> >  		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
> >  	};
> > +
> > +Example isl1219 node with #IRQ pin and #EVDET pin connected to SoC gpio1 pin 12:
> > +
> > +	isl1219: isl1219@68 {
> > +		compatible = "intersil,isl1219";
> > +		reg = <0x68>;
> > +		interrupts-extended = <&gpio1 12 IRQ_TYPE_EDGE_FALLING>;
> > +	};
> > +
> > diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> > index fc337c317c673..a12b3c2b2a18c 100644
> > --- a/Documentation/hwmon/sysfs-interface
> > +++ b/Documentation/hwmon/sysfs-interface
> > @@ -702,6 +702,13 @@ intrusion[0-*]_alarm
> >  		the user. This is done by writing 0 to the file. Writing
> >  		other values is unsupported.
> >  
> > +intrusion[0-*]_timestamp
> > +		Chassis intrusion detection
> > +		YYYY-MM-DD HH:MM:SS UTC (ts.sec): intrusion detected
> > +		RO
> > +		The corresponding timestamp on which the intrustion
> > +		was detected.
> > +
> 
> Sneaky. Nack. You don't just add attributes to the ABI because you want it,
> without serious discussion, and much less so hidden in an RTC driver
> (and even less as unparseable attribute).

Right; but it was not meant to be sneaky. I should have stick to my first
thought and label this patch RFC. Sorry for that.

> In addition to that, I consider the attribute unnecessary. The intrusion
> already generates an event which should be sufficient for all practical
> purposes.

Would it make sense in between the other sysfs attributes of this driver?

Thanks,
Michael

> >  intrusion[0-*]_beep
> >  		Chassis intrusion beep
> >  		0: disable
> > diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> > index a13a4ba79004d..6e4d24614d98b 100644
> > --- a/drivers/rtc/rtc-isl1208.c
> > +++ b/drivers/rtc/rtc-isl1208.c
> > @@ -14,6 +14,8 @@
> >  #include <linux/i2c.h>
> >  #include <linux/bcd.h>
> >  #include <linux/rtc.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> >  
> >  /* Register map */
> >  /* rtc section */
> > @@ -33,6 +35,7 @@
> >  #define ISL1208_REG_SR_ARST    (1<<7)	/* auto reset */
> >  #define ISL1208_REG_SR_XTOSCB  (1<<6)	/* crystal oscillator */
> >  #define ISL1208_REG_SR_WRTC    (1<<4)	/* write rtc */
> > +#define ISL1208_REG_SR_EVT     (1<<3)	/* event */
> >  #define ISL1208_REG_SR_ALM     (1<<2)	/* alarm */
> >  #define ISL1208_REG_SR_BAT     (1<<1)	/* battery */
> >  #define ISL1208_REG_SR_RTCF    (1<<0)	/* rtc fail */
> > @@ -57,8 +60,29 @@
> >  #define ISL1208_REG_USR2 0x13
> >  #define ISL1208_USR_SECTION_LEN 2
> >  
> > +/* event section */
> > +#define ISL1208_REG_SCT 0x14
> > +#define ISL1208_REG_MNT 0x15
> > +#define ISL1208_REG_HRT 0x16
> > +#define ISL1208_REG_DTT 0x17
> > +#define ISL1208_REG_MOT 0x18
> > +#define ISL1208_REG_YRT 0x19
> > +#define ISL1208_EVT_SECTION_LEN 6
> > +
> >  static struct i2c_driver isl1208_driver;
> >  
> > +/* ISL1208 various variants */
> > +enum {
> > +	TYPE_ISL1208 = 0,
> > +	TYPE_ISL1218,
> > +	TYPE_ISL1219,
> > +};
> > +
> > +struct isl1208 {
> > +	struct rtc_device *rtc;
> > +	struct device *hwmon;
> > +};
> > +
> >  /* block read */
> >  static int
> >  isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
> > @@ -80,8 +104,8 @@ isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
> >  	};
> >  	int ret;
> >  
> > -	BUG_ON(reg > ISL1208_REG_USR2);
> > -	BUG_ON(reg + len > ISL1208_REG_USR2 + 1);
> > +	WARN_ON(reg > ISL1208_REG_YRT);
> > +	WARN_ON(reg + len > ISL1208_REG_YRT + 1);
> >  
> >  	ret = i2c_transfer(client->adapter, msgs, 2);
> >  	if (ret > 0)
> > @@ -104,8 +128,8 @@ isl1208_i2c_set_regs(struct i2c_client *client, u8 reg, u8 const buf[],
> >  	};
> >  	int ret;
> >  
> > -	BUG_ON(reg > ISL1208_REG_USR2);
> > -	BUG_ON(reg + len > ISL1208_REG_USR2 + 1);
> > +	WARN_ON(reg > ISL1208_REG_YRT);
> > +	WARN_ON(reg + len > ISL1208_REG_YRT + 1);
> >  
> >  	i2c_buf[0] = reg;
> >  	memcpy(&i2c_buf[1], &buf[0], len);
> > @@ -493,12 +517,128 @@ isl1208_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> >  	return isl1208_i2c_set_alarm(to_i2c_client(dev), alarm);
> >  }
> >  
> > +static ssize_t isl1208_hwmon_show_tamper(struct device *dev,
> > +				struct device_attribute *attr, char *buf)
> > +{
> > +	struct i2c_client *client = dev_get_drvdata(dev);
> > +	int sr;
> > +
> > +	sr = isl1208_i2c_get_sr(client);
> > +	if (sr < 0) {
> > +		dev_err(dev, "%s: reading SR failed\n", __func__);
> > +		return sr;
> > +	}
> > +
> > +	if (sr & ISL1208_REG_SR_EVT)
> > +		return sprintf(buf, "1\n");
> > +
> > +	return sprintf(buf, "0\n");
> > +};
> > +
> > +static ssize_t isl1208_hwmon_clear_caseopen(struct device *dev,
> > +		struct device_attribute *attr,
> > +		const char *buf, size_t count)
> > +{
> > +	struct i2c_client *client = dev_get_drvdata(dev);
> > +	unsigned long val;
> > +	int sr;
> > +
> > +	if (kstrtoul(buf, 10, &val) || val != 0)
> > +		return -EINVAL;
> > +
> > +	sr = isl1208_i2c_get_sr(client);
> > +	if (sr < 0) {
> > +		dev_err(dev, "%s: reading SR failed\n", __func__);
> > +		return sr;
> > +	}
> > +
> > +	sr &= ~ISL1208_REG_SR_EVT;
> > +
> > +	sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr);
> > +	if (sr < 0)
> > +		dev_err(dev, "%s: writing SR failed\n",
> > +			__func__);
> > +
> > +	return count;
> > +};
> > +
> > +static ssize_t isl1208_hwmon_show_tamper_timestamp(struct device *dev,
> > +				struct device_attribute *attr, char *buf)
> > +{
> > +	struct i2c_client *client = dev_get_drvdata(dev);
> > +	u8 regs[ISL1208_EVT_SECTION_LEN] = { 0, };
> > +	struct timespec64 tv64;
> > +	struct rtc_time tm;
> > +	int sr;
> > +
> > +	sr = isl1208_i2c_get_sr(client);
> > +	if (sr < 0) {
> > +		dev_err(dev, "%s: reading SR failed\n", __func__);
> > +		return sr;
> > +	}
> > +
> > +	if (!(sr & ISL1208_REG_SR_EVT))
> > +		return 0;
> > +
> > +	sr = isl1208_i2c_read_regs(client, ISL1208_REG_SCT, regs,
> > +				   ISL1208_EVT_SECTION_LEN);
> > +	if (sr < 0) {
> > +		dev_err(dev, "%s: reading event section failed\n",
> > +			__func__);
> > +		return 0;
> > +	}
> > +
> > +	/* MSB of each alarm register is an enable bit */
> > +	tm.tm_sec = bcd2bin(regs[ISL1208_REG_SCT - ISL1208_REG_SCT] & 0x7f);
> > +	tm.tm_min = bcd2bin(regs[ISL1208_REG_MNT - ISL1208_REG_SCT] & 0x7f);
> > +	tm.tm_hour = bcd2bin(regs[ISL1208_REG_HRT - ISL1208_REG_SCT] & 0x3f);
> > +	tm.tm_mday = bcd2bin(regs[ISL1208_REG_DTT - ISL1208_REG_SCT] & 0x3f);
> > +	tm.tm_mon =
> > +		bcd2bin(regs[ISL1208_REG_MOT - ISL1208_REG_SCT] & 0x1f) - 1;
> > +	tm.tm_year = bcd2bin(regs[ISL1208_REG_YRT - ISL1208_REG_SCT]) + 100;
> > +
> > +	tv64.tv_sec = rtc_tm_to_time64(&tm);
> > +
> > +	return sprintf(buf,
> > +		"%d-%02d-%02d %02d:%02d:%02d UTC (%lld)\n",
> > +		tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
> > +		tm.tm_hour, tm.tm_min, tm.tm_sec,
> > +		(long long) tv64.tv_sec);
> > +};
> > +
> > +static SENSOR_DEVICE_ATTR(intrusion0_alarm, 0644, isl1208_hwmon_show_tamper,
> > +			isl1208_hwmon_clear_caseopen, 0);
> > +
> > +static SENSOR_DEVICE_ATTR(intrusion0_timestamp, 0444,
> > +		isl1208_hwmon_show_tamper_timestamp, NULL, 0);
> > +
> > +static struct attribute *isl1208_hwmon_attrs[] = {
> > +	&sensor_dev_attr_intrusion0_alarm.dev_attr.attr,
> > +	&sensor_dev_attr_intrusion0_timestamp.dev_attr.attr,
> > +	NULL,
> > +};
> > +ATTRIBUTE_GROUPS(isl1208_hwmon);
> > +
> > +static void isl1208_hwmon_register(struct i2c_client *client)
> > +{
> > +	struct isl1208 *isl1208 = i2c_get_clientdata(client);
> > +
> > +	isl1208->hwmon = devm_hwmon_device_register_with_groups(&client->dev,
> > +						client->name,
> > +						client, isl1208_hwmon_groups);
> > +	if (IS_ERR(isl1208->hwmon)) {
> > +		dev_warn(&client->dev,
> > +			"unable to register hwmon device %ld\n",
> > +			PTR_ERR(isl1208->hwmon));
> > +	}
> > +}
> > +
> >  static irqreturn_t
> >  isl1208_rtc_interrupt(int irq, void *data)
> >  {
> >  	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
> >  	struct i2c_client *client = data;
> > -	struct rtc_device *rtc = i2c_get_clientdata(client);
> > +	struct isl1208 *isl1208 = i2c_get_clientdata(client);
> >  	int handled = 0, sr, err;
> >  
> >  	/*
> > @@ -521,7 +661,7 @@ isl1208_rtc_interrupt(int irq, void *data)
> >  	if (sr & ISL1208_REG_SR_ALM) {
> >  		dev_dbg(&client->dev, "alarm!\n");
> >  
> > -		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
> > +		rtc_update_irq(isl1208->rtc, 1, RTC_IRQF | RTC_AF);
> >  
> >  		/* Clear the alarm */
> >  		sr &= ~ISL1208_REG_SR_ALM;
> > @@ -538,6 +678,13 @@ isl1208_rtc_interrupt(int irq, void *data)
> >  			return err;
> >  	}
> >  
> > +	if (isl1208->hwmon && (sr & ISL1208_REG_SR_EVT)) {
> > +		sysfs_notify(&isl1208->hwmon->kobj, NULL,
> > +			sensor_dev_attr_intrusion0_alarm.dev_attr.attr.name);
> > +		dev_warn(&client->dev, "event detected");
> > +		handled = 1;
> > +	}
> > +
> >  	return handled ? IRQ_HANDLED : IRQ_NONE;
> >  }
> >  
> > @@ -627,7 +774,7 @@ static int
> >  isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  {
> >  	int rc = 0;
> > -	struct rtc_device *rtc;
> > +	struct isl1208 *isl1208;
> >  
> >  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> >  		return -ENODEV;
> > @@ -635,13 +782,19 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  	if (isl1208_i2c_validate_client(client) < 0)
> >  		return -ENODEV;
> >  
> > -	rtc = devm_rtc_device_register(&client->dev, isl1208_driver.driver.name,
> > +	isl1208 = devm_kzalloc(&client->dev, sizeof(struct isl1208),
> > +				GFP_KERNEL);
> > +	if (!isl1208)
> > +		return -ENOMEM;
> > +
> > +	isl1208->rtc = devm_rtc_device_register(&client->dev,
> > +				  isl1208_driver.driver.name,
> >  				  &isl1208_rtc_ops,
> >  				  THIS_MODULE);
> > -	if (IS_ERR(rtc))
> > -		return PTR_ERR(rtc);
> > +	if (IS_ERR(isl1208->rtc))
> > +		return PTR_ERR(isl1208->rtc);
> >  
> > -	i2c_set_clientdata(client, rtc);
> > +	i2c_set_clientdata(client, isl1208);
> >  
> >  	rc = isl1208_i2c_get_sr(client);
> >  	if (rc < 0) {
> > @@ -657,6 +810,15 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  	if (rc)
> >  		return rc;
> >  
> > +	if (id->driver_data == TYPE_ISL1219) {
> > +		rc = i2c_smbus_write_byte_data(client, ISL1208_REG_09, 0x10);
> > +		if (rc < 0) {
> > +			dev_err(&client->dev, "could not enable tamper detection\n");
> > +			return rc;
> > +		}
> > +		isl1208_hwmon_register(client);
> > +	}
> > +
> >  	if (client->irq > 0) {
> >  		rc = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> >  					       isl1208_rtc_interrupt,
> > @@ -686,8 +848,9 @@ isl1208_remove(struct i2c_client *client)
> >  }
> >  
> >  static const struct i2c_device_id isl1208_id[] = {
> > -	{ "isl1208", 0 },
> > -	{ "isl1218", 0 },
> > +	{ "isl1208", TYPE_ISL1208 },
> > +	{ "isl1218", TYPE_ISL1218 },
> > +	{ "isl1219", TYPE_ISL1219 },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(i2c, isl1208_id);
> > @@ -695,6 +858,7 @@ MODULE_DEVICE_TABLE(i2c, isl1208_id);
> >  static const struct of_device_id isl1208_of_match[] = {
> >  	{ .compatible = "isil,isl1208" },
> >  	{ .compatible = "isil,isl1218" },
> > +	{ .compatible = "isil,isl1219" },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(of, isl1208_of_match);
> > -- 
> > 2.11.0
> > 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
  2018-01-24  9:03         ` Michael Grzeschik
@ 2018-01-24 12:10           ` Michael Grzeschik
       [not found]           ` <20180124090333.r5o2mzpm4q536w4r-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
  1 sibling, 0 replies; 23+ messages in thread
From: Michael Grzeschik @ 2018-01-24 12:10 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: linux-rtc, a.zummo, jdelvare, devicetree, linux-kernel,
	Denis.Osterland, alexandre.belloni, kernel

[-- Attachment #1: Type: text/plain, Size: 16022 bytes --]

On Wed, Jan 24, 2018 at 10:03:33AM +0100, Michael Grzeschik wrote:
> On Tue, Jan 23, 2018 at 10:22:54AM -0800, Guenter Roeck wrote:
> > On Tue, Jan 23, 2018 at 01:18:01PM +0100, Michael Grzeschik wrote:
> > > We add support for the ISL1219 chip that got an integrated tamper
> > > detection function. This patch implements the feature by using an hwmon
> > > interface.
> > > 
> > > The ISL1219 can also describe the timestamp of the intrusion
> > > event. For this we add the documentation of the new interface
> > > intrusion[0-*]_timestamp.
> > > 
> > > The devicetree documentation for the ISL1219 device tree
> > > binding is added with an short example.
> > > 
> > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > > Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
> > > ---
> > >  .../rtc/{intersil,isl1208.txt => isil,isl1208.txt} |  18 +-
> > >  Documentation/hwmon/sysfs-interface                |   7 +
> > >  drivers/rtc/rtc-isl1208.c                          | 190 +++++++++++++++++++--
> > >  3 files changed, 201 insertions(+), 14 deletions(-)
> > >  rename Documentation/devicetree/bindings/rtc/{intersil,isl1208.txt => isil,isl1208.txt} (57%)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> > > similarity index 57%
> > > rename from Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> > > rename to Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> > > index a54e99feae1ca..d549699e1cfc4 100644
> > > --- a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> > > +++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> > > @@ -1,14 +1,21 @@
> > > -Intersil ISL1208, ISL1218 I2C RTC/Alarm chip
> > > +Intersil ISL1208, ISL1218, ISL1219 I2C RTC/Alarm chip
> > >  
> > >  ISL1208 is a trivial I2C device (it has simple device tree bindings,
> > >  consisting of a compatible field, an address and possibly an interrupt
> > >  line).
> > >  
> > > +ISL1219 supports tamper detection user space representation through
> > > +case intrusion hwmon sensor.
> > > +ISL1219 has additional pins EVIN and #EVDET for tamper detection.
> > > +I2C devices support only one irq. #IRQ and #EVDET are open-drain active low,
> > > +so it is possible layout them to one SoC pin with pull-up.
> > > +
> > >  Required properties supported by the device:
> > >  
> > >   - "compatible": must be one of
> > >  	"isil,isl1208"
> > >  	"isil,isl1218"
> > > +	"isil,isl1219"
> > >   - "reg": I2C bus address of the device
> > >  
> > >  Optional properties:
> > > @@ -33,3 +40,12 @@ Example isl1208 node with #IRQ pin connected to SoC gpio1 pin 12:
> > >  		interrupt-parent = <&gpio1>;
> > >  		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
> > >  	};
> > > +
> > > +Example isl1219 node with #IRQ pin and #EVDET pin connected to SoC gpio1 pin 12:
> > > +
> > > +	isl1219: isl1219@68 {
> > > +		compatible = "intersil,isl1219";
> > > +		reg = <0x68>;
> > > +		interrupts-extended = <&gpio1 12 IRQ_TYPE_EDGE_FALLING>;
> > > +	};
> > > +
> > > diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> > > index fc337c317c673..a12b3c2b2a18c 100644
> > > --- a/Documentation/hwmon/sysfs-interface
> > > +++ b/Documentation/hwmon/sysfs-interface
> > > @@ -702,6 +702,13 @@ intrusion[0-*]_alarm
> > >  		the user. This is done by writing 0 to the file. Writing
> > >  		other values is unsupported.
> > >  
> > > +intrusion[0-*]_timestamp
> > > +		Chassis intrusion detection
> > > +		YYYY-MM-DD HH:MM:SS UTC (ts.sec): intrusion detected
> > > +		RO
> > > +		The corresponding timestamp on which the intrustion
> > > +		was detected.
> > > +
> > 
> > Sneaky. Nack. You don't just add attributes to the ABI because you want it,
> > without serious discussion, and much less so hidden in an RTC driver
> > (and even less as unparseable attribute).
> 
> Right; but it was not meant to be sneaky. I should have stick to my first
> thought and label this patch RFC. Sorry for that.
> 
> > In addition to that, I consider the attribute unnecessary. The intrusion
> > already generates an event which should be sufficient for all practical
> > purposes.
> 
> Would it make sense in between the other sysfs attributes of this driver?

I rethought that case and can tell that this timestamp has more value
here. The rtc is able to generate the timestamp even when the hardware
is not running and the rtc is still powered by battery. In that case it
would trigger an interrupt as soon the driver is running. For that
situation the intrusion timestamp does not correlate with the interrupt
event.

Michael

> > >  intrusion[0-*]_beep
> > >  		Chassis intrusion beep
> > >  		0: disable
> > > diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> > > index a13a4ba79004d..6e4d24614d98b 100644
> > > --- a/drivers/rtc/rtc-isl1208.c
> > > +++ b/drivers/rtc/rtc-isl1208.c
> > > @@ -14,6 +14,8 @@
> > >  #include <linux/i2c.h>
> > >  #include <linux/bcd.h>
> > >  #include <linux/rtc.h>
> > > +#include <linux/hwmon.h>
> > > +#include <linux/hwmon-sysfs.h>
> > >  
> > >  /* Register map */
> > >  /* rtc section */
> > > @@ -33,6 +35,7 @@
> > >  #define ISL1208_REG_SR_ARST    (1<<7)	/* auto reset */
> > >  #define ISL1208_REG_SR_XTOSCB  (1<<6)	/* crystal oscillator */
> > >  #define ISL1208_REG_SR_WRTC    (1<<4)	/* write rtc */
> > > +#define ISL1208_REG_SR_EVT     (1<<3)	/* event */
> > >  #define ISL1208_REG_SR_ALM     (1<<2)	/* alarm */
> > >  #define ISL1208_REG_SR_BAT     (1<<1)	/* battery */
> > >  #define ISL1208_REG_SR_RTCF    (1<<0)	/* rtc fail */
> > > @@ -57,8 +60,29 @@
> > >  #define ISL1208_REG_USR2 0x13
> > >  #define ISL1208_USR_SECTION_LEN 2
> > >  
> > > +/* event section */
> > > +#define ISL1208_REG_SCT 0x14
> > > +#define ISL1208_REG_MNT 0x15
> > > +#define ISL1208_REG_HRT 0x16
> > > +#define ISL1208_REG_DTT 0x17
> > > +#define ISL1208_REG_MOT 0x18
> > > +#define ISL1208_REG_YRT 0x19
> > > +#define ISL1208_EVT_SECTION_LEN 6
> > > +
> > >  static struct i2c_driver isl1208_driver;
> > >  
> > > +/* ISL1208 various variants */
> > > +enum {
> > > +	TYPE_ISL1208 = 0,
> > > +	TYPE_ISL1218,
> > > +	TYPE_ISL1219,
> > > +};
> > > +
> > > +struct isl1208 {
> > > +	struct rtc_device *rtc;
> > > +	struct device *hwmon;
> > > +};
> > > +
> > >  /* block read */
> > >  static int
> > >  isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
> > > @@ -80,8 +104,8 @@ isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
> > >  	};
> > >  	int ret;
> > >  
> > > -	BUG_ON(reg > ISL1208_REG_USR2);
> > > -	BUG_ON(reg + len > ISL1208_REG_USR2 + 1);
> > > +	WARN_ON(reg > ISL1208_REG_YRT);
> > > +	WARN_ON(reg + len > ISL1208_REG_YRT + 1);
> > >  
> > >  	ret = i2c_transfer(client->adapter, msgs, 2);
> > >  	if (ret > 0)
> > > @@ -104,8 +128,8 @@ isl1208_i2c_set_regs(struct i2c_client *client, u8 reg, u8 const buf[],
> > >  	};
> > >  	int ret;
> > >  
> > > -	BUG_ON(reg > ISL1208_REG_USR2);
> > > -	BUG_ON(reg + len > ISL1208_REG_USR2 + 1);
> > > +	WARN_ON(reg > ISL1208_REG_YRT);
> > > +	WARN_ON(reg + len > ISL1208_REG_YRT + 1);
> > >  
> > >  	i2c_buf[0] = reg;
> > >  	memcpy(&i2c_buf[1], &buf[0], len);
> > > @@ -493,12 +517,128 @@ isl1208_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> > >  	return isl1208_i2c_set_alarm(to_i2c_client(dev), alarm);
> > >  }
> > >  
> > > +static ssize_t isl1208_hwmon_show_tamper(struct device *dev,
> > > +				struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct i2c_client *client = dev_get_drvdata(dev);
> > > +	int sr;
> > > +
> > > +	sr = isl1208_i2c_get_sr(client);
> > > +	if (sr < 0) {
> > > +		dev_err(dev, "%s: reading SR failed\n", __func__);
> > > +		return sr;
> > > +	}
> > > +
> > > +	if (sr & ISL1208_REG_SR_EVT)
> > > +		return sprintf(buf, "1\n");
> > > +
> > > +	return sprintf(buf, "0\n");
> > > +};
> > > +
> > > +static ssize_t isl1208_hwmon_clear_caseopen(struct device *dev,
> > > +		struct device_attribute *attr,
> > > +		const char *buf, size_t count)
> > > +{
> > > +	struct i2c_client *client = dev_get_drvdata(dev);
> > > +	unsigned long val;
> > > +	int sr;
> > > +
> > > +	if (kstrtoul(buf, 10, &val) || val != 0)
> > > +		return -EINVAL;
> > > +
> > > +	sr = isl1208_i2c_get_sr(client);
> > > +	if (sr < 0) {
> > > +		dev_err(dev, "%s: reading SR failed\n", __func__);
> > > +		return sr;
> > > +	}
> > > +
> > > +	sr &= ~ISL1208_REG_SR_EVT;
> > > +
> > > +	sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr);
> > > +	if (sr < 0)
> > > +		dev_err(dev, "%s: writing SR failed\n",
> > > +			__func__);
> > > +
> > > +	return count;
> > > +};
> > > +
> > > +static ssize_t isl1208_hwmon_show_tamper_timestamp(struct device *dev,
> > > +				struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct i2c_client *client = dev_get_drvdata(dev);
> > > +	u8 regs[ISL1208_EVT_SECTION_LEN] = { 0, };
> > > +	struct timespec64 tv64;
> > > +	struct rtc_time tm;
> > > +	int sr;
> > > +
> > > +	sr = isl1208_i2c_get_sr(client);
> > > +	if (sr < 0) {
> > > +		dev_err(dev, "%s: reading SR failed\n", __func__);
> > > +		return sr;
> > > +	}
> > > +
> > > +	if (!(sr & ISL1208_REG_SR_EVT))
> > > +		return 0;
> > > +
> > > +	sr = isl1208_i2c_read_regs(client, ISL1208_REG_SCT, regs,
> > > +				   ISL1208_EVT_SECTION_LEN);
> > > +	if (sr < 0) {
> > > +		dev_err(dev, "%s: reading event section failed\n",
> > > +			__func__);
> > > +		return 0;
> > > +	}
> > > +
> > > +	/* MSB of each alarm register is an enable bit */
> > > +	tm.tm_sec = bcd2bin(regs[ISL1208_REG_SCT - ISL1208_REG_SCT] & 0x7f);
> > > +	tm.tm_min = bcd2bin(regs[ISL1208_REG_MNT - ISL1208_REG_SCT] & 0x7f);
> > > +	tm.tm_hour = bcd2bin(regs[ISL1208_REG_HRT - ISL1208_REG_SCT] & 0x3f);
> > > +	tm.tm_mday = bcd2bin(regs[ISL1208_REG_DTT - ISL1208_REG_SCT] & 0x3f);
> > > +	tm.tm_mon =
> > > +		bcd2bin(regs[ISL1208_REG_MOT - ISL1208_REG_SCT] & 0x1f) - 1;
> > > +	tm.tm_year = bcd2bin(regs[ISL1208_REG_YRT - ISL1208_REG_SCT]) + 100;
> > > +
> > > +	tv64.tv_sec = rtc_tm_to_time64(&tm);
> > > +
> > > +	return sprintf(buf,
> > > +		"%d-%02d-%02d %02d:%02d:%02d UTC (%lld)\n",
> > > +		tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
> > > +		tm.tm_hour, tm.tm_min, tm.tm_sec,
> > > +		(long long) tv64.tv_sec);
> > > +};
> > > +
> > > +static SENSOR_DEVICE_ATTR(intrusion0_alarm, 0644, isl1208_hwmon_show_tamper,
> > > +			isl1208_hwmon_clear_caseopen, 0);
> > > +
> > > +static SENSOR_DEVICE_ATTR(intrusion0_timestamp, 0444,
> > > +		isl1208_hwmon_show_tamper_timestamp, NULL, 0);
> > > +
> > > +static struct attribute *isl1208_hwmon_attrs[] = {
> > > +	&sensor_dev_attr_intrusion0_alarm.dev_attr.attr,
> > > +	&sensor_dev_attr_intrusion0_timestamp.dev_attr.attr,
> > > +	NULL,
> > > +};
> > > +ATTRIBUTE_GROUPS(isl1208_hwmon);
> > > +
> > > +static void isl1208_hwmon_register(struct i2c_client *client)
> > > +{
> > > +	struct isl1208 *isl1208 = i2c_get_clientdata(client);
> > > +
> > > +	isl1208->hwmon = devm_hwmon_device_register_with_groups(&client->dev,
> > > +						client->name,
> > > +						client, isl1208_hwmon_groups);
> > > +	if (IS_ERR(isl1208->hwmon)) {
> > > +		dev_warn(&client->dev,
> > > +			"unable to register hwmon device %ld\n",
> > > +			PTR_ERR(isl1208->hwmon));
> > > +	}
> > > +}
> > > +
> > >  static irqreturn_t
> > >  isl1208_rtc_interrupt(int irq, void *data)
> > >  {
> > >  	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
> > >  	struct i2c_client *client = data;
> > > -	struct rtc_device *rtc = i2c_get_clientdata(client);
> > > +	struct isl1208 *isl1208 = i2c_get_clientdata(client);
> > >  	int handled = 0, sr, err;
> > >  
> > >  	/*
> > > @@ -521,7 +661,7 @@ isl1208_rtc_interrupt(int irq, void *data)
> > >  	if (sr & ISL1208_REG_SR_ALM) {
> > >  		dev_dbg(&client->dev, "alarm!\n");
> > >  
> > > -		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
> > > +		rtc_update_irq(isl1208->rtc, 1, RTC_IRQF | RTC_AF);
> > >  
> > >  		/* Clear the alarm */
> > >  		sr &= ~ISL1208_REG_SR_ALM;
> > > @@ -538,6 +678,13 @@ isl1208_rtc_interrupt(int irq, void *data)
> > >  			return err;
> > >  	}
> > >  
> > > +	if (isl1208->hwmon && (sr & ISL1208_REG_SR_EVT)) {
> > > +		sysfs_notify(&isl1208->hwmon->kobj, NULL,
> > > +			sensor_dev_attr_intrusion0_alarm.dev_attr.attr.name);
> > > +		dev_warn(&client->dev, "event detected");
> > > +		handled = 1;
> > > +	}
> > > +
> > >  	return handled ? IRQ_HANDLED : IRQ_NONE;
> > >  }
> > >  
> > > @@ -627,7 +774,7 @@ static int
> > >  isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > >  {
> > >  	int rc = 0;
> > > -	struct rtc_device *rtc;
> > > +	struct isl1208 *isl1208;
> > >  
> > >  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > >  		return -ENODEV;
> > > @@ -635,13 +782,19 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > >  	if (isl1208_i2c_validate_client(client) < 0)
> > >  		return -ENODEV;
> > >  
> > > -	rtc = devm_rtc_device_register(&client->dev, isl1208_driver.driver.name,
> > > +	isl1208 = devm_kzalloc(&client->dev, sizeof(struct isl1208),
> > > +				GFP_KERNEL);
> > > +	if (!isl1208)
> > > +		return -ENOMEM;
> > > +
> > > +	isl1208->rtc = devm_rtc_device_register(&client->dev,
> > > +				  isl1208_driver.driver.name,
> > >  				  &isl1208_rtc_ops,
> > >  				  THIS_MODULE);
> > > -	if (IS_ERR(rtc))
> > > -		return PTR_ERR(rtc);
> > > +	if (IS_ERR(isl1208->rtc))
> > > +		return PTR_ERR(isl1208->rtc);
> > >  
> > > -	i2c_set_clientdata(client, rtc);
> > > +	i2c_set_clientdata(client, isl1208);
> > >  
> > >  	rc = isl1208_i2c_get_sr(client);
> > >  	if (rc < 0) {
> > > @@ -657,6 +810,15 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > >  	if (rc)
> > >  		return rc;
> > >  
> > > +	if (id->driver_data == TYPE_ISL1219) {
> > > +		rc = i2c_smbus_write_byte_data(client, ISL1208_REG_09, 0x10);
> > > +		if (rc < 0) {
> > > +			dev_err(&client->dev, "could not enable tamper detection\n");
> > > +			return rc;
> > > +		}
> > > +		isl1208_hwmon_register(client);
> > > +	}
> > > +
> > >  	if (client->irq > 0) {
> > >  		rc = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> > >  					       isl1208_rtc_interrupt,
> > > @@ -686,8 +848,9 @@ isl1208_remove(struct i2c_client *client)
> > >  }
> > >  
> > >  static const struct i2c_device_id isl1208_id[] = {
> > > -	{ "isl1208", 0 },
> > > -	{ "isl1218", 0 },
> > > +	{ "isl1208", TYPE_ISL1208 },
> > > +	{ "isl1218", TYPE_ISL1218 },
> > > +	{ "isl1219", TYPE_ISL1219 },
> > >  	{ }
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, isl1208_id);
> > > @@ -695,6 +858,7 @@ MODULE_DEVICE_TABLE(i2c, isl1208_id);
> > >  static const struct of_device_id isl1208_of_match[] = {
> > >  	{ .compatible = "isil,isl1208" },
> > >  	{ .compatible = "isil,isl1218" },
> > > +	{ .compatible = "isil,isl1219" },
> > >  	{ }
> > >  };
> > >  MODULE_DEVICE_TABLE(of, isl1208_of_match);
> > > -- 
> > > 2.11.0
> > > 
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
       [not found]           ` <20180124090333.r5o2mzpm4q536w4r-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2018-01-29 21:59             ` Guenter Roeck
  2018-01-30 10:27               ` Alexandre Belloni
  0 siblings, 1 reply; 23+ messages in thread
From: Guenter Roeck @ 2018-01-29 21:59 UTC (permalink / raw)
  To: Michael Grzeschik
  Cc: a.zummo-BfzFCNDTiLLj+vYz1yj4TQ,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	linux-rtc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, jdelvare-IBi9RG/b67k,
	kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
	Denis.Osterland-85mDkmTllUAAvxtiuMwx3w

On Wed, Jan 24, 2018 at 10:03:33AM +0100, Michael Grzeschik wrote:
[ ... ]
> > > +
> > > diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> > > index fc337c317c673..a12b3c2b2a18c 100644
> > > --- a/Documentation/hwmon/sysfs-interface
> > > +++ b/Documentation/hwmon/sysfs-interface
> > > @@ -702,6 +702,13 @@ intrusion[0-*]_alarm
> > >  		the user. This is done by writing 0 to the file. Writing
> > >  		other values is unsupported.
> > >  
> > > +intrusion[0-*]_timestamp
> > > +		Chassis intrusion detection
> > > +		YYYY-MM-DD HH:MM:SS UTC (ts.sec): intrusion detected
> > > +		RO
> > > +		The corresponding timestamp on which the intrustion
> > > +		was detected.
> > > +
> > 
> > Sneaky. Nack. You don't just add attributes to the ABI because you want it,
> > without serious discussion, and much less so hidden in an RTC driver
> > (and even less as unparseable attribute).
> 
> Right; but it was not meant to be sneaky. I should have stick to my first
> thought and label this patch RFC. Sorry for that.
> 
> > In addition to that, I consider the attribute unnecessary. The intrusion
> > already generates an event which should be sufficient for all practical
> > purposes.
> 
> Would it make sense in between the other sysfs attributes of this driver?
> 
I don't understand what you mean with that, sorry.

>From an ABI perspective, the attibute doesn't add value since it is
highly device specific (or at least it is the only chip I am aware of
which reports such a time stamp). Feel free to add the attribute to the
driver and document it, but not as part of the hwmon ABI. In that
case I would be inclined to accept it. However, keep in mind that
your version, reporting a human readable date/time, would effectively
preclude it from ever making it into the ABI.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/4] rtc: isl1208: Add device tree binding documentation
  2018-01-23 12:17   ` [PATCH 2/4] rtc: isl1208: Add device tree binding documentation Michael Grzeschik
@ 2018-01-29 23:34     ` Rob Herring
  2018-01-30 10:06     ` Alexandre Belloni
  1 sibling, 0 replies; 23+ messages in thread
From: Rob Herring @ 2018-01-29 23:34 UTC (permalink / raw)
  To: Michael Grzeschik
  Cc: a.zummo, alexandre.belloni, linux-rtc, linux-kernel, devicetree,
	linux, jdelvare, kernel, Denis.Osterland

On Tue, Jan 23, 2018 at 01:17:59PM +0100, Michael Grzeschik wrote:
> From: Denis Osterland <Denis.Osterland@diehl.com>
> 
> Wrote documentation for ISL1208, ISL1218 device tree
> binding with short examples.
> 
> Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  .../devicetree/bindings/rtc/intersil,isl1208.txt   | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl1208.txt

A couple of nits, otherwise:

Reviewed-by: Rob Herring <robh@kernel.org>

> 
> diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt b/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> new file mode 100644
> index 0000000000000..a54e99feae1ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> @@ -0,0 +1,35 @@
> +Intersil ISL1208, ISL1218 I2C RTC/Alarm chip
> +
> +ISL1208 is a trivial I2C device (it has simple device tree bindings,
> +consisting of a compatible field, an address and possibly an interrupt
> +line).
> +
> +Required properties supported by the device:
> +
> + - "compatible": must be one of
> +	"isil,isl1208"
> +	"isil,isl1218"
> + - "reg": I2C bus address of the device
> +
> +Optional properties:
> +
> + - "interrupt-parent", "interrupts", "interrupts-extended":
> +   for passing the interrupt line of the SoC connected to #IRQ pin
> +   of the RTC chip.
> +
> +
> +Example isl1208 node without #IRQ pin connected:
> +
> +	isl1208: isl1208@68 {

rtc@68

> +		compatible = "isil,isl1208";
> +		reg = <0x68>;
> +	};
> +
> +Example isl1208 node with #IRQ pin connected to SoC gpio1 pin 12:
> +
> +	isl1208: isl1208@68 {

rtc@68

> +		compatible = "isil,isl1208";
> +		reg = <0x68>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
> +	};
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
       [not found]     ` <20180123121801.4214-5-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
  2018-01-23 18:22       ` Guenter Roeck
@ 2018-01-29 23:41       ` Rob Herring
  2018-01-30  8:56         ` Denis OSTERLAND
  1 sibling, 1 reply; 23+ messages in thread
From: Rob Herring @ 2018-01-29 23:41 UTC (permalink / raw)
  To: Michael Grzeschik
  Cc: a.zummo-BfzFCNDTiLLj+vYz1yj4TQ,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	linux-rtc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, linux-0h96xk9xTtrk1uMJSBkQmQ,
	jdelvare-IBi9RG/b67k, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
	Denis.Osterland-85mDkmTllUAAvxtiuMwx3w

On Tue, Jan 23, 2018 at 01:18:01PM +0100, Michael Grzeschik wrote:
> We add support for the ISL1219 chip that got an integrated tamper
> detection function. This patch implements the feature by using an hwmon
> interface.
> 
> The ISL1219 can also describe the timestamp of the intrusion
> event. For this we add the documentation of the new interface
> intrusion[0-*]_timestamp.
> 
> The devicetree documentation for the ISL1219 device tree
> binding is added with an short example.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Denis Osterland <Denis.Osterland-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
> ---
>  .../rtc/{intersil,isl1208.txt => isil,isl1208.txt} |  18 +-
>  Documentation/hwmon/sysfs-interface                |   7 +
>  drivers/rtc/rtc-isl1208.c                          | 190 +++++++++++++++++++--
>  3 files changed, 201 insertions(+), 14 deletions(-)
>  rename Documentation/devicetree/bindings/rtc/{intersil,isl1208.txt => isil,isl1208.txt} (57%)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> similarity index 57%
> rename from Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> rename to Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> index a54e99feae1ca..d549699e1cfc4 100644
> --- a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> +++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> @@ -1,14 +1,21 @@
> -Intersil ISL1208, ISL1218 I2C RTC/Alarm chip
> +Intersil ISL1208, ISL1218, ISL1219 I2C RTC/Alarm chip
>  
>  ISL1208 is a trivial I2C device (it has simple device tree bindings,
>  consisting of a compatible field, an address and possibly an interrupt
>  line).
>  
> +ISL1219 supports tamper detection user space representation through
> +case intrusion hwmon sensor.

User space and hwmon are Linux details not relevant to the binding. Just 
describe the h/w.

> +ISL1219 has additional pins EVIN and #EVDET for tamper detection.
> +I2C devices support only one irq. #IRQ and #EVDET are open-drain active low,
> +so it is possible layout them to one SoC pin with pull-up.
> +
>  Required properties supported by the device:
>  
>   - "compatible": must be one of
>  	"isil,isl1208"
>  	"isil,isl1218"
> +	"isil,isl1219"
>   - "reg": I2C bus address of the device
>  
>  Optional properties:
> @@ -33,3 +40,12 @@ Example isl1208 node with #IRQ pin connected to SoC gpio1 pin 12:
>  		interrupt-parent = <&gpio1>;
>  		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
>  	};
> +
> +Example isl1219 node with #IRQ pin and #EVDET pin connected to SoC gpio1 pin 12:
> +
> +	isl1219: isl1219@68 {
> +		compatible = "intersil,isl1219";
> +		reg = <0x68>;
> +		interrupts-extended = <&gpio1 12 IRQ_TYPE_EDGE_FALLING>;

With 2 interrupts, you should have 2 values. If they are connected 
together, just repeat the connection. Otherwise, you can't tell if EVDET 
is a no connect.

There's not much point in having an example for every compatible. This 
binding is simple enough, one should be enough.

> +	};
> +
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
  2018-01-29 23:41       ` Rob Herring
@ 2018-01-30  8:56         ` Denis OSTERLAND
  2018-01-30 14:41           ` Rob Herring
  0 siblings, 1 reply; 23+ messages in thread
From: Denis OSTERLAND @ 2018-01-30  8:56 UTC (permalink / raw)
  To: robh, m.grzeschik
  Cc: linux-kernel, devicetree, linux, a.zummo, jdelvare,
	alexandre.belloni, linux-rtc, kernel

Am Montag, den 29.01.2018, 17:41 -0600 schrieb Rob Herring:
> On Tue, Jan 23, 2018 at 01:18:01PM +0100, Michael Grzeschik wrote:
> > 
> > We add support for the ISL1219 chip that got an integrated tamper
> > detection function. This patch implements the feature by using an hwmon
> > interface.
> > 
> > The ISL1219 can also describe the timestamp of the intrusion
> > event. For this we add the documentation of the new interface
> > intrusion[0-*]_timestamp.
> > 
> > The devicetree documentation for the ISL1219 device tree
> > binding is added with an short example.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
> > ---
> >  .../rtc/{intersil,isl1208.txt => isil,isl1208.txt} |  18 +-
> >  Documentation/hwmon/sysfs-interface                |   7 +
> >  drivers/rtc/rtc-isl1208.c                          | 190 +++++++++++++++++++--
> >  3 files changed, 201 insertions(+), 14 deletions(-)
> >  rename Documentation/devicetree/bindings/rtc/{intersil,isl1208.txt => isil,isl1208.txt} (57%)
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> > similarity index 57%
> > rename from Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> > rename to Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> > index a54e99feae1ca..d549699e1cfc4 100644
> > --- a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> > +++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
> > @@ -1,14 +1,21 @@
> > -Intersil ISL1208, ISL1218 I2C RTC/Alarm chip
> > +Intersil ISL1208, ISL1218, ISL1219 I2C RTC/Alarm chip
> >  
> >  ISL1208 is a trivial I2C device (it has simple device tree bindings,
> >  consisting of a compatible field, an address and possibly an interrupt
> >  line).
> >  
> > +ISL1219 supports tamper detection user space representation through
> > +case intrusion hwmon sensor.
> User space and hwmon are Linux details not relevant to the binding. Just 
> describe the h/w.
OK.
> 
> > 
> > +ISL1219 has additional pins EVIN and #EVDET for tamper detection.
> > +I2C devices support only one irq. #IRQ and #EVDET are open-drain active low,
> > +so it is possible layout them to one SoC pin with pull-up.
> > +
> >  Required properties supported by the device:
> >  
> >   - "compatible": must be one of
> >  	"isil,isl1208"
> >  	"isil,isl1218"
> > +	"isil,isl1219"
> >   - "reg": I2C bus address of the device
> >  
> >  Optional properties:
> > @@ -33,3 +40,12 @@ Example isl1208 node with #IRQ pin connected to SoC gpio1 pin 12:
> >  		interrupt-parent = <&gpio1>;
> >  		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
> >  	};
> > +
> > +Example isl1219 node with #IRQ pin and #EVDET pin connected to SoC gpio1 pin 12:
> > +
> > +	isl1219: isl1219@68 {
> > +		compatible = "intersil,isl1219";
> > +		reg = <0x68>;
> > +		interrupts-extended = <&gpio1 12 IRQ_TYPE_EDGE_FALLING>;
> With 2 interrupts, you should have 2 values. If they are connected 
> together, just repeat the connection. Otherwise, you can't tell if EVDET 
> is a no connect.
If I got you right, you suggest an additional IRQ entry to parse.
A short example, #IRQ pin connected to gpio1 pin 12 and
#EVDET pin connected to gpio2 pin 24:

isl1219: rtc@68 {
	compatible = "intersil,isl1219";
	reg = <0x68>;
	interrupt-names = "irq", "evdet";
	interrupts-extended = <&gpio1 12 IRQ_TYPE_EDGE_FALLING>,
		<&gpio2 24 IRQ_TYPE_EDGE_FALLING>;
};

In driver implementation we need only one interrupt, because we can
determinate action to take based on value of status register.
In current implementation there was no need to do some additional
OF parsing, everything is done by I2C generic code.
I guess, it is not much additional work to do so, but I am not sure
if it´s worthwhile.
> 
> There's not much point in having an example for every compatible. This 
> binding is simple enough, one should be enough.
Shell we remove the example without an interrupt?
> 
> > 
> > +	};
> > +
Diehl AKO Stiftung & Co. KG, Pfannerstraße 75-83, 88239 Wangen im Allgäu
Bereichsvorstand: Dr.-Ing. Michael Siedentop (Sprecher), Josef Fellner (Mitglied)
Sitz der Gesellschaft: Wangen i.A. – Registergericht: Amtsgericht Ulm HRA 620609 – Persönlich haftende Gesellschafterin: Diehl Verwaltungs-Stiftung – Sitz: Nürnberg – Registergericht: Amtsgericht Nürnberg HRA 11756 –
Vorstand: Dr.-Ing. E.h. Thomas Diehl (†) (Vorsitzender), Herr Dipl.-Wirtsch.-Ing. Wolfgang Weggen (stellvertretender Vorsitzender), Dipl.-Kfm. Claus Günther, Dipl.-Kfm. Frank Gutzeit, Dr.-Ing. Heinrich Schunk, Dr.-Ing. Michael Siedentop , Dipl.-Kfm. Dr.-Ing. Martin Sommer, Dipl.-Ing. (FH) Rainer von Borstel, Vorsitzender des Aufsichtsrates: Dr. Klaus Maier
___________________________________________________________________________________________________
Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht. Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/4] rtc: isl1208: Add device tree binding documentation
  2018-01-23 12:17   ` [PATCH 2/4] rtc: isl1208: Add device tree binding documentation Michael Grzeschik
  2018-01-29 23:34     ` Rob Herring
@ 2018-01-30 10:06     ` Alexandre Belloni
  1 sibling, 0 replies; 23+ messages in thread
From: Alexandre Belloni @ 2018-01-30 10:06 UTC (permalink / raw)
  To: Michael Grzeschik
  Cc: a.zummo, linux-rtc, linux-kernel, devicetree, linux, jdelvare,
	kernel, Denis.Osterland

Hi,

On 23/01/2018 at 13:17:59 +0100, Michael Grzeschik wrote:
> From: Denis Osterland <Denis.Osterland@diehl.com>
> 
> Wrote documentation for ISL1208, ISL1218 device tree
> binding with short examples.
> 

This binding is already in
Documentation/devicetree/bindings/trivial-devices.txt, no need to
duplicate.

> Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  .../devicetree/bindings/rtc/intersil,isl1208.txt   | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt b/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> new file mode 100644
> index 0000000000000..a54e99feae1ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
> @@ -0,0 +1,35 @@
> +Intersil ISL1208, ISL1218 I2C RTC/Alarm chip
> +
> +ISL1208 is a trivial I2C device (it has simple device tree bindings,
> +consisting of a compatible field, an address and possibly an interrupt
> +line).
> +
> +Required properties supported by the device:
> +
> + - "compatible": must be one of
> +	"isil,isl1208"
> +	"isil,isl1218"
> + - "reg": I2C bus address of the device
> +
> +Optional properties:
> +
> + - "interrupt-parent", "interrupts", "interrupts-extended":
> +   for passing the interrupt line of the SoC connected to #IRQ pin
> +   of the RTC chip.
> +
> +
> +Example isl1208 node without #IRQ pin connected:
> +
> +	isl1208: isl1208@68 {
> +		compatible = "isil,isl1208";
> +		reg = <0x68>;
> +	};
> +
> +Example isl1208 node with #IRQ pin connected to SoC gpio1 pin 12:
> +
> +	isl1208: isl1208@68 {
> +		compatible = "isil,isl1208";
> +		reg = <0x68>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
> +	};
> -- 
> 2.11.0
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
  2018-01-29 21:59             ` Guenter Roeck
@ 2018-01-30 10:27               ` Alexandre Belloni
       [not found]                 ` <20180130102740.GD2809-m++hUPXGwpdeoWH0uzbU5w@public.gmane.org>
  0 siblings, 1 reply; 23+ messages in thread
From: Alexandre Belloni @ 2018-01-30 10:27 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Michael Grzeschik, a.zummo, linux-rtc, linux-kernel, devicetree,
	jdelvare, kernel, Denis.Osterland

On 29/01/2018 at 13:59:19 -0800, Guenter Roeck wrote:
> On Wed, Jan 24, 2018 at 10:03:33AM +0100, Michael Grzeschik wrote:
> [ ... ]
> > > > +
> > > > diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> > > > index fc337c317c673..a12b3c2b2a18c 100644
> > > > --- a/Documentation/hwmon/sysfs-interface
> > > > +++ b/Documentation/hwmon/sysfs-interface
> > > > @@ -702,6 +702,13 @@ intrusion[0-*]_alarm
> > > >  		the user. This is done by writing 0 to the file. Writing
> > > >  		other values is unsupported.
> > > >  
> > > > +intrusion[0-*]_timestamp
> > > > +		Chassis intrusion detection
> > > > +		YYYY-MM-DD HH:MM:SS UTC (ts.sec): intrusion detected
> > > > +		RO
> > > > +		The corresponding timestamp on which the intrustion
> > > > +		was detected.
> > > > +
> > > 
> > > Sneaky. Nack. You don't just add attributes to the ABI because you want it,
> > > without serious discussion, and much less so hidden in an RTC driver
> > > (and even less as unparseable attribute).
> > 
> > Right; but it was not meant to be sneaky. I should have stick to my first
> > thought and label this patch RFC. Sorry for that.
> > 
> > > In addition to that, I consider the attribute unnecessary. The intrusion
> > > already generates an event which should be sufficient for all practical
> > > purposes.
> > 
> > Would it make sense in between the other sysfs attributes of this driver?
> > 
> I don't understand what you mean with that, sorry.
> 
> From an ABI perspective, the attibute doesn't add value since it is
> highly device specific (or at least it is the only chip I am aware of
> which reports such a time stamp). Feel free to add the attribute to the
> driver and document it, but not as part of the hwmon ABI. In that
> case I would be inclined to accept it. However, keep in mind that
> your version, reporting a human readable date/time, would effectively
> preclude it from ever making it into the ABI.
> 

Actually, there are many RTCs that are able to register one or more
timestamps. My plan was to add support for that soon but I was not
planning to do so in the hwmon ABI as this may be used for something
that is not intrusion detection (interval timers for example).


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/4] rtc: isl1208: enable interrupt after context preparation
  2018-01-23 12:18 ` [PATCH 3/4] rtc: isl1208: enable interrupt after context preparation Michael Grzeschik
@ 2018-01-30 10:34   ` Alexandre Belloni
  0 siblings, 0 replies; 23+ messages in thread
From: Alexandre Belloni @ 2018-01-30 10:34 UTC (permalink / raw)
  To: Michael Grzeschik
  Cc: a.zummo, linux-rtc, linux-kernel, devicetree, linux, jdelvare,
	kernel, Denis.Osterland

Hi,

On 23/01/2018 at 13:18:00 +0100, Michael Grzeschik wrote:
> The interrupt handler got enabled very early. If the interrupt cause is
> triggering immediately before the context is fully prepared. This can
> lead to undefined behaviour. Therefor we move the interrupt enable code
> to the end of the probe function.
> 

Can you fix it using devm_rtc_allocate_device()/rtc_register_device() as
done in https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git/commit/?h=rtc-next&id=994ec64c0a193940be7a6fd074668b9446d3b6c3

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
> ---
>  drivers/rtc/rtc-isl1208.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> index c8b4953482296..a13a4ba79004d 100644
> --- a/drivers/rtc/rtc-isl1208.c
> +++ b/drivers/rtc/rtc-isl1208.c
> @@ -635,23 +635,6 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	if (isl1208_i2c_validate_client(client) < 0)
>  		return -ENODEV;
>  
> -	if (client->irq > 0) {
> -		rc = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> -					       isl1208_rtc_interrupt,
> -					       IRQF_SHARED | IRQF_ONESHOT,
> -					       isl1208_driver.driver.name,
> -					       client);
> -		if (!rc) {
> -			device_init_wakeup(&client->dev, 1);
> -			enable_irq_wake(client->irq);
> -		} else {
> -			dev_err(&client->dev,
> -				"Unable to request irq %d, no alarm support\n",
> -				client->irq);
> -			client->irq = 0;
> -		}
> -	}
> -
>  	rtc = devm_rtc_device_register(&client->dev, isl1208_driver.driver.name,
>  				  &isl1208_rtc_ops,
>  				  THIS_MODULE);
> @@ -674,6 +657,23 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	if (rc)
>  		return rc;
>  
> +	if (client->irq > 0) {
> +		rc = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					       isl1208_rtc_interrupt,
> +					       IRQF_SHARED | IRQF_ONESHOT,
> +					       isl1208_driver.driver.name,
> +					       client);
> +		if (!rc) {
> +			device_init_wakeup(&client->dev, 1);
> +			enable_irq_wake(client->irq);
> +		} else {
> +			dev_err(&client->dev,
> +				"Unable to request irq %d, no alarm support\n",
> +				client->irq);
> +			client->irq = 0;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.11.0
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
       [not found]                 ` <20180130102740.GD2809-m++hUPXGwpdeoWH0uzbU5w@public.gmane.org>
@ 2018-01-30 11:40                   ` Denis OSTERLAND
       [not found]                     ` <1517312409.5307.22.camel-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 23+ messages in thread
From: Denis OSTERLAND @ 2018-01-30 11:40 UTC (permalink / raw)
  To: linux-0h96xk9xTtrk1uMJSBkQmQ,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: linux-rtc-u79uwXL29TY76Z2rM5mHXA, a.zummo-BfzFCNDTiLLj+vYz1yj4TQ,
	kernel-bIcnvbaLZ9MEGnE8C9+IrQ, mgr-bIcnvbaLZ9MEGnE8C9+IrQ,
	jdelvare-IBi9RG/b67k, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

Am Dienstag, den 30.01.2018, 11:27 +0100 schrieb Alexandre Belloni:
> On 29/01/2018 at 13:59:19 -0800, Guenter Roeck wrote:
> > 
> > On Wed, Jan 24, 2018 at 10:03:33AM +0100, Michael Grzeschik wrote:
> > [ ... ]
> > > 
> > > > 
> > > > > 
> > > > > +
> > > > > diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> > > > > index fc337c317c673..a12b3c2b2a18c 100644
> > > > > --- a/Documentation/hwmon/sysfs-interface
> > > > > +++ b/Documentation/hwmon/sysfs-interface
> > > > > @@ -702,6 +702,13 @@ intrusion[0-*]_alarm
> > > > >  		the user. This is done by writing 0 to the file. Writing
> > > > >  		other values is unsupported.
> > > > >  
> > > > > +intrusion[0-*]_timestamp
> > > > > +		Chassis intrusion detection
> > > > > +		YYYY-MM-DD HH:MM:SS UTC (ts.sec): intrusion detected
> > > > > +		RO
> > > > > +		The corresponding timestamp on which the intrustion
> > > > > +		was detected.
> > > > > +
> > > > Sneaky. Nack. You don't just add attributes to the ABI because you want it,
> > > > without serious discussion, and much less so hidden in an RTC driver
> > > > (and even less as unparseable attribute).
> > > Right; but it was not meant to be sneaky. I should have stick to my first
> > > thought and label this patch RFC. Sorry for that.
> > > 
> > > > 
> > > > In addition to that, I consider the attribute unnecessary. The intrusion
> > > > already generates an event which should be sufficient for all practical
> > > > purposes.
> > > Would it make sense in between the other sysfs attributes of this driver?
> > > 
> > I don't understand what you mean with that, sorry.
> > 
> > From an ABI perspective, the attibute doesn't add value since it is
> > highly device specific (or at least it is the only chip I am aware of
> > which reports such a time stamp). Feel free to add the attribute to the
> > driver and document it, but not as part of the hwmon ABI. In that
> > case I would be inclined to accept it. However, keep in mind that
> > your version, reporting a human readable date/time, would effectively
> > preclude it from ever making it into the ABI.
> > 
> Actually, there are many RTCs that are able to register one or more
> timestamps. My plan was to add support for that soon but I was not
> planning to do so in the hwmon ABI as this may be used for something
> that is not intrusion detection (interval timers for example).
What would you suggest?
I think about something like this:
event[0-*]_timestamp: timestamp in seconds since epoch or empty if not triggered
event[0-*]_alarm: 1 if event was triggered, else 0; write 0 to clear event
> 
> 
Diehl AKO Stiftung & Co. KG, Pfannerstraße 75-83, 88239 Wangen im Allgäu
Bereichsvorstand: Dr.-Ing. Michael Siedentop (Sprecher), Josef Fellner (Mitglied)
Sitz der Gesellschaft: Wangen i.A. – Registergericht: Amtsgericht Ulm HRA 620609 – Persönlich haftende Gesellschafterin: Diehl Verwaltungs-Stiftung – Sitz: Nürnberg – Registergericht: Amtsgericht Nürnberg HRA 11756 –
Vorstand: Dr.-Ing. E.h. Thomas Diehl (†) (Vorsitzender), Herr Dipl.-Wirtsch.-Ing. Wolfgang Weggen (stellvertretender Vorsitzender), Dipl.-Kfm. Claus Günther, Dipl.-Kfm. Frank Gutzeit, Dr.-Ing. Heinrich Schunk, Dr.-Ing. Michael Siedentop , Dipl.-Kfm. Dr.-Ing. Martin Sommer, Dipl.-Ing. (FH) Rainer von Borstel, Vorsitzender des Aufsichtsrates: Dr. Klaus Maier
___________________________________________________________________________________________________
Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht. Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
       [not found]                     ` <1517312409.5307.22.camel-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
@ 2018-01-30 14:15                       ` Guenter Roeck
  2018-01-31 10:54                         ` Alexandre Belloni
  0 siblings, 1 reply; 23+ messages in thread
From: Guenter Roeck @ 2018-01-30 14:15 UTC (permalink / raw)
  To: Denis OSTERLAND, alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: linux-rtc-u79uwXL29TY76Z2rM5mHXA, a.zummo-BfzFCNDTiLLj+vYz1yj4TQ,
	kernel-bIcnvbaLZ9MEGnE8C9+IrQ, mgr-bIcnvbaLZ9MEGnE8C9+IrQ,
	jdelvare-IBi9RG/b67k, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 01/30/2018 03:40 AM, Denis OSTERLAND wrote:
> Am Dienstag, den 30.01.2018, 11:27 +0100 schrieb Alexandre Belloni:
>> On 29/01/2018 at 13:59:19 -0800, Guenter Roeck wrote:
>>>
>>> On Wed, Jan 24, 2018 at 10:03:33AM +0100, Michael Grzeschik wrote:
>>> [ ... ]
>>>>
>>>>>
>>>>>>
>>>>>> +
>>>>>> diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
>>>>>> index fc337c317c673..a12b3c2b2a18c 100644
>>>>>> --- a/Documentation/hwmon/sysfs-interface
>>>>>> +++ b/Documentation/hwmon/sysfs-interface
>>>>>> @@ -702,6 +702,13 @@ intrusion[0-*]_alarm
>>>>>>   		the user. This is done by writing 0 to the file. Writing
>>>>>>   		other values is unsupported.
>>>>>>   
>>>>>> +intrusion[0-*]_timestamp
>>>>>> +		Chassis intrusion detection
>>>>>> +		YYYY-MM-DD HH:MM:SS UTC (ts.sec): intrusion detected
>>>>>> +		RO
>>>>>> +		The corresponding timestamp on which the intrustion
>>>>>> +		was detected.
>>>>>> +
>>>>> Sneaky. Nack. You don't just add attributes to the ABI because you want it,
>>>>> without serious discussion, and much less so hidden in an RTC driver
>>>>> (and even less as unparseable attribute).
>>>> Right; but it was not meant to be sneaky. I should have stick to my first
>>>> thought and label this patch RFC. Sorry for that.
>>>>
>>>>>
>>>>> In addition to that, I consider the attribute unnecessary. The intrusion
>>>>> already generates an event which should be sufficient for all practical
>>>>> purposes.
>>>> Would it make sense in between the other sysfs attributes of this driver?
>>>>
>>> I don't understand what you mean with that, sorry.
>>>
>>>  From an ABI perspective, the attibute doesn't add value since it is
>>> highly device specific (or at least it is the only chip I am aware of
>>> which reports such a time stamp). Feel free to add the attribute to the
>>> driver and document it, but not as part of the hwmon ABI. In that
>>> case I would be inclined to accept it. However, keep in mind that
>>> your version, reporting a human readable date/time, would effectively
>>> preclude it from ever making it into the ABI.
>>>
>> Actually, there are many RTCs that are able to register one or more
>> timestamps. My plan was to add support for that soon but I was not
>> planning to do so in the hwmon ABI as this may be used for something
>> that is not intrusion detection (interval timers for example).
> What would you suggest?
> I think about something like this:
> event[0-*]_timestamp: timestamp in seconds since epoch or empty if not triggered
> event[0-*]_alarm: 1 if event was triggered, else 0; write 0 to clear event

Sure, that makes sense if the events are not specifically related
to intrusion detection. Question is if there would ever be more than one
or if event_timestamp and event_alarm would be sufficient.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
  2018-01-30  8:56         ` Denis OSTERLAND
@ 2018-01-30 14:41           ` Rob Herring
       [not found]             ` <CAL_JsqKEXtL0hvEgFALKaCgW2Go==wj14_dDomdK_hPU23oXLQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 23+ messages in thread
From: Rob Herring @ 2018-01-30 14:41 UTC (permalink / raw)
  To: Denis OSTERLAND
  Cc: m.grzeschik, linux-kernel, devicetree, linux, a.zummo, jdelvare,
	alexandre.belloni, linux-rtc, kernel

On Tue, Jan 30, 2018 at 2:56 AM, Denis OSTERLAND
<denis.osterland@diehl.com> wrote:
> Am Montag, den 29.01.2018, 17:41 -0600 schrieb Rob Herring:
>> On Tue, Jan 23, 2018 at 01:18:01PM +0100, Michael Grzeschik wrote:
>> >
>> > We add support for the ISL1219 chip that got an integrated tamper
>> > detection function. This patch implements the feature by using an hwmon
>> > interface.
>> >
>> > The ISL1219 can also describe the timestamp of the intrusion
>> > event. For this we add the documentation of the new interface
>> > intrusion[0-*]_timestamp.
>> >
>> > The devicetree documentation for the ISL1219 device tree
>> > binding is added with an short example.
>> >
>> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> > Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
>> > ---
>> >  .../rtc/{intersil,isl1208.txt => isil,isl1208.txt} |  18 +-
>> >  Documentation/hwmon/sysfs-interface                |   7 +
>> >  drivers/rtc/rtc-isl1208.c                          | 190 +++++++++++++++++++--
>> >  3 files changed, 201 insertions(+), 14 deletions(-)
>> >  rename Documentation/devicetree/bindings/rtc/{intersil,isl1208.txt => isil,isl1208.txt} (57%)
>> >
>> > diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
>> > similarity index 57%
>> > rename from Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
>> > rename to Documentation/devicetree/bindings/rtc/isil,isl1208.txt
>> > index a54e99feae1ca..d549699e1cfc4 100644
>> > --- a/Documentation/devicetree/bindings/rtc/intersil,isl1208.txt
>> > +++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
>> > @@ -1,14 +1,21 @@
>> > -Intersil ISL1208, ISL1218 I2C RTC/Alarm chip
>> > +Intersil ISL1208, ISL1218, ISL1219 I2C RTC/Alarm chip
>> >
>> >  ISL1208 is a trivial I2C device (it has simple device tree bindings,
>> >  consisting of a compatible field, an address and possibly an interrupt
>> >  line).
>> >
>> > +ISL1219 supports tamper detection user space representation through
>> > +case intrusion hwmon sensor.
>> User space and hwmon are Linux details not relevant to the binding. Just
>> describe the h/w.
> OK.
>>
>> >
>> > +ISL1219 has additional pins EVIN and #EVDET for tamper detection.
>> > +I2C devices support only one irq. #IRQ and #EVDET are open-drain active low,
>> > +so it is possible layout them to one SoC pin with pull-up.
>> > +
>> >  Required properties supported by the device:
>> >
>> >   - "compatible": must be one of
>> >     "isil,isl1208"
>> >     "isil,isl1218"
>> > +   "isil,isl1219"
>> >   - "reg": I2C bus address of the device
>> >
>> >  Optional properties:
>> > @@ -33,3 +40,12 @@ Example isl1208 node with #IRQ pin connected to SoC gpio1 pin 12:
>> >             interrupt-parent = <&gpio1>;
>> >             interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
>> >     };
>> > +
>> > +Example isl1219 node with #IRQ pin and #EVDET pin connected to SoC gpio1 pin 12:
>> > +
>> > +   isl1219: isl1219@68 {
>> > +           compatible = "intersil,isl1219";
>> > +           reg = <0x68>;
>> > +           interrupts-extended = <&gpio1 12 IRQ_TYPE_EDGE_FALLING>;
>> With 2 interrupts, you should have 2 values. If they are connected
>> together, just repeat the connection. Otherwise, you can't tell if EVDET
>> is a no connect.
> If I got you right, you suggest an additional IRQ entry to parse.

Yes.

> A short example, #IRQ pin connected to gpio1 pin 12 and
> #EVDET pin connected to gpio2 pin 24:
>
> isl1219: rtc@68 {
>         compatible = "intersil,isl1219";
>         reg = <0x68>;
>         interrupt-names = "irq", "evdet";
>         interrupts-extended = <&gpio1 12 IRQ_TYPE_EDGE_FALLING>,
>                 <&gpio2 24 IRQ_TYPE_EDGE_FALLING>;
> };
>
> In driver implementation we need only one interrupt, because we can
> determinate action to take based on value of status register.

That's fine. The binding describes the hardware. Drivers can support
as much or as little as they like.

> In current implementation there was no need to do some additional
> OF parsing, everything is done by I2C generic code.
> I guess, it is not much additional work to do so, but I am not sure
> if it´s worthwhile.

If you don't care about the 2nd interrupt, I don't think you'd have to
change anything.

>>
>> There's not much point in having an example for every compatible. This
>> binding is simple enough, one should be enough.
> Shell we remove the example without an interrupt?

The existing example has a single interrupt, right? That should be
enough. You just need to document for the interrupts property which
devices have 2 interrupts and what the order is.

Rob

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
       [not found]             ` <CAL_JsqKEXtL0hvEgFALKaCgW2Go==wj14_dDomdK_hPU23oXLQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2018-01-30 14:44               ` Rob Herring
  0 siblings, 0 replies; 23+ messages in thread
From: Rob Herring @ 2018-01-30 14:44 UTC (permalink / raw)
  To: Denis OSTERLAND
  Cc: m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, linux-0h96xk9xTtrk1uMJSBkQmQ,
	a.zummo-BfzFCNDTiLLj+vYz1yj4TQ, jdelvare-IBi9RG/b67k,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	linux-rtc-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ

On Tue, Jan 30, 2018 at 8:41 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Jan 30, 2018 at 2:56 AM, Denis OSTERLAND
> <denis.osterland-85mDkmTllUAAvxtiuMwx3w@public.gmane.org> wrote:
>> Am Montag, den 29.01.2018, 17:41 -0600 schrieb Rob Herring:
>>> On Tue, Jan 23, 2018 at 01:18:01PM +0100, Michael Grzeschik wrote:
>>> >
>>> > We add support for the ISL1219 chip that got an integrated tamper
>>> > detection function. This patch implements the feature by using an hwmon
>>> > interface.

[...]

>>> There's not much point in having an example for every compatible. This
>>> binding is simple enough, one should be enough.
>> Shell we remove the example without an interrupt?
>
> The existing example has a single interrupt, right? That should be
> enough. You just need to document for the interrupts property which
> devices have 2 interrupts and what the order is.

Looking at the first patch now, yes you can drop the 1st example and
just keep the 2nd example.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection
  2018-01-30 14:15                       ` Guenter Roeck
@ 2018-01-31 10:54                         ` Alexandre Belloni
  0 siblings, 0 replies; 23+ messages in thread
From: Alexandre Belloni @ 2018-01-31 10:54 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Denis OSTERLAND, linux-rtc, a.zummo, kernel, mgr, jdelvare,
	devicetree, linux-kernel

On 30/01/2018 at 06:15:18 -0800, Guenter Roeck wrote:
> On 01/30/2018 03:40 AM, Denis OSTERLAND wrote:
> > Am Dienstag, den 30.01.2018, 11:27 +0100 schrieb Alexandre Belloni:
> > > On 29/01/2018 at 13:59:19 -0800, Guenter Roeck wrote:
> > > > 
> > > > On Wed, Jan 24, 2018 at 10:03:33AM +0100, Michael Grzeschik wrote:
> > > > [ ... ]
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > +
> > > > > > > diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> > > > > > > index fc337c317c673..a12b3c2b2a18c 100644
> > > > > > > --- a/Documentation/hwmon/sysfs-interface
> > > > > > > +++ b/Documentation/hwmon/sysfs-interface
> > > > > > > @@ -702,6 +702,13 @@ intrusion[0-*]_alarm
> > > > > > >   		the user. This is done by writing 0 to the file. Writing
> > > > > > >   		other values is unsupported.
> > > > > > > +intrusion[0-*]_timestamp
> > > > > > > +		Chassis intrusion detection
> > > > > > > +		YYYY-MM-DD HH:MM:SS UTC (ts.sec): intrusion detected
> > > > > > > +		RO
> > > > > > > +		The corresponding timestamp on which the intrustion
> > > > > > > +		was detected.
> > > > > > > +
> > > > > > Sneaky. Nack. You don't just add attributes to the ABI because you want it,
> > > > > > without serious discussion, and much less so hidden in an RTC driver
> > > > > > (and even less as unparseable attribute).
> > > > > Right; but it was not meant to be sneaky. I should have stick to my first
> > > > > thought and label this patch RFC. Sorry for that.
> > > > > 
> > > > > > 
> > > > > > In addition to that, I consider the attribute unnecessary. The intrusion
> > > > > > already generates an event which should be sufficient for all practical
> > > > > > purposes.
> > > > > Would it make sense in between the other sysfs attributes of this driver?
> > > > > 
> > > > I don't understand what you mean with that, sorry.
> > > > 
> > > >  From an ABI perspective, the attibute doesn't add value since it is
> > > > highly device specific (or at least it is the only chip I am aware of
> > > > which reports such a time stamp). Feel free to add the attribute to the
> > > > driver and document it, but not as part of the hwmon ABI. In that
> > > > case I would be inclined to accept it. However, keep in mind that
> > > > your version, reporting a human readable date/time, would effectively
> > > > preclude it from ever making it into the ABI.
> > > > 
> > > Actually, there are many RTCs that are able to register one or more
> > > timestamps. My plan was to add support for that soon but I was not
> > > planning to do so in the hwmon ABI as this may be used for something
> > > that is not intrusion detection (interval timers for example).
> > What would you suggest?
> > I think about something like this:
> > event[0-*]_timestamp: timestamp in seconds since epoch or empty if not triggered
> > event[0-*]_alarm: 1 if event was triggered, else 0; write 0 to clear event
> 
> Sure, that makes sense if the events are not specifically related
> to intrusion detection. Question is if there would ever be more than one
> or if event_timestamp and event_alarm would be sufficient.
> 

My target is a PCF85363A which supports up to 3 timestamps. SO I'd go
for timestamp[0-*]. This would be empty if it never triggered (or the
timestamp is invalid) writing anything to that file resets the event. I
don't think we need more than one file per timestamp.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/4] rtc: isl1208: Fix unintended clear of SR bits
       [not found]     ` <20180123121801.4214-2-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2018-02-14 20:26       ` Alexandre Belloni
  2018-02-15  7:27         ` Denis OSTERLAND
  0 siblings, 1 reply; 23+ messages in thread
From: Alexandre Belloni @ 2018-02-14 20:26 UTC (permalink / raw)
  To: Michael Grzeschik
  Cc: a.zummo-BfzFCNDTiLLj+vYz1yj4TQ, linux-rtc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, linux-0h96xk9xTtrk1uMJSBkQmQ,
	jdelvare-IBi9RG/b67k, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
	Denis.Osterland-85mDkmTllUAAvxtiuMwx3w

On 23/01/2018 at 13:17:58 +0100, Michael Grzeschik wrote:
> From: Denis Osterland <Denis.Osterland-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
> 
> After successful
> sr = isl1208_i2c_set_regs(client, 0, regs, ISL1208_RTC_SECTION_LEN);
> sr will be 0.
> As a result
> sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR,
> 			sr & ~ISL1208_REG_SR_WRTC);
> is equal to
> sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR, 0);
> which clears all flags in SR.
> 
> Add an additional read of SR, to have value of SR in sr again.
> 
> Signed-off-by: Denis Osterland <Denis.Osterland-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  drivers/rtc/rtc-isl1208.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/4] rtc: isl1208: Fix unintended clear of SR bits
  2018-02-14 20:26       ` Alexandre Belloni
@ 2018-02-15  7:27         ` Denis OSTERLAND
       [not found]           ` <1518679666.5448.4.camel-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 23+ messages in thread
From: Denis OSTERLAND @ 2018-02-15  7:27 UTC (permalink / raw)
  To: m.grzeschik, alexandre.belloni
  Cc: linux, linux-rtc, a.zummo, linux-kernel, jdelvare, devicetree, kernel

Am Mittwoch, den 14.02.2018, 21:26 +0100 schrieb Alexandre Belloni:
> On 23/01/2018 at 13:17:58 +0100, Michael Grzeschik wrote:
> > 
> > From: Denis Osterland <Denis.Osterland@diehl.com>
> > 
> > After successful
> > sr = isl1208_i2c_set_regs(client, 0, regs, ISL1208_RTC_SECTION_LEN);
> > sr will be 0.
> > As a result
> > sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR,
> > 			sr & ~ISL1208_REG_SR_WRTC);
> > is equal to
> > sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR, 0);
> > which clears all flags in SR.
> > 
> > Add an additional read of SR, to have value of SR in sr again.
> > 
> > Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> >  drivers/rtc/rtc-isl1208.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> Applied, thanks.
> 
You are welcome.

One question, shall we avoid resent this patch in v2 of this series?
I ask because we are pretty far with the suggested changes.

Regards Denis.
Diehl AKO Stiftung & Co. KG, Pfannerstraße 75-83, 88239 Wangen im Allgäu
Bereichsvorstand: Dr.-Ing. Michael Siedentop (Sprecher), Josef Fellner (Mitglied)
Sitz der Gesellschaft: Wangen i.A. – Registergericht: Amtsgericht Ulm HRA 620609 – Persönlich haftende Gesellschafterin: Diehl Verwaltungs-Stiftung – Sitz: Nürnberg – Registergericht: Amtsgericht Nürnberg HRA 11756 –
Vorstand: Dr.-Ing. E.h. Thomas Diehl (†) (Vorsitzender), Herr Dipl.-Wirtsch.-Ing. Wolfgang Weggen (stellvertretender Vorsitzender), Dipl.-Kfm. Claus Günther, Dipl.-Kfm. Frank Gutzeit, Dr.-Ing. Heinrich Schunk, Dr.-Ing. Michael Siedentop , Dipl.-Kfm. Dr.-Ing. Martin Sommer, Dipl.-Ing. (FH) Rainer von Borstel, Vorsitzender des Aufsichtsrates: Dr. Klaus Maier
___________________________________________________________________________________________________
Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht. Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/4] rtc: isl1208: Fix unintended clear of SR bits
       [not found]           ` <1518679666.5448.4.camel-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
@ 2018-02-15  8:30             ` Alexandre Belloni
  0 siblings, 0 replies; 23+ messages in thread
From: Alexandre Belloni @ 2018-02-15  8:30 UTC (permalink / raw)
  To: Denis OSTERLAND
  Cc: m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ, linux-0h96xk9xTtrk1uMJSBkQmQ,
	linux-rtc-u79uwXL29TY76Z2rM5mHXA, a.zummo-BfzFCNDTiLLj+vYz1yj4TQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, jdelvare-IBi9RG/b67k,
	devicetree-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ

On 15/02/2018 at 07:27:47 +0000, Denis OSTERLAND wrote:
> Am Mittwoch, den 14.02.2018, 21:26 +0100 schrieb Alexandre Belloni:
> > On 23/01/2018 at 13:17:58 +0100, Michael Grzeschik wrote:
> > > 
> > > From: Denis Osterland <Denis.Osterland-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
> > > 
> > > After successful
> > > sr = isl1208_i2c_set_regs(client, 0, regs, ISL1208_RTC_SECTION_LEN);
> > > sr will be 0.
> > > As a result
> > > sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR,
> > > 			sr & ~ISL1208_REG_SR_WRTC);
> > > is equal to
> > > sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR, 0);
> > > which clears all flags in SR.
> > > 
> > > Add an additional read of SR, to have value of SR in sr again.
> > > 
> > > Signed-off-by: Denis Osterland <Denis.Osterland-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > ---
> > >  drivers/rtc/rtc-isl1208.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > Applied, thanks.
> > 
> You are welcome.
> 
> One question, shall we avoid resent this patch in v2 of this series?
> I ask because we are pretty far with the suggested changes.
> 

No need to resend. If necessary, you can base v2 on top of rtc-next.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2018-02-15  8:30 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-23 12:17 [PATCH 0/4] rtc: isl1208: fixes, documentation and isl1219 support Michael Grzeschik
     [not found] ` <20180123121801.4214-1-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2018-01-23 12:17   ` [PATCH 1/4] rtc: isl1208: Fix unintended clear of SR bits Michael Grzeschik
     [not found]     ` <20180123121801.4214-2-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2018-02-14 20:26       ` Alexandre Belloni
2018-02-15  7:27         ` Denis OSTERLAND
     [not found]           ` <1518679666.5448.4.camel-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
2018-02-15  8:30             ` Alexandre Belloni
2018-01-23 12:17   ` [PATCH 2/4] rtc: isl1208: Add device tree binding documentation Michael Grzeschik
2018-01-29 23:34     ` Rob Herring
2018-01-30 10:06     ` Alexandre Belloni
2018-01-23 12:18   ` [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection Michael Grzeschik
     [not found]     ` <20180123121801.4214-5-m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2018-01-23 18:22       ` Guenter Roeck
2018-01-24  9:03         ` Michael Grzeschik
2018-01-24 12:10           ` Michael Grzeschik
     [not found]           ` <20180124090333.r5o2mzpm4q536w4r-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2018-01-29 21:59             ` Guenter Roeck
2018-01-30 10:27               ` Alexandre Belloni
     [not found]                 ` <20180130102740.GD2809-m++hUPXGwpdeoWH0uzbU5w@public.gmane.org>
2018-01-30 11:40                   ` Denis OSTERLAND
     [not found]                     ` <1517312409.5307.22.camel-85mDkmTllUAAvxtiuMwx3w@public.gmane.org>
2018-01-30 14:15                       ` Guenter Roeck
2018-01-31 10:54                         ` Alexandre Belloni
2018-01-29 23:41       ` Rob Herring
2018-01-30  8:56         ` Denis OSTERLAND
2018-01-30 14:41           ` Rob Herring
     [not found]             ` <CAL_JsqKEXtL0hvEgFALKaCgW2Go==wj14_dDomdK_hPU23oXLQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-01-30 14:44               ` Rob Herring
2018-01-23 12:18 ` [PATCH 3/4] rtc: isl1208: enable interrupt after context preparation Michael Grzeschik
2018-01-30 10:34   ` Alexandre Belloni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).