Re: [PATCH v5 3/5] RTC: RK808: add RTC driver for RK808

2014-08-26 Thread Doug Anderson
Chris,

On Tue, Aug 26, 2014 at 2:47 AM, Chris Zhong  wrote:
>
> On 08/26/2014 11:22 AM, Doug Anderson wrote:
>>>
>>> +   int ret;
>>> >+
>>> >+   /* Has the RTC been programmed? */
>>> >+   ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG,
>>> >+BIT_RTC_CTRL_REG_RTC_V_OPT_M, 0);
>>
>> Can you explain what you're doing here?  The comment seems wrong since
>> it implies that you're checking something.
>>
>> >From what I can tell from the manual you're setting "RTC_READSEL" to 0
>> which means "Read access directly to dynamic registers.".  That's not
>> clear here, and RTC_V_OPT_M makes no sense to me.
>
>
> RK808 has a shadowed register for saving a "frozen" RTC time.
> When user setting "RTC_READSEL" to 1, the time will save in this shadowed
> register.
> In this case, user read rtc time register, actually get the time of that
> moment.
> So, I move it to probe, this bit needn't clear every time

OK, now that I understand what BIT_RTC_CTRL_REG_RTC_READSEL_M is, I
think that we need it here.

At the beginning to readtime() you should set
BIT_RTC_CTRL_REG_RTC_READSEL_M to 1.  At the end of readtime() you
should set it to 0.  Make sure you set it back to 0 in the error
condition, too.

If you don't use READSEL you can get in confusing situations when
times wrap over.  Pretend that it's 11:59:59 when you start.  You read
the seconds.  Then the time ticks and you read the minutes and hours.
You'll end up reporting 12:00:59 when you really should report
12:00:00 or 11:59:59


>>> >+   tm->tm_min = bcd2bin(rtc_data[1]) & MINUTES_REG_MAK;
>>> >+   tm->tm_hour = bcd2bin(rtc_data[2]) & HOURS_REG_MSK;
>>> >+   tm->tm_mday = bcd2bin(rtc_data[3]) & DAYS_REG_MSK;
>>> >+   tm->tm_mon = (bcd2bin(rtc_data[4]) & MONTHS_REG_MSK) - 1;
>>> >+   tm->tm_year = (bcd2bin(rtc_data[5]) & YEARS_REG_MSK) + 100;
>>> >+   tm->tm_wday = bcd2bin(rtc_data[6]) & WEEKS_REG_MSK;
>>> >+   dev_dbg(dev, "RTC date/time %4d-%02d-%02d(%d) %02d:%02d:%02d\n",
>>> >+   1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
>>> >+   tm->tm_wday, tm->tm_hour , tm->tm_min, tm->tm_sec);
>>> >+
>>> >+   return 0;
>>> >+}
>>> >+
>>> >+/*
>>> >+ * Set current time and date in RTC
>>> >+ */
>>> >+static int rk808_rtc_set_time(struct device *dev, struct rtc_time *tm)
>>
>> Make this "const struct rtc_time *tm"
>
> It will be a warning if add const.
> It should be match
> int (*set_time)(struct device *, struct rtc_time *);
> in rtc.h

Oh, right.  Thanks.


>>> +   rk808_rtc_set_time(>dev, _def);
>>> >+   }
>>> >+
>>> >+   device_init_wakeup(>dev, 1);
>>
>> You can skip this.  We'll set "wakeup-source" in the device tree.
>> That will set I2C_CLIENT_WAKE.  That will cause the i2c core to call
>> device_init_wakeup() for you.
>
> If I remove it, wakealarm is disappear, even though I add "wakeup-source" in
> device tree.
> So, I keep it temporarily

OK, I think I understand.  It's because MFD doesn't percolate things
down to the devices.  The main device is setup as a wakeup-source but
not this one.  I think you can leave it as you have it..


>>> +   if (IS_ERR(rk808_rtc->rtc)) {
>>> >+   ret = PTR_ERR(rk808_rtc->rtc);
>>> >+   return ret;
>>> >+   }
>>> >+
>>> >+   alm_irq  = platform_get_irq(pdev, 0);
>>
>> Are you sure that platform_get_irq() works in this case?  In Javier's
>> in-flight max77802 driver he use regmap_irq_get_virq().
>>
>> ...oh, maybe your way does work!  You've got the rtc_resources to
>> specify things, so that looks good...
>>
>> ...but I tried testing it by doing:
>>
>> cd /sys/class/rtc/rtc0
>> echo +2 > wakealarm
>> sleep 5
>> grep 808 /proc/interrupts
>>
>> ...and I didn't see an interrupt go off.  Any idea why?
>
> It works well.

I figured it out.  We need
.  Maybe you
have a different bootloader or some other code in your kernel that
made it so you didn't see the problem.  ...and it's better to change
the IRQ in the dts to "level low".

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v5 3/5] RTC: RK808: add RTC driver for RK808

