Re: [PATCH v3] rtc: omap: add support for pmic_power_en

2014-10-29 Thread Johan Hovold
On Tue, Oct 28, 2014 at 02:18:05PM -0700, Andrew Morton wrote:
 On Tue, 28 Oct 2014 09:36:33 +0100 Johan Hovold jo...@kernel.org wrote:
 
   But it doesn't explain *why* we want the alarm to trigger before
   returning.
  
  Should we really require every power-off handler to document arch
  behaviour (even if its inconsistent and currently undocumented); in
  this case that some arches return to user-space where we may oops if
  called from process 0 (e.g. systemd, but not if using sysvinit)?
 
 The kernel really doesn't have a problem related to excessive amounts
 of useful code comments :(
 
 The bottom line is: did I provide a reader with the ability to
 understand why the code is this way?  If no then improvements beckon.
 
 This does look like one code site where an elaborate explanation is
 warranted.  There's no way in which a reader can get to your above
 paragraph from the current rtc-omap.c.

I agree with you that I should add a comment on why that mdelay is
there to make it perfectly clear and obvious that it's purpose is to let
the alarm trigger, which in turn causes the pmic to power off the
system.

It is a power-off handler, and any power-off handler should not return
until it has at least attempted to power off the system. In this case,
that mandates a two-second delay.

So far, so good. I don't agree with you that every power-off handler
should document what happens if it fails to power-off the system. That
is, to describe what arches will happily return to user space, and under
what conditions (e.g. what init system is used) that this will cause a
panic.

If anything that belongs in arch code or kernel/reboot.c, and is
also something that is likely to change over time (consider the
power-off-handler call chains).

Johan
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3] rtc: omap: add support for pmic_power_en

2014-10-29 Thread Johan Hovold
On Tue, Oct 28, 2014 at 09:36:33AM +0100, Johan Hovold wrote:
 On Mon, Oct 27, 2014 at 03:40:31PM -0700, Andrew Morton wrote:
  On Mon, 27 Oct 2014 09:09:28 +0100 Johan Hovold jo...@kernel.org wrote:

 But in general, how do you want to handle updates to a single patch in a
 series you already have in your tree? Do you prefer a proper
 incremental-fix patch (with commit message), just an updated single
 patch, or a resend of the whole series?

How should I best send the updated patch? Can you just replace the
current three incremental patches:

rtc-omap-add-support-for-pmic_power_en.patch
rtc-omap-add-support-for-pmic_power_en-v3.patch
rtc-omap-add-support-for-pmic_power_en-v3-fix.patch

that you have in your tree, with a single new v4 which adds a more
elaborate comment?

Thanks,
Johan
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3] rtc: omap: add support for pmic_power_en

2014-10-29 Thread Andrew Morton
On Wed, 29 Oct 2014 13:50:05 +0100 Johan Hovold jo...@kernel.org wrote:

 On Tue, Oct 28, 2014 at 09:36:33AM +0100, Johan Hovold wrote:
  On Mon, Oct 27, 2014 at 03:40:31PM -0700, Andrew Morton wrote:
   On Mon, 27 Oct 2014 09:09:28 +0100 Johan Hovold jo...@kernel.org wrote:
 
  But in general, how do you want to handle updates to a single patch in a
  series you already have in your tree? Do you prefer a proper
  incremental-fix patch (with commit message), just an updated single
  patch, or a resend of the whole series?
 
 How should I best send the updated patch? Can you just replace the
 current three incremental patches:
 
   rtc-omap-add-support-for-pmic_power_en.patch
   rtc-omap-add-support-for-pmic_power_en-v3.patch
   rtc-omap-add-support-for-pmic_power_en-v3-fix.patch
 
 that you have in your tree, with a single new v4 which adds a more
 elaborate comment?

Yep, that works.  I'll almost always turn the new patch into an
incremental, mainly so that I (and others) can see what was changed.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3] rtc: omap: add support for pmic_power_en

