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 <[email protected]>
> ---
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
