[PATCH v1 0/2] hwmon: (it87) Set second Super-IO chip in configuration mode

[PATCH v1 0/2] hwmon: (it87) Set second Super-IO chip in configuration mode

[Frank Crawford]

On various Gigabyte AM4 boards (AB350, AX370), the second Super-IO chip
(IT8792E) needs to be in configuration mode before accessing the first
due to a bug in IT8792E which otherwise results in LPC bus access errors.
This needs to be done before accessing the first Super-IO chip since
the second chip may have been accessed prior to loading this driver.

The problem is also reported to affect IT8795E, which is used on X299 boards
and has the same chip ID as IT8792E (0x8733). It also appears to affect
systems with IT8790E, which is used on some Z97X-Gaming boards as well as
Z87X-OC.

Frank Crawford (2):
  Allow calling __superio_enter outside mux
  Set second Super-IO chip in configuration mode

 drivers/hwmon/it87.c | 71 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 67 insertions(+), 4 deletions(-)

-- 
2.38.1

[PATCH v1 1/2] hwmon: (it87) Allow calling superio_enter outside mux

[Frank Crawford]

Allow for superio_enter to be called outside mux, in particular for
initialisation of the second chipset, which must be entered, but never
exited.

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

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 9997f76b1f4a..4ebce2c661d7 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -87,6 +87,14 @@ static struct platform_device *it87_pdev[2];
 #define	DEVID	0x20	/* Register: Device ID */
 #define	DEVREV	0x22	/* Register: Device Revision */
 
+static inline void __superio_enter(int ioreg)
+{
+	outb(0x87, ioreg);
+	outb(0x01, ioreg);
+	outb(0x55, ioreg);
+	outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg);
+}
+
 static inline int superio_inb(int ioreg, int reg)
 {
 	outb(reg, ioreg);
@@ -124,10 +132,7 @@ static inline int superio_enter(int ioreg)
 	if (!request_muxed_region(ioreg, 2, DRVNAME))
 		return -EBUSY;
 
-	outb(0x87, ioreg);
-	outb(0x01, ioreg);
-	outb(0x55, ioreg);
-	outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg);
+	__superio_enter(ioreg);
 	return 0;
 }
 
-- 
2.38.1

[PATCH v1 2/2] hwmon: (it87) Add entries to dmi_table

[Frank Crawford]

Call initialisation of second chipset.

Update the dmi_table with mother boards that have been tested.

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

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 4ebce2c661d7..2ecfa2c901f6 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -3315,6 +3315,27 @@ static int it87_dmi_cb(const struct dmi_system_id *dmi_entry)
 	return 1;
 }
 
