Re: [RFC 4/5] RTC: rtc-at91sam9: add device-tree support

[Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>]

On 04/08/2013 12:03 PM, Jean-Christophe PLAGNIOL-VILLARD :
> On 11:57 Mon 08 Apr     , Nicolas Ferre wrote:
>> On 04/08/2013 11:00 AM, Johan Hovold :
>>> On Mon, Apr 08, 2013 at 09:38:07AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>> On 17:12 Sun 07 Apr     , Johan Hovold wrote:
>>>>> Add device-tree support.
>>>>>
>>>>> The AT91 RTT can be used as an RTC if the atmel,at91-rtt-as-rtc-gpbr
>>>>> property is present and set to the general-purpose backup register to
>>>>> use to store the RTC time base.
>>>>>
>>>>> Signed-off-by: Johan Hovold <jhovold-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>> ---
>>>>>  .../devicetree/bindings/rtc/rtc-at91sam9.txt       | 19 ++++++++++++
>>>>>  drivers/rtc/rtc-at91sam9.c                         | 36 +++++++++++++++++++++-
>>>>>  2 files changed, 54 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt
>>>>> new file mode 100644
>>>>> index 0000000..0f54988
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt
>>>>> @@ -0,0 +1,19 @@
>>>>> +Atmel AT91 RTT as RTC
>>>>> +=====================
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible: Should be "atmel,at91sam9260-rtt"
>>>>> +- reg: Should contain register location and length
>>>>> +- interrupts: Should contain interrupt for the RTT which is the IRQ line
>>>>> +  shared across all System Controller members.
>>>>> +- atmel,rtt-as-rtc-gpbr: Should contain the backup-register to use to store
>>>>> +  the RTC time base
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +rtt@fffffd20 {
>>>>> +		compatible = "atmel,at91sam9g45-rtt", "atmel,at91sam9260-rtt";
>>
>> No, there is no visible difference between the sam9g45 RTT and the
>> sam9260 one. So the most precise compatibility string is still sam9260.
>> If one day we feel the need for a advanced feature that exists on a more
>> recent SoC, we have the possibility to add it at that time...
>>
>>>>> +		reg = <0xfffffd20 0x10>;
>>>>> +		interrupts = <1 4 7>;
>>>>> +		atmel,at91-rtt-as-rtc-gpbr = <0>;
>>>> no you miss the point of the DT
>>>>
>>>> you need to describe the hardware no a particular use of it
>>>
>>> That was what I was trying to achieve by adding the two use-neutral
>>> rtt and gpbr-nodes. But then the question is how would you influence
>>> which out of two rtt-drivers to use?
>>>
>>> Adding a property as above in the final board descriptions seemed
>>> preferable to adding rtt-as-rtc to the compatible string of the rtt as
>>> that would mean describing use rather than just hardware.
>>
>> Well, re-reading the Device_Tree_Usage page, I found this sentence:
>> "
>> Understanding the compatible Property
>>
>> Every node in the tree that represents a device is required to have the
>> compatible property. compatible is the key an operating system uses to
>> decide which device driver to bind to a device.
>> "
>> or ePARP:
>>
>> "
>> The compatible property value consists of one or more strings that
>> define the specific programming model for the device.
>> "
>>
>> We have the notion of link between hardware and software in this
>> *compatible* sting, even if the *node* itself is about hardware description.
>>
>>>> the RTT is a general purpose timer backuped that we use in linux as a
>>>> RTC with a gpbr to store the time
>>>>
>>>> you need 2 binding on for the RTT one the RTT-rtc
>>>
>>> As in adding some virtual hardware-node which uses the rtt and gpbr as
>>> resources?
>>
>> So, why not simply having a compatibility string that collects the uses
>> of this RTT node:
>>
>> compatible = "atmel,at91sam9260-rtt-as-rtc", "atmel,at91sam9260-rtt";
>>
>> And then "decide which device driver to bind to [the RTT] device"...
>> If the rtt-as-rtc driver is not selected, the device can still be used
>> as a simple "rtt". The board .dts can overload a compatibility string
>> according to the use, etc.
>>
>> Then the way do describe which GPBR to use has still to be discussed.
>> But for the RTT itself, I would keep it simple like that.
> 
> no as infact the rtc-at91sam9 should not even exist
> as this is much more generic
> 
> we use a backped register and a timer to emulate a RTC this can be unsed by
> any one
> 
> and I can use any backuped timer
> 
> we need to have frameworks

Is it possible to focus on the current problem instead of calling for
the creation for Yet Another Framework?

> where the gpbr are tracked and the rtt
> 
> 	for you describe the resources
> 
> 	rtt0: rtt@fffffd20 {
> 		compatible = "atmel,at91sam9260-rtt";
> 		reg = <0xfffffd20 0x10>;
> 		interrupts = <1 4 7>;
> 	};
> 
> 	rtc-timer {
> 		compatible = "linux,rtc-timer";
> 		timer = <&rtt0>;
> 		backuped-register = <&gpbr 0>;
> 	};
> 
> this need to SoC implemetation generic

OMG, so well, we will have to re-write the whole rtc-at91sam9 now??!! Oh
yes, I see, we do not have more urgent things to do...

Please stop trying re-inventing things that will do everything but
solving the real issues: why not agree on a sort term solution about
this rtt/rtc thing: I have *never* heard complains about this precise
driver. And we must concentrate now on problem solving.

Bye,
-- 
Nicolas Ferre