| Message ID | 20221030083841.3433967-1-frank@crawford.emu.id.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3,1/1] hwmon: (it87) Add DMI table for future extensions | expand |
On Sun, Oct 30, 2022 at 07:38:41PM +1100, 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. > > * 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. > > v3: further updates following comments: > > * Proper use of callback functions for DMI functions. This also > involves saving dmi_data in a static variable for use as required. > > * Moved to dmi_check_system() for testing DMI table. > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > --- > drivers/hwmon/it87.c | 72 ++++++++++++++++++++++++++++++++------------ > 1 file changed, 53 insertions(+), 19 deletions(-) > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > index 73ed21ab325b..6eac15a5f647 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 */ > +}; > + > +/* Global for results from DMI matching, if needed */ > +static struct it87_dmi_data *dmi_data = NULL; > + static variables do not need to be initialized with NULL/0. > static int adc_lsb(const struct it87_data *data, int nr) > { > int lsb; > @@ -2393,7 +2401,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); > @@ -2812,24 +2819,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->skip_pwm) > + sio_data->skip_pwm |= dmi_data->skip_pwm; The second condition is unnecessary. If dmi_data->skip_pwm is 0 the |= won't do anything. > > exit: > superio_exit(sioaddr); > @@ -3307,6 +3299,46 @@ static int __init it87_device_add(int index, unsigned short address, > return err; > } > > +/* callback function for DMI */ > +static int it87_dmi_cb(const struct dmi_system_id *dmi_entry) > +{ > + dmi_data = dmi_entry->driver_data; > + > + if (dmi_data && dmi_data->skip_pwm) A dmi entry without dmi_data would be pointless, or am I missing something ? That means that checking for dmi_data should be unnecessary because it should always be set (and anyone trying to add an entry into the match table without it would learn quickly that it does not work). > + pr_info("Disabling pwm2 due to hardware constraints\n"); > + > + return 1; > +} > + > +/* > + * 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), > +}; > + > +#define IT87_DMI_MATCH_VND(vendor, name, cb, data) \ > + { \ > + .callback = cb, \ Do you envison more than one callback function ? Because if not the above could just point to it87_dmi_cb directly. > + .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", &it87_dmi_cb, &nvidia_fn68pt), The callback function does not need a & > + { } > + > +}; > +MODULE_DEVICE_TABLE(dmi, it87_dmi_table); > + > static int __init sm_it87_init(void) > { > int sioaddr[2] = { REG_2E, REG_4E }; > @@ -3319,6 +3351,8 @@ static int __init sm_it87_init(void) > if (err) > return err; > > + dmi_check_system(it87_dmi_table); > + > for (i = 0; i < ARRAY_SIZE(sioaddr); i++) { > memset(&sio_data, 0, sizeof(struct it87_sio_data)); > isa_address[i] = 0; > -- > 2.37.3 >
Guenter, First, thanks for your comments, it has forced me to look at some of the other kernel functions and I've learnt from it. On Tue, 2022-11-01 at 07:37 -0700, Guenter Roeck wrote: > On Sun, Oct 30, 2022 at 07:38:41PM +1100, 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. > > > > * 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. > > > > v3: further updates following comments: > > > > * Proper use of callback functions for DMI functions. This also > > involves saving dmi_data in a static variable for use as > > required. > > > > * Moved to dmi_check_system() for testing DMI table. > > > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > > --- > > drivers/hwmon/it87.c | 72 ++++++++++++++++++++++++++++++++-------- > > ---- > > 1 file changed, 53 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > > index 73ed21ab325b..6eac15a5f647 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 */ > > +}; > > + > > +/* Global for results from DMI matching, if needed */ > > +static struct it87_dmi_data *dmi_data = NULL; > > + > > static variables do not need to be initialized with NULL/0. Okay, will drop it in the next version. > > > static int adc_lsb(const struct it87_data *data, int nr) > > { > > int lsb; > > @@ -2393,7 +2401,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); > > @@ -2812,24 +2819,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->skip_pwm) > > + sio_data->skip_pwm |= dmi_data->skip_pwm; > > The second condition is unnecessary. If dmi_data->skip_pwm is 0 the > |= > won't do anything. Okay, will also drop that. > > > > > exit: > > superio_exit(sioaddr); > > @@ -3307,6 +3299,46 @@ static int __init it87_device_add(int index, > > unsigned short address, > > return err; > > } > > > > +/* callback function for DMI */ > > +static int it87_dmi_cb(const struct dmi_system_id *dmi_entry) > > +{ > > + dmi_data = dmi_entry->driver_data; > > + > > + if (dmi_data && dmi_data->skip_pwm) > > A dmi entry without dmi_data would be pointless, or am I missing > something ? That means that checking for dmi_data should be > unnecessary > because it should always be set (and anyone trying to add an entry > into > the match table without it would learn quickly that it does not > work). For this simple case, true, but one of the patches I would like to put up shortly is one that is used the callback to set a second chip in configuration mode before accessing the first chip. This appears to be a common requirement for all recent boards that have 2 chips, but not for those boards that have only a single chip. > > > + pr_info("Disabling pwm2 due to hardware > > constraints\n"); > > + > > + return 1; > > +} > > + > > +/* > > + * 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), > > +}; > > + > > +#define IT87_DMI_MATCH_VND(vendor, name, cb, data) \ > > + { \ > > + .callback = cb, \ > > Do you envison more than one callback function ? Because if not > the above could just point to it87_dmi_cb directly. Yes, see the note above. > > > + .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", &it87_dmi_cb, > > &nvidia_fn68pt), > > The callback function does not need a & Okay, will fix that. > > > + { } > > + > > +}; > > +MODULE_DEVICE_TABLE(dmi, it87_dmi_table); > > + > > static int __init sm_it87_init(void) > > { > > int sioaddr[2] = { REG_2E, REG_4E }; > > @@ -3319,6 +3351,8 @@ static int __init sm_it87_init(void) > > if (err) > > return err; > > > > + dmi_check_system(it87_dmi_table); > > + > > for (i = 0; i < ARRAY_SIZE(sioaddr); i++) { > > memset(&sio_data, 0, sizeof(struct it87_sio_data)); > > isa_address[i] = 0; > > -- > > 2.37.3 > > Regards Frank
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c index 73ed21ab325b..6eac15a5f647 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 */ +}; + +/* Global for results from DMI matching, if needed */ +static struct it87_dmi_data *dmi_data = NULL; + static int adc_lsb(const struct it87_data *data, int nr) { int lsb; @@ -2393,7 +2401,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); @@ -2812,24 +2819,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->skip_pwm) + sio_data->skip_pwm |= dmi_data->skip_pwm; exit: superio_exit(sioaddr); @@ -3307,6 +3299,46 @@ static int __init it87_device_add(int index, unsigned short address, return err; } +/* callback function for DMI */ +static int it87_dmi_cb(const struct dmi_system_id *dmi_entry) +{ + dmi_data = dmi_entry->driver_data; + + if (dmi_data && dmi_data->skip_pwm) + pr_info("Disabling pwm2 due to hardware constraints\n"); + + return 1; +} + +/* + * 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), +}; + +#define IT87_DMI_MATCH_VND(vendor, name, cb, data) \ + { \ + .callback = cb, \ + .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", &it87_dmi_cb, &nvidia_fn68pt), + { } + +}; +MODULE_DEVICE_TABLE(dmi, it87_dmi_table); + static int __init sm_it87_init(void) { int sioaddr[2] = { REG_2E, REG_4E }; @@ -3319,6 +3351,8 @@ static int __init sm_it87_init(void) if (err) return err; + dmi_check_system(it87_dmi_table); + for (i = 0; i < ARRAY_SIZE(sioaddr); i++) { memset(&sio_data, 0, sizeof(struct it87_sio_data)); isa_address[i] = 0;
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. v3: further updates following comments: * Proper use of callback functions for DMI functions. This also involves saving dmi_data in a static variable for use as required. * Moved to dmi_check_system() for testing DMI table. Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> --- drivers/hwmon/it87.c | 72 ++++++++++++++++++++++++++++++++------------ 1 file changed, 53 insertions(+), 19 deletions(-)