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

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 04-08-2023 19:56, Guenter Roeck wrote:
> 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.
Thanks for clarification. There is only one parent device, so will try 
to accommodate one hwmon device approach.

Regards,
Badal
> 
> Guenter
>