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

From: Xianwei Zhao <xianwei.zhao@amlogic.com>
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Yiting Deng <yiting.deng@amlogic.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	linux-amlogic@lists.infradead.org, linux-rtc@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] rtc: support for the Amlogic on-chip RTC
Date: Tue, 3 Sep 2024 10:48:27 +0800	[thread overview]
Message-ID: <d6187076-5fab-4804-a008-cfb5d85f8cd1@amlogic.com> (raw)
In-Reply-To: <2024090220195462df6c95@mail.local>

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

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic