Re: [PATCH v6 22/23] rtc: Add driver for Maxim 77802 PMIC Real-Time-Clock

From: Javier Martinez Canillas
Date: Fri Jul 04 2014 - 09:23:59 EST

Next message: Nicolas Ferre: "Re: [PATCH] drivers: video: fbdev: atmel_lcdfb.c: Add ability to inverted backlight PWM."
Previous message: Masami Hiramatsu: "Re: [RFC][PATCH 0/3] ftrace: Add dynamically allocated trampolines"
In reply to: Krzysztof Kozlowski: "Re: [PATCH v6 22/23] rtc: Add driver for Maxim 77802 PMIC Real-Time-Clock"
Next in thread: Javier Martinez Canillas: "[PATCH v6 21/23] clk: max77802: Add DT binding documentation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hello Krzysztof,

On 07/04/2014 03:11 PM, Krzysztof Kozlowski wrote:
> On piÄ, 2014-07-04 at 14:52 +0200, Javier Martinez Canillas wrote:
>> Hello Krzysztof,
>>
>> Thanks a lot for your feedback.
>>
>> On 07/04/2014 01:56 PM, Krzysztof Kozlowski wrote:
>> > On piÄ, 2014-07-04 at 11:55 +0200, Javier Martinez Canillas wrote:
>> >> The MAX7802 PMIC has a Real-Time-Clock (RTC) with two alarms.
>> >> This patch adds support for the RTC and is based on a driver
>> >> added by Simon Glass to the Chrome OS kernel 3.8 tree.
>> >>
>> >> Signed-off-by: Javier Martinez Canillas <javier.martinez@xxxxxxxxxxxxxxx>
>> >> ---
>> >>
>> >> Changes since v5: None
>> >>
>> >> Changes since v4: None
>> >>
>> >> Changes since v3: None
>> >> ---
>> >> drivers/rtc/Kconfig | 10 +
>> >> drivers/rtc/Makefile | 1 +
>> >> drivers/rtc/rtc-max77802.c | 637 +++++++++++++++++++++++++++++++++++++++++++++
>> >> 3 files changed, 648 insertions(+)
>> >> create mode 100644 drivers/rtc/rtc-max77802.c
>> >>
>> >> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>> >> index a672dd1..243ac72 100644
>> >> --- a/drivers/rtc/Kconfig
>> >> +++ b/drivers/rtc/Kconfig
>> >> @@ -288,6 +288,16 @@ 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_MAX77802
>> >> + tristate "Maxim 77802 RTC"
>> >> + depends on MFD_MAX77686
>> >> + help
>> >> + If you say yes here you will get support for the
>> >> + RTC of Maxim MAX77802 PMIC.
>> >> +
>> >> + This driver can also be built as a module. If so, the module
>> >> + will be called rtc-max77802.
>> >> +
>> >> config RTC_DRV_RS5C372
>> >> tristate "Ricoh R2025S/D, RS5C372A/B, RV5C386, RV5C387A"
>> >> help
>> >> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
>> >> index 70347d0..247de78 100644
>> >> --- a/drivers/rtc/Makefile
>> >> +++ b/drivers/rtc/Makefile
>> >> @@ -81,6 +81,7 @@ obj-$(CONFIG_RTC_DRV_MAX8998) += rtc-max8998.o
>> >> obj-$(CONFIG_RTC_DRV_MAX8997) += rtc-max8997.o
>> >> obj-$(CONFIG_RTC_DRV_MAX6902) += rtc-max6902.o
>> >> obj-$(CONFIG_RTC_DRV_MAX77686) += rtc-max77686.o
>> >> +obj-$(CONFIG_RTC_DRV_MAX77802) += rtc-max77802.o
>> >> obj-$(CONFIG_RTC_DRV_MC13XXX) += rtc-mc13xxx.o
>> >> obj-$(CONFIG_RTC_DRV_MCP795) += rtc-mcp795.o
>> >> obj-$(CONFIG_RTC_DRV_MSM6242) += rtc-msm6242.o
>> >> diff --git a/drivers/rtc/rtc-max77802.c b/drivers/rtc/rtc-max77802.c
>> >> new file mode 100644
>> >> index 0000000..2f4fc2e
>> >> --- /dev/null
>> >> +++ b/drivers/rtc/rtc-max77802.c
>> >> @@ -0,0 +1,637 @@
>> >> +/*
>> >> + * RTC driver for Maxim MAX77802
>> >> + *
>> >> + * Copyright (C) 2013 Google, Inc
>> >> + *
>> >> + * 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 <linux/slab.h>
>> >> +#include <linux/rtc.h>
>> >> +#include <linux/delay.h>
>> >> +#include <linux/mutex.h>
>> >> +#include <linux/module.h>
>> >> +#include <linux/platform_device.h>
>> >> +#include <linux/mfd/max77686-private.h>
>> >> +#include <linux/irqdomain.h>
>> >> +#include <linux/regmap.h>
>> >> +
>> >> +/* 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_SHIFT 4
>> >> +#define RTC_RBUDR_MASK (1 << RTC_RBUDR_SHIFT)
>> >> +/* WTSR and SMPL Register */
>> >> +#define WTSRT_SHIFT 0
>> >> +#define SMPLT_SHIFT 2
>> >> +#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)
>> >> +
>> >> +/* For the RTCAE1 register, we write this value to enable the alarm */
>> >> +#define ALARM_ENABLE_VALUE 0x77
>> >> +
>> >> +#define MAX77802_RTC_UPDATE_DELAY_US 200
>> >> +#undef MAX77802_RTC_WTSR_SMPL
>> >
>> > Hmmm... I am not sure what is the purpose of this undef. It disables
>> > some functions below. I saw same code in 77686 RTC driver but this does
>> > not help me :).
>> >
>> > If this is on purpose can you add a comment explaining the
>> > purpose/cause?
>> >
>>
>> This is a left over from when a combined 77686/802 driver was attempted since as
>> you said the 77686 RTC driver does the same.
>>
>> I just checked MAX77802 data sheet and the MAX77802_RTC_WTSR_SMPL register is to
>> control the SMPL (Sudden Momentary Power Loss) and WTSR (Watchdog Timeout and
>> Software Resets) features.
>>
>> Now, I wanted to figure out why the 77686 driver unset that register and have
>> those conditionals but git blame shows me that this was already in the original
>> commit that added the driver: fca1dd03 ("rtc: max77686: add Maxim 77686 driver").
>>
>> Also, I see that the MAX8997 driver (drivers/rtc/rtc-max8997.c) also has a
>> similar register but actually uses it and doesn't have the conditionals if is
>> disabled. But this driver has two module parameters to control if these features
>> are enabled or not (wtsr_en and smpl_en).
>>
>> If these two features have been disabled since the max77686 driver was merged
>> then I guess that the dead code should be removed from that driver as well? Or
>> do you have more hints about why it has been disabled?
>
> If the max77802 driver in current form works fine for your setup then I
> think these functions can be removed completely. It should not harm
> since this is dead code anyway and it will simplify the driver.
>

Agreed, I'll just remove it.

> I think the same applies to max77686 (especially that as you said - this
> is dead since beginning). Just remove it in separate patch.
>

Ok, I'll add that as a separate cleanup patch in the next version of the series
as well.

> Best regards,
> Krzysztof
>
>

Best regards,
Javier

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

Next message: Nicolas Ferre: "Re: [PATCH] drivers: video: fbdev: atmel_lcdfb.c: Add ability to inverted backlight PWM."
Previous message: Masami Hiramatsu: "Re: [RFC][PATCH 0/3] ftrace: Add dynamically allocated trampolines"
In reply to: Krzysztof Kozlowski: "Re: [PATCH v6 22/23] rtc: Add driver for Maxim 77802 PMIC Real-Time-Clock"
Next in thread: Javier Martinez Canillas: "[PATCH v6 21/23] clk: max77802: Add DT binding documentation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]