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@xxxxxxxxxxx>
> ---
> 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
>
