Re: [PATCH v3 02/12] thermal/of: Replace device node match with device node search

Lukasz Luba
2022-07-04T07:59:30Z

On 7/3/22 19:30, Daniel Lezcano wrote:
> The thermal_of code builds a trip array associated with the node
> pointer in order to compare the trip point phandle with the list.
>
> The thermal trip is a thermal zone property and should be moved
> there. If some sensors have hardcoded trip points, they should use the
> exported structure instead of redefining again and again their own
> structure and data to describe exactly the same things.
>
> In order to move this to the thermal.h header and allow more cleanup,
> we need to remove the node pointer from the structure.
>
> Instead of building storing the device node, we search directly in the
> device tree the corresponding node. That results in a simplification
> of the code and allows to move the structure to thermal.h
>
> Cc: Alexandre Bailon <abailon@baylibre.com>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc; Eduardo Valentin <eduval@amazon.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linexp.org>
> ---
> drivers/thermal/thermal_of.c | 62 ++++++++++++++++++++++++------------
> 1 file changed, 42 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index b65d435cb92f..04c910ca8623 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -671,6 +671,35 @@ EXPORT_SYMBOL_GPL(devm_thermal_zone_of_sensor_unregister);
>
> /*** functions parsing device tree nodes ***/
>
> +static int of_find_trip_id(struct device_node *np, struct device_node *trip)
> +{
> + struct device_node *trips;
> + struct device_node *t;
> + int i = 0;
> +
> + trips = of_get_child_by_name(np, "trips");
> + if (!trips) {
> + pr_err("Failed to find 'trips' node\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * Find the trip id point associated with the cooling device map
> + */
> + for_each_child_of_node(trips, t) {
> +
> + if (t == trip)
> + goto out;
> + i++;
> + }
> +
> + i = -ENXIO;
> +out:
> + of_node_put(trips);
> +
> + return i;
> +}
> +
> /**
> * thermal_of_populate_bind_params - parse and fill cooling map data
> * @np: DT node containing a cooling-map node
> @@ -686,14 +715,13 @@ EXPORT_SYMBOL_GPL(devm_thermal_zone_of_sensor_unregister);
> * Return: 0 on success, proper error code otherwise
> */
> static int thermal_of_populate_bind_params(struct device_node *np,
> - struct __thermal_bind_params *__tbp,
> - struct thermal_trip *trips,
> - int ntrips)
> + struct __thermal_bind_params *__tbp)
> {
> struct of_phandle_args cooling_spec;
> struct __thermal_cooling_bind_param *__tcbp;
> struct device_node *trip;
> int ret, i, count;
> + int trip_id;
> u32 prop;
>
> /* Default weight. Usage is optional */
> @@ -708,18 +736,14 @@ static int thermal_of_populate_bind_params(struct device_node *np,
> return -ENODEV;
> }
>
> - /* match using device_node */
> - for (i = 0; i < ntrips; i++)
> - if (trip == trips[i].np) {
> - __tbp->trip_id = i;
> - break;
> - }
> -
> - if (i == ntrips) {
> - ret = -ENODEV;
> + trip_id = of_find_trip_id(np, trip);
> + if (trip_id < 0) {
> + ret = trip_id;
> goto end;
> }
>
> + __tbp->trip_id = trip_id;
> +
> count = of_count_phandle_with_args(np, "cooling-device",
> "#cooling-cells");
> if (count <= 0) {
> @@ -868,6 +892,7 @@ static struct __thermal_zone
> __init *thermal_of_build_thermal_zone(struct device_node *np)
> {
> struct device_node *child = NULL, *gchild;
> + struct device_node *trips;
> struct __thermal_zone *tz;
> int ret, i;
> u32 prop, coef[2];
> @@ -910,13 +935,13 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
> }
>
> /* trips */
> - child = of_get_child_by_name(np, "trips");
> + trips = of_get_child_by_name(np, "trips");
>
> /* No trips provided */
> - if (!child)
> + if (!trips)
> goto finish;
>
> - tz->ntrips = of_get_child_count(child);
> + tz->ntrips = of_get_child_count(trips);
> if (tz->ntrips == 0) /* must have at least one child */
> goto finish;
>
> @@ -927,14 +952,12 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
> }
>
> i = 0;
> - for_each_child_of_node(child, gchild) {
> + for_each_child_of_node(trips, gchild) {
> ret = thermal_of_populate_trip(gchild, &tz->trips[i++]);
> if (ret)
> goto free_trips;
> }
>
> - of_node_put(child);
> -

It's probably needed to put the 'trips' here, isn't it?
Or at the end in 'free_trips'.

> /* cooling-maps */
> child = of_get_child_by_name(np, "cooling-maps");
>
> @@ -954,8 +977,7 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
>
> i = 0;
> for_each_child_of_node(child, gchild) {
> - ret = thermal_of_populate_bind_params(gchild, &tz->tbps[i++],
> - tz->trips, tz->ntrips);
> + ret = thermal_of_populate_bind_params(gchild, &tz->tbps[i++]);
> if (ret)
> goto free_tbps;
> }