Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 6 Aug 2018 11:05:52 -0700
From:   Eduardo Valentin <edubezval@gmail.com>
To:     Viresh Kumar <viresh.kumar@linaro.org>
Cc:     Zhang Rui <rui.zhang@intel.com>, linux-pm@vger.kernel.org,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] of: thermal: Allow multiple devices to share cooling map
Message-ID: <20180806180551.GA4760@localhost.localdomain>
References: <cover.1530766981.git.viresh.kumar@linaro.org>
 <b5555967e713fd601bc74de0cab8ab45608399c4.1531824436.git.viresh.kumar@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b5555967e713fd601bc74de0cab8ab45608399c4.1531824436.git.viresh.kumar@linaro.org>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello,

On Tue, Jul 17, 2018 at 04:24:16PM +0530, Viresh Kumar wrote:
> A cooling map entry may now contain a list of phandles and their
> arguments representing multiple devices which share the trip point.
> 
> This patch updates the thermal OF core to parse them properly.

I am mostly fine with the patch idea, specially because we wont be
breaking existing DT blobs out there.

See comment below.

> 
> Tested on Hikey960.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> This is a follow up patch to the DT bindings patchset:
> 
> https://lkml.kernel.org/r/cover.1530766981.git.viresh.kumar@linaro.org
> 
>  drivers/thermal/of-thermal.c | 140 +++++++++++++++++++++++++----------
>  1 file changed, 102 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 977a8307fbb1..79c06c4c861b 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -19,22 +19,33 @@
>  /***   Private data structures to represent thermal device tree data ***/
>  
>  /**
> - * struct __thermal_bind_param - a match between trip and cooling device
> + * struct __thermal_cooling_bind_param - a cooling device for a trip point
>   * @cooling_device: a pointer to identify the referred cooling device
> - * @trip_id: the trip point index
> - * @usage: the percentage (from 0 to 100) of cooling contribution
>   * @min: minimum cooling state used at this trip point
>   * @max: maximum cooling state used at this trip point
>   */
>  
> -struct __thermal_bind_params {
> +struct __thermal_cooling_bind_param {
>  	struct device_node *cooling_device;
> -	unsigned int trip_id;
> -	unsigned int usage;
>  	unsigned long min;
>  	unsigned long max;
>  };
>  
> +/**
> + * struct __thermal_bind_param - a match between trip and cooling device
> + * @tcbp: a pointer to an array of cooling devices
> + * @count: number of elements in array
> + * @trip_id: the trip point index
> + * @usage: the percentage (from 0 to 100) of cooling contribution
> + */
> +
> +struct __thermal_bind_params {
> +	struct __thermal_cooling_bind_param *tcbp;
> +	unsigned int count;
> +	unsigned int trip_id;
> +	unsigned int usage;
> +};

Do you mind elaborating why you needed to do this split and adding a new
struct? More important, please describe in your commit message.

> +
>  /**
>   * struct __thermal_zone - internal representation of a thermal zone
>   * @mode: current thermal zone device mode (enabled/disabled)
> @@ -192,25 +203,31 @@ static int of_thermal_bind(struct thermal_zone_device *thermal,
>  			   struct thermal_cooling_device *cdev)
>  {
>  	struct __thermal_zone *data = thermal->devdata;
> -	int i;
> +	struct __thermal_bind_params *tbp;
> +	struct __thermal_cooling_bind_param *tcbp;
> +	int i, j;
>  
>  	if (!data || IS_ERR(data))
>  		return -ENODEV;
>  
>  	/* find where to bind */
>  	for (i = 0; i < data->num_tbps; i++) {
> -		struct __thermal_bind_params *tbp = data->tbps + i;
> +		tbp = data->tbps + i;
>  
> -		if (tbp->cooling_device == cdev->np) {
> -			int ret;
> +		for (j = 0; j < tbp->count; j++) {
> +			tcbp = tbp->tcbp + j;
>  
> -			ret = thermal_zone_bind_cooling_device(thermal,
> +			if (tcbp->cooling_device == cdev->np) {
> +				int ret;
> +
> +				ret = thermal_zone_bind_cooling_device(thermal,
>  						tbp->trip_id, cdev,
> -						tbp->max,
> -						tbp->min,
> +						tcbp->max,
> +						tcbp->min,
>  						tbp->usage);
> -			if (ret)
> -				return ret;
> +				if (ret)
> +					return ret;
> +			}
>  		}
>  	}
>  
> @@ -221,22 +238,28 @@ static int of_thermal_unbind(struct thermal_zone_device *thermal,
>  			     struct thermal_cooling_device *cdev)
>  {
>  	struct __thermal_zone *data = thermal->devdata;
> -	int i;
> +	struct __thermal_bind_params *tbp;
> +	struct __thermal_cooling_bind_param *tcbp;
> +	int i, j;
>  
>  	if (!data || IS_ERR(data))
>  		return -ENODEV;
>  
>  	/* find where to unbind */
>  	for (i = 0; i < data->num_tbps; i++) {
> -		struct __thermal_bind_params *tbp = data->tbps + i;
> +		tbp = data->tbps + i;
> +
> +		for (j = 0; j < tbp->count; j++) {
> +			tcbp = tbp->tcbp + j;
>  
> -		if (tbp->cooling_device == cdev->np) {
> -			int ret;
> +			if (tcbp->cooling_device == cdev->np) {
> +				int ret;
>  
> -			ret = thermal_zone_unbind_cooling_device(thermal,
> -						tbp->trip_id, cdev);
> -			if (ret)
> -				return ret;
> +				ret = thermal_zone_unbind_cooling_device(thermal,
> +							tbp->trip_id, cdev);
> +				if (ret)
> +					return ret;
> +			}
>  		}
>  	}
>  
> @@ -652,8 +675,9 @@ static int thermal_of_populate_bind_params(struct device_node *np,
>  					   int ntrips)
>  {
>  	struct of_phandle_args cooling_spec;
> +	struct __thermal_cooling_bind_param *__tcbp;
>  	struct device_node *trip;
> -	int ret, i;
> +	int ret, i, count;
>  	u32 prop;
>  
>  	/* Default weight. Usage is optional */
> @@ -680,20 +704,44 @@ static int thermal_of_populate_bind_params(struct device_node *np,
>  		goto end;
>  	}
>  
> -	ret = of_parse_phandle_with_args(np, "cooling-device", "#cooling-cells",
> -					 0, &cooling_spec);
> -	if (ret < 0) {
> +	count = of_count_phandle_with_args(np, "cooling-device",
> +					   "#cooling-cells");
> +	if (!count) {
>  		pr_err("missing cooling_device property\n");

Probably the above error message deserves a better fit to the current
situation. Maybe:
+  		pr_err("Add a cooling_device property with at least one device\n");

>  		goto end;
>  	}
> -	__tbp->cooling_device = cooling_spec.np;
> -	if (cooling_spec.args_count >= 2) { /* at least min and max */
> -		__tbp->min = cooling_spec.args[0];
> -		__tbp->max = cooling_spec.args[1];
> -	} else {
> -		pr_err("wrong reference to cooling device, missing limits\n");
> +
> +	__tcbp = kcalloc(count, sizeof(*__tcbp), GFP_KERNEL);
> +	if (!__tcbp)
> +		goto end;
> +
> +	for (i = 0; i < count; i++) {
> +		ret = of_parse_phandle_with_args(np, "cooling-device",
> +				"#cooling-cells", i, &cooling_spec);
> +		if (ret < 0) {
> +			pr_err("missing cooling_device property\n");

Here is a wrongly written cooling_device phandle, no?

> +			goto free_tcbp;
> +		}
> +
> +		__tcbp[i].cooling_device = cooling_spec.np;
> +
> +		if (cooling_spec.args_count >= 2) { /* at least min and max */
> +			__tcbp[i].min = cooling_spec.args[0];
> +			__tcbp[i].max = cooling_spec.args[1];
> +		} else {
> +			pr_err("wrong reference to cooling device, missing limits\n");
> +		}
>  	}
>  
> +	__tbp->tcbp= __tcbp;
> +	__tbp->count = count;
> +
> +	goto end;
> +
> +free_tcbp:
> +	for (i = i - 1; i >= 0; i--)
> +		of_node_put(__tcbp[i].cooling_device);
> +	kfree(__tcbp);
>  end:
>  	of_node_put(trip);
>  
> @@ -900,8 +948,16 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
>  	return tz;
>  
>  free_tbps:
> -	for (i = i - 1; i >= 0; i--)
> -		of_node_put(tz->tbps[i].cooling_device);
> +	for (i = i - 1; i >= 0; i--) {
> +		struct __thermal_bind_params *tbp = tz->tbps + i;
> +		int j;
> +
> +		for (j = 0; j < tbp->count; j++)
> +			of_node_put(tbp->tcbp[j].cooling_device);
> +
> +		kfree(tbp->tcbp);
> +	}
> +
>  	kfree(tz->tbps);
>  free_trips:
>  	for (i = 0; i < tz->ntrips; i++)
> @@ -917,10 +973,18 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
>  
>  static inline void of_thermal_free_zone(struct __thermal_zone *tz)
>  {
> -	int i;
> +	struct __thermal_bind_params *tbp;
> +	int i, j;
> +
> +	for (i = 0; i < tz->num_tbps; i++) {
> +		tbp = tz->tbps + i;
> +
> +		for (j = 0; j < tbp->count; j++)
> +			of_node_put(tbp->tcbp[j].cooling_device);
> +
> +		kfree(tbp->tcbp);
> +	}
>  
> -	for (i = 0; i < tz->num_tbps; i++)
> -		of_node_put(tz->tbps[i].cooling_device);
>  	kfree(tz->tbps);
>  	for (i = 0; i < tz->ntrips; i++)
>  		of_node_put(tz->trips[i].np);
> -- 
> 2.18.0.rc1.242.g61856ae69a2c
>