2014-10-28 Thread Johan Hovold
On Mon, Oct 27, 2014 at 03:40:31PM -0700, Andrew Morton wrote:
 On Mon, 27 Oct 2014 09:09:28 +0100 Johan Hovold jo...@kernel.org wrote:
 
  Add new property ti,system-power-controller to register the RTC as a
  power-off handler.
  
  Some RTC IP revisions can control an external PMIC via the pmic_power_en
  pin, which can be configured to transition to OFF on ALARM2 events and
  back to ON on subsequent ALARM (wakealarm) events.
  
  This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
  
  [1] https://www.mail-archive.com/linux-omap@vger.kernel.org/msg82127.html
  
  Tested-by: Felipe Balbi ba...@ti.com
  Signed-off-by: Johan Hovold jo...@kernel.org
  ---
  
  Changes since v2:
   - add two-second delay to allow alarm to trigger before returning
 
 hmpf.  As this sentence is below the ^--- it doesn't get into the
 changelog.

We usually don't keep the patch-revision change log in the commit message
(e.g. put in the cover letter).

But in general, how do you want to handle updates to a single patch in a
series you already have in your tree? Do you prefer a proper
incremental-fix patch (with commit message), just an updated single
patch, or a resend of the whole series?

  ...
 
  +static void omap_rtc_power_off(void)
  +{
  +   struct omap_rtc *rtc = omap_rtc_power_off_rtc;
  +   struct rtc_time tm;
  +   unsigned long now;
  +   u32 val;
  +
  +   /* enable pmic_power_en control */
  +   val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
  +   rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN);
  +
  +   /* set alarm two seconds from now */
  +   omap_rtc_read_time_raw(rtc, tm);
  +   bcd2tm(tm);
  +   rtc_tm_to_time(tm, now);
  +   rtc_time_to_tm(now + 2, tm);
  +
  +   if (tm2bcd(tm)  0) {
  +   dev_err(rtc-rtc-dev, power off failed\n);
  +   return;
  +   }
  +
  +   rtc_wait_not_busy(rtc);
  +
  +   rtc_write(rtc, OMAP_RTC_ALARM2_SECONDS_REG, tm.tm_sec);
  +   rtc_write(rtc, OMAP_RTC_ALARM2_MINUTES_REG, tm.tm_min);
  +   rtc_write(rtc, OMAP_RTC_ALARM2_HOURS_REG, tm.tm_hour);
  +   rtc_write(rtc, OMAP_RTC_ALARM2_DAYS_REG, tm.tm_mday);
  +   rtc_write(rtc, OMAP_RTC_ALARM2_MONTHS_REG, tm.tm_mon);
  +   rtc_write(rtc, OMAP_RTC_ALARM2_YEARS_REG, tm.tm_year);
  +
  +   /*
  +* enable ALARM2 interrupt
  +*
  +* NOTE: this fails on AM3352 if rtc_write (writeb) is used
  +*/
  +   val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
  +   rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
  +   val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
  +
  +   mdelay(2000);
 
 And it is uncommented.
 
 How on earth is a reader to know why this is here?

The comment above the function reads:

 * The RTC can be used to control an external PMIC via the pmic_power_en pin,
 * which can be configured to transition to OFF on ALARM2 events.
 *
 * Notes:
 * The two-second alarm offset is the shortest offset possible as the alarm
 * registers must be set before the next timer update and the offset
 * calculation is too heavy for everything to be done within a single access
 * period (~15us).

So it's effect is at least fairly obvious and documented.

 I can do this
 
 --- a/drivers/rtc/rtc-omap.c~rtc-omap-add-support-for-pmic_power_en-v3-fix
 +++ a/drivers/rtc/rtc-omap.c
 @@ -417,6 +417,7 @@ static void omap_rtc_power_off(void)
   rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
   val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
  
 + /* Allow alarm to trigger before returning */
   mdelay(2000);
  }

Looks good, and I should have put something like that there nonetheless.

 But it doesn't explain *why* we want the alarm to trigger before
 returning.

Should we really require every power-off handler to document arch
behaviour (even if its inconsistent and currently undocumented); in
this case that some arches return to user-space where we may oops if
called from process 0 (e.g. systemd, but not if using sysvinit)?

Johan
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3] rtc: omap: add support for pmic_power_en

2014-10-28 Thread Andrew Morton
On Tue, 28 Oct 2014 09:36:33 +0100 Johan Hovold jo...@kernel.org wrote:

  But it doesn't explain *why* we want the alarm to trigger before
  returning.
 
 Should we really require every power-off handler to document arch
 behaviour (even if its inconsistent and currently undocumented); in
 this case that some arches return to user-space where we may oops if
 called from process 0 (e.g. systemd, but not if using sysvinit)?