2014-08-26 Thread Chris Zhong


On 08/26/2014 11:22 AM, Doug Anderson wrote:

+   int ret;
>+
>+   /* Has the RTC been programmed? */
>+   ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG,
>+BIT_RTC_CTRL_REG_RTC_V_OPT_M, 0);

Can you explain what you're doing here?  The comment seems wrong since
it implies that you're checking something.

>From what I can tell from the manual you're setting "RTC_READSEL" to 0
which means "Read access directly to dynamic registers.".  That's not
clear here, and RTC_V_OPT_M makes no sense to me.


RK808 has a shadowed register for saving a "frozen" RTC time.
When user setting "RTC_READSEL" to 1, the time will save in this 
shadowed register.
In this case, user read rtc time register, actually get the time of that 
moment.

So, I move it to probe, this bit needn't clear every time

>+   tm->tm_min = bcd2bin(rtc_data[1]) & MINUTES_REG_MAK;
>+   tm->tm_hour = bcd2bin(rtc_data[2]) & HOURS_REG_MSK;
>+   tm->tm_mday = bcd2bin(rtc_data[3]) & DAYS_REG_MSK;
>+   tm->tm_mon = (bcd2bin(rtc_data[4]) & MONTHS_REG_MSK) - 1;
>+   tm->tm_year = (bcd2bin(rtc_data[5]) & YEARS_REG_MSK) + 100;
>+   tm->tm_wday = bcd2bin(rtc_data[6]) & WEEKS_REG_MSK;
>+   dev_dbg(dev, "RTC date/time %4d-%02d-%02d(%d) %02d:%02d:%02d\n",
>+   1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
>+   tm->tm_wday, tm->tm_hour , tm->tm_min, tm->tm_sec);
>+
>+   return 0;
>+}
>+
>+/*
>+ * Set current time and date in RTC
>+ */
>+static int rk808_rtc_set_time(struct device *dev, struct rtc_time *tm)

Make this "const struct rtc_time *tm"

It will be a warning if add const.
It should be match
int (*set_time)(struct device *, struct rtc_time *);
in rtc.h



+   rk808_rtc_set_time(>dev, _def);
>+   }
>+
>+   device_init_wakeup(>dev, 1);

You can skip this.  We'll set "wakeup-source" in the device tree.
That will set I2C_CLIENT_WAKE.  That will cause the i2c core to call
device_init_wakeup() for you.
If I remove it, wakealarm is disappear, even though I add 
"wakeup-source" in device tree.

So, I keep it temporarily



+   if (IS_ERR(rk808_rtc->rtc)) {
>+   ret = PTR_ERR(rk808_rtc->rtc);
>+   return ret;
>+   }
>+
>+   alm_irq  = platform_get_irq(pdev, 0);

Are you sure that platform_get_irq() works in this case?  In Javier's
in-flight max77802 driver he use regmap_irq_get_virq().

...oh, maybe your way does work!  You've got the rtc_resources to
specify things, so that looks good...

...but I tried testing it by doing:

cd /sys/class/rtc/rtc0
echo +2 > wakealarm
sleep 5
grep 808 /proc/interrupts

...and I didn't see an interrupt go off.  Any idea why?

It works well.





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v5 3/5] RTC: RK808: add RTC driver for RK808

2014-08-26 Thread Chris Zhong


On 08/26/2014 11:22 AM, Doug Anderson wrote:

+   int ret;
+
+   /* Has the RTC been programmed? */
+   ret = regmap_update_bits(rk808-regmap, RK808_RTC_CTRL_REG,
+BIT_RTC_CTRL_REG_RTC_V_OPT_M, 0);

Can you explain what you're doing here?  The comment seems wrong since
it implies that you're checking something.

From what I can tell from the manual you're setting RTC_READSEL to 0
which means Read access directly to dynamic registers..  That's not
clear here, and RTC_V_OPT_M makes no sense to me.


RK808 has a shadowed register for saving a frozen RTC time.
When user setting RTC_READSEL to 1, the time will save in this 
shadowed register.
In this case, user read rtc time register, actually get the time of that 
moment.

So, I move it to probe, this bit needn't clear every time

+   tm-tm_min = bcd2bin(rtc_data[1])  MINUTES_REG_MAK;
+   tm-tm_hour = bcd2bin(rtc_data[2])  HOURS_REG_MSK;
+   tm-tm_mday = bcd2bin(rtc_data[3])  DAYS_REG_MSK;
+   tm-tm_mon = (bcd2bin(rtc_data[4])  MONTHS_REG_MSK) - 1;
+   tm-tm_year = (bcd2bin(rtc_data[5])  YEARS_REG_MSK) + 100;
+   tm-tm_wday = bcd2bin(rtc_data[6])  WEEKS_REG_MSK;
+   dev_dbg(dev, RTC date/time %4d-%02d-%02d(%d) %02d:%02d:%02d\n,
+   1900 + tm-tm_year, tm-tm_mon + 1, tm-tm_mday,
+   tm-tm_wday, tm-tm_hour , tm-tm_min, tm-tm_sec);
+
+   return 0;
+}
+
+/*
+ * Set current time and date in RTC
+ */
+static int rk808_rtc_set_time(struct device *dev, struct rtc_time *tm)

