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

Am Mittwoch, den 07.03.2018, 11:47 +0100 schrieb Alexandre Belloni:

quoted

quoted

quoted

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

quoted

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.

quoted

quoted

quoted

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