Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201117212201.1288608-1-alexandre.belloni@bootlin.com>
In-Reply-To: <20201117212201.1288608-1-alexandre.belloni@bootlin.com>
From:   Chunyan Zhang <zhang.lyra@gmail.com>
Date:   Wed, 18 Nov 2020 14:31:14 +0800
Message-ID: <CAAfSe-t6heLksvauKVBci1_aCi0Y044ugx7fEU=D3iuJQAG32Q@mail.gmail.com>
Subject: Re: [PATCH] rtc: sc27xx: Always read normal alarm
To:     Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc:     Alessandro Zummo <a.zummo@towertech.it>,
        Orson Zhai <orsonzhai@gmail.com>,
        Baolin Wang <baolin.wang7@gmail.com>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        linux-rtc@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Alexandre,

Thanks for this cleanup.

On Wed, 18 Nov 2020 at 05:22, Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> The RTC core only reads the alarm from the hardware at boot time, to know
> whether an alarm was already set before booting. It keeps track of all the
> alarms after that so there is no need to ever read the auxiliary alarm.
>
> Commit 3822d1bb0df1 ("rtc: sc27xx: Always read normal alarm when
> registering RTC device") already effectively removed the capability to read
> the auxiliary alarm a .read_alarm is always called with rtc->registered set

one nit suggestion: add a comma before "a .read_alarm is ..."

> to false.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

Reviewed-by: Chunyan Zhang <zhang.lyra@gmail.com>

Cheers,
Chunyan

> ---
>  drivers/rtc/rtc-sc27xx.c | 38 ++------------------------------------
>  1 file changed, 2 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/rtc/rtc-sc27xx.c b/drivers/rtc/rtc-sc27xx.c
> index 6e65f68ea86d..a953bc0a5a5b 100644
> --- a/drivers/rtc/rtc-sc27xx.c
> +++ b/drivers/rtc/rtc-sc27xx.c
> @@ -299,33 +299,6 @@ static int sprd_rtc_set_secs(struct sprd_rtc *rtc, enum sprd_rtc_reg_types type,
>                             sts_mask);
>  }
>
> -static int sprd_rtc_read_aux_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> -{
> -       struct sprd_rtc *rtc = dev_get_drvdata(dev);
> -       time64_t secs;
> -       u32 val;
> -       int ret;
> -
> -       ret = sprd_rtc_get_secs(rtc, SPRD_RTC_AUX_ALARM, &secs);
> -       if (ret)
> -               return ret;
> -
> -       rtc_time64_to_tm(secs, &alrm->time);
> -
> -       ret = regmap_read(rtc->regmap, rtc->base + SPRD_RTC_INT_EN, &val);
> -       if (ret)
> -               return ret;
> -
> -       alrm->enabled = !!(val & SPRD_RTC_AUXALM_EN);
> -
> -       ret = regmap_read(rtc->regmap, rtc->base + SPRD_RTC_INT_RAW_STS, &val);
> -       if (ret)
> -               return ret;
> -
> -       alrm->pending = !!(val & SPRD_RTC_AUXALM_EN);
> -       return 0;
> -}
> -
>  static int sprd_rtc_set_aux_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  {
>         struct sprd_rtc *rtc = dev_get_drvdata(dev);
> @@ -415,16 +388,9 @@ static int sprd_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>         u32 val;
>
>         /*
> -        * Before RTC device is registered, it will check to see if there is an
> -        * alarm already set in RTC hardware, and we always read the normal
> -        * alarm at this time.
> -        *
> -        * Or if aie_timer is enabled, we should get the normal alarm time.
> -        * Otherwise we should get auxiliary alarm time.
> +        * The RTC core checks to see if there is an alarm already set in RTC
> +        * hardware, and we always read the normal alarm at this time.
>          */
> -       if (rtc->rtc && rtc->rtc->registered && rtc->rtc->aie_timer.enabled == 0)
> -               return sprd_rtc_read_aux_alarm(dev, alrm);
> -
>         ret = sprd_rtc_get_secs(rtc, SPRD_RTC_ALARM, &secs);
>         if (ret)
>                 return ret;
> --
> 2.28.0
>