+/*
+ * On various Gigabyte AM4 boards (AB350, AX370), the second Super-IO chip
+ * (IT8792E) needs to be in configuration mode before accessing the first
+ * due to a bug in IT8792E which otherwise results in LPC bus access errors.
+ * This needs to be done before accessing the first Super-IO chip since
+ * the second chip may have been accessed prior to loading this driver.
+ *
+ * The problem is also reported to affect IT8795E, which is used on X299 boards
+ * and has the same chip ID as IT8792E (0x8733). It also appears to affect
+ * systems with IT8790E, which is used on some Z97X-Gaming boards as well as
+ * Z87X-OC.
+ * DMI entries for those systems will be added as they become available and
+ * as the problem is confirmed to affect those boards.
+ */
+static int gigabyte_sio2_force(const struct dmi_system_id *dmi_entry)
+{
+	__superio_enter(REG_4E);
+
+	return it87_dmi_cb(dmi_entry);
+};
+
 /*
  * On the Shuttle SN68PT, FAN_CTL2 is apparently not
  * connected to a fan, but to something else. One user
@@ -3337,7 +3358,44 @@ static struct it87_dmi_data nvidia_fn68pt = {
 		.driver_data = data, \
 	}
 
+#define IT87_DMI_MATCH_GBT(name, cb, data) \
+	IT87_DMI_MATCH_VND("Gigabyte Technology Co., Ltd.", name, cb, data)
+
 static const struct dmi_system_id it87_dmi_table[] __initconst = {
+	IT87_DMI_MATCH_GBT("AB350", gigabyte_sio2_force, NULL),
+		/* ? + IT8792E/IT8795E */
+	IT87_DMI_MATCH_GBT("AX370", gigabyte_sio2_force, NULL),
+		/* ? + IT8792E/IT8795E */
+	IT87_DMI_MATCH_GBT("Z97X-Gaming G1", gigabyte_sio2_force, NULL),
+		/* ? + IT8790E */
+	IT87_DMI_MATCH_GBT("TRX40 AORUS XTREME", gigabyte_sio2_force, NULL),
+		/* IT8688E + IT8792E/IT8795E */
+	IT87_DMI_MATCH_GBT("Z390 AORUS ULTRA-CF", gigabyte_sio2_force, NULL),
+		/* IT8688E + IT8792E/IT8795E */
+	IT87_DMI_MATCH_GBT("Z490 AORUS ELITE AC", it87_dmi_cb, NULL),
+		/* IT8688E */
+	IT87_DMI_MATCH_GBT("B550 AORUS PRO AC", gigabyte_sio2_force, NULL),
+		/* IT8688E + IT8792E/IT8795E */
+	IT87_DMI_MATCH_GBT("B560I AORUS PRO AX", it87_dmi_cb, NULL),
+		/* IT8689E */
+	IT87_DMI_MATCH_GBT("X570 AORUS ELITE WIFI", it87_dmi_cb, NULL),
+		/* IT8688E */
+	IT87_DMI_MATCH_GBT("X570 AORUS MASTER", gigabyte_sio2_force, NULL),
+		/* IT8688E + IT8792E/IT8795E */
+	IT87_DMI_MATCH_GBT("X570 AORUS PRO", gigabyte_sio2_force, NULL),
+		/* IT8688E + IT8792E/IT8795E */
+	IT87_DMI_MATCH_GBT("X570 AORUS PRO WIFI", gigabyte_sio2_force, NULL),
+		/* IT8688E + IT8792E/IT8795E */
+	IT87_DMI_MATCH_GBT("X570 I AORUS PRO WIFI", it87_dmi_cb, NULL),
+		/* IT8688E */
+	IT87_DMI_MATCH_GBT("X570S AERO G", gigabyte_sio2_force, NULL),
+		/* IT8689E + IT87952E */
+	IT87_DMI_MATCH_GBT("X670E AORUS MASTER", it87_dmi_cb, NULL),
+		/* IT8689E - Note there may also be a second chip */
+	IT87_DMI_MATCH_GBT("Z690 AORUS PRO DDR4", gigabyte_sio2_force, NULL),
+		/* IT8689E + IT87952E */
+	IT87_DMI_MATCH_GBT("Z690 AORUS PRO", gigabyte_sio2_force, NULL),
+		/* IT8689E + IT87952E */
 	IT87_DMI_MATCH_VND("nVIDIA", "FN68PT", it87_dmi_cb, &nvidia_fn68pt),
 	{ }
 
-- 
2.38.1

Re: [PATCH v1 1/2] hwmon: (it87) Allow calling superio_enter outside mux

[Guenter Roeck]

On Tue, Jan 03, 2023 at 05:46:11PM +1100, Frank Crawford wrote:
> Allow for superio_enter to be called outside mux, in particular for

"outside mux" is really a bad wording. I had to look into the code
to understand what it means. "without requesting the muxed memory
region", maybe.

Guenter

> initialisation of the second chipset, which must be entered, but never
> exited.

The second chipset is not "entered", it must enter configuration
mode (or be put into configuration mode). The name of the function
does not reflect the associated functionality.

Please rephrase.

Thanks,
Guenter

> 
> Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
> ---
>  drivers/hwmon/it87.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 9997f76b1f4a..4ebce2c661d7 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -87,6 +87,14 @@ static struct platform_device *it87_pdev[2];
>  #define	DEVID	0x20	/* Register: Device ID */
>  #define	DEVREV	0x22	/* Register: Device Revision */
>  
> +static inline void __superio_enter(int ioreg)
> +{
> +	outb(0x87, ioreg);
> +	outb(0x01, ioreg);
> +	outb(0x55, ioreg);
> +	outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg);
> +}
> +
>  static inline int superio_inb(int ioreg, int reg)
>  {
>  	outb(reg, ioreg);
> @@ -124,10 +132,7 @@ static inline int superio_enter(int ioreg)
>  	if (!request_muxed_region(ioreg, 2, DRVNAME))
>  		return -EBUSY;
>  
> -	outb(0x87, ioreg);
> -	outb(0x01, ioreg);
> -	outb(0x55, ioreg);
> -	outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg);
> +	__superio_enter(ioreg);
>  	return 0;
>  }
>  
> -- 
> 2.38.1
> 

