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@xxxxxxxxxxxxxxxxxx>
> ---
> 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
>
