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

[Xianwei Zhao]

Hi Alexandre,
    Thanks for your reply.

On 2024/9/3 04:19, Alexandre Belloni wrote:
> [ EXTERNAL EMAIL ]
>
> 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@xxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxx>
> > > > Signed-off-by: Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx>
> > > > ---
> > > >    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.

Will modify driver file name to rtc-amlogic-a4.c


> > > > +             /* 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.

Got it.

> > 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.

OK will place enable action in the set_time process.

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