Re: [PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling

From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Zhang Rui <rui.zhang@intel.com>
Cc: srinivas.pandruvada@linux.intel.com, rkumbako@codeaurora.org,
	amit.kucheria@linaro.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling
Date: Wed, 1 Jul 2020 10:22:49 +0200	[thread overview]
Message-ID: <ac55475f-7c38-399f-b710-a671074f577d@linaro.org> (raw)
In-Reply-To: <171936e84c416d7647756d9b453ef2d4475ebdc8.camel@intel.com>

On 01/07/2020 09:41, Zhang Rui wrote:
> Hi, Daniel,
> 
> On Tue, 2020-06-30 at 20:32 +0200, Daniel Lezcano wrote:
>> Hi Zhang,
>>
>> thanks for taking the time to review
>>
>>
>> On 30/06/2020 18:12, Zhang Rui wrote:
>>
>> [ ... ]
>>
>>>> +int thermal_notify_tz_enable(int tz_id);
>>>> +int thermal_notify_tz_disable(int tz_id);
>>>
>>> these two will be used after merging the mode enhancement patches
>>> from
>>> Andrzej Pietrasiewicz, right?
>>
>> Yes, that is correct.
>>
>>>> +int thermal_notify_tz_trip_down(int tz_id, int id);
>>>> +int thermal_notify_tz_trip_up(int tz_id, int 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);
>>>
>>> is there any case we need to use these two?
>>
>> There is the initial proposal to add/del trip points via sysfs which
>> is
>> not merged because of no comments and the presentation from Srinivas
>> giving the 'add' and 'del' notification description, so I assumed the
>> feature would be added soon.
>>
>> These functions are here ready to be connected to the core code. If
>> anyone is planning to implement add/del trip point, he won't have to
>> implement these two notifications as they will be ready to be called.
>>
> Then I'd prefer we only introduce the events that are used or will be
> used soon, like the tz disable/enable, to avoid some potential dead
> code.
> We can easily add more events when they are needed.

Sure, if Srinivas does not need them, I can drop these notifications.

However, I would suggest to keep at the uapi header file with the
definition of the events and the attributes in order to reduce the
impact of the userspace change when adding these two notifications in
the future.

> Srinivas, do you have plan to use the trip add/delete events?
> 
>>>> +int thermal_notify_tz_trip_change(int tz_id, int id, int type,
>>>> +				  int temp, int hyst);
>>>> +int thermal_notify_cdev_update(int cdev_id, int state);
>>>
>>> This is used when the cdev cur_state is changed.
>>> what about cases that cdev->max_state changes? I think we need an
>>> extra
>>> event for it, right?
>>
>> Sure, I can add:
>>
>> int thermal_notify_cdev_change(...)
> 
> thermal_notify_cdev_change() and thermal_notify_cdev_update() looks
> confusing to me.
> Can we use thermal_notify_cdev_update_cur() and
> thermal_notify_cdev_update_max()?
> Or can we use one event, with updated cur_state and max_state?

I think it is a good idea to use the same event and depending on the
change we can add the cur_state or the max_state attribute. Up to the
userspace to figure out which one is present.

[ ... ]

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