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

[Paul Cercueil]

Hi,

Le ven. 20 juil. 2018 à 17:39, Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >  > >  > >  ---
> >  > >  > >   
> > 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?

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

Thanks,
-Paul