Make this const struct rtc_time *tm

It will be a warning if add const.
It should be match
int (*set_time)(struct device *, struct rtc_time *);
in rtc.h



+   rk808_rtc_set_time(pdev-dev, tm_def);
+   }
+
+   device_init_wakeup(pdev-dev, 1);

You can skip this.  We'll set wakeup-source in the device tree.
That will set I2C_CLIENT_WAKE.  That will cause the i2c core to call
device_init_wakeup() for you.
If I remove it, wakealarm is disappear, even though I add 
wakeup-source in device tree.

So, I keep it temporarily



+   if (IS_ERR(rk808_rtc-rtc)) {
+   ret = PTR_ERR(rk808_rtc-rtc);
+   return ret;
+   }
+
+   alm_irq  = platform_get_irq(pdev, 0);

Are you sure that platform_get_irq() works in this case?  In Javier's
in-flight max77802 driver he use regmap_irq_get_virq().

...oh, maybe your way does work!  You've got the rtc_resources to
specify things, so that looks good...

...but I tried testing it by doing:

cd /sys/class/rtc/rtc0
echo +2  wakealarm
sleep 5
grep 808 /proc/interrupts

...and I didn't see an interrupt go off.  Any idea why?

It works well.





--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v5 3/5] RTC: RK808: add RTC driver for RK808

2014-08-26 Thread Doug Anderson
Chris,

On Tue, Aug 26, 2014 at 2:47 AM, Chris Zhong z...@rock-chips.com wrote:

 On 08/26/2014 11:22 AM, Doug Anderson wrote:

 +   int ret;
 +
 +   /* Has the RTC been programmed? */
 +   ret = regmap_update_bits(rk808-regmap, RK808_RTC_CTRL_REG,
 +BIT_RTC_CTRL_REG_RTC_V_OPT_M, 0);

 Can you explain what you're doing here?  The comment seems wrong since
 it implies that you're checking something.

 From what I can tell from the manual you're setting RTC_READSEL to 0
 which means Read access directly to dynamic registers..  That's not
 clear here, and RTC_V_OPT_M makes no sense to me.


 RK808 has a shadowed register for saving a frozen RTC time.
 When user setting RTC_READSEL to 1, the time will save in this shadowed
 register.
 In this case, user read rtc time register, actually get the time of that
 moment.
 So, I move it to probe, this bit needn't clear every time

OK, now that I understand what BIT_RTC_CTRL_REG_RTC_READSEL_M is, I
think that we need it here.

At the beginning to readtime() you should set
BIT_RTC_CTRL_REG_RTC_READSEL_M to 1.  At the end of readtime() you
should set it to 0.  Make sure you set it back to 0 in the error
condition, too.

If you don't use READSEL you can get in confusing situations when
times wrap over.  Pretend that it's 11:59:59 when you start.  You read
the seconds.  Then the time ticks and you read the minutes and hours.
You'll end up reporting 12:00:59 when you really should report
12:00:00 or 11:59:59


 +   tm-tm_min = bcd2bin(rtc_data[1])  MINUTES_REG_MAK;
 +   tm-tm_hour = bcd2bin(rtc_data[2])  HOURS_REG_MSK;
 +   tm-tm_mday = bcd2bin(rtc_data[3])  DAYS_REG_MSK;
 +   tm-tm_mon = (bcd2bin(rtc_data[4])  MONTHS_REG_MSK) - 1;
 +   tm-tm_year = (bcd2bin(rtc_data[5])  YEARS_REG_MSK) + 100;
 +   tm-tm_wday = bcd2bin(rtc_data[6])  WEEKS_REG_MSK;
 +   dev_dbg(dev, RTC date/time %4d-%02d-%02d(%d) %02d:%02d:%02d\n,
 +   1900 + tm-tm_year, tm-tm_mon + 1, tm-tm_mday,
 +   tm-tm_wday, tm-tm_hour , tm-tm_min, tm-tm_sec);
 +
 +   return 0;
 +}
 +
 +/*
 + * Set current time and date in RTC
 + */
 +static int rk808_rtc_set_time(struct device *dev, struct rtc_time *tm)

 Make this const struct rtc_time *tm

 It will be a warning if add const.
 It should be match
 int (*set_time)(struct device *, struct rtc_time *);
 in rtc.h

Oh, right.  Thanks.


 +   rk808_rtc_set_time(pdev-dev, tm_def);
 +   }
 +
 +   device_init_wakeup(pdev-dev, 1);

 You can skip this.  We'll set wakeup-source in the device tree.
 That will set I2C_CLIENT_WAKE.  That will cause the i2c core to call
 device_init_wakeup() for you.

 If I remove it, wakealarm is disappear, even though I add wakeup-source in
 device tree.
 So, I keep it temporarily

