Re: [PATCH 04/15] hwmon: (it87) Introduce feature flag to reflect internal in7 sensor

[Guenter Roeck <linux@xxxxxxxxxxxx>]

On 03/30/2015 01:05 PM, Jean Delvare wrote:
> On Sun, 29 Mar 2015 23:33:44 -0700, Guenter Roeck wrote:
> > On some chips, in7 is always an internal voltage sensor. Introduce
> > feature flag to reflect this condition to simplify adding support
> > for new chips.
> >
> > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > ---
> >   drivers/hwmon/it87.c | 25 +++++++++++++++----------
> >   1 file changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > index bdd6b33a3b25..2b41a598cee7 100644
> > --- a/drivers/hwmon/it87.c
> > +++ b/drivers/hwmon/it87.c
> > @@ -258,6 +258,7 @@ struct it87_devices {
> >   #define FEAT_FAN16_CONFIG	(1 << 7)	/* Need to enable 16-bit fans */
> >   #define FEAT_FIVE_FANS		(1 << 8)	/* Supports five fans */
> >   #define FEAT_VID		(1 << 9)	/* Set if chip supports VID */
> > +#define FEAT_IN7_INTERNAL	(1 << 10)	/* Set if in7 is internal */
> >
> >   static const struct it87_devices it87_devices[] = {
> >   	[it87] = {
> > @@ -295,7 +296,7 @@ static const struct it87_devices it87_devices[] = {
> >   		.name = "it8721",
> >   		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
> >   		  | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI
> > -		  | FEAT_FAN16_CONFIG | FEAT_FIVE_FANS,
> > +		  | FEAT_FAN16_CONFIG | FEAT_FIVE_FANS | FEAT_IN7_INTERNAL,
> >   		.peci_mask = 0x05,
> >   		.old_peci_mask = 0x02,	/* Actually reports PCH */
> >   		.suffix = "F",
> > @@ -303,14 +304,15 @@ static const struct it87_devices it87_devices[] = {
> >   	[it8728] = {
> >   		.name = "it8728",
> >   		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
> > -		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_FIVE_FANS,
> > +		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_FIVE_FANS
> > +		  | FEAT_IN7_INTERNAL,
> >   		.peci_mask = 0x07,
> >   		.suffix = "F",
> >   	},
> >   	[it8771] = {
> >   		.name = "it8771",
> >   		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
> > -		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI,
> > +		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL,
> >   				/* PECI: guesswork */
> >   				/* 12mV ADC (OHM) */
> >   				/* 16 bit fans (OHM) */
> > @@ -321,7 +323,7 @@ static const struct it87_devices it87_devices[] = {
> >   	[it8772] = {
> >   		.name = "it8772",
> >   		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
> > -		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI,
> > +		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL,
> >   				/* PECI (coreboot) */
> >   				/* 12mV ADC (HWSensors4, OHM) */
> >   				/* 16 bit fans (HWSensors4, OHM) */
> > @@ -353,14 +355,14 @@ static const struct it87_devices it87_devices[] = {
> >   	[it8786] = {
> >   		.name = "it8786",
> >   		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
> > -		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI,
> > +		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL,
> >   		.peci_mask = 0x07,
> >   		.suffix = "E",
> >   	},
> >   	[it8603] = {
> >   		.name = "it8603",
> >   		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
> > -		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI,
> > +		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL,
> >   		.peci_mask = 0x07,
> >   		.suffix = "E",
> >   	},
> > @@ -379,6 +381,7 @@ static const struct it87_devices it87_devices[] = {
> >   #define has_fan16_config(data)	((data)->features & FEAT_FAN16_CONFIG)
> >   #define has_five_fans(data)	((data)->features & FEAT_FIVE_FANS)
> >   #define has_vid(data)		((data)->features & FEAT_VID)
> > +#define has_in7_internal(data)	((data)->features & FEAT_IN7_INTERNAL)
> >
> >   struct it87_sio_data {
> >   	enum chips type;
> > @@ -1862,6 +1865,11 @@ static int __init it87_find(unsigned short *address,
> >
> >   	/* in8 (Vbat) is always internal */
> >   	sio_data->internal = (1 << 2);
> > +
> > +	/* in7 (VSB or VCCH5V) is always internal on some chips */
> > +	if (it87_devices[sio_data->type].features & FEAT_IN7_INTERNAL)
> > +		sio_data->internal |= (1 << 1);
> > +
> >   	/* Only the IT8603E has in9 */
> >   	if (sio_data->type != it8603)
> >   		sio_data->skip_in |= (1 << 9);
>
> Nitpicking, but wouldn't it make more sense to deal with in7 first,
> then in8, then in9?

Agreed, and done.

> > @@ -1962,7 +1970,6 @@ static int __init it87_find(unsigned short *address,
> >   		sio_data->skip_in |= (1 << 5); /* No VIN5 */
> >   		sio_data->skip_in |= (1 << 6); /* No VIN6 */
> >
> > -		sio_data->internal |= (1 << 1); /* in7 is VSB */
> >   		sio_data->internal |= (1 << 3); /* in9 is AVCC */
> >
> >   		sio_data->beep_pin = superio_inb(IT87_SIO_BEEP_PIN_REG) & 0x3f;
> > @@ -2023,9 +2030,7 @@ static int __init it87_find(unsigned short *address,
> >   		}
> >   		if (reg & (1 << 0))
> >   			sio_data->internal |= (1 << 0);
> > -		if ((reg & (1 << 1)) || sio_data->type == it8721 ||
> > -		    sio_data->type == it8728 || sio_data->type == it8771 ||
> > -		    sio_data->type == it8772 || sio_data->type == it8786)
> > +		if (reg & (1 << 1))
> >   			sio_data->internal |= (1 << 1);
> >
> >   		/*
>
> Other than that, no objection.
>
> Reviewed-by: Jean Delvare <jdelvare@xxxxxxx>
Thanks again!

Guenter


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