Skip to contentSkip to conversation

max6875: new i2c device driver

PATCH New [email protected] 2 participants 5 messages Jun 3, 2005 – Jun 24, 2005 I2C

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 max6875: new i2c device driver

This patch adds support for the MAX6875/MAX6874 chips.

Signed-off-by Ben Gardner <[email protected]>

-------------- next part --------------

drivers/i2c/chips/Kconfig +12 -0
diff -Nuar linux-2.6.12-rc5-mm2-clean/drivers/i2c/chips/Kconfig linux-2.6.12-rc5-mm2-max6875/drivers/i2c/chips/Kconfig
@@ -498,4 +498,16 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called m41t00.
 
+config SENSORS_MAX6875
+	tristate "MAXIM MAX6875 Power supply supervisor"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the MAX6875
+	  EEPROM-Programmable, Hex/Quad, Power-Suppy Sequencers/Supervisors.
+	  
+          This provides a interface to program the EEPROM and reset the chip.
+	  
+	  This driver can also be built as a module.  If so, the module
+	  will be called max6875.
+
 endmenu
drivers/i2c/chips/Makefile +1 -0
diff -Nuar linux-2.6.12-rc5-mm2-clean/drivers/i2c/chips/Makefile linux-2.6.12-rc5-mm2-max6875/drivers/i2c/chips/Makefile
@@ -32,6 +32,7 @@
 obj-$(CONFIG_SENSORS_LM90)	+= lm90.o
 obj-$(CONFIG_SENSORS_LM92)	+= lm92.o
 obj-$(CONFIG_SENSORS_MAX1619)	+= max1619.o
+obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
 obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
drivers/i2c/chips/max6875.c +473 -0

473 lines not on this page · raw message

Documentation/i2c/chips/max6875 +53 -0

53 lines not on this page · raw message

#1 lore raw

PATCH 1/1 max6875: new i2c device driver

Hi Ben,

Quoted 1 line

This patch adds support for the MAX6875/MAX6874 chips.

Sorry for the long delay. I finally took some time this afternoon to review your driver. It's a bit late as Greg already pushed it to Linus, but nevertheless, I'd appreciate it if you could provide an incremental patch addressing the issues I found.

I also have suggestions for the documentation and the Kconfig help text, I'll submit patches for that right after.

Quoted 1 line

#include <linux/sched.h>

I don't think you need this one. eeprom.c has it for capable() but you don't use it here.

Quoted 1 line

static int allow_write = 0;

Do not explicitely initialize static variables to 0.

Quoted 4 lines

/* CONFIG EEPROM is at addresses 0x8000 - 0x8045, registers are at 0 - 0x45 */
#define CONFIG_EEPROM_BASE 0x8000
#define CONFIG_EEPROM_SIZE 0x0046
#define CONFIG_EEPROM_SLICES 5

These could conflict with future Linux configuration (Kconfig) settings. You shouldn't prefix your own defines with CONFIG_.

As a side note, I am not convinced that you have any benefit in using the slice approach here. It was convenient for the eeprom driver due to the various use it has and the overall simplicity of the driver, but in your case it makes the code significantly more complex and the detection step significantly slower as well. What are the typical accesses to the interface files in your case?

Quoted 3 lines

#define MAX6875_CMD_BLOCK_WRITE 0x83
(...)
#define MAX6875_CMD_REBOOT 0x88

Why define these since you don't use them anywhere?

Quoted 1 line
	u8 rdbuf[SLICE_SIZE];

Why do you need this temporary buffer?

Quoted 2 lines
	    (jiffies - blk->updated[slice] > 300 * HZ) ||
	    (jiffies < blk->updated[slice])) {

Please use time_after() like all the other i2c chip drivers now do.

Quoted 3 lines
				dev_dbg(&client->dev, "max6875 register select has failed!\n");
(...)
				dev_dbg(&client->dev, "max6875 address set has failed!\n");

This should probably be error messages rather than a debug messages.

Quoted 7 lines
		if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
			if (i2c_smbus_read_i2c_block_data(client, MAX6875_CMD_BLOCK_READ,
							  rdbuf) != SLICE_SIZE)
			{
				retval = -1;
				goto exit;
			}

Misplaced parentheses.

Quoted 5 lines
				if (j < 0)
				{
					retval = -1;
					goto exit;
				}

Misplaced parentheses.

Quoted 1 line
				blk->data[(slice << SLICE_BITS) + i] = (u8) j;

The cast is not needed.

Quoted 1 line

exit:

Labels should be left aligned.

Quoted 6 lines

static ssize_t max6875_user_read(struct kobject *kobj, char *buf, loff_t off, size_t count)

{
	return max6875_read(kobj, buf, off, count, max6875_eeprom_user);
}

static ssize_t max6875_config_read(struct kobject *kobj, char *buf, loff_t off, size_t count)

Quoted 8 lines
{
	return max6875_read(kobj, buf, off, count, max6875_eeprom_config);
}

static ssize_t max6875_cfgreg_read(struct kobject *kobj, char *buf, loff_t off, size_t count)

{
	return max6875_read(kobj, buf, off, count, max6875_register_config);
}

Please look into the new callback capabilities introduced recently by Yani Ioannou. Examples can be found in the adm1026, it87, lm63, lm83 and lm90 drivers. Basically, this would ley you get rid of these multiple callback functions, all three files would have the same callback with an additional argument. Same for the write callbacks, of course. Maybe you'll have to add some code by yourself if the binary file case isn't handled yet.

Quoted 2 lines
	if (down_interruptible(&data->update_lock))
		return -EAGAIN;

Aren't you supposed to include <asm/semaphore.h> for this one?

Quoted 7 lines
		for (sent = 0; sent < count; sent++) {
			addr = off + sent;
			if (addr = 0x44)
				continue;

			retval = i2c_smbus_write_byte_data(client, addr, buf[sent]);
		}

You should interrupt the loop if retval < 0. With the current code, errors could easily go unnoticed.

Quoted 1 line
			val = (addr & 0xff) | (buf[sent] << 8);	// reversed

No C++-like comments please.

Quoted 2 lines
			if (addr = 0x8044)
				continue;

You could check this earlier.

Quoted 3 lines
	/* Invalidate the scratch buffer */
	for (slice = (off >> SLICE_BITS); slice <= ((off + count - 1) >> SLICE_BITS); slice++)
		blk->valid &= ~(1 << slice);

Why don't you rather copy the freshly written data to the cache?

Quoted 1 line

error_exit:

Align to left.

Quoted 3 lines

if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_READ_BYTE_DATA |
I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))

