Re: [PATCH 7/7] thermal: ti-soc-thermal: add DT example for DRA752 chip

[Eduardo Valentin <eduardo.valentin@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3596 bytes --]

On 30-05-2013 04:56, keerthy wrote:
> Hi Eduardo,
> 
> On 05/29/2013 08:37 PM, Eduardo Valentin wrote:
> 
>> Update documentation by adding an example for DRA752 on DT description.
>>
>> Cc: linux-pm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: Grant Likely <grant.likely@linaro.org>
>> Cc: Rob Herring <rob.herring@calxeda.com>
>> Cc: Rob Landley <rob@landley.net>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> Cc: J Keerthy <j-keerthy@ti.com>
>> Cc: devicetree-discuss@lists.ozlabs.org
>> Cc: linux-doc@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
>> ---
>>  Documentation/devicetree/bindings/thermal/ti_soc_thermal.txt | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/ti_soc_thermal.txt b/Documentation/devicetree/bindings/thermal/ti_soc_thermal.txt
>> index 1629652..1953b33 100644
>> --- a/Documentation/devicetree/bindings/thermal/ti_soc_thermal.txt
>> +++ b/Documentation/devicetree/bindings/thermal/ti_soc_thermal.txt
>> @@ -59,3 +59,15 @@ bandgap {
>>  	compatible = "ti,omap5430-bandgap";
>>  	interrupts = <0 126 4>; /* talert */
>>  };
>> +
>> +DRA752:
>> +bandgap {
>> +	reg = <0x4a0021e0 0xc
>> +		0x4a00232c 0xc
>> +		0x4a002380 0x2c
>> +		0x4a0023C0 0x3c
>> +		0x4a002564 0x8
>> +		0x4a002574 0x50>;
>> +	compatible = "ti,dra752-bandgap";
> 
> 
> The general convention seems to be keeping it as dra7xx or dra7.
> Why add specific nodes like dra752? We can always reuse for
> forthcoming dra7 processors if the same address mapping and IP
> is retained right?
> 
>>


In fact, I must agree with you that using a node such as dra752 is
restrictive. And I must also admit my original plan was to use dra7xx
naming convention. Not only on DT nodes, but in driver source code
symbols too. Well, this is about re-usability, right?

However, I took a step back and thought about the bandgap IP history.
Take that in mind, just look to the source code. We never succeed to
reuse nodes fully and it is because bandgap IPs do change from chip
version to chip version. That is also understandable as every chip has
its own thermal peculiarities. For instance, we do not have a omap4xxx
node because each OMAP4 chip has a different bandgap IP, with different
features.

I really would like to use a dra7xxx node, but that would contradict the
IP history :-). If we are trying to avoid patching the driver in the
future, I would say it is more likely to have a patch for another dra7
chip with specifics different from the current one than having a patch
reusing the existing dra752 node. This statement is obviously based on
driver and IP history (I might be wrong of course and the IP could
actually have reached a development stabilization point).

 +	interrupts = <0 126 4>; /* talert */
> 
> 
> The TRM does not have any term called talert. Can the comment
> be /* thermal_alert */ so it is easy to figure out what it is.
> 


Indeed, TRM does not, in fact! But the TI thermal engineering community
have coined this term and I think it is quite common used (within that
community) and if you look to the driver code too, it is all over. I
believe there is no doubt that talert stands for thermal alert. The
driver code does have documentation about that.

>> +};
> 
> 
> Regards,
> Keerthy
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]