Re: [PATCH 2.6] I2C: New chip driver: sis5595

From: Alexey Dobriyan
Date: Tue Feb 01 2005 - 08:15:32 EST


On Tuesday 01 February 2005 13:49, Jean Delvare wrote:

> > Maybe you should call sis5595_update_device() in initialization function
> > and get rid of "value" field. It's sole purpose to fill "struct sis5595"
> > when it's known that "last_updated" field contains crap.

> Doesn't work. If you discard the "valid" field and call the update
> function at init time, you cannot be sure that the update function will
> actually do something. It all depends on the jiffies value relative to
> last_updated (0 at this point). This is exactly what "valid" is there
> for.

What about making sis5595_update_device() a simple jiffies-related wrapper
around function that updates "struct sis5595" unconditionally. I'm not sure
I plugged sis5595_do_update_client right, but you'll get the idea.

Alexey

--- linux-2.6.11-rc2-bk9-i2c/drivers/i2c/chips/sis5595.c.orig 2005-02-01 15:34:43.000000000 +0200
+++ linux-2.6.11-rc2-bk9-i2c/drivers/i2c/chips/sis5595.c 2005-02-01 15:58:39.000000000 +0200
@@ -184,7 +184,6 @@ struct sis5595_data {
struct semaphore lock;

struct semaphore update_lock;
- char valid; /* !=0 if following fields are valid */
unsigned long last_updated; /* In jiffies */
char maxins; /* == 3 if temp enabled, otherwise == 4 */
u8 revision; /* Reg. value */
@@ -209,6 +208,7 @@ static int sis5595_detach_client(struct

static int sis5595_read_value(struct i2c_client *client, u8 register);
static int sis5595_write_value(struct i2c_client *client, u8 register, u8 value);
+static void sis5595_do_update_client(struct i2c_client *client);
static struct sis5595_data *sis5595_update_device(struct device *dev);
static void sis5595_init_client(struct i2c_client *client);
static int sis5595_find_sis(int *address);
@@ -586,7 +586,6 @@ int sis5595_detect(struct i2c_adapter *a
/* Fill in the remaining client fields and put it into the global list */
strlcpy(new_client->name, "sis5595", I2C_NAME_SIZE);

- data->valid = 0;
init_MUTEX(&data->update_lock);

/* Tell the I2C layer a new client has arrived */
@@ -691,56 +690,52 @@ static void sis5595_init_client(struct i
if (!(config & 0x01))
sis5595_write_value(client, SIS5595_REG_CONFIG,
(config & 0xf7) | 0x01);
+ sis5595_do_update_client(client);
}

-static struct sis5595_data *sis5595_update_device(struct device *dev)
+static void sis5595_do_update_client(struct i2c_client *client)
{
- struct i2c_client *client = to_i2c_client(dev);
struct sis5595_data *data = i2c_get_clientdata(client);
int i;

down(&data->update_lock);
-
- if ((jiffies - data->last_updated > HZ + HZ / 2) ||
- (jiffies < data->last_updated) || !data->valid) {
-
- for (i = 0; i <= data->maxins; i++) {
- data->in[i] =
- sis5595_read_value(client, SIS5595_REG_IN(i));
- data->in_min[i] =
- sis5595_read_value(client,
- SIS5595_REG_IN_MIN(i));
- data->in_max[i] =
- sis5595_read_value(client,
- SIS5595_REG_IN_MAX(i));
- }
- for (i = 0; i < 2; i++) {
- data->fan[i] =
- sis5595_read_value(client, SIS5595_REG_FAN(i));
- data->fan_min[i] =
- sis5595_read_value(client,
- SIS5595_REG_FAN_MIN(i));
- }
- if(data->maxins == 3) {
- data->temp =
- sis5595_read_value(client, SIS5595_REG_TEMP);
- data->temp_over =
- sis5595_read_value(client, SIS5595_REG_TEMP_OVER);
- data->temp_hyst =
- sis5595_read_value(client, SIS5595_REG_TEMP_HYST);
- }
- i = sis5595_read_value(client, SIS5595_REG_FANDIV);
- data->fan_div[0] = (i >> 4) & 0x03;
- data->fan_div[1] = i >> 6;
- data->alarms =
- sis5595_read_value(client, SIS5595_REG_ALARM1) |
- (sis5595_read_value(client, SIS5595_REG_ALARM2) << 8);
- data->last_updated = jiffies;
- data->valid = 1;
+ for (i = 0; i <= data->maxins; i++) {
+ data->in[i] = sis5595_read_value(client, SIS5595_REG_IN(i));
+ data->in_min[i] = sis5595_read_value(client,
+ SIS5595_REG_IN_MIN(i));
+ data->in_max[i] = sis5595_read_value(client,
+ SIS5595_REG_IN_MAX(i));
}
-
+ for (i = 0; i < 2; i++) {
+ data->fan[i] = sis5595_read_value(client, SIS5595_REG_FAN(i));
+ data->fan_min[i] = sis5595_read_value(client,
+ SIS5595_REG_FAN_MIN(i));
+ }
+ if (data->maxins == 3) {
+ data->temp = sis5595_read_value(client, SIS5595_REG_TEMP);
+ data->temp_over = sis5595_read_value(client,
+ SIS5595_REG_TEMP_OVER);
+ data->temp_hyst = sis5595_read_value(client,
+ SIS5595_REG_TEMP_HYST);
+ }
+ i = sis5595_read_value(client, SIS5595_REG_FANDIV);
+ data->fan_div[0] = (i >> 4) & 0x03;
+ data->fan_div[1] = i >> 6;
+ data->alarms = sis5595_read_value(client, SIS5595_REG_ALARM1) |
+ (sis5595_read_value(client, SIS5595_REG_ALARM2) << 8);
+ data->last_updated = jiffies;
up(&data->update_lock);
+}
+
+static struct sis5595_data *sis5595_update_device(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct sis5595_data *data = i2c_get_clientdata(client);

+ if ((jiffies - data->last_updated > HZ + HZ / 2) ||
+ (jiffies < data->last_updated)) {
+ sis5595_do_update_client(client);
+ }
return data;
}

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/