Thank you for your comments.
On Mon, 2004-04-12 at 18:17, Jean Delvare wrote:
> > Patch against 2.4 kernel.
> > Simple temperature sensor chip.
> > Implemented read temperature and write low/high values.
> > Tested on UP ppc32.
> >
> > Please review and commit.
>
> We don't take patches for the 2.4 kernel. Marcelo doesn't want i2c
> patches from us anymore.
Closed circle...
> Either go and see with Marcelo directly (good luck) or submit your
> driver as a patch against our CVS lm_sensors2 repository.
Will do when all technical questions will be resolved.
I don't think sending it to Marcelo has any sense.
> A few comments BTW:
>
> First of all: which MAX663x chips is the driver for? MAX6629-MAX6632 are
> non-I2C chips according to Maxim-IC, so I guess it's MAX6633-MAX6635?
MAX663x == any max663*. Maxim-IC produces only MAX6633-MAX6635 devices
matching this regexp.
> You better name your driver max6633.c (or max6634.c, since that's the
> chip I suspect you are actually working with) then. We don't much like
> "x" in the driver names, they tend to be confusing (you may know which
> values of "x" it implies, but the newcomer doesn't). And list the
> supported chips in the header. See w83781d.c for a nice example.
>
> > + * The GNU GPL is contained in /usr/doc/copyright/GPL on a Debian
> > + * system and in the file COPYING in the Linux kernel source.
>
> This doesn't belong to kernel code IMHO.
It does.
> > +static struct i2c_driver max663x_driver = {
> > + .name = "max663x",
> > + .flags = I2C_DF_NOTIFY,
> > + .id = 1234,
>
> No. Just don't give it an ID if you don't need one.
Done.
> > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
> > + {
> > + printk(KERN_ERR "Adapter doesn't support functionality.\n");
> > + goto error1;
> > + }
>
> That's not an error. As you load an i2c chip driver, all adapters will
> be scanned. This may include adapters that don't support these
> functionalities (and so they just can't have a MAX6633 on them). I
> wouldn't even print a message.
What about situation when chip is connected to unsupported adapter or
adapter which doesn't provide needed functionality?
Any reading/writing will confuse bus and reader/writer.
I think chip should not work if it is not connected to the right
adapter.
> > + new_client->id = 123;
>
> No. The other drivers have a global, static variable for that. Just do
> the same.
Done.
> I notice that your driver has no detection mechanism at all. I agree
> that the MAX6634 doesn't look like a chip that want to be identified (no
> manufacturer ID registers, no chip ID register, no die/revision
> registers...) but there is a common trick of checking for unused bits,
> which always return 0. There are two of them in the configuration
> register, and 7 more in each temperature limit register. 30 zero bits
> verified is far better than no check at all.
One may not rely on control register.
One of MAX663xes here has 1 in it's "reserved" field but working
absolutely right.
I have done "identification" based on reserved bits in MAX, LOW, HIGH
and HYST registers.
> Also, since it seems that the address pointer registers only has 3 used
> bits, I would expect values to cycle every 8 registers. See lm75.c as an
> example. Could you please provide an i2cdump of the chip for
> confirmation (both byte and word modes)? I would then add support for
> the MAX6633/4/5 and LM76 (which seems to be compatible, but has
> different unused bits) to our sensors-detect script.
No, I can't.
ls /dev/i2c/
-------------- dir /dev/i2c/ ----------
0 .. .
MSG[14]:ls /dev/i2c/
/bin/i2cdump 0 0x4f b
New process started PID=13
file: /bin/i2cdump
MSG[23]:/bin/i2cdumpfile: /bin/i2cdump
WARNING! This program can confuse your I2C bus, cause data loss and
worse!
I will probe file /dev/i2c/0, address 0x4f, mode byte
You have five seconds to reconsider and press CTRL-C!
ls /proc/13/
scandir("/proc/13/")No such file or directory
MSG[14]:ls /proc/13/
Without explicit fflush() one can't read output just after error with
our embedded init.
Or may be i2cdump doesn't like big endian ppc32 and cross-compilation.
Don't know where exit happens in i2cdump.
> > + __max663x_temp((unsigned long)new_client);
>
> The function that reads values is usually called update. If is also
> supposed to cache the read data for a reasonable amount of time
> (typically 1.5 second) and it has a protecting lock. And it is usually
> not called at load time (although I admit we are reconsidering in some
> cases at the moment).
Done.
Actually there is race here, since update may only happen in process
context but any userspace reading is not protected.
Now update_lock is using for protecting max663x_data when writing into
it.
> > + printk("Found max663x I2C temperature sensor.\n");
>
> KERN_INFO or KERN_DEBUG missing.
Done.
> > +static inline int reg2temp(uint16_t temp)
> > +{
> > + if (temp & 0x8000)
> > + {
> > + return (255 - (temp >> 7));
> > + }
> > + else
> > + {
> > + return (temp >> 7);
> > + }
> > +}
> > +
> > +static inline uint16_t temp2reg(int temp)
> > +{
> > + uint16_t reg;
> > +
> > + if (temp > 255)
> > + temp = 255;
> > + if (temp < -255)
> > + temp = -255;
> > +
> > + if (temp < 0)
> > + {
> > + reg = temp + 255;
> > + reg <<= 7;
> > + reg |= 0x8000;
> > + }
> > + else
> > + {
> > + reg = (temp << 7);
> > + }
> > +
> > + return reg;
> > +}
>
> These are usually defined as macros at the top of the file, with nice
> names. See exiting drivers for examples of what we expect you to do. It
> is important that all drivers follow a few common guidelines so that
> maintaining the whole thing (49 chip drivers so far!) is possible.
Done.
> > +void max663x_temp(struct i2c_client *client, int operation, int ctl_name, int *nrels_mag, long *results)
> > +{
> > + struct max663x_data *data = client->data;
> > +
> > + if (operation == SENSORS_PROC_REAL_INFO)
> > + *nrels_mag = 1;
> > + else if (operation == SENSORS_PROC_REAL_READ)
> > + {
> > + __max663x_temp((unsigned long)client);
> > +
> > + results[0] = reg2temp(data->temp_max);
> > + results[1] = reg2temp(data->temp_hyst);
> > + results[2] = reg2temp(data->temp);
> > + *nrels_mag = 3;
> > + }
> > + else if (operation == SENSORS_PROC_REAL_WRITE)
> > + {
> > + if (*nrels_mag >= 1)
> > + {
> > + data->temp_max = temp2reg(results[0]);
> > + i2c_smbus_write_word_data(client, MAX663x_HIGH, swap_bytes(data->temp_high));
> > + }
> > + if (*nrels_mag >= 2)
> > + {
> > + data->temp_max = temp2reg(results[1]);
> > + i2c_smbus_write_word_data(client, MAX663x_LOW, swap_bytes(data->temp_low));
> > + }
> > + }
> > +}
>
> That's odd. Writing to the file doesn't set the same limits as the ones
> returned when reading the files. Confusing to say the least. And this
> also means that some limits cannot be read (low, high), and others
> cannot be set (max, hyst)? Oh well, the write part of your function, is
> plain broken anyway.
>
> Either have a 5-value file and put everything in it (the lm80 and lm92
> drivers do that, you would follow the order of lm92 (although the rest
> of the driver is *not* a good example for you to follow)) or put, say,
> max and hyst current value in the "temp" file and have one or two other
> files for high and low.
Done.
now one can read 5 values and write 4 values:
read: max, hyst, low, high, temp
write: max, hyst, low, high.
> > +static void __exit max663x_fini(void)
> > +{
> > + i2c_del_driver(&max663x_driver);
> > +}
>
> Nice to see a french word in kernel code, still _exit is the common
> suffix for that function.
Actually I always use _fini/_init suffixes but I don't insist.
Please review and comment.
If all design and technical issues are resloved I will make a patch
against lm_sensors tree.
Thank you for your comments.
--
Evgeniy Polaykov ( s0mbre )
Crash is better than data corruption. -- Art Grabowski
-------------- next part --------------
===== drivers/i2c/max663x.c 1.2 vs edited =====
--- 1.2/drivers/i2c/max663x.c Mon Apr 12 17:00:33 2004
+++ edited/drivers/i2c/max663x.c Mon Apr 12 20:23:03 2004
@@ -23,6 +23,10 @@
*
*/
+/*
+ * Driver for Maxim-IC MAX6633, MAX6634, MAX6635 temperature sensors.
+ */
+
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/module.h>
@@ -40,8 +44,9 @@
static int max663x_attach_adapter(struct i2c_adapter *);
static int max663x_detect(struct i2c_adapter *, int, unsigned short, int);
static int max663x_detach_client(struct i2c_client *);
-static void __max663x_temp(unsigned long);
+static void max663x_update(unsigned long);
void max663x_temp(struct i2c_client *, int, int, int *, long *);
+static int max663x_check(struct i2c_client *);
static ctl_table max663x_ctl_table[] =
@@ -57,6 +62,8 @@
SENSORS_INSMOD_1(max663x);
+static int max663x_id;
+
struct max663x_data {
struct semaphore update_lock;
char valid;
@@ -70,16 +77,35 @@
static struct i2c_driver max663x_driver = {
.name = "max663x",
.flags = I2C_DF_NOTIFY,
- .id = 1234,
.attach_adapter = max663x_attach_adapter,
.detach_client = max663x_detach_client,
};
+#define reg2temp(temp) ((temp & 0x8000)?(255 - (temp >> 7)):(temp >> 7))
+#define temp2reg(temp) ((temp < 0)?(((temp + 255) << 7) | 0x8000):(temp << 7))
+
+static __inline__ u16 swap_bytes(u16 val)
+{
+ return (val >> 8) | (val << 8);
+}
+
static int max663x_attach_adapter(struct i2c_adapter *adapter)
{
return i2c_detect(adapter, &addr_data, max663x_detect);
}
+static int max663x_check(struct i2c_client *client)
+{
+ u16 temp_low, temp_high, temp_max, temp_hyst;
+
+ temp_low = swap_bytes(i2c_smbus_read_word_data(client, MAX663x_LOW));
+ temp_high = swap_bytes(i2c_smbus_read_word_data(client, MAX663x_HIGH));
+ temp_hyst = swap_bytes(i2c_smbus_read_word_data(client, MAX663x_HYST));
+ temp_max = swap_bytes(i2c_smbus_read_word_data(client, MAX663x_MAX));
+
+ return (!(temp_low & 0x7f) && !(temp_high & 0x7f) && !(temp_hyst & 0x7f) && !(temp_max & 0x7f));
+}
+
static int max663x_detect(struct i2c_adapter *adapter, int address, unsigned short flags, int kind)
{
struct i2c_client *new_client = NULL;
@@ -90,6 +116,7 @@
if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
{
printk(KERN_ERR "Adapter doesn't support functionality.\n");
+ err = -EINVAL;
goto error1;
}
@@ -118,12 +145,20 @@
new_client->adapter = adapter;
new_client->driver = &max663x_driver;
new_client->flags = 0;
- new_client->id = 123;
+ new_client->id = max663x_id++;
strncpy(new_client->name, "max663x", sizeof(new_client->name));
data->valid = 0;
init_MUTEX(&data->update_lock);
+ if (!max663x_check(new_client))
+ {
+ printk(KERN_ERR "It doesn't look like MAX663x chip.\n");
+ err = -ENODEV;
+ goto error3;
+ }
+
+
err = i2c_attach_client(new_client);
if (err)
{
@@ -139,15 +174,16 @@
goto error4;
}
- __max663x_temp((unsigned long)new_client);
+ max663x_update((unsigned long)new_client);
- printk("Found max663x I2C temperature sensor.\n");
+ printk(KERN_INFO "Found max663x I2C temperature sensor.\n");
return 0;
error4:
i2c_detach_client(new_client);
error3:
+ max663x_id--;
kfree(new_client);
error2:
kfree(data);
@@ -172,57 +208,24 @@
return 0;
}
-static inline int reg2temp(uint16_t temp)
-{
- if (temp & 0x8000)
- {
- return (255 - (temp >> 7));
- }
- else
- {
- return (temp >> 7);
- }
-}
-
-static inline uint16_t temp2reg(int temp)
-{
- uint16_t reg;
-
- if (temp > 255)
- temp = 255;
- if (temp < -255)
- temp = -255;
-
- if (temp < 0)
- {
- reg = temp + 255;
- reg <<= 7;
- reg |= 0x8000;
- }
- else
- {
- reg = (temp << 7);
- }
-
- return reg;
-}
-
-static u16 swap_bytes(u16 val)
-{
- return (val >> 8) | (val << 8);
-}
-
-void __max663x_temp(unsigned long __data)
+void max663x_update(unsigned long __data)
{
struct i2c_client *client = (struct i2c_client *)__data;
struct max663x_data *data = client->data;
+ if (down_interruptible(&data->update_lock))
+ return;
+
data->temp = swap_bytes(i2c_smbus_read_word_data(client, MAX663x_TEMP));
data->control = i2c_smbus_read_byte_data(client, MAX663x_CONTROL);
data->temp_low = swap_bytes(i2c_smbus_read_word_data(client, MAX663x_LOW));
data->temp_high = swap_bytes(i2c_smbus_read_word_data(client, MAX663x_HIGH));
data->temp_hyst = swap_bytes(i2c_smbus_read_word_data(client, MAX663x_HYST));
data->temp_max = swap_bytes(i2c_smbus_read_word_data(client, MAX663x_MAX));
+
+ data->last_updated = jiffies;
+
+ up(&data->update_lock);
}
void max663x_temp(struct i2c_client *client, int operation, int ctl_name, int *nrels_mag, long *results)
@@ -233,24 +236,41 @@
*nrels_mag = 1;
else if (operation == SENSORS_PROC_REAL_READ)
{
- __max663x_temp((unsigned long)client);
+ max663x_update((unsigned long)client);
results[0] = reg2temp(data->temp_max);
results[1] = reg2temp(data->temp_hyst);
- results[2] = reg2temp(data->temp);
- *nrels_mag = 3;
+ results[2] = reg2temp(data->temp_low);
+ results[3] = reg2temp(data->temp_high);
+ results[4] = reg2temp(data->temp);
+ *nrels_mag = 5;
}
else if (operation == SENSORS_PROC_REAL_WRITE)
{
- if (*nrels_mag >= 1)
+ if (!down_interruptible(&data->update_lock))
{
- data->temp_max = temp2reg(results[0]);
- i2c_smbus_write_word_data(client, MAX663x_HIGH, swap_bytes(data->temp_high));
- }
- if (*nrels_mag >= 2)
- {
- data->temp_max = temp2reg(results[1]);
- i2c_smbus_write_word_data(client, MAX663x_LOW, swap_bytes(data->temp_low));
+ if (*nrels_mag >= 1)
+ {
+ data->temp_max = temp2reg(results[0]);
+ i2c_smbus_write_word_data(client, MAX663x_MAX, swap_bytes(data->temp_max));
+ }
+ if (*nrels_mag >= 2)
+ {
+ data->temp_hyst = temp2reg(results[1]);
+ i2c_smbus_write_word_data(client, MAX663x_HYST, swap_bytes(data->temp_hyst));
+ }
+ if (*nrels_mag >= 3)
+ {
+ data->temp_low = temp2reg(results[2]);
+ i2c_smbus_write_word_data(client, MAX663x_LOW, swap_bytes(data->temp_low));
+ }
+ if (*nrels_mag >= 4)
+ {
+ data->temp_high = temp2reg(results[3]);
+ i2c_smbus_write_word_data(client, MAX663x_HIGH, swap_bytes(data->temp_high));
+ }
+
+ up(&data->update_lock);
}
}
}
@@ -260,7 +280,7 @@
return i2c_add_driver(&max663x_driver);
}
-static void __exit max663x_fini(void)
+static void __exit max663x_exit(void)
{
i2c_del_driver(&max663x_driver);
}
@@ -271,4 +291,4 @@
MODULE_LICENSE("GPL");
module_init(max663x_init);
-module_exit(max663x_fini);
+module_exit(max663x_exit);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040412/7576eca0/attachment.bin
