On Sun, 25 Jul 2021 08:11:49 +0200
Jernej Škrabec <[email protected]> wrote:

Hi Jernej,

> Dne petek, 23. julij 2021 ob 17:38:32 CEST je Andre Przywara napisal(a):
> > Newer versions of the Allwinner RTC, for instance as found in the H616
> > SoC, not only store the current day as a linear number, but also change
> > the way the alarm is handled: There are now two registers, that
> > explicitly store the wakeup time, in the same format as the current
> > time.
> > 
> > Add support for that variant by writing the requested wakeup time
> > directly into the registers, instead of programming the seconds left, as
> > the old SoCs required.
> > 
> > Signed-off-by: Andre Przywara <[email protected]>
> > ---
> >  drivers/rtc/rtc-sun6i.c | 60 +++++++++++++++++++++++++++++------------
> >  1 file changed, 43 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > index a02be8bca6f3..f0ee20bfdccb 100644
> > --- a/drivers/rtc/rtc-sun6i.c
> > +++ b/drivers/rtc/rtc-sun6i.c
> > @@ -48,7 +48,8 @@
> >  
> >  /* Alarm 0 (counter) */
> >  #define SUN6I_ALRM_COUNTER                 0x0020
> > -#define SUN6I_ALRM_CUR_VAL                 0x0024
> > +/* This holds the remaining alarm seconds on older SoCs (current value) */
> > +#define SUN6I_ALRM_COUNTER_HMS                     0x0024
> >  #define SUN6I_ALRM_EN                              0x0028
> >  #define SUN6I_ALRM_EN_CNT_EN                       BIT(0)
> >  #define SUN6I_ALRM_IRQ_EN                  0x002c
> > @@ -523,32 +524,57 @@ static int sun6i_rtc_setalarm(struct device *dev,   
> struct rtc_wkalrm *wkalrm)
> >     struct sun6i_rtc_dev *chip = dev_get_drvdata(dev);
> >     struct rtc_time *alrm_tm = &wkalrm->time;
> >     struct rtc_time tm_now;
> > -   time64_t time_now, time_set;
> > +   time64_t time_set;
> > +   u32 counter_val, counter_val_hms;
> >     int ret;
> >  
> > -   ret = sun6i_rtc_gettime(dev, &tm_now);
> > -   if (ret < 0) {
> > -           dev_err(dev, "Error in getting time\n");
> > -           return -EINVAL;
> > -   }
> > -
> >     time_set = rtc_tm_to_time64(alrm_tm);
> > -   time_now = rtc_tm_to_time64(&tm_now);
> > -   if (time_set <= time_now) {
> > -           dev_err(dev, "Date to set in the past\n");
> > -           return -EINVAL;
> > -   }
> >  
> > -   if ((time_set - time_now) > U32_MAX) {
> > -           dev_err(dev, "Date too far in the future\n");
> > -           return -EINVAL;
> > +   if (chip->flags & RTC_LINEAR_DAY) {
> > +           time64_t seconds;
> > +
> > +           /*
> > +            * The alarm registers hold the actual alarm time,   
> encoded
> > +            * in the same way (linear day + HMS) as the current   
> time.
> > +            */
> > +           counter_val_hms = SUN6I_TIME_SET_SEC_VALUE(alrm_tm-
> >tm_sec)  |
> > +                               
> SUN6I_TIME_SET_MIN_VALUE(alrm_tm->tm_min)  |
> > +                               
> SUN6I_TIME_SET_HOUR_VALUE(alrm_tm->tm_hour);
> > +           seconds = mktime64(alrm_tm->tm_year + 1900, alrm_tm-
> >tm_mon,
> > +                              alrm_tm->tm_mday, 0, 0, 0);  
> 
> While above line should work, it's confusing why you're adding 1900 to years. 
> In my opinion it would be better to use same trick you used in patch 4 - 
> rtc_tm_to_time64() with hours, minutes and seconds set to 0.

IIRC setalarm does not want the time struct to be changed, but this is
OK in settime. But anyway ...

> Or maybe you 
> don't even need to do that, since division by SECS_PER_DAY will round down 
> anyway. In such way you hide RTC internal representation detail which doesn't 
> need to be exposed here.

That is indeed a very clever idea! I changed both settime and setalarm
accordingly now, adding a comment about this.

> 
> Once fixed:
> Reviewed by: Jernej Skrabec <[email protected]>

Many thanks for the reviews!

Cheers,
Andre

> > +           counter_val = div_u64(seconds, SECS_PER_DAY);
> > +   } else {
> > +           /* The alarm register holds the number of seconds left.   
> */
> > +           time64_t time_now;
> > +
> > +           ret = sun6i_rtc_gettime(dev, &tm_now);
> > +           if (ret < 0) {
> > +                   dev_err(dev, "Error in getting time\n");
> > +                   return -EINVAL;
> > +           }
> > +
> > +           time_now = rtc_tm_to_time64(&tm_now);
> > +           if (time_set <= time_now) {
> > +                   dev_err(dev, "Date to set in the past\n");
> > +                   return -EINVAL;
> > +           }
> > +           if ((time_set - time_now) > U32_MAX) {
> > +                   dev_err(dev, "Date too far in the future\n");
> > +                   return -EINVAL;
> > +           }
> > +
> > +           counter_val = time_set - time_now;
> >     }
> >  
> >     sun6i_rtc_setaie(0, chip);
> >     writel(0, chip->base + SUN6I_ALRM_COUNTER);
> > +   if (chip->flags & RTC_LINEAR_DAY)
> > +           writel(0, chip->base + SUN6I_ALRM_COUNTER_HMS);
> >     usleep_range(100, 300);
> >  
> > -   writel(time_set - time_now, chip->base + SUN6I_ALRM_COUNTER);
> > +   writel(counter_val, chip->base + SUN6I_ALRM_COUNTER);
> > +   if (chip->flags & RTC_LINEAR_DAY)
> > +           writel(counter_val_hms, chip->base +   
> SUN6I_ALRM_COUNTER_HMS);
> >     chip->alarm = time_set;
> >  
> >     sun6i_rtc_setaie(wkalrm->enabled, chip);
> > -- 
> > 2.17.6
> > 
> >   
> 
> 
> 

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
To view this discussion on the web, visit 
https://groups.google.com/d/msgid/linux-sunxi/20210802013902.7164876f%40slackpad.fritz.box.

Reply via email to