Re: [PATCH v1 2/2] hwmon: (it87) Add entries to dmi_table

[Guenter Roeck]

On Tue, Jan 03, 2023 at 05:46:12PM +1100, Frank Crawford wrote:
> Call initialisation of second chipset.
> 
> Update the dmi_table with mother boards that have been tested.
> 

That doesn't really describe what is done and why. See
Documentation/process/submitting-patches.rst, "Describe your changes".
The comment iin the code is much better. 

> Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
> ---
>  drivers/hwmon/it87.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 4ebce2c661d7..2ecfa2c901f6 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -3315,6 +3315,27 @@ static int it87_dmi_cb(const struct dmi_system_id *dmi_entry)
>  	return 1;
>  }
>  
> +/*
> + * On various Gigabyte AM4 boards (AB350, AX370), the second Super-IO chip
> + * (IT8792E) needs to be in configuration mode before accessing the first
> + * due to a bug in IT8792E which otherwise results in LPC bus access errors.
> + * This needs to be done before accessing the first Super-IO chip since
> + * the second chip may have been accessed prior to loading this driver.
> + *
> + * The problem is also reported to affect IT8795E, which is used on X299 boards
> + * and has the same chip ID as IT8792E (0x8733). It also appears to affect
> + * systems with IT8790E, which is used on some Z97X-Gaming boards as well as
> + * Z87X-OC.
> + * DMI entries for those systems will be added as they become available and
> + * as the problem is confirmed to affect those boards.
> + */
> +static int gigabyte_sio2_force(const struct dmi_system_id *dmi_entry)

s/gigabyte/it87/

While this is seen on Gigabyte boards, it doesn't _have_ to be Gigabyte
specific. Other boards i using the same chips (it there are any) may be
affected as well.

> +{
> +	__superio_enter(REG_4E);
> +
> +	return it87_dmi_cb(dmi_entry);
> +};
> +
>  /*
>   * On the Shuttle SN68PT, FAN_CTL2 is apparently not
>   * connected to a fan, but to something else. One user
> @@ -3337,7 +3358,44 @@ static struct it87_dmi_data nvidia_fn68pt = {
>  		.driver_data = data, \
>  	}
>  
> +#define IT87_DMI_MATCH_GBT(name, cb, data) \
> +	IT87_DMI_MATCH_VND("Gigabyte Technology Co., Ltd.", name, cb, data)
> +
>  static const struct dmi_system_id it87_dmi_table[] __initconst = {
> +	IT87_DMI_MATCH_GBT("AB350", gigabyte_sio2_force, NULL),
> +		/* ? + IT8792E/IT8795E */
> +	IT87_DMI_MATCH_GBT("AX370", gigabyte_sio2_force, NULL),
> +		/* ? + IT8792E/IT8795E */
> +	IT87_DMI_MATCH_GBT("Z97X-Gaming G1", gigabyte_sio2_force, NULL),
> +		/* ? + IT8790E */
> +	IT87_DMI_MATCH_GBT("TRX40 AORUS XTREME", gigabyte_sio2_force, NULL),
> +		/* IT8688E + IT8792E/IT8795E */
> +	IT87_DMI_MATCH_GBT("Z390 AORUS ULTRA-CF", gigabyte_sio2_force, NULL),
> +		/* IT8688E + IT8792E/IT8795E */
> +	IT87_DMI_MATCH_GBT("Z490 AORUS ELITE AC", it87_dmi_cb, NULL),
> +		/* IT8688E */

Why list boards with only a single chip ?

> +	IT87_DMI_MATCH_GBT("B550 AORUS PRO AC", gigabyte_sio2_force, NULL),
> +		/* IT8688E + IT8792E/IT8795E */
> +	IT87_DMI_MATCH_GBT("B560I AORUS PRO AX", it87_dmi_cb, NULL),
> +		/* IT8689E */
> +	IT87_DMI_MATCH_GBT("X570 AORUS ELITE WIFI", it87_dmi_cb, NULL),
> +		/* IT8688E */
> +	IT87_DMI_MATCH_GBT("X570 AORUS MASTER", gigabyte_sio2_force, NULL),
> +		/* IT8688E + IT8792E/IT8795E */
> +	IT87_DMI_MATCH_GBT("X570 AORUS PRO", gigabyte_sio2_force, NULL),
> +		/* IT8688E + IT8792E/IT8795E */
> +	IT87_DMI_MATCH_GBT("X570 AORUS PRO WIFI", gigabyte_sio2_force, NULL),
> +		/* IT8688E + IT8792E/IT8795E */
> +	IT87_DMI_MATCH_GBT("X570 I AORUS PRO WIFI", it87_dmi_cb, NULL),
> +		/* IT8688E */
> +	IT87_DMI_MATCH_GBT("X570S AERO G", gigabyte_sio2_force, NULL),
> +		/* IT8689E + IT87952E */

