| Message ID | 20180710090710.9066-3-Denis.Osterland@diehl.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc: isl1208: fixes, documentation and isl1219 support | expand |
Hi Michael,
I love your patch! Yet something to improve:
[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on v4.18-rc4 next-20180709]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Denis-OSTERLAND/rtc-isl1208-fixes-documentation-and-isl1219-support/20180710-181709
base: https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: x86_64-randconfig-s4-07101857 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
>> ERROR: "rtc_add_group" [drivers/rtc/rtc-isl1208.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi, seems 2/5 was applied before 1/5. 1/5 introduces rtc_add_group. Regards Denis Am Dienstag, den 10.07.2018, 21:20 +0800 schrieb kbuild test robot: > Hi Michael, > > I love your patch! Yet something to improve: > > [auto build test ERROR on abelloni/rtc-next] > [also build test ERROR on v4.18-rc4 next-20180709] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Denis-OSTERLAND/rtc-isl1208-fixes-documentation-and-isl1219-support/20180710-181709 > base: https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next > config: x86_64-randconfig-s4-07101857 (attached as .config) > compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 > reproduce: > # save the attached .config to linux build tree > make ARCH=x86_64 > > All errors (new ones prefixed by >>): > > > > > > > > > ERROR: "rtc_add_group" [drivers/rtc/rtc-isl1208.ko] undefined! > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation Diehl Connectivity Solutions GmbH Geschäftsführung: Horst Leonberger Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht Nürnberg: HRB 32315
Hi, Denis The patch was applied in correct sequence as you can see in the github link. I think the question here is rtc-isl1208 can be built as a built-in module, but it would fail if it was built as a ko. Thanks, Xiaolong On 07/10, Denis OSTERLAND wrote: >Hi, > >seems 2/5 was applied before 1/5. >1/5 introduces rtc_add_group. > >Regards Denis > >Am Dienstag, den 10.07.2018, 21:20 +0800 schrieb kbuild test robot: >> Hi Michael, >> >> I love your patch! Yet something to improve: >> >> [auto build test ERROR on abelloni/rtc-next] >> [also build test ERROR on v4.18-rc4 next-20180709] >> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] >> >> url: https://github.com/0day-ci/linux/commits/Denis-OSTERLAND/rtc-isl1208-fixes-documentation-and-isl1219-support/20180710-181709 >> base: https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next >> config: x86_64-randconfig-s4-07101857 (attached as .config) >> compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 >> reproduce: >> # save the attached .config to linux build tree >> make ARCH=x86_64 >> >> All errors (new ones prefixed by >>): >> >> > >> > > >> > > ERROR: "rtc_add_group" [drivers/rtc/rtc-isl1208.ko] undefined! >> --- >> 0-DAY kernel test infrastructure Open Source Technology Center >> https://lists.01.org/pipermail/kbuild-all Intel Corporation > >Diehl Connectivity Solutions GmbH >Geschäftsführung: Horst Leonberger >Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht >Nürnberg: HRB 32315 >___________________________________________________________________________________________________ > >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.
On 10/07/2018 09:44:15+0000, Denis OSTERLAND wrote: > static int > isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id) > { > @@ -642,6 +744,7 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id) > rtc->ops = &isl1208_rtc_ops; > > i2c_set_clientdata(client, rtc); > + dev_set_drvdata(&rtc->dev, client); > > rc = isl1208_i2c_get_sr(client); > if (rc < 0) { > @@ -653,6 +756,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"); > > + if (id->driver_data == TYPE_ISL1219) { > + rc = ISL1219_REG_EV_EVEN; > + rc = i2c_smbus_write_byte_data(client, ISL1219_REG_EV, rc); I'd pass ISL1219_REG_EV_EVEN directly instead of assigning it to rc. > + if (rc < 0) { > + dev_err(&client->dev, "could not enable tamper detection\n"); > + return rc; > + } > + rc = rtc_add_group(rtc, &isl1219_rtc_sysfs_files); > + if (rc) > + return rc; > + } > + > rc = sysfs_create_group(&client->dev.kobj, &isl1208_rtc_sysfs_files); > if (rc) > return rc; > @@ -686,8 +801,9 @@ isl1208_remove(struct i2c_client *client) > } > > static const struct i2c_device_id isl1208_id[] = { > - { "isl1208", 0 }, > - { "isl1218", 0 }, > + { "isl1208", TYPE_ISL1208 }, > + { "isl1218", TYPE_ISL1218 }, > + { "isl1219", TYPE_ISL1219 }, > { } > }; > MODULE_DEVICE_TABLE(i2c, isl1208_id); > @@ -695,6 +811,7 @@ MODULE_DEVICE_TABLE(i2c, isl1208_id); > static const struct of_device_id isl1208_of_match[] = { > { .compatible = "isil,isl1208" }, > { .compatible = "isil,isl1218" }, > + { .compatible = "isil,isl1219" }, > { } > }; > MODULE_DEVICE_TABLE(of, isl1208_of_match); > -- > 2.18.0 > > > > Diehl Connectivity Solutions GmbH > Geschäftsführung: Horst Leonberger > Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht > Nürnberg: HRB 32315 > ___________________________________________________________________________________________________ > > 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.
Hi, Am Mittwoch, den 18.07.2018, 10:13 +0200 schrieb Alexandre Belloni: > On 10/07/2018 09:44:15+0000, Denis OSTERLAND wrote: > > > > + if (id->driver_data == TYPE_ISL1219) { > > + rc = ISL1219_REG_EV_EVEN; > > + rc = i2c_smbus_write_byte_data(client, ISL1219_REG_EV, rc); > I'd pass ISL1219_REG_EV_EVEN directly instead of assigning it to rc. > Because of the 80 character limit per row, I thought this: rc = ISL1219_REG_EV_EVEN; rc = i2c_smbus_write_byte_data(client, ISL1219_REG_EV, rc); would look nicer than that: rc = i2c_smbus_write_byte_data(client, ISL1219_REG_EV, ISL1219_REG_EV_EVEN); Are there any reasons why I should prefer the second one? Regards Denis Diehl Connectivity Solutions GmbH Geschäftsführung: Horst Leonberger Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht Nürnberg: HRB 32315
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c index 1a2c38cc0178..bb189fe6744a 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 "rtc-core.h" /* Register map */ /* rtc section */ @@ -33,13 +34,15 @@ #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 */ #define ISL1208_REG_INT 0x08 #define ISL1208_REG_INT_ALME (1<<6) /* alarm enable */ #define ISL1208_REG_INT_IM (1<<7) /* interrupt/alarm mode */ -#define ISL1208_REG_09 0x09 /* reserved */ +#define ISL1219_REG_EV 0x09 +#define ISL1219_REG_EV_EVEN (1<<4) /* event detection enable */ #define ISL1208_REG_ATR 0x0a #define ISL1208_REG_DTR 0x0b @@ -57,8 +60,24 @@ #define ISL1208_REG_USR2 0x13 #define ISL1208_USR_SECTION_LEN 2 +/* event section */ +#define ISL1219_REG_SCT 0x14 +#define ISL1219_REG_MNT 0x15 +#define ISL1219_REG_HRT 0x16 +#define ISL1219_REG_DTT 0x17 +#define ISL1219_REG_MOT 0x18 +#define ISL1219_REG_YRT 0x19 +#define ISL1219_EVT_SECTION_LEN 6 + static struct i2c_driver isl1208_driver; +/* ISL1208 various variants */ +enum { + TYPE_ISL1208 = 0, + TYPE_ISL1218, + TYPE_ISL1219, +}; + /* block read */ static int isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[], @@ -80,8 +99,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 > ISL1219_REG_YRT); + WARN_ON(reg + len > ISL1219_REG_YRT + 1); ret = i2c_transfer(client->adapter, msgs, 2); if (ret > 0) @@ -104,8 +123,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 > ISL1219_REG_YRT); + WARN_ON(reg + len > ISL1219_REG_YRT + 1); i2c_buf[0] = reg; memcpy(&i2c_buf[1], &buf[0], len); @@ -493,6 +512,73 @@ isl1208_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm) return isl1208_i2c_set_alarm(to_i2c_client(dev), alarm); } +static ssize_t timestamp0_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct i2c_client *client = dev_get_drvdata(dev); + int sr; + + sr = isl1208_i2c_get_sr(client); + if (sr < 0) { + dev_err(dev, "%s: reading SR failed\n", __func__); + return sr; + } + + 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 timestamp0_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct i2c_client *client = dev_get_drvdata(dev); + u8 regs[ISL1219_EVT_SECTION_LEN] = { 0, }; + struct rtc_time tm; + int sr; + + sr = isl1208_i2c_get_sr(client); + if (sr < 0) { + dev_err(dev, "%s: reading SR failed\n", __func__); + return sr; + } + + if (!(sr & ISL1208_REG_SR_EVT)) + return 0; + + sr = isl1208_i2c_read_regs(client, ISL1219_REG_SCT, regs, + ISL1219_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[ISL1219_REG_SCT - ISL1219_REG_SCT] & 0x7f); + tm.tm_min = bcd2bin(regs[ISL1219_REG_MNT - ISL1219_REG_SCT] & 0x7f); + tm.tm_hour = bcd2bin(regs[ISL1219_REG_HRT - ISL1219_REG_SCT] & 0x3f); + tm.tm_mday = bcd2bin(regs[ISL1219_REG_DTT - ISL1219_REG_SCT] & 0x3f); + tm.tm_mon = + bcd2bin(regs[ISL1219_REG_MOT - ISL1219_REG_SCT] & 0x1f) - 1; + tm.tm_year = bcd2bin(regs[ISL1219_REG_YRT - ISL1219_REG_SCT]) + 100; + + sr = rtc_valid_tm(&tm); + if (sr) + return sr; + + return sprintf(buf, "%llu\n", + (unsigned long long)rtc_tm_to_time64(&tm)); +}; + +static DEVICE_ATTR_RW(timestamp0); + static irqreturn_t isl1208_rtc_interrupt(int irq, void *data) { @@ -538,6 +624,13 @@ isl1208_rtc_interrupt(int irq, void *data) return err; } + if (sr & ISL1208_REG_SR_EVT) { + sysfs_notify(&rtc->dev.kobj, NULL, + dev_attr_timestamp0.attr.name); + dev_warn(&client->dev, "event detected"); + handled = 1; + } + return handled ? IRQ_HANDLED : IRQ_NONE; } @@ -623,6 +716,15 @@ static const struct attribute_group isl1208_rtc_sysfs_files = { .attrs = isl1208_rtc_attrs, }; +static struct attribute *isl1219_rtc_attrs[] = { + &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) { @@ -642,6 +744,7 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id) rtc->ops = &isl1208_rtc_ops; i2c_set_clientdata(client, rtc); + dev_set_drvdata(&rtc->dev, client); rc = isl1208_i2c_get_sr(client); if (rc < 0) { @@ -653,6 +756,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"); + if (id->driver_data == TYPE_ISL1219) { + rc = ISL1219_REG_EV_EVEN; + rc = i2c_smbus_write_byte_data(client, ISL1219_REG_EV, rc); + if (rc < 0) { + dev_err(&client->dev, "could not enable tamper detection\n"); + return rc; + } + rc = rtc_add_group(rtc, &isl1219_rtc_sysfs_files); + if (rc) + return rc; + } + rc = sysfs_create_group(&client->dev.kobj, &isl1208_rtc_sysfs_files); if (rc) return rc; @@ -686,8 +801,9 @@ isl1208_remove(struct i2c_client *client) } static const struct i2c_device_id isl1208_id[] = { - { "isl1208", 0 }, - { "isl1218", 0 }, + { "isl1208", TYPE_ISL1208 }, + { "isl1218", TYPE_ISL1218 }, + { "isl1219", TYPE_ISL1219 }, { } }; MODULE_DEVICE_TABLE(i2c, isl1208_id); @@ -695,6 +811,7 @@ MODULE_DEVICE_TABLE(i2c, isl1208_id); static const struct of_device_id isl1208_of_match[] = { { .compatible = "isil,isl1208" }, { .compatible = "isil,isl1218" }, + { .compatible = "isil,isl1219" }, { } }; MODULE_DEVICE_TABLE(of, isl1208_of_match);