[PATCH] rtc: s3c: Rewrite clock handling

[PATCH] rtc: s3c: Rewrite clock handling

[Marek Szyprowski]

s3c_rtc_enable/disable_clk() functions were designed to be called multiple
times without reference counting, because they were initially used in
alarm setting/clearing functions, which can be called both when alarm is
already set or not. Later however, calls to those functions have been added to
other places in the driver - like time and /proc reading callbacks, what
results in broken alarm if any of such events happens after the alarm has
been set. Fix this by simplifying s3c_rtc_enable/disable_clk() functions
to rely on proper reference counting in clock core and move alarm enable
counter to s3c_rtc_setaie() function.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/rtc/rtc-s3c.c | 67 ++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 39 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 04c68178c42d..e682977b4f6e 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -39,7 +39,7 @@ struct s3c_rtc {
 	void __iomem *base;
 	struct clk *rtc_clk;
 	struct clk *rtc_src_clk;
-	bool clk_disabled;
+	bool alarm_enabled;
 
 	const struct s3c_rtc_data *data;
 
@@ -47,7 +47,7 @@ struct s3c_rtc {
 	int irq_tick;
 
 	spinlock_t pie_lock;
-	spinlock_t alarm_clk_lock;
+	spinlock_t alarm_lock;
 
 	int ticnt_save;
 	int ticnt_en_save;
@@ -70,44 +70,28 @@ struct s3c_rtc_data {
 
 static int s3c_rtc_enable_clk(struct s3c_rtc *info)
 {
-	unsigned long irq_flags;
 	int ret = 0;
 
-	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
+	ret = clk_enable(info->rtc_clk);
+	if (ret)
+		goto out;
 
-	if (info->clk_disabled) {
-		ret = clk_enable(info->rtc_clk);
-		if (ret)
+	if (info->data->needs_src_clk) {
+		ret = clk_enable(info->rtc_src_clk);
+		if (ret) {
+			clk_disable(info->rtc_clk);
 			goto out;
-
-		if (info->data->needs_src_clk) {
-			ret = clk_enable(info->rtc_src_clk);
-			if (ret) {
-				clk_disable(info->rtc_clk);
-				goto out;
-			}
 		}
-		info->clk_disabled = false;
 	}
-
 out:
-	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
-
 	return ret;
 }
 
 static void s3c_rtc_disable_clk(struct s3c_rtc *info)
 {
-	unsigned long irq_flags;
-
-	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
-	if (!info->clk_disabled) {
-		if (info->data->needs_src_clk)
-			clk_disable(info->rtc_src_clk);
-		clk_disable(info->rtc_clk);
-		info->clk_disabled = true;
-	}
-	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
+	clk_disable(info->rtc_clk);
 }
 
 /* IRQ Handlers */
@@ -135,6 +119,7 @@ static irqreturn_t s3c_rtc_alarmirq(int irq, void *id)
 static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
 {
 	struct s3c_rtc *info = dev_get_drvdata(dev);
+	unsigned long flags;
 	unsigned int tmp;
 	int ret;
 
@@ -151,17 +136,19 @@ static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
 
 	writeb(tmp, info->base + S3C2410_RTCALM);
 
-	s3c_rtc_disable_clk(info);
+	spin_lock_irqsave(&info->alarm_lock, flags);
 
-	if (enabled) {
-		ret = s3c_rtc_enable_clk(info);
-		if (ret)
-			return ret;
-	} else {
+	if (info->alarm_enabled && !enabled)
 		s3c_rtc_disable_clk(info);
-	}
+	else if (!info->alarm_enabled && enabled)
+		ret = s3c_rtc_enable_clk(info);
 
-	return 0;
+	info->alarm_enabled = enabled;
+	spin_unlock_irqrestore(&info->alarm_lock, flags);
+
+	s3c_rtc_disable_clk(info);
+
+	return ret;
 }
 
 /* Set RTC frequency */
@@ -357,10 +344,10 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 
 	writeb(alrm_en, info->base + S3C2410_RTCALM);
 
-	s3c_rtc_disable_clk(info);
-
 	s3c_rtc_setaie(dev, alrm->enabled);
 
+	s3c_rtc_disable_clk(info);
+
 	return 0;
 }
 
@@ -491,7 +478,7 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 	spin_lock_init(&info->pie_lock);
-	spin_lock_init(&info->alarm_clk_lock);
+	spin_lock_init(&info->alarm_lock);
 
 	platform_set_drvdata(pdev, info);
 
@@ -591,6 +578,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 
 	s3c_rtc_setfreq(info, 1);
 
+	s3c_rtc_disable_clk(info);
+
 	return 0;
 
 err_nortc:
-- 
2.17.1

Re: [PATCH] rtc: s3c: Rewrite clock handling

[Krzysztof Kozlowski]

On Fri, Jan 18, 2019 at 02:27:54PM +0100, Marek Szyprowski wrote:
> s3c_rtc_enable/disable_clk() functions were designed to be called multiple
> times without reference counting, because they were initially used in

s/used/used only/
(if I understand correctly the logic)

> alarm setting/clearing functions, which can be called both when alarm is
> already set or not. Later however, calls to those functions have been added to
> other places in the driver - like time and /proc reading callbacks, what
> results in broken alarm if any of such events happens after the alarm has
> been set. Fix this by simplifying s3c_rtc_enable/disable_clk() functions
> to rely on proper reference counting in clock core and move alarm enable
> counter to s3c_rtc_setaie() function.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/rtc/rtc-s3c.c | 67 ++++++++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
> index 04c68178c42d..e682977b4f6e 100644
> --- a/drivers/rtc/rtc-s3c.c
> +++ b/drivers/rtc/rtc-s3c.c
> @@ -39,7 +39,7 @@ struct s3c_rtc {
>  	void __iomem *base;
>  	struct clk *rtc_clk;
>  	struct clk *rtc_src_clk;
> -	bool clk_disabled;
> +	bool alarm_enabled;
>  
>  	const struct s3c_rtc_data *data;
>  
> @@ -47,7 +47,7 @@ struct s3c_rtc {
>  	int irq_tick;
>  
>  	spinlock_t pie_lock;
> -	spinlock_t alarm_clk_lock;
> +	spinlock_t alarm_lock;

Maybe add short comment that it protects only "alarm_enabled" property?

>  
>  	int ticnt_save;
>  	int ticnt_en_save;
> @@ -70,44 +70,28 @@ struct s3c_rtc_data {
>  
>  static int s3c_rtc_enable_clk(struct s3c_rtc *info)
>  {
> -	unsigned long irq_flags;
>  	int ret = 0;
>  
> -	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
> +	ret = clk_enable(info->rtc_clk);
> +	if (ret)
> +		goto out;

The out label is now empty so just "return ret". It is easier to read -
no need to jump anywhere to see the simple return.

>  
> -	if (info->clk_disabled) {
> -		ret = clk_enable(info->rtc_clk);
> -		if (ret)
> +	if (info->data->needs_src_clk) {
> +		ret = clk_enable(info->rtc_src_clk);
> +		if (ret) {
> +			clk_disable(info->rtc_clk);
>  			goto out;
> -
> -		if (info->data->needs_src_clk) {
> -			ret = clk_enable(info->rtc_src_clk);
> -			if (ret) {
> -				clk_disable(info->rtc_clk);
> -				goto out;
> -			}
>  		}
> -		info->clk_disabled = false;
>  	}
> -
>  out:
> -	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
> -
>  	return ret;
>  }
>  
>  static void s3c_rtc_disable_clk(struct s3c_rtc *info)
>  {
> -	unsigned long irq_flags;
> -
> -	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
> -	if (!info->clk_disabled) {
> -		if (info->data->needs_src_clk)
> -			clk_disable(info->rtc_src_clk);
> -		clk_disable(info->rtc_clk);
> -		info->clk_disabled = true;
> -	}
> -	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
> +	if (info->data->needs_src_clk)
> +		clk_disable(info->rtc_src_clk);
> +	clk_disable(info->rtc_clk);
>  }
>  
>  /* IRQ Handlers */
> @@ -135,6 +119,7 @@ static irqreturn_t s3c_rtc_alarmirq(int irq, void *id)
>  static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
>  {
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
> +	unsigned long flags;
>  	unsigned int tmp;
>  	int ret;
>  
> @@ -151,17 +136,19 @@ static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
>  
>  	writeb(tmp, info->base + S3C2410_RTCALM);
>  
> -	s3c_rtc_disable_clk(info);
> +	spin_lock_irqsave(&info->alarm_lock, flags);
>  
> -	if (enabled) {
> -		ret = s3c_rtc_enable_clk(info);
> -		if (ret)
> -			return ret;
> -	} else {
> +	if (info->alarm_enabled && !enabled)
>  		s3c_rtc_disable_clk(info);
> -	}
> +	else if (!info->alarm_enabled && enabled)
> +		ret = s3c_rtc_enable_clk(info);
>  
> -	return 0;
> +	info->alarm_enabled = enabled;
> +	spin_unlock_irqrestore(&info->alarm_lock, flags);
> +
> +	s3c_rtc_disable_clk(info);
> +
> +	return ret;
>  }
>  
>  /* Set RTC frequency */
> @@ -357,10 +344,10 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
>  
>  	writeb(alrm_en, info->base + S3C2410_RTCALM);
>  
> -	s3c_rtc_disable_clk(info);
> -
>  	s3c_rtc_setaie(dev, alrm->enabled);
>  
> +	s3c_rtc_disable_clk(info);

I do not understand this change - why do you have to move the disable
clk? The s3c_rtc_setaie() takes care about clock enabling/disabling for
the time of accessing registers.

> +
>  	return 0;
>  }
>  
> @@ -491,7 +478,7 @@ static int s3c_rtc_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  	spin_lock_init(&info->pie_lock);
> -	spin_lock_init(&info->alarm_clk_lock);
> +	spin_lock_init(&info->alarm_lock);
>  
>  	platform_set_drvdata(pdev, info);
>  
> @@ -591,6 +578,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)
>  
>  	s3c_rtc_setfreq(info, 1);
>  
> +	s3c_rtc_disable_clk(info);

I cannot find the reason why this is related to this particular change.
I mean, it looks reasonable because previously the clock looked like it
was enabled all the time... but maybe this should be separate pach?


Best regards,
Krzysztof

> +
>  	return 0;
>  
>  err_nortc:
> -- 
> 2.17.1
> 

Re: [PATCH] rtc: s3c: Rewrite clock handling

[Marek Szyprowski]

Hi Krzysztof,

On 2019-01-19 21:17, Krzysztof Kozlowski wrote:
> On Fri, Jan 18, 2019 at 02:27:54PM +0100, Marek Szyprowski wrote:
>> s3c_rtc_enable/disable_clk() functions were designed to be called multiple
>> times without reference counting, because they were initially used in
> s/used/used only/
> (if I understand correctly the logic)
>
>> alarm setting/clearing functions, which can be called both when alarm is
>> already set or not. Later however, calls to those functions have been added to
>> other places in the driver - like time and /proc reading callbacks, what
>> results in broken alarm if any of such events happens after the alarm has
>> been set. Fix this by simplifying s3c_rtc_enable/disable_clk() functions
>> to rely on proper reference counting in clock core and move alarm enable
>> counter to s3c_rtc_setaie() function.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>  drivers/rtc/rtc-s3c.c | 67 ++++++++++++++++++-------------------------
>>  1 file changed, 28 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
>> index 04c68178c42d..e682977b4f6e 100644
>> --- a/drivers/rtc/rtc-s3c.c
>> +++ b/drivers/rtc/rtc-s3c.c
>> @@ -39,7 +39,7 @@ struct s3c_rtc {
>>  	void __iomem *base;
>>  	struct clk *rtc_clk;
>>  	struct clk *rtc_src_clk;
>> -	bool clk_disabled;
>> +	bool alarm_enabled;
>>  
>>  	const struct s3c_rtc_data *data;
>>  
>> @@ -47,7 +47,7 @@ struct s3c_rtc {
>>  	int irq_tick;
>>  
>>  	spinlock_t pie_lock;
>> -	spinlock_t alarm_clk_lock;
>> +	spinlock_t alarm_lock;
> Maybe add short comment that it protects only "alarm_enabled" property?
>
>>  
>>  	int ticnt_save;
>>  	int ticnt_en_save;
>> @@ -70,44 +70,28 @@ struct s3c_rtc_data {
>>  
>>  static int s3c_rtc_enable_clk(struct s3c_rtc *info)
>>  {
>> -	unsigned long irq_flags;
>>  	int ret = 0;
>>  
>> -	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
>> +	ret = clk_enable(info->rtc_clk);
>> +	if (ret)
>> +		goto out;
> The out label is now empty so just "return ret". It is easier to read -
> no need to jump anywhere to see the simple return.
>
>>  
>> -	if (info->clk_disabled) {
>> -		ret = clk_enable(info->rtc_clk);
>> -		if (ret)
>> +	if (info->data->needs_src_clk) {
>> +		ret = clk_enable(info->rtc_src_clk);
>> +		if (ret) {
>> +			clk_disable(info->rtc_clk);
>>  			goto out;
>> -
>> -		if (info->data->needs_src_clk) {
>> -			ret = clk_enable(info->rtc_src_clk);
>> -			if (ret) {
>> -				clk_disable(info->rtc_clk);
>> -				goto out;
>> -			}
>>  		}
>> -		info->clk_disabled = false;
>>  	}
>> -
>>  out:
>> -	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
>> -
>>  	return ret;
>>  }
>>  
>>  static void s3c_rtc_disable_clk(struct s3c_rtc *info)
>>  {
>> -	unsigned long irq_flags;
>> -
>> -	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
>> -	if (!info->clk_disabled) {
>> -		if (info->data->needs_src_clk)
>> -			clk_disable(info->rtc_src_clk);
>> -		clk_disable(info->rtc_clk);
>> -		info->clk_disabled = true;
>> -	}
>> -	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
>> +	if (info->data->needs_src_clk)
>> +		clk_disable(info->rtc_src_clk);
>> +	clk_disable(info->rtc_clk);
>>  }
>>  
>>  /* IRQ Handlers */
>> @@ -135,6 +119,7 @@ static irqreturn_t s3c_rtc_alarmirq(int irq, void *id)
>>  static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
>>  {
>>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>> +	unsigned long flags;
>>  	unsigned int tmp;
>>  	int ret;
>>  
>> @@ -151,17 +136,19 @@ static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
>>  
>>  	writeb(tmp, info->base + S3C2410_RTCALM);
>>  
>> -	s3c_rtc_disable_clk(info);
>> +	spin_lock_irqsave(&info->alarm_lock, flags);
>>  
>> -	if (enabled) {
>> -		ret = s3c_rtc_enable_clk(info);
>> -		if (ret)
>> -			return ret;
>> -	} else {
>> +	if (info->alarm_enabled && !enabled)
>>  		s3c_rtc_disable_clk(info);
>> -	}
>> +	else if (!info->alarm_enabled && enabled)
>> +		ret = s3c_rtc_enable_clk(info);
>>  
>> -	return 0;
>> +	info->alarm_enabled = enabled;
>> +	spin_unlock_irqrestore(&info->alarm_lock, flags);
>> +
>> +	s3c_rtc_disable_clk(info);
>> +
>> +	return ret;
>>  }
>>  
>>  /* Set RTC frequency */
>> @@ -357,10 +344,10 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
>>  
>>  	writeb(alrm_en, info->base + S3C2410_RTCALM);
>>  
>> -	s3c_rtc_disable_clk(info);
>> -
>>  	s3c_rtc_setaie(dev, alrm->enabled);
>>  
>> +	s3c_rtc_disable_clk(info);
> I do not understand this change - why do you have to move the disable
> clk? The s3c_rtc_setaie() takes care about clock enabling/disabling for
> the time of accessing registers.

This was micro optimization, s3c_rtc_setaie() increases clock enable
count, so by changing the call order we can avoid one disable/enable
sequence.

>> +
>>  	return 0;
>>  }
>>  
>> @@ -491,7 +478,7 @@ static int s3c_rtc_probe(struct platform_device *pdev)
>>  		return -EINVAL;
>>  	}
>>  	spin_lock_init(&info->pie_lock);
>> -	spin_lock_init(&info->alarm_clk_lock);
>> +	spin_lock_init(&info->alarm_lock);
>>  
>>  	platform_set_drvdata(pdev, info);
>>  
>> @@ -591,6 +578,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)
>>  
>>  	s3c_rtc_setfreq(info, 1);
>>  
>> +	s3c_rtc_disable_clk(info);
> I cannot find the reason why this is related to this particular change.
> I mean, it looks reasonable because previously the clock looked like it
> was enabled all the time... but maybe this should be separate pach?

It wasn't enabled all the time, because the call to s3c_rtc_setfreq()
disabled it (remember there was no clock enable reference counting!).
Now once we have enable/disable reference counting, we need to keep them
balanced.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] rtc: s3c: Rewrite clock handling

[Krzysztof Kozlowski]

On Mon, 21 Jan 2019 at 09:39, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Hi Krzysztof,
>
> On 2019-01-19 21:17, Krzysztof Kozlowski wrote:
> > On Fri, Jan 18, 2019 at 02:27:54PM +0100, Marek Szyprowski wrote:
> >> s3c_rtc_enable/disable_clk() functions were designed to be called multiple
> >> times without reference counting, because they were initially used in
> > s/used/used only/
> > (if I understand correctly the logic)
> >
> >> alarm setting/clearing functions, which can be called both when alarm is
> >> already set or not. Later however, calls to those functions have been added to
> >> other places in the driver - like time and /proc reading callbacks, what
> >> results in broken alarm if any of such events happens after the alarm has
> >> been set. Fix this by simplifying s3c_rtc_enable/disable_clk() functions
> >> to rely on proper reference counting in clock core and move alarm enable
> >> counter to s3c_rtc_setaie() function.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> ---
> >>  drivers/rtc/rtc-s3c.c | 67 ++++++++++++++++++-------------------------
> >>  1 file changed, 28 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
> >> index 04c68178c42d..e682977b4f6e 100644
> >> --- a/drivers/rtc/rtc-s3c.c
> >> +++ b/drivers/rtc/rtc-s3c.c
> >> @@ -39,7 +39,7 @@ struct s3c_rtc {
> >>      void __iomem *base;
> >>      struct clk *rtc_clk;
> >>      struct clk *rtc_src_clk;
> >> -    bool clk_disabled;
> >> +    bool alarm_enabled;
> >>
> >>      const struct s3c_rtc_data *data;
> >>
> >> @@ -47,7 +47,7 @@ struct s3c_rtc {
> >>      int irq_tick;
> >>
> >>      spinlock_t pie_lock;
> >> -    spinlock_t alarm_clk_lock;
> >> +    spinlock_t alarm_lock;
> > Maybe add short comment that it protects only "alarm_enabled" property?
> >
> >>
> >>      int ticnt_save;
> >>      int ticnt_en_save;
> >> @@ -70,44 +70,28 @@ struct s3c_rtc_data {
> >>
> >>  static int s3c_rtc_enable_clk(struct s3c_rtc *info)
> >>  {
> >> -    unsigned long irq_flags;
> >>      int ret = 0;
> >>
> >> -    spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
> >> +    ret = clk_enable(info->rtc_clk);
> >> +    if (ret)
> >> +            goto out;
> > The out label is now empty so just "return ret". It is easier to read -
> > no need to jump anywhere to see the simple return.
> >
> >>
> >> -    if (info->clk_disabled) {
> >> -            ret = clk_enable(info->rtc_clk);
> >> -            if (ret)
> >> +    if (info->data->needs_src_clk) {
> >> +            ret = clk_enable(info->rtc_src_clk);
> >> +            if (ret) {
> >> +                    clk_disable(info->rtc_clk);
> >>                      goto out;
> >> -
> >> -            if (info->data->needs_src_clk) {
> >> -                    ret = clk_enable(info->rtc_src_clk);
> >> -                    if (ret) {
> >> -                            clk_disable(info->rtc_clk);
> >> -                            goto out;
> >> -                    }
> >>              }
> >> -            info->clk_disabled = false;
> >>      }
> >> -
> >>  out:
> >> -    spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
> >> -
> >>      return ret;
> >>  }
> >>
> >>  static void s3c_rtc_disable_clk(struct s3c_rtc *info)
> >>  {
> >> -    unsigned long irq_flags;
> >> -
> >> -    spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
> >> -    if (!info->clk_disabled) {
> >> -            if (info->data->needs_src_clk)
> >> -                    clk_disable(info->rtc_src_clk);
> >> -            clk_disable(info->rtc_clk);
> >> -            info->clk_disabled = true;
> >> -    }
> >> -    spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
> >> +    if (info->data->needs_src_clk)
> >> +            clk_disable(info->rtc_src_clk);
> >> +    clk_disable(info->rtc_clk);
> >>  }
> >>
> >>  /* IRQ Handlers */
> >> @@ -135,6 +119,7 @@ static irqreturn_t s3c_rtc_alarmirq(int irq, void *id)
> >>  static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
> >>  {
> >>      struct s3c_rtc *info = dev_get_drvdata(dev);
> >> +    unsigned long flags;
> >>      unsigned int tmp;
> >>      int ret;
> >>
> >> @@ -151,17 +136,19 @@ static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
> >>
> >>      writeb(tmp, info->base + S3C2410_RTCALM);
> >>
> >> -    s3c_rtc_disable_clk(info);
> >> +    spin_lock_irqsave(&info->alarm_lock, flags);
> >>
> >> -    if (enabled) {
> >> -            ret = s3c_rtc_enable_clk(info);
> >> -            if (ret)
> >> -                    return ret;
> >> -    } else {
> >> +    if (info->alarm_enabled && !enabled)
> >>              s3c_rtc_disable_clk(info);
> >> -    }
> >> +    else if (!info->alarm_enabled && enabled)
> >> +            ret = s3c_rtc_enable_clk(info);
> >>
> >> -    return 0;
> >> +    info->alarm_enabled = enabled;
> >> +    spin_unlock_irqrestore(&info->alarm_lock, flags);
> >> +
> >> +    s3c_rtc_disable_clk(info);
> >> +
> >> +    return ret;
> >>  }
> >>
> >>  /* Set RTC frequency */
> >> @@ -357,10 +344,10 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
> >>
> >>      writeb(alrm_en, info->base + S3C2410_RTCALM);
> >>
> >> -    s3c_rtc_disable_clk(info);
> >> -
> >>      s3c_rtc_setaie(dev, alrm->enabled);
> >>
> >> +    s3c_rtc_disable_clk(info);
> > I do not understand this change - why do you have to move the disable
> > clk? The s3c_rtc_setaie() takes care about clock enabling/disabling for
> > the time of accessing registers.
>
> This was micro optimization, s3c_rtc_setaie() increases clock enable
> count, so by changing the call order we can avoid one disable/enable
> sequence.

OK.

> >> +
> >>      return 0;
> >>  }
> >>
> >> @@ -491,7 +478,7 @@ static int s3c_rtc_probe(struct platform_device *pdev)
> >>              return -EINVAL;
> >>      }
> >>      spin_lock_init(&info->pie_lock);
> >> -    spin_lock_init(&info->alarm_clk_lock);
> >> +    spin_lock_init(&info->alarm_lock);
> >>
> >>      platform_set_drvdata(pdev, info);
> >>
> >> @@ -591,6 +578,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)
> >>
> >>      s3c_rtc_setfreq(info, 1);
> >>
> >> +    s3c_rtc_disable_clk(info);
> > I cannot find the reason why this is related to this particular change.
> > I mean, it looks reasonable because previously the clock looked like it
> > was enabled all the time... but maybe this should be separate pach?
>
> It wasn't enabled all the time, because the call to s3c_rtc_setfreq()
> disabled it (remember there was no clock enable reference counting!).
> Now once we have enable/disable reference counting, we need to keep them
> balanced.

I get it, thanks!

Best regards,
Krzysztof