IT87952E ? Is that chip known to be affected as well ?

> +	IT87_DMI_MATCH_GBT("X670E AORUS MASTER", it87_dmi_cb, NULL),
> +		/* IT8689E - Note there may also be a second chip */

Don't add an entry unless known that it is needed.

> +	IT87_DMI_MATCH_GBT("Z690 AORUS PRO DDR4", gigabyte_sio2_force, NULL),
> +		/* IT8689E + IT87952E */
> +	IT87_DMI_MATCH_GBT("Z690 AORUS PRO", gigabyte_sio2_force, NULL),
> +		/* IT8689E + IT87952E */
>  	IT87_DMI_MATCH_VND("nVIDIA", "FN68PT", it87_dmi_cb, &nvidia_fn68pt),
>  	{ }
>  
> -- 
> 2.38.1
> 

Re: [PATCH v1 1/2] hwmon: (it87) Allow calling superio_enter outside mux

[Frank Crawford]

On Tue, 2023-01-03 at 10:37 -0800, Guenter Roeck wrote:
> On Tue, Jan 03, 2023 at 05:46:11PM +1100, Frank Crawford wrote:
> > Allow for superio_enter to be called outside mux, in particular for
> 
> "outside mux" is really a bad wording. I had to look into the code
> to understand what it means. "without requesting the muxed memory
> region", maybe.
> 
> Guenter

Will update with better wording, such as you suggest.
> 
> > initialisation of the second chipset, which must be entered, but
> > never
> > exited.
> 
> The second chipset is not "entered", it must enter configuration
> mode (or be put into configuration mode). The name of the function
> does not reflect the associated functionality.
> 
> Please rephrase.

Will do.

Do you believe the function should be renamed as well?
> 
> Thanks,
> Guenter

Regards
Frank
> 
> > 
> > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
> > ---
> >  drivers/hwmon/it87.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > index 9997f76b1f4a..4ebce2c661d7 100644
> > --- a/drivers/hwmon/it87.c
> > +++ b/drivers/hwmon/it87.c
> > @@ -87,6 +87,14 @@ static struct platform_device *it87_pdev[2];
> >  #define        DEVID   0x20    /* Register: Device ID */
> >  #define        DEVREV  0x22    /* Register: Device Revision */
> >  
> > +static inline void __superio_enter(int ioreg)
> > +{
> > +       outb(0x87, ioreg);
> > +       outb(0x01, ioreg);
> > +       outb(0x55, ioreg);
> > +       outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg);
> > +}
> > +
> >  static inline int superio_inb(int ioreg, int reg)
> >  {
> >         outb(reg, ioreg);
> > @@ -124,10 +132,7 @@ static inline int superio_enter(int ioreg)
> >         if (!request_muxed_region(ioreg, 2, DRVNAME))
> >                 return -EBUSY;
> >  
> > -       outb(0x87, ioreg);
> > -       outb(0x01, ioreg);
> > -       outb(0x55, ioreg);
> > -       outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg);
> > +       __superio_enter(ioreg);
> >         return 0;
> >  }
> >  
> > -- 
> > 2.38.1
> > 

Re: [PATCH v1 2/2] hwmon: (it87) Add entries to dmi_table

[Frank Crawford]

On Tue, 2023-01-03 at 10:46 -0800, Guenter Roeck wrote:
> On Tue, Jan 03, 2023 at 05:46:12PM +1100, Frank Crawford wrote:
> > Call initialisation of second chipset.
> > 
> > Update the dmi_table with mother boards that have been tested.
> > 
> 
> That doesn't really describe what is done and why. See
> Documentation/process/submitting-patches.rst, "Describe your
> changes".
> The comment iin the code is much better. 

Will fix up the descriptions.

> > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
> > ---
> >  drivers/hwmon/it87.c | 58
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> > 
> > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > index 4ebce2c661d7..2ecfa2c901f6 100644
> > --- a/drivers/hwmon/it87.c
> > +++ b/drivers/hwmon/it87.c
> > @@ -3315,6 +3315,27 @@ static int it87_dmi_cb(const struct
> > dmi_system_id *dmi_entry)
> >         return 1;
> >  }
> >  
> > +/*
> > + * On various Gigabyte AM4 boards (AB350, AX370), the second
> > Super-IO chip
> > + * (IT8792E) needs to be in configuration mode before accessing
> > the first
> > + * due to a bug in IT8792E which otherwise results in LPC bus
> > access errors.
> > + * This needs to be done before accessing the first Super-IO chip
> > since
> > + * the second chip may have been accessed prior to loading this
> > driver.
> > + *
> > + * The problem is also reported to affect IT8795E, which is used
> > on X299 boards
> > + * and has the same chip ID as IT8792E (0x8733). It also appears
> > to affect
> > + * systems with IT8790E, which is used on some Z97X-Gaming boards
> > as well as
> > + * Z87X-OC.
> > + * DMI entries for those systems will be added as they become
> > available and
> > + * as the problem is confirmed to affect those boards.
> > + */
> > +static int gigabyte_sio2_force(const struct dmi_system_id
> > *dmi_entry)
> 
> s/gigabyte/it87/
> 
> While this is seen on Gigabyte boards, it doesn't _have_ to be
> Gigabyte
> specific. Other boards i using the same chips (it there are any) may
> be
> affected as well.

Will update it.
> 
> > +{
> > +       __superio_enter(REG_4E);
> > +
> > +       return it87_dmi_cb(dmi_entry);
> > +};
> > +
> >  /*
> >   * On the Shuttle SN68PT, FAN_CTL2 is apparently not
> >   * connected to a fan, but to something else. One user
> > @@ -3337,7 +3358,44 @@ static struct it87_dmi_data nvidia_fn68pt =
> > {
> >                 .driver_data = data, \
> >         }
> >  
> > +#define IT87_DMI_MATCH_GBT(name, cb, data) \
> > +       IT87_DMI_MATCH_VND("Gigabyte Technology Co., Ltd.", name,
> > cb, data)
> > +
> >  static const struct dmi_system_id it87_dmi_table[] __initconst = {
> > +       IT87_DMI_MATCH_GBT("AB350", gigabyte_sio2_force, NULL),
> > +               /* ? + IT8792E/IT8795E */
> > +       IT87_DMI_MATCH_GBT("AX370", gigabyte_sio2_force, NULL),
> > +               /* ? + IT8792E/IT8795E */
> > +       IT87_DMI_MATCH_GBT("Z97X-Gaming G1", gigabyte_sio2_force,
> > NULL),
> > +               /* ? + IT8790E */
> > +       IT87_DMI_MATCH_GBT("TRX40 AORUS XTREME",
> > gigabyte_sio2_force, NULL),
> > +               /* IT8688E + IT8792E/IT8795E */
> > +       IT87_DMI_MATCH_GBT("Z390 AORUS ULTRA-CF",
> > gigabyte_sio2_force, NULL),
> > +               /* IT8688E + IT8792E/IT8795E */
> > +       IT87_DMI_MATCH_GBT("Z490 AORUS ELITE AC", it87_dmi_cb,
> > NULL),
> > +               /* IT8688E */
> 
> Why list boards with only a single chip ?

Because I'm stupid and just copied and pasted from my other code.

I'll drop out the single chips and resubmit.
> 
> > +       IT87_DMI_MATCH_GBT("B550 AORUS PRO AC",
> > gigabyte_sio2_force, NULL),
> > +               /* IT8688E + IT8792E/IT8795E */
> > +       IT87_DMI_MATCH_GBT("B560I AORUS PRO AX", it87_dmi_cb,
> > NULL),
> > +               /* IT8689E */
> > +       IT87_DMI_MATCH_GBT("X570 AORUS ELITE WIFI", it87_dmi_cb,
> > NULL),
> > +               /* IT8688E */
> > +       IT87_DMI_MATCH_GBT("X570 AORUS MASTER",
> > gigabyte_sio2_force, NULL),
> > +               /* IT8688E + IT8792E/IT8795E */
> > +       IT87_DMI_MATCH_GBT("X570 AORUS PRO", gigabyte_sio2_force,
> > NULL),
> > +               /* IT8688E + IT8792E/IT8795E */
> > +       IT87_DMI_MATCH_GBT("X570 AORUS PRO WIFI",
> > gigabyte_sio2_force, NULL),
> > +               /* IT8688E + IT8792E/IT8795E */
> > +       IT87_DMI_MATCH_GBT("X570 I AORUS PRO WIFI", it87_dmi_cb,
> > NULL),
> > +               /* IT8688E */
> > +       IT87_DMI_MATCH_GBT("X570S AERO G", gigabyte_sio2_force,
> > NULL),
> > +               /* IT8689E + IT87952E */
> 
> IT87952E ? Is that chip known to be affected as well ?
> 
> > +       IT87_DMI_MATCH_GBT("X670E AORUS MASTER", it87_dmi_cb,
> > NULL),
> > +               /* IT8689E - Note there may also be a second chip
> > */
> 
> Don't add an entry unless known that it is needed.