OK, I think I understand.  It's because MFD doesn't percolate things
down to the devices.  The main device is setup as a wakeup-source but
not this one.  I think you can leave it as you have it..


 +   if (IS_ERR(rk808_rtc-rtc)) {
 +   ret = PTR_ERR(rk808_rtc-rtc);
 +   return ret;
 +   }
 +
 +   alm_irq  = platform_get_irq(pdev, 0);

 Are you sure that platform_get_irq() works in this case?  In Javier's
 in-flight max77802 driver he use regmap_irq_get_virq().

 ...oh, maybe your way does work!  You've got the rtc_resources to
 specify things, so that looks good...

 ...but I tried testing it by doing:

 cd /sys/class/rtc/rtc0
 echo +2  wakealarm
 sleep 5
 grep 808 /proc/interrupts

 ...and I didn't see an interrupt go off.  Any idea why?

 It works well.

I figured it out.  We need
https://chromium-review.googlesource.com/#/c/214241/.  Maybe you
have a different bootloader or some other code in your kernel that
made it so you didn't see the problem.  ...and it's better to change
the IRQ in the dts to level low.

-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v5 3/5] RTC: RK808: add RTC driver for RK808

2014-08-25 Thread Doug Anderson
Chris,

On Mon, Aug 25, 2014 at 6:34 AM, Chris Zhong  wrote:
> Adding RTC driver for supporting RTC device present inside RK808 PMIC.
>
> Signed-off-by: Chris Zhong 

Add Signed-off-by: Zhang Qing 


> ---
>
> Changes in v5:
> - fixed a bug about set_time failed
>
> Changes in v4:
> - use >dev replace rk808->dev
>
> Changes in v3: None
> Changes in v2:
> Adviced by javier.martinez
> - Add a separate clock driver, rather than in RTC driver
>
>  drivers/rtc/Kconfig |   11 ++
>  drivers/rtc/Makefile|1 +
>  drivers/rtc/rtc-rk808.c |  442 
> +++
>  3 files changed, 454 insertions(+)
>  create mode 100644 drivers/rtc/rtc-rk808.c
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index a168e96..48f61b2 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -288,6 +288,17 @@ config RTC_DRV_MAX77686
>   This driver can also be built as a module. If so, the module
>   will be called rtc-max77686.
>
> +config RTC_DRV_RK808
> +   tristate "Rockchip RK808 RTC"
> +   depends on MFD_RK808
> +   help
> + If you say yes here you will get support for the
> + RTC of Rk808 PMIC.

Capitalization.  RK808.


> +
> + This driver can also be built as a module. If so, the module
> + will be called rtc-rk808.

Have you tried that?  From the code I'd guess "rk808-rtc", not "rtc-rk808".


>  config RTC_DRV_RS5C372
> tristate "Ricoh R2025S/D, RS5C372A/B, RV5C386, RV5C387A"
> help
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 56f061c..91fe4647 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -109,6 +109,7 @@ obj-$(CONFIG_RTC_DRV_PUV3)  += rtc-puv3.o
>  obj-$(CONFIG_RTC_DRV_PXA)  += rtc-pxa.o
>  obj-$(CONFIG_RTC_DRV_R9701)+= rtc-r9701.o
>  obj-$(CONFIG_RTC_DRV_RC5T583)  += rtc-rc5t583.o
> +obj-$(CONFIG_RTC_DRV_RK808)+= rtc-rk808.o
>  obj-$(CONFIG_RTC_DRV_RP5C01)   += rtc-rp5c01.o
>  obj-$(CONFIG_RTC_DRV_RS5C313)  += rtc-rs5c313.o
>  obj-$(CONFIG_RTC_DRV_RS5C348)  += rtc-rs5c348.o
> diff --git a/drivers/rtc/rtc-rk808.c b/drivers/rtc/rtc-rk808.c
> new file mode 100644
> index 000..b5f0df5
> --- /dev/null
> +++ b/drivers/rtc/rtc-rk808.c
> @@ -0,0 +1,442 @@
> +/*
> + * RTC driver for Rockchip RK808
> + *
> + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * Author: Chris Zhong 
> + * Author: Zhang Qing 

Author is already below (see MODULE_AUTHOR).  No need to repeat.

> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *

In a different patch I think Lee wanted the extra blank "*" line gone.

> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* RTC_CTRL_REG bitfields */
> +#define BIT_RTC_CTRL_REG_STOP_RTC_MBIT(0)
> +#define BIT_RTC_CTRL_REG_RTC_V_OPT_M   BIT(7)
> +#define BIT_RTC_INTERRUPTS_REG_IT_ALARM_M  BIT(3)
> +
> +#define SECONDS_REG_MSK0x7F
> +#define MINUTES_REG_MAK0x7F
> +#define HOURS_REG_MSK  0x3F
> +#define DAYS_REG_MSK   0x3F
> +#define MONTHS_REG_MSK 0x1F
> +#define YEARS_REG_MSK  0xFF
> +#define WEEKS_REG_MSK  0x7
> +
> +/* REG_SECONDS_REG through REG_YEARS_REG is how many registers? */
> +
> +#define ALL_TIME_REGS  7

Change this to (RK808_WEEKS_REG - RK808_SECONDS_REG + 1)

...and probably call it NUM_TIME_REGS

> +#define ALL_ALM_REGS   6

Change this to (RK808_ALARM_YEARS_REG - RK808_ALARM_SECONDS_REG + 1)

...and call it NUM_ALARM_REGS

> +
> +struct rk808_rtc {
> +   struct rk808 *rk808;
> +   struct rtc_device *rtc;
> +   unsigned int alarm_enabled:1;
> +};
> +
> +/*
> + * Read current time and date in RTC
> + */
> +static int rk808_rtc_readtime(struct device *dev, struct rtc_time *tm)
> +{
> +   struct rk808_rtc *rk808_rtc = dev_get_drvdata(dev);
> +   struct rk808 *rk808 = rk808_rtc->rk808;
> +   unsigned char rtc_data[ALL_TIME_REGS];

nit: u8, not "unsigned char"

> +   int ret;
> +
> +   /* Has the RTC been programmed? */
> +   ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG,
> +BIT_RTC_CTRL_REG_RTC_V_OPT_M, 0);

Can you explain what you're doing here?  The comment seems wrong since
it implies that you're checking something.

>From what I can tell from the manual you're setting "RTC_READSEL" to 0
which means "Read access directly to dynamic registers.".  That's not
clear here, and 

[PATCH v5 3/5] RTC: RK808: add RTC driver for RK808

2014-08-25 Thread Chris Zhong
Adding RTC driver for supporting RTC device present inside RK808 PMIC.

Signed-off-by: Chris Zhong 

---

Changes in v5:
- fixed a bug about set_time failed

Changes in v4:
- use >dev replace rk808->dev

Changes in v3: None
Changes in v2:
Adviced by javier.martinez
- Add a separate clock driver, rather than in RTC driver

 drivers/rtc/Kconfig |   11 ++
 drivers/rtc/Makefile|1 +
 drivers/rtc/rtc-rk808.c |  442 +++
 3 files changed, 454 insertions(+)
 create mode 100644 drivers/rtc/rtc-rk808.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index a168e96..48f61b2 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -288,6 +288,17 @@ config RTC_DRV_MAX77686
  This driver can also be built as a module. If so, the module
  will be called rtc-max77686.
 
+config RTC_DRV_RK808
+   tristate "Rockchip RK808 RTC"
+   depends on MFD_RK808
+   help
+ If you say yes here you will get support for the
+ RTC of Rk808 PMIC.
+
+ This driver can also be built as a module. If so, the module
+ will be called rtc-rk808.
+
+
 config RTC_DRV_RS5C372
tristate "Ricoh R2025S/D, RS5C372A/B, RV5C386, RV5C387A"
help
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 56f061c..91fe4647 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -109,6 +109,7 @@ obj-$(CONFIG_RTC_DRV_PUV3)  += rtc-puv3.o
 obj-$(CONFIG_RTC_DRV_PXA)  += rtc-pxa.o
 obj-$(CONFIG_RTC_DRV_R9701)+= rtc-r9701.o
 obj-$(CONFIG_RTC_DRV_RC5T583)  += rtc-rc5t583.o
+obj-$(CONFIG_RTC_DRV_RK808)+= rtc-rk808.o
 obj-$(CONFIG_RTC_DRV_RP5C01)   += rtc-rp5c01.o
 obj-$(CONFIG_RTC_DRV_RS5C313)  += rtc-rs5c313.o
 obj-$(CONFIG_RTC_DRV_RS5C348)  += rtc-rs5c348.o
diff --git a/drivers/rtc/rtc-rk808.c b/drivers/rtc/rtc-rk808.c
new file mode 100644
index 000..b5f0df5
--- /dev/null
+++ b/drivers/rtc/rtc-rk808.c
@@ -0,0 +1,442 @@
+/*
+ * RTC driver for Rockchip RK808
+ *
+ * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * Author: Chris Zhong 
+ * Author: Zhang Qing 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* RTC_CTRL_REG bitfields */
+#define BIT_RTC_CTRL_REG_STOP_RTC_MBIT(0)
+#define BIT_RTC_CTRL_REG_RTC_V_OPT_M   BIT(7)
+#define BIT_RTC_INTERRUPTS_REG_IT_ALARM_M  BIT(3)
+
+#define SECONDS_REG_MSK0x7F
+#define MINUTES_REG_MAK0x7F
+#define HOURS_REG_MSK  0x3F
+#define DAYS_REG_MSK   0x3F
+#define MONTHS_REG_MSK 0x1F
+#define YEARS_REG_MSK  0xFF
+#define WEEKS_REG_MSK  0x7
+
+/* REG_SECONDS_REG through REG_YEARS_REG is how many registers? */
+
+#define ALL_TIME_REGS  7
+#define ALL_ALM_REGS   6
+
+struct rk808_rtc {
+   struct rk808 *rk808;
+   struct rtc_device *rtc;
+   unsigned int alarm_enabled:1;
+};
+
+/*
+ * Read current time and date in RTC
+ */
+static int rk808_rtc_readtime(struct device *dev, struct rtc_time *tm)
+{
+   struct rk808_rtc *rk808_rtc = dev_get_drvdata(dev);
+   struct rk808 *rk808 = rk808_rtc->rk808;
+   unsigned char rtc_data[ALL_TIME_REGS];
+   int ret;
+
+   /* Has the RTC been programmed? */
+   ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG,
+BIT_RTC_CTRL_REG_RTC_V_OPT_M, 0);
+   if (ret) {
+   dev_err(dev, "Failed to update RTC control: %d\n", ret);
+   return ret;
+   }
+
+   ret = regmap_bulk_read(rk808->regmap, RK808_SECONDS_REG,
+  rtc_data, ALL_TIME_REGS);
+   if (ret) {
+   dev_err(dev, "Failed to bulk read rtc_data: %d\n", ret);
+   return ret;
+   }
+
+   tm->tm_sec = bcd2bin(rtc_data[0]) & SECONDS_REG_MSK;
+   tm->tm_min = bcd2bin(rtc_data[1]) & MINUTES_REG_MAK;
+   tm->tm_hour = bcd2bin(rtc_data[2]) & HOURS_REG_MSK;
+   tm->tm_mday = bcd2bin(rtc_data[3]) & DAYS_REG_MSK;
+   tm->tm_mon = (bcd2bin(rtc_data[4]) & MONTHS_REG_MSK) - 1;
+   tm->tm_year = (bcd2bin(rtc_data[5]) & YEARS_REG_MSK) + 100;
+   tm->tm_wday = bcd2bin(rtc_data[6]) & WEEKS_REG_MSK;
+   dev_dbg(dev, "RTC date/time %4d-%02d-%02d(%d) %02d:%02d:%02d\n",
+   1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
+   tm->tm_wday, tm->tm_hour , tm->tm_min, tm->tm_sec);
+
+   return 0;
+}
+
+/*
+ * Set current time and date in 

[PATCH v5 3/5] RTC: RK808: add RTC driver for RK808

2014-08-25 Thread Chris Zhong
Adding RTC driver for supporting RTC device present inside RK808 PMIC.

Signed-off-by: Chris Zhong z...@rock-chips.com

---

Changes in v5:
- fixed a bug about set_time failed

Changes in v4:
- use client-dev replace rk808-dev

Changes in v3: None
Changes in v2:
Adviced by javier.martinez
- Add a separate clock driver, rather than in RTC driver

 drivers/rtc/Kconfig |   11 ++
 drivers/rtc/Makefile|1 +
 drivers/rtc/rtc-rk808.c |  442 +++
 3 files changed, 454 insertions(+)
 create mode 100644 drivers/rtc/rtc-rk808.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index a168e96..48f61b2 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -288,6 +288,17 @@ config RTC_DRV_MAX77686
  This driver can also be built as a module. If so, the module
  will be called rtc-max77686.
 
+config RTC_DRV_RK808
+   tristate Rockchip RK808 RTC
+   depends on MFD_RK808
+   help
+ If you say yes here you will get support for the
+ RTC of Rk808 PMIC.
+
+ This driver can also be built as a module. If so, the module
+ will be called rtc-rk808.
+
+
 config RTC_DRV_RS5C372
tristate Ricoh R2025S/D, RS5C372A/B, RV5C386, RV5C387A
help
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 56f061c..91fe4647 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -109,6 +109,7 @@ obj-$(CONFIG_RTC_DRV_PUV3)  += rtc-puv3.o
 obj-$(CONFIG_RTC_DRV_PXA)  += rtc-pxa.o
 obj-$(CONFIG_RTC_DRV_R9701)+= rtc-r9701.o
 obj-$(CONFIG_RTC_DRV_RC5T583)  += rtc-rc5t583.o
+obj-$(CONFIG_RTC_DRV_RK808)+= rtc-rk808.o
 obj-$(CONFIG_RTC_DRV_RP5C01)   += rtc-rp5c01.o
 obj-$(CONFIG_RTC_DRV_RS5C313)  += rtc-rs5c313.o
 obj-$(CONFIG_RTC_DRV_RS5C348)  += rtc-rs5c348.o
diff --git a/drivers/rtc/rtc-rk808.c b/drivers/rtc/rtc-rk808.c
new file mode 100644
index 000..b5f0df5
--- /dev/null
+++ b/drivers/rtc/rtc-rk808.c
@@ -0,0 +1,442 @@
+/*
+ * RTC driver for Rockchip RK808
+ *
+ * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * Author: Chris Zhong z...@rock-chips.com
+ * Author: Zhang Qing zhangq...@rock-chips.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#include linux/module.h
+#include linux/kernel.h
+#include linux/rtc.h
+#include linux/bcd.h
+#include linux/mfd/rk808.h
+#include linux/platform_device.h
+#include linux/i2c.h
+
+/* RTC_CTRL_REG bitfields */
+#define BIT_RTC_CTRL_REG_STOP_RTC_MBIT(0)
+#define BIT_RTC_CTRL_REG_RTC_V_OPT_M   BIT(7)
+#define BIT_RTC_INTERRUPTS_REG_IT_ALARM_M  BIT(3)
+
+#define SECONDS_REG_MSK0x7F
+#define MINUTES_REG_MAK0x7F
+#define HOURS_REG_MSK  0x3F
+#define DAYS_REG_MSK   0x3F
+#define MONTHS_REG_MSK 0x1F
+#define YEARS_REG_MSK  0xFF
+#define WEEKS_REG_MSK  0x7
+
+/* REG_SECONDS_REG through REG_YEARS_REG is how many registers? */
+
+#define ALL_TIME_REGS  7
+#define ALL_ALM_REGS   6
+
+struct rk808_rtc {
+   struct rk808 *rk808;
+   struct rtc_device *rtc;
+   unsigned int alarm_enabled:1;
+};
+
+/*
+ * Read current time and date in RTC
+ */
+static int rk808_rtc_readtime(struct device *dev, struct rtc_time *tm)
+{
+   struct rk808_rtc *rk808_rtc = dev_get_drvdata(dev);
+   struct rk808 *rk808 = rk808_rtc-rk808;
+   unsigned char rtc_data[ALL_TIME_REGS];
+   int ret;
+
+   /* Has the RTC been programmed? */
+   ret = regmap_update_bits(rk808-regmap, RK808_RTC_CTRL_REG,
+BIT_RTC_CTRL_REG_RTC_V_OPT_M, 0);
+   if (ret) {
+   dev_err(dev, Failed to update RTC control: %d\n, ret);
+   return ret;
+   }
+
+   ret = regmap_bulk_read(rk808-regmap, RK808_SECONDS_REG,
+  rtc_data, ALL_TIME_REGS);
+   if (ret) {
+   dev_err(dev, Failed to bulk read rtc_data: %d\n, ret);
+   return ret;
+   }
+
+   tm-tm_sec = bcd2bin(rtc_data[0])  SECONDS_REG_MSK;
+   tm-tm_min = bcd2bin(rtc_data[1])  MINUTES_REG_MAK;
+   tm-tm_hour = bcd2bin(rtc_data[2])  HOURS_REG_MSK;
+   tm-tm_mday = bcd2bin(rtc_data[3])  DAYS_REG_MSK;
+   tm-tm_mon = (bcd2bin(rtc_data[4])  MONTHS_REG_MSK) - 1;
+   tm-tm_year = (bcd2bin(rtc_data[5])  YEARS_REG_MSK) + 100;
+   tm-tm_wday = bcd2bin(rtc_data[6])  WEEKS_REG_MSK;
+   dev_dbg(dev, RTC date/time %4d-%02d-%02d(%d) %02d:%02d:%02d\n,
+   1900 + tm-tm_year, tm-tm_mon + 1, 

Re: [PATCH v5 3/5] RTC: RK808: add RTC driver for RK808

2014-08-25 Thread Doug Anderson
Chris,

On Mon, Aug 25, 2014 at 6:34 AM, Chris Zhong z...@rock-chips.com wrote:
 Adding RTC driver for supporting RTC device present inside RK808 PMIC.

 Signed-off-by: Chris Zhong z...@rock-chips.com

Add Signed-off-by: Zhang Qing zhangq...@rock-chips.com


 ---

 Changes in v5:
 - fixed a bug about set_time failed

 Changes in v4:
 - use client-dev replace rk808-dev

 Changes in v3: None
 Changes in v2:
 Adviced by javier.martinez
 - Add a separate clock driver, rather than in RTC driver

  drivers/rtc/Kconfig |   11 ++
  drivers/rtc/Makefile|1 +
  drivers/rtc/rtc-rk808.c |  442 
 +++
  3 files changed, 454 insertions(+)
  create mode 100644 drivers/rtc/rtc-rk808.c

 diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
 index a168e96..48f61b2 100644
 --- a/drivers/rtc/Kconfig
 +++ b/drivers/rtc/Kconfig
 @@ -288,6 +288,17 @@ config RTC_DRV_MAX77686
   This driver can also be built as a module. If so, the module
   will be called rtc-max77686.

 +config RTC_DRV_RK808
 +   tristate Rockchip RK808 RTC
 +   depends on MFD_RK808
 +   help
 + If you say yes here you will get support for the
 + RTC of Rk808 PMIC.

Capitalization.  RK808.


 +
 + This driver can also be built as a module. If so, the module
 + will be called rtc-rk808.

Have you tried that?  From the code I'd guess rk808-rtc, not rtc-rk808.


  config RTC_DRV_RS5C372
 tristate Ricoh R2025S/D, RS5C372A/B, RV5C386, RV5C387A
 help
 diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
 index 56f061c..91fe4647 100644
 --- a/drivers/rtc/Makefile
 +++ b/drivers/rtc/Makefile
 @@ -109,6 +109,7 @@ obj-$(CONFIG_RTC_DRV_PUV3)  += rtc-puv3.o
  obj-$(CONFIG_RTC_DRV_PXA)  += rtc-pxa.o
  obj-$(CONFIG_RTC_DRV_R9701)+= rtc-r9701.o
  obj-$(CONFIG_RTC_DRV_RC5T583)  += rtc-rc5t583.o
 +obj-$(CONFIG_RTC_DRV_RK808)+= rtc-rk808.o
  obj-$(CONFIG_RTC_DRV_RP5C01)   += rtc-rp5c01.o
  obj-$(CONFIG_RTC_DRV_RS5C313)  += rtc-rs5c313.o
  obj-$(CONFIG_RTC_DRV_RS5C348)  += rtc-rs5c348.o
 diff --git a/drivers/rtc/rtc-rk808.c b/drivers/rtc/rtc-rk808.c
 new file mode 100644
 index 000..b5f0df5
 --- /dev/null
 +++ b/drivers/rtc/rtc-rk808.c
 @@ -0,0 +1,442 @@
 +/*
 + * RTC driver for Rockchip RK808
 + *
 + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
 + *
 + * Author: Chris Zhong z...@rock-chips.com
 + * Author: Zhang Qing zhangq...@rock-chips.com

Author is already below (see MODULE_AUTHOR).  No need to repeat.

 + *
 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms and conditions of the GNU General Public License,
 + * version 2, as published by the Free Software Foundation.
 + *
 + * This program is distributed in the hope it will be useful, but WITHOUT
 + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
 + * more details.
 + *

In a different patch I think Lee wanted the extra blank * line gone.

 + */
 +
 +#include linux/module.h
 +#include linux/kernel.h
 +#include linux/rtc.h
 +#include linux/bcd.h
 +#include linux/mfd/rk808.h
 +#include linux/platform_device.h
 +#include linux/i2c.h
 +
 +/* RTC_CTRL_REG bitfields */
 +#define BIT_RTC_CTRL_REG_STOP_RTC_MBIT(0)
 +#define BIT_RTC_CTRL_REG_RTC_V_OPT_M   BIT(7)
 +#define BIT_RTC_INTERRUPTS_REG_IT_ALARM_M  BIT(3)
 +
 +#define SECONDS_REG_MSK0x7F
 +#define MINUTES_REG_MAK0x7F
 +#define HOURS_REG_MSK  0x3F
 +#define DAYS_REG_MSK   0x3F
 +#define MONTHS_REG_MSK 0x1F
 +#define YEARS_REG_MSK  0xFF
 +#define WEEKS_REG_MSK  0x7
 +
 +/* REG_SECONDS_REG through REG_YEARS_REG is how many registers? */
 +
 +#define ALL_TIME_REGS  7

Change this to (RK808_WEEKS_REG - RK808_SECONDS_REG + 1)

...and probably call it NUM_TIME_REGS

 +#define ALL_ALM_REGS   6

Change this to (RK808_ALARM_YEARS_REG - RK808_ALARM_SECONDS_REG + 1)

...and call it NUM_ALARM_REGS

 +
 +struct rk808_rtc {
 +   struct rk808 *rk808;
 +   struct rtc_device *rtc;
 +   unsigned int alarm_enabled:1;
 +};
 +
 +/*
 + * Read current time and date in RTC
 + */
 +static int rk808_rtc_readtime(struct device *dev, struct rtc_time *tm)
 +{
 +   struct rk808_rtc *rk808_rtc = dev_get_drvdata(dev);
 +   struct rk808 *rk808 = rk808_rtc-rk808;
 +   unsigned char rtc_data[ALL_TIME_REGS];

nit: u8, not unsigned char

 +   int ret;
 +
 +   /* Has the RTC been programmed? */
 +   ret = regmap_update_bits(rk808-regmap, RK808_RTC_CTRL_REG,
 +BIT_RTC_CTRL_REG_RTC_V_OPT_M, 0);

Can you explain what you're doing here?  The comment seems wrong since
it implies that you're checking something.

From what I can tell from the manual you're setting RTC_READSEL to 0
which means Read