| Message ID | 20221029103057.3234561-1-frank@crawford.emu.id.au (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/1] hwmon: (it87) Create DMI matching table for various board settings | expand |
On 10/29/22 03:30, Frank Crawford wrote: > Changes in this patch set: > > * Define the DMI matching table for board specific settings during the > chip initialisation and move the only current board specific setting > to this new table. > > * Export the table for use by udev. > > v2: updates following comments: > > * Converted to use callback function. > > * Moved call to callback funtion to sio_data into it87_find in line > with other settings for sio_data. This requires dmi_data also passed > to access additional data. > 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. Thanks, Guenter
On Sat, 2022-10-29 at 07:04 -0700, Guenter Roeck wrote: > On 10/29/22 03:30, Frank Crawford wrote: > > Changes in this patch set: > > > > * Define the DMI matching table for board specific settings during > > the > > chip initialisation and move the only current board specific > > setting > > to this new table. > > > > * Export the table for use by udev. > > > > v2: updates following comments: > > > > * Converted to use callback function. > > > > * Moved call to callback funtion to sio_data into it87_find in line > > with other settings for sio_data. This requires dmi_data also > > passed > > to access additional data. > > > > 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. If I go this way, I'll would probably still use the DMI driver_data, save it, and apply that where required, rather than doing it directly in the callback function. This would be safer as, for example, in the current patch, setting the sio_data->skip_pwm is safest done after all the other settings in it87_find(), not prior to all the other setups done in that function. > > Thanks, > Guenter Regards Frank
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: > > On 10/29/22 03:30, Frank Crawford wrote: > > > Changes in this patch set: > > > > > > * Define the DMI matching table for board specific settings during > > > the > > > chip initialisation and move the only current board specific > > > setting > > > to this new table. > > > > > > * Export the table for use by udev. > > > > > > v2: updates following comments: > > > > > > * Converted to use callback function. > > > > > > * Moved call to callback funtion to sio_data into it87_find in line > > > with other settings for sio_data. This requires dmi_data also > > > passed > > > to access additional data. > > > > > > > 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. Guenter
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. 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. > > Guenter Regards Frank
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
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
On Sun, 2022-10-30 at 20:18 -0700, Guenter Roeck wrote: > On Sun, Oct 30, 2022 at 01:06:45PM +1100, Frank Crawford wrote: > > ... > > 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. No, you get me wrong, I'm not completely opposed, although partly it was a misunderstanding on my part, as there are no good examples of the use of callbacks for DMI tables in the codes I looked though. I had to go looking into the actual functions to see how they are supposed to be used. Also, the most of the items I raise are not future cases for me, as they are in the out-of-tree version, and I am working on keeping that roughly in sync with the in-tree version, while I work on trying to merge most of that functionality in. Anyway, you should see that I've now submitted a v3 patch, which I believe does use the callback correctly, and is generally suitable for handling future updates as well. > > Thanks, > Guenter Regards Frank
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c index 73ed21ab325b..4314bbca2c22 100644 --- a/drivers/hwmon/it87.c +++ b/drivers/hwmon/it87.c @@ -567,6 +567,14 @@ struct it87_data { s8 auto_temp[NUM_AUTO_PWM][5]; /* [nr][0] is point1_temp_hyst */ }; +/* Board specific settings from DMI matching */ +struct it87_dmi_data { + u8 skip_pwm; /* pwm channels to skip for this board */ + /* Callback for option setting */ + void (*apply_cb)(struct it87_sio_data *sio_data, + struct it87_dmi_data *dmi_data); +}; + static int adc_lsb(const struct it87_data *data, int nr) { int lsb; @@ -2389,11 +2397,11 @@ static const struct attribute_group it87_group_auto_pwm = { /* SuperIO detection - will change isa_address if a chip is found */ static int __init it87_find(int sioaddr, unsigned short *address, - struct it87_sio_data *sio_data) + struct it87_sio_data *sio_data, + struct it87_dmi_data *dmi_data) { int err; u16 chip_type; - const char *board_vendor, *board_name; const struct it87_devices *config; err = superio_enter(sioaddr); @@ -2812,24 +2820,9 @@ static int __init it87_find(int sioaddr, unsigned short *address, if (sio_data->beep_pin) pr_info("Beeping is supported\n"); - /* Disable specific features based on DMI strings */ - board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR); - board_name = dmi_get_system_info(DMI_BOARD_NAME); - if (board_vendor && board_name) { - if (strcmp(board_vendor, "nVIDIA") == 0 && - strcmp(board_name, "FN68PT") == 0) { - /* - * On the Shuttle SN68PT, FAN_CTL2 is apparently not - * connected to a fan, but to something else. One user - * has reported instant system power-off when changing - * the PWM2 duty cycle, so we disable it. - * I use the board name string as the trigger in case - * the same board is ever used in other systems. - */ - pr_info("Disabling pwm2 due to hardware constraints\n"); - sio_data->skip_pwm = BIT(1); - } - } + /* Set values based on DMI matches */ + if (dmi_data && dmi_data->apply_cb) + dmi_data->apply_cb(sio_data, dmi_data); exit: superio_exit(sioaddr); @@ -3307,14 +3300,57 @@ static int __init it87_device_add(int index, unsigned short address, return err; } +static void it87_dmi_cb_apply_data(struct it87_sio_data *sio_data, + struct it87_dmi_data *dmi_data) +{ + if (dmi_data->skip_pwm) { + pr_info("Disabling pwm2 due to hardware constraints\n"); + sio_data->skip_pwm |= dmi_data->skip_pwm; + } +} + +/* + * On the Shuttle SN68PT, FAN_CTL2 is apparently not + * connected to a fan, but to something else. One user + * has reported instant system power-off when changing + * the PWM2 duty cycle, so we disable it. + * I use the board name string as the trigger in case + * the same board is ever used in other systems. + */ +static struct it87_dmi_data nvidia_fn68pt = { + .skip_pwm = BIT(1), + .apply_cb = it87_dmi_cb_apply_data, +}; + +#define IT87_DMI_MATCH_VND(vendor, name, data) \ + { \ + .matches = { \ + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, vendor), \ + DMI_EXACT_MATCH(DMI_BOARD_NAME, name), \ + }, \ + .driver_data = data, \ + } + +static const struct dmi_system_id it87_dmi_table[] __initconst = { + IT87_DMI_MATCH_VND("nVIDIA", "FN68PT", &nvidia_fn68pt), + { } + +}; +MODULE_DEVICE_TABLE(dmi, it87_dmi_table); + static int __init sm_it87_init(void) { + const struct dmi_system_id *dmi = dmi_first_match(it87_dmi_table); + struct it87_dmi_data *dmi_data = NULL; int sioaddr[2] = { REG_2E, REG_4E }; struct it87_sio_data sio_data; unsigned short isa_address[2]; bool found = false; int i, err; + if (dmi) + dmi_data = dmi->driver_data; + err = platform_driver_register(&it87_driver); if (err) return err; @@ -3322,7 +3358,8 @@ static int __init sm_it87_init(void) for (i = 0; i < ARRAY_SIZE(sioaddr); i++) { memset(&sio_data, 0, sizeof(struct it87_sio_data)); isa_address[i] = 0; - err = it87_find(sioaddr[i], &isa_address[i], &sio_data); + err = it87_find(sioaddr[i], &isa_address[i], &sio_data, + dmi_data); if (err || isa_address[i] == 0) continue; /*
Changes in this patch set: * Define the DMI matching table for board specific settings during the chip initialisation and move the only current board specific setting to this new table. * Export the table for use by udev. v2: updates following comments: * Converted to use callback function. * Moved call to callback funtion to sio_data into it87_find in line with other settings for sio_data. This requires dmi_data also passed to access additional data. * Added macro for defining entries in DMI table to simplify future additions. * Note dmi_data is defined in sm_it87_init to simplify tests and for future additions. Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> --- drivers/hwmon/it87.c | 79 ++++++++++++++++++++++++++++++++------------ 1 file changed, 58 insertions(+), 21 deletions(-)