* [PATCH v3 4/4] rtc: isl1208: Add "evdet" interrupt source for isl1219.
2018-03-05 10:43 [PATCH v3 0/4] rtc: isl1208: fixes, documentation and isl1219 support Denis OSTERLAND
@ 2018-03-05 10:43 ` Denis OSTERLAND
2018-03-05 10:43 ` [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper detection Denis OSTERLAND
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Denis OSTERLAND @ 2018-03-05 10:43 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: linux-kernel, mgr, devicetree, linux, jdelvare, linux-rtc,
kernel, Denis OSTERLAND
From: Denis Osterland <Denis.Osterland@diehl.com>
Add support for "evdet" named interrupt source.
The check if i2c client irq matches evdet irq is needed
for the case that there is only one interrupt named "evdet".
In this case i2c client code handles this like an unnamed
interrupt souce and assigns the value.
Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
Reviewed-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
drivers/rtc/rtc-isl1208.c | 47 +++++++++++++++++++++++++++++++----------------
1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index 164371b..f2f148b 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -14,6 +14,7 @@
#include <linux/i2c.h>
#include <linux/bcd.h>
#include <linux/rtc.h>
+#include <linux/of_irq.h>
/* Register map */
/* rtc section */
@@ -79,6 +80,7 @@ enum {
struct isl1208 {
struct rtc_device *rtc;
const struct attribute_group *sysfs_files;
+ int evdet_irq;
};
/* block read */
@@ -730,6 +732,24 @@ static const struct attribute_group isl1219_rtc_sysfs_files = {
.attrs = isl1219_rtc_attrs,
};
+static int isl1208_setup_irq(struct i2c_client *client, int irq)
+{
+ int rc = devm_request_threaded_irq(&client->dev, 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(irq);
+ } else {
+ dev_err(&client->dev,
+ "Unable to request irq %d, no alarm support\n",
+ irq);
+ }
+ return rc;
+}
+
static int
isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
@@ -772,6 +792,8 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
return rc;
}
isl1208->sysfs_files = &isl1219_rtc_sysfs_files;
+ isl1208->evdet_irq = of_irq_get_byname(client->dev.of_node,
+ "evdet");
} else {
isl1208->sysfs_files = &isl1208_rtc_sysfs_files;
}
@@ -780,22 +802,15 @@ 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;
- }
- }
+ if (client->irq > 0)
+ rc = isl1208_setup_irq(client, client->irq);
+ if (rc)
+ return rc;
+
+ if (isl1208->evdet_irq > 0 && isl1208->evdet_irq != client->irq)
+ rc = isl1208_setup_irq(client, isl1208->evdet_irq);
+ if (rc)
+ return rc;
return rtc_register_device(isl1208->rtc);
}
--
2.7.4
Diehl AKO Stiftung & Co. KG, Pfannerstraße 75-83, 88239 Wangen im Allgäu
Bereichsvorstand: Dr.-Ing. Michael Siedentop (Sprecher), Josef Fellner (Mitglied)
Sitz der Gesellschaft: Wangen i.A. – Registergericht: Amtsgericht Ulm HRA 620609 – Persönlich haftende Gesellschafterin: Diehl Verwaltungs-Stiftung – Sitz: Nürnberg – Registergericht: Amtsgericht Nürnberg HRA 11756 –
Vorstand: Dr.-Ing. E.h. Thomas Diehl (†) (Vorsitzender), Herr Dipl.-Wirtsch.-Ing. Wolfgang Weggen (stellvertretender Vorsitzender), Dipl.-Kfm. Claus Günther, Dipl.-Kfm. Frank Gutzeit, Dr.-Ing. Heinrich Schunk, Dr.-Ing. Michael Siedentop , Dipl.-Kfm. Dr.-Ing. Martin Sommer, Dipl.-Ing. (FH) Rainer von Borstel, Vorsitzender des Aufsichtsrates: Dr. Klaus Maier
___________________________________________________________________________________________________
Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht. Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper detection
2018-03-05 10:43 [PATCH v3 0/4] rtc: isl1208: fixes, documentation and isl1219 support Denis OSTERLAND
2018-03-05 10:43 ` [PATCH v3 4/4] rtc: isl1208: Add "evdet" interrupt source for isl1219 Denis OSTERLAND
@ 2018-03-05 10:43 ` Denis OSTERLAND
2018-03-06 20:42 ` Alexandre Belloni
2018-03-07 22:02 ` Rob Herring
2018-03-05 10:43 ` [PATCH v3 2/4] rtc: isl1208: switch to rtc_register_device Denis OSTERLAND
2018-03-05 10:43 ` [PATCH v3 1/4] rtc: isl1208: enable interrupt after context preparation Denis OSTERLAND
3 siblings, 2 replies; 13+ messages in thread
From: Denis OSTERLAND @ 2018-03-05 10:43 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: linux-kernel, mgr, m.grzeschik, devicetree, linux, jdelvare,
linux-rtc, kernel, Denis OSTERLAND
From: Michael Grzeschik <m.grzeschik@pengutronix.de>
We add support for the ISL1219 chip that got an integrated tamper
detection function. This patch implements the feature by adding
an additional timestamp0 file to sysfs device path.
This file contains seconds since epoch, if an event occurred,
or is empty, if none occurred.
The devicetree documentation for the ISL1219 device tree
binding is added with an short example. It is not a trivial
device, because it supports two interrupt souces.
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
---
.../devicetree/bindings/rtc/isil,isl1219.txt | 28 ++++
drivers/rtc/rtc-isl1208.c | 160 ++++++++++++++++++---
2 files changed, 171 insertions(+), 17 deletions(-)
create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl1219.txt
diff --git a/Documentation/devicetree/bindings/rtc/isil,isl1219.txt b/Documentation/devicetree/bindings/rtc/isil,isl1219.txt
new file mode 100644
index 0000000..7937c13
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/isil,isl1219.txt
@@ -0,0 +1,28 @@
+Intersil ISL1219 I2C RTC/Alarm chip with event in
+
+ISL1219 has additional pins EVIN and #EVDET for tamper detection.
+
+Required properties supported by the device:
+
+ - "compatible": must be "isil,isl1219"
+ - "reg": I2C bus address of the device
+
+Optional properties:
+
+ - "interrupt-names": list which may contains "irq" and "evdet"
+ - "interrupt-parent", "interrupts", "interrupts-extended":
+ for passing the interrupt line of the SoC connected to #IRQ pin
+ and #EVDET pin of the RTC chip.
+
+
+Example isl1219 node with #IRQ pin connected to SoC gpio1 pin12
+ and #EVDET pin connected to SoC gpio2 pin 24:
+
+ isl1219: rtc@68 {
+ compatible = "isil,isl1219";
+ reg = <0x68>;
+ interrupt-names = "irq", "evdet";
+ interrupts-extended = <&gpio1 12 IRQ_TYPE_EDGE_FALLING>,
+ <&gpio2 24 IRQ_TYPE_EDGE_FALLING>;
+ };
+
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index 1a2c38c..164371b 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -33,6 +33,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 +58,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;
+ const struct attribute_group *sysfs_files;
+};
+
/* block read */
static int
isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
@@ -80,8 +102,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 +126,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 +515,78 @@ 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_rtc_event_clear(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ int sr;
+
+ 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_rtc_event_show_timestamp(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct i2c_client *client = to_i2c_client(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 sprintf(buf, "\n");
+
+ 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, "%lld\n", (long long) tv64.tv_sec);
+};
+
+static DEVICE_ATTR(timestamp0, 0640,
+ isl1208_rtc_event_show_timestamp, isl1208_rtc_event_clear);
+
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 +609,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 +626,13 @@ isl1208_rtc_interrupt(int irq, void *data)
return err;
}
+ if (sr & ISL1208_REG_SR_EVT) {
+ sysfs_notify(&client->dev.kobj, NULL,
+ dev_attr_timestamp0.attr.name);
+ dev_warn(&client->dev, "event detected");
+ handled = 1;
+ }
+
return handled ? IRQ_HANDLED : IRQ_NONE;
}
@@ -623,11 +718,23 @@ static const struct attribute_group isl1208_rtc_sysfs_files = {
.attrs = isl1208_rtc_attrs,
};
+static struct attribute *isl1219_rtc_attrs[] = {
+ &dev_attr_atrim.attr,
+ &dev_attr_dtrim.attr,
+ &dev_attr_usr.attr,
+ &dev_attr_timestamp0.attr,
+ NULL
+};
+
+static const struct attribute_group isl1219_rtc_sysfs_files = {
+ .attrs = isl1219_rtc_attrs,
+};
+
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 +742,18 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
if (isl1208_i2c_validate_client(client) < 0)
return -ENODEV;
- rtc = devm_rtc_allocate_device(&client->dev);
- if (IS_ERR(rtc))
- return PTR_ERR(rtc);
+ isl1208 = devm_kzalloc(&client->dev, sizeof(struct isl1208),
+ GFP_KERNEL);
+ if (!isl1208)
+ return -ENOMEM;
- rtc->ops = &isl1208_rtc_ops;
+ isl1208->rtc = devm_rtc_allocate_device(&client->dev);
+ if (IS_ERR(isl1208->rtc))
+ return PTR_ERR(isl1208->rtc);
- i2c_set_clientdata(client, rtc);
+ isl1208->rtc->ops = &isl1208_rtc_ops;
+
+ i2c_set_clientdata(client, isl1208);
rc = isl1208_i2c_get_sr(client);
if (rc < 0) {
@@ -653,7 +765,18 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
dev_warn(&client->dev, "rtc power failure detected, "
"please set clock.\n");
- rc = sysfs_create_group(&client->dev.kobj, &isl1208_rtc_sysfs_files);
+ 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->sysfs_files = &isl1219_rtc_sysfs_files;
+ } else {
+ isl1208->sysfs_files = &isl1208_rtc_sysfs_files;
+ }
+
+ rc = sysfs_create_group(&client->dev.kobj, isl1208->sysfs_files);
if (rc)
return rc;
@@ -674,20 +797,23 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
}
}
- return rtc_register_device(rtc);
+ return rtc_register_device(isl1208->rtc);
}
static int
isl1208_remove(struct i2c_client *client)
{
- sysfs_remove_group(&client->dev.kobj, &isl1208_rtc_sysfs_files);
+ struct isl1208 *isl1208 = i2c_get_clientdata(client);
+
+ sysfs_remove_group(&client->dev.kobj, isl1208->sysfs_files);
return 0;
}
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);
--
2.7.4
Diehl AKO Stiftung & Co. KG, Pfannerstraße 75-83, 88239 Wangen im Allgäu
Bereichsvorstand: Dr.-Ing. Michael Siedentop (Sprecher), Josef Fellner (Mitglied)
Sitz der Gesellschaft: Wangen i.A. – Registergericht: Amtsgericht Ulm HRA 620609 – Persönlich haftende Gesellschafterin: Diehl Verwaltungs-Stiftung – Sitz: Nürnberg – Registergericht: Amtsgericht Nürnberg HRA 11756 –
Vorstand: Dr.-Ing. E.h. Thomas Diehl (†) (Vorsitzender), Herr Dipl.-Wirtsch.-Ing. Wolfgang Weggen (stellvertretender Vorsitzender), Dipl.-Kfm. Claus Günther, Dipl.-Kfm. Frank Gutzeit, Dr.-Ing. Heinrich Schunk, Dr.-Ing. Michael Siedentop , Dipl.-Kfm. Dr.-Ing. Martin Sommer, Dipl.-Ing. (FH) Rainer von Borstel, Vorsitzender des Aufsichtsrates: Dr. Klaus Maier
___________________________________________________________________________________________________
Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht. Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper detection
2018-03-05 10:43 ` [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper detection Denis OSTERLAND
@ 2018-03-06 20:42 ` Alexandre Belloni
2018-03-07 8:19 ` Denis OSTERLAND
2018-03-07 22:02 ` Rob Herring
1 sibling, 1 reply; 13+ messages in thread
From: Alexandre Belloni @ 2018-03-06 20:42 UTC (permalink / raw)
To: Denis OSTERLAND
Cc: a.zummo, linux-kernel, mgr, m.grzeschik, devicetree, linux,
jdelvare, linux-rtc, kernel
On 05/03/2018 at 10:43:52 +0000, Denis OSTERLAND wrote:
> diff --git a/Documentation/devicetree/bindings/rtc/isil,isl1219.txt b/Documentation/devicetree/bindings/rtc/isil,isl1219.txt
> new file mode 100644
> index 0000000..7937c13
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/isil,isl1219.txt
If you want that file to be reviewed by Rob (DT maintainer), you should
probably separate it from that patch and copy his email. The bindings
seem fine to me though.
> diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> index 1a2c38c..164371b 100644
> --- a/drivers/rtc/rtc-isl1208.c
> +++ b/drivers/rtc/rtc-isl1208.c
> @@ -33,6 +33,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 +58,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
> +
Because they are not available on ISL1208, maybe it would be better to
prefix them with ISL1219.
> +
> + tv64.tv_sec = rtc_tm_to_time64(&tm);
Why not using an unsigned long long directly here? time64_t is not the
correct type.
> +
> + return sprintf(buf, "%lld\n", (long long) tv64.tv_sec);
And this should become %llu
> +};
> +
> +static DEVICE_ATTR(timestamp0, 0640,
Shouldn't the permissions be 644?
> + isl1208_rtc_event_show_timestamp, isl1208_rtc_event_clear);
> +
> 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 +609,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 +626,13 @@ isl1208_rtc_interrupt(int irq, void *data)
> return err;
> }
>
> + if (sr & ISL1208_REG_SR_EVT) {
> + sysfs_notify(&client->dev.kobj, NULL,
> + dev_attr_timestamp0.attr.name);
> + dev_warn(&client->dev, "event detected");
> + handled = 1;
> + }
> +
> return handled ? IRQ_HANDLED : IRQ_NONE;
> }
>
> @@ -623,11 +718,23 @@ static const struct attribute_group isl1208_rtc_sysfs_files = {
> .attrs = isl1208_rtc_attrs,
> };
>
> +static struct attribute *isl1219_rtc_attrs[] = {
> + &dev_attr_atrim.attr,
> + &dev_attr_dtrim.attr,
> + &dev_attr_usr.attr,
> + &dev_attr_timestamp0.attr,
> + NULL
> +};
> +
> +static const struct attribute_group isl1219_rtc_sysfs_files = {
> + .attrs = isl1219_rtc_attrs,
> +};
> +
> 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 +742,18 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> if (isl1208_i2c_validate_client(client) < 0)
> return -ENODEV;
>
> - rtc = devm_rtc_allocate_device(&client->dev);
> - if (IS_ERR(rtc))
> - return PTR_ERR(rtc);
> + isl1208 = devm_kzalloc(&client->dev, sizeof(struct isl1208),
> + GFP_KERNEL);
> + if (!isl1208)
> + return -ENOMEM;
>
> - rtc->ops = &isl1208_rtc_ops;
> + isl1208->rtc = devm_rtc_allocate_device(&client->dev);
> + if (IS_ERR(isl1208->rtc))
> + return PTR_ERR(isl1208->rtc);
>
> - i2c_set_clientdata(client, rtc);
> + isl1208->rtc->ops = &isl1208_rtc_ops;
> +
> + i2c_set_clientdata(client, isl1208);
>
> rc = isl1208_i2c_get_sr(client);
> if (rc < 0) {
> @@ -653,7 +765,18 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> dev_warn(&client->dev, "rtc power failure detected, "
> "please set clock.\n");
>
> - rc = sysfs_create_group(&client->dev.kobj, &isl1208_rtc_sysfs_files);
> + 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->sysfs_files = &isl1219_rtc_sysfs_files;
> + } else {
> + isl1208->sysfs_files = &isl1208_rtc_sysfs_files;
> + }
> +
I don't think the whole isl1208 is necessary. You should probably use
the .is_visible callback of isl1219_rtc_sysfs_files. This will make the
changelog quite smaller.
> + rc = sysfs_create_group(&client->dev.kobj, isl1208->sysfs_files);
> if (rc)
> return rc;
>
> @@ -674,20 +797,23 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> }
> }
>
> - return rtc_register_device(rtc);
> + return rtc_register_device(isl1208->rtc);
> }
>
> static int
> isl1208_remove(struct i2c_client *client)
> {
> - sysfs_remove_group(&client->dev.kobj, &isl1208_rtc_sysfs_files);
> + struct isl1208 *isl1208 = i2c_get_clientdata(client);
> +
> + sysfs_remove_group(&client->dev.kobj, isl1208->sysfs_files);
>
> return 0;
> }
>
> 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);
> --
> 2.7.4
>
>
> Diehl AKO Stiftung & Co. KG, Pfannerstraße 75-83, 88239 Wangen im Allgäu
> Bereichsvorstand: Dr.-Ing. Michael Siedentop (Sprecher), Josef Fellner (Mitglied)
> Sitz der Gesellschaft: Wangen i.A. – Registergericht: Amtsgericht Ulm HRA 620609 – Persönlich haftende Gesellschafterin: Diehl Verwaltungs-Stiftung – Sitz: Nürnberg – Registergericht: Amtsgericht Nürnberg HRA 11756 –
> Vorstand: Dr.-Ing. E.h. Thomas Diehl (†) (Vorsitzender), Herr Dipl.-Wirtsch.-Ing. Wolfgang Weggen (stellvertretender Vorsitzender), Dipl.-Kfm. Claus Günther, Dipl.-Kfm. Frank Gutzeit, Dr.-Ing. Heinrich Schunk, Dr.-Ing. Michael Siedentop , Dipl.-Kfm. Dr.-Ing. Martin Sommer, Dipl.-Ing. (FH) Rainer von Borstel, Vorsitzender des Aufsichtsrates: Dr. Klaus Maier
> ___________________________________________________________________________________________________
> Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
> Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht. Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
> The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper detection
2018-03-06 20:42 ` Alexandre Belloni
@ 2018-03-07 8:19 ` Denis OSTERLAND
2018-03-07 10:47 ` Alexandre Belloni
0 siblings, 1 reply; 13+ messages in thread
From: Denis OSTERLAND @ 2018-03-07 8:19 UTC (permalink / raw)
To: alexandre.belloni
Cc: linux-kernel, mgr, m.grzeschik, devicetree, a.zummo, linux,
jdelvare, linux-rtc, kernel
Am Dienstag, den 06.03.2018, 21:42 +0100 schrieb Alexandre Belloni:
> On 05/03/2018 at 10:43:52 +0000, Denis OSTERLAND wrote:
> >
> > diff --git a/Documentation/devicetree/bindings/rtc/isil,isl1219.txt b/Documentation/devicetree/bindings/rtc/isil,isl1219.txt
> > new file mode 100644
> > index 0000000..7937c13
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/isil,isl1219.txt
> If you want that file to be reviewed by Rob (DT maintainer), you should
> probably separate it from that patch and copy his email. The bindings
> seem fine to me though.
OK
>
> >
> > diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> > index 1a2c38c..164371b 100644
> > --- a/drivers/rtc/rtc-isl1208.c
> > +++ b/drivers/rtc/rtc-isl1208.c
> > @@ -33,6 +33,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 +58,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
> > +
> Because they are not available on ISL1208, maybe it would be better to
> prefix them with ISL1219.
I see. Yes, this would clarify that they are only available on isl1219.
Shall we rename isl1208_rtc_event_show_timestamp/isl1208_rtc_event_clear
to isl1219_rtc_event_show_timestamp/isl1219_rtc_event_clear, too?
>
> >
> > +
> > + tv64.tv_sec = rtc_tm_to_time64(&tm);
> Why not using an unsigned long long directly here? time64_t is not the
> correct type.
Do you mean timespec64 is not the correct type here?
Then yes, sould be time64_t.
If you mean time64_t is not the correct type here,
then can you give me some detail why there is no rtc_tm_to_u64,
or something like that?
sprintf(buf, "%lld\n", rtc_tm_to_time64(&tm)) seems correct to me.
By the way, is it needed to check for seconds < 0 and return error?
>
> >
> > +
> > + return sprintf(buf, "%lld\n", (long long) tv64.tv_sec);
> And this should become %llu
>
> >
> > +};
> > +
> > +static DEVICE_ATTR(timestamp0, 0640,
> Shouldn't the permissions be 644?
644 is OK
>
> >
> > + isl1208_rtc_event_show_timestamp, isl1208_rtc_event_clear);
> > +
> > 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 +609,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 +626,13 @@ isl1208_rtc_interrupt(int irq, void *data)
> > return err;
> > }
> >
> > + if (sr & ISL1208_REG_SR_EVT) {
> > + sysfs_notify(&client->dev.kobj, NULL,
> > + dev_attr_timestamp0.attr.name);
> > + dev_warn(&client->dev, "event detected");
> > + handled = 1;
> > + }
> > +
> > return handled ? IRQ_HANDLED : IRQ_NONE;
> > }
> >
> > @@ -623,11 +718,23 @@ static const struct attribute_group isl1208_rtc_sysfs_files = {
> > .attrs = isl1208_rtc_attrs,
> > };
> >
> > +static struct attribute *isl1219_rtc_attrs[] = {
> > + &dev_attr_atrim.attr,
> > + &dev_attr_dtrim.attr,
> > + &dev_attr_usr.attr,
> > + &dev_attr_timestamp0.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group isl1219_rtc_sysfs_files = {
> > + .attrs = isl1219_rtc_attrs,
> > +};
> > +
> > 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 +742,18 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > if (isl1208_i2c_validate_client(client) < 0)
> > return -ENODEV;
> >
> > - rtc = devm_rtc_allocate_device(&client->dev);
> > - if (IS_ERR(rtc))
> > - return PTR_ERR(rtc);
> > + isl1208 = devm_kzalloc(&client->dev, sizeof(struct isl1208),
> > + GFP_KERNEL);
> > + if (!isl1208)
> > + return -ENOMEM;
> >
> > - rtc->ops = &isl1208_rtc_ops;
> > + isl1208->rtc = devm_rtc_allocate_device(&client->dev);
> > + if (IS_ERR(isl1208->rtc))
> > + return PTR_ERR(isl1208->rtc);
> >
> > - i2c_set_clientdata(client, rtc);
> > + isl1208->rtc->ops = &isl1208_rtc_ops;
> > +
> > + i2c_set_clientdata(client, isl1208);
> >
> > rc = isl1208_i2c_get_sr(client);
> > if (rc < 0) {
> > @@ -653,7 +765,18 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > dev_warn(&client->dev, "rtc power failure detected, "
> > "please set clock.\n");
> >
> > - rc = sysfs_create_group(&client->dev.kobj, &isl1208_rtc_sysfs_files);
> > + 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->sysfs_files = &isl1219_rtc_sysfs_files;
> > + } else {
> > + isl1208->sysfs_files = &isl1208_rtc_sysfs_files;
> > + }
> > +
> I don't think the whole isl1208 is necessary. You should probably use
> the .is_visible callback of isl1219_rtc_sysfs_files. This will make the
> changelog quite smaller.
>
Well, I don´t know how to access i2c_device_id from kobject.
rtc_attr_is_visible shows how to convert kobject to device and rtc_device,
but how to do (id->driver_data == TYPE_ISL1219) here?
> >
> > + rc = sysfs_create_group(&client->dev.kobj, isl1208->sysfs_files);
> > if (rc)
> > return rc;
> >
> > @@ -674,20 +797,23 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > }
> > }
> >
> > - return rtc_register_device(rtc);
> > + return rtc_register_device(isl1208->rtc);
> > }
> >
> > static int
> > isl1208_remove(struct i2c_client *client)
> > {
> > - sysfs_remove_group(&client->dev.kobj, &isl1208_rtc_sysfs_files);
> > + struct isl1208 *isl1208 = i2c_get_clientdata(client);
> > +
> > + sysfs_remove_group(&client->dev.kobj, isl1208->sysfs_files);
> >
> > return 0;
> > }
> >
> > 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);
> > --
> > 2.7.4
> >
> >
> > Diehl AKO Stiftung & Co. KG, Pfannerstraße 75-83, 88239 Wangen im Allgäu
> > Bereichsvorstand: Dr.-Ing. Michael Siedentop (Sprecher), Josef Fellner (Mitglied)
> > Sitz der Gesellschaft: Wangen i.A. – Registergericht: Amtsgericht Ulm HRA 620609 – Persönlich haftende Gesellschafterin: Diehl Verwaltungs-Stiftung – Sitz: Nürnberg – Registergericht: Amtsgericht
> > Nürnberg HRA 11756 –
> > Vorstand: Dr.-Ing. E.h. Thomas Diehl (†) (Vorsitzender), Herr Dipl.-Wirtsch.-Ing. Wolfgang Weggen (stellvertretender Vorsitzender), Dipl.-Kfm. Claus Günther, Dipl.-Kfm. Frank Gutzeit, Dr.-Ing.
> > Heinrich Schunk, Dr.-Ing. Michael Siedentop , Dipl.-Kfm. Dr.-Ing. Martin Sommer, Dipl.-Ing. (FH) Rainer von Borstel, Vorsitzender des Aufsichtsrates: Dr. Klaus Maier
> > ___________________________________________________________________________________________________
> > Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
> > Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht. Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung,
> > Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
> > The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by
> > mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.
Diehl AKO Stiftung & Co. KG, Pfannerstraße 75-83, 88239 Wangen im Allgäu
Bereichsvorstand: Dr.-Ing. Michael Siedentop (Sprecher), Josef Fellner (Mitglied)
Sitz der Gesellschaft: Wangen i.A. – Registergericht: Amtsgericht Ulm HRA 620609 – Persönlich haftende Gesellschafterin: Diehl Verwaltungs-Stiftung – Sitz: Nürnberg – Registergericht: Amtsgericht Nürnberg HRA 11756 –
Vorstand: Dr.-Ing. E.h. Thomas Diehl (†) (Vorsitzender), Herr Dipl.-Wirtsch.-Ing. Wolfgang Weggen (stellvertretender Vorsitzender), Dipl.-Kfm. Claus Günther, Dipl.-Kfm. Frank Gutzeit, Dr.-Ing. Heinrich Schunk, Dr.-Ing. Michael Siedentop , Dipl.-Kfm. Dr.-Ing. Martin Sommer, Dipl.-Ing. (FH) Rainer von Borstel, Vorsitzender des Aufsichtsrates: Dr. Klaus Maier
___________________________________________________________________________________________________
Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht. Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper detection
2018-03-07 8:19 ` Denis OSTERLAND
@ 2018-03-07 10:47 ` Alexandre Belloni
2018-03-08 11:53 ` Denis OSTERLAND
0 siblings, 1 reply; 13+ messages in thread
From: Alexandre Belloni @ 2018-03-07 10:47 UTC (permalink / raw)
To: Denis OSTERLAND
Cc: linux-kernel, mgr, m.grzeschik, devicetree, a.zummo, linux,
jdelvare, linux-rtc, kernel
On 07/03/2018 at 08:19:15 +0000, Denis OSTERLAND wrote:
> > > +/* 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
> > > +
> > Because they are not available on ISL1208, maybe it would be better to
> > prefix them with ISL1219.
> I see. Yes, this would clarify that they are only available on isl1219.
> Shall we rename isl1208_rtc_event_show_timestamp/isl1208_rtc_event_clear
> to isl1219_rtc_event_show_timestamp/isl1219_rtc_event_clear, too?
That could be done too, yes.
> >
> > >
> > > +
> > > + tv64.tv_sec = rtc_tm_to_time64(&tm);
> > Why not using an unsigned long long directly here? time64_t is not the
> > correct type.
> Do you mean timespec64 is not the correct type here?
> Then yes, sould be time64_t.
> If you mean time64_t is not the correct type here,
> then can you give me some detail why there is no rtc_tm_to_u64,
> or something like that?
The rtc subsystem forbids negative times, the proper type should be
unsigned.
> sprintf(buf, "%lld\n", rtc_tm_to_time64(&tm)) seems correct to me.
> By the way, is it needed to check for seconds < 0 and return error?
Indeed, you shoud check the tm with rtc_valid_tm before calling
rtc_tm_to_time64.
> > > - rc = sysfs_create_group(&client->dev.kobj, &isl1208_rtc_sysfs_files);
> > > + 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->sysfs_files = &isl1219_rtc_sysfs_files;
> > > + } else {
> > > + isl1208->sysfs_files = &isl1208_rtc_sysfs_files;
> > > + }
> > > +
> > I don't think the whole isl1208 is necessary. You should probably use
> > the .is_visible callback of isl1219_rtc_sysfs_files. This will make the
> > changelog quite smaller.
> >
> Well, I don´t know how to access i2c_device_id from kobject.
> rtc_attr_is_visible shows how to convert kobject to device and rtc_device,
> but how to do (id->driver_data == TYPE_ISL1219) here?
I'd use i2c_set_clientdata/i2c_get_clientdata but I agree that then it
is basically the same as having isl1208->sysfs_files.
but this makes me realize that the timestamp file doesn't end up at the
correct location. What you do now is placing it under the i2c device
while it should be placed under the rtc device (i.e. in
/sys/class/rtc/rtcX/). This was a mistake made back in 2006.
I guess you'll have to add a new group instead of adding to the current
one.
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper detection
2018-03-07 10:47 ` Alexandre Belloni
@ 2018-03-08 11:53 ` Denis OSTERLAND
2018-03-08 12:05 ` Alexandre Belloni
0 siblings, 1 reply; 13+ messages in thread
From: Denis OSTERLAND @ 2018-03-08 11:53 UTC (permalink / raw)
To: alexandre.belloni
Cc: linux-kernel, mgr, m.grzeschik, devicetree, a.zummo, linux,
jdelvare, linux-rtc, kernel
Am Mittwoch, den 07.03.2018, 11:47 +0100 schrieb Alexandre Belloni:
> > > > +
> > > > + tv64.tv_sec = rtc_tm_to_time64(&tm);
> > > Why not using an unsigned long long directly here? time64_t is not the
> > > correct type.
> > Do you mean timespec64 is not the correct type here?
> > Then yes, sould be time64_t.
> > If you mean time64_t is not the correct type here,
> > then can you give me some detail why there is no rtc_tm_to_u64,
> > or something like that?
> The rtc subsystem forbids negative times, the proper type should be
> unsigned.
I will add rtc_vaild_tm check.
Which sequence for time conversion would you expect?
time64_t secs = rtc_tm_to_time64(&tm);
BUG_ON(secs < 0);
return sprintf(buf, "%llu\n", (unsigned long long)secs);
or
return sprintf(buf, "%llu\n", (unsigned long long)rtc_tm_to_time64(&tm));
>
> >
> > sprintf(buf, "%lld\n", rtc_tm_to_time64(&tm)) seems correct to me.
> > By the way, is it needed to check for seconds < 0 and return error?
> Indeed, you shoud check the tm with rtc_valid_tm before calling
> rtc_tm_to_time64.
>
> >
> > >
> > > >
> > > > - rc = sysfs_create_group(&client->dev.kobj, &isl1208_rtc_sysfs_files);
> > > > + 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->sysfs_files = &isl1219_rtc_sysfs_files;
> > > > + } else {
> > > > + isl1208->sysfs_files = &isl1208_rtc_sysfs_files;
> > > > + }
> > > > +
> > > I don't think the whole isl1208 is necessary. You should probably use
> > > the .is_visible callback of isl1219_rtc_sysfs_files. This will make the
> > > changelog quite smaller.
> > >
> > Well, I don´t know how to access i2c_device_id from kobject.
> > rtc_attr_is_visible shows how to convert kobject to device and rtc_device,
> > but how to do (id->driver_data == TYPE_ISL1219) here?
> I'd use i2c_set_clientdata/i2c_get_clientdata but I agree that then it
> is basically the same as having isl1208->sysfs_files.
>
> but this makes me realize that the timestamp file doesn't end up at the
> correct location. What you do now is placing it under the i2c device
> while it should be placed under the rtc device (i.e. in
> /sys/class/rtc/rtcX/). This was a mistake made back in 2006.
>
> I guess you'll have to add a new group instead of adding to the current
> one.
I guess I found a way to do it.
static struct attribute *isl1219_rtc_attrs[] = {
&dev_attr_timestamp0.attr,
NULL
};
in probe
if (id->driver_data == TYPE_ISL1219) {
sysfs_merge_group(&rtc->kobj, &isl1219_rtc_sysfs_files);
in remove
struct rtc_device *rtc = i2c_get_clientdata(client);
sysfs_unmerge_group(&rtc->kobj, &isl1219_rtc_sysfs_files);
As far as I got it, I can call unmerge even if group was not merged before.
If it works I don´t need struct isl1208 at all.
>
Diehl AKO Stiftung & Co. KG, Pfannerstraße 75-83, 88239 Wangen im Allgäu
Bereichsvorstand: Dr.-Ing. Michael Siedentop (Sprecher), Josef Fellner (Mitglied)
Sitz der Gesellschaft: Wangen i.A. – Registergericht: Amtsgericht Ulm HRA 620609 – Persönlich haftende Gesellschafterin: Diehl Verwaltungs-Stiftung – Sitz: Nürnberg – Registergericht: Amtsgericht Nürnberg HRA 11756 –
Vorstand: Dr.-Ing. E.h. Thomas Diehl (†) (Vorsitzender), Herr Dipl.-Wirtsch.-Ing. Wolfgang Weggen (stellvertretender Vorsitzender), Dipl.-Kfm. Claus Günther, Dipl.-Kfm. Frank Gutzeit, Dr.-Ing. Heinrich Schunk, Dr.-Ing. Michael Siedentop , Dipl.-Kfm. Dr.-Ing. Martin Sommer, Dipl.-Ing. (FH) Rainer von Borstel, Vorsitzender des Aufsichtsrates: Dr. Klaus Maier
___________________________________________________________________________________________________
Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht. Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper detection
2018-03-08 11:53 ` Denis OSTERLAND
@ 2018-03-08 12:05 ` Alexandre Belloni
0 siblings, 0 replies; 13+ messages in thread
From: Alexandre Belloni @ 2018-03-08 12:05 UTC (permalink / raw)
To: Denis OSTERLAND
Cc: linux-kernel, mgr, m.grzeschik, devicetree, a.zummo, linux,
jdelvare, linux-rtc, kernel
On 08/03/2018 at 11:53:09 +0000, Denis OSTERLAND wrote:
> Am Mittwoch, den 07.03.2018, 11:47 +0100 schrieb Alexandre Belloni:
> > > > > +
> > > > > + tv64.tv_sec = rtc_tm_to_time64(&tm);
> > > > Why not using an unsigned long long directly here? time64_t is not the
> > > > correct type.
> > > Do you mean timespec64 is not the correct type here?
> > > Then yes, sould be time64_t.
> > > If you mean time64_t is not the correct type here,
> > > then can you give me some detail why there is no rtc_tm_to_u64,
> > > or something like that?
> > The rtc subsystem forbids negative times, the proper type should be
> > unsigned.
> I will add rtc_vaild_tm check.
>
> Which sequence for time conversion would you expect?
>
> time64_t secs = rtc_tm_to_time64(&tm);
> BUG_ON(secs < 0);
> return sprintf(buf, "%llu\n", (unsigned long long)secs);
>
> or
>
> return sprintf(buf, "%llu\n", (unsigned long long)rtc_tm_to_time64(&tm));
rtc_vaild_tm will already return EINVAL in case of negative time so this
is the one you should use.
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper detection
2018-03-05 10:43 ` [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper detection Denis OSTERLAND
2018-03-06 20:42 ` Alexandre Belloni
@ 2018-03-07 22:02 ` Rob Herring
1 sibling, 0 replies; 13+ messages in thread
From: Rob Herring @ 2018-03-07 22:02 UTC (permalink / raw)
To: Denis OSTERLAND
Cc: a.zummo, alexandre.belloni, linux-kernel, mgr, m.grzeschik,
devicetree, linux, jdelvare, linux-rtc, kernel
On Mon, Mar 05, 2018 at 10:43:52AM +0000, Denis OSTERLAND wrote:
> From: Michael Grzeschik <m.grzeschik@pengutronix.de>
>
> We add support for the ISL1219 chip that got an integrated tamper
> detection function. This patch implements the feature by adding
> an additional timestamp0 file to sysfs device path.
> This file contains seconds since epoch, if an event occurred,
> or is empty, if none occurred.
>
> The devicetree documentation for the ISL1219 device tree
> binding is added with an short example. It is not a trivial
> device, because it supports two interrupt souces.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
> ---
> .../devicetree/bindings/rtc/isil,isl1219.txt | 28 ++++
While preferred to separate, I'm not going to ask for that if there are
no other changes on the binding.
Reviewed-by: Rob Herring <robh@kernel.org>
> drivers/rtc/rtc-isl1208.c | 160 ++++++++++++++++++---
> 2 files changed, 171 insertions(+), 17 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl1219.txt
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 2/4] rtc: isl1208: switch to rtc_register_device
2018-03-05 10:43 [PATCH v3 0/4] rtc: isl1208: fixes, documentation and isl1219 support Denis OSTERLAND
2018-03-05 10:43 ` [PATCH v3 4/4] rtc: isl1208: Add "evdet" interrupt source for isl1219 Denis OSTERLAND
2018-03-05 10:43 ` [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper detection Denis OSTERLAND
@ 2018-03-05 10:43 ` Denis OSTERLAND
2018-03-06 20:20 ` Alexandre Belloni
2018-03-05 10:43 ` [PATCH v3 1/4] rtc: isl1208: enable interrupt after context preparation Denis OSTERLAND
3 siblings, 1 reply; 13+ messages in thread
From: Denis OSTERLAND @ 2018-03-05 10:43 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: linux-kernel, mgr, devicetree, linux, jdelvare, linux-rtc,
kernel, Denis OSTERLAND
From: Denis Osterland <Denis.Osterland@diehl.com>
Fix possible race condition.
It is not allowed to return with an error code after RTC is registered.
Suggested-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
Reviewed-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
drivers/rtc/rtc-isl1208.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index a13a4ba..1a2c38c 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -635,12 +635,12 @@ 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_rtc_ops,
- THIS_MODULE);
+ rtc = devm_rtc_allocate_device(&client->dev);
if (IS_ERR(rtc))
return PTR_ERR(rtc);
+ rtc->ops = &isl1208_rtc_ops;
+
i2c_set_clientdata(client, rtc);
rc = isl1208_i2c_get_sr(client);
@@ -674,7 +674,7 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
}
}
- return 0;
+ return rtc_register_device(rtc);
}
static int
--
2.7.4
Diehl AKO Stiftung & Co. KG, Pfannerstraße 75-83, 88239 Wangen im Allgäu
Bereichsvorstand: Dr.-Ing. Michael Siedentop (Sprecher), Josef Fellner (Mitglied)
Sitz der Gesellschaft: Wangen i.A. – Registergericht: Amtsgericht Ulm HRA 620609 – Persönlich haftende Gesellschafterin: Diehl Verwaltungs-Stiftung – Sitz: Nürnberg – Registergericht: Amtsgericht Nürnberg HRA 11756 –
Vorstand: Dr.-Ing. E.h. Thomas Diehl (†) (Vorsitzender), Herr Dipl.-Wirtsch.-Ing. Wolfgang Weggen (stellvertretender Vorsitzender), Dipl.-Kfm. Claus Günther, Dipl.-Kfm. Frank Gutzeit, Dr.-Ing. Heinrich Schunk, Dr.-Ing. Michael Siedentop , Dipl.-Kfm. Dr.-Ing. Martin Sommer, Dipl.-Ing. (FH) Rainer von Borstel, Vorsitzender des Aufsichtsrates: Dr. Klaus Maier
___________________________________________________________________________________________________
Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht. Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/4] rtc: isl1208: switch to rtc_register_device
2018-03-05 10:43 ` [PATCH v3 2/4] rtc: isl1208: switch to rtc_register_device Denis OSTERLAND
@ 2018-03-06 20:20 ` Alexandre Belloni
0 siblings, 0 replies; 13+ messages in thread
From: Alexandre Belloni @ 2018-03-06 20:20 UTC (permalink / raw)
To: Denis OSTERLAND
Cc: a.zummo, linux-kernel, mgr, devicetree, linux, jdelvare,
linux-rtc, kernel
On 05/03/2018 at 10:43:53 +0000, Denis OSTERLAND wrote:
> From: Denis Osterland <Denis.Osterland@diehl.com>
>
> Fix possible race condition.
> It is not allowed to return with an error code after RTC is registered.
>
> Suggested-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
> Reviewed-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> drivers/rtc/rtc-isl1208.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
Applied, thanks.
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/4] rtc: isl1208: enable interrupt after context preparation
2018-03-05 10:43 [PATCH v3 0/4] rtc: isl1208: fixes, documentation and isl1219 support Denis OSTERLAND
` (2 preceding siblings ...)
2018-03-05 10:43 ` [PATCH v3 2/4] rtc: isl1208: switch to rtc_register_device Denis OSTERLAND
@ 2018-03-05 10:43 ` Denis OSTERLAND
2018-03-06 20:20 ` Alexandre Belloni
3 siblings, 1 reply; 13+ messages in thread
From: Denis OSTERLAND @ 2018-03-05 10:43 UTC (permalink / raw)
To: a.zummo, alexandre.belloni
Cc: linux-kernel, mgr, m.grzeschik, devicetree, linux, jdelvare,
linux-rtc, kernel, Denis OSTERLAND
From: Michael Grzeschik <m.grzeschik@pengutronix.de>
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 c8b4953..a13a4ba 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.7.4
Diehl AKO Stiftung & Co. KG, Pfannerstraße 75-83, 88239 Wangen im Allgäu
Bereichsvorstand: Dr.-Ing. Michael Siedentop (Sprecher), Josef Fellner (Mitglied)
Sitz der Gesellschaft: Wangen i.A. – Registergericht: Amtsgericht Ulm HRA 620609 – Persönlich haftende Gesellschafterin: Diehl Verwaltungs-Stiftung – Sitz: Nürnberg – Registergericht: Amtsgericht Nürnberg HRA 11756 –
Vorstand: Dr.-Ing. E.h. Thomas Diehl (†) (Vorsitzender), Herr Dipl.-Wirtsch.-Ing. Wolfgang Weggen (stellvertretender Vorsitzender), Dipl.-Kfm. Claus Günther, Dipl.-Kfm. Frank Gutzeit, Dr.-Ing. Heinrich Schunk, Dr.-Ing. Michael Siedentop , Dipl.-Kfm. Dr.-Ing. Martin Sommer, Dipl.-Ing. (FH) Rainer von Borstel, Vorsitzender des Aufsichtsrates: Dr. Klaus Maier
___________________________________________________________________________________________________
Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht. Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3 1/4] rtc: isl1208: enable interrupt after context preparation
2018-03-05 10:43 ` [PATCH v3 1/4] rtc: isl1208: enable interrupt after context preparation Denis OSTERLAND
@ 2018-03-06 20:20 ` Alexandre Belloni
0 siblings, 0 replies; 13+ messages in thread
From: Alexandre Belloni @ 2018-03-06 20:20 UTC (permalink / raw)
To: Denis OSTERLAND
Cc: a.zummo, linux-kernel, mgr, m.grzeschik, devicetree, linux,
jdelvare, linux-rtc, kernel
On 05/03/2018 at 10:43:53 +0000, Denis OSTERLAND wrote:
> From: Michael Grzeschik <m.grzeschik@pengutronix.de>
>
> 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(-)
>
Applied, thanks.
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 13+ messages in thread