The kernel really doesn't have a problem related to excessive amounts
of useful code comments :(

The bottom line is: did I provide a reader with the ability to
understand why the code is this way?  If no then improvements beckon.

This does look like one code site where an elaborate explanation is
warranted.  There's no way in which a reader can get to your above
paragraph from the current rtc-omap.c.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v3] rtc: omap: add support for pmic_power_en

2014-10-27 Thread Johan Hovold
Add new property ti,system-power-controller to register the RTC as a
power-off handler.

Some RTC IP revisions can control an external PMIC via the pmic_power_en
pin, which can be configured to transition to OFF on ALARM2 events and
back to ON on subsequent ALARM (wakealarm) events.

This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]

[1] https://www.mail-archive.com/linux-omap@vger.kernel.org/msg82127.html

Tested-by: Felipe Balbi ba...@ti.com
Signed-off-by: Johan Hovold jo...@kernel.org
---

Changes since v2:
 - add two-second delay to allow alarm to trigger before returning

Andrew, can you replace just this patch in the series that you already
have in your tree, or do you prefer I resend the whole series (with
Felipe's Tested-by tags)?

Thanks,
Johan


 Documentation/devicetree/bindings/rtc/rtc-omap.txt |  9 +-
 drivers/rtc/rtc-omap.c | 95 ++
 2 files changed, 103 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt 
b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
index 5a0f02d34d95..750efd40c72e 100644
--- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt
+++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
@@ -5,11 +5,17 @@ Required properties:
- ti,da830-rtc  - for RTC IP used similar to that on DA8xx SoC family.
- ti,am3352-rtc - for RTC IP used similar to that on AM335x SoC 
family.
This RTC IP has special WAKE-EN Register to enable
-   Wakeup generation for event Alarm.
+   Wakeup generation for event Alarm. It can also be
+   used to control an external PMIC via the
+   pmic_power_en pin.
 - reg: Address range of rtc register set
 - interrupts: rtc timer, alarm interrupts in order
 - interrupt-parent: phandle for the interrupt controller
 
+Optional properties:
+- ti,system-power-controller: whether the rtc is controlling the system power
+  through pmic_power_en
+
 Example:
 
 rtc@1c23000 {
@@ -18,4 +24,5 @@ rtc@1c23000 {
interrupts = 19
  19;
interrupt-parent = intc;
+   ti,system-power-controller;
 };
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index c508e45ca3ce..87cfee751554 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -68,6 +68,15 @@
 
 #define OMAP_RTC_IRQWAKEEN 0x7c
 
+#define OMAP_RTC_ALARM2_SECONDS_REG0x80
+#define OMAP_RTC_ALARM2_MINUTES_REG0x84
+#define OMAP_RTC_ALARM2_HOURS_REG  0x88
+#define OMAP_RTC_ALARM2_DAYS_REG   0x8c
+#define OMAP_RTC_ALARM2_MONTHS_REG 0x90
+#define OMAP_RTC_ALARM2_YEARS_REG  0x94
+
+#define OMAP_RTC_PMIC_REG  0x98
+
 /* OMAP_RTC_CTRL_REG bit fields: */
 #define OMAP_RTC_CTRL_SPLITBIT(7)
 #define OMAP_RTC_CTRL_DISABLE  BIT(6)
@@ -80,6 +89,7 @@
 
 /* OMAP_RTC_STATUS_REG bit fields: */
 #define OMAP_RTC_STATUS_POWER_UP   BIT(7)
+#define OMAP_RTC_STATUS_ALARM2 BIT(7)
 #define OMAP_RTC_STATUS_ALARM  BIT(6)
 #define OMAP_RTC_STATUS_1D_EVENT   BIT(5)
 #define OMAP_RTC_STATUS_1H_EVENT   BIT(4)
@@ -89,6 +99,7 @@
 #define OMAP_RTC_STATUS_BUSY   BIT(0)
 
 /* OMAP_RTC_INTERRUPTS_REG bit fields: */
+#define OMAP_RTC_INTERRUPTS_IT_ALARM2  BIT(4)
 #define OMAP_RTC_INTERRUPTS_IT_ALARM   BIT(3)
 #define OMAP_RTC_INTERRUPTS_IT_TIMER   BIT(2)
 
@@ -98,6 +109,9 @@
 /* OMAP_RTC_IRQWAKEEN bit fields: */
 #define OMAP_RTC_IRQWAKEEN_ALARM_WAKEENBIT(1)
 
+/* OMAP_RTC_PMIC bit fields: */
+#define OMAP_RTC_PMIC_POWER_EN_EN  BIT(16)
+
 /* OMAP_RTC_KICKER values */
 #defineKICK0_VALUE 0x83e70b13
 #defineKICK1_VALUE 0x95a4f1e0
@@ -106,6 +120,7 @@ struct omap_rtc_device_type {
bool has_32kclk_en;
bool has_kicker;
bool has_irqwakeen;
+   bool has_pmic_mode;
bool has_power_up_reset;
 };
 
@@ -115,6 +130,7 @@ struct omap_rtc {
int irq_alarm;
int irq_timer;
u8 interrupts_reg;
+   bool is_pmic_controller;
const struct omap_rtc_device_type *type;
 };
 
@@ -345,6 +361,65 @@ static int omap_rtc_set_alarm(struct device *dev, struct 
rtc_wkalrm *alm)
return 0;
 }
 
+static struct omap_rtc *omap_rtc_power_off_rtc;
+
+/*
+ * omap_rtc_poweroff: RTC-controlled power off
+ *
+ * The RTC can be used to control an external PMIC via the pmic_power_en pin,
+ * which can be configured to transition to OFF on ALARM2 events.
+ *
+ * Notes:
+ * The two-second alarm offset is the shortest offset possible as the alarm
+ * registers must be set before the next timer update and the offset
+ * calculation is too heavy for everything to be done within a single access
+ * period (~15us).
+ *
+ * Called with local interrupts disabled.
+ */
+static void omap_rtc_power_off(void)
+{
+   struct omap_rtc *rtc = omap_rtc_power_off_rtc;
+ 

Re: [PATCH v3] rtc: omap: add support for pmic_power_en

2014-10-27 Thread Felipe Balbi
On Mon, Oct 27, 2014 at 09:09:28AM +0100, Johan Hovold wrote:
 Add new property ti,system-power-controller to register the RTC as a
 power-off handler.
 
 Some RTC IP revisions can control an external PMIC via the pmic_power_en
 pin, which can be configured to transition to OFF on ALARM2 events and
 back to ON on subsequent ALARM (wakealarm) events.
 
 This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
 
 [1] https://www.mail-archive.com/linux-omap@vger.kernel.org/msg82127.html
 
 Tested-by: Felipe Balbi ba...@ti.com
 Signed-off-by: Johan Hovold jo...@kernel.org
 ---
 
 Changes since v2:
  - add two-second delay to allow alarm to trigger before returning
 
 Andrew, can you replace just this patch in the series that you already
 have in your tree, or do you prefer I resend the whole series (with
 Felipe's Tested-by tags)?
 
 Thanks,
 Johan
 
 
  Documentation/devicetree/bindings/rtc/rtc-omap.txt |  9 +-
  drivers/rtc/rtc-omap.c | 95 
 ++
  2 files changed, 103 insertions(+), 1 deletion(-)
 
 diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt 
 b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
 index 5a0f02d34d95..750efd40c72e 100644
 --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt
 +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
 @@ -5,11 +5,17 @@ Required properties:
   - ti,da830-rtc  - for RTC IP used similar to that on DA8xx SoC family.
   - ti,am3352-rtc - for RTC IP used similar to that on AM335x SoC 
 family.
   This RTC IP has special WAKE-EN Register to enable
 - Wakeup generation for event Alarm.
 + Wakeup generation for event Alarm. It can also be
 + used to control an external PMIC via the
 + pmic_power_en pin.
  - reg: Address range of rtc register set
  - interrupts: rtc timer, alarm interrupts in order
  - interrupt-parent: phandle for the interrupt controller
  
 +Optional properties:
 +- ti,system-power-controller: whether the rtc is controlling the system power

isn't there a discussion going on to drop the vendor prefix ? I wonder
if we should just use the final binding to avoid supporting this for
rtc-omap too. OTOH, all of that can be hidden under
of_is_system_power_controller() itself.

-- 
balbi


signature.asc
Description: Digital signature


Re: [PATCH v3] rtc: omap: add support for pmic_power_en

2014-10-27 Thread Johan Hovold
On Mon, Oct 27, 2014 at 11:45:44AM -0500, Felipe Balbi wrote:
 On Mon, Oct 27, 2014 at 09:09:28AM +0100, Johan Hovold wrote:
  Add new property ti,system-power-controller to register the RTC as a
  power-off handler.
  
  Some RTC IP revisions can control an external PMIC via the pmic_power_en
  pin, which can be configured to transition to OFF on ALARM2 events and
  back to ON on subsequent ALARM (wakealarm) events.
  
  This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
  
  [1] https://www.mail-archive.com/linux-omap@vger.kernel.org/msg82127.html
  
  Tested-by: Felipe Balbi ba...@ti.com
  Signed-off-by: Johan Hovold jo...@kernel.org
  ---
  
  Changes since v2:
   - add two-second delay to allow alarm to trigger before returning
  
  Andrew, can you replace just this patch in the series that you already
  have in your tree, or do you prefer I resend the whole series (with
  Felipe's Tested-by tags)?
  
  Thanks,
  Johan
  
  
   Documentation/devicetree/bindings/rtc/rtc-omap.txt |  9 +-
   drivers/rtc/rtc-omap.c | 95 
  ++
   2 files changed, 103 insertions(+), 1 deletion(-)
  
  diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt 
  b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
  index 5a0f02d34d95..750efd40c72e 100644
  --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt
  +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
  @@ -5,11 +5,17 @@ Required properties:
  - ti,da830-rtc  - for RTC IP used similar to that on DA8xx SoC family.
  - ti,am3352-rtc - for RTC IP used similar to that on AM335x SoC 
  family.
  This RTC IP has special WAKE-EN Register to enable
  -   Wakeup generation for event Alarm.
  +   Wakeup generation for event Alarm. It can also be
  +   used to control an external PMIC via the
  +   pmic_power_en pin.
   - reg: Address range of rtc register set
   - interrupts: rtc timer, alarm interrupts in order
   - interrupt-parent: phandle for the interrupt controller
   
  +Optional properties:
  +- ti,system-power-controller: whether the rtc is controlling the system 
  power
 
 isn't there a discussion going on to drop the vendor prefix ? I wonder
 if we should just use the final binding to avoid supporting this for
 rtc-omap too. OTOH, all of that can be hidden under
 of_is_system_power_controller() itself.

Exactly, Romain intends to support both versions (i.e. with and without
the prefix) with his helper function as there are already other uses of
this variant in the wild.

As I believe I mentioned in my cover letter, I suggest simply updating
to the generic property name (i.e. use the helper and drop the prefix)
once that code has been merged. Either way, we should always be able
to change the property name before it has been used in a mainline
release (as long as we update the driver and any dts use in one commit
in order not to break bisectability).

Johan
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3] rtc: omap: add support for pmic_power_en

2014-10-27 Thread Felipe Balbi
On Mon, Oct 27, 2014 at 05:56:54PM +0100, Johan Hovold wrote:
 On Mon, Oct 27, 2014 at 11:45:44AM -0500, Felipe Balbi wrote:
  On Mon, Oct 27, 2014 at 09:09:28AM +0100, Johan Hovold wrote:
   Add new property ti,system-power-controller to register the RTC as a
   power-off handler.
   
   Some RTC IP revisions can control an external PMIC via the pmic_power_en
   pin, which can be configured to transition to OFF on ALARM2 events and
   back to ON on subsequent ALARM (wakealarm) events.
   
   This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
   
   [1] https://www.mail-archive.com/linux-omap@vger.kernel.org/msg82127.html
   
   Tested-by: Felipe Balbi ba...@ti.com
   Signed-off-by: Johan Hovold jo...@kernel.org
   ---
   
   Changes since v2:
- add two-second delay to allow alarm to trigger before returning
   
   Andrew, can you replace just this patch in the series that you already
   have in your tree, or do you prefer I resend the whole series (with
   Felipe's Tested-by tags)?
   
   Thanks,
   Johan
   
   
Documentation/devicetree/bindings/rtc/rtc-omap.txt |  9 +-
drivers/rtc/rtc-omap.c | 95 
   ++
2 files changed, 103 insertions(+), 1 deletion(-)
   
   diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt 
   b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
   index 5a0f02d34d95..750efd40c72e 100644
   --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt
   +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
   @@ -5,11 +5,17 @@ Required properties:
 - ti,da830-rtc  - for RTC IP used similar to that on DA8xx SoC family.
 - ti,am3352-rtc - for RTC IP used similar to that on AM335x SoC 
   family.
 This RTC IP has special WAKE-EN Register to enable
   - Wakeup generation for event Alarm.
   + Wakeup generation for event Alarm. It can also be
   + used to control an external PMIC via the
   + pmic_power_en pin.
- reg: Address range of rtc register set
- interrupts: rtc timer, alarm interrupts in order
- interrupt-parent: phandle for the interrupt controller

   +Optional properties:
   +- ti,system-power-controller: whether the rtc is controlling the system 
   power
  
  isn't there a discussion going on to drop the vendor prefix ? I wonder
  if we should just use the final binding to avoid supporting this for
  rtc-omap too. OTOH, all of that can be hidden under
  of_is_system_power_controller() itself.
 
 Exactly, Romain intends to support both versions (i.e. with and without
 the prefix) with his helper function as there are already other uses of
 this variant in the wild.
 
 As I believe I mentioned in my cover letter, I suggest simply updating
 to the generic property name (i.e. use the helper and drop the prefix)
 once that code has been merged. Either way, we should always be able
 to change the property name before it has been used in a mainline
 release (as long as we update the driver and any dts use in one commit
 in order not to break bisectability).

fine by me :-) cheers

-- 
balbi


signature.asc
Description: Digital signature


Re: [PATCH v3] rtc: omap: add support for pmic_power_en

2014-10-27 Thread Andrew Morton
On Mon, 27 Oct 2014 09:09:28 +0100 Johan Hovold jo...@kernel.org wrote:

 Add new property ti,system-power-controller to register the RTC as a
 power-off handler.
 
 Some RTC IP revisions can control an external PMIC via the pmic_power_en
 pin, which can be configured to transition to OFF on ALARM2 events and
 back to ON on subsequent ALARM (wakealarm) events.
 
 This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
 
 [1] https://www.mail-archive.com/linux-omap@vger.kernel.org/msg82127.html
 
 Tested-by: Felipe Balbi ba...@ti.com
 Signed-off-by: Johan Hovold jo...@kernel.org
 ---
 
 Changes since v2:
  - add two-second delay to allow alarm to trigger before returning

hmpf.  As this sentence is below the ^--- it doesn't get into the
changelog.

 ...

 +static void omap_rtc_power_off(void)
 +{
 + struct omap_rtc *rtc = omap_rtc_power_off_rtc;
 + struct rtc_time tm;
 + unsigned long now;
 + u32 val;
 +
 + /* enable pmic_power_en control */
 + val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
 + rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN);
 +
 + /* set alarm two seconds from now */
 + omap_rtc_read_time_raw(rtc, tm);
 + bcd2tm(tm);
 + rtc_tm_to_time(tm, now);
 + rtc_time_to_tm(now + 2, tm);
 +
 + if (tm2bcd(tm)  0) {
 + dev_err(rtc-rtc-dev, power off failed\n);
 + return;
 + }
 +
 + rtc_wait_not_busy(rtc);
 +
 + rtc_write(rtc, OMAP_RTC_ALARM2_SECONDS_REG, tm.tm_sec);
 + rtc_write(rtc, OMAP_RTC_ALARM2_MINUTES_REG, tm.tm_min);
 + rtc_write(rtc, OMAP_RTC_ALARM2_HOURS_REG, tm.tm_hour);
 + rtc_write(rtc, OMAP_RTC_ALARM2_DAYS_REG, tm.tm_mday);
 + rtc_write(rtc, OMAP_RTC_ALARM2_MONTHS_REG, tm.tm_mon);
 + rtc_write(rtc, OMAP_RTC_ALARM2_YEARS_REG, tm.tm_year);
 +
 + /*
 +  * enable ALARM2 interrupt
 +  *
 +  * NOTE: this fails on AM3352 if rtc_write (writeb) is used
 +  */
 + val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG);
 + rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
 + val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
 +
 + mdelay(2000);

And it is uncommented.

How on earth is a reader to know why this is here?

I can do this

--- a/drivers/rtc/rtc-omap.c~rtc-omap-add-support-for-pmic_power_en-v3-fix
+++ a/drivers/rtc/rtc-omap.c
@@ -417,6 +417,7 @@ static void omap_rtc_power_off(void)
rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG,
val | OMAP_RTC_INTERRUPTS_IT_ALARM2);
 
+   /* Allow alarm to trigger before returning */
mdelay(2000);
 }
 


But it doesn't explain *why* we want the alarm to trigger before
returning.

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