Messages in this thread | | | Date | Mon, 23 Jul 2018 16:20:40 +0200 | From | Paul Cercueil <> | Subject | Re: [PATCH] rtc: jz4740: Add support for the JZ4725B, JZ4760, JZ4770 |
| |
Hi,
Le lun. 23 juil. 2018 à 16:18, Rob Herring <robh@kernel.org> a écrit : > On Fri, Jul 20, 2018 at 1:04 PM Paul Cercueil <paul@crapouillou.net> > wrote: >> >> Hi, >> >> Le ven. 20 juil. 2018 à 17:39, Rob Herring <robh@kernel.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@bootlin.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@bootlin.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@crapouillou.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
| |