[PATCH v2 1/1] hwmon: (it87) Create DMI matching table for various board settings

[PATCH v2 1/1] hwmon: (it87) Create DMI matching table for various board settings

[Frank Crawford]

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.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
 drivers/hwmon/it87.c | 79 ++++++++++++++++++++++++++++++++------------
 1 file changed, 58 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 73ed21ab325b..4314bbca2c22 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  */
+	/* Callback for option setting */
+	void (*apply_cb)(struct it87_sio_data *sio_data,
+			 struct it87_dmi_data *dmi_data);
+};
+
 static int adc_lsb(const struct it87_data *data, int nr)
 {
 	int lsb;
@@ -2389,11 +2397,11 @@ static const struct attribute_group it87_group_auto_pwm = {
 
 /* SuperIO detection - will change isa_address if a chip is found */
 static int __init it87_find(int sioaddr, unsigned short *address,
-			    struct it87_sio_data *sio_data)
+			    struct it87_sio_data *sio_data,
+			    struct it87_dmi_data *dmi_data)
 {
 	int err;
 	u16 chip_type;
-	const char *board_vendor, *board_name;
 	const struct it87_devices *config;
 
 	err = superio_enter(sioaddr);
@@ -2812,24 +2820,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->apply_cb)
+		dmi_data->apply_cb(sio_data, dmi_data);
 
 exit:
 	superio_exit(sioaddr);
@@ -3307,14 +3300,57 @@ static int __init it87_device_add(int index, unsigned short address,
 	return err;
 }
 
+static void it87_dmi_cb_apply_data(struct it87_sio_data *sio_data,
+				   struct it87_dmi_data *dmi_data)
+{
+	if (dmi_data->skip_pwm) {
+		pr_info("Disabling pwm2 due to hardware constraints\n");
+		sio_data->skip_pwm |= dmi_data->skip_pwm;
+	}
+}
+
+/*
+ * 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),
+	.apply_cb = it87_dmi_cb_apply_data,
+};
+
+#define IT87_DMI_MATCH_VND(vendor, name, data) \
+	{ \
+		.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", &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;
@@ -3322,7 +3358,8 @@ static int __init sm_it87_init(void)
 	for (i = 0; i < ARRAY_SIZE(sioaddr); i++) {
 		memset(&sio_data, 0, sizeof(struct it87_sio_data));
 		isa_address[i] = 0;
-		err = it87_find(sioaddr[i], &isa_address[i], &sio_data);
+		err = it87_find(sioaddr[i], &isa_address[i], &sio_data,
+				dmi_data);
 		if (err || isa_address[i] == 0)
 			continue;
 		/*
-- 
2.37.3

Re: [PATCH v2 1/1] hwmon: (it87) Create DMI matching table for various board settings

[Guenter Roeck]

On 10/29/22 03:30, 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.
> 

That is really not what I meant when I asked to use a callback function.
As written, the code might as well call that function directly from the
init code, and there would be no reason to have a callback function
pointer.

A callback function would only make sense to me if it is added
to struct dmi_system_id, and called via dmi_check_system().
See other callers of dmi_check_system() for examples.

Thanks,
Guenter

Re: [PATCH v2 1/1] hwmon: (it87) Create DMI matching table for various board settings

[Frank Crawford]

On Sat, 2022-10-29 at 07:04 -0700, Guenter Roeck wrote:
> On 10/29/22 03:30, 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.
> > 
> 
> That is really not what I meant when I asked to use a callback
> function.
> As written, the code might as well call that function directly from
> the
> init code, and there would be no reason to have a callback function
> pointer.
> 
> A callback function would only make sense to me if it is added
> to struct dmi_system_id, and called via dmi_check_system().
> See other callers of dmi_check_system() for examples.

Oh, investigating other kernel code I see what you mean, and it does
simplify one possible future update, but looking through the other
modules in hwmon, I can't see any using a DMI callback.  The primary
use of dmi_check_system() is just as a count of successful matches.

Also, just going back to a previous comment about creating a static
version of sio_data and updating this in the callback, this does worry
me going forward as in future I hope to add code to handle the case of
multiple chips.  Updating the static version for one chip may cause
issues with the other chips.

If I go this way, I'll would probably still use the DMI driver_data,
save it, and apply that where required, rather than doing it directly
in the callback function.  This would be safer as, for example, in the
current patch, setting the sio_data->skip_pwm is safest done after all
the other settings in it87_find(), not prior to all the other setups
done in that function.

> 
> Thanks,
> Guenter

Regards
Frank

Re: [PATCH v2 1/1] hwmon: (it87) Create DMI matching table for various board settings

[Guenter Roeck]

On Sun, Oct 30, 2022 at 10:43:59AM +1100, Frank Crawford wrote:
> On Sat, 2022-10-29 at 07:04 -0700, Guenter Roeck wrote:
> > On 10/29/22 03:30, 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.
> > > 
> > 
> > That is really not what I meant when I asked to use a callback
> > function.
> > As written, the code might as well call that function directly from
> > the
> > init code, and there would be no reason to have a callback function
> > pointer.
> > 
> > A callback function would only make sense to me if it is added
> > to struct dmi_system_id, and called via dmi_check_system().
> > See other callers of dmi_check_system() for examples.
> 
> Oh, investigating other kernel code I see what you mean, and it does
> simplify one possible future update, but looking through the other
> modules in hwmon, I can't see any using a DMI callback.  The primary
> use of dmi_check_system() is just as a count of successful matches.
> 
> Also, just going back to a previous comment about creating a static
> version of sio_data and updating this in the callback, this does worry
> me going forward as in future I hope to add code to handle the case of
> multiple chips.  Updating the static version for one chip may cause
> issues with the other chips.
> 
The value is set based on DMI data. I don't see how that would make
a difference even if there are multiple chips. The DMI data would
still be the same and is board specific, not chip specific.

Guenter

Re: [PATCH v2 1/1] hwmon: (it87) Create DMI matching table for various board settings

[Frank Crawford]

On Sat, 2022-10-29 at 18:39 -0700, Guenter Roeck wrote:
> On Sun, Oct 30, 2022 at 10:43:59AM +1100, Frank Crawford wrote:
> > On Sat, 2022-10-29 at 07:04 -0700, Guenter Roeck wrote:
> > > 
...
> > > That is really not what I meant when I asked to use a callback
> > > function.
> > > As written, the code might as well call that function directly
> > > from
> > > the
> > > init code, and there would be no reason to have a callback
> > > function
> > > pointer.
> > > 
> > > A callback function would only make sense to me if it is added
> > > to struct dmi_system_id, and called via dmi_check_system().
> > > See other callers of dmi_check_system() for examples.
> > 
> > Oh, investigating other kernel code I see what you mean, and it
> > does
> > simplify one possible future update, but looking through the other
> > modules in hwmon, I can't see any using a DMI callback.  The
> > primary
> > use of dmi_check_system() is just as a count of successful matches.
> > 
> > Also, just going back to a previous comment about creating a static
> > version of sio_data and updating this in the callback, this does
> > worry
> > me going forward as in future I hope to add code to handle the case
> > of
> > multiple chips.  Updating the static version for one chip may cause
> > issues with the other chips.
> > 
> The value is set based on DMI data. I don't see how that would make
> a difference even if there are multiple chips. The DMI data would
> still be the same and is board specific, not chip specific.

For present cases, yes, but consider the current setting, which
disables pmw2 for the FN68PT board, if there was a second chip on that
board, you would not want the same setting for both chips.

I haven't yet worked out how it would be distinguish at the time, but
also it hasn't been strictly necessary.

A simple case I have coming up for future patch is to use the DMI table
to ignore ACPI conflicts when we know it is safe, but that should be
done on each chip separately, not necessarily globally for all chips on
that board.  Again, in practice it isn't important, and I haven't
worked out how to specify it separately yet.

Also, I have been looking at the difference in the use of
dmi_check_system() and what the use of dmi_first_match() does and it
really is just a case of the callback being used at the time of
matching vs deferring the actions to be performed at later and possibly
more appropriate stages.

> 
> Guenter

Regards
Frank

Re: [PATCH v2 1/1] hwmon: (it87) Create DMI matching table for various board settings

[Guenter Roeck]

On 10/29/22 19:06, Frank Crawford wrote:
> On Sat, 2022-10-29 at 18:39 -0700, Guenter Roeck wrote:
>> On Sun, Oct 30, 2022 at 10:43:59AM +1100, Frank Crawford wrote:
>>> On Sat, 2022-10-29 at 07:04 -0700, Guenter Roeck wrote:
>>>>
> ...
>>>> That is really not what I meant when I asked to use a callback
>>>> function.
>>>> As written, the code might as well call that function directly
>>>> from
>>>> the
>>>> init code, and there would be no reason to have a callback
>>>> function
>>>> pointer.
>>>>
>>>> A callback function would only make sense to me if it is added
>>>> to struct dmi_system_id, and called via dmi_check_system().
>>>> See other callers of dmi_check_system() for examples.
>>>
>>> Oh, investigating other kernel code I see what you mean, and it
>>> does
>>> simplify one possible future update, but looking through the other
>>> modules in hwmon, I can't see any using a DMI callback.  The
>>> primary
>>> use of dmi_check_system() is just as a count of successful matches.
>>>
>>> Also, just going back to a previous comment about creating a static
>>> version of sio_data and updating this in the callback, this does
>>> worry
>>> me going forward as in future I hope to add code to handle the case
>>> of
>>> multiple chips.  Updating the static version for one chip may cause
>>> issues with the other chips.
>>>
>> The value is set based on DMI data. I don't see how that would make
>> a difference even if there are multiple chips. The DMI data would
>> still be the same and is board specific, not chip specific.
> 
> For present cases, yes, but consider the current setting, which
> disables pmw2 for the FN68PT board, if there was a second chip on that
> board, you would not want the same setting for both chips.
> 

Quite obviously it _is_ known that this board only has a single chip,
and it will never magically have a second chip.

Chip specific code should be implemented in the probe function (which
is chip specific), not in the init function (which isn't).

> I haven't yet worked out how it would be distinguish at the time, but
> also it hasn't been strictly necessary.
> 
> A simple case I have coming up for future patch is to use the DMI table
> to ignore ACPI conflicts when we know it is safe, but that should be
> done on each chip separately, not necessarily globally for all chips on
> that board.  Again, in practice it isn't important, and I haven't
> worked out how to specify it separately yet.
> 
> Also, I have been looking at the difference in the use of
> dmi_check_system() and what the use of dmi_first_match() does and it
> really is just a case of the callback being used at the time of
> matching vs deferring the actions to be performed at later and possibly
> more appropriate stages.
> 

It doesn't need to do anything but set values or parameters. Any action
should be done in the probe function, not in the init function.

Guenter

Re: [PATCH v2 1/1] hwmon: (it87) Create DMI matching table for various board settings

[Guenter Roeck]

On Sun, Oct 30, 2022 at 01:06:45PM +1100, Frank Crawford wrote:
> On Sat, 2022-10-29 at 18:39 -0700, Guenter Roeck wrote:
> > On Sun, Oct 30, 2022 at 10:43:59AM +1100, Frank Crawford wrote:
> > > On Sat, 2022-10-29 at 07:04 -0700, Guenter Roeck wrote:
> > > > 
> ...
> > > > That is really not what I meant when I asked to use a callback
> > > > function.
> > > > As written, the code might as well call that function directly
> > > > from
> > > > the
> > > > init code, and there would be no reason to have a callback
> > > > function
> > > > pointer.
> > > > 
> > > > A callback function would only make sense to me if it is added
> > > > to struct dmi_system_id, and called via dmi_check_system().
> > > > See other callers of dmi_check_system() for examples.
> > > 
> > > Oh, investigating other kernel code I see what you mean, and it
> > > does
> > > simplify one possible future update, but looking through the other
> > > modules in hwmon, I can't see any using a DMI callback.  The
> > > primary
> > > use of dmi_check_system() is just as a count of successful matches.
> > > 
> > > Also, just going back to a previous comment about creating a static
> > > version of sio_data and updating this in the callback, this does
> > > worry
> > > me going forward as in future I hope to add code to handle the case
> > > of
> > > multiple chips.  Updating the static version for one chip may cause
> > > issues with the other chips.
> > > 
> > The value is set based on DMI data. I don't see how that would make
> > a difference even if there are multiple chips. The DMI data would
> > still be the same and is board specific, not chip specific.
> 
> For present cases, yes, but consider the current setting, which
> disables pmw2 for the FN68PT board, if there was a second chip on that
> board, you would not want the same setting for both chips.
> 

Sorry, that is purely hypothetical. Kernel code should not be optimized
for hypothetical situations.

> I haven't yet worked out how it would be distinguish at the time, but
> also it hasn't been strictly necessary.
> 
> A simple case I have coming up for future patch is to use the DMI table
> to ignore ACPI conflicts when we know it is safe, but that should be
> done on each chip separately, not necessarily globally for all chips on
> that board.  Again, in practice it isn't important, and I haven't
> worked out how to specify it separately yet.
> 
> Also, I have been looking at the difference in the use of
> dmi_check_system() and what the use of dmi_first_match() does and it
> really is just a case of the callback being used at the time of
> matching vs deferring the actions to be performed at later and possibly
> more appropriate stages.
> 

I can see that you are for whatever reason completely opposed to using 
the dmi callback function. Fine, but then don't introduce another one
just to have a callback function and in case it may possibly be needed
sometime in the future. Introduce it if and when it is needed, and only
then. The same applies to all other infrastructure: Introduce it if and
only if it is needed for more than one use case, not because it may be
needed for some unspecified use case some time in the future.

Thanks,
Guenter

Re: [PATCH v2 1/1] hwmon: (it87) Create DMI matching table for various board settings

[Frank Crawford]

On Sun, 2022-10-30 at 20:18 -0700, Guenter Roeck wrote:
> On Sun, Oct 30, 2022 at 01:06:45PM +1100, Frank Crawford wrote:
> 
> 
...
> 
> I can see that you are for whatever reason completely opposed to
> using 
> the dmi callback function. Fine, but then don't introduce another one
> just to have a callback function and in case it may possibly be
> needed
> sometime in the future. Introduce it if and when it is needed, and
> only
> then. The same applies to all other infrastructure: Introduce it if
> and
> only if it is needed for more than one use case, not because it may
> be
> needed for some unspecified use case some time in the future.

No, you get me wrong, I'm not completely opposed, although partly it
was a misunderstanding on my part, as there are no good examples of the
use of callbacks for DMI tables in the codes I looked though.  I had to
go looking into the actual functions to see how they are supposed to be
used.

Also, the most of the items I raise are not future cases for me, as
they are in the out-of-tree version, and I am working on keeping that
roughly in sync with the in-tree version, while I work on trying to
merge most of that functionality in.

Anyway, you should see that I've now submitted a v3 patch, which I
believe does use the callback correctly, and is generally suitable for
handling future updates as well.
> 
> Thanks,
> Guenter

Regards
Frank