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