[PATCH 0/4] rtc: isl1208: fixes, documentation and isl1219 support

[PATCH 0/4] rtc: isl1208: fixes, documentation and isl1219 support

[Michael Grzeschik]

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

[PATCH 1/4] rtc: isl1208: Fix unintended clear of SR bits

[Michael Grzeschik]

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(+)

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

Re: [PATCH 1/4] rtc: isl1208: Fix unintended clear of SR bits

[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.

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH 1/4] rtc: isl1208: Fix unintended clear of SR bits

[Denis OSTERLAND]

QW0gTWl0dHdvY2gsIGRlbiAxNC4wMi4yMDE4LCAyMToyNiArMDEwMCBzY2hyaWViIEFsZXhh
bmRyZSBCZWxsb25pOg0KPiBPbiAyMy8wMS8yMDE4IGF0IDEzOjE3OjU4ICswMTAwLCBNaWNo
YWVsIEdyemVzY2hpayB3cm90ZToNCj4gPiANCj4gPiBGcm9tOiBEZW5pcyBPc3RlcmxhbmQg
PERlbmlzLk9zdGVybGFuZEBkaWVobC5jb20+DQo+ID4gDQo+ID4gQWZ0ZXIgc3VjY2Vzc2Z1
bA0KPiA+IHNyID0gaXNsMTIwOF9pMmNfc2V0X3JlZ3MoY2xpZW50LCAwLCByZWdzLCBJU0wx
MjA4X1JUQ19TRUNUSU9OX0xFTik7DQo+ID4gc3Igd2lsbCBiZSAwLg0KPiA+IEFzIGEgcmVz
dWx0DQo+ID4gc3IgPSBpMmNfc21idXNfd3JpdGVfYnl0ZV9kYXRhKGNsaWVudCwgSVNMMTIw
OF9SRUdfU1IsDQo+ID4gCQkJc3IgJiB+SVNMMTIwOF9SRUdfU1JfV1JUQyk7DQo+ID4gaXMg
ZXF1YWwgdG8NCj4gPiBzciA9IGkyY19zbWJ1c193cml0ZV9ieXRlX2RhdGEoY2xpZW50LCBJ
U0wxMjA4X1JFR19TUiwgMCk7DQo+ID4gd2hpY2ggY2xlYXJzIGFsbCBmbGFncyBpbiBTUi4N
Cj4gPiANCj4gPiBBZGQgYW4gYWRkaXRpb25hbCByZWFkIG9mIFNSLCB0byBoYXZlIHZhbHVl
IG9mIFNSIGluIHNyIGFnYWluLg0KPiA+IA0KPiA+IFNpZ25lZC1vZmYtYnk6IERlbmlzIE9z
dGVybGFuZCA8RGVuaXMuT3N0ZXJsYW5kQGRpZWhsLmNvbT4NCj4gPiBTaWduZWQtb2ZmLWJ5
OiBNaWNoYWVsIEdyemVzY2hpayA8bS5ncnplc2NoaWtAcGVuZ3V0cm9uaXguZGU+DQo+ID4g
LS0tDQo+ID4gwqBkcml2ZXJzL3J0Yy9ydGMtaXNsMTIwOC5jIHwgNSArKysrKw0KPiA+IMKg
MSBmaWxlIGNoYW5nZWQsIDUgaW5zZXJ0aW9ucygrKQ0KPiA+IA0KPiBBcHBsaWVkLCB0aGFu
a3MuDQo+IA0KWW91IGFyZSB3ZWxjb21lLg0KDQpPbmUgcXVlc3Rpb24sIHNoYWxsIHdlIGF2
b2lkIHJlc2VudCB0aGlzIHBhdGNoIGluIHYyIG9mIHRoaXMgc2VyaWVzPw0KSSBhc2sgYmVj
YXVzZSB3ZSBhcmUgcHJldHR5IGZhciB3aXRoIHRoZSBzdWdnZXN0ZWQgY2hhbmdlcy4NCg0K
UmVnYXJkcyBEZW5pcy4NCkRpZWhsIEFLTyBTdGlmdHVuZyAmIENvLiBLRywgUGZhbm5lcnN0
cmHDn2UgNzUtODMsIDg4MjM5IFdhbmdlbiBpbSBBbGxnw6R1DQpCZXJlaWNoc3ZvcnN0YW5k
OiBEci4tSW5nLiBNaWNoYWVsIFNpZWRlbnRvcCAoU3ByZWNoZXIpLCBKb3NlZiBGZWxsbmVy
IChNaXRnbGllZCkNClNpdHogZGVyIEdlc2VsbHNjaGFmdDogV2FuZ2VuIGkuQS4g4oCTIFJl
Z2lzdGVyZ2VyaWNodDogQW10c2dlcmljaHQgVWxtIEhSQSA2MjA2MDkg4oCTIFBlcnPDtm5s
aWNoIGhhZnRlbmRlIEdlc2VsbHNjaGFmdGVyaW46IERpZWhsIFZlcndhbHR1bmdzLVN0aWZ0
dW5nIOKAkyBTaXR6OiBOw7xybmJlcmcg4oCTIFJlZ2lzdGVyZ2VyaWNodDogQW10c2dlcmlj
aHQgTsO8cm5iZXJnIEhSQSAxMTc1NiDigJMNClZvcnN0YW5kOiBEci4tSW5nLiBFLmguIFRo
b21hcyBEaWVobCAo4oCgKSAoVm9yc2l0emVuZGVyKSwgSGVyciBEaXBsLi1XaXJ0c2NoLi1J
bmcuIFdvbGZnYW5nIFdlZ2dlbiAoc3RlbGx2ZXJ0cmV0ZW5kZXIgVm9yc2l0emVuZGVyKSwg
RGlwbC4tS2ZtLiBDbGF1cyBHw7xudGhlciwgRGlwbC4tS2ZtLiBGcmFuayBHdXR6ZWl0LCBE
ci4tSW5nLiBIZWlucmljaCBTY2h1bmssIERyLi1JbmcuIE1pY2hhZWwgU2llZGVudG9wICwg
RGlwbC4tS2ZtLiBEci4tSW5nLiBNYXJ0aW4gU29tbWVyLCBEaXBsLi1JbmcuIChGSCkgUmFp
bmVyIHZvbiBCb3JzdGVsLCBWb3JzaXR6ZW5kZXIgZGVzIEF1ZnNpY2h0c3JhdGVzOiBEci4g
S2xhdXMgTWFpZXINCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fXw0KRGVyIEluaGFsdCBkZXIgdm9yc3RlaGVuZGVuIEUtTWFpbCBpc3QgbmljaHQgcmVj
aHRsaWNoIGJpbmRlbmQuIERpZXNlIEUtTWFpbCBlbnRoYWVsdCB2ZXJ0cmF1bGljaGUgdW5k
L29kZXIgcmVjaHRsaWNoIGdlc2NodWV0enRlIEluZm9ybWF0aW9uZW4uDQpJbmZvcm1pZXJl
biBTaWUgdW5zIGJpdHRlLCB3ZW5uIFNpZSBkaWVzZSBFLU1haWwgZmFlbHNjaGxpY2hlcndl
aXNlIGVyaGFsdGVuIGhhYmVuLiBCaXR0ZSBsb2VzY2hlbiBTaWUgaW4gZGllc2VtIEZhbGwg
ZGllIE5hY2hyaWNodC4gSmVkZSB1bmVybGF1YnRlIEZvcm0gZGVyIFJlcHJvZHVrdGlvbiwg
QmVrYW5udGdhYmUsIEFlbmRlcnVuZywgVmVydGVpbHVuZyB1bmQvb2RlciBQdWJsaWthdGlv
biBkaWVzZXIgRS1NYWlsIGlzdCBzdHJlbmdzdGVucyB1bnRlcnNhZ3QuDQpUaGUgY29udGVu
dHMgb2YgdGhlIGFib3ZlIG1lbnRpb25lZCBlLW1haWwgaXMgbm90IGxlZ2FsbHkgYmluZGlu
Zy4gVGhpcyBlLW1haWwgY29udGFpbnMgY29uZmlkZW50aWFsIGFuZC9vciBsZWdhbGx5IHBy
b3RlY3RlZCBpbmZvcm1hdGlvbi4gUGxlYXNlIGluZm9ybSB1cyBpZiB5b3UgaGF2ZSByZWNl
aXZlZCB0aGlzIGUtbWFpbCBieSBtaXN0YWtlIGFuZCBkZWxldGUgaXQgaW4gc3VjaCBhIGNh
c2UuIEVhY2ggdW5hdXRob3JpemVkIHJlcHJvZHVjdGlvbiwgZGlzY2xvc3VyZSwgYWx0ZXJh
dGlvbiwgZGlzdHJpYnV0aW9uIGFuZC9vciBwdWJsaWNhdGlvbiBvZiB0aGlzIGUtbWFpbCBp
cyBzdHJpY3RseSBwcm9oaWJpdGVkLg==

