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

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

[Paul Cercueil]

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,
 	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)
 		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 },
 	{ .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 },
 	{ "jz4780-rtc", ID_JZ4780 },
 	{}
 };
-- 
2.11.0

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

[Alexandre Belloni]

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.

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

>  		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 },
>  	{ .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 },
>  	{ "jz4780-rtc", ID_JZ4780 },
>  	{}
>  };
> -- 
> 2.11.0
> 

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Paul Cercueil]

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.

>>   		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 },
>>   	{ .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 },
>>   	{ "jz4780-rtc", ID_JZ4780 },
>>   	{}
>>   };
>>  --
>>  2.11.0
>> 
> 
> --
> Alexandre Belloni, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com

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

[Alexandre Belloni]

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 },

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

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

[Paul Cercueil]

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

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

[Alexandre Belloni]

On 14/07/2018 15:50:08+0200, Paul Cercueil wrote:
> > >  > 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...
> 

I think it makes the code simpler to follow . Regarding future
proofness, you will have to add code and probably use a switch case at
the time you need to handle an RTC differently. At that time, the >=
test trick will not work anymore anyway.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Rob Herring]

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.

It may look a bit wierd if 4780 is the fallback for 4770, but if the 
4780 is older, then that actually makes sense.

Rob

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

[Paul Cercueil]

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?

> 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

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

[Rob Herring]

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.

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

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

[Paul Cercueil]

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