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

2018-03-08 Thread Alexandre Belloni
On 08/03/2018 at 11:53:09 +, Denis OSTERLAND wrote:
> Am Mittwoch, den 07.03.2018, 11:47 +0100 schrieb Alexandre Belloni:
> > > > > +
> > > > > + tv64.tv_sec = rtc_tm_to_time64();
> > > > 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();
> 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());

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


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

2018-03-08 Thread Alexandre Belloni
On 08/03/2018 at 11:53:09 +, Denis OSTERLAND wrote:
> Am Mittwoch, den 07.03.2018, 11:47 +0100 schrieb Alexandre Belloni:
> > > > > +
> > > > > + tv64.tv_sec = rtc_tm_to_time64();
> > > > 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();
> 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());

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


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

2018-03-08 Thread Denis OSTERLAND
Am Mittwoch, den 07.03.2018, 11:47 +0100 schrieb Alexandre Belloni:
> > > > +
> > > > +   tv64.tv_sec = rtc_tm_to_time64();
> > > 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();
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());
> 
> > 
> > sprintf(buf, "%lld\n", rtc_tm_to_time64()) 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(>dev.kobj, 
> > > > _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(>dev, "could not enable tamper 
> > > > detection\n");
> > > > +   return rc;
> > > > +   }
> > > > +   isl1208->sysfs_files = _rtc_sysfs_files;
> > > > +   } else {
> > > > +   isl1208->sysfs_files = _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[] = {
_attr_timestamp0.attr,
NULL
};

in probe
if (id->driver_data == TYPE_ISL1219) {
sysfs_merge_group(>kobj, _rtc_sysfs_files);

in remove
struct rtc_device *rtc = i2c_get_clientdata(client);
sysfs_unmerge_group(>kobj, _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.

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

2018-03-08 Thread Denis OSTERLAND
Am Mittwoch, den 07.03.2018, 11:47 +0100 schrieb Alexandre Belloni:
> > > > +
> > > > +   tv64.tv_sec = rtc_tm_to_time64();
> > > 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();
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());
> 
> > 
> > sprintf(buf, "%lld\n", rtc_tm_to_time64()) 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(>dev.kobj, 
> > > > _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(>dev, "could not enable tamper 
> > > > detection\n");
> > > > +   return rc;
> > > > +   }
> > > > +   isl1208->sysfs_files = _rtc_sysfs_files;
> > > > +   } else {
> > > > +   isl1208->sysfs_files = _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[] = {
_attr_timestamp0.attr,
NULL
};

in probe
if (id->driver_data == TYPE_ISL1219) {
sysfs_merge_group(>kobj, _rtc_sysfs_files);

in remove
struct rtc_device *rtc = i2c_get_clientdata(client);
sysfs_unmerge_group(>kobj, _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.

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

2018-03-07 Thread Rob Herring
On Mon, Mar 05, 2018 at 10:43:52AM +, Denis OSTERLAND wrote:
> From: Michael Grzeschik 
> 
> 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 
> Signed-off-by: Denis Osterland 
> ---
>  .../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 

>  drivers/rtc/rtc-isl1208.c  | 160 
> ++---
>  2 files changed, 171 insertions(+), 17 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl1219.txt


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

2018-03-07 Thread Rob Herring
On Mon, Mar 05, 2018 at 10:43:52AM +, Denis OSTERLAND wrote:
> From: Michael Grzeschik 
> 
> 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 
> Signed-off-by: Denis Osterland 
> ---
>  .../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 

>  drivers/rtc/rtc-isl1208.c  | 160 
> ++---
>  2 files changed, 171 insertions(+), 17 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl1219.txt


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

2018-03-07 Thread Alexandre Belloni
On 07/03/2018 at 08:19:15 +, 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();
> > 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()) 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(>dev.kobj, _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(>dev, "could not enable tamper 
> > > detection\n");
> > > + return rc;
> > > + }
> > > + isl1208->sysfs_files = _rtc_sysfs_files;
> > > + } else {
> > > + isl1208->sysfs_files = _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


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

2018-03-07 Thread Alexandre Belloni
On 07/03/2018 at 08:19:15 +, 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();
> > 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()) 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(>dev.kobj, _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(>dev, "could not enable tamper 
> > > detection\n");
> > > + return rc;
> > > + }
> > > + isl1208->sysfs_files = _rtc_sysfs_files;
> > > + } else {
> > > + isl1208->sysfs_files = _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


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

2018-03-07 Thread Denis OSTERLAND
Am Dienstag, den 06.03.2018, 21:42 +0100 schrieb Alexandre Belloni:
> On 05/03/2018 at 10:43:52 +, 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 000..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();
> 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()) 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(>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(>dev.kobj, NULL,
> > +   dev_attr_timestamp0.attr.name);
> > +   dev_warn(>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[] = {
> > +   _attr_atrim.attr,
> > +   _attr_dtrim.attr,
> > +   _attr_usr.attr,
> > +   _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(>dev);
> > -   if (IS_ERR(rtc))
> > -   return PTR_ERR(rtc);
> > +   isl1208 = devm_kzalloc(>dev, sizeof(struct isl1208),
> > +

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

2018-03-07 Thread Denis OSTERLAND
Am Dienstag, den 06.03.2018, 21:42 +0100 schrieb Alexandre Belloni:
> On 05/03/2018 at 10:43:52 +, 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 000..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();
> 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()) 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(>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(>dev.kobj, NULL,
> > +   dev_attr_timestamp0.attr.name);
> > +   dev_warn(>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[] = {
> > +   _attr_atrim.attr,
> > +   _attr_dtrim.attr,
> > +   _attr_usr.attr,
> > +   _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(>dev);
> > -   if (IS_ERR(rtc))
> > -   return PTR_ERR(rtc);
> > +   isl1208 = devm_kzalloc(>dev, sizeof(struct isl1208),
> > +

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

2018-03-06 Thread Alexandre Belloni
On 05/03/2018 at 10:43:52 +, 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 000..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();

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(>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(>dev.kobj, NULL,
> + dev_attr_timestamp0.attr.name);
> + dev_warn(>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[] = {
> + _attr_atrim.attr,
> + _attr_dtrim.attr,
> + _attr_usr.attr,
> + _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(>dev);
> - if (IS_ERR(rtc))
> - return PTR_ERR(rtc);
> + isl1208 = devm_kzalloc(>dev, sizeof(struct isl1208),
> + GFP_KERNEL);
> + if (!isl1208)
> + return -ENOMEM;
> 
> - rtc->ops = _rtc_ops;
> + isl1208->rtc = devm_rtc_allocate_device(>dev);
> + if (IS_ERR(isl1208->rtc))
> + return PTR_ERR(isl1208->rtc);
> 
> - i2c_set_clientdata(client, rtc);
> + isl1208->rtc->ops = _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(>dev, "rtc power failure detected, "
>"please set clock.\n");
> 
> - rc = sysfs_create_group(>dev.kobj, _rtc_sysfs_files);
> + if (id->driver_data == TYPE_ISL1219) {
> + rc = i2c_smbus_write_byte_data(client, 

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

2018-03-06 Thread Alexandre Belloni
On 05/03/2018 at 10:43:52 +, 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 000..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();

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(>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(>dev.kobj, NULL,
> + dev_attr_timestamp0.attr.name);
> + dev_warn(>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[] = {
> + _attr_atrim.attr,
> + _attr_dtrim.attr,
> + _attr_usr.attr,
> + _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(>dev);
> - if (IS_ERR(rtc))
> - return PTR_ERR(rtc);
> + isl1208 = devm_kzalloc(>dev, sizeof(struct isl1208),
> + GFP_KERNEL);
> + if (!isl1208)
> + return -ENOMEM;
> 
> - rtc->ops = _rtc_ops;
> + isl1208->rtc = devm_rtc_allocate_device(>dev);
> + if (IS_ERR(isl1208->rtc))
> + return PTR_ERR(isl1208->rtc);
> 
> - i2c_set_clientdata(client, rtc);
> + isl1208->rtc->ops = _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(>dev, "rtc power failure detected, "
>"please set clock.\n");
> 
> - rc = sysfs_create_group(>dev.kobj, _rtc_sysfs_files);
> + if (id->driver_data == TYPE_ISL1219) {
> + rc = i2c_smbus_write_byte_data(client,