Re: [PATCHv1] Add support for Intersil ISL12057 I2C RTC chip

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux