Re: [PATCH 08/14] ARM64: dts: hisilicon: Add tsensor interrupt name

[Daniel Lezcano]

Hi Rob,

thanks for the review.

On 15/10/2018 18:28, Rob Herring wrote:
> On Tue, Sep 25, 2018 at 11:03:06AM +0200, Daniel Lezcano wrote:
>> Add the interrupt names for the sensors, so the code can rely on them
>> instead of dealing with index which are prone to error.
>>
>> The name comes from the Hisilicon documentation found on internet.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
>> ---
>>  .../bindings/thermal/hisilicon-thermal.txt         |  3 ++
>>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi          | 63 +++++++++++-----------
>>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi          |  1 +
>>  3 files changed, 36 insertions(+), 31 deletions(-)
> 
> Lots of whitespace errors reported by checkpatch.pl.

Yeah, I don't know why but I have something bad with my emacs
configuration when changing DT files. Will look at this.

>> diff --git a/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt b/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
>> index cef716a..3edfae3 100644
>> --- a/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
>> +++ b/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
>> @@ -7,6 +7,7 @@
>>    region.
>>  - interrupt: The interrupt number to the cpu. Defines the interrupt used
>>    by /SOCTHERM/tsensor.
>> +- interrupt-names: The interrupt names for the different sensors
> 
> Need to define what the names are.
> 
>>  - clock-names: Input clock name, should be 'thermal_clk'.
>>  - clocks: phandles for clock specified in "clock-names" property.
>>  - #thermal-sensor-cells: Should be 1. See ./thermal.txt for a description.
>> @@ -18,6 +19,7 @@ for Hi6220:
>>  		compatible = "hisilicon,tsensor";
>>  		reg = <0x0 0xf7030700 0x0 0x1000>;
>>  		interrupts = <0 7 0x4>;
>> +		interrupt-names = "tsensor_intr";
> 
> That name seems pretty pointless.
> 
>>  		clocks = <&sys_ctrl HI6220_TSENSOR_CLK>;
>>  		clock-names = "thermal_clk";
>>  		#thermal-sensor-cells = <1>;
>> @@ -28,5 +30,6 @@ for Hi3660:
>>  		compatible = "hisilicon,hi3660-tsensor";
>>  		reg = <0x0 0xfff30000 0x0 0x1000>;
>>  		interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
>> +		interrupt-names = "tsensor_a73";
> 
> Just 'a73' is sufficient.

This is the name defined in the board documentation to give a name to
the interrupt when requesting it. This one appears in /proc/interrupts.

I can replace the 'tsensor_intr' by 'tsensor', but if 'tsensor_a73' is
replaced by 'a73' that may looks odd in /proc/interrupts to see an
interrupts line with the 'a73' name.

However this is the preparation for the multiple sensors support so we
will have more interrupt names.

Is it possible to keep these names ?

 - tsensor
 - tsensor_a73
 - tsensor_a57
 - tsensor_gpu




-- 
 <http://www.linaro.org/> Linaro.org â Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog