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

[Krzysztof Kozlowski]

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.

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

Best regards,
Krzysztof


--
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/