| Message ID | 20221015120110.1472074-2-frank@crawford.emu.id.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (it87) Add DMI table | expand |
On 10/15/22 05:01, Frank Crawford wrote: > 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. > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > --- > drivers/hwmon/it87.c | 57 ++++++++++++++++++++++++++++---------------- > 1 file changed, 37 insertions(+), 20 deletions(-) > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > index 7bd154ba351b..b83ef7c89095 100644 > --- a/drivers/hwmon/it87.c > +++ b/drivers/hwmon/it87.c > @@ -2389,7 +2389,6 @@ static int __init it87_find(int sioaddr, unsigned short *address, > { > int err; > u16 chip_type; > - const char *board_vendor, *board_name; > const struct it87_devices *config; > > err = superio_enter(sioaddr); > @@ -2802,25 +2801,6 @@ 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"); This info message gets lost. > - sio_data->skip_pwm = BIT(1); > - } > - } > - > exit: > superio_exit(sioaddr); > return err; > @@ -3295,14 +3275,48 @@ static int __init it87_device_add(int index, unsigned short address, > return err; > } > > +struct it87_dmi_data { > + u8 skip_pwm; /* pwm channels to skip for this board */ > +}; > + > +/* > + * 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), > +}; > + > +static const struct dmi_system_id it87_dmi_table[] __initconst = { > + { > + .matches = { > + DMI_MATCH(DMI_BOARD_VENDOR, "nVIDIA"), > + DMI_MATCH(DMI_BOARD_NAME, "FN68PT"), > + }, > + .driver_data = &nvidia_fn68pt, Maybe add a callback function and have it display the info message. Using a callback function may make it easier to add functionality needed for other boards. Hard to say, though, since it depends on what is coming next. > + }, > + { } > + > +}; > +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; > + dmi_data is really unnecessary. Might just check for dmi below and dereference there if not NULL. > err = platform_driver_register(&it87_driver); > if (err) > return err; > @@ -3320,6 +3334,9 @@ static int __init sm_it87_init(void) > if (i && isa_address[i] == isa_address[0]) > break; > > + if (dmi_data) > + sio_data.skip_pwm |= dmi_data->skip_pwm; > + Personally I would prefer a solution which does not require extra code here, but I don't know a good one. The only means for the callback function to return data to here seems to be through static variables. Any idea what else is coming ? One option might be to keep a global copy of sio_data (eg it87_sio_data), initialize it from the dmi callback function, and use sio_data = it87_sio_data; instead of memset() to initialize it. Thanks, Guenter > err = it87_device_add(i, isa_address[i], &sio_data); > if (err) > goto exit_dev_unregister;
On 10/15/22 05:01, Frank Crawford wrote: > 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. > Just noticed: The subject of this patch is wrong. s/it86/it87/ Guenter > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > --- > drivers/hwmon/it87.c | 57 ++++++++++++++++++++++++++++---------------- > 1 file changed, 37 insertions(+), 20 deletions(-) > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > index 7bd154ba351b..b83ef7c89095 100644 > --- a/drivers/hwmon/it87.c > +++ b/drivers/hwmon/it87.c > @@ -2389,7 +2389,6 @@ static int __init it87_find(int sioaddr, unsigned short *address, > { > int err; > u16 chip_type; > - const char *board_vendor, *board_name; > const struct it87_devices *config; > > err = superio_enter(sioaddr); > @@ -2802,25 +2801,6 @@ 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); > - } > - } > - > exit: > superio_exit(sioaddr); > return err; > @@ -3295,14 +3275,48 @@ static int __init it87_device_add(int index, unsigned short address, > return err; > } > > +struct it87_dmi_data { > + u8 skip_pwm; /* pwm channels to skip for this board */ > +}; > + > +/* > + * 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), > +}; > + > +static const struct dmi_system_id it87_dmi_table[] __initconst = { > + { > + .matches = { > + DMI_MATCH(DMI_BOARD_VENDOR, "nVIDIA"), > + DMI_MATCH(DMI_BOARD_NAME, "FN68PT"), > + }, > + .driver_data = &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; > @@ -3320,6 +3334,9 @@ static int __init sm_it87_init(void) > if (i && isa_address[i] == isa_address[0]) > break; > > + if (dmi_data) > + sio_data.skip_pwm |= dmi_data->skip_pwm; > + > err = it87_device_add(i, isa_address[i], &sio_data); > if (err) > goto exit_dev_unregister;
On Sat, 2022-10-15 at 05:27 -0700, Guenter Roeck wrote: > On 10/15/22 05:01, Frank Crawford wrote: > > 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. > > > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > > --- > > drivers/hwmon/it87.c | 57 ++++++++++++++++++++++++++++----------- > > ----- > > 1 file changed, 37 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > > index 7bd154ba351b..b83ef7c89095 100644 > > --- a/drivers/hwmon/it87.c > > +++ b/drivers/hwmon/it87.c > > @@ -2389,7 +2389,6 @@ static int __init it87_find(int sioaddr, > > unsigned short *address, > > { > > int err; > > u16 chip_type; > > - const char *board_vendor, *board_name; > > const struct it87_devices *config; > > > > err = superio_enter(sioaddr); > > @@ -2802,25 +2801,6 @@ 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"); > > This info message gets lost. True, although I doubt most users will understand what it means. However, if we go through with your later suggestions I will add it back in. > > > - sio_data->skip_pwm = BIT(1); > > - } > > - } > > - > > exit: > > superio_exit(sioaddr); > > return err; > > @@ -3295,14 +3275,48 @@ static int __init it87_device_add(int > > index, unsigned short address, > > return err; > > } > > > > +struct it87_dmi_data { > > + u8 skip_pwm; /* pwm channels to skip for this > > board */ > > +}; > > + > > +/* > > + * 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), > > +}; > > + > > +static const struct dmi_system_id it87_dmi_table[] __initconst = { > > + { > > + .matches = { > > + DMI_MATCH(DMI_BOARD_VENDOR, "nVIDIA"), > > + DMI_MATCH(DMI_BOARD_NAME, "FN68PT"), > > + }, > > + .driver_data = &nvidia_fn68pt, > > Maybe add a callback function and have it display the info message. > Using a callback function may make it easier to add functionality > needed for other boards. Hard to say, though, since it depends on > what is coming next. The current planned additions are settings to disable ACPI failures on boards we are aware of (following on from a recent patch to sometimes ignore ACPI issues), and one to initialise chips on certain boards with multi-chip boards. Certainly, a callback function may make some things easier, and I'll look into it. > > > + }, > > + { } > > + > > +}; > > +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; > > + > > dmi_data is really unnecessary. Might just check for dmi below and > dereference there if not NULL. Yes, will do that. > > > err = platform_driver_register(&it87_driver); > > if (err) > > return err; > > @@ -3320,6 +3334,9 @@ static int __init sm_it87_init(void) > > if (i && isa_address[i] == isa_address[0]) > > break; > > > > + if (dmi_data) > > + sio_data.skip_pwm |= dmi_data->skip_pwm; > > + > > Personally I would prefer a solution which does not require extra > code here, > but I don't know a good one. The only means for the callback function > to return > data to here seems to be through static variables. Any idea what else > is coming ? See above, although the planned updates include setting variable it87_sio_data and/or calling an initialisation function for a chip. > One option might be to keep a global copy of sio_data (eg > it87_sio_data), > initialize it from the dmi callback function, and use > sio_data = it87_sio_data; > instead of memset() to initialize it. However, yes I'll see what I can do about this. > > Thanks, > Guenter > > > err = it87_device_add(i, isa_address[i], > > &sio_data); > > if (err) > > goto exit_dev_unregister; > Thanks Frank
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c index 7bd154ba351b..b83ef7c89095 100644 --- a/drivers/hwmon/it87.c +++ b/drivers/hwmon/it87.c @@ -2389,7 +2389,6 @@ static int __init it87_find(int sioaddr, unsigned short *address, { int err; u16 chip_type; - const char *board_vendor, *board_name; const struct it87_devices *config; err = superio_enter(sioaddr); @@ -2802,25 +2801,6 @@ 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); - } - } - exit: superio_exit(sioaddr); return err; @@ -3295,14 +3275,48 @@ static int __init it87_device_add(int index, unsigned short address, return err; } +struct it87_dmi_data { + u8 skip_pwm; /* pwm channels to skip for this board */ +}; + +/* + * 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), +}; + +static const struct dmi_system_id it87_dmi_table[] __initconst = { + { + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "nVIDIA"), + DMI_MATCH(DMI_BOARD_NAME, "FN68PT"), + }, + .driver_data = &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; @@ -3320,6 +3334,9 @@ static int __init sm_it87_init(void) if (i && isa_address[i] == isa_address[0]) break; + if (dmi_data) + sio_data.skip_pwm |= dmi_data->skip_pwm; + err = it87_device_add(i, isa_address[i], &sio_data); if (err) goto exit_dev_unregister;
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. Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> --- drivers/hwmon/it87.c | 57 ++++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 20 deletions(-)