[lm-sensors] [PATCH 1/3] hwmon: (it87) Create voltage attributes only if voltage is enabled

[lm-sensors] [PATCH 1/3] hwmon: (it87) Create voltage attributes only if voltage is enabled

[Guenter Roeck]

On IT8782F and IT8783F, some voltage input pins may be disabled. Don't create
sysfs attribute files if that is the case.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/it87.c |  146 ++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 113 insertions(+), 33 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index aebac13..baf4173 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -234,6 +234,7 @@ struct it87_sio_data {
 	u8 beep_pin;
 	u8 internal;	/* Internal sensors can be labeled */
 	/* Features skipped based on config or DMI */
+	u16 skip_in;
 	u8 skip_vid;
 	u8 skip_fan;
 	u8 skip_pwm;
@@ -1371,41 +1372,73 @@ static ssize_t show_name(struct device *dev, struct device_attribute
 }
 static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
 
-static struct attribute *it87_attributes[] = {
+static struct attribute *it87_attributes_in[9][5] = {
+{
 	&sensor_dev_attr_in0_input.dev_attr.attr,
-	&sensor_dev_attr_in1_input.dev_attr.attr,
-	&sensor_dev_attr_in2_input.dev_attr.attr,
-	&sensor_dev_attr_in3_input.dev_attr.attr,
-	&sensor_dev_attr_in4_input.dev_attr.attr,
-	&sensor_dev_attr_in5_input.dev_attr.attr,
-	&sensor_dev_attr_in6_input.dev_attr.attr,
-	&sensor_dev_attr_in7_input.dev_attr.attr,
-	&sensor_dev_attr_in8_input.dev_attr.attr,
 	&sensor_dev_attr_in0_min.dev_attr.attr,
-	&sensor_dev_attr_in1_min.dev_attr.attr,
-	&sensor_dev_attr_in2_min.dev_attr.attr,
-	&sensor_dev_attr_in3_min.dev_attr.attr,
-	&sensor_dev_attr_in4_min.dev_attr.attr,
-	&sensor_dev_attr_in5_min.dev_attr.attr,
-	&sensor_dev_attr_in6_min.dev_attr.attr,
-	&sensor_dev_attr_in7_min.dev_attr.attr,
 	&sensor_dev_attr_in0_max.dev_attr.attr,
-	&sensor_dev_attr_in1_max.dev_attr.attr,
-	&sensor_dev_attr_in2_max.dev_attr.attr,
-	&sensor_dev_attr_in3_max.dev_attr.attr,
-	&sensor_dev_attr_in4_max.dev_attr.attr,
-	&sensor_dev_attr_in5_max.dev_attr.attr,
-	&sensor_dev_attr_in6_max.dev_attr.attr,
-	&sensor_dev_attr_in7_max.dev_attr.attr,
 	&sensor_dev_attr_in0_alarm.dev_attr.attr,
+	NULL
+}, {
+	&sensor_dev_attr_in1_input.dev_attr.attr,
+	&sensor_dev_attr_in1_min.dev_attr.attr,
+	&sensor_dev_attr_in1_max.dev_attr.attr,
 	&sensor_dev_attr_in1_alarm.dev_attr.attr,
+	NULL
+}, {
+	&sensor_dev_attr_in2_input.dev_attr.attr,
+	&sensor_dev_attr_in2_min.dev_attr.attr,
+	&sensor_dev_attr_in2_max.dev_attr.attr,
 	&sensor_dev_attr_in2_alarm.dev_attr.attr,
+	NULL
+}, {
+	&sensor_dev_attr_in3_input.dev_attr.attr,
+	&sensor_dev_attr_in3_min.dev_attr.attr,
+	&sensor_dev_attr_in3_max.dev_attr.attr,
 	&sensor_dev_attr_in3_alarm.dev_attr.attr,
+	NULL
+}, {
+	&sensor_dev_attr_in4_input.dev_attr.attr,
+	&sensor_dev_attr_in4_min.dev_attr.attr,
+	&sensor_dev_attr_in4_max.dev_attr.attr,
 	&sensor_dev_attr_in4_alarm.dev_attr.attr,
+	NULL
+}, {
+	&sensor_dev_attr_in5_input.dev_attr.attr,
+	&sensor_dev_attr_in5_min.dev_attr.attr,
+	&sensor_dev_attr_in5_max.dev_attr.attr,
 	&sensor_dev_attr_in5_alarm.dev_attr.attr,
+	NULL
+}, {
+	&sensor_dev_attr_in6_input.dev_attr.attr,
+	&sensor_dev_attr_in6_min.dev_attr.attr,
+	&sensor_dev_attr_in6_max.dev_attr.attr,
 	&sensor_dev_attr_in6_alarm.dev_attr.attr,
+	NULL
+}, {
+	&sensor_dev_attr_in7_input.dev_attr.attr,
+	&sensor_dev_attr_in7_min.dev_attr.attr,
+	&sensor_dev_attr_in7_max.dev_attr.attr,
 	&sensor_dev_attr_in7_alarm.dev_attr.attr,
+	NULL
+}, {
+	&sensor_dev_attr_in8_input.dev_attr.attr,
+	NULL
+} };
 
+static const struct attribute_group it87_group_in[9] = {
+	{ .attrs = it87_attributes_in[0] },
+	{ .attrs = it87_attributes_in[1] },
+	{ .attrs = it87_attributes_in[2] },
+	{ .attrs = it87_attributes_in[3] },
+	{ .attrs = it87_attributes_in[4] },
+	{ .attrs = it87_attributes_in[5] },
+	{ .attrs = it87_attributes_in[6] },
+	{ .attrs = it87_attributes_in[7] },
+	{ .attrs = it87_attributes_in[8] },
+};
+
+static struct attribute *it87_attributes[] = {
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
 	&sensor_dev_attr_temp2_input.dev_attr.attr,
 	&sensor_dev_attr_temp3_input.dev_attr.attr,
@@ -1432,7 +1465,7 @@ static const struct attribute_group it87_group = {
 	.attrs = it87_attributes,
 };
 
-static struct attribute *it87_attributes_beep[] = {
+static struct attribute *it87_attributes_in_beep[] = {
 	&sensor_dev_attr_in0_beep.dev_attr.attr,
 	&sensor_dev_attr_in1_beep.dev_attr.attr,
 	&sensor_dev_attr_in2_beep.dev_attr.attr,
@@ -1441,7 +1474,10 @@ static struct attribute *it87_attributes_beep[] = {
 	&sensor_dev_attr_in5_beep.dev_attr.attr,
 	&sensor_dev_attr_in6_beep.dev_attr.attr,
 	&sensor_dev_attr_in7_beep.dev_attr.attr,
+	NULL
+};
 
+static struct attribute *it87_attributes_beep[] = {
 	&sensor_dev_attr_temp1_beep.dev_attr.attr,
 	&sensor_dev_attr_temp2_beep.dev_attr.attr,
 	&sensor_dev_attr_temp3_beep.dev_attr.attr,
@@ -1725,18 +1761,31 @@ static int __init it87_find(unsigned short *address,
 
 		/* VIN5 */
 		if ((reg27 & (1 << 0)) || uart6)
-			; /* No VIN5 */
+			sio_data->skip_in |= (1 << 5); /* No VIN5 */
 
 		/* VIN6 */
 		if ((reg27 & (1 << 1)) || uart6)
-			; /* No VIN6 */
+			sio_data->skip_in |= (1 << 6); /* No VIN6 */
 
 		/*
 		 * VIN7
 		 * Does not depend on bit 2 of Reg2C, contrary to datasheet.
 		 */
-		if (reg27 & (1 << 2))
-			; /* No VIN7 (unless internal) */
+		if (reg27 & (1 << 2)) {
+			/*
+			 * The data sheet is a bit uncear regarding the internal
+			 * voltage divider for VCCH5V. It says
+			 * "This bit enables and switches VIN7 (pin91) to the
+			 * internal voltage divider for VCCH5V".
+			 * This is different to other chips, where the internal
+			 * voltage divider would connect VIN7 to an internal
+			 * voltage source. Maybe that is the case here as well,
+			 * but at least for now follow the data sheet and assume
+			 * that there is no VIN7 if pin 91 is not configured as
+			 * VIN7 input.
+			 */
+			sio_data->skip_in |= (1 << 7);
+		}
 
 		if (reg2C & (1 << 0))
 			sio_data->internal |= (1 << 0);
@@ -1747,6 +1796,7 @@ static int __init it87_find(unsigned short *address,
 
 	} else {
 		int reg;
+		bool uart6;
 
 		superio_select(GPIO);
 
@@ -1784,6 +1834,9 @@ static int __init it87_find(unsigned short *address,
 			sio_data->vid_value = superio_inb(IT87_SIO_VID_REG);
 
 		reg = superio_inb(IT87_SIO_PINX2_REG);
+
+		uart6 = sio_data->type = it8782 && (reg & (1 << 2));
+
 		/*
 		 * The IT8720F has no VIN7 pin, so VCCH should always be
 		 * routed internally to VIN7 with an internal divider.
@@ -1795,11 +1848,10 @@ static int __init it87_find(unsigned short *address,
 		 * setting. So we force the internal routing in this case.
 		 *
 		 * On IT8782F, VIN7 is multiplexed with one of the UART6 pins.
-		 * If UART6 is enabled, re-route VIN7 to the internal divider.
+		 * If UART6 is enabled, re-route VIN7 to the internal divider
+		 * if that is not already the case.
 		 */
-		if ((sio_data->type = it8720 ||
-		    (sio_data->type = it8782 && (reg & (1 << 2))))
-		    && !(reg & (1 << 1))) {
+		if ((sio_data->type = it8720 || uart6) && !(reg & (1 << 1))) {
 			reg |= (1 << 1);
 			superio_outb(IT87_SIO_PINX2_REG, reg);
 			pr_notice("Routing internal VCCH to in7\n");
@@ -1810,6 +1862,14 @@ static int __init it87_find(unsigned short *address,
 		    sio_data->type = it8728)
 			sio_data->internal |= (1 << 1);
 
+		/*
+		 * On IT8782F, UART6 pins overlap with VIN5, VIN6, and VIN7.
+		 * While VIN7 can be routed to the internal voltage divider,
+		 * VIN5 and VIN6 are not available if UART6 is enabled.
+		 */
+		if (uart6)
+			sio_data->skip_in |= (1 << 5) | (1 << 6);
+
 		sio_data->beep_pin = superio_inb(IT87_SIO_BEEP_PIN_REG) & 0x3f;
 	}
 	if (sio_data->beep_pin)
@@ -1847,6 +1907,12 @@ static void it87_remove_files(struct device *dev)
 	int i;
 
 	sysfs_remove_group(&dev->kobj, &it87_group);
+	for (i = 0; i < 9; i++) {
+		sysfs_remove_group(&dev->kobj, &it87_group_in[i]);
+		if (it87_attributes_in_beep[i])
+			sysfs_remove_file(&dev->kobj,
+					  it87_attributes_in_beep[i]);
+	}
 	if (sio_data->beep_pin)
 		sysfs_remove_group(&dev->kobj, &it87_group_beep);
 	for (i = 0; i < 5; i++) {
@@ -1945,9 +2011,23 @@ static int __devinit it87_probe(struct platform_device *pdev)
 	it87_init_device(pdev);
 
 	/* Register sysfs hooks */
+	for (i = 0; i < 9; i++) {
+		if (sio_data->skip_in & (1 << i))
+			continue;
+		err = sysfs_create_group(&dev->kobj, &it87_group_in[i]);
+		if (err)
+			goto ERROR4;
+		if (sio_data->beep_pin && it87_attributes_in_beep[i]) {
+			err = sysfs_create_file(&dev->kobj,
+						it87_attributes_in_beep[i]);
+			if (err)
+				goto ERROR4;
+		}
+	}
+
 	err = sysfs_create_group(&dev->kobj, &it87_group);
 	if (err)
-		goto ERROR2;
+		goto ERROR4;
 
 	if (sio_data->beep_pin) {
 		err = sysfs_create_group(&dev->kobj, &it87_group_beep);
-- 
1.7.5.4


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/3] hwmon: (it87) Create voltage attributes only if voltage is enabled

[Jean Delvare]

Hi Guenter,

On Tue,  8 May 2012 19:31:52 -0700, Guenter Roeck wrote:
> On IT8782F and IT8783F, some voltage input pins may be disabled. Don't create
> sysfs attribute files if that is the case.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/it87.c |  146 ++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 113 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index aebac13..baf4173 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> (...)
> @@ -1725,18 +1761,31 @@ static int __init it87_find(unsigned short *address,
>  
>  		/* VIN5 */
>  		if ((reg27 & (1 << 0)) || uart6)
> -			; /* No VIN5 */
> +			sio_data->skip_in |= (1 << 5); /* No VIN5 */
>  
>  		/* VIN6 */
>  		if ((reg27 & (1 << 1)) || uart6)
> -			; /* No VIN6 */
> +			sio_data->skip_in |= (1 << 6); /* No VIN6 */
>  
>  		/*
>  		 * VIN7
>  		 * Does not depend on bit 2 of Reg2C, contrary to datasheet.
>  		 */
> -		if (reg27 & (1 << 2))
> -			; /* No VIN7 (unless internal) */
> +		if (reg27 & (1 << 2)) {
> +			/*
> +			 * The data sheet is a bit uncear regarding the internal

Typo: "unclear".

> +			 * voltage divider for VCCH5V. It says
> +			 * "This bit enables and switches VIN7 (pin91) to the

Missing space between "pin" and "91".

> +			 * internal voltage divider for VCCH5V".
> +			 * This is different to other chips, where the internal
> +			 * voltage divider would connect VIN7 to an internal
> +			 * voltage source. Maybe that is the case here as well,
> +			 * but at least for now follow the data sheet and assume
> +			 * that there is no VIN7 if pin 91 is not configured as
> +			 * VIN7 input.
> +			 */

I think this is the unfortunate result of a copy / paste / edit from
another datasheet. If you look at the IT8716F datasheet, it said:

"Enables PCIRSTIN# (pin 91), and switches VIN7 function to internal
voltage divider for VCCH5V"

So my take is that the editor dropped the reference to PCIRSTIN#
because pin 91 can't be that on the IT8783E/F, but forgot to mention
the other alternative functions (which the IT8716F didn't have.) The
wording was pretty confusing in the first place, and the many
configuration options of the IT8783F only make things worse. I just
can't get how manufacturers can't come up with less confusing designs.

So my personal guess is that this bit switches VIN7 to the internal
source just as with the other chips.

> +			sio_data->skip_in |= (1 << 7);
> +		}
>  
>  		if (reg2C & (1 << 0))
>  			sio_data->internal |= (1 << 0);
> (...)
> @@ -1847,6 +1907,12 @@ static void it87_remove_files(struct device *dev)
>  	int i;
>  
>  	sysfs_remove_group(&dev->kobj, &it87_group);
> +	for (i = 0; i < 9; i++) {

For consistency, you could check sio_data->skip_in. I know no harm is
done if you don't, but the driver does it for fan inputs and PWM
outputs already.

> +		sysfs_remove_group(&dev->kobj, &it87_group_in[i]);
> +		if (it87_attributes_in_beep[i])
> +			sysfs_remove_file(&dev->kobj,
> +					  it87_attributes_in_beep[i]);
> +	}
>  	if (sio_data->beep_pin)
>  		sysfs_remove_group(&dev->kobj, &it87_group_beep);
>  	for (i = 0; i < 5; i++) {
> @@ -1945,9 +2011,23 @@ static int __devinit it87_probe(struct platform_device *pdev)
>  	it87_init_device(pdev);
>  
>  	/* Register sysfs hooks */
> +	for (i = 0; i < 9; i++) {
> +		if (sio_data->skip_in & (1 << i))
> +			continue;
> +		err = sysfs_create_group(&dev->kobj, &it87_group_in[i]);
> +		if (err)
> +			goto ERROR4;
> +		if (sio_data->beep_pin && it87_attributes_in_beep[i]) {
> +			err = sysfs_create_file(&dev->kobj,
> +						it87_attributes_in_beep[i]);
> +			if (err)
> +				goto ERROR4;
> +		}
> +	}
> +
>  	err = sysfs_create_group(&dev->kobj, &it87_group);
>  	if (err)
> -		goto ERROR2;
> +		goto ERROR4;

Why didn't you leave this call first? This would avoid having to change
this label -> smaller patch.

>  
>  	if (sio_data->beep_pin) {
>  		err = sysfs_create_group(&dev->kobj, &it87_group_beep);

These are all minor things, overall the patch looks very good, so you
can add:

Acked-by: Jean Delvare <khali@linux-fr.org>

after addressing the points I made that you find relevant. And sorry
again for the late review.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/3] hwmon: (it87) Create voltage attributes only if voltage is enabled

[Guenter Roeck]

Hi Jean,

On Thu, May 10, 2012 at 04:32:33PM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> On Tue,  8 May 2012 19:31:52 -0700, Guenter Roeck wrote:
> > On IT8782F and IT8783F, some voltage input pins may be disabled. Don't create
> > sysfs attribute files if that is the case.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/hwmon/it87.c |  146 ++++++++++++++++++++++++++++++++++++++-----------
> >  1 files changed, 113 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > index aebac13..baf4173 100644
> > --- a/drivers/hwmon/it87.c
> > +++ b/drivers/hwmon/it87.c
> > (...)
> > @@ -1725,18 +1761,31 @@ static int __init it87_find(unsigned short *address,
> >  
> >  		/* VIN5 */
> >  		if ((reg27 & (1 << 0)) || uart6)
> > -			; /* No VIN5 */
> > +			sio_data->skip_in |= (1 << 5); /* No VIN5 */
> >  
> >  		/* VIN6 */
> >  		if ((reg27 & (1 << 1)) || uart6)
> > -			; /* No VIN6 */
> > +			sio_data->skip_in |= (1 << 6); /* No VIN6 */
> >  
> >  		/*
> >  		 * VIN7
> >  		 * Does not depend on bit 2 of Reg2C, contrary to datasheet.
> >  		 */
> > -		if (reg27 & (1 << 2))
> > -			; /* No VIN7 (unless internal) */
> > +		if (reg27 & (1 << 2)) {
> > +			/*
> > +			 * The data sheet is a bit uncear regarding the internal
> 
> Typo: "unclear".
> 
Fixed.

> > +			 * voltage divider for VCCH5V. It says
> > +			 * "This bit enables and switches VIN7 (pin91) to the
> 
> Missing space between "pin" and "91".
> 
Fixed.

> > +			 * internal voltage divider for VCCH5V".
> > +			 * This is different to other chips, where the internal
> > +			 * voltage divider would connect VIN7 to an internal
> > +			 * voltage source. Maybe that is the case here as well,
> > +			 * but at least for now follow the data sheet and assume
> > +			 * that there is no VIN7 if pin 91 is not configured as
> > +			 * VIN7 input.
> > +			 */
> 
> I think this is the unfortunate result of a copy / paste / edit from
> another datasheet. If you look at the IT8716F datasheet, it said:
> 
> "Enables PCIRSTIN# (pin 91), and switches VIN7 function to internal
> voltage divider for VCCH5V"
> 
> So my take is that the editor dropped the reference to PCIRSTIN#
> because pin 91 can't be that on the IT8783E/F, but forgot to mention
> the other alternative functions (which the IT8716F didn't have.) The
> wording was pretty confusing in the first place, and the many
> configuration options of the IT8783F only make things worse. I just
> can't get how manufacturers can't come up with less confusing designs.
> 
Whoever finds the most confusing solution gets a prize.

But, seriously, it is not entirely their fault. They probably have n customers
with n^2 conflicting requirements, and try to meet them all.

> So my personal guess is that this bit switches VIN7 to the internal
> source just as with the other chips.
> 
Hmmm ... no idea what I should do. Play it safe or assume this is an error
in the data sheet ? I tend towards playing safe, but not too much.
Guess I am waiting for someone to convince me otherwise...

> > +			sio_data->skip_in |= (1 << 7);
> > +		}
> >  
> >  		if (reg2C & (1 << 0))
> >  			sio_data->internal |= (1 << 0);
> > (...)
> > @@ -1847,6 +1907,12 @@ static void it87_remove_files(struct device *dev)
> >  	int i;
> >  
> >  	sysfs_remove_group(&dev->kobj, &it87_group);
> > +	for (i = 0; i < 9; i++) {
> 
> For consistency, you could check sio_data->skip_in. I know no harm is
> done if you don't, but the driver does it for fan inputs and PWM
> outputs already.
> 
Done.

> > +		sysfs_remove_group(&dev->kobj, &it87_group_in[i]);
> > +		if (it87_attributes_in_beep[i])
> > +			sysfs_remove_file(&dev->kobj,
> > +					  it87_attributes_in_beep[i]);
> > +	}
> >  	if (sio_data->beep_pin)
> >  		sysfs_remove_group(&dev->kobj, &it87_group_beep);
> >  	for (i = 0; i < 5; i++) {
> > @@ -1945,9 +2011,23 @@ static int __devinit it87_probe(struct platform_device *pdev)
> >  	it87_init_device(pdev);
> >  
> >  	/* Register sysfs hooks */
> > +	for (i = 0; i < 9; i++) {
> > +		if (sio_data->skip_in & (1 << i))
> > +			continue;
> > +		err = sysfs_create_group(&dev->kobj, &it87_group_in[i]);
> > +		if (err)
> > +			goto ERROR4;
> > +		if (sio_data->beep_pin && it87_attributes_in_beep[i]) {
> > +			err = sysfs_create_file(&dev->kobj,
> > +						it87_attributes_in_beep[i]);
> > +			if (err)
> > +				goto ERROR4;
> > +		}
> > +	}
> > +
> >  	err = sysfs_create_group(&dev->kobj, &it87_group);
> >  	if (err)
> > -		goto ERROR2;
> > +		goto ERROR4;
> 
> Why didn't you leave this call first? This would avoid having to change
> this label -> smaller patch.
> 
No special reason. I moved it.

> >  
> >  	if (sio_data->beep_pin) {
> >  		err = sysfs_create_group(&dev->kobj, &it87_group_beep);
> 
> These are all minor things, overall the patch looks very good, so you
> can add:
> 
> Acked-by: Jean Delvare <khali@linux-fr.org>
> 
> after addressing the points I made that you find relevant. And sorry
> again for the late review.
> 
No problem. Thanks a lot for the review!

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/3] hwmon: (it87) Create voltage attributes only if voltage is enabled

[Jean Delvare]

On Thu, 10 May 2012 21:20:44 -0700, Guenter Roeck wrote:
> On Thu, May 10, 2012 at 04:32:33PM -0400, Jean Delvare wrote:
> > I think this is the unfortunate result of a copy / paste / edit from
> > another datasheet. If you look at the IT8716F datasheet, it said:
> > 
> > "Enables PCIRSTIN# (pin 91), and switches VIN7 function to internal
> > voltage divider for VCCH5V"
> > 
> > So my take is that the editor dropped the reference to PCIRSTIN#
> > because pin 91 can't be that on the IT8783E/F, but forgot to mention
> > the other alternative functions (which the IT8716F didn't have.) The
> > wording was pretty confusing in the first place, and the many
> > configuration options of the IT8783F only make things worse. I just
> > can't get how manufacturers can't come up with less confusing designs.
> > 
> Whoever finds the most confusing solution gets a prize.
> 
> But, seriously, it is not entirely their fault. They probably have n customers
> with n^2 conflicting requirements, and try to meet them all.

Having a few options is fine, but when it reaches that level of
complexity, I believe they should simply manufacture more chip variants
for the different use cases.

If they really don't want to do that, I'm sure it would be possible to
reach the same level of flexibility with less clutter and confusion. I
suspect they keep reusing past designs and only modify one thing each
time. Starting from scratch once in a while would help a lot.

> > So my personal guess is that this bit switches VIN7 to the internal
> > source just as with the other chips.
> > 
> Hmmm ... no idea what I should do. Play it safe or assume this is an error
> in the data sheet ? I tend towards playing safe, but not too much.
> Guess I am waiting for someone to convince me otherwise...

I would not skip VIN7 until we know more. People will never report
about missing 5VSB monitoring, but they will report if 5VSB looks wrong.

Do we have any tester for the IT8783F yet? What you could do for now is
emit a warning / call for report when the case happens. When someone
reports, we have our tester and we'll know what should be done.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/3] hwmon: (it87) Create voltage attributes only if voltage is enabled

[Guenter Roeck]

On Fri, May 11, 2012 at 05:23:52AM -0400, Jean Delvare wrote:
[ ... ]
> 
> > > So my personal guess is that this bit switches VIN7 to the internal
> > > source just as with the other chips.
> > > 
> > Hmmm ... no idea what I should do. Play it safe or assume this is an error
> > in the data sheet ? I tend towards playing safe, but not too much.
> > Guess I am waiting for someone to convince me otherwise...
> 
> I would not skip VIN7 until we know more. People will never report
> about missing 5VSB monitoring, but they will report if 5VSB looks wrong.
> 
> Do we have any tester for the IT8783F yet? What you could do for now is

Bjoern Gerhart has been testing it quite extensively. I don't recall
what they do with VIN7, though.

> emit a warning / call for report when the case happens. When someone
> reports, we have our tester and we'll know what should be done.
> 
That makes sense. I'll do that.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/3] hwmon: (it87) Create voltage attributes only if voltage is enabled

[Björn Gerhart]

Hi @ll,

Am 11.05.2012 um 15:38 schrieb Guenter Roeck:
> On Fri, May 11, 2012 at 05:23:52AM -0400, Jean Delvare wrote:
> [ ... ]
>> 
>>>> So my personal guess is that this bit switches VIN7 to the internal
>>>> source just as with the other chips.
>>>> 
>>> Hmmm ... no idea what I should do. Play it safe or assume this is an error
>>> in the data sheet ? I tend towards playing safe, but not too much.
>>> Guess I am waiting for someone to convince me otherwise...
>> 
>> I would not skip VIN7 until we know more. People will never report
>> about missing 5VSB monitoring, but they will report if 5VSB looks wrong.
>> 
>> Do we have any tester for the IT8783F yet? What you could do for now is
> 
> Bjoern Gerhart has been testing it quite extensively. I don't recall
> what they do with VIN7, though.
> 
Yes, I have an IT8783F hw configuration with voltage in7 connected to 1.1V nominal. I can give it a re-test with the latest patches applied on beginning of next week against kernel 2.6.32. Then I can also ask if my hw colleagues have an idea about the pin 91 mystery.

Björn


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/3] hwmon: (it87) Create voltage attributes only if voltage is enabled

[Björn Gerhart]

Hi Guenter,

Am 11.05.2012 um 21:15 schrieb Björn Gerhart:
> Hi @ll,
> 
> Am 11.05.2012 um 15:38 schrieb Guenter Roeck:
>> On Fri, May 11, 2012 at 05:23:52AM -0400, Jean Delvare wrote:
>> [ ... ]
>>> 
>>>>> So my personal guess is that this bit switches VIN7 to the internal
>>>>> source just as with the other chips.
>>>>> 
>>>> Hmmm ... no idea what I should do. Play it safe or assume this is an error
>>>> in the data sheet ? I tend towards playing safe, but not too much.
>>>> Guess I am waiting for someone to convince me otherwise...
>>> 
>>> I would not skip VIN7 until we know more. People will never report
>>> about missing 5VSB monitoring, but they will report if 5VSB looks wrong.
>>> 
>>> Do we have any tester for the IT8783F yet? What you could do for now is
>> 
>> Bjoern Gerhart has been testing it quite extensively. I don't recall
>> what they do with VIN7, though.
>> 
> Yes, I have an IT8783F hw configuration with voltage in7 connected to 1.1V nominal. I can give it a re-test with the latest patches applied on beginning of next week against kernel 2.6.32. Then I can also ask if my hw colleagues have an idea about the pin 91 mystery.
> 
In our design, the IT8783F's Pin 91 is defined as VIN7, and likewise bit2 of index 0x2C is 1.
As I'm not yet used to git: which would be the easiest way to get the it87 with all patches integrated for kernel 2.6.32 for testing purposes? On your github site, I didn't find the appropriate entry for the patches you sent...

Björn


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/3] hwmon: (it87) Create voltage attributes only if voltage is enabled

[Jean Delvare]

T24gTW9uLCAxNCBNYXkgMjAxMiAyMzoyMTozNyArMDIwMCwgQmrDtnJuIEdlcmhhcnQgd3JvdGU6
Cj4gSW4gb3VyIGRlc2lnbiwgdGhlIElUODc4M0YncyBQaW4gOTEgaXMgZGVmaW5lZCBhcyBWSU43
LCBhbmQgbGlrZXdpc2UgYml0MiBvZiBpbmRleCAweDJDIGlzIDEuCgpXaGF0IGFib3V0IGJpdCAx
IG9mIHJlZ2lzdGVyIDB4MkMgYW5kIGJpdCAyIG9mIHJlZ2lzdGVyIDB4Mjc/Cgo+IEFzIEknbSBu
b3QgeWV0IHVzZWQgdG8gZ2l0OiB3aGljaCB3b3VsZCBiZSB0aGUgZWFzaWVzdCB3YXkgdG8gZ2V0
IHRoZSBpdDg3IHdpdGggYWxsIHBhdGNoZXMgaW50ZWdyYXRlZCBmb3Iga2VybmVsIDIuNi4zMiBm
b3IgdGVzdGluZyBwdXJwb3Nlcz8gT24geW91ciBnaXRodWIgc2l0ZSwgSSBkaWRuJ3QgZmluZCB0
aGUgYXBwcm9wcmlhdGUgZW50cnkgZm9yIHRoZSBwYXRjaGVzIHlvdSBzZW50Li4uCgpTdGFuZGFs
b25lIGRyaXZlciBpcyBhdDoKaHR0cDovL2toYWxpLmxpbnV4LWZyLm9yZy9kZXZlbC9taXNjL2l0
ODcvCgpJbnN0cnVjdGlvbnMgYXQ6Cmh0dHA6Ly9raGFsaS5saW51eC1mci5vcmcvZGV2ZWwvbWlz
Yy9JTlNUQUxMCgotLSAKSmVhbiBEZWx2YXJlCmh0dHA6Ly9raGFsaS5saW51eC1mci5vcmcvd2lz
aGxpc3QuaHRtbAoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X18KbG0tc2Vuc29ycyBtYWlsaW5nIGxpc3QKbG0tc2Vuc29yc0BsbS1zZW5zb3JzLm9yZwpodHRw
Oi8vbGlzdHMubG0tc2Vuc29ycy5vcmcvbWFpbG1hbi9saXN0aW5mby9sbS1zZW5zb3Jz

Re: [lm-sensors] [PATCH 1/3] hwmon: (it87) Create voltage attributes only if voltage is enabled

[Björn Gerhart]

Hi Jean,

Am 15.05.2012 um 08:22 schrieb Jean Delvare:
> On Mon, 14 May 2012 23:21:37 +0200, Björn Gerhart wrote:
>> In our design, the IT8783F's Pin 91 is defined as VIN7, and likewise bit2 of index 0x2C is 1.
> 
> What about bit 1 of register 0x2C and bit 2 of register 0x27?
> 
bit 1 / register 0x2C: 0 (meaning: internal voltage divider is disabled)
bit 2 / register 0x27: 0 (meaning: pin91 is vin7)

To make the configuration complete:
- index register 0x27 has 0x10 as value
- index register 0x2C has 0x95 as value
- UART6 moves to pins 100~106 by JP4 (pin1) set to 0


>> As I'm not yet used to git: which would be the easiest way to get the it87 with all patches integrated for kernel 2.6.32 for testing purposes? On your github site, I didn't find the appropriate entry for the patches you sent...
> 
> Standalone driver is at:
> http://khali.linux-fr.org/devel/misc/it87/
> 
Ah nice, thanks for the URL. It was no problem to compile against the CentOS 6.2 kernel 2.6.32.

Good news, vin7 (connected to 1.1V nominal voltage) is displayed correctly.

However, with this version of it87, the voltage -12V (vin6) disappears on my IT8873F. Loading the older it87 version from 2012/3/8, vin6 appears again - while /etc/sensors3.conf remains the same.

As mentioned, UART6 is enabled, and at the same time shifted UART6 to pins 100~106. Therefore, pin 92 and 93 get usable. Because this setting is not detectable (as discussed before with Guenter), the dependancy to UART6 maybe has to be removed here also.
A separate tiny patch is on its way. A test with my IT8783F configuration worked fine, and vin6 was displayed again.

Björn


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors