Re: [PATCH v2 1/1] hwmon: (it87) Create DMI matching table for various board settings

[Guenter Roeck <linux@roeck-us.net>]

On 10/29/22 19:06, Frank Crawford wrote:
> On Sat, 2022-10-29 at 18:39 -0700, Guenter Roeck wrote:
>> On Sun, Oct 30, 2022 at 10:43:59AM +1100, Frank Crawford wrote:
>>> On Sat, 2022-10-29 at 07:04 -0700, Guenter Roeck wrote:
>>>>
> ...
>>>> That is really not what I meant when I asked to use a callback
>>>> function.
>>>> As written, the code might as well call that function directly
>>>> from
>>>> the
>>>> init code, and there would be no reason to have a callback
>>>> function
>>>> pointer.
>>>>
>>>> A callback function would only make sense to me if it is added
>>>> to struct dmi_system_id, and called via dmi_check_system().
>>>> See other callers of dmi_check_system() for examples.
>>>
>>> Oh, investigating other kernel code I see what you mean, and it
>>> does
>>> simplify one possible future update, but looking through the other
>>> modules in hwmon, I can't see any using a DMI callback.  The
>>> primary
>>> use of dmi_check_system() is just as a count of successful matches.
>>>
>>> Also, just going back to a previous comment about creating a static
>>> version of sio_data and updating this in the callback, this does
>>> worry
>>> me going forward as in future I hope to add code to handle the case
>>> of
>>> multiple chips.  Updating the static version for one chip may cause
>>> issues with the other chips.
>>>
>> The value is set based on DMI data. I don't see how that would make
>> a difference even if there are multiple chips. The DMI data would
>> still be the same and is board specific, not chip specific.
> 
> For present cases, yes, but consider the current setting, which
> disables pmw2 for the FN68PT board, if there was a second chip on that
> board, you would not want the same setting for both chips.
> 

Quite obviously it _is_ known that this board only has a single chip,
and it will never magically have a second chip.

Chip specific code should be implemented in the probe function (which
is chip specific), not in the init function (which isn't).

> I haven't yet worked out how it would be distinguish at the time, but
> also it hasn't been strictly necessary.
> 
> A simple case I have coming up for future patch is to use the DMI table
> to ignore ACPI conflicts when we know it is safe, but that should be
> done on each chip separately, not necessarily globally for all chips on
> that board.  Again, in practice it isn't important, and I haven't
> worked out how to specify it separately yet.
> 
> Also, I have been looking at the difference in the use of
> dmi_check_system() and what the use of dmi_first_match() does and it
> really is just a case of the callback being used at the time of
> matching vs deferring the actions to be performed at later and possibly
> more appropriate stages.
> 

It doesn't need to do anything but set values or parameters. Any action
should be done in the probe function, not in the init function.

Guenter