On Fri, Nov 15, 2013 at 11:45:27PM +0000, Arnaud Ebalard wrote: > > Intersil ISL12057 I2C RTC chip also supports two alarms. This patch > only adds support for basic RTC functionalities (i.e. getting and > setting time). Tests have been performed on NETGEAR ReadyNAS 102 w/ > startup/shutdown scripts, hwclock, ntpdate and openntpd. > > Signed-off-by: Arnaud Ebalard <arno@xxxxxxxxxxxx> > --- > Hi, > > In order to get at least some initial support for the chip available in > mainline kernel (3.14 will have three users waiting for it: ReadyNAS > 102, 104 and 2120), I decided to temporarily remove the alarm and IRQ > related code I pushed earlier (see [1] below). I will submit a patch > later to add back Alarm/IRQ support once I have understood how to > correctly handle my use case described via existing RTC logic. > > Anyway, comments are welcome on the patch! > > Changes since previous v0: > - removed alarm and IRQ related code > - Switched to isl for intersil (no existing driver has any reference > to isil, even though this would be the recommended choice) > - Added intersil info in vendor-prefixes.txt file > - Added entry for ISL 12057 in I2C trivial-devices.txt file > - Added mutex protection for non atomic read/write > > > Now, regarding later submission of Alarm/IRQ functionality, here is the > use case: on all 3 NETGEAR NAS above (102, 104 and RN2120), the Alarm > interrupt pins of the ISL 12057 are not connected to the SoC, i.e. Linux > kernel never gets warned via an interrupt when the alarm goes off. The > reason is that the alarm interrupt pin is connected to some regulator > which will power up the (previously off) device when the alarm goes > off. It raises some questions for which I am looking for some supports > from RTC maintainer and people familiar w/ RTC framework: > > - Are there no previous example of such need from existing platform? > - How can this use case be supported? Tested? > - in usual context, what is the purpose of having an IRQ handler for > such an alarm event: just acknowledge it and remove the alarm? Or > are there other reasons? > > As a side note, I want to repeat that previous code (the one I dropped > from current patch) worked fine to get alarm installed (i.e. once the > device powered off, it woke up as expected using the alarm). I was just > unable to test the interrupt handling part, for the reason described > above. > > Cheers, > > a+ > > [1]: http://thread.gmane.org/gmane.linux.drivers.devicetree/46581 > > > .../devicetree/bindings/i2c/trivial-devices.txt | 1 + > .../devicetree/bindings/vendor-prefixes.txt | 1 + > drivers/rtc/Kconfig | 9 + > drivers/rtc/Makefile | 1 + > drivers/rtc/rtc-isl12057.c | 397 +++++++++++++++++++++ > 5 files changed, 409 insertions(+) > create mode 100644 drivers/rtc/rtc-isl12057.c > > diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt > index ad6a738..b57c71e 100644 > --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt > +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt > @@ -37,6 +37,7 @@ fsl,mpr121 MPR121: Proximity Capacitive Touch Sensor Controller > fsl,sgtl5000 SGTL5000: Ultra Low-Power Audio Codec > infineon,slb9635tt Infineon SLB9635 (Soft-) I2C TPM (old protocol, max 100khz) > infineon,slb9645tt Infineon SLB9645 I2C TPM (new protocol, max 400khz) > +isl,isl12057 Intersil ISL12057 I2C RTC Chip > maxim,ds1050 5 Bit Programmable, Pulse-Width Modulator > maxim,max1237 Low-Power, 4-/12-Channel, 2-Wire Serial, 12-Bit ADCs > maxim,max6625 9-Bit/12-Bit Temperature Sensors with I²C-Compatible Serial Interface > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt > index ce95ed1..507761a 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt > @@ -38,6 +38,7 @@ ibm International Business Machines (IBM) > idt Integrated Device Technologies, Inc. > img Imagination Technologies Ltd. > intercontrol Inter Control Group > +isl Intersil > linux Linux-specific binding > lsi LSI Corp. (LSI Logic) > marvell Marvell Technology Group Ltd. > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > index 15f166a..bcc0c17 100644 The binding additions look fine to me. [...] > +static int isl12057_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[], > + unsigned len) > +{ > + struct i2c_msg msgs[2] = { > + { > + .addr = client->addr, > + .flags = 0, > + .len = sizeof(u8), > + .buf = buf > + }, > + { > + .addr = client->addr, > + .flags = I2C_M_RD, > + .len = len, > + .buf = buf > + } > + }; > + int ret; > + > + BUG_ON(reg > ISL12057_REG_SR); > + BUG_ON(reg + len > ISL12057_REG_SR + 1); > + > + ret = i2c_transfer(client->adapter, msgs, 2); > + if (ret != 2) > + return ret; This might return 1. Given that in isl12057_rtc_read_time you check if the return value is negative (also indirectly via isl12057_i2c_validate_client), I think it would make sense to always either return a negative error code or 0. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
