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

[Guenter Roeck <guenter.roeck@ericsson.com>]

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