Re: [rtc-linux] [RESEND PATCH] RTC: MAX77686: Add Maxim 77686 driver
Hi, Andrew,
On 2012년 12월 19일 09:30, Andrew Morton wrote:
> On Wed, 28 Nov 2012 15:50:57 +0900
> Jonghwa Lee wrote:
>
>> Add driver for support max77686 rtc.
>> MAX77686 rtc support smpl and wtsr mode. It has two alarm register
>> which can be used for alarming to wake system up. This drvier uses regmap
>> to access its register.
>>
>> ...
>>
>> +static inline int max77686_rtc_calculate_wday(u8 shifted)
>> +{
>> +int counter = -1;
>> +while (shifted) {
>> +shifted >>= 1;
>> +counter++;
>> +}
>> +return counter;
>> +}
>
> We should be able to use log2() in here?
>
Okay, and I'd like to use Joe's recommendation. fls(shitfted) - 1
>>
>> ...
>>
>> +static inline int max77686_rtc_update(struct max77686_rtc_info *info,
>> +enum MAX77686_RTC_OP op)
>> +{
>> +int ret;
>> +unsigned int data;
>> +
>> +switch (op) {
>> +case MAX77686_RTC_WRITE:
>> +data = 1 << RTC_UDR_SHIFT;
>> +break;
>> +case MAX77686_RTC_READ:
>> +data = 1 << RTC_RBUDR_SHIFT;
>> +break;
>> +}
>> +
>> +ret = regmap_update_bits(info->max77686->rtc_regmap,
>> + MAX77686_RTC_UPDATE0, data, data);
>> +if (ret < 0)
>> +dev_err(info->dev, "%s: fail to write update reg(ret=%d,
>> data=0x%x)\n",
>> +__func__, ret, data);
>> +else {
>> +/* Minimum 16ms delay required before RTC update. */
>> +msleep(MAX77686_RTC_UPDATE_DELAY);
>> +}
>> +
>> +return ret;
>> +}
>
> This function has about ten callers. No way in the world do we want it
> to be inlined! Probably the compiler will just ignore your `inline',
> but we should remove it to be sure.
>
Okay, I'll remove it.
>>
>> ...
>>
>> +static const struct rtc_class_ops max77686_rtc_ops = {
>> +.read_time = max77686_rtc_read_time,
>> +.set_time = max77686_rtc_set_time,
>> +.read_alarm = max77686_rtc_read_alarm,
>> +.set_alarm = max77686_rtc_set_alarm,
>> +.alarm_irq_enable = max77686_rtc_alarm_irq_enable,
>> +};
>> +
>> +#ifdef MAX77686_RTC_WTSR_SMPL
>
> hm, this is always undefined. Why is this code here?
>
Yes, you're right, It isn't needed. I'll fix it right.
>> +static void max77686_rtc_enable_wtsr(struct max77686_rtc_info *info, bool
>> enable)
>> +{
>> +int ret;
>> +unsigned int val, mask;
>> +
>> +if (enable)
>> +val = (1 << WTSR_EN_SHIFT) | (3 << WTSRT_SHIFT);
>> +else
>> +val = 0;
>> +
>> +mask = WTSR_EN_MASK | WTSRT_MASK;
>> +
>> +dev_info(info->dev, "%s: %s WTSR\n", __func__,
>> +enable ? "enable" : "disable");
>> +
>> +ret = regmap_update_bits(info->max77686->rtc_regmap,
>> + MAX77686_WTSR_SMPL_CNTL, mask, val);
>> +if (ret < 0) {
>> +dev_err(info->dev, "%s: fail to update WTSR reg(%d)\n",
>> +__func__, ret);
>> +return;
>> +}
>> +
>> +max77686_rtc_update(info, MAX77686_RTC_WRITE);
>> +}
>> +
>> +static void max77686_rtc_enable_smpl(struct max77686_rtc_info *info, bool
>> enable)
>> +{
>> +int ret;
>> +unsigned int val, mask;
>> +
>> +if (enable)
>> +val = (1 << SMPL_EN_SHIFT) | (0 << SMPLT_SHIFT);
>> +else
>> +val = 0;
>> +
>> +mask = SMPL_EN_MASK | SMPLT_MASK;
>> +
>> +dev_info(info->dev, "%s: %s SMPL\n", __func__,
>> +enable ? "enable" : "disable");
>> +
>> +ret = regmap_update_bits(info->max77686->rtc_regmap,
>> + MAX77686_WTSR_SMPL_CNTL, mask, val);
>> +if (ret < 0) {
>> +dev_err(info->dev, "%s: fail to update SMPL reg(%d)\n",
>> +__func__, ret);
>> +return;
>> +}
>> +
>> +max77686_rtc_update(info, MAX77686_RTC_WRITE);
>> +
>> +val = 0;
>> +regmap_read(info->max77686->rtc_regmap, MAX77686_WTSR_SMPL_CNTL, &val);
>> +pr_info("%s: WTSR_SMPL(0x%02x)\n", __func__, val);
>> +}
>> +#endif /* MAX77686_RTC_WTSR_SMPL */
>>
>> ...
>>
>> +static int __devinit max77686_rtc_probe(struct platform_device *pdev)
>
> CONFIG_HOTPLUG is removed, so we should stop using devinit, devexit,
> devinit_p, etc.
>
>>
>> ...
>>
>
> --- a/drivers/rtc/rtc-max77686.c~rtc-max77686-add-maxim-77686-driver-fix
> +++ a/drivers/rtc/rtc-max77686.c
> @@ -129,7 +129,7 @@ static int max77686_rtc_tm_to_data(struc
> return 0;
> }
>
> -static inline int max77686_rtc_update(struct max77686_rtc_info *info,
> +static int max77686_rtc_update(struct max77686_rtc_info *info,
> enum MAX77686_RTC_OP op)
> {
> int ret;
> @@ -501,7 +501,7 @@ static struct regmap_config max77686_rtc
> .val_bits = 8,
> };
>
> -static int __devinit max77686_rtc_probe(struct platform_device *pdev)
> +static int max77686_rtc_probe(struct platform_device *pdev)
> {
> struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
> st
Re: [rtc-linux] [RESEND PATCH] RTC: MAX77686: Add Maxim 77686 driver
On Tue, 2012-12-18 at 16:30 -0800, Andrew Morton wrote:
> On Wed, 28 Nov 2012 15:50:57 +0900
> Jonghwa Lee wrote:
>
> > Add driver for support max77686 rtc.
> > MAX77686 rtc support smpl and wtsr mode. It has two alarm register
> > which can be used for alarming to wake system up. This drvier uses regmap
> > to access its register.
> >
> > ...
> >
> > +static inline int max77686_rtc_calculate_wday(u8 shifted)
> > +{
> > + int counter = -1;
> > + while (shifted) {
> > + shifted >>= 1;
> > + counter++;
> > + }
> > + return counter;
> > +}
>
> We should be able to use log2() in here?
fls(shifted) - 1
--
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: [rtc-linux] [RESEND PATCH] RTC: MAX77686: Add Maxim 77686 driver
On Wed, 28 Nov 2012 15:50:57 +0900
Jonghwa Lee wrote:
> Add driver for support max77686 rtc.
> MAX77686 rtc support smpl and wtsr mode. It has two alarm register
> which can be used for alarming to wake system up. This drvier uses regmap
> to access its register.
>
> ...
>
> +static inline int max77686_rtc_calculate_wday(u8 shifted)
> +{
> + int counter = -1;
> + while (shifted) {
> + shifted >>= 1;
> + counter++;
> + }
> + return counter;
> +}
We should be able to use log2() in here?
>
> ...
>
> +static inline int max77686_rtc_update(struct max77686_rtc_info *info,
> + enum MAX77686_RTC_OP op)
> +{
> + int ret;
> + unsigned int data;
> +
> + switch (op) {
> + case MAX77686_RTC_WRITE:
> + data = 1 << RTC_UDR_SHIFT;
> + break;
> + case MAX77686_RTC_READ:
> + data = 1 << RTC_RBUDR_SHIFT;
> + break;
> + }
> +
> + ret = regmap_update_bits(info->max77686->rtc_regmap,
> + MAX77686_RTC_UPDATE0, data, data);
> + if (ret < 0)
> + dev_err(info->dev, "%s: fail to write update reg(ret=%d,
> data=0x%x)\n",
> + __func__, ret, data);
> + else {
> + /* Minimum 16ms delay required before RTC update. */
> + msleep(MAX77686_RTC_UPDATE_DELAY);
> + }
> +
> + return ret;
> +}
This function has about ten callers. No way in the world do we want it
to be inlined! Probably the compiler will just ignore your `inline',
but we should remove it to be sure.
>
> ...
>
> +static const struct rtc_class_ops max77686_rtc_ops = {
> + .read_time = max77686_rtc_read_time,
> + .set_time = max77686_rtc_set_time,
> + .read_alarm = max77686_rtc_read_alarm,
> + .set_alarm = max77686_rtc_set_alarm,
> + .alarm_irq_enable = max77686_rtc_alarm_irq_enable,
> +};
> +
> +#ifdef MAX77686_RTC_WTSR_SMPL
hm, this is always undefined. Why is this code here?
> +static void max77686_rtc_enable_wtsr(struct max77686_rtc_info *info, bool
> enable)
> +{
> + int ret;
> + unsigned int val, mask;
> +
> + if (enable)
> + val = (1 << WTSR_EN_SHIFT) | (3 << WTSRT_SHIFT);
> + else
> + val = 0;
> +
> + mask = WTSR_EN_MASK | WTSRT_MASK;
> +
> + dev_info(info->dev, "%s: %s WTSR\n", __func__,
> + enable ? "enable" : "disable");
> +
> + ret = regmap_update_bits(info->max77686->rtc_regmap,
> + MAX77686_WTSR_SMPL_CNTL, mask, val);
> + if (ret < 0) {
> + dev_err(info->dev, "%s: fail to update WTSR reg(%d)\n",
> + __func__, ret);
> + return;
> + }
> +
> + max77686_rtc_update(info, MAX77686_RTC_WRITE);
> +}
> +
> +static void max77686_rtc_enable_smpl(struct max77686_rtc_info *info, bool
> enable)
> +{
> + int ret;
> + unsigned int val, mask;
> +
> + if (enable)
> + val = (1 << SMPL_EN_SHIFT) | (0 << SMPLT_SHIFT);
> + else
> + val = 0;
> +
> + mask = SMPL_EN_MASK | SMPLT_MASK;
> +
> + dev_info(info->dev, "%s: %s SMPL\n", __func__,
> + enable ? "enable" : "disable");
> +
> + ret = regmap_update_bits(info->max77686->rtc_regmap,
> + MAX77686_WTSR_SMPL_CNTL, mask, val);
> + if (ret < 0) {
> + dev_err(info->dev, "%s: fail to update SMPL reg(%d)\n",
> + __func__, ret);
> + return;
> + }
> +
> + max77686_rtc_update(info, MAX77686_RTC_WRITE);
> +
> + val = 0;
> + regmap_read(info->max77686->rtc_regmap, MAX77686_WTSR_SMPL_CNTL, &val);
> + pr_info("%s: WTSR_SMPL(0x%02x)\n", __func__, val);
> +}
> +#endif /* MAX77686_RTC_WTSR_SMPL */
>
> ...
>
> +static int __devinit max77686_rtc_probe(struct platform_device *pdev)
CONFIG_HOTPLUG is removed, so we should stop using devinit, devexit,
devinit_p, etc.
>
> ...
>
--- a/drivers/rtc/rtc-max77686.c~rtc-max77686-add-maxim-77686-driver-fix
+++ a/drivers/rtc/rtc-max77686.c
@@ -129,7 +129,7 @@ static int max77686_rtc_tm_to_data(struc
return 0;
}
-static inline int max77686_rtc_update(struct max77686_rtc_info *info,
+static int max77686_rtc_update(struct max77686_rtc_info *info,
enum MAX77686_RTC_OP op)
{
int ret;
@@ -501,7 +501,7 @@ static struct regmap_config max77686_rtc
.val_bits = 8,
};
-static int __devinit max77686_rtc_probe(struct platform_device *pdev)
+static int max77686_rtc_probe(struct platform_device *pdev)
{
struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
struct max77686_rtc_info *info;
@@ -575,7 +575,7 @@ out:
return ret;
}
-static int __devexit max77686_rtc_remove(struct platform_device *pdev)
+static int max77686_rtc_remove(struct platform_device *pdev)
{
struct max77686_rtc_info *info = platform_get_drvdata(pdev);
@@ -623,7 +623,7 @@ static struct
[RESEND PATCH] RTC: MAX77686: Add Maxim 77686 driver
Add driver for support max77686 rtc.
MAX77686 rtc support smpl and wtsr mode. It has two alarm register
which can be used for alarming to wake system up. This drvier uses regmap
to access its register.
Signed-off-by: Chiwoong Byun
Signed-off-by: Jonghwa Lee
Signed-off-by: Myugnjoo Ham
Signed-off-by: Kyungmin Park
---
drivers/rtc/Kconfig| 10 +
drivers/rtc/Makefile |1 +
drivers/rtc/rtc-max77686.c | 645
3 files changed, 656 insertions(+), 0 deletions(-)
create mode 100644 drivers/rtc/rtc-max77686.c
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 19c03ab..4848563 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -233,6 +233,16 @@ config RTC_DRV_MAX8998
This driver can also be built as a module. If so, the module
will be called rtc-max8998.
+config RTC_DRV_MAX77686
+ tristate "Maxim MAX77686"
+ depends on MFD_MAX77686
+ help
+ If you say yes here you will get support for the
+ RTC of Maxim MAX77686 PMIC.
+
+ This driver can also be built as a module. If so, the module
+ will be called rtc-max77686.
+
config RTC_DRV_RS5C372
tristate "Ricoh R2025S/D, RS5C372A/B, RV5C386, RV5C387A"
help
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 56297f0..3cc94c9 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_RTC_DRV_MAX8907) += rtc-max8907.o
obj-$(CONFIG_RTC_DRV_MAX8925) += rtc-max8925.o
obj-$(CONFIG_RTC_DRV_MAX8998) += rtc-max8998.o
obj-$(CONFIG_RTC_DRV_MAX6902) += rtc-max6902.o
+obj-$(CONFIG_RTC_DRV_MAX77686) += rtc-max77686.o
obj-$(CONFIG_RTC_DRV_MC13XXX) += rtc-mc13xxx.o
obj-$(CONFIG_RTC_DRV_MSM6242) += rtc-msm6242.o
obj-$(CONFIG_RTC_DRV_MPC5121) += rtc-mpc5121.o
diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
new file mode 100644
index 000..63da55a
--- /dev/null
+++ b/drivers/rtc/rtc-max77686.c
@@ -0,0 +1,645 @@
+/*
+ * RTC driver for Maxim MAX77686
+ *
+ * Copyright (C) 2012 Samsung Electronics Co.Ltd
+ *
+ * based on rtc-max8997.c
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* RTC Control Register */
+#define BCD_EN_SHIFT 0
+#define BCD_EN_MASK(1 << BCD_EN_SHIFT)
+#define MODEL24_SHIFT 1
+#define MODEL24_MASK (1 << MODEL24_SHIFT)
+/* RTC Update Register1 */
+#define RTC_UDR_SHIFT 0
+#define RTC_UDR_MASK (1 << RTC_UDR_SHIFT)
+#define RTC_RBUDR_SHIFT4
+#define RTC_RBUDR_MASK (1 << RTC_RBUDR_SHIFT)
+/* WTSR and SMPL Register */
+#define WTSRT_SHIFT0
+#define SMPLT_SHIFT2
+#define WTSR_EN_SHIFT 6
+#define SMPL_EN_SHIFT 7
+#define WTSRT_MASK (3 << WTSRT_SHIFT)
+#define SMPLT_MASK (3 << SMPLT_SHIFT)
+#define WTSR_EN_MASK (1 << WTSR_EN_SHIFT)
+#define SMPL_EN_MASK (1 << SMPL_EN_SHIFT)
+/* RTC Hour register */
+#define HOUR_PM_SHIFT 6
+#define HOUR_PM_MASK (1 << HOUR_PM_SHIFT)
+/* RTC Alarm Enable */
+#define ALARM_ENABLE_SHIFT 7
+#define ALARM_ENABLE_MASK (1 << ALARM_ENABLE_SHIFT)
+
+#define MAX77686_RTC_UPDATE_DELAY 16
+#undef MAX77686_RTC_WTSR_SMPL
+
+enum {
+ RTC_SEC = 0,
+ RTC_MIN,
+ RTC_HOUR,
+ RTC_WEEKDAY,
+ RTC_MONTH,
+ RTC_YEAR,
+ RTC_DATE,
+ RTC_NR_TIME
+};
+
+struct max77686_rtc_info {
+ struct device *dev;
+ struct max77686_dev *max77686;
+ struct i2c_client *rtc;
+ struct rtc_device *rtc_dev;
+ struct mutexlock;
+
+ struct regmap *regmap;
+
+ int virq;
+ int rtc_24hr_mode;
+};
+
+enum MAX77686_RTC_OP {
+ MAX77686_RTC_WRITE,
+ MAX77686_RTC_READ,
+};
+
+static inline int max77686_rtc_calculate_wday(u8 shifted)
+{
+ int counter = -1;
+ while (shifted) {
+ shifted >>= 1;
+ counter++;
+ }
+ return counter;
+}
+
+static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
+ int rtc_24hr_mode)
+{
+ tm->tm_sec = data[RTC_SEC] & 0x7f;
+ tm->tm_min = data[RTC_MIN] & 0x7f;
+ if (rtc_24hr_mode)
+ tm->tm_hour = data[RTC_HOUR] & 0x1f;
+ else {
+ tm->tm_hour = data[RTC_HOUR] & 0x0f;
+ if (data[RTC_HOUR] & HOUR
