This patch adds support for W83667HG-B to the w83627ehf driver.
Signed-off-by: Guenter Roeck <[email protected]>
---
Key relevant difference between W83667HG and W83667HG-B are the Chip ID as well
as the fan output and step output register addresses. Those differences are
addressed with this patch.
There are other relevant changes in the mapping of input sensors to fan
control (W83667HG datasheet chapter 8.7, W83667HG-B datasheet chapter 8.5).
However, control of those mappings is not implemented in the driver, thus the
respective changes should not have an impact on driver operation.
Changes made in this patch are based on information from datasheets only. The
patch has not yet been tested with real hardware. The patch must be tested with
real hardware before it is integrated, and is thus submitted as RFC.
---
drivers/hwmon/w83627ehf.c | 44 +++++++++++++++++++++++++++++++++++++-------
1 files changed, 37 insertions(+), 7 deletions(-)
diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index 0dcaba9..074e15b 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -39,6 +39,7 @@
w83627dhg 9 5 4 3 0xa020 0xc1 0x5ca3
w83627dhg-p 9 5 4 3 0xb070 0xc1 0x5ca3
w83667hg 9 5 3 3 0xa510 0xc1 0x5ca3
+ w83667hg-b 9 5 3 3 0xb350 0xc1 0x5ca3
*/
#include <linux/module.h>
@@ -55,7 +56,7 @@
#include <linux/io.h>
#include "lm75.h"
-enum kinds { w83627ehf, w83627dhg, w83627dhg_p, w83667hg };
+enum kinds { w83627ehf, w83627dhg, w83627dhg_p, w83667hg, w83667hg_b };
/* used to set data->name = w83627ehf_device_names[data->sio_kind] */
static const char * w83627ehf_device_names[] = {
@@ -63,6 +64,7 @@ static const char * w83627ehf_device_names[] = {
"w83627dhg",
"w83627dhg",
"w83667hg",
+ "w83667hg-b",
};
static unsigned short force_id;
@@ -91,6 +93,7 @@ MODULE_PARM_DESC(force_id, "Override the detected device ID");
#define SIO_W83627DHG_ID 0xa020
#define SIO_W83627DHG_P_ID 0xb070
#define SIO_W83667HG_ID 0xa510
+#define SIO_W83667HG_B_ID 0xb350
#define SIO_ID_MASK 0xFFF0
static inline void
@@ -201,8 +204,17 @@ static const u8 W83627EHF_REG_TOLERANCE[] = { 0x07, 0x07, 0x14, 0x62 };
static const u8 W83627EHF_REG_FAN_START_OUTPUT[] = { 0x0a, 0x0b, 0x16, 0x65 };
static const u8 W83627EHF_REG_FAN_STOP_OUTPUT[] = { 0x08, 0x09, 0x15, 0x64 };
static const u8 W83627EHF_REG_FAN_STOP_TIME[] = { 0x0c, 0x0d, 0x17, 0x66 };
-static const u8 W83627EHF_REG_FAN_MAX_OUTPUT[] = { 0xff, 0x67, 0xff, 0x69 };
-static const u8 W83627EHF_REG_FAN_STEP_OUTPUT[] = { 0xff, 0x68, 0xff, 0x6a };
+
+static const u8 *W83627EHF_REG_FAN_MAX_OUTPUT;
+static const u8 *W83627EHF_REG_FAN_STEP_OUTPUT;
+
+static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_COMMON[]
+ = { 0xff, 0x67, 0xff, 0x69 };
+static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_COMMON[]
+ = { 0xff, 0x68, 0xff, 0x6a };
+
+static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_W83667_B[] = { 0x67, 0x69, 0x6b };
+static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B[] = { 0x68, 0x6a, 0x6c };
/*
* Conversions
@@ -1343,22 +1355,35 @@ static int __devinit w83627ehf_probe(struct platform_device *pdev)
/* 627EHG and 627EHF have 10 voltage inputs; 627DHG and 667HG have 9 */
data->in_num = (sio_data->kind == w83627ehf) ? 10 : 9;
/* 667HG has 3 pwms */
- data->pwm_num = (sio_data->kind == w83667hg) ? 3 : 4;
+ data->pwm_num = (sio_data->kind == w83667hg
+ || sio_data->kind == w83667hg_b) ? 3 : 4;
/* Check temp3 configuration bit for 667HG */
- if (sio_data->kind == w83667hg) {
+ if (sio_data->kind == w83667hg || sio_data->kind == w83667hg_b) {
data->temp3_disable = w83627ehf_read_value(data,
W83627EHF_REG_TEMP_CONFIG[1]) & 0x01;
data->in6_skip = !data->temp3_disable;
}
+ if (sio_data->kind == w83667hg_b) {
+ W83627EHF_REG_FAN_MAX_OUTPUT
+ = W83627EHF_REG_FAN_MAX_OUTPUT_W83667_B;
+ W83627EHF_REG_FAN_STEP_OUTPUT
+ = W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B;
+ } else {
+ W83627EHF_REG_FAN_MAX_OUTPUT
+ = W83627EHF_REG_FAN_MAX_OUTPUT_COMMON;
+ W83627EHF_REG_FAN_STEP_OUTPUT
+ = W83627EHF_REG_FAN_STEP_OUTPUT_COMMON;
+ }
+
/* Initialize the chip */
w83627ehf_init_device(data);
data->vrm = vid_which_vrm();
superio_enter(sio_data->sioreg);
/* Read VID value */
- if (sio_data->kind == w83667hg) {
+ if (sio_data->kind == w83667hg || sio_data->kind == w83667hg_b) {
/* W83667HG has different pins for VID input and output, so
we can get the VID input values directly at logical device D
0xe3. */
@@ -1409,7 +1434,7 @@ static int __devinit w83627ehf_probe(struct platform_device *pdev)
}
/* fan4 and fan5 share some pins with the GPIO and serial flash */
- if (sio_data->kind == w83667hg) {
+ if (sio_data->kind == w83667hg || sio_data->kind == w83667hg_b) {
fan5pin = superio_inb(sio_data->sioreg, 0x27) & 0x20;
fan4pin = superio_inb(sio_data->sioreg, 0x27) & 0x40;
} else {
@@ -1556,6 +1581,7 @@ static int __init w83627ehf_find(int sioaddr, unsigned short *addr,
static const char __initdata sio_name_W83627DHG[] = "W83627DHG";
static const char __initdata sio_name_W83627DHG_P[] = "W83627DHG-P";
static const char __initdata sio_name_W83667HG[] = "W83667HG";
+ static const char __initdata sio_name_W83667HG_B[] = "W83667HG-B";
u16 val;
const char *sio_name;
@@ -1588,6 +1614,10 @@ static int __init w83627ehf_find(int sioaddr, unsigned short *addr,
sio_data->kind = w83667hg;
sio_name = sio_name_W83667HG;
break;
+ case SIO_W83667HG_B_ID:
+ sio_data->kind = w83667hg_b;
+ sio_name = sio_name_W83667HG_B;
+ break;
default:
if (val != 0xffff)
pr_debug(DRVNAME ": unsupported chip ID: 0x%04x\n",
--
1.7.0.87.g0901d
Hi Guenter,
On Thu, 1 Jul 2010 15:02:15 -0700, Guenter Roeck wrote:
> This patch adds support for W83667HG-B to the w83627ehf driver.
>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
>
> Key relevant difference between W83667HG and W83667HG-B are the Chip ID as well
> as the fan output and step output register addresses. Those differences are
> addressed with this patch.
>
> There are other relevant changes in the mapping of input sensors to fan
> control (W83667HG datasheet chapter 8.7, W83667HG-B datasheet chapter 8.5).
> However, control of those mappings is not implemented in the driver, thus the
> respective changes should not have an impact on driver operation.
>
> Changes made in this patch are based on information from datasheets only. The
> patch has not yet been tested with real hardware. The patch must be tested with
> real hardware before it is integrated, and is thus submitted as RFC.
Thanks for doing this. Quick review:
>
> ---
> drivers/hwmon/w83627ehf.c | 44 +++++++++++++++++++++++++++++++++++++-------
> 1 files changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
> index 0dcaba9..074e15b 100644
> --- a/drivers/hwmon/w83627ehf.c
> +++ b/drivers/hwmon/w83627ehf.c
> @@ -39,6 +39,7 @@
> w83627dhg 9 5 4 3 0xa020 0xc1 0x5ca3
> w83627dhg-p 9 5 4 3 0xb070 0xc1 0x5ca3
> w83667hg 9 5 3 3 0xa510 0xc1 0x5ca3
> + w83667hg-b 9 5 3 3 0xb350 0xc1 0x5ca3
> */
>
> #include <linux/module.h>
> @@ -55,7 +56,7 @@
> #include <linux/io.h>
> #include "lm75.h"
>
> -enum kinds { w83627ehf, w83627dhg, w83627dhg_p, w83667hg };
> +enum kinds { w83627ehf, w83627dhg, w83627dhg_p, w83667hg, w83667hg_b };
>
> /* used to set data->name = w83627ehf_device_names[data->sio_kind] */
> static const char * w83627ehf_device_names[] = {
> @@ -63,6 +64,7 @@ static const char * w83627ehf_device_names[] = {
> "w83627dhg",
> "w83627dhg",
> "w83667hg",
> + "w83667hg-b",
Dashes aren't allowed in hwmon device names. For consistency with what
we did for the W83627DHG-Pg, you should simply drop the "-b". It's a small
detail of little interest for the user anyway.
> };
>
> static unsigned short force_id;
> @@ -91,6 +93,7 @@ MODULE_PARM_DESC(force_id, "Override the detected device ID");
> #define SIO_W83627DHG_ID 0xa020
> #define SIO_W83627DHG_P_ID 0xb070
> #define SIO_W83667HG_ID 0xa510
> +#define SIO_W83667HG_B_ID 0xb350
> #define SIO_ID_MASK 0xFFF0
>
> static inline void
> @@ -201,8 +204,17 @@ static const u8 W83627EHF_REG_TOLERANCE[] = { 0x07, 0x07, 0x14, 0x62 };
> static const u8 W83627EHF_REG_FAN_START_OUTPUT[] = { 0x0a, 0x0b, 0x16, 0x65 };
> static const u8 W83627EHF_REG_FAN_STOP_OUTPUT[] = { 0x08, 0x09, 0x15, 0x64 };
> static const u8 W83627EHF_REG_FAN_STOP_TIME[] = { 0x0c, 0x0d, 0x17, 0x66 };
> -static const u8 W83627EHF_REG_FAN_MAX_OUTPUT[] = { 0xff, 0x67, 0xff, 0x69 };
> -static const u8 W83627EHF_REG_FAN_STEP_OUTPUT[] = { 0xff, 0x68, 0xff, 0x6a };
> +
> +static const u8 *W83627EHF_REG_FAN_MAX_OUTPUT;
> +static const u8 *W83627EHF_REG_FAN_STEP_OUTPUT;
> +
> +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_COMMON[]
> + = { 0xff, 0x67, 0xff, 0x69 };
> +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_COMMON[]
> + = { 0xff, 0x68, 0xff, 0x6a };
> +
> +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_W83667_B[] = { 0x67, 0x69, 0x6b };
> +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B[] = { 0x68, 0x6a, 0x6c };
Is it just me or these arrays aren't used anywhere?
I think I would just drop them. The "0xff" are suspicious in the
original arrays, and the size difference between the common and
W83667HG-B cases is tricky. Anyone willing to add support for this
feature will need to read the datasheets anyway, so you don't add any
value by including the register addresses here.
>
> /*
> * Conversions
> @@ -1343,22 +1355,35 @@ static int __devinit w83627ehf_probe(struct platform_device *pdev)
> /* 627EHG and 627EHF have 10 voltage inputs; 627DHG and 667HG have 9 */
> data->in_num = (sio_data->kind == w83627ehf) ? 10 : 9;
> /* 667HG has 3 pwms */
> - data->pwm_num = (sio_data->kind == w83667hg) ? 3 : 4;
> + data->pwm_num = (sio_data->kind == w83667hg
> + || sio_data->kind == w83667hg_b) ? 3 : 4;
>
> /* Check temp3 configuration bit for 667HG */
> - if (sio_data->kind == w83667hg) {
> + if (sio_data->kind == w83667hg || sio_data->kind == w83667hg_b) {
> data->temp3_disable = w83627ehf_read_value(data,
> W83627EHF_REG_TEMP_CONFIG[1]) & 0x01;
> data->in6_skip = !data->temp3_disable;
> }
>
> + if (sio_data->kind == w83667hg_b) {
> + W83627EHF_REG_FAN_MAX_OUTPUT
> + = W83627EHF_REG_FAN_MAX_OUTPUT_W83667_B;
> + W83627EHF_REG_FAN_STEP_OUTPUT
> + = W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B;
> + } else {
> + W83627EHF_REG_FAN_MAX_OUTPUT
> + = W83627EHF_REG_FAN_MAX_OUTPUT_COMMON;
> + W83627EHF_REG_FAN_STEP_OUTPUT
> + = W83627EHF_REG_FAN_STEP_OUTPUT_COMMON;
> + }
That's not correct. It would be valid (although totally unexpected) to
have two different chips on the same system. Thus
W83627EHF_REG_FAN_MAX_OUTPUT and W83627EHF_REG_FAN_STEP_OUTPUT
shouldn't be global pointers but per-device pointers. You can simply
add them to the data structure.
But as said above, the easier is probably to just drop them.
> +
> /* Initialize the chip */
> w83627ehf_init_device(data);
>
> data->vrm = vid_which_vrm();
> superio_enter(sio_data->sioreg);
> /* Read VID value */
> - if (sio_data->kind == w83667hg) {
> + if (sio_data->kind == w83667hg || sio_data->kind == w83667hg_b) {
> /* W83667HG has different pins for VID input and output, so
> we can get the VID input values directly at logical device D
> 0xe3. */
> @@ -1409,7 +1434,7 @@ static int __devinit w83627ehf_probe(struct platform_device *pdev)
> }
>
> /* fan4 and fan5 share some pins with the GPIO and serial flash */
> - if (sio_data->kind == w83667hg) {
> + if (sio_data->kind == w83667hg || sio_data->kind == w83667hg_b) {
> fan5pin = superio_inb(sio_data->sioreg, 0x27) & 0x20;
> fan4pin = superio_inb(sio_data->sioreg, 0x27) & 0x40;
> } else {
> @@ -1556,6 +1581,7 @@ static int __init w83627ehf_find(int sioaddr, unsigned short *addr,
> static const char __initdata sio_name_W83627DHG[] = "W83627DHG";
> static const char __initdata sio_name_W83627DHG_P[] = "W83627DHG-P";
> static const char __initdata sio_name_W83667HG[] = "W83667HG";
> + static const char __initdata sio_name_W83667HG_B[] = "W83667HG-B";
>
> u16 val;
> const char *sio_name;
> @@ -1588,6 +1614,10 @@ static int __init w83627ehf_find(int sioaddr, unsigned short *addr,
> sio_data->kind = w83667hg;
> sio_name = sio_name_W83667HG;
> break;
> + case SIO_W83667HG_B_ID:
> + sio_data->kind = w83667hg_b;
> + sio_name = sio_name_W83667HG_B;
> + break;
> default:
> if (val != 0xffff)
> pr_debug(DRVNAME ": unsupported chip ID: 0x%04x\n",
--
Jean Delvare
On Fri, Jul 02, 2010 at 03:20:11AM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Thu, 1 Jul 2010 15:02:15 -0700, Guenter Roeck wrote:
> > This patch adds support for W83667HG-B to the w83627ehf driver.
> >
> > Signed-off-by: Guenter Roeck <[email protected]>
> > ---
> >
> > Key relevant difference between W83667HG and W83667HG-B are the Chip ID as well
> > as the fan output and step output register addresses. Those differences are
> > addressed with this patch.
> >
> > There are other relevant changes in the mapping of input sensors to fan
> > control (W83667HG datasheet chapter 8.7, W83667HG-B datasheet chapter 8.5).
> > However, control of those mappings is not implemented in the driver, thus the
> > respective changes should not have an impact on driver operation.
> >
> > Changes made in this patch are based on information from datasheets only. The
> > patch has not yet been tested with real hardware. The patch must be tested with
> > real hardware before it is integrated, and is thus submitted as RFC.
>
> Thanks for doing this. Quick review:
>
> >
> > ---
> > drivers/hwmon/w83627ehf.c | 44 +++++++++++++++++++++++++++++++++++++-------
> > 1 files changed, 37 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
> > index 0dcaba9..074e15b 100644
> > --- a/drivers/hwmon/w83627ehf.c
> > +++ b/drivers/hwmon/w83627ehf.c
> > @@ -39,6 +39,7 @@
> > w83627dhg 9 5 4 3 0xa020 0xc1 0x5ca3
> > w83627dhg-p 9 5 4 3 0xb070 0xc1 0x5ca3
> > w83667hg 9 5 3 3 0xa510 0xc1 0x5ca3
> > + w83667hg-b 9 5 3 3 0xb350 0xc1 0x5ca3
> > */
> >
> > #include <linux/module.h>
> > @@ -55,7 +56,7 @@
> > #include <linux/io.h>
> > #include "lm75.h"
> >
> > -enum kinds { w83627ehf, w83627dhg, w83627dhg_p, w83667hg };
> > +enum kinds { w83627ehf, w83627dhg, w83627dhg_p, w83667hg, w83667hg_b };
> >
> > /* used to set data->name = w83627ehf_device_names[data->sio_kind] */
> > static const char * w83627ehf_device_names[] = {
> > @@ -63,6 +64,7 @@ static const char * w83627ehf_device_names[] = {
> > "w83627dhg",
> > "w83627dhg",
> > "w83667hg",
> > + "w83667hg-b",
>
> Dashes aren't allowed in hwmon device names. For consistency with what
> we did for the W83627DHG-Pg, you should simply drop the "-b". It's a small
> detail of little interest for the user anyway.
>
Ok. Actually, since the only real difference was in the registers which
are not not used anywhere, I'll drop the enum type (w83667hg_b) as well.
I'll also add a check for the HG-I.
> > };
> >
> > static unsigned short force_id;
> > @@ -91,6 +93,7 @@ MODULE_PARM_DESC(force_id, "Override the detected device ID");
> > #define SIO_W83627DHG_ID 0xa020
> > #define SIO_W83627DHG_P_ID 0xb070
> > #define SIO_W83667HG_ID 0xa510
> > +#define SIO_W83667HG_B_ID 0xb350
> > #define SIO_ID_MASK 0xFFF0
> >
> > static inline void
> > @@ -201,8 +204,17 @@ static const u8 W83627EHF_REG_TOLERANCE[] = { 0x07, 0x07, 0x14, 0x62 };
> > static const u8 W83627EHF_REG_FAN_START_OUTPUT[] = { 0x0a, 0x0b, 0x16, 0x65 };
> > static const u8 W83627EHF_REG_FAN_STOP_OUTPUT[] = { 0x08, 0x09, 0x15, 0x64 };
> > static const u8 W83627EHF_REG_FAN_STOP_TIME[] = { 0x0c, 0x0d, 0x17, 0x66 };
> > -static const u8 W83627EHF_REG_FAN_MAX_OUTPUT[] = { 0xff, 0x67, 0xff, 0x69 };
> > -static const u8 W83627EHF_REG_FAN_STEP_OUTPUT[] = { 0xff, 0x68, 0xff, 0x6a };
> > +
> > +static const u8 *W83627EHF_REG_FAN_MAX_OUTPUT;
> > +static const u8 *W83627EHF_REG_FAN_STEP_OUTPUT;
> > +
> > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_COMMON[]
> > + = { 0xff, 0x67, 0xff, 0x69 };
> > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_COMMON[]
> > + = { 0xff, 0x68, 0xff, 0x6a };
> > +
> > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_W83667_B[] = { 0x67, 0x69, 0x6b };
> > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B[] = { 0x68, 0x6a, 0x6c };
>
> Is it just me or these arrays aren't used anywhere?
>
Yes, you are right. For some reason I thought they were used.
> I think I would just drop them. The "0xff" are suspicious in the
> original arrays, and the size difference between the common and
> W83667HG-B cases is tricky. Anyone willing to add support for this
> feature will need to read the datasheets anyway, so you don't add any
> value by including the register addresses here.
>
Agreed. I'll drop the arrays.
> >
> > /*
> > * Conversions
> > @@ -1343,22 +1355,35 @@ static int __devinit w83627ehf_probe(struct platform_device *pdev)
> > /* 627EHG and 627EHF have 10 voltage inputs; 627DHG and 667HG have 9 */
> > data->in_num = (sio_data->kind == w83627ehf) ? 10 : 9;
> > /* 667HG has 3 pwms */
> > - data->pwm_num = (sio_data->kind == w83667hg) ? 3 : 4;
> > + data->pwm_num = (sio_data->kind == w83667hg
> > + || sio_data->kind == w83667hg_b) ? 3 : 4;
> >
> > /* Check temp3 configuration bit for 667HG */
> > - if (sio_data->kind == w83667hg) {
> > + if (sio_data->kind == w83667hg || sio_data->kind == w83667hg_b) {
> > data->temp3_disable = w83627ehf_read_value(data,
> > W83627EHF_REG_TEMP_CONFIG[1]) & 0x01;
> > data->in6_skip = !data->temp3_disable;
> > }
> >
> > + if (sio_data->kind == w83667hg_b) {
> > + W83627EHF_REG_FAN_MAX_OUTPUT
> > + = W83627EHF_REG_FAN_MAX_OUTPUT_W83667_B;
> > + W83627EHF_REG_FAN_STEP_OUTPUT
> > + = W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B;
> > + } else {
> > + W83627EHF_REG_FAN_MAX_OUTPUT
> > + = W83627EHF_REG_FAN_MAX_OUTPUT_COMMON;
> > + W83627EHF_REG_FAN_STEP_OUTPUT
> > + = W83627EHF_REG_FAN_STEP_OUTPUT_COMMON;
> > + }
>
> That's not correct. It would be valid (although totally unexpected) to
> have two different chips on the same system. Thus
> W83627EHF_REG_FAN_MAX_OUTPUT and W83627EHF_REG_FAN_STEP_OUTPUT
> shouldn't be global pointers but per-device pointers. You can simply
> add them to the data structure.
>
Basic idea was not to change W83627EHF_REG_FAN_MAX_OUTPUT, but you are right,
it should be per-device pointers (if used).
> But as said above, the easier is probably to just drop them.
>
Yes, that is what I'll do.
Guenter
On Fri, 2 Jul 2010 01:07:04 -0700, Guenter Roeck wrote:
> On Fri, Jul 02, 2010 at 03:20:11AM -0400, Jean Delvare wrote:
> > On Thu, 1 Jul 2010 15:02:15 -0700, Guenter Roeck wrote:
> > > @@ -63,6 +64,7 @@ static const char * w83627ehf_device_names[] = {
> > > "w83627dhg",
> > > "w83627dhg",
> > > "w83667hg",
> > > + "w83667hg-b",
> >
> > Dashes aren't allowed in hwmon device names. For consistency with what
> > we did for the W83627DHG-Pg, you should simply drop the "-b". It's a small
> > detail of little interest for the user anyway.
>
> Ok. Actually, since the only real difference was in the registers which
> are not not used anywhere, I'll drop the enum type (w83667hg_b) as well.
Makes sense. It can be added later if needed.
> I'll also add a check for the HG-I.
Do you have a datasheet for that one too?
--
Jean Delvare
On Fri, Jul 02, 2010 at 03:20:11AM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Thu, 1 Jul 2010 15:02:15 -0700, Guenter Roeck wrote:
> > This patch adds support for W83667HG-B to the w83627ehf driver.
> >
> > Signed-off-by: Guenter Roeck <[email protected]>
> > ---
> >
> > Key relevant difference between W83667HG and W83667HG-B are the Chip ID as well
> > as the fan output and step output register addresses. Those differences are
> > addressed with this patch.
> >
> > There are other relevant changes in the mapping of input sensors to fan
> > control (W83667HG datasheet chapter 8.7, W83667HG-B datasheet chapter 8.5).
> > However, control of those mappings is not implemented in the driver, thus the
> > respective changes should not have an impact on driver operation.
> >
> > Changes made in this patch are based on information from datasheets only. The
> > patch has not yet been tested with real hardware. The patch must be tested with
> > real hardware before it is integrated, and is thus submitted as RFC.
>
> Thanks for doing this. Quick review:
>
> >
> > ---
> > drivers/hwmon/w83627ehf.c | 44 +++++++++++++++++++++++++++++++++++++-------
> > 1 files changed, 37 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
> > index 0dcaba9..074e15b 100644
> > --- a/drivers/hwmon/w83627ehf.c
> > +++ b/drivers/hwmon/w83627ehf.c
> > @@ -39,6 +39,7 @@
> > w83627dhg 9 5 4 3 0xa020 0xc1 0x5ca3
> > w83627dhg-p 9 5 4 3 0xb070 0xc1 0x5ca3
> > w83667hg 9 5 3 3 0xa510 0xc1 0x5ca3
> > + w83667hg-b 9 5 3 3 0xb350 0xc1 0x5ca3
> > */
> >
> > #include <linux/module.h>
> > @@ -55,7 +56,7 @@
> > #include <linux/io.h>
> > #include "lm75.h"
> >
> > -enum kinds { w83627ehf, w83627dhg, w83627dhg_p, w83667hg };
> > +enum kinds { w83627ehf, w83627dhg, w83627dhg_p, w83667hg, w83667hg_b };
> >
> > /* used to set data->name = w83627ehf_device_names[data->sio_kind] */
> > static const char * w83627ehf_device_names[] = {
> > @@ -63,6 +64,7 @@ static const char * w83627ehf_device_names[] = {
> > "w83627dhg",
> > "w83627dhg",
> > "w83667hg",
> > + "w83667hg-b",
>
> Dashes aren't allowed in hwmon device names. For consistency with what
> we did for the W83627DHG-Pg, you should simply drop the "-b". It's a small
> detail of little interest for the user anyway.
>
> > };
> >
> > static unsigned short force_id;
> > @@ -91,6 +93,7 @@ MODULE_PARM_DESC(force_id, "Override the detected device ID");
> > #define SIO_W83627DHG_ID 0xa020
> > #define SIO_W83627DHG_P_ID 0xb070
> > #define SIO_W83667HG_ID 0xa510
> > +#define SIO_W83667HG_B_ID 0xb350
> > #define SIO_ID_MASK 0xFFF0
> >
> > static inline void
> > @@ -201,8 +204,17 @@ static const u8 W83627EHF_REG_TOLERANCE[] = { 0x07, 0x07, 0x14, 0x62 };
> > static const u8 W83627EHF_REG_FAN_START_OUTPUT[] = { 0x0a, 0x0b, 0x16, 0x65 };
> > static const u8 W83627EHF_REG_FAN_STOP_OUTPUT[] = { 0x08, 0x09, 0x15, 0x64 };
> > static const u8 W83627EHF_REG_FAN_STOP_TIME[] = { 0x0c, 0x0d, 0x17, 0x66 };
> > -static const u8 W83627EHF_REG_FAN_MAX_OUTPUT[] = { 0xff, 0x67, 0xff, 0x69 };
> > -static const u8 W83627EHF_REG_FAN_STEP_OUTPUT[] = { 0xff, 0x68, 0xff, 0x6a };
> > +
> > +static const u8 *W83627EHF_REG_FAN_MAX_OUTPUT;
> > +static const u8 *W83627EHF_REG_FAN_STEP_OUTPUT;
> > +
> > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_COMMON[]
> > + = { 0xff, 0x67, 0xff, 0x69 };
> > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_COMMON[]
> > + = { 0xff, 0x68, 0xff, 0x6a };
> > +
> > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_W83667_B[] = { 0x67, 0x69, 0x6b };
> > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B[] = { 0x68, 0x6a, 0x6c };
>
> Is it just me or these arrays aren't used anywhere?
>
> I think I would just drop them. The "0xff" are suspicious in the
> original arrays, and the size difference between the common and
> W83667HG-B cases is tricky. Anyone willing to add support for this
> feature will need to read the datasheets anyway, so you don't add any
> value by including the register addresses here.
>
After removing the defines and trying to compile I remembered.
I _knew_ there was a reason for not removing them.
Guess it's too late (or early) here to do serious work.
The defines _are_ used, in:
fan_functions(fan_max_output, FAN_MAX_OUTPUT)
fan_functions(fan_step_output, FAN_STEP_OUTPUT)
which expands to W83627EHF_REG_FAN_MAX_OUTPUT and W83627EHF_REG_FAN_STEP_OUTPUT.
Tricky ... and that was also the reason why I retained the original
global variables.
I'll move the pointers into per-device code as you suggested, but I'll
have to think about how to do that w/o having to change a lot of code.
As for the 0xff - that pretty much applies to all chips supported by this driver.
I guess it is supposed to mean "not supported", and as a result the code will
write to a non-existing register. I don't really want to touch that.
The size difference (3 entries vs. 4) doesn't matter, since the chips are both
configured to have only three pwm fan controllers (even though the W83667HG
is supposed to have four per its datasheet). So the 4th element of the arrays
will not be accessed by the code if W83667HG(-B) is detected.
Guenter
On Fri, Jul 02, 2010 at 04:13:45AM -0400, Jean Delvare wrote:
> On Fri, 2 Jul 2010 01:07:04 -0700, Guenter Roeck wrote:
> > On Fri, Jul 02, 2010 at 03:20:11AM -0400, Jean Delvare wrote:
> > > On Thu, 1 Jul 2010 15:02:15 -0700, Guenter Roeck wrote:
> > > > @@ -63,6 +64,7 @@ static const char * w83627ehf_device_names[] = {
> > > > "w83627dhg",
> > > > "w83627dhg",
> > > > "w83667hg",
> > > > + "w83667hg-b",
> > >
> > > Dashes aren't allowed in hwmon device names. For consistency with what
> > > we did for the W83627DHG-Pg, you should simply drop the "-b". It's a small
> > > detail of little interest for the user anyway.
> >
> > Ok. Actually, since the only real difference was in the registers which
> > are not not used anywhere, I'll drop the enum type (w83667hg_b) as well.
>
> Makes sense. It can be added later if needed.
>
> > I'll also add a check for the HG-I.
>
> Do you have a datasheet for that one too?
>
No. However, since we have a tester, we will have test coverage,
so I figured it should be worth a try. If it doesn't work
I can still drop it from the final patch.
I suspect it is the same as the HG with a different chip ID.
Guenter
Hi Guenter,
On Fri, 2 Jul 2010 01:25:30 -0700, Guenter Roeck wrote:
> On Fri, Jul 02, 2010 at 03:20:11AM -0400, Jean Delvare wrote:
> > On Thu, 1 Jul 2010 15:02:15 -0700, Guenter Roeck wrote:
> > > -static const u8 W83627EHF_REG_FAN_MAX_OUTPUT[] = { 0xff, 0x67, 0xff, 0x69 };
> > > -static const u8 W83627EHF_REG_FAN_STEP_OUTPUT[] = { 0xff, 0x68, 0xff, 0x6a };
> > > +
> > > +static const u8 *W83627EHF_REG_FAN_MAX_OUTPUT;
> > > +static const u8 *W83627EHF_REG_FAN_STEP_OUTPUT;
> > > +
> > > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_COMMON[]
> > > + = { 0xff, 0x67, 0xff, 0x69 };
> > > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_COMMON[]
> > > + = { 0xff, 0x68, 0xff, 0x6a };
> > > +
> > > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_W83667_B[] = { 0x67, 0x69, 0x6b };
> > > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B[] = { 0x68, 0x6a, 0x6c };
> >
> > Is it just me or these arrays aren't used anywhere?
> >
> > I think I would just drop them. The "0xff" are suspicious in the
> > original arrays, and the size difference between the common and
> > W83667HG-B cases is tricky. Anyone willing to add support for this
> > feature will need to read the datasheets anyway, so you don't add any
> > value by including the register addresses here.
>
> After removing the defines and trying to compile I remembered.
> I _knew_ there was a reason for not removing them.
> Guess it's too late (or early) here to do serious work.
>
> The defines _are_ used, in:
>
> fan_functions(fan_max_output, FAN_MAX_OUTPUT)
> fan_functions(fan_step_output, FAN_STEP_OUTPUT)
>
> which expands to W83627EHF_REG_FAN_MAX_OUTPUT and W83627EHF_REG_FAN_STEP_OUTPUT.
>
> Tricky ... and that was also the reason why I retained the original
> global variables.
Tricky indeed. We normally don't accept code like this in the kernel.
> I'll move the pointers into per-device code as you suggested, but I'll
> have to think about how to do that w/o having to change a lot of code.
If code changes are desirable, let's just do them. You can do that in a
preliminary patch, and then your patch adding support for the
W83667HG-B goes on top of it.
> As for the 0xff - that pretty much applies to all chips supported by this driver.
> I guess it is supposed to mean "not supported", and as a result the code will
> write to a non-existing register. I don't really want to touch that.
I want you to touch that. Writing to non-existing registers is a bad
idea. You never know what actually happens when you do that.
> The size difference (3 entries vs. 4) doesn't matter, since the chips are both
> configured to have only three pwm fan controllers (even though the W83667HG
> is supposed to have four per its datasheet). So the 4th element of the arrays
> will not be accessed by the code if W83667HG(-B) is detected.
OK.
--
Jean Delvare
On Fri, 2 Jul 2010 01:31:44 -0700, Guenter Roeck wrote:
> On Fri, Jul 02, 2010 at 04:13:45AM -0400, Jean Delvare wrote:
> > On Fri, 2 Jul 2010 01:07:04 -0700, Guenter Roeck wrote:
> > > I'll also add a check for the HG-I.
> >
> > Do you have a datasheet for that one too?
> >
> No. However, since we have a tester, we will have test coverage,
> so I figured it should be worth a try. If it doesn't work
> I can still drop it from the final patch.
FYI, I have a datasheet, which unfortunately I am not allowed to share.
Please let me know if you want me to look up something for you.
> I suspect it is the same as the HG with a different chip ID.
Beware, it's a different device number (677 vs. 667) so I would expect
more differences.
--
Jean Delvare
On Fri, Jul 02, 2010 at 05:51:23AM -0400, Jean Delvare wrote:
> On Fri, 2 Jul 2010 01:31:44 -0700, Guenter Roeck wrote:
> > On Fri, Jul 02, 2010 at 04:13:45AM -0400, Jean Delvare wrote:
> > > On Fri, 2 Jul 2010 01:07:04 -0700, Guenter Roeck wrote:
> > > > I'll also add a check for the HG-I.
> > >
> > > Do you have a datasheet for that one too?
> > >
> > No. However, since we have a tester, we will have test coverage,
> > so I figured it should be worth a try. If it doesn't work
> > I can still drop it from the final patch.
>
> FYI, I have a datasheet, which unfortunately I am not allowed to share.
> Please let me know if you want me to look up something for you.
>
How about the changed registers ? Would give us an idea if the chip is closer to
667 or to 667-B.
> > I suspect it is the same as the HG with a different chip ID.
>
> Beware, it's a different device number (677 vs. 667) so I would expect
> more differences.
>
Ah, I missed that. I thought it was 667-I. Yes, you are right, that suggests
more differences.
Guenter
> --
> Jean Delvare
On Fri, Jul 02, 2010 at 05:49:49AM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Fri, 2 Jul 2010 01:25:30 -0700, Guenter Roeck wrote:
> > On Fri, Jul 02, 2010 at 03:20:11AM -0400, Jean Delvare wrote:
> > > On Thu, 1 Jul 2010 15:02:15 -0700, Guenter Roeck wrote:
> > > > -static const u8 W83627EHF_REG_FAN_MAX_OUTPUT[] = { 0xff, 0x67, 0xff, 0x69 };
> > > > -static const u8 W83627EHF_REG_FAN_STEP_OUTPUT[] = { 0xff, 0x68, 0xff, 0x6a };
> > > > +
> > > > +static const u8 *W83627EHF_REG_FAN_MAX_OUTPUT;
> > > > +static const u8 *W83627EHF_REG_FAN_STEP_OUTPUT;
> > > > +
> > > > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_COMMON[]
> > > > + = { 0xff, 0x67, 0xff, 0x69 };
> > > > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_COMMON[]
> > > > + = { 0xff, 0x68, 0xff, 0x6a };
> > > > +
> > > > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_W83667_B[] = { 0x67, 0x69, 0x6b };
> > > > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B[] = { 0x68, 0x6a, 0x6c };
> > >
> > > Is it just me or these arrays aren't used anywhere?
> > >
> > > I think I would just drop them. The "0xff" are suspicious in the
> > > original arrays, and the size difference between the common and
> > > W83667HG-B cases is tricky. Anyone willing to add support for this
> > > feature will need to read the datasheets anyway, so you don't add any
> > > value by including the register addresses here.
> >
> > After removing the defines and trying to compile I remembered.
> > I _knew_ there was a reason for not removing them.
> > Guess it's too late (or early) here to do serious work.
> >
> > The defines _are_ used, in:
> >
> > fan_functions(fan_max_output, FAN_MAX_OUTPUT)
> > fan_functions(fan_step_output, FAN_STEP_OUTPUT)
> >
> > which expands to W83627EHF_REG_FAN_MAX_OUTPUT and W83627EHF_REG_FAN_STEP_OUTPUT.
> >
> > Tricky ... and that was also the reason why I retained the original
> > global variables.
>
> Tricky indeed. We normally don't accept code like this in the kernel.
>
> > I'll move the pointers into per-device code as you suggested, but I'll
> > have to think about how to do that w/o having to change a lot of code.
>
> If code changes are desirable, let's just do them. You can do that in a
> preliminary patch, and then your patch adding support for the
> W83667HG-B goes on top of it.
>
Without the support for -B the changes are not really needed, so that patch
would not make much sense without it. Have you looked at v2 of the patch ?
> > As for the 0xff - that pretty much applies to all chips supported by this driver.
> > I guess it is supposed to mean "not supported", and as a result the code will
> > write to a non-existing register. I don't really want to touch that.
>
> I want you to touch that. Writing to non-existing registers is a bad
> idea. You never know what actually happens when you do that.
>
Good point.
Clean fix would be not to provide the unsupported attributes. Simple workaround
would be to return an error if a write is attempted on a non-supported attribute.
I am sure it would be better to not provide the attribute, but would you accept
the workaround ?
> > The size difference (3 entries vs. 4) doesn't matter, since the chips are both
> > configured to have only three pwm fan controllers (even though the W83667HG
> > is supposed to have four per its datasheet). So the 4th element of the arrays
> > will not be accessed by the code if W83667HG(-B) is detected.
>
> OK.
>
On a side note, any idea why the 4th pwm is disabled for the W83667HG ?
Guenter
On Fri, 2 Jul 2010 07:09:44 -0700, Guenter Roeck wrote:
> On Fri, Jul 02, 2010 at 05:51:23AM -0400, Jean Delvare wrote:
> > On Fri, 2 Jul 2010 01:31:44 -0700, Guenter Roeck wrote:
> > > On Fri, Jul 02, 2010 at 04:13:45AM -0400, Jean Delvare wrote:
> > > > On Fri, 2 Jul 2010 01:07:04 -0700, Guenter Roeck wrote:
> > > > > I'll also add a check for the HG-I.
> > > >
> > > > Do you have a datasheet for that one too?
> > > >
> > > No. However, since we have a tester, we will have test coverage,
> > > so I figured it should be worth a try. If it doesn't work
> > > I can still drop it from the final patch.
> >
> > FYI, I have a datasheet, which unfortunately I am not allowed to share.
> > Please let me know if you want me to look up something for you.
> >
> How about the changed registers ? Would give us an idea if the chip is closer to
> 667 or to 667-B.
There is no summary of this available. The only way is to go through
both datasheets and compare all registers in sequence. This takes time,
which is exactly why I couldn't find the time to do it :(
> > > I suspect it is the same as the HG with a different chip ID.
> >
> > Beware, it's a different device number (677 vs. 667) so I would expect
> > more differences.
>
> Ah, I missed that. I thought it was 667-I. Yes, you are right, that suggests
> more differences.
--
Jean Delvare
On Fri, Jul 02, 2010 at 10:59:03AM -0400, Jean Delvare wrote:
> On Fri, 2 Jul 2010 07:09:44 -0700, Guenter Roeck wrote:
> > On Fri, Jul 02, 2010 at 05:51:23AM -0400, Jean Delvare wrote:
> > > On Fri, 2 Jul 2010 01:31:44 -0700, Guenter Roeck wrote:
> > > > On Fri, Jul 02, 2010 at 04:13:45AM -0400, Jean Delvare wrote:
> > > > > On Fri, 2 Jul 2010 01:07:04 -0700, Guenter Roeck wrote:
> > > > > > I'll also add a check for the HG-I.
> > > > >
> > > > > Do you have a datasheet for that one too?
> > > > >
> > > > No. However, since we have a tester, we will have test coverage,
> > > > so I figured it should be worth a try. If it doesn't work
> > > > I can still drop it from the final patch.
> > >
> > > FYI, I have a datasheet, which unfortunately I am not allowed to share.
> > > Please let me know if you want me to look up something for you.
> > >
> > How about the changed registers ? Would give us an idea if the chip is closer to
> > 667 or to 667-B.
>
> There is no summary of this available. The only way is to go through
> both datasheets and compare all registers in sequence. This takes time,
> which is exactly why I couldn't find the time to do it :(
>
Yes, I noticed. Changes can be quite subtle, too.
I'll remove the HG-I chip detection from the next patch rev
unless I manage to get a datasheet as well.
Guenter
On Fri, 2 Jul 2010 09:15:21 -0700, Guenter Roeck wrote:
> On Fri, Jul 02, 2010 at 10:59:03AM -0400, Jean Delvare wrote:
> > On Fri, 2 Jul 2010 07:09:44 -0700, Guenter Roeck wrote:
> > > On Fri, Jul 02, 2010 at 05:51:23AM -0400, Jean Delvare wrote:
> > > > FYI, I have a datasheet, which unfortunately I am not allowed to share.
> > > > Please let me know if you want me to look up something for you.
> > > >
> > > How about the changed registers ? Would give us an idea if the chip is closer to
> > > 667 or to 667-B.
> >
> > There is no summary of this available. The only way is to go through
> > both datasheets and compare all registers in sequence. This takes time,
> > which is exactly why I couldn't find the time to do it :(
> >
> Yes, I noticed. Changes can be quite subtle, too.
>
> I'll remove the HG-I chip detection from the next patch rev
> unless I manage to get a datasheet as well.
I agree. The 667HG-B support seems to be easier to get, and more urgent
too, judging by the number of requests. Let's get this tested and
upstream first, and then we see what can be done for the 677HG-I.
--
Jean Delvare
Hi Guenter,
On Fri, 2 Jul 2010 07:54:04 -0700, Guenter Roeck wrote:
> On Fri, Jul 02, 2010 at 05:49:49AM -0400, Jean Delvare wrote:
> > On Fri, 2 Jul 2010 01:25:30 -0700, Guenter Roeck wrote:
> > > After removing the defines and trying to compile I remembered.
> > > I _knew_ there was a reason for not removing them.
> > > Guess it's too late (or early) here to do serious work.
> > >
> > > The defines _are_ used, in:
> > >
> > > fan_functions(fan_max_output, FAN_MAX_OUTPUT)
> > > fan_functions(fan_step_output, FAN_STEP_OUTPUT)
> > >
> > > which expands to W83627EHF_REG_FAN_MAX_OUTPUT and W83627EHF_REG_FAN_STEP_OUTPUT.
> > >
> > > Tricky ... and that was also the reason why I retained the original
> > > global variables.
> >
> > Tricky indeed. We normally don't accept code like this in the kernel.
> >
> > > I'll move the pointers into per-device code as you suggested, but I'll
> > > have to think about how to do that w/o having to change a lot of code.
> >
> > If code changes are desirable, let's just do them. You can do that in a
> > preliminary patch, and then your patch adding support for the
> > W83667HG-B goes on top of it.
> >
> Without the support for -B the changes are not really needed, so that patch
> would not make much sense without it.
It is still a good practice to split patches in this case. This makes
review and bisection easier. Simply, we don't get to apply the first
patch if we don't also apply the second.
> Have you looked at v2 of the patch ?
Not yet, will do now.
> > > As for the 0xff - that pretty much applies to all chips supported by this driver.
> > > I guess it is supposed to mean "not supported", and as a result the code will
> > > write to a non-existing register. I don't really want to touch that.
> >
> > I want you to touch that. Writing to non-existing registers is a bad
> > idea. You never know what actually happens when you do that.
>
> Good point.
BTW, as I wasn't clear enough about this originally: I didn't mean this
to fix to be included into your W83667HG-B support patch. It should be
a separate patch, so that it can be easily backported.
> Clean fix would be not to provide the unsupported attributes. Simple workaround
> would be to return an error if a write is attempted on a non-supported attribute.
> I am sure it would be better to not provide the attribute, but would you accept
> the workaround ?
I will accept whatever you are willing to provide. As you're the one
spending the time, I'm not the one deciding how much you spend.
> > > The size difference (3 entries vs. 4) doesn't matter, since the chips are both
> > > configured to have only three pwm fan controllers (even though the W83667HG
> > > is supposed to have four per its datasheet). So the 4th element of the arrays
> > > will not be accessed by the code if W83667HG(-B) is detected.
> >
> > OK.
>
> On a side note, any idea why the 4th pwm is disabled for the W83667HG ?
The W83667HG chip has only 3 FANOUT pins (98, 125 and 127), so it is
expected that the driver exposes only 3 pwm outputs for this chip. I
don't understand why you think it is a problem?
Pin 98 can be used for an alternative function, so the corresponding
pwm output should not even be unconditionally instantiated.
Now the chip is quite complex in that it apparently has 4 fan control
units, and which one controls which fan output can be decided by
register values. I have no idea why they made it so complex at the
hardware level, but this certainly explains why the driver is a little
messy in this respect. We might have to move 667HG and later support to
a separate driver at some point, I don't know.
Now I'm sure you understand why I never took the time to look into
adding W83667HG-B support ;)
--
Jean Delvare
On Sat, Jul 03, 2010 at 04:09:10AM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Fri, 2 Jul 2010 07:54:04 -0700, Guenter Roeck wrote:
> > On Fri, Jul 02, 2010 at 05:49:49AM -0400, Jean Delvare wrote:
> > > On Fri, 2 Jul 2010 01:25:30 -0700, Guenter Roeck wrote:
> > > > After removing the defines and trying to compile I remembered.
> > > > I _knew_ there was a reason for not removing them.
> > > > Guess it's too late (or early) here to do serious work.
> > > >
> > > > The defines _are_ used, in:
> > > >
> > > > fan_functions(fan_max_output, FAN_MAX_OUTPUT)
> > > > fan_functions(fan_step_output, FAN_STEP_OUTPUT)
> > > >
> > > > which expands to W83627EHF_REG_FAN_MAX_OUTPUT and W83627EHF_REG_FAN_STEP_OUTPUT.
> > > >
> > > > Tricky ... and that was also the reason why I retained the original
> > > > global variables.
> > >
> > > Tricky indeed. We normally don't accept code like this in the kernel.
> > >
> > > > I'll move the pointers into per-device code as you suggested, but I'll
> > > > have to think about how to do that w/o having to change a lot of code.
> > >
> > > If code changes are desirable, let's just do them. You can do that in a
> > > preliminary patch, and then your patch adding support for the
> > > W83667HG-B goes on top of it.
> > >
> > Without the support for -B the changes are not really needed, so that patch
> > would not make much sense without it.
>
> It is still a good practice to split patches in this case. This makes
> review and bisection easier. Simply, we don't get to apply the first
> patch if we don't also apply the second.
>
> > Have you looked at v2 of the patch ?
>
> Not yet, will do now.
>
> > > > As for the 0xff - that pretty much applies to all chips supported by this driver.
> > > > I guess it is supposed to mean "not supported", and as a result the code will
> > > > write to a non-existing register. I don't really want to touch that.
> > >
> > > I want you to touch that. Writing to non-existing registers is a bad
> > > idea. You never know what actually happens when you do that.
> >
> > Good point.
>
> BTW, as I wasn't clear enough about this originally: I didn't mean this
> to fix to be included into your W83667HG-B support patch. It should be
> a separate patch, so that it can be easily backported.
>
Actually, turns out the 0xff register values are not a problem. sysfs attribute files
for max and step registers are only created for fan 2 and 4, but not for fan 1 and 3.
Thus, the 0xff array entries will not be used. I'll have to change that for rev -B,
because those registers _do_ exist there, and the files should exist.
> > Clean fix would be not to provide the unsupported attributes. Simple workaround
> > would be to return an error if a write is attempted on a non-supported attribute.
> > I am sure it would be better to not provide the attribute, but would you accept
> > the workaround ?
>
> I will accept whatever you are willing to provide. As you're the one
> spending the time, I'm not the one deciding how much you spend.
>
> > > > The size difference (3 entries vs. 4) doesn't matter, since the chips are both
> > > > configured to have only three pwm fan controllers (even though the W83667HG
> > > > is supposed to have four per its datasheet). So the 4th element of the arrays
> > > > will not be accessed by the code if W83667HG(-B) is detected.
> > >
> > > OK.
> >
> > On a side note, any idea why the 4th pwm is disabled for the W83667HG ?
>
> The W83667HG chip has only 3 FANOUT pins (98, 125 and 127), so it is
> expected that the driver exposes only 3 pwm outputs for this chip. I
> don't understand why you think it is a problem?
>
Not a problem. Just wondering.
Guenter
> Pin 98 can be used for an alternative function, so the corresponding
> pwm output should not even be unconditionally instantiated.
>
> Now the chip is quite complex in that it apparently has 4 fan control
> units, and which one controls which fan output can be decided by
> register values. I have no idea why they made it so complex at the
> hardware level, but this certainly explains why the driver is a little
> messy in this respect. We might have to move 667HG and later support to
> a separate driver at some point, I don't know.
>
> Now I'm sure you understand why I never took the time to look into
> adding W83667HG-B support ;)
>