That is more a note for future testing, but for now will drop this
board as it is currently treated as a single chip board.
> 
> > +       IT87_DMI_MATCH_GBT("Z690 AORUS PRO DDR4",
> > gigabyte_sio2_force, NULL),
> > +               /* IT8689E + IT87952E */
> > +       IT87_DMI_MATCH_GBT("Z690 AORUS PRO", gigabyte_sio2_force,
> > NULL),
> > +               /* IT8689E + IT87952E */
> >         IT87_DMI_MATCH_VND("nVIDIA", "FN68PT", it87_dmi_cb,
> > &nvidia_fn68pt),
> >         { }
> >  
> > -- 
> > 2.38.1

Regards
Frank
> 

Re: [PATCH v1 1/2] hwmon: (it87) Allow calling superio_enter outside mux

[Guenter Roeck]

On Wed, Jan 04, 2023 at 11:37:28AM +1100, Frank Crawford wrote:
> On Tue, 2023-01-03 at 10:37 -0800, Guenter Roeck wrote:
> > On Tue, Jan 03, 2023 at 05:46:11PM +1100, Frank Crawford wrote:
> > > Allow for superio_enter to be called outside mux, in particular for
> > 
> > "outside mux" is really a bad wording. I had to look into the code
> > to understand what it means. "without requesting the muxed memory
> > region", maybe.
> > 
> > Guenter
> 
> Will update with better wording, such as you suggest.
> > 
> > > initialisation of the second chipset, which must be entered, but
> > > never
> > > exited.
> > 
> > The second chipset is not "entered", it must enter configuration
> > mode (or be put into configuration mode). The name of the function
> > does not reflect the associated functionality.
> > 
> > Please rephrase.
> 
> Will do.
> 
> Do you believe the function should be renamed as well?

No. "enter" in the function name is short for "enter configuration mode".
That is good enough for a function name. Description and documentation need
to be more elaborate, though.

Guenter

> > 
> > Thanks,
> > Guenter
> 
> Regards
> Frank
> > 
> > > 
> > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
> > > ---
> > >  drivers/hwmon/it87.c | 13 +++++++++----
> > >  1 file changed, 9 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > > index 9997f76b1f4a..4ebce2c661d7 100644
> > > --- a/drivers/hwmon/it87.c
> > > +++ b/drivers/hwmon/it87.c
> > > @@ -87,6 +87,14 @@ static struct platform_device *it87_pdev[2];
> > >  #define        DEVID   0x20    /* Register: Device ID */
> > >  #define        DEVREV  0x22    /* Register: Device Revision */
> > >  
> > > +static inline void __superio_enter(int ioreg)
> > > +{
> > > +       outb(0x87, ioreg);
> > > +       outb(0x01, ioreg);
> > > +       outb(0x55, ioreg);
> > > +       outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg);
> > > +}
> > > +
> > >  static inline int superio_inb(int ioreg, int reg)
> > >  {
> > >         outb(reg, ioreg);
> > > @@ -124,10 +132,7 @@ static inline int superio_enter(int ioreg)
> > >         if (!request_muxed_region(ioreg, 2, DRVNAME))
> > >                 return -EBUSY;
> > >  
> > > -       outb(0x87, ioreg);
> > > -       outb(0x01, ioreg);
> > > -       outb(0x55, ioreg);
> > > -       outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg);
> > > +       __superio_enter(ioreg);
> > >         return 0;
> > >  }
> > >  
> > > -- 
> > > 2.38.1
> > >