The check isnt't complete, as you use i2c_smbus_write_word_data later. OTOH you don't actually seem to use i2c_smbus_read_byte_data, so why check for its availability?

I also notice that you didn't implement block write although the chip appears to support it. This would probably improve the write speed quite significantly, and I2C_FUNC_SMBUS_WRITE_BLOCK is much more widely available on bus masters than I2C_FUNC_SMBUS_READ_I2C_BLOCK.

Quoted 3 lines

/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet. But it allows us to access eeprom_{read,write}_value. */

Bogus comment, there is no such functions. BTW the comment is bogus in the eeprom driver as well, I'll fix it there.

Please add the 24RF08 corruption preventer quirk in your driver (see the eeprom driver). Every I2C driver in the 0x50-0x5f range should include it (until I finally rewrite the whole thing).

Quoted 9 lines
	/* create the sysfs eeprom files with the correct permissions */
	if (allow_write = 0) {
		user_eeprom_attr.attr.mode &= ~S_IWUGO;
		user_eeprom_attr.write = NULL;
		config_eeprom_attr.attr.mode &= ~S_IWUGO;
		config_eeprom_attr.write = NULL;
		config_register_attr.attr.mode &= ~S_IWUGO;
		config_register_attr.write = NULL;
	}

Sounds like an unsafe approach to me. Why not define the files without the write permission at first, and add it if and only if allow_write is non-zero?

Quoted 1 line
		dev_err(&client->dev, "Client deregistration failed, client not detached.\n");

Line too long, please split.

That's all as far as the code itself is concerned.

But in fact I am also wondering what the point of this driver is. I'm not happy with a binary file containing the full set of registers, this is awfully not convenient to use. Imagine the mess it would be if all i2c drivers would do the same... Binary files in sysfs are supposed to be an exception, not the rule.

If this is done this way because you simply flush a blob into the eeprom once as startup, then a userspace tool relying on i2c-dev would have been just as efficient, and more flexible.

If OTOH writing individual values to registers happens, then a real interface to the chip would be much better, with individual files mapping to individual registers. See how hardware monitoring drivers do. BTW, the MAX6875 has voltage sensing inputs, so it would somewhat fit in the hardware monitoring interface layer.

Thanks,

Signature · 1 line
Jean Delvare
author
#2 lore raw

PATCH 1/1 max6875: new i2c device driver

In reply to [email protected]

Hi Jean,

First off, thanks for taking the time to review the code.

[...]

Quoted 1 line

What are the typical accesses to the interface files in your case?

The configuration is written once at board-level check-out. This consists of about 20 EEPROM writes scattered across the 70 registers.

The user EEPROM, which contains the serial/model numbers, is written at top-level check-out.
(That's when the board is put in the nice black box with a nice stamped aluminum serial number plate.)

After that, the user EEPROM is read at power-up.
The configuration stuff is never accessed again.

[...]

Quoted 2 lines

But in fact I am also wondering what the point of this driver is. I'm not happy with a binary file containing the full set of registers,

this

Quoted 5 lines

is awfully not convenient to use. Imagine the mess it would be if all i2c drivers would do the same... Binary files in sysfs are supposed to be an exception, not the rule.

If this is done this way because you simply flush a blob into the

eeprom

Quoted 2 lines

once as startup, then a userspace tool relying on i2c-dev would have been just as efficient, and more flexible.

I have to agree with you there.
I had to write a tool to program the device anyway, so it would be trivial to use i2c-dev instead of the sysfs files.

Would it be acceptable to axe register access entirely and only provide an interface to the user EEPROM?
I would be happy with read-only access to the EEPROM.

Thanks,
Ben

#3 lore raw

PATCH 1/1 max6875: new i2c device driver

In reply to [email protected]

Hi Ben,

Quoted 6 lines

I had to write a tool to program the device anyway, so it would be trivial to use i2c-dev instead of the sysfs files.

Would it be acceptable to axe register access entirely and only provide an interface to the user EEPROM?
I would be happy with read-only access to the EEPROM.

Yup, sounds sane to me. This will make the driver much more simple and smaller, which can't hurt.

Thanks,
--
Jean Delvare

#4 lore raw

PATCH 1/1 max6875: new i2c device driver

In reply to [email protected]

Hi again Ben,

One more thing about the max6875 driver. After reading the datasheet, I think I understand that each chip will respond to two I2C addresses (0x50+0x51 or 0x52+0x53). Your driver should allocate a subclient for the second address (see the asb100 driver for an example) so as to prevent another driver (e.g. eeprom) from requesting it and cause a conflict. Also, this should give you the possibility to improve your device identification procedure: if the same register or EEPROM location read from both addresses return different results, this certainly is a misdetection. And the sole fact of ensuring that the other address is responsive will avoid some misdetections too.

Thanks,
--
Jean Delvare