Re: [PATCH 1/4] rtc: isl1208: Fix unintended clear of SR bits

[Alexandre Belloni]

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@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.
> 

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

[PATCH 2/4] rtc: isl1208: Add device tree binding documentation

[Michael Grzeschik]

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

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

Re: [PATCH 2/4] rtc: isl1208: Add device tree binding documentation

[Rob Herring]

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

Re: [PATCH 2/4] rtc: isl1208: Add device tree binding documentation

[Alexandre Belloni]

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

[PATCH 3/4] rtc: isl1208: enable interrupt after context preparation

[Michael Grzeschik]

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

Re: [PATCH 3/4] rtc: isl1208: enable interrupt after context preparation

[Alexandre Belloni]

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

[PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[Michael Grzeschik]

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.
+
 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

Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[Guenter Roeck]

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).

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
> 

Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[Michael Grzeschik]

[-- 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 --]

Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[Michael Grzeschik]

[-- 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 --]

Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[Guenter Roeck]

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

Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[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).


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

Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[Denis OSTERLAND]

QW0gRGllbnN0YWcsIGRlbiAzMC4wMS4yMDE4LCAxMToyNyArMDEwMCBzY2hyaWViIEFsZXhh
bmRyZSBCZWxsb25pOg0KPiBPbiAyOS8wMS8yMDE4IGF0IDEzOjU5OjE5IC0wODAwLCBHdWVu
dGVyIFJvZWNrIHdyb3RlOg0KPiA+IA0KPiA+IE9uIFdlZCwgSmFuIDI0LCAyMDE4IGF0IDEw
OjAzOjMzQU0gKzAxMDAsIE1pY2hhZWwgR3J6ZXNjaGlrIHdyb3RlOg0KPiA+IFsgLi4uIF0N
Cj4gPiA+IA0KPiA+ID4gPiANCj4gPiA+ID4gPiANCj4gPiA+ID4gPiArDQo+ID4gPiA+ID4g
ZGlmZiAtLWdpdCBhL0RvY3VtZW50YXRpb24vaHdtb24vc3lzZnMtaW50ZXJmYWNlIGIvRG9j
dW1lbnRhdGlvbi9od21vbi9zeXNmcy1pbnRlcmZhY2UNCj4gPiA+ID4gPiBpbmRleCBmYzMz
N2MzMTdjNjczLi5hMTJiM2MyYjJhMThjIDEwMDY0NA0KPiA+ID4gPiA+IC0tLSBhL0RvY3Vt
ZW50YXRpb24vaHdtb24vc3lzZnMtaW50ZXJmYWNlDQo+ID4gPiA+ID4gKysrIGIvRG9jdW1l
bnRhdGlvbi9od21vbi9zeXNmcy1pbnRlcmZhY2UNCj4gPiA+ID4gPiBAQCAtNzAyLDYgKzcw
MiwxMyBAQCBpbnRydXNpb25bMC0qXV9hbGFybQ0KPiA+ID4gPiA+IMKgCQl0aGUgdXNlci4g
VGhpcyBpcyBkb25lIGJ5IHdyaXRpbmcgMCB0byB0aGUgZmlsZS4gV3JpdGluZw0KPiA+ID4g
PiA+IMKgCQlvdGhlciB2YWx1ZXMgaXMgdW5zdXBwb3J0ZWQuDQo+ID4gPiA+ID4gwqANCj4g
PiA+ID4gPiAraW50cnVzaW9uWzAtKl1fdGltZXN0YW1wDQo+ID4gPiA+ID4gKwkJQ2hhc3Np
cyBpbnRydXNpb24gZGV0ZWN0aW9uDQo+ID4gPiA+ID4gKwkJWVlZWS1NTS1ERCBISDpNTTpT
UyBVVEMgKHRzLnNlYyk6IGludHJ1c2lvbiBkZXRlY3RlZA0KPiA+ID4gPiA+ICsJCVJPDQo+
ID4gPiA+ID4gKwkJVGhlIGNvcnJlc3BvbmRpbmcgdGltZXN0YW1wIG9uIHdoaWNoIHRoZSBp
bnRydXN0aW9uDQo+ID4gPiA+ID4gKwkJd2FzIGRldGVjdGVkLg0KPiA+ID4gPiA+ICsNCj4g
PiA+ID4gU25lYWt5LiBOYWNrLiBZb3UgZG9uJ3QganVzdCBhZGQgYXR0cmlidXRlcyB0byB0
aGUgQUJJIGJlY2F1c2UgeW91IHdhbnQgaXQsDQo+ID4gPiA+IHdpdGhvdXQgc2VyaW91cyBk
aXNjdXNzaW9uLCBhbmQgbXVjaCBsZXNzIHNvIGhpZGRlbiBpbiBhbiBSVEMgZHJpdmVyDQo+
ID4gPiA+IChhbmQgZXZlbiBsZXNzIGFzIHVucGFyc2VhYmxlIGF0dHJpYnV0ZSkuDQo+ID4g
PiBSaWdodDsgYnV0IGl0IHdhcyBub3QgbWVhbnQgdG8gYmUgc25lYWt5LiBJIHNob3VsZCBo
YXZlIHN0aWNrIHRvIG15IGZpcnN0DQo+ID4gPiB0aG91Z2h0IGFuZCBsYWJlbCB0aGlzIHBh
dGNoIFJGQy4gU29ycnkgZm9yIHRoYXQuDQo+ID4gPiANCj4gPiA+ID4gDQo+ID4gPiA+IElu
IGFkZGl0aW9uIHRvIHRoYXQsIEkgY29uc2lkZXIgdGhlIGF0dHJpYnV0ZSB1bm5lY2Vzc2Fy
eS4gVGhlIGludHJ1c2lvbg0KPiA+ID4gPiBhbHJlYWR5IGdlbmVyYXRlcyBhbiBldmVudCB3
aGljaCBzaG91bGQgYmUgc3VmZmljaWVudCBmb3IgYWxsIHByYWN0aWNhbA0KPiA+ID4gPiBw
dXJwb3Nlcy4NCj4gPiA+IFdvdWxkIGl0IG1ha2Ugc2Vuc2UgaW4gYmV0d2VlbiB0aGUgb3Ro
ZXIgc3lzZnMgYXR0cmlidXRlcyBvZiB0aGlzIGRyaXZlcj8NCj4gPiA+IA0KPiA+IEkgZG9u
J3QgdW5kZXJzdGFuZCB3aGF0IHlvdSBtZWFuIHdpdGggdGhhdCwgc29ycnkuDQo+ID4gDQo+
ID4gRnJvbSBhbiBBQkkgcGVyc3BlY3RpdmUsIHRoZSBhdHRpYnV0ZSBkb2Vzbid0IGFkZCB2
YWx1ZSBzaW5jZSBpdCBpcw0KPiA+IGhpZ2hseSBkZXZpY2Ugc3BlY2lmaWMgKG9yIGF0IGxl
YXN0IGl0IGlzIHRoZSBvbmx5IGNoaXAgSSBhbSBhd2FyZSBvZg0KPiA+IHdoaWNoIHJlcG9y
dHMgc3VjaCBhIHRpbWUgc3RhbXApLiBGZWVsIGZyZWUgdG8gYWRkIHRoZSBhdHRyaWJ1dGUg
dG8gdGhlDQo+ID4gZHJpdmVyIGFuZCBkb2N1bWVudCBpdCwgYnV0IG5vdCBhcyBwYXJ0IG9m
IHRoZSBod21vbiBBQkkuIEluIHRoYXQNCj4gPiBjYXNlIEkgd291bGQgYmUgaW5jbGluZWQg
dG8gYWNjZXB0IGl0LiBIb3dldmVyLCBrZWVwIGluIG1pbmQgdGhhdA0KPiA+IHlvdXIgdmVy
c2lvbiwgcmVwb3J0aW5nIGEgaHVtYW4gcmVhZGFibGUgZGF0ZS90aW1lLCB3b3VsZCBlZmZl
Y3RpdmVseQ0KPiA+IHByZWNsdWRlIGl0IGZyb20gZXZlciBtYWtpbmcgaXQgaW50byB0aGUg
QUJJLg0KPiA+IA0KPiBBY3R1YWxseSwgdGhlcmUgYXJlIG1hbnkgUlRDcyB0aGF0IGFyZSBh
YmxlIHRvIHJlZ2lzdGVyIG9uZSBvciBtb3JlDQo+IHRpbWVzdGFtcHMuIE15IHBsYW4gd2Fz
IHRvIGFkZCBzdXBwb3J0IGZvciB0aGF0IHNvb24gYnV0IEkgd2FzIG5vdA0KPiBwbGFubmlu
ZyB0byBkbyBzbyBpbiB0aGUgaHdtb24gQUJJIGFzIHRoaXMgbWF5IGJlIHVzZWQgZm9yIHNv
bWV0aGluZw0KPiB0aGF0IGlzIG5vdCBpbnRydXNpb24gZGV0ZWN0aW9uIChpbnRlcnZhbCB0
aW1lcnMgZm9yIGV4YW1wbGUpLg0KV2hhdCB3b3VsZCB5b3Ugc3VnZ2VzdD8NCkkgdGhpbmsg
YWJvdXQgc29tZXRoaW5nIGxpa2UgdGhpczoNCmV2ZW50WzAtKl1fdGltZXN0YW1wOiB0aW1l
c3RhbXAgaW4gc2Vjb25kcyBzaW5jZSBlcG9jaCBvciBlbXB0eSBpZiBub3QgdHJpZ2dlcmVk
DQpldmVudFswLSpdX2FsYXJtOiAxIGlmIGV2ZW50IHdhcyB0cmlnZ2VyZWQsIGVsc2UgMDsg
d3JpdGUgMCB0byBjbGVhciBldmVudA0KPiANCj4gDQpEaWVobCBBS08gU3RpZnR1bmcgJiBD
by4gS0csIFBmYW5uZXJzdHJhw59lIDc1LTgzLCA4ODIzOSBXYW5nZW4gaW0gQWxsZ8OkdQ0K
QmVyZWljaHN2b3JzdGFuZDogRHIuLUluZy4gTWljaGFlbCBTaWVkZW50b3AgKFNwcmVjaGVy
KSwgSm9zZWYgRmVsbG5lciAoTWl0Z2xpZWQpDQpTaXR6IGRlciBHZXNlbGxzY2hhZnQ6IFdh
bmdlbiBpLkEuIOKAkyBSZWdpc3RlcmdlcmljaHQ6IEFtdHNnZXJpY2h0IFVsbSBIUkEgNjIw
NjA5IOKAkyBQZXJzw7ZubGljaCBoYWZ0ZW5kZSBHZXNlbGxzY2hhZnRlcmluOiBEaWVobCBW
ZXJ3YWx0dW5ncy1TdGlmdHVuZyDigJMgU2l0ejogTsO8cm5iZXJnIOKAkyBSZWdpc3Rlcmdl
cmljaHQ6IEFtdHNnZXJpY2h0IE7DvHJuYmVyZyBIUkEgMTE3NTYg4oCTDQpWb3JzdGFuZDog
RHIuLUluZy4gRS5oLiBUaG9tYXMgRGllaGwgKOKAoCkgKFZvcnNpdHplbmRlciksIEhlcnIg
RGlwbC4tV2lydHNjaC4tSW5nLiBXb2xmZ2FuZyBXZWdnZW4gKHN0ZWxsdmVydHJldGVuZGVy
IFZvcnNpdHplbmRlciksIERpcGwuLUtmbS4gQ2xhdXMgR8O8bnRoZXIsIERpcGwuLUtmbS4g
RnJhbmsgR3V0emVpdCwgRHIuLUluZy4gSGVpbnJpY2ggU2NodW5rLCBEci4tSW5nLiBNaWNo
YWVsIFNpZWRlbnRvcCAsIERpcGwuLUtmbS4gRHIuLUluZy4gTWFydGluIFNvbW1lciwgRGlw
bC4tSW5nLiAoRkgpIFJhaW5lciB2b24gQm9yc3RlbCwgVm9yc2l0emVuZGVyIGRlcyBBdWZz
aWNodHNyYXRlczogRHIuIEtsYXVzIE1haWVyDQpfX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fX18NCkRlciBJbmhhbHQgZGVyIHZvcnN0ZWhlbmRlbiBFLU1h
aWwgaXN0IG5pY2h0IHJlY2h0bGljaCBiaW5kZW5kLiBEaWVzZSBFLU1haWwgZW50aGFlbHQg
dmVydHJhdWxpY2hlIHVuZC9vZGVyIHJlY2h0bGljaCBnZXNjaHVldHp0ZSBJbmZvcm1hdGlv
bmVuLg0KSW5mb3JtaWVyZW4gU2llIHVucyBiaXR0ZSwgd2VubiBTaWUgZGllc2UgRS1NYWls
IGZhZWxzY2hsaWNoZXJ3ZWlzZSBlcmhhbHRlbiBoYWJlbi4gQml0dGUgbG9lc2NoZW4gU2ll
IGluIGRpZXNlbSBGYWxsIGRpZSBOYWNocmljaHQuIEplZGUgdW5lcmxhdWJ0ZSBGb3JtIGRl
ciBSZXByb2R1a3Rpb24sIEJla2FubnRnYWJlLCBBZW5kZXJ1bmcsIFZlcnRlaWx1bmcgdW5k
L29kZXIgUHVibGlrYXRpb24gZGllc2VyIEUtTWFpbCBpc3Qgc3RyZW5nc3RlbnMgdW50ZXJz
YWd0Lg0KVGhlIGNvbnRlbnRzIG9mIHRoZSBhYm92ZSBtZW50aW9uZWQgZS1tYWlsIGlzIG5v
dCBsZWdhbGx5IGJpbmRpbmcuIFRoaXMgZS1tYWlsIGNvbnRhaW5zIGNvbmZpZGVudGlhbCBh
bmQvb3IgbGVnYWxseSBwcm90ZWN0ZWQgaW5mb3JtYXRpb24uIFBsZWFzZSBpbmZvcm0gdXMg
aWYgeW91IGhhdmUgcmVjZWl2ZWQgdGhpcyBlLW1haWwgYnkgbWlzdGFrZSBhbmQgZGVsZXRl
IGl0IGluIHN1Y2ggYSBjYXNlLiBFYWNoIHVuYXV0aG9yaXplZCByZXByb2R1Y3Rpb24sIGRp
c2Nsb3N1cmUsIGFsdGVyYXRpb24sIGRpc3RyaWJ1dGlvbiBhbmQvb3IgcHVibGljYXRpb24g
b2YgdGhpcyBlLW1haWwgaXMgc3RyaWN0bHkgcHJvaGliaXRlZC4=

Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[Guenter Roeck]

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

Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[Alexandre Belloni]

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

Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[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.

> +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.

> +	};
> +

Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[Denis OSTERLAND]

