Re: [PATCH 2/3] rtc: support for the Amlogic on-chip RTC

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 02/09/2024 16:14:45+0800, Xianwei Zhao wrote:
> Hi Alexandre,
>     Thanks for your reply.
> 
> On 2024/8/26 17:45, Alexandre Belloni wrote:
> > [ EXTERNAL EMAIL ]
> > 
> > On 23/08/2024 17:19:45+0800, Xianwei Zhao via B4 Relay wrote:
> > > From: Yiting Deng <yiting.deng@amlogic.com>
> > > 
> > > Support for the on-chip RTC found in some of Amlogic's SoCs such as the
> > > A113L2 and A113X2.
> > > 
> > > Signed-off-by: Yiting Deng <yiting.deng@amlogic.com>
> > > Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
> > > ---
> > >   drivers/rtc/Kconfig       |  12 +
> > >   drivers/rtc/Makefile      |   1 +
> > >   drivers/rtc/rtc-amlogic.c | 589 ++++++++++++++++++++++++++++++++++++++++++++++
> > 
> > As pointed out, this is the third amlogic driver so the name of the file
> > must be more specific.
> > 
> 
> This RTC hardware includes a timing function and an alarm function.
> But the existing has only timing function, alarm function is using the
> system clock to implement a virtual alarm. And the relevant register access
> method is also different.
> 
> The "meson" string is meaningless, it just keeps going, and now the new
> hardware uses the normal naming.

The proper naming is then definitively not just amlogic, because in 5
year, you are going to say the exact same thing about this driver
"register access is different, this is for old SoCs, etc"

amlogc-a4 would be more appropriate.

> > > +             /* Enable RTC */
> > > +             regmap_write_bits(rtc->map, RTC_CTRL, RTC_ENABLE, RTC_ENABLE);
> > 
> >                  This must not be done at probe time, else you loose the
> >                  important information taht the time has never been set. Instead,
> >                  it should only be enabled on the first .set_time invocation do
> >                  you could now in .read_time that the time is currently invalid.
> > 
> There are some doubts about this place.
> 
> You mean that after the system is up, unless the time is set, it will fail
> to read the time at any time, and the alarm clock will also fail.
> In this case, the system must set a time.

Exactly, reading the time must not succeed if the time is known to be
bad.

> 
> When read time invlalid, system is will set time.
> This part of the logic I see the kernel part has not been implemented, so
> only the user application has been implemented. Whether this is reasonable,
> if not set time, you will never use RTC module.

This is not going to be implemented in the kernel. The kernel can't know
what is the proper time to set unless userspace tells it.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com