| 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
| ||
|
Message-Id: <1532355640.2289.0@smtp.crapouillou.net>
Date: Mon, 23 Jul 2018 16:20:40 +0200
From: Paul Cercueil <paul@...pouillou.net>
To: Rob Herring <robh@...nel.org>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>,
Alessandro Zummo <a.zummo@...ertech.it>,
Mark Rutland <mark.rutland@....com>,
"open list:REAL TIME CLOCK (RTC) SUBSYSTEM"
<linux-rtc@...r.kernel.org>, devicetree@...r.kernel.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] rtc: jz4740: Add support for the JZ4725B, JZ4760, JZ4770
Hi,
Le lun. 23 juil. 2018 à 16:18, Rob Herring <robh@...nel.org> a écrit :
> On Fri, Jul 20, 2018 at 1:04 PM Paul Cercueil <paul@...pouillou.net>
> wrote:
>>
>> Hi,
>>
>> Le ven. 20 juil. 2018 à 17:39, Rob Herring <robh@...nel.org> a
>> écrit :
>> > On Sat, Jul 14, 2018 at 03:50:08PM +0200, Paul Cercueil wrote:
>> >>
>> >>
>> >> 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...
>> >
>> > Looks to me like this can be handled entirely in DT without driver
>> > changes like the other patches. Correct usage of compatible
>> strings is
>> > what gives you future-proofing. And no driver change is better
>> than
>> > needless changing.
>>
>> If I make e.g. the jz4760 and jz4770 use the jz4780 compatible
>> string,
>> but
>> then I want to implement a new feature that only exists on the
>> jz4780,
>> how
>> can I do it without breaking everything?
>
> You specify both:
>
> compatible = "ingenic,jz4760-rtc", "ingenic,jz4780-rtc";
>
> You match on the less specific compatible until you have features or
> bugs to handle for the more specific compatible. In your case, you
> would have to start matching on the 4760 and 4770 and not support the
> feature for those. A bit backwards from normal, but would still work.
Right, I didn't think about that.
Thanks!
>> > It may look a bit wierd if 4780 is the fallback for 4770, but if
>> the
>> > 4780 is older, then that actually makes sense.
>>
>> The 4770 is older than the 4780.
>
> As I said, it's weird, but it will all still work. You could fix this
> if you want. It would only matter if you had a old dtb with a new
> kernel and care about that case working.
>
> Rob
Powered by blists - more mailing lists