[Intel-xe] [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Guenter Roeck]

On 8/4/23 06:19, Nilawar, Badal wrote:
> 
> Hi Guenter,
> On 03-08-2023 04:42, Guenter Roeck wrote:
>> On 8/2/23 15:40, Andi Shyti wrote:
>>> Hi Badal,
>>>
>>> [...]
>>>
>>>> +struct xe_hwmon_data {
>>>> +    struct device *hwmon_dev;
>>>> +    struct xe_gt *gt;
>>>> +    char name[12];
>>>> +};
>>>> +
>>>> +struct xe_hwmon {
>>>> +    struct xe_hwmon_data ddat;
>>>> +    struct mutex hwmon_lock;
>>>> +};
>>>
>>> why do we need two structures here? Can we merge them?
>>>
>>
>> A later patch adds multiple hwmon devices which makes use of it.
>> I think that is flawed, and I am not inclined to accept it.
> Is there any obvious reason that there shouldn't be multiple devices? In i915 we are doing the same. https://patchwork.freedesktop.org/patch/497324/?series=104278&rev=3
> 

Technically you can do whatever you like as long as the code doesn't reside
in drivers/hwmon. I won't NACK it, but I won't give it a Reviewed-by:
either. i915 shouldn't do it, but I didn't realize what they are doing
at the time. Other drivers doing it wrong is not an argument. You can't
argue that you may drive faster than the speed limit because others do it
or because police didn't stop you last time you did either.

One chip, one hwmon device. Do you have separate parent devices for
all your hwmon devices ? If yes, you can argue that having multiple hwmon
devices make sense. If not, you can't.

Guenter