Re: [PATCH v1 2/6] thermal: netlink: Pass pointers to thermal_notify_tz_trip_change()

Lukasz Luba
2024-01-03T19:56:56Z

On 12/15/23 19:56, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Instead of requiring the caller of thermal_notify_tz_trip_change() to
> provide specific values needed to populate struct param in it, make it
> extract those values from objects passed to it by the caller via const
> pointers.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/thermal_netlink.c | 12 +++++++-----
> drivers/thermal/thermal_netlink.h | 8 ++++----
> drivers/thermal/thermal_trip.c | 8 ++------
> 3 files changed, 13 insertions(+), 15 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_netlink.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_netlink.c
> +++ linux-pm/drivers/thermal/thermal_netlink.c
> @@ -361,12 +361,14 @@ int thermal_notify_tz_trip_delete(int tz
> return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_DELETE, &p);
> }
>
> -int thermal_notify_tz_trip_change(int tz_id, int trip_id, int trip_type,
> - int trip_temp, int trip_hyst)
> +int thermal_notify_tz_trip_change(const struct thermal_zone_device *tz,
> + const struct thermal_trip *trip)
> {
> - struct param p = { .tz_id = tz_id, .trip_id = trip_id,
> - .trip_type = trip_type, .trip_temp = trip_temp,
> - .trip_hyst = trip_hyst };
> + struct param p = { .tz_id = tz->id,
> + .trip_id = thermal_zone_trip_id(tz, trip),
> + .trip_type = trip->type,
> + .trip_temp = trip->temperature,
> + .trip_hyst = trip->hysteresis };
>
> return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_CHANGE, &p);
> }
> Index: linux-pm/drivers/thermal/thermal_netlink.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_netlink.h
> +++ linux-pm/drivers/thermal/thermal_netlink.h
> @@ -23,8 +23,8 @@ int thermal_notify_tz_trip_up(int tz_id,
> int thermal_notify_tz_trip_delete(int tz_id, int id);
> int thermal_notify_tz_trip_add(int tz_id, int id, int type,
> int temp, int hyst);
> -int thermal_notify_tz_trip_change(int tz_id, int id, int type,
> - int temp, int hyst);
> +int thermal_notify_tz_trip_change(const struct thermal_zone_device *tz,
> + const struct thermal_trip *trip);
> int thermal_notify_cdev_state_update(int cdev_id, int state);
> int thermal_notify_cdev_add(int cdev_id, const char *name, int max_state);
> int thermal_notify_cdev_delete(int cdev_id);
> @@ -79,8 +79,8 @@ static inline int thermal_notify_tz_trip
> return 0;
> }
>
> -static inline int thermal_notify_tz_trip_change(int tz_id, int id, int type,
> - int temp, int hyst)
> +static inline int thermal_notify_tz_trip_change(const struct thermal_zone_device *tz,

I was wondering if it's not too long line, but I can see it's common
in that header file. Shouldn't we break such lines like:

static inline
int thermal_notify_tz_trip_change(const struct thermal_zone_device *tz,
const struct thermal_trip *trip)

?
Although, it would apply to all other long lines in that header, so not
particularly to this $subject.

> + const struct thermal_trip *trip)
> {
> return 0;
> }
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -155,9 +155,7 @@ int thermal_zone_trip_id(const struct th
> void thermal_zone_trip_updated(struct thermal_zone_device *tz,
> const struct thermal_trip *trip)
> {
> - thermal_notify_tz_trip_change(tz->id, thermal_zone_trip_id(tz, trip),
> - trip->type, trip->temperature,
> - trip->hysteresis);
> + thermal_notify_tz_trip_change(tz, trip);
> __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
> }
>
> @@ -168,8 +166,6 @@ void thermal_zone_set_trip_temp(struct t
> return;
>
> trip->temperature = temp;
> - thermal_notify_tz_trip_change(tz->id, thermal_zone_trip_id(tz, trip),
> - trip->type, trip->temperature,
> - trip->hysteresis);
> + thermal_notify_tz_trip_change(tz, trip);
> }
> EXPORT_SYMBOL_GPL(thermal_zone_set_trip_temp);
>
>
>

Other than the comment above, LGTM.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>