From: Krzysztof Kozlowski <k.kozlowski@samsung.com>
To: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Cc: Lee Jones <lee.jones@linaro.org>, Mark Brown <broonie@kernel.org>,
Mike Turquette <mturquette@linaro.org>,
Liam Girdwood <lgirdwood@gmail.com>,
Alessandro Zummo <a.zummo@towertech.it>,
Kukjin Kim <kgene.kim@samsung.com>,
Doug Anderson <dianders@chromium.org>,
Olof Johansson <olof@lixom.net>,
Tomeu Vizoso <tomeu.vizoso@collabora.com>,
Yadwinder Singh Brar <yadi.brar01@gmail.com>,
Tushar Behera <trblinux@gmail.com>,
Andreas Farber <afaerber@suse.de>,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 22/23] rtc: Add driver for Maxim 77802 PMIC Real-Time-Clock
Date: Fri, 04 Jul 2014 15:11:18 +0200 [thread overview]
Message-ID: <1404479478.2653.5.camel@AMDC1943> (raw)
In-Reply-To: <53B6A3A3.9090300@collabora.co.uk>
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@collabora.co.uk>
> >> ---
> >>
> >> 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
next prev parent reply other threads:[~2014-07-04 13:11 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-04 9:54 [PATCH v6 00/23] Add Maxim 77802 PMIC support Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 01/23] mfd: max77686: Convert to use regmap_irq Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 02/23] mfd: max77686: Add power management support Javier Martinez Canillas
2014-07-04 10:51 ` Krzysztof Kozlowski
2014-07-04 9:55 ` [PATCH v6 03/23] mfd: max77686: Don't define dummy function if OF isn't enabled Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 04/23] mfd: max77686: Make platform data over-rule DT Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 05/23] mfd: max77686: Return correct error when pdata isn't found Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 06/23] mfd: max77686: Make error checking consistent Javier Martinez Canillas
2014-07-04 10:54 ` Krzysztof Kozlowski
2014-07-04 9:55 ` [PATCH v6 07/23] mfd: max77686: Remove unneeded OOM error message Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 08/23] mfd: max77686: Add Dynamic Voltage Scaling (DVS) support Javier Martinez Canillas
2014-07-04 11:15 ` Krzysztof Kozlowski
2014-07-04 11:32 ` Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 09/23] rtc: max77686: Allow the max77686 rtc to wakeup the system Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 10/23] clk: max77686: Add DT include for MAX77686 PMIC clock Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 11/23] clk: Add generic driver for Maxim PMIC clocks Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 12/23] clk: max77686: Convert to the generic max clock driver Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 13/23] clk: max77686: Improve Maxim 77686 PMIC clocks binding Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 14/23] regmap: Add regmap_reg_copy function Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 15/23] regulator: max77686: Setup DVS-related GPIOs on probe Javier Martinez Canillas
2014-07-10 10:08 ` amit daniel kachhap
[not found] ` <CADGdYn4EURaHinWVksQSPzVr=CkZYcrqvWsO7mKDz_gY9XB__Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-07-10 13:25 ` Lee Jones
2014-07-11 2:03 ` Javier Martinez Canillas
2014-07-11 9:31 ` amit daniel kachhap
2014-07-04 9:55 ` [PATCH v6 16/23] mfd: max77686: Add documentation for DVS bindings Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 17/23] mfd: max77686: Add Maxim 77802 PMIC support Javier Martinez Canillas
2014-07-04 11:30 ` Krzysztof Kozlowski
2014-07-04 11:35 ` Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 18/23] mfd: max77802: Add DT binding documentation Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 19/23] regulator: Add driver for Maxim 77802 PMIC regulators Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 20/23] clk: Add driver for Maxim 77802 PMIC clocks Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 21/23] clk: max77802: Add DT binding documentation Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 22/23] rtc: Add driver for Maxim 77802 PMIC Real-Time-Clock Javier Martinez Canillas
2014-07-04 11:56 ` Krzysztof Kozlowski
2014-07-04 12:52 ` Javier Martinez Canillas
2014-07-04 13:11 ` Krzysztof Kozlowski [this message]
2014-07-04 13:23 ` Javier Martinez Canillas
2014-07-04 9:55 ` [PATCH v6 23/23] ARM: dts: Add max77802 to exynos5420-peach-pit and exynos5800-peach-pi Javier Martinez Canillas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1404479478.2653.5.camel@AMDC1943 \
--to=k.kozlowski@samsung.com \
--cc=a.zummo@towertech.it \
--cc=afaerber@suse.de \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dianders@chromium.org \
--cc=javier.martinez@collabora.co.uk \
--cc=kgene.kim@samsung.com \
--cc=lee.jones@linaro.org \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=mturquette@linaro.org \
--cc=olof@lixom.net \
--cc=tomeu.vizoso@collabora.com \
--cc=trblinux@gmail.com \
--cc=yadi.brar01@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).