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

[Guenter Roeck]

On 8/8/23 14:31, Rodrigo Vivi wrote:
> On Fri, Aug 04, 2023 at 08:06:22PM +0530, Nilawar, Badal wrote:
>>
>>
>> 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 single hwmon device.
> 
> Well, it is one PCI device, but under 1 pci device we can have multiple "tiles"
> that can duplicate many components. Inside each tile we can even have multiple
> "gt"s.
> 
> But back to the tile, each tile has its own metrics. It's own power delivery,
> own sensors and all. They can even be seen as independent devices from this
> angle.
> 
> I'm afraid that the attempt to put everything as one device, but all the
> entries duplicated per tile/gt we might end up with a messed api.
> 

Your argument does not make sense. I am not asking to duplicate anything.

Guenter