[lm-sensors] [PATCH v2 1/3] hwmon: (lm90) Introduce chip parameter

[lm-sensors] [PATCH v2 1/3] hwmon: (lm90) Introduce chip parameter

[Guenter Roeck]

Instead of using switch/case and if statements in probe, define chip specific
functionality in a parameter structure array.

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
 drivers/hwmon/lm90.c |   92 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 60 insertions(+), 32 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 302d9eb..9df08e1 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -190,6 +190,63 @@ static const struct i2c_device_id lm90_id[] = {
 MODULE_DEVICE_TABLE(i2c, lm90_id);
 
 /*
+ * chip type specific parameters
+ */
+struct lm90_params {
+	u32 flags;		/* Capabilities */
+	u16 alert_alarms;	/* Which alarm bits trigger ALERT# */
+				/* Upper 8 bits for max6695/96 */
+};
+
+static const struct lm90_params lm90_params[] = {
+	[adm1032] = {
+		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
+		.alert_alarms = 0x7c,
+	},
+	[adt7461] = {
+		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
+		.alert_alarms = 0x7c,
+	},
+	[lm86] = {
+		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
+		.alert_alarms = 0x7b,
+	},
+	[lm90] = {
+		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
+		.alert_alarms = 0x7b,
+	},
+	[lm99] = {
+		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
+		.alert_alarms = 0x7b,
+	},
+	[max6646] = {
+		.flags = LM90_HAVE_LOCAL_EXT,
+		.alert_alarms = 0x7c,
+	},
+	[max6657] = {
+		.flags = LM90_HAVE_LOCAL_EXT,
+		.alert_alarms = 0x7c,
+	},
+	[max6659] = {
+		.flags = LM90_HAVE_LOCAL_EXT | LM90_HAVE_EMERGENCY,
+		.alert_alarms = 0x7c,
+	},
+	[max6680] = {
+		.flags = LM90_HAVE_OFFSET,
+		.alert_alarms = 0x7c,
+	},
+	[max6696] = {
+		.flags = LM90_HAVE_LOCAL_EXT | LM90_HAVE_EMERGENCY
+		  | LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3,
+		.alert_alarms = 0x187c,
+	},
+	[w83l771] = {
+		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
+		.alert_alarms = 0x7c,
+	},
+};
+
+/*
  * Client data (each client gets its own)
  */
 
@@ -199,7 +256,7 @@ struct lm90_data {
 	char valid; /* zero until following fields are valid */
 	unsigned long last_updated; /* in jiffies */
 	int kind;
-	int flags;
+	u32 flags;
 
 	u8 config_orig;		/* Original configuration register value */
 	u16 alert_alarms;	/* Which alarm bits trigger ALERT# */
@@ -1201,39 +1258,10 @@ static int lm90_probe(struct i2c_client *new_client,
 
 	/* Different devices have different alarm bits triggering the
 	 * ALERT# output */
-	switch (data->kind) {
-	case lm90:
-	case lm99:
-	case lm86:
-		data->alert_alarms = 0x7b;
-		break;
-	case max6696:
-		data->alert_alarms = 0x187c;
-		break;
-	default:
-		data->alert_alarms = 0x7c;
-		break;
-	}
+	data->alert_alarms = lm90_params[data->kind].alert_alarms;
 
 	/* Set chip capabilities */
-	if (data->kind != max6657 && data->kind != max6659
-	    && data->kind != max6646 && data->kind != max6696)
-		data->flags |= LM90_HAVE_OFFSET;
-
-	if (data->kind = max6657 || data->kind = max6659
-	    || data->kind = max6646 || data->kind = max6696)
-		data->flags |= LM90_HAVE_LOCAL_EXT;
-
-	if (data->kind != max6657 && data->kind != max6659
-	    && data->kind != max6646 && data->kind != max6680
-	    && data->kind != max6696)
-		data->flags |= LM90_HAVE_REM_LIMIT_EXT;
-
-	if (data->kind = max6659 || data->kind = max6696)
-		data->flags |= LM90_HAVE_EMERGENCY;
-
-	if (data->kind = max6696)
-		data->flags |= LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3;
+	data->flags = lm90_params[data->kind].flags;
 
 	/* Initialize the LM90 chip */
 	lm90_init_client(new_client);
-- 
1.7.3.1


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

Re: [lm-sensors] [PATCH v2 1/3] hwmon: (lm90) Introduce chip

[Jean Delvare]

Hi Guenter,

On Tue, 5 Oct 2010 08:38:26 -0700, Guenter Roeck wrote:
> Instead of using switch/case and if statements in probe, define chip specific
> functionality in a parameter structure array.
> 
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
>  drivers/hwmon/lm90.c |   92 ++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 60 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 302d9eb..9df08e1 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -190,6 +190,63 @@ static const struct i2c_device_id lm90_id[] = {
>  MODULE_DEVICE_TABLE(i2c, lm90_id);
>  
>  /*
> + * chip type specific parameters
> + */
> +struct lm90_params {
> +	u32 flags;		/* Capabilities */

Any reason why you don't use u16? there aren't that many flags yet, and
u16 divides the size of lm90_params[] below by 2.

> +	u16 alert_alarms;	/* Which alarm bits trigger ALERT# */
> +				/* Upper 8 bits for max6695/96 */
> +};
> +
> +static const struct lm90_params lm90_params[] = {
> +	[adm1032] = {
> +		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> +		.alert_alarms = 0x7c,
> +	},
> +	[adt7461] = {
> +		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> +		.alert_alarms = 0x7c,
> +	},
> +	[lm86] = {
> +		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> +		.alert_alarms = 0x7b,
> +	},
> +	[lm90] = {
> +		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> +		.alert_alarms = 0x7b,
> +	},
> +	[lm99] = {
> +		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> +		.alert_alarms = 0x7b,
> +	},
> +	[max6646] = {
> +		.flags = LM90_HAVE_LOCAL_EXT,
> +		.alert_alarms = 0x7c,
> +	},
> +	[max6657] = {
> +		.flags = LM90_HAVE_LOCAL_EXT,
> +		.alert_alarms = 0x7c,
> +	},
> +	[max6659] = {
> +		.flags = LM90_HAVE_LOCAL_EXT | LM90_HAVE_EMERGENCY,
> +		.alert_alarms = 0x7c,
> +	},
> +	[max6680] = {
> +		.flags = LM90_HAVE_OFFSET,
> +		.alert_alarms = 0x7c,
> +	},
> +	[max6696] = {
> +		.flags = LM90_HAVE_LOCAL_EXT | LM90_HAVE_EMERGENCY
> +		  | LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3,
> +		.alert_alarms = 0x187c,
> +	},
> +	[w83l771] = {
> +		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> +		.alert_alarms = 0x7c,
> +	},
> +};
> +
> +/*
>   * Client data (each client gets its own)
>   */
>  
> @@ -199,7 +256,7 @@ struct lm90_data {
>  	char valid; /* zero until following fields are valid */
>  	unsigned long last_updated; /* in jiffies */
>  	int kind;
> -	int flags;
> +	u32 flags;
>  
>  	u8 config_orig;		/* Original configuration register value */
>  	u16 alert_alarms;	/* Which alarm bits trigger ALERT# */
> @@ -1201,39 +1258,10 @@ static int lm90_probe(struct i2c_client *new_client,
>  
>  	/* Different devices have different alarm bits triggering the
>  	 * ALERT# output */
> -	switch (data->kind) {
> -	case lm90:
> -	case lm99:
> -	case lm86:
> -		data->alert_alarms = 0x7b;
> -		break;
> -	case max6696:
> -		data->alert_alarms = 0x187c;
> -		break;
> -	default:
> -		data->alert_alarms = 0x7c;
> -		break;
> -	}
> +	data->alert_alarms = lm90_params[data->kind].alert_alarms;
>  
>  	/* Set chip capabilities */
> -	if (data->kind != max6657 && data->kind != max6659
> -	    && data->kind != max6646 && data->kind != max6696)
> -		data->flags |= LM90_HAVE_OFFSET;
> -
> -	if (data->kind = max6657 || data->kind = max6659
> -	    || data->kind = max6646 || data->kind = max6696)
> -		data->flags |= LM90_HAVE_LOCAL_EXT;
> -
> -	if (data->kind != max6657 && data->kind != max6659
> -	    && data->kind != max6646 && data->kind != max6680
> -	    && data->kind != max6696)
> -		data->flags |= LM90_HAVE_REM_LIMIT_EXT;
> -
> -	if (data->kind = max6659 || data->kind = max6696)
> -		data->flags |= LM90_HAVE_EMERGENCY;
> -
> -	if (data->kind = max6696)
> -		data->flags |= LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3;
> +	data->flags = lm90_params[data->kind].flags;
>  
>  	/* Initialize the LM90 chip */
>  	lm90_init_client(new_client);

I like this change a lot, even though it makes the binary module a
little larger. This is a lot cleaner. Consider it applied.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH v2 1/3] hwmon: (lm90) Introduce chip

[Jean Delvare]

On Wed, 6 Oct 2010 17:05:53 +0200, Jean Delvare wrote:
> I like this change a lot, even though it makes the binary module a
> little larger.

Err, scratch that, it doesn't. Not sure why I thought it did...

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH v2 1/3] hwmon: (lm90) Introduce chip

[Guenter Roeck]

Hi Jean,

On Wed, Oct 06, 2010 at 11:05:53AM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> On Tue, 5 Oct 2010 08:38:26 -0700, Guenter Roeck wrote:
> > Instead of using switch/case and if statements in probe, define chip specific
> > functionality in a parameter structure array.
> > 
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> >  drivers/hwmon/lm90.c |   92 ++++++++++++++++++++++++++++++++-----------------
> >  1 files changed, 60 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index 302d9eb..9df08e1 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -190,6 +190,63 @@ static const struct i2c_device_id lm90_id[] = {
> >  MODULE_DEVICE_TABLE(i2c, lm90_id);
> >  
> >  /*
> > + * chip type specific parameters
> > + */
> > +struct lm90_params {
> > +	u32 flags;		/* Capabilities */
> 
> Any reason why you don't use u16? there aren't that many flags yet, and
> u16 divides the size of lm90_params[] below by 2.
> 
Access to 32 bit variables is a bit faster on many CPUs, due to the bit masking
required otherwise. Also, the flags field in lm90_data was int and I changed
it to u32 (seemed to be cleaner for a bitfield). Seemed to make sense to use
the same type for both.

Not that it matters much here, if at all, since this is a bit mask anyway.
I am fine either way.

> > +	u16 alert_alarms;	/* Which alarm bits trigger ALERT# */
> > +				/* Upper 8 bits for max6695/96 */
> > +};
> > +
> > +static const struct lm90_params lm90_params[] = {
> > +	[adm1032] = {
> > +		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> > +		.alert_alarms = 0x7c,
> > +	},
> > +	[adt7461] = {
> > +		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> > +		.alert_alarms = 0x7c,
> > +	},
> > +	[lm86] = {
> > +		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> > +		.alert_alarms = 0x7b,
> > +	},
> > +	[lm90] = {
> > +		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> > +		.alert_alarms = 0x7b,
> > +	},
> > +	[lm99] = {
> > +		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> > +		.alert_alarms = 0x7b,
> > +	},
> > +	[max6646] = {
> > +		.flags = LM90_HAVE_LOCAL_EXT,
> > +		.alert_alarms = 0x7c,
> > +	},
> > +	[max6657] = {
> > +		.flags = LM90_HAVE_LOCAL_EXT,
> > +		.alert_alarms = 0x7c,
> > +	},
> > +	[max6659] = {
> > +		.flags = LM90_HAVE_LOCAL_EXT | LM90_HAVE_EMERGENCY,
> > +		.alert_alarms = 0x7c,
> > +	},
> > +	[max6680] = {
> > +		.flags = LM90_HAVE_OFFSET,
> > +		.alert_alarms = 0x7c,
> > +	},
> > +	[max6696] = {
> > +		.flags = LM90_HAVE_LOCAL_EXT | LM90_HAVE_EMERGENCY
> > +		  | LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3,
> > +		.alert_alarms = 0x187c,
> > +	},
> > +	[w83l771] = {
> > +		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> > +		.alert_alarms = 0x7c,
> > +	},
> > +};
> > +
> > +/*
> >   * Client data (each client gets its own)
> >   */
> >  
> > @@ -199,7 +256,7 @@ struct lm90_data {
> >  	char valid; /* zero until following fields are valid */
> >  	unsigned long last_updated; /* in jiffies */
> >  	int kind;
> > -	int flags;
> > +	u32 flags;
> >  
> >  	u8 config_orig;		/* Original configuration register value */
> >  	u16 alert_alarms;	/* Which alarm bits trigger ALERT# */
> > @@ -1201,39 +1258,10 @@ static int lm90_probe(struct i2c_client *new_client,
> >  
> >  	/* Different devices have different alarm bits triggering the
> >  	 * ALERT# output */
> > -	switch (data->kind) {
> > -	case lm90:
> > -	case lm99:
> > -	case lm86:
> > -		data->alert_alarms = 0x7b;
> > -		break;
> > -	case max6696:
> > -		data->alert_alarms = 0x187c;
> > -		break;
> > -	default:
> > -		data->alert_alarms = 0x7c;
> > -		break;
> > -	}
> > +	data->alert_alarms = lm90_params[data->kind].alert_alarms;
> >  
> >  	/* Set chip capabilities */
> > -	if (data->kind != max6657 && data->kind != max6659
> > -	    && data->kind != max6646 && data->kind != max6696)
> > -		data->flags |= LM90_HAVE_OFFSET;
> > -
> > -	if (data->kind = max6657 || data->kind = max6659
> > -	    || data->kind = max6646 || data->kind = max6696)
> > -		data->flags |= LM90_HAVE_LOCAL_EXT;
> > -
> > -	if (data->kind != max6657 && data->kind != max6659
> > -	    && data->kind != max6646 && data->kind != max6680
> > -	    && data->kind != max6696)
> > -		data->flags |= LM90_HAVE_REM_LIMIT_EXT;
> > -
> > -	if (data->kind = max6659 || data->kind = max6696)
> > -		data->flags |= LM90_HAVE_EMERGENCY;
> > -
> > -	if (data->kind = max6696)
> > -		data->flags |= LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3;
> > +	data->flags = lm90_params[data->kind].flags;
> >  
> >  	/* Initialize the LM90 chip */
> >  	lm90_init_client(new_client);
> 
> I like this change a lot, even though it makes the binary module a
> little larger. This is a lot cleaner. Consider it applied.
> 
It makes it a bit larger, but I figured we might make up for that later on.
And, yes, it _is_ cleaner.

Do you want me to change the flags to u16, or keep as is ?

Thanks,
Guenter

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

Re: [lm-sensors] [PATCH v2 1/3] hwmon: (lm90) Introduce chip

[Guenter Roeck]

On Wed, Oct 06, 2010 at 11:09:30AM -0400, Jean Delvare wrote:
> On Wed, 6 Oct 2010 17:05:53 +0200, Jean Delvare wrote:
> > I like this change a lot, even though it makes the binary module a
> > little larger.
> 
> Err, scratch that, it doesn't. Not sure why I thought it did...
> 
At least it makes the source a bit larger.

One thought: Would it make sense to use initdata for the initialization code ?

Guenter

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

Re: [lm-sensors] [PATCH v2 1/3] hwmon: (lm90) Introduce chip

[Jean Delvare]

On Wed, 6 Oct 2010 08:30:21 -0700, Guenter Roeck wrote:
> On Wed, Oct 06, 2010 at 11:09:30AM -0400, Jean Delvare wrote:
> > On Wed, 6 Oct 2010 17:05:53 +0200, Jean Delvare wrote:
> > > I like this change a lot, even though it makes the binary module a
> > > little larger.
> > 
> > Err, scratch that, it doesn't. Not sure why I thought it did...
> > 
> At least it makes the source a bit larger.
> 
> One thought: Would it make sense to use initdata for the initialization code ?

Almost impossible for I2C device drivers, as you never know when
probe() will be called. For example, lm90 could be built into the
kernel, but the underlying i2c bus driver be loaded as a module...

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH v2 1/3] hwmon: (lm90) Introduce chip

[Jean Delvare]

On Wed, 6 Oct 2010 08:28:37 -0700, Guenter Roeck wrote:
> Hi Jean,
> 
> On Wed, Oct 06, 2010 at 11:05:53AM -0400, Jean Delvare wrote:
> > Hi Guenter,
> > 
> > On Tue, 5 Oct 2010 08:38:26 -0700, Guenter Roeck wrote:
> > > Instead of using switch/case and if statements in probe, define chip specific
> > > functionality in a parameter structure array.
> > > 
> > > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > > ---
> > >  drivers/hwmon/lm90.c |   92 ++++++++++++++++++++++++++++++++-----------------
> > >  1 files changed, 60 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > > index 302d9eb..9df08e1 100644
> > > --- a/drivers/hwmon/lm90.c
> > > +++ b/drivers/hwmon/lm90.c
> > > @@ -190,6 +190,63 @@ static const struct i2c_device_id lm90_id[] = {
> > >  MODULE_DEVICE_TABLE(i2c, lm90_id);
> > >  
> > >  /*
> > > + * chip type specific parameters
> > > + */
> > > +struct lm90_params {
> > > +	u32 flags;		/* Capabilities */
> > 
> > Any reason why you don't use u16? there aren't that many flags yet, and
> > u16 divides the size of lm90_params[] below by 2.
> > 
> Access to 32 bit variables is a bit faster on many CPUs, due to the bit masking
> required otherwise.

... but alert_alarms is a u16, so apparently speed wasn't considered
(quite rightly so IMHO, as this is initialization data only read once.)

> Also, the flags field in lm90_data was int and I changed
> it to u32 (seemed to be cleaner for a bitfield). Seemed to make sense to use
> the same type for both.

This is a much better reason :)

> Not that it matters much here, if at all, since this is a bit mask anyway.
> I am fine either way.

Indeed...

> (...)
> Do you want me to change the flags to u16, or keep as is ?

I'll keep it as is. I just noticed that the following patch makes my
struct size argument moot anyway.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH v2 1/3] hwmon: (lm90) Introduce chip

[Guenter Roeck]

On Wed, Oct 06, 2010 at 12:10:35PM -0400, Jean Delvare wrote:
> On Wed, 6 Oct 2010 08:30:21 -0700, Guenter Roeck wrote:
> > On Wed, Oct 06, 2010 at 11:09:30AM -0400, Jean Delvare wrote:
> > > On Wed, 6 Oct 2010 17:05:53 +0200, Jean Delvare wrote:
> > > > I like this change a lot, even though it makes the binary module a
> > > > little larger.
> > > 
> > > Err, scratch that, it doesn't. Not sure why I thought it did...
> > > 
> > At least it makes the source a bit larger.
> > 
> > One thought: Would it make sense to use initdata for the initialization code ?
> 
> Almost impossible for I2C device drivers, as you never know when
> probe() will be called. For example, lm90 could be built into the
> kernel, but the underlying i2c bus driver be loaded as a module...
> 
Good point.

Guenter

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

Re: [lm-sensors] [PATCH v2 1/3] hwmon: (lm90) Introduce chip

[Jean Delvare]

On Wed, 6 Oct 2010 08:28:37 -0700, Guenter Roeck wrote:
> Access to 32 bit variables is a bit faster on many CPUs, due to the bit masking
> required otherwise.

Oh, and for completeness: more compact structures are better for
L1 caching. This can easily compensate for the extra CPU operations
required to access individual fields.


-- 
Jean Delvare

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