QW0gTW9udGFnLCBkZW4gMjkuMDEuMjAxOCwgMTc6NDEgLTA2MDAgc2NocmllYiBSb2IgSGVy
cmluZzoNCj4gT24gVHVlLCBKYW4gMjMsIDIwMTggYXQgMDE6MTg6MDFQTSArMDEwMCwgTWlj
aGFlbCBHcnplc2NoaWsgd3JvdGU6DQo+ID4gDQo+ID4gV2UgYWRkIHN1cHBvcnQgZm9yIHRo
ZSBJU0wxMjE5IGNoaXAgdGhhdCBnb3QgYW4gaW50ZWdyYXRlZCB0YW1wZXINCj4gPiBkZXRl
Y3Rpb24gZnVuY3Rpb24uIFRoaXMgcGF0Y2ggaW1wbGVtZW50cyB0aGUgZmVhdHVyZSBieSB1
c2luZyBhbiBod21vbg0KPiA+IGludGVyZmFjZS4NCj4gPiANCj4gPiBUaGUgSVNMMTIxOSBj
YW4gYWxzbyBkZXNjcmliZSB0aGUgdGltZXN0YW1wIG9mIHRoZSBpbnRydXNpb24NCj4gPiBl
dmVudC4gRm9yIHRoaXMgd2UgYWRkIHRoZSBkb2N1bWVudGF0aW9uIG9mIHRoZSBuZXcgaW50
ZXJmYWNlDQo+ID4gaW50cnVzaW9uWzAtKl1fdGltZXN0YW1wLg0KPiA+IA0KPiA+IFRoZSBk
ZXZpY2V0cmVlIGRvY3VtZW50YXRpb24gZm9yIHRoZSBJU0wxMjE5IGRldmljZSB0cmVlDQo+
ID4gYmluZGluZyBpcyBhZGRlZCB3aXRoIGFuIHNob3J0IGV4YW1wbGUuDQo+ID4gDQo+ID4g
U2lnbmVkLW9mZi1ieTogTWljaGFlbCBHcnplc2NoaWsgPG0uZ3J6ZXNjaGlrQHBlbmd1dHJv
bml4LmRlPg0KPiA+IFNpZ25lZC1vZmYtYnk6IERlbmlzIE9zdGVybGFuZCA8RGVuaXMuT3N0
ZXJsYW5kQGRpZWhsLmNvbT4NCj4gPiAtLS0NCj4gPiDCoC4uLi9ydGMve2ludGVyc2lsLGlz
bDEyMDgudHh0ID0+IGlzaWwsaXNsMTIwOC50eHR9IHzCoMKgMTggKy0NCj4gPiDCoERvY3Vt
ZW50YXRpb24vaHdtb24vc3lzZnMtaW50ZXJmYWNlwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqB8wqDCoMKgNyArDQo+ID4gwqBkcml2ZXJzL3J0Yy9ydGMtaXNsMTIwOC5jwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoHwgMTkw
ICsrKysrKysrKysrKysrKysrKystLQ0KPiA+IMKgMyBmaWxlcyBjaGFuZ2VkLCAyMDEgaW5z
ZXJ0aW9ucygrKSwgMTQgZGVsZXRpb25zKC0pDQo+ID4gwqByZW5hbWUgRG9jdW1lbnRhdGlv
bi9kZXZpY2V0cmVlL2JpbmRpbmdzL3J0Yy97aW50ZXJzaWwsaXNsMTIwOC50eHQgPT4gaXNp
bCxpc2wxMjA4LnR4dH0gKDU3JSkNCj4gPiANCj4gPiBkaWZmIC0tZ2l0IGEvRG9jdW1lbnRh
dGlvbi9kZXZpY2V0cmVlL2JpbmRpbmdzL3J0Yy9pbnRlcnNpbCxpc2wxMjA4LnR4dCBiL0Rv
Y3VtZW50YXRpb24vZGV2aWNldHJlZS9iaW5kaW5ncy9ydGMvaXNpbCxpc2wxMjA4LnR4dA0K
PiA+IHNpbWlsYXJpdHkgaW5kZXggNTclDQo+ID4gcmVuYW1lIGZyb20gRG9jdW1lbnRhdGlv
bi9kZXZpY2V0cmVlL2JpbmRpbmdzL3J0Yy9pbnRlcnNpbCxpc2wxMjA4LnR4dA0KPiA+IHJl
bmFtZSB0byBEb2N1bWVudGF0aW9uL2RldmljZXRyZWUvYmluZGluZ3MvcnRjL2lzaWwsaXNs
MTIwOC50eHQNCj4gPiBpbmRleCBhNTRlOTlmZWFlMWNhLi5kNTQ5Njk5ZTFjZmM0IDEwMDY0
NA0KPiA+IC0tLSBhL0RvY3VtZW50YXRpb24vZGV2aWNldHJlZS9iaW5kaW5ncy9ydGMvaW50
ZXJzaWwsaXNsMTIwOC50eHQNCj4gPiArKysgYi9Eb2N1bWVudGF0aW9uL2RldmljZXRyZWUv
YmluZGluZ3MvcnRjL2lzaWwsaXNsMTIwOC50eHQNCj4gPiBAQCAtMSwxNCArMSwyMSBAQA0K
PiA+IC1JbnRlcnNpbCBJU0wxMjA4LCBJU0wxMjE4IEkyQyBSVEMvQWxhcm0gY2hpcA0KPiA+
ICtJbnRlcnNpbCBJU0wxMjA4LCBJU0wxMjE4LCBJU0wxMjE5IEkyQyBSVEMvQWxhcm0gY2hp
cA0KPiA+IMKgDQo+ID4gwqBJU0wxMjA4IGlzIGEgdHJpdmlhbCBJMkMgZGV2aWNlIChpdCBo
YXMgc2ltcGxlIGRldmljZSB0cmVlIGJpbmRpbmdzLA0KPiA+IMKgY29uc2lzdGluZyBvZiBh
IGNvbXBhdGlibGUgZmllbGQsIGFuIGFkZHJlc3MgYW5kIHBvc3NpYmx5IGFuIGludGVycnVw
dA0KPiA+IMKgbGluZSkuDQo+ID4gwqANCj4gPiArSVNMMTIxOSBzdXBwb3J0cyB0YW1wZXIg
ZGV0ZWN0aW9uIHVzZXIgc3BhY2UgcmVwcmVzZW50YXRpb24gdGhyb3VnaA0KPiA+ICtjYXNl
IGludHJ1c2lvbiBod21vbiBzZW5zb3IuDQo+IFVzZXIgc3BhY2UgYW5kIGh3bW9uIGFyZSBM
aW51eCBkZXRhaWxzIG5vdCByZWxldmFudCB0byB0aGUgYmluZGluZy4gSnVzdMKgDQo+IGRl
c2NyaWJlIHRoZSBoL3cuDQpPSy4NCj4gDQo+ID4gDQo+ID4gK0lTTDEyMTkgaGFzIGFkZGl0
aW9uYWwgcGlucyBFVklOIGFuZCAjRVZERVQgZm9yIHRhbXBlciBkZXRlY3Rpb24uDQo+ID4g
K0kyQyBkZXZpY2VzIHN1cHBvcnQgb25seSBvbmUgaXJxLiAjSVJRIGFuZCAjRVZERVQgYXJl
IG9wZW4tZHJhaW4gYWN0aXZlIGxvdywNCj4gPiArc28gaXQgaXMgcG9zc2libGUgbGF5b3V0
IHRoZW0gdG8gb25lIFNvQyBwaW4gd2l0aCBwdWxsLXVwLg0KPiA+ICsNCj4gPiDCoFJlcXVp
cmVkIHByb3BlcnRpZXMgc3VwcG9ydGVkIGJ5IHRoZSBkZXZpY2U6DQo+ID4gwqANCj4gPiDC
oCAtICJjb21wYXRpYmxlIjogbXVzdCBiZSBvbmUgb2YNCj4gPiDCoAkiaXNpbCxpc2wxMjA4
Ig0KPiA+IMKgCSJpc2lsLGlzbDEyMTgiDQo+ID4gKwkiaXNpbCxpc2wxMjE5Ig0KPiA+IMKg
IC0gInJlZyI6IEkyQyBidXMgYWRkcmVzcyBvZiB0aGUgZGV2aWNlDQo+ID4gwqANCj4gPiDC
oE9wdGlvbmFsIHByb3BlcnRpZXM6DQo+ID4gQEAgLTMzLDMgKzQwLDEyIEBAIEV4YW1wbGUg
aXNsMTIwOCBub2RlIHdpdGggI0lSUSBwaW4gY29ubmVjdGVkIHRvIFNvQyBncGlvMSBwaW4g
MTI6DQo+ID4gwqAJCWludGVycnVwdC1wYXJlbnQgPSA8JmdwaW8xPjsNCj4gPiDCoAkJaW50
ZXJydXB0cyA9IDwxMiBJUlFfVFlQRV9FREdFX0ZBTExJTkc+Ow0KPiA+IMKgCX07DQo+ID4g
Kw0KPiA+ICtFeGFtcGxlIGlzbDEyMTkgbm9kZSB3aXRoICNJUlEgcGluIGFuZCAjRVZERVQg
cGluIGNvbm5lY3RlZCB0byBTb0MgZ3BpbzEgcGluIDEyOg0KPiA+ICsNCj4gPiArCWlzbDEy
MTk6IGlzbDEyMTlANjggew0KPiA+ICsJCWNvbXBhdGlibGUgPSAiaW50ZXJzaWwsaXNsMTIx
OSI7DQo+ID4gKwkJcmVnID0gPDB4Njg+Ow0KPiA+ICsJCWludGVycnVwdHMtZXh0ZW5kZWQg
PSA8JmdwaW8xIDEyIElSUV9UWVBFX0VER0VfRkFMTElORz47DQo+IFdpdGggMiBpbnRlcnJ1
cHRzLCB5b3Ugc2hvdWxkIGhhdmUgMiB2YWx1ZXMuIElmIHRoZXkgYXJlIGNvbm5lY3RlZMKg
DQo+IHRvZ2V0aGVyLCBqdXN0IHJlcGVhdCB0aGUgY29ubmVjdGlvbi4gT3RoZXJ3aXNlLCB5
b3UgY2FuJ3QgdGVsbCBpZiBFVkRFVMKgDQo+IGlzIGEgbm8gY29ubmVjdC4NCklmIEkgZ290
IHlvdSByaWdodCwgeW91IHN1Z2dlc3QgYW4gYWRkaXRpb25hbCBJUlEgZW50cnkgdG8gcGFy
c2UuDQpBIHNob3J0IGV4YW1wbGUsICNJUlEgcGluIGNvbm5lY3RlZCB0byBncGlvMSBwaW4g
MTIgYW5kDQojRVZERVQgcGluIGNvbm5lY3RlZCB0byBncGlvMiBwaW4gMjQ6DQoNCmlzbDEy
MTk6IHJ0Y0A2OMKgew0KCWNvbXBhdGlibGUgPSAiaW50ZXJzaWwsaXNsMTIxOSI7DQoJcmVn
ID0gPDB4Njg+Ow0KCWludGVycnVwdC1uYW1lcyA9ICJpcnEiLCAiZXZkZXQiOw0KCWludGVy
cnVwdHMtZXh0ZW5kZWQgPSA8JmdwaW8xIDEyIElSUV9UWVBFX0VER0VfRkFMTElORz4sDQoJ
CTwmZ3BpbzIgMjQgSVJRX1RZUEVfRURHRV9GQUxMSU5HPjsNCn07DQoNCkluIGRyaXZlciBp
bXBsZW1lbnRhdGlvbiB3ZSBuZWVkIG9ubHkgb25lIGludGVycnVwdCwgYmVjYXVzZSB3ZSBj
YW4NCmRldGVybWluYXRlIGFjdGlvbiB0byB0YWtlIGJhc2VkIG9uIHZhbHVlIG9mIHN0YXR1
cyByZWdpc3Rlci4NCkluIGN1cnJlbnQgaW1wbGVtZW50YXRpb24gdGhlcmUgd2FzIG5vIG5l
ZWQgdG8gZG8gc29tZSBhZGRpdGlvbmFsDQpPRiBwYXJzaW5nLCBldmVyeXRoaW5nIGlzIGRv
bmUgYnkgSTJDIGdlbmVyaWMgY29kZS4NCkkgZ3Vlc3MsIGl0IGlzIG5vdCBtdWNoIGFkZGl0
aW9uYWwgd29yayB0byBkbyBzbywgYnV0IEkgYW0gbm90IHN1cmUNCmlmIGl0wrRzIHdvcnRo
d2hpbGUuDQo+IA0KPiBUaGVyZSdzIG5vdCBtdWNoIHBvaW50IGluIGhhdmluZyBhbiBleGFt
cGxlIGZvciBldmVyeSBjb21wYXRpYmxlLiBUaGlzwqANCj4gYmluZGluZyBpcyBzaW1wbGUg
ZW5vdWdoLCBvbmUgc2hvdWxkIGJlIGVub3VnaC4NClNoZWxsIHdlIHJlbW92ZSB0aGUgZXhh
bXBsZSB3aXRob3V0IGFuIGludGVycnVwdD8NCj4gDQo+ID4gDQo+ID4gKwl9Ow0KPiA+ICsN
CkRpZWhsIEFLTyBTdGlmdHVuZyAmIENvLiBLRywgUGZhbm5lcnN0cmHDn2UgNzUtODMsIDg4
MjM5IFdhbmdlbiBpbSBBbGxnw6R1DQpCZXJlaWNoc3ZvcnN0YW5kOiBEci4tSW5nLiBNaWNo
YWVsIFNpZWRlbnRvcCAoU3ByZWNoZXIpLCBKb3NlZiBGZWxsbmVyIChNaXRnbGllZCkNClNp
dHogZGVyIEdlc2VsbHNjaGFmdDogV2FuZ2VuIGkuQS4g4oCTIFJlZ2lzdGVyZ2VyaWNodDog
QW10c2dlcmljaHQgVWxtIEhSQSA2MjA2MDkg4oCTIFBlcnPDtm5saWNoIGhhZnRlbmRlIEdl
c2VsbHNjaGFmdGVyaW46IERpZWhsIFZlcndhbHR1bmdzLVN0aWZ0dW5nIOKAkyBTaXR6OiBO
w7xybmJlcmcg4oCTIFJlZ2lzdGVyZ2VyaWNodDogQW10c2dlcmljaHQgTsO8cm5iZXJnIEhS
QSAxMTc1NiDigJMNClZvcnN0YW5kOiBEci4tSW5nLiBFLmguIFRob21hcyBEaWVobCAo4oCg
KSAoVm9yc2l0emVuZGVyKSwgSGVyciBEaXBsLi1XaXJ0c2NoLi1JbmcuIFdvbGZnYW5nIFdl
Z2dlbiAoc3RlbGx2ZXJ0cmV0ZW5kZXIgVm9yc2l0emVuZGVyKSwgRGlwbC4tS2ZtLiBDbGF1
cyBHw7xudGhlciwgRGlwbC4tS2ZtLiBGcmFuayBHdXR6ZWl0LCBEci4tSW5nLiBIZWlucmlj
aCBTY2h1bmssIERyLi1JbmcuIE1pY2hhZWwgU2llZGVudG9wICwgRGlwbC4tS2ZtLiBEci4t
SW5nLiBNYXJ0aW4gU29tbWVyLCBEaXBsLi1JbmcuIChGSCkgUmFpbmVyIHZvbiBCb3JzdGVs
LCBWb3JzaXR6ZW5kZXIgZGVzIEF1ZnNpY2h0c3JhdGVzOiBEci4gS2xhdXMgTWFpZXINCl9f
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXw0KRGVyIEluaGFs
dCBkZXIgdm9yc3RlaGVuZGVuIEUtTWFpbCBpc3QgbmljaHQgcmVjaHRsaWNoIGJpbmRlbmQu
IERpZXNlIEUtTWFpbCBlbnRoYWVsdCB2ZXJ0cmF1bGljaGUgdW5kL29kZXIgcmVjaHRsaWNo
IGdlc2NodWV0enRlIEluZm9ybWF0aW9uZW4uDQpJbmZvcm1pZXJlbiBTaWUgdW5zIGJpdHRl
LCB3ZW5uIFNpZSBkaWVzZSBFLU1haWwgZmFlbHNjaGxpY2hlcndlaXNlIGVyaGFsdGVuIGhh
YmVuLiBCaXR0ZSBsb2VzY2hlbiBTaWUgaW4gZGllc2VtIEZhbGwgZGllIE5hY2hyaWNodC4g
SmVkZSB1bmVybGF1YnRlIEZvcm0gZGVyIFJlcHJvZHVrdGlvbiwgQmVrYW5udGdhYmUsIEFl
bmRlcnVuZywgVmVydGVpbHVuZyB1bmQvb2RlciBQdWJsaWthdGlvbiBkaWVzZXIgRS1NYWls
IGlzdCBzdHJlbmdzdGVucyB1bnRlcnNhZ3QuDQpUaGUgY29udGVudHMgb2YgdGhlIGFib3Zl
IG1lbnRpb25lZCBlLW1haWwgaXMgbm90IGxlZ2FsbHkgYmluZGluZy4gVGhpcyBlLW1haWwg
Y29udGFpbnMgY29uZmlkZW50aWFsIGFuZC9vciBsZWdhbGx5IHByb3RlY3RlZCBpbmZvcm1h
dGlvbi4gUGxlYXNlIGluZm9ybSB1cyBpZiB5b3UgaGF2ZSByZWNlaXZlZCB0aGlzIGUtbWFp
bCBieSBtaXN0YWtlIGFuZCBkZWxldGUgaXQgaW4gc3VjaCBhIGNhc2UuIEVhY2ggdW5hdXRo
b3JpemVkIHJlcHJvZHVjdGlvbiwgZGlzY2xvc3VyZSwgYWx0ZXJhdGlvbiwgZGlzdHJpYnV0
aW9uIGFuZC9vciBwdWJsaWNhdGlvbiBvZiB0aGlzIGUtbWFpbCBpcyBzdHJpY3RseSBwcm9o
aWJpdGVkLg==

Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[Rob Herring]

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

Re: [PATCH 4/4] rtc: isl1208: add support for isl1219 with hwmon for tamper detection

[Rob Herring]

On Tue, Jan 30, 2018 at 8:41 AM, Rob Herring <robh@kernel.org> wrote:
> 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.

[...]

>>> 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