Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752568AbdIAUsx (ORCPT ); Fri, 1 Sep 2017 16:48:53 -0400 Received: from mail-wm0-f51.google.com ([74.125.82.51]:33712 "EHLO mail-wm0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752393AbdIAUsv (ORCPT ); Fri, 1 Sep 2017 16:48:51 -0400 X-Google-Smtp-Source: ADKCNb5xFExw2j8k3JcHSZVv8dgNkfVhCBe1+K2soG7J7UMlTnE746mwbQtiO3Cs6ytfWIXDNTb2IQ== Subject: Re: [PATCH 02/13] thermal/drivers/hisi: Remove the multiple sensors support To: Leo Yan Cc: rui.zhang@intel.com, edubezval@gmail.com, linux-pm@vger.kernel.org, kevin.wangtao@linaro.org, open list References: <1504082857-21702-1-git-send-email-daniel.lezcano@linaro.org> <1504082857-21702-2-git-send-email-daniel.lezcano@linaro.org> <20170901140523.GB26177@leoy-linaro> From: Daniel Lezcano Message-ID: <2f333866-9001-9a67-4ae1-7dc92fc28b26@linaro.org> Date: Fri, 1 Sep 2017 22:48:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170901140523.GB26177@leoy-linaro> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3279 Lines: 89 On 01/09/2017 16:05, Leo Yan wrote: > Hi Daniel, > > On Wed, Aug 30, 2017 at 10:47:26AM +0200, Daniel Lezcano wrote: >> By essence, the tsensor does not really support multiple sensor at the same >> time. It allows to set a sensor and use it to get the temperature, another >> sensor could be switched but with a delay of 3-5ms. It is difficult to read >> simultaneously several sensors without a big delay. >> >> Today, just one sensor is used, it is not necessary to deal with multiple >> sensors in the code. Remove them and if it is needed in the future add them >> on top of a code which will be clean up in the meantime. >> >> Signed-off-by: Daniel Lezcano >> --- >> drivers/thermal/hisi_thermal.c | 77 +++++++++++------------------------------- >> 1 file changed, 20 insertions(+), 57 deletions(-) >> >> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c >> index 8381696..92b6889 100644 >> --- a/drivers/thermal/hisi_thermal.c >> +++ b/drivers/thermal/hisi_thermal.c >> @@ -39,6 +39,7 @@ >> #define HISI_TEMP_RESET (100000) >> >> #define HISI_MAX_SENSORS 4 >> +#define HISI_DEFAULT_SENSOR 2 > > So if we always set the sensor id 2 as default sensor, that means it's > pointless to bind sensor id in dts. E.g. I change to bind thermal > sensor 0, then it's impossible to bind successfully: > > - thermal-sensors = <&tsensor 2>; > + thermal-sensors = <&tsensor 0>; > > It's okay for this change, do you think we need reflect this in DT > binding doc [1] ? > > [1] Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt Actually, the current behavior is to go through all the sensors and register them. By side effect, we end up by registering a sensor with an id matching the one specified in the DT, others are failing. That is because there is something missing in the of-thermal API where we can't call thermal_zone_of_sensor_register() without a matching index. That is probably why we have in the drivers/thermal/qoriq_thermal.c file the qoriq_tmu_get_sensor_id() which inspect the thermal-zone DT structure for the thermal-sensors and read the sensor's id from there in order to reuse it for thermal_zone_of_sensor_register(). Needless to say that is not the right place to do that. IMHO, as nothing gets changed for the moment on hi6220, we can live with HISI_DEFAULT_SENSOR until we sort out what to do with thermal_zone_of_sensor_register(). >> struct hisi_thermal_sensor { >> struct hisi_thermal_data *thermal; >> @@ -53,11 +54,10 @@ struct hisi_thermal_data { >> struct mutex thermal_lock; /* protects register data */ >> struct platform_device *pdev; >> struct clk *clk; >> - struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS]; >> - >> - int irq, irq_bind_sensor; >> + struct hisi_thermal_sensor sensors; >> + int irq; >> bool irq_enabled; >> - >> + > > Repots whitespace error when I apply patch. Ok, I will fix that. [ ... ] Thanks for the review. -- Daniel -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog