On Sun, Oct 30, 2022 at 01:06:45PM +1100, 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. > Sorry, that is purely hypothetical. Kernel code should not be optimized for hypothetical situations. > 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. > I can see that you are for whatever reason completely opposed to using the dmi callback function. Fine, but then don't introduce another one just to have a callback function and in case it may possibly be needed sometime in the future. Introduce it if and when it is needed, and only then. The same applies to all other infrastructure: Introduce it if and only if it is needed for more than one use case, not because it may be needed for some unspecified use case some time in the future. Thanks, Guenter
