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.��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
