linux-kernel - Re: [PATCH] rtc: jz4740: Add support for the JZ4725B, JZ4760, JZ4770

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1531576208.2831.1@smtp.crapouillou.net>
Date:   Sat, 14 Jul 2018 15:50:08 +0200
From:   Paul Cercueil <paul@...pouillou.net>
To:     Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc:     Alessandro Zummo <a.zummo@...ertech.it>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>, linux-rtc@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rtc: jz4740: Add support for the JZ4725B, JZ4760, JZ4770



Le sam. 14 juil. 2018 à 15:32, Alexandre Belloni 
<alexandre.belloni@...tlin.com> a écrit :
> On 14/07/2018 15:25:33+0200, Paul Cercueil wrote:
>>  Hi Alexandre,
>> 
>>  Le sam. 14 juil. 2018 à 15:19, Alexandre Belloni
>>  <alexandre.belloni@...tlin.com> a écrit :
>>  > Hello,
>>  >
>>  > On 13/07/2018 17:14:24+0200, Paul Cercueil wrote:
>>  > >  The RTC in the JZ4725B works just like the one in the JZ4740.
>>  > >
>>  > >  The RTC in the JZ4760 and JZ4770 work just like the one in the
>>  > > JZ4780.
>>  > >
>>  > >  Signed-off-by: Paul Cercueil <paul@...pouillou.net>
>>  > >  ---
>>  > >   Documentation/devicetree/bindings/rtc/ingenic,jz4740-rtc.txt 
>> |  3
>>  > > +++
>>  > >   drivers/rtc/rtc-jz4740.c                                     
>> | 11
>>  > > ++++++++++-
>>  > >   2 files changed, 13 insertions(+), 1 deletion(-)
>>  > >
>>  > >  diff --git
>>  > > a/Documentation/devicetree/bindings/rtc/ingenic,jz4740-rtc.txt
>>  > > b/Documentation/devicetree/bindings/rtc/ingenic,jz4740-rtc.txt
>>  > >  index 41c7ae18fd7b..a9e821de84f2 100644
>>  > >  --- 
>> a/Documentation/devicetree/bindings/rtc/ingenic,jz4740-rtc.txt
>>  > >  +++ 
>> b/Documentation/devicetree/bindings/rtc/ingenic,jz4740-rtc.txt
>>  > >  @@ -4,6 +4,9 @@ Required properties:
>>  > >
>>  > >   - compatible: One of:
>>  > >     - "ingenic,jz4740-rtc" - for use with the JZ4740 SoC
>>  > >  +  - "ingenic,jz4725b-rtc" - for use with the JZ4725B SoC
>>  > >  +  - "ingenic,jz4760-rtc" - for use with the JZ4760 SoC
>>  > >  +  - "ingenic,jz4770-rtc" - for use with the JZ4770 SoC
>>  > >     - "ingenic,jz4780-rtc" - for use with the JZ4780 SoC
>>  > >   - reg: Address range of rtc register set
>>  > >   - interrupts: IRQ number for the alarm interrupt
>>  > >  diff --git a/drivers/rtc/rtc-jz4740.c 
>> b/drivers/rtc/rtc-jz4740.c
>>  > >  index d0a891777f44..1c867e3a0ea5 100644
>>  > >  --- a/drivers/rtc/rtc-jz4740.c
>>  > >  +++ b/drivers/rtc/rtc-jz4740.c
>>  > >  @@ -54,6 +54,9 @@
>>  > >
>>  > >   enum jz4740_rtc_type {
>>  > >   	ID_JZ4740,
>>  > >  +	ID_JZ4725B,
>>  > >  +	ID_JZ4760,
>>  > >  +	ID_JZ4770,
>>  >
>>  > I wouldn't introduce those ids unless there are handling 
>> differences at
>>  > some point.
>> 
>>  Well there are handling differences, see below.
>> 
>>  > >   	ID_JZ4780,
>>  > >   };
>>  > >
>>  > >  @@ -114,7 +117,7 @@ static inline int 
>> jz4740_rtc_reg_write(struct
>>  > > jz4740_rtc *rtc, size_t reg,
>>  > >   {
>>  > >   	int ret = 0;
>>  > >
>>  > >  -	if (rtc->type >= ID_JZ4780)
>>  > >  +	if (rtc->type >= ID_JZ4760)
>>  >
>>  > This would avoid that change (and the test would preferably be
>>  > (rtc->type == ID_JZ4780))
>> 
>>  That branch should be taken if the SoC is JZ4760, JZ4770 or JZ4780.
>>  It should not be taken if the SoC is JZ4740 or JZ4725B.
> 
> Sure but you can achieve that with only 2 ids...
> 
>> 
>>  > >   		ret = jz4780_rtc_enable_write(rtc);
>>  > >   	if (ret == 0)
>>  > >   		ret = jz4740_rtc_wait_write_ready(rtc);
>>  > >  @@ -300,6 +303,9 @@ static void jz4740_rtc_power_off(void)
>>  > >
>>  > >   static const struct of_device_id jz4740_rtc_of_match[] = {
>>  > >   	{ .compatible = "ingenic,jz4740-rtc", .data = (void 
>> *)ID_JZ4740 },
>>  > >  +	{ .compatible = "ingenic,jz4725b-rtc", .data = (void 
>> *)ID_JZ4725B
>>  > > },
>>  > >  +	{ .compatible = "ingenic,jz4760-rtc", .data = (void 
>> *)ID_JZ4760 },
>>  > >  +	{ .compatible = "ingenic,jz4770-rtc", .data = (void 
>> *)ID_JZ4770 },
> 
> By doing the correct mapping here e.g:
> 
> { .compatible = "ingenic,jz4725b-rtc", .data = (void *)ID_JZ4740 },

Not very pretty and future-proof if you ask me...
But you're the boss...

>>  > >   	{ .compatible = "ingenic,jz4780-rtc", .data = (void 
>> *)ID_JZ4780 },
>>  > >   	{},
>>  > >   };
>>  > >  @@ -428,6 +434,9 @@ static const struct dev_pm_ops 
>> jz4740_pm_ops = {
>>  > >
>>  > >   static const struct platform_device_id jz4740_rtc_ids[] = {
>>  > >   	{ "jz4740-rtc", ID_JZ4740 },
>>  > >  +	{ "jz4725b-rtc", ID_JZ4725B },
>>  > >  +	{ "jz4760-rtc", ID_JZ4760 },
>>  > >  +	{ "jz4770-rtc", ID_JZ4770 },
> 
> And here
> 
>>  > >   	{ "jz4780-rtc", ID_JZ4780 },
>>  > >   	{}
>>  > >   };
>>  > >  --
>>  > >  2.11.0
>>  > >
>>  >
>>  > --
>>  > Alexandre Belloni, Bootlin (formerly Free Electrons)
>>  > Embedded Linux and Kernel engineering
>>  > https://bootlin.com
>> 
> 
> --
> Alexandre Belloni, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com