| Message ID | 1392123837-5517-6-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
> The S2MPS11 RTC has two alarms: alarm0 and alarm1 (corresponding > interrupts are named similarly). Use consistent names for interrupts to > limit possible errors. > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > drivers/mfd/sec-irq.c | 8 ++++---- > include/linux/mfd/samsung/irq.h | 4 ++-- > 2 files changed, 6 insertions(+), 6 deletions(-) <snip> > #define S2MPS11_IRQ_RTC60S_MASK (1 << 0) > #define S2MPS11_IRQ_RTCA1_MASK (1 << 1) > -#define S2MPS11_IRQ_RTCA2_MASK (1 << 2) > +#define S2MPS11_IRQ_RTCA0_MASK (1 << 2) This doesn't look correct to me. > #define S2MPS11_IRQ_SMPL_MASK (1 << 3) > #define S2MPS11_IRQ_RTC1S_MASK (1 << 4) > #define S2MPS11_IRQ_WTSR_MASK (1 << 5)
On Wed, 2014-02-12 at 09:07 +0000, Lee Jones wrote: > > The S2MPS11 RTC has two alarms: alarm0 and alarm1 (corresponding > > interrupts are named similarly). Use consistent names for interrupts to > > limit possible errors. > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > --- > > drivers/mfd/sec-irq.c | 8 ++++---- > > include/linux/mfd/samsung/irq.h | 4 ++-- > > 2 files changed, 6 insertions(+), 6 deletions(-) > > <snip> > > > #define S2MPS11_IRQ_RTC60S_MASK (1 << 0) > > #define S2MPS11_IRQ_RTCA1_MASK (1 << 1) > > -#define S2MPS11_IRQ_RTCA2_MASK (1 << 2) > > +#define S2MPS11_IRQ_RTCA0_MASK (1 << 2) > > This doesn't look correct to me. It is just renaming RTCA2 to RTCA0 because there is no "alarm 2" registers. Actually the behavior of driver does not change (especially that there is no RTC driver for S2MPS11) but now it looks properly: - set ALARM0 registers for RTCA0 interrupt, - set ALARM1 registers for RTCA1 interrupt, This patch is not essential. Best regards, Krzysztof > > > #define S2MPS11_IRQ_SMPL_MASK (1 << 3) > > #define S2MPS11_IRQ_RTC1S_MASK (1 << 4) > > #define S2MPS11_IRQ_WTSR_MASK (1 << 5) > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 12 Feb 2014, Krzysztof Kozlowski wrote: > On Wed, 2014-02-12 at 09:07 +0000, Lee Jones wrote: > > > The S2MPS11 RTC has two alarms: alarm0 and alarm1 (corresponding > > > interrupts are named similarly). Use consistent names for interrupts to > > > limit possible errors. > > > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > > --- > > > drivers/mfd/sec-irq.c | 8 ++++---- > > > include/linux/mfd/samsung/irq.h | 4 ++-- > > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > > <snip> > > > > > #define S2MPS11_IRQ_RTC60S_MASK (1 << 0) > > > #define S2MPS11_IRQ_RTCA1_MASK (1 << 1) > > > -#define S2MPS11_IRQ_RTCA2_MASK (1 << 2) > > > +#define S2MPS11_IRQ_RTCA0_MASK (1 << 2) > > > > This doesn't look correct to me. > > It is just renaming RTCA2 to RTCA0 because there is no "alarm 2" > registers. Actually the behavior of driver does not change (especially > that there is no RTC driver for S2MPS11) but now it looks properly: > - set ALARM0 registers for RTCA0 interrupt, > - set ALARM1 registers for RTCA1 interrupt, > > This patch is not essential. I mean the logic. If these masks are used for registers then I assume RTCA0 would be BIT(1) amd RTCA1 would be BIT(2), but this patch swaps them round. > > > #define S2MPS11_IRQ_SMPL_MASK (1 << 3) > > > #define S2MPS11_IRQ_RTC1S_MASK (1 << 4) > > > #define S2MPS11_IRQ_WTSR_MASK (1 << 5) > > >
On Wed, 2014-02-12 at 10:02 +0000, Lee Jones wrote: > On Wed, 12 Feb 2014, Krzysztof Kozlowski wrote: > > > On Wed, 2014-02-12 at 09:07 +0000, Lee Jones wrote: > > > > The S2MPS11 RTC has two alarms: alarm0 and alarm1 (corresponding > > > > interrupts are named similarly). Use consistent names for interrupts to > > > > limit possible errors. > > > > > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > > > --- > > > > drivers/mfd/sec-irq.c | 8 ++++---- > > > > include/linux/mfd/samsung/irq.h | 4 ++-- > > > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > > > > <snip> > > > > > > > #define S2MPS11_IRQ_RTC60S_MASK (1 << 0) > > > > #define S2MPS11_IRQ_RTCA1_MASK (1 << 1) > > > > -#define S2MPS11_IRQ_RTCA2_MASK (1 << 2) > > > > +#define S2MPS11_IRQ_RTCA0_MASK (1 << 2) > > > > > > This doesn't look correct to me. > > > > It is just renaming RTCA2 to RTCA0 because there is no "alarm 2" > > registers. Actually the behavior of driver does not change (especially > > that there is no RTC driver for S2MPS11) but now it looks properly: > > - set ALARM0 registers for RTCA0 interrupt, > > - set ALARM1 registers for RTCA1 interrupt, > > > > This patch is not essential. > > I mean the logic. > > If these masks are used for registers then I assume RTCA0 would be > BIT(1) amd RTCA1 would be BIT(2), but this patch swaps them round. Yes, one could assume that and in case of S5M8767 this is right (the order is proper)... but on S2MPS11/S2MPS14 this is reverted: - BIT(0): RTC periodic 60s - BIT(1): RTC Alarm 1 - BIT(2): RTC Alarm 0 The original code (BIT(1) for RTCA1 and BIT(2) for RTCA2) was wrong here and may lead to errors. I think that this was changed during mainstream process to match S5M8767. However some old internal driver sources for S2MPS11 have: #define S2MPS11_IRQ_RTCA2_MASK (1 << 1) #define S2MPS11_IRQ_RTCA1_MASK (1 << 2) Best regards, Krzysztof > > > > > #define S2MPS11_IRQ_SMPL_MASK (1 << 3) > > > > #define S2MPS11_IRQ_RTC1S_MASK (1 << 4) > > > > #define S2MPS11_IRQ_WTSR_MASK (1 << 5) > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > > The S2MPS11 RTC has two alarms: alarm0 and alarm1 (corresponding > > > > > interrupts are named similarly). Use consistent names for interrupts to > > > > > limit possible errors. > > > > > > > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > > > > --- > > > > > drivers/mfd/sec-irq.c | 8 ++++---- > > > > > include/linux/mfd/samsung/irq.h | 4 ++-- > > > > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > > > > > > <snip> > > > > > > > > > #define S2MPS11_IRQ_RTC60S_MASK (1 << 0) > > > > > #define S2MPS11_IRQ_RTCA1_MASK (1 << 1) > > > > > -#define S2MPS11_IRQ_RTCA2_MASK (1 << 2) > > > > > +#define S2MPS11_IRQ_RTCA0_MASK (1 << 2) > > > > > > > > This doesn't look correct to me. > > > > > > It is just renaming RTCA2 to RTCA0 because there is no "alarm 2" > > > registers. Actually the behavior of driver does not change (especially > > > that there is no RTC driver for S2MPS11) but now it looks properly: > > > - set ALARM0 registers for RTCA0 interrupt, > > > - set ALARM1 registers for RTCA1 interrupt, > > > > > > This patch is not essential. > > > > I mean the logic. > > > > If these masks are used for registers then I assume RTCA0 would be > > BIT(1) amd RTCA1 would be BIT(2), but this patch swaps them round. > > > Yes, one could assume that and in case of S5M8767 this is right (the > order is proper)... but on S2MPS11/S2MPS14 this is reverted: > - BIT(0): RTC periodic 60s > - BIT(1): RTC Alarm 1 > - BIT(2): RTC Alarm 0 > > The original code (BIT(1) for RTCA1 and BIT(2) for RTCA2) was wrong here > and may lead to errors. I think that this was changed during mainstream > process to match S5M8767. However some old internal driver sources for > S2MPS11 have: > #define S2MPS11_IRQ_RTCA2_MASK (1 << 1) > #define S2MPS11_IRQ_RTCA1_MASK (1 << 2) Okay, if you're happy that this is correct: Acked-by: Lee Jones <lee.jones@linaro.org> > > > > > #define S2MPS11_IRQ_SMPL_MASK (1 << 3) > > > > > #define S2MPS11_IRQ_RTC1S_MASK (1 << 4) > > > > > #define S2MPS11_IRQ_WTSR_MASK (1 << 5) > > > > > > > > > >
diff --git a/drivers/mfd/sec-irq.c b/drivers/mfd/sec-irq.c index 4de494f51d40..e403c293b437 100644 --- a/drivers/mfd/sec-irq.c +++ b/drivers/mfd/sec-irq.c @@ -59,13 +59,13 @@ static const struct regmap_irq s2mps11_irqs[] = { .reg_offset = 1, .mask = S2MPS11_IRQ_RTC60S_MASK, }, - [S2MPS11_IRQ_RTCA1] = { + [S2MPS11_IRQ_RTCA0] = { .reg_offset = 1, - .mask = S2MPS11_IRQ_RTCA1_MASK, + .mask = S2MPS11_IRQ_RTCA0_MASK, }, - [S2MPS11_IRQ_RTCA2] = { + [S2MPS11_IRQ_RTCA1] = { .reg_offset = 1, - .mask = S2MPS11_IRQ_RTCA2_MASK, + .mask = S2MPS11_IRQ_RTCA1_MASK, }, [S2MPS11_IRQ_SMPL] = { .reg_offset = 1, diff --git a/include/linux/mfd/samsung/irq.h b/include/linux/mfd/samsung/irq.h index d43b4f9e7fb2..abe1a6aae3b7 100644 --- a/include/linux/mfd/samsung/irq.h +++ b/include/linux/mfd/samsung/irq.h @@ -24,8 +24,8 @@ enum s2mps11_irq { S2MPS11_IRQ_MRB, S2MPS11_IRQ_RTC60S, + S2MPS11_IRQ_RTCA0, S2MPS11_IRQ_RTCA1, - S2MPS11_IRQ_RTCA2, S2MPS11_IRQ_SMPL, S2MPS11_IRQ_RTC1S, S2MPS11_IRQ_WTSR, @@ -47,7 +47,7 @@ enum s2mps11_irq { #define S2MPS11_IRQ_RTC60S_MASK (1 << 0) #define S2MPS11_IRQ_RTCA1_MASK (1 << 1) -#define S2MPS11_IRQ_RTCA2_MASK (1 << 2) +#define S2MPS11_IRQ_RTCA0_MASK (1 << 2) #define S2MPS11_IRQ_SMPL_MASK (1 << 3) #define S2MPS11_IRQ_RTC1S_MASK (1 << 4) #define S2MPS11_IRQ_WTSR_MASK (1 << 5)
The S2MPS11 RTC has two alarms: alarm0 and alarm1 (corresponding interrupts are named similarly). Use consistent names for interrupts to limit possible errors. Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- drivers/mfd/sec-irq.c | 8 ++++---- include/linux/mfd/samsung/irq.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-)