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

[Paul Cercueil <paul@crapouillou.net>]

Hi,

Le lun. 23 juil. 2018 =E0 16:18, Rob Herring <robh@kernel.org> a =E9crit :
> On Fri, Jul 20, 2018 at 1:04 PM Paul Cercueil <paul@crapouillou.net>=20
> wrote:
>>=20
>>  Hi,
>>=20
>>  Le ven. 20 juil. 2018 =E0 17:39, Rob Herring <robh@kernel.org> a=20
>> =E9crit :
>>  > On Sat, Jul 14, 2018 at 03:50:08PM +0200, Paul Cercueil wrote:
>>  >>
>>  >>
>>  >>  Le sam. 14 juil. 2018 =E0 15:32, Alexandre Belloni
>>  >>  <alexandre.belloni@bootlin.com> a =E9crit :
>>  >>  > On 14/07/2018 15:25:33+0200, Paul Cercueil wrote:
>>  >>  > >  Hi Alexandre,
>>  >>  > >
>>  >>  > >  Le sam. 14 juil. 2018 =E0 15:19, Alexandre Belloni
>>  >>  > >  <alexandre.belloni@bootlin.com> a =E9crit :
>>  >>  > >  > 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=20
>> 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
>>  >>  > >  > >  ---
>>  >>  > >=20
>> a/Documentation/devicetree/bindings/rtc/ingenic,jz4740-rtc.txt
>>  >>  > >  > >  +++
>>  >>  > >=20
>> 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=20
>> SoC
>>  >>  > >  > >  +  - "ingenic,jz4725b-rtc" - for use with the JZ4725B=20
>> SoC
>>  >>  > >  > >  +  - "ingenic,jz4760-rtc" - for use with the JZ4760=20
>> SoC
>>  >>  > >  > >  +  - "ingenic,jz4770-rtc" - for use with the JZ4770=20
>> SoC
>>  >>  > >  > >     - "ingenic,jz4780-rtc" - for use with the JZ4780=20
>> 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 =3D 0;
>>  >>  > >  > >
>>  >>  > >  > >  - if (rtc->type >=3D ID_JZ4780)
>>  >>  > >  > >  + if (rtc->type >=3D ID_JZ4760)
>>  >>  > >  >
>>  >>  > >  > This would avoid that change (and the test would=20
>> preferably
>>  >> be
>>  >>  > >  > (rtc->type =3D=3D 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 =3D jz4780_rtc_enable_write(rtc);
>>  >>  > >  > >    if (ret =3D=3D 0)
>>  >>  > >  > >            ret =3D jz4740_rtc_wait_write_ready(rtc);
>>  >>  > >  > >  @@ -300,6 +303,9 @@ static void=20
>> jz4740_rtc_power_off(void)
>>  >>  > >  > >
>>  >>  > >  > >   static const struct of_device_id=20
>> jz4740_rtc_of_match[] =3D
>>  >> {
>>  >>  > >  > >    { .compatible =3D "ingenic,jz4740-rtc", .data =3D (void
>>  >>  > > *)ID_JZ4740 },
>>  >>  > >  > >  + { .compatible =3D "ingenic,jz4725b-rtc", .data =3D (voi=
d
>>  >>  > > *)ID_JZ4725B
>>  >>  > >  > > },
>>  >>  > >  > >  + { .compatible =3D "ingenic,jz4760-rtc", .data =3D (void
>>  >>  > > *)ID_JZ4760 },
>>  >>  > >  > >  + { .compatible =3D "ingenic,jz4770-rtc", .data =3D (void
>>  >>  > > *)ID_JZ4770 },
>>  >>  >
>>  >>  > By doing the correct mapping here e.g:
>>  >>  >
>>  >>  > { .compatible =3D "ingenic,jz4725b-rtc", .data =3D (void=20
>> *)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=20
>> strings is
>>  > what gives you future-proofing. And no driver change is better=20
>> than
>>  > needless changing.
>>=20
>>  If I make e.g. the jz4760 and jz4770 use the jz4780 compatible=20
>> string,
>>  but
>>  then I want to implement a new feature that only exists on the=20
>> jz4780,
>>  how
>>  can I do it without breaking everything?
>=20
> You specify both:
>=20
> compatible =3D "ingenic,jz4760-rtc", "ingenic,jz4780-rtc";
>=20
> 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=20
>> the
>>  > 4780 is older, then that actually makes sense.
>>=20
>>  The 4770 is older than the 4780.
>=20
> 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.
>=20
> Rob
=