Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2aafa6cc-a7de-0b7a-571f-04593ad53787@linaro.org>
Date:   Fri, 21 Oct 2022 21:59:56 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 1/2] thermal: rockchip: Support RK3588 SoC in the thermal
 driver
Content-Language: en-US
To:     Sebastian Reichel <sebastian.reichel@collabora.com>,
        Heiko Stuebner <heiko@sntech.de>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-pm@vger.kernel.org, linux-rockchip@lists.infradead.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Finley Xiao <finley.xiao@rock-chips.com>, kernel@collabora.com
References: <20221021174721.92468-1-sebastian.reichel@collabora.com>
 <20221021174721.92468-2-sebastian.reichel@collabora.com>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <20221021174721.92468-2-sebastian.reichel@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 21/10/2022 19:47, Sebastian Reichel wrote:
> From: Finley Xiao <finley.xiao@rock-chips.com>
> 
> The RK3588 SoC has seven channels TS-ADC(TOP, BIG_CORE0, BIG_CORE1,
> LITTEL_CORE, CENTER, GPU, and NPU).

Is possible to give a more elaborate description of those sensors?

What is TOP and CENTER ?

There are 4 Bigs on this platform but two sensors ?


> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> [rebase, squash fixes]
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>   drivers/thermal/rockchip_thermal.c | 182 ++++++++++++++++++++++++++++-
>   1 file changed, 179 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index 819e059cde71..82f475a6448f 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -61,10 +61,9 @@ enum adc_sort_mode {
>   #include "thermal_hwmon.h"
>   
>   /**
> - * The max sensors is two in rockchip SoCs.
> - * Two sensors: CPU and GPU sensor.
> + * The max sensors is seven in rockchip SoCs.
>    */
> -#define SOC_MAX_SENSORS	2
> +#define SOC_MAX_SENSORS	7

You may get rid of this and replace the sensors array to an dyn 
allocation based on chn_num

[ ... ]

> +static const struct rockchip_tsadc_chip rk3588_tsadc_data = {
> +	/* top, big_core0, big_core1, little_core, center, gpu, npu */
> +	.chn_id = {0, 1, 2, 3, 4, 5, 6},

You may want to revisit that. Actually, chn_id is not useful and can be 
removed everywhere as there is no hole. Otherwise a bit mask could be 
used. By removing it, SENSOR_CPU and SENSOR_GPU can be removed too.

> +	.chn_num = 7, /* seven channels for tsadc
> +	.tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> +	.tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> +	.tshut_temp = 95000,
> +	.initialize = rk_tsadcv8_initialize,
> +	.irq_ack = rk_tsadcv4_irq_ack,
> +	.control = rk_tsadcv4_control,
> +	.get_temp = rk_tsadcv4_get_temp,
> +	.set_alarm_temp = rk_tsadcv3_alarm_temp,
> +	.set_tshut_temp = rk_tsadcv3_tshut_temp,
> +	.set_tshut_mode = rk_tsadcv3_tshut_mode,
> +	.table = {
> +		.id = rk3588_code_table,
> +		.length = ARRAY_SIZE(rk3588_code_table),
> +		.data_mask = TSADCV4_DATA_MASK,
> +		.mode = ADC_INCREMENT,
> +	},
> +};
> +
>   static const struct of_device_id of_rockchip_thermal_match[] = {
>   	{	.compatible = "rockchip,px30-tsadc",
>   		.data = (void *)&px30_tsadc_data,
> @@ -1180,6 +1352,10 @@ static const struct of_device_id of_rockchip_thermal_match[] = {
>   		.compatible = "rockchip,rk3568-tsadc",
>   		.data = (void *)&rk3568_tsadc_data,
>   	},
> +	{
> +		.compatible = "rockchip,rk3588-tsadc",
> +		.data = (void *)&rk3588_tsadc_data,
> +	},
>   	{ /* end */ },
>   };
>   MODULE_DEVICE_TABLE(of, of_rockchip_thermal_match);


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