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