[lm-sensors] [patch] ds2482: i2c to 1-wire bridge

[lm-sensors] [patch] ds2482: i2c to 1-wire bridge

[Ben Gardner]

This patch adds support for the Maxim DS2482-100 and DS2482-800 chips,
which are i2c devices that provide 1 or 8 1-wire masters.

I put the code under drivers/i2c/chips, but perhaps it should go under
drivers/w1.

Signed-off-by: Ben Gardner <bgardner@wabtec.com>
-------------- next part --------------
 drivers/i2c/chips/Kconfig  |   12 
 drivers/i2c/chips/Makefile |    1 
 drivers/i2c/chips/ds2482.c |  557 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 570 insertions(+)

Index: linux-2.6.15-rc3-mm1/drivers/i2c/chips/Makefile
=================================--- linux-2.6.15-rc3-mm1.orig/drivers/i2c/chips/Makefile
+++ linux-2.6.15-rc3-mm1/drivers/i2c/chips/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
 obj-$(CONFIG_SENSORS_RTC8564)	+= rtc8564.o
 obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
+obj-$(CONFIG_SENSORS_DS2482)	+= ds2482.o
 obj-$(CONFIG_TPS65010)		+= tps65010.o
 obj-$(CONFIG_RTC_X1205_I2C)	+= x1205.o
 
Index: linux-2.6.15-rc3-mm1/drivers/i2c/chips/ds2482.c
=================================--- /dev/null
+++ linux-2.6.15-rc3-mm1/drivers/i2c/chips/ds2482.c
@@ -0,0 +1,557 @@
+/**
+ * ds2482.c - provides i2c to w1-master bridge(s)
+ * Copyright (C) 2005  Ben Gardner <bgardner@wabtec.com>
+ *
+ * The DS2482 is a sensor chip made by Dallis Semiconductor (Maxim).
+ * It is a I2C to 1-wire bridge.
+ * There are two variations: -100 and -800, which have 1 or 8 1-wire ports.
+ * The complete datasheet can be obtained from MAXIM's website at:
+ *   http://www.maxim-ic.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <asm/delay.h>
+
+#include "../../w1/w1.h"
+#include "../../w1/w1_int.h"
+
+/**
+ * Address is selected using 2 pins, resulting in 4 possible addresses.
+ *  0x18, 0x19, 0x1a, 0x1b
+ * However, this chip is rare and should not be detected, so use the force
+ * module parameter.
+ */
+static unsigned short normal_i2c[] = {I2C_CLIENT_END};
+
+/**
+ * Insmod parameters
+ */
+I2C_CLIENT_INSMOD_1(ds2482);
+
+/**
+ * The DS2482 registers - there are 3 registers that are addressed by a read
+ * pointer. The read pointer is set by the last command executed.
+ *
+ * To read the data, issue a register read for any address
+ */
+#define DS2482_CMD_RESET               0xF0	/* No param */
+#define DS2482_CMD_SET_READ_PTR        0xE1	/* Param: DS2482_PTR_CODE_xxx */
+#define   DS2482_PTR_CODE_STATUS         0xF0
+#define   DS2482_PTR_CODE_DATA           0xE1
+#define   DS2482_PTR_CODE_CHANNEL        0xD2	/* DS2482-800 only */
+#define   DS2482_PTR_CODE_CONFIG         0xC3
+#define DS2482_CMD_CHANNEL_SELECT      0xC3	/* Param: Channel byte - DS2482-800 only */
+#define DS2482_CMD_WRITE_CONFIG        0xD2	/* Param: Config byte */
+#define DS2482_CMD_1WIRE_RESET         0xB4	/* Param: None */
+#define DS2482_CMD_1WIRE_SINGLE_BIT    0x87	/* Param: Bit byte (bit7) */
+#define DS2482_CMD_1WIRE_WRITE_BYTE    0xA5	/* Param: Data byte */
+#define DS2482_CMD_1WIRE_READ_BYTE     0x96	/* Param: None */
+/* Note to read the byte, Set the ReadPtr to Data then read (any addr) */
+#define DS2482_CMD_1WIRE_TRIPLET       0x78	/* Param: Dir byte (bit7) */
+
+/**
+ * Configure Register bit definitions
+ * The top 4 bits always read 0.
+ * To write, the top nibble must be the 1's compl. of the low nibble.
+ */
+#define DS2482_REG_CFG_1WS             0x08
+#define DS2482_REG_CFG_SPU             0x04
+#define DS2482_REG_CFG_PPM             0x02
+#define DS2482_REG_CFG_APU             0x01
+
+/* Write and verify codes for the CHANNEL_SELECT command (DS2482-800 only) */
+static u8 ds2482_chan_wr[8] = { 0xF0, 0xE1, 0xD2, 0xC3, 0xB4, 0xA5, 0x96, 0x87};
+static u8 ds2482_chan_rd[8] = { 0xB8, 0xB1, 0xAA, 0xA3, 0x9C, 0x95, 0x8E, 0x87};
+
+
+/**
+ * Status Register bit definitions (read only)
+ */
+#define DS2482_REG_STS_DIR             0x80
+#define DS2482_REG_STS_TSB             0x40
+#define DS2482_REG_STS_SBR             0x20
+#define DS2482_REG_STS_RST             0x10
+#define DS2482_REG_STS_LL              0x08
+#define DS2482_REG_STS_SD              0x04
+#define DS2482_REG_STS_PPD             0x02
+#define DS2482_REG_STS_1WB             0x01
+
+
+static int ds2482_attach_adapter(struct i2c_adapter *adapter);
+static int ds2482_detect(struct i2c_adapter *adapter, int address, int kind);
+static int ds2482_detach_client(struct i2c_client *client);
+
+
+/**
+ * Driver data (common to all clients)
+ */
+static struct i2c_driver ds2482_driver = {
+	.owner		= THIS_MODULE,
+	.name		= "ds2482",
+	.flags		= I2C_DF_NOTIFY,
+	.attach_adapter	= ds2482_attach_adapter,
+	.detach_client	= ds2482_detach_client,
+};
+
+/*
+ * Client data (each client gets its own)
+ */
+
+struct ds2482_data;
+
+struct ds2482_w1_chan {
+	struct ds2482_data	*pdev;
+	u8			channel;
+	struct w1_bus_master	w1_bus_master;
+};
+
+struct ds2482_data {
+	struct i2c_client	client;
+	struct semaphore	access_lock;
+
+	/* 1-wire interface(s) */
+	int			w1_count;	/* 1 or 8 */
+	struct ds2482_w1_chan	w1_ch[8];
+
+	/* per-device values */
+	u8			channel;
+	u8			read_prt;	/* see DS2482_PTR_CODE_xxx */
+	u8			reg_config;
+};
+
+
+/**
+ * Sets the read pointer.
+ * @param pdev       The ds2482 client pointer
+ * @param read_ptr   see DS2482_PTR_CODE_xxx above
+ * @return -1 on failure, 0 on success
+ */
+static inline int ds2482_select_register(struct ds2482_data *pdev, u8 read_ptr)
+{
+	if ( pdev->read_prt != read_ptr ) {
+		if ( i2c_smbus_write_byte_data(&pdev->client,
+					       DS2482_CMD_SET_READ_PTR,
+					       read_ptr) < 0 ) {
+			return(-1);
+		}
+		pdev->read_prt = read_ptr;
+	}
+	return(0);
+}
+
+/**
+ * Sends a command without a parameter
+ * @param pdev       The ds2482 client pointer
+ * @param cmd        DS2482_CMD_RESET,
+ *                   DS2482_CMD_1WIRE_RESET,
+ *                   DS2482_CMD_1WIRE_READ_BYTE
+ * @return -1 on failure, 0 on success
+ */
+static inline int ds2482_send_cmd(struct ds2482_data *pdev, u8 cmd)
+{
+	if ( i2c_smbus_write_byte(&pdev->client, cmd) < 0 ) {
+		return(-1);
+	}
+	pdev->read_prt = DS2482_PTR_CODE_STATUS;
+	return(0);
+}
+
+/**
+ * Sends a command with a parameter
+ * @param pdev       The ds2482 client pointer
+ * @param cmd        DS2482_CMD_WRITE_CONFIG,
+ *                   DS2482_CMD_1WIRE_SINGLE_BIT,
+ *                   DS2482_CMD_1WIRE_WRITE_BYTE,
+ *                   DS2482_CMD_1WIRE_TRIPLET
+ * @param byte       The data to send
+ * @return -1 on failure, 0 on success
+ */
+static inline int ds2482_send_cmd_data(struct ds2482_data *pdev, u8 cmd, u8 byte)
+{
+	if ( i2c_smbus_write_byte_data(&pdev->client, cmd, byte) < 0 ) {
+		return(-1);
+	}
+	/* all cmds leave in STATUS, except CONFIG */
+	pdev->read_prt = (cmd != DS2482_CMD_WRITE_CONFIG) ?
+			 DS2482_PTR_CODE_STATUS : DS2482_PTR_CODE_CONFIG;
+	return(0);
+}
+
+
+/*
+ * 1-Wire interface code
+ */
+
+
+/**
+ * Waits until the 1-wire interface is idle (not busy)
+ *
+ * @param pdev Pointer to the device structure
+ * @return the last value read from status or -1 (failure)
+ */
+static inline int ds2482_wait_1wire_idle(struct ds2482_data *pdev)
+{
+	int temp = -1;
+	if ( !ds2482_select_register(pdev, DS2482_PTR_CODE_STATUS) ) {
+		do {
+			temp = i2c_smbus_read_byte(&pdev->client);
+		} while ( (temp >= 0) && (temp & DS2482_REG_STS_1WB) );
+	}
+	return(temp);
+}
+
+/**
+ * Selects a w1 channel.
+ * The 1-wire interface must be idle before calling this function.
+ *
+ * @param pdev       The ds2482 client pointer
+ * @param channel    0-7
+ * @return           -1 (failure) or 0 (success)
+ */
+static inline int ds2482_set_channel(struct ds2482_data *pdev, u8 channel)
+{
+	if (channel >= 8)
+		return -1;
+
+	if ( i2c_smbus_write_byte_data(&pdev->client, DS2482_CMD_CHANNEL_SELECT,
+				       ds2482_chan_wr[channel]) < 0 )
+		return -1;
+
+	pdev->read_prt = DS2482_PTR_CODE_CHANNEL;
+	pdev->channel = -1;
+	if (i2c_smbus_read_byte(&pdev->client) = ds2482_chan_rd[channel]) {
+		pdev->channel = channel;
+		return 0;
+	}
+	return -1;
+}
+
+
+/**
+ * Performs the touch-bit function, which writes a 0 or 1 and reads the level.
+ *
+ * @param data       The ds2482 channel pointer
+ * @param bit        The level to write: 0 or non-zero
+ * @return           The level read: 0 or 1
+ */
+static u8 ds2482_w1_touch_bit(unsigned long data, u8 bit)
+{
+	struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
+	struct ds2482_data    *pdev = pchan->pdev;
+	int status = -1;
+
+	down(&pdev->access_lock);
+
+	/* Select the channel */
+	ds2482_wait_1wire_idle(pdev);
+	if (pdev->w1_count > 1) {
+		ds2482_set_channel(pdev, pchan->channel);
+	}
+
+	if (!ds2482_send_cmd_data(pdev, DS2482_CMD_1WIRE_SINGLE_BIT,
+                                  bit ? 0xFF : 0)) {
+		/* Wait until 1WB = 0, return the status */
+		status = ds2482_wait_1wire_idle(pdev);
+	}
+	up(&pdev->access_lock);
+
+	return((status & DS2482_REG_STS_SBR) ? 1 : 0);
+}
+
+/**
+ * Performs the triplet function, which reads two bits and writes a bit.
+ * The bit written is determined by the two reads:
+ *   00 => dbit, 01 => 0, 10 => 1
+ *
+ * @param data       The ds2482 channel pointer
+ * @param dbit       The direction to choose if both branches are valid
+ * @return           b0=read1 b1=read2 b3=bit written
+ */
+static u8 ds2482_w1_triplet(unsigned long data, u8 dbit)
+{
+	struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
+	struct ds2482_data    *pdev = pchan->pdev;
+	int status=3, result;
+
+	down(&pdev->access_lock);
+
+	/* Select the channel */
+	ds2482_wait_1wire_idle(pdev);
+	if (pdev->w1_count > 1) {
+		ds2482_set_channel(pdev, pchan->channel);
+	}
+
+	/* Send the triplet command */
+	if (!ds2482_send_cmd_data(pdev, DS2482_CMD_1WIRE_TRIPLET,
+                                  dbit ? 0xFF : 0)) {
+		/* Wait until 1WB = 0, grab the status */
+		status = ds2482_wait_1wire_idle(pdev);
+	}
+	up(&pdev->access_lock);
+
+	/* Decode the status */
+	result = (status >> 5);
+	return(result);
+}
+
+/**
+ * Performs the write byte function.
+ *
+ * @param data       The ds2482 channel pointer
+ * @param byte       The value to write
+ */
+static void ds2482_w1_write_byte(unsigned long data, u8 byte)
+{
+	struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
+	struct ds2482_data    *pdev = pchan->pdev;
+
+	down(&pdev->access_lock);
+
+	/* Select the channel */
+	ds2482_wait_1wire_idle(pdev);
+	if (pdev->w1_count > 1) {
+		ds2482_set_channel(pdev, pchan->channel);
+	}
+
+	/* Send the write byte command */
+	ds2482_send_cmd_data(pdev, DS2482_CMD_1WIRE_WRITE_BYTE, byte);
+
+	up(&pdev->access_lock);
+}
+
+/**
+ * Performs the read byte function.
+ *
+ * @param data       The ds2482 channel pointer
+ * @return           The value read
+ */
+static u8 ds2482_w1_read_byte(unsigned long data)
+{
+	struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
+	struct ds2482_data    *pdev = pchan->pdev;
+	int result;
+
+	down(&pdev->access_lock);
+
+	/* Select the channel */
+	ds2482_wait_1wire_idle(pdev);
+	if (pdev->w1_count > 1) {
+		ds2482_set_channel(pdev, pchan->channel);
+	}
+
+	/* Send the read byte command */
+	ds2482_send_cmd(pdev, DS2482_CMD_1WIRE_READ_BYTE);
+
+	/* Wait until 1WB = 0 */
+	ds2482_wait_1wire_idle(pdev);
+
+	/* Select the data register */
+	ds2482_select_register(pdev, DS2482_PTR_CODE_DATA);
+
+	/* Read the data byte */
+	result = i2c_smbus_read_byte(&pdev->client);
+
+	up(&pdev->access_lock);
+
+	return((u8)result);
+}
+
+
+/**
+ * Sends a reset on the 1-wire interface
+ *
+ * @param data    The ds2482 channel pointer
+ * @return        0Þvice present, 1=No device present or error
+ */
+static u8 ds2482_w1_reset_bus(unsigned long data)
+{
+	struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
+	struct ds2482_data    *pdev = pchan->pdev;
+	int err;
+	u8 retval = 1;
+
+	down(&pdev->access_lock);
+
+	/* Select the channel */
+	ds2482_wait_1wire_idle(pdev);
+	if (pdev->w1_count > 1) {
+		ds2482_set_channel(pdev, pchan->channel);
+	}
+
+	/* Send the reset command */
+	err = ds2482_send_cmd(pdev, DS2482_CMD_1WIRE_RESET);
+	if (err >= 0) {
+		/* Wait until the reset is complete */
+		err = ds2482_wait_1wire_idle(pdev);
+		retval = (err & DS2482_REG_STS_PPD) ? 0 : 1;
+
+		/* If the chip did reset since detect, re-config it */
+		if (err & DS2482_REG_STS_RST) {
+			ds2482_send_cmd_data(pdev, DS2482_CMD_WRITE_CONFIG,
+                                             0xF0);
+		}
+	}
+
+	up(&pdev->access_lock);
+
+	return(retval);
+}
+
+
+/**
+ * Called to see if the device exists on an i2c bus.
+ */
+static int ds2482_attach_adapter(struct i2c_adapter *adapter)
+{
+	return(i2c_probe(adapter, &addr_data, ds2482_detect));
+}
+
+
+/*
+ * The following function does more than just detection. If detection
+ * succeeds, it also registers the new chip.
+ */
+static int ds2482_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	struct ds2482_data *data;
+	struct i2c_client  *new_client;
+	int err = 0;
+	int temp1;
+	int idx;
+
+	if ( !i2c_check_functionality(adapter,
+				      I2C_FUNC_SMBUS_BYTE_DATA |
+				      I2C_FUNC_SMBUS_BYTE) ) {
+		goto exit;
+	}
+
+	if ( !(data = kmalloc(sizeof(struct ds2482_data), GFP_KERNEL)) ) {
+		err = -ENOMEM;
+		goto exit;
+	}
+	memset(data, 0, sizeof(struct ds2482_data));
+
+	new_client = &data->client;
+	i2c_set_clientdata(new_client, data);
+	new_client->addr = address;
+	new_client->adapter = adapter;
+	new_client->driver = &ds2482_driver;
+	new_client->flags = 0;
+
+	/* Reset the device (sets the read_ptr to status) */
+	if ( ds2482_send_cmd(data, DS2482_CMD_RESET) < 0 ) {
+		dev_dbg(&adapter->dev, "DS2482 reset failed at 0x%02x.\n",
+			address);
+		goto exit_free;
+	}
+
+	/* Sleep at least 525ns to allow the reset to complete */
+	ndelay(525);
+
+	/* Read the status byte - expect reset bit and line to be set */
+	temp1 = i2c_smbus_read_byte(new_client);
+	if (temp1 != (DS2482_REG_STS_LL | DS2482_REG_STS_RST)) {
+		dev_dbg(&adapter->dev, "DS2482 (0x%02x) reset status "
+			"0x%02X - not a DS2482\n", address, temp1);
+		goto exit_free;
+	}
+
+	/* Detect the 8-port version */
+	data->w1_count = 1;
+	if (ds2482_set_channel(data, 7) = 0) {
+		data->w1_count = 8;
+	}
+
+	/* Set all config items to 0 (off) */
+	ds2482_send_cmd_data(data, DS2482_CMD_WRITE_CONFIG, 0xF0);
+
+	/* We can fill in the remaining client fields */
+	sprintf(new_client->name, "ds2482-%d00", data->w1_count);
+
+	init_MUTEX(&data->access_lock);
+
+	/* Tell the I2C layer a new client has arrived */
+	if ( (err = i2c_attach_client(new_client)) )
+		goto exit_free;
+
+	/* Register 1-wire interface(s) */
+	for (idx = 0; idx < data->w1_count; idx++) {
+		data->w1_ch[idx].pdev = data;
+		data->w1_ch[idx].channel = idx;
+
+		/* Populate all the w1 bus master stuff */
+		data->w1_ch[idx].w1_bus_master.data = (unsigned long)&data->w1_ch[idx];
+		data->w1_ch[idx].w1_bus_master.read_byte  = ds2482_w1_read_byte;
+		data->w1_ch[idx].w1_bus_master.write_byte = ds2482_w1_write_byte;
+		data->w1_ch[idx].w1_bus_master.touch_bit  = ds2482_w1_touch_bit;
+		data->w1_ch[idx].w1_bus_master.triplet    = ds2482_w1_triplet;
+		data->w1_ch[idx].w1_bus_master.reset_bus  = ds2482_w1_reset_bus;
+
+		err = w1_add_master_device(&data->w1_ch[idx].w1_bus_master);
+		if ( err ) {
+			data->w1_ch[idx].pdev = NULL;
+			goto exit_w1_remove;
+		}
+	}
+
+	return(0);
+
+exit_w1_remove:
+	for (idx = 0; idx < data->w1_count; idx++) {
+		if (data->w1_ch[idx].pdev != NULL) {
+			w1_remove_master_device(&data->w1_ch[idx].w1_bus_master);
+		}
+	}
+exit_free:
+	kfree(data);
+exit:
+	return(err);
+}
+
+static int ds2482_detach_client(struct i2c_client *client)
+{
+	struct ds2482_data   *data = i2c_get_clientdata(client);
+	int err, idx;
+
+	/* Unregister the 1-wire bridge(s) */
+	for (idx = 0; idx < data->w1_count; idx++) {
+		if (data->w1_ch[idx].pdev != NULL) {
+			w1_remove_master_device(&data->w1_ch[idx].w1_bus_master);
+		}
+	}
+
+	/* Detach the i2c device */
+	if ( (err = i2c_detach_client(client)) ) {
+		dev_err(&client->dev,
+			"Client deregistration failed, client not detached.\n");
+		return(err);
+	}
+
+	/* Free the memory */
+	kfree(data);
+	return(0);
+}
+
+static int __init sensors_ds2482_init(void)
+{
+	return(i2c_add_driver(&ds2482_driver));
+}
+
+static void __exit sensors_ds2482_exit(void)
+{
+	i2c_del_driver(&ds2482_driver);
+}
+
+MODULE_AUTHOR("Ben Gardner <bgardner@wabtec.com>");
+MODULE_DESCRIPTION("DS2482 driver");
+MODULE_LICENSE("GPL");
+
+module_init(sensors_ds2482_init);
+module_exit(sensors_ds2482_exit);
Index: linux-2.6.15-rc3-mm1/drivers/i2c/chips/Kconfig
=================================--- linux-2.6.15-rc3-mm1.orig/drivers/i2c/chips/Kconfig
+++ linux-2.6.15-rc3-mm1/drivers/i2c/chips/Kconfig
@@ -135,4 +135,16 @@ config RTC_X1205_I2C
 	  This driver can also be built as a module. If so, the module
 	  will be called x1205.
 
+config SENSORS_DS2482
+	tristate "Maxim DS2482 I2C to 1-Wire bridge"
+	depends on I2C && EXPERIMENTAL
+	select W1
+	help
+	  If you say yes here you get support for the Maxim DS2482
+	  I2C to 1-Wire bridge.
+	  This, obviously, requires the 1-Wire subsystem (W1)
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ds2482.
+
 endmenu

[lm-sensors] Re: [patch] ds2482: i2c to 1-wire bridge

[Evgeniy Polyakov]

On Fri, Dec 02, 2005 at 09:34:22AM -0600, Ben Gardner (gardner.ben@gmail.com) wrote:
> This patch adds support for the Maxim DS2482-100 and DS2482-800 chips,
> which are i2c devices that provide 1 or 8 1-wire masters.
 
Groovy.
But there are couple of indentification nitpicks.

> I put the code under drivers/i2c/chips, but perhaps it should go under
> drivers/w1.
>
> Signed-off-by: Ben Gardner <bgardner@wabtec.com>

>  drivers/i2c/chips/Kconfig  |   12 
>  drivers/i2c/chips/Makefile |    1 
>  drivers/i2c/chips/ds2482.c |  557 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 570 insertions(+)
> 
> Index: linux-2.6.15-rc3-mm1/drivers/i2c/chips/Makefile
> =================================> --- linux-2.6.15-rc3-mm1.orig/drivers/i2c/chips/Makefile
> +++ linux-2.6.15-rc3-mm1/drivers/i2c/chips/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574
>  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
>  obj-$(CONFIG_SENSORS_RTC8564)	+= rtc8564.o
>  obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
> +obj-$(CONFIG_SENSORS_DS2482)	+= ds2482.o
>  obj-$(CONFIG_TPS65010)		+= tps65010.o
>  obj-$(CONFIG_RTC_X1205_I2C)	+= x1205.o
>  
> Index: linux-2.6.15-rc3-mm1/drivers/i2c/chips/ds2482.c
> =================================> --- /dev/null
> +++ linux-2.6.15-rc3-mm1/drivers/i2c/chips/ds2482.c
> @@ -0,0 +1,557 @@
> +/**
> + * ds2482.c - provides i2c to w1-master bridge(s)
> + * Copyright (C) 2005  Ben Gardner <bgardner@wabtec.com>
> + *
> + * The DS2482 is a sensor chip made by Dallis Semiconductor (Maxim).
> + * It is a I2C to 1-wire bridge.
> + * There are two variations: -100 and -800, which have 1 or 8 1-wire ports.
> + * The complete datasheet can be obtained from MAXIM's website at:
> + *   http://www.maxim-ic.com
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <asm/delay.h>
> +
> +#include "../../w1/w1.h"
> +#include "../../w1/w1_int.h"
> +
> +/**
> + * Address is selected using 2 pins, resulting in 4 possible addresses.
> + *  0x18, 0x19, 0x1a, 0x1b
> + * However, this chip is rare and should not be detected, so use the force
> + * module parameter.
> + */
> +static unsigned short normal_i2c[] = {I2C_CLIENT_END};
> +
> +/**
> + * Insmod parameters
> + */
> +I2C_CLIENT_INSMOD_1(ds2482);
> +
> +/**
> + * The DS2482 registers - there are 3 registers that are addressed by a read
> + * pointer. The read pointer is set by the last command executed.
> + *
> + * To read the data, issue a register read for any address
> + */
> +#define DS2482_CMD_RESET               0xF0	/* No param */
> +#define DS2482_CMD_SET_READ_PTR        0xE1	/* Param: DS2482_PTR_CODE_xxx */
> +#define   DS2482_PTR_CODE_STATUS         0xF0
> +#define   DS2482_PTR_CODE_DATA           0xE1
> +#define   DS2482_PTR_CODE_CHANNEL        0xD2	/* DS2482-800 only */
> +#define   DS2482_PTR_CODE_CONFIG         0xC3

Indent.

> +#define DS2482_CMD_CHANNEL_SELECT      0xC3	/* Param: Channel byte - DS2482-800 only */
> +#define DS2482_CMD_WRITE_CONFIG        0xD2	/* Param: Config byte */
> +#define DS2482_CMD_1WIRE_RESET         0xB4	/* Param: None */
> +#define DS2482_CMD_1WIRE_SINGLE_BIT    0x87	/* Param: Bit byte (bit7) */
> +#define DS2482_CMD_1WIRE_WRITE_BYTE    0xA5	/* Param: Data byte */
> +#define DS2482_CMD_1WIRE_READ_BYTE     0x96	/* Param: None */
> +/* Note to read the byte, Set the ReadPtr to Data then read (any addr) */
> +#define DS2482_CMD_1WIRE_TRIPLET       0x78	/* Param: Dir byte (bit7) */
> +
> +/**
> + * Configure Register bit definitions
> + * The top 4 bits always read 0.
> + * To write, the top nibble must be the 1's compl. of the low nibble.
> + */
> +#define DS2482_REG_CFG_1WS             0x08
> +#define DS2482_REG_CFG_SPU             0x04
> +#define DS2482_REG_CFG_PPM             0x02
> +#define DS2482_REG_CFG_APU             0x01
> +
> +/* Write and verify codes for the CHANNEL_SELECT command (DS2482-800 only) */
> +static u8 ds2482_chan_wr[8] = { 0xF0, 0xE1, 0xD2, 0xC3, 0xB4, 0xA5, 0x96, 0x87};
> +static u8 ds2482_chan_rd[8] = { 0xB8, 0xB1, 0xAA, 0xA3, 0x9C, 0x95, 0x8E, 0x87};
> +
> +
> +/**
> + * Status Register bit definitions (read only)
> + */
> +#define DS2482_REG_STS_DIR             0x80
> +#define DS2482_REG_STS_TSB             0x40
> +#define DS2482_REG_STS_SBR             0x20
> +#define DS2482_REG_STS_RST             0x10
> +#define DS2482_REG_STS_LL              0x08
> +#define DS2482_REG_STS_SD              0x04
> +#define DS2482_REG_STS_PPD             0x02
> +#define DS2482_REG_STS_1WB             0x01
> +
> +
> +static int ds2482_attach_adapter(struct i2c_adapter *adapter);
> +static int ds2482_detect(struct i2c_adapter *adapter, int address, int kind);
> +static int ds2482_detach_client(struct i2c_client *client);
> +
> +
> +/**
> + * Driver data (common to all clients)
> + */
> +static struct i2c_driver ds2482_driver = {
> +	.owner		= THIS_MODULE,
> +	.name		= "ds2482",
> +	.flags		= I2C_DF_NOTIFY,
> +	.attach_adapter	= ds2482_attach_adapter,
> +	.detach_client	= ds2482_detach_client,
> +};
> +
> +/*
> + * Client data (each client gets its own)
> + */
> +
> +struct ds2482_data;
> +
> +struct ds2482_w1_chan {
> +	struct ds2482_data	*pdev;
> +	u8			channel;
> +	struct w1_bus_master	w1_bus_master;
> +};
> +
> +struct ds2482_data {
> +	struct i2c_client	client;
> +	struct semaphore	access_lock;
> +
> +	/* 1-wire interface(s) */
> +	int			w1_count;	/* 1 or 8 */
> +	struct ds2482_w1_chan	w1_ch[8];
> +
> +	/* per-device values */
> +	u8			channel;
> +	u8			read_prt;	/* see DS2482_PTR_CODE_xxx */
> +	u8			reg_config;
> +};
> +
> +
> +/**
> + * Sets the read pointer.
> + * @param pdev       The ds2482 client pointer
> + * @param read_ptr   see DS2482_PTR_CODE_xxx above
> + * @return -1 on failure, 0 on success
> + */
> +static inline int ds2482_select_register(struct ds2482_data *pdev, u8 read_ptr)
> +{
> +	if ( pdev->read_prt != read_ptr ) {
> +		if ( i2c_smbus_write_byte_data(&pdev->client,
> +					       DS2482_CMD_SET_READ_PTR,
> +					       read_ptr) < 0 ) {
> +			return(-1);
> +		}

Indent.
I do not know about return(-1),
but people will scream when they found

if () {
	only one operator inside the {} block
}

And if you like return(-1), it is better to use it in all places, not
mixing with return -1.

> +		pdev->read_prt = read_ptr;
> +	}
> +	return(0);
> +}
> +
> +/**
> + * Sends a command without a parameter
> + * @param pdev       The ds2482 client pointer
> + * @param cmd        DS2482_CMD_RESET,
> + *                   DS2482_CMD_1WIRE_RESET,
> + *                   DS2482_CMD_1WIRE_READ_BYTE
> + * @return -1 on failure, 0 on success
> + */
> +static inline int ds2482_send_cmd(struct ds2482_data *pdev, u8 cmd)
> +{
> +	if ( i2c_smbus_write_byte(&pdev->client, cmd) < 0 ) {
> +		return(-1);
> +	}
> +	pdev->read_prt = DS2482_PTR_CODE_STATUS;
> +	return(0);
> +}
> +
> +/**
> + * Sends a command with a parameter
> + * @param pdev       The ds2482 client pointer
> + * @param cmd        DS2482_CMD_WRITE_CONFIG,
> + *                   DS2482_CMD_1WIRE_SINGLE_BIT,
> + *                   DS2482_CMD_1WIRE_WRITE_BYTE,
> + *                   DS2482_CMD_1WIRE_TRIPLET
> + * @param byte       The data to send
> + * @return -1 on failure, 0 on success
> + */
> +static inline int ds2482_send_cmd_data(struct ds2482_data *pdev, u8 cmd, u8 byte)
> +{
> +	if ( i2c_smbus_write_byte_data(&pdev->client, cmd, byte) < 0 ) {
> +		return(-1);
> +	}
> +	/* all cmds leave in STATUS, except CONFIG */
> +	pdev->read_prt = (cmd != DS2482_CMD_WRITE_CONFIG) ?
> +			 DS2482_PTR_CODE_STATUS : DS2482_PTR_CODE_CONFIG;
> +	return(0);
> +}
> +
> +
> +/*
> + * 1-Wire interface code
> + */
> +
> +
> +/**
> + * Waits until the 1-wire interface is idle (not busy)
> + *
> + * @param pdev Pointer to the device structure
> + * @return the last value read from status or -1 (failure)
> + */
> +static inline int ds2482_wait_1wire_idle(struct ds2482_data *pdev)
> +{
> +	int temp = -1;
> +	if ( !ds2482_select_register(pdev, DS2482_PTR_CODE_STATUS) ) {
> +		do {
> +			temp = i2c_smbus_read_byte(&pdev->client);
> +		} while ( (temp >= 0) && (temp & DS2482_REG_STS_1WB) );
> +	}
> +	return(temp);
> +}

Do you really want to read it indefenitely, if there are bus/ds2482 problems?

> +/**
> + * Selects a w1 channel.
> + * The 1-wire interface must be idle before calling this function.
> + *
> + * @param pdev       The ds2482 client pointer
> + * @param channel    0-7
> + * @return           -1 (failure) or 0 (success)
> + */
> +static inline int ds2482_set_channel(struct ds2482_data *pdev, u8 channel)
> +{
> +	if (channel >= 8)
> +		return -1;
> +
> +	if ( i2c_smbus_write_byte_data(&pdev->client, DS2482_CMD_CHANNEL_SELECT,
> +				       ds2482_chan_wr[channel]) < 0 )
> +		return -1;
> +
> +	pdev->read_prt = DS2482_PTR_CODE_CHANNEL;
> +	pdev->channel = -1;
> +	if (i2c_smbus_read_byte(&pdev->client) = ds2482_chan_rd[channel]) {
> +		pdev->channel = channel;
> +		return 0;
> +	}
> +	return -1;
> +}
> +
> +
> +/**
> + * Performs the touch-bit function, which writes a 0 or 1 and reads the level.
> + *
> + * @param data       The ds2482 channel pointer
> + * @param bit        The level to write: 0 or non-zero
> + * @return           The level read: 0 or 1
> + */
> +static u8 ds2482_w1_touch_bit(unsigned long data, u8 bit)
> +{
> +	struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
> +	struct ds2482_data    *pdev = pchan->pdev;
> +	int status = -1;
> +
> +	down(&pdev->access_lock);
> +
> +	/* Select the channel */
> +	ds2482_wait_1wire_idle(pdev);
> +	if (pdev->w1_count > 1) {
> +		ds2482_set_channel(pdev, pchan->channel);
> +	}
> +
> +	if (!ds2482_send_cmd_data(pdev, DS2482_CMD_1WIRE_SINGLE_BIT,
> +                                  bit ? 0xFF : 0)) {
> +		/* Wait until 1WB = 0, return the status */
> +		status = ds2482_wait_1wire_idle(pdev);
> +	}
> +	up(&pdev->access_lock);
> +
> +	return((status & DS2482_REG_STS_SBR) ? 1 : 0);
> +}
> +
> +/**
> + * Performs the triplet function, which reads two bits and writes a bit.
> + * The bit written is determined by the two reads:
> + *   00 => dbit, 01 => 0, 10 => 1
> + *
> + * @param data       The ds2482 channel pointer
> + * @param dbit       The direction to choose if both branches are valid
> + * @return           b0=read1 b1=read2 b3=bit written
> + */
> +static u8 ds2482_w1_triplet(unsigned long data, u8 dbit)
> +{
> +	struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
> +	struct ds2482_data    *pdev = pchan->pdev;
> +	int status=3, result;
> +
> +	down(&pdev->access_lock);
> +
> +	/* Select the channel */
> +	ds2482_wait_1wire_idle(pdev);
> +	if (pdev->w1_count > 1) {
> +		ds2482_set_channel(pdev, pchan->channel);
> +	}
> +
> +	/* Send the triplet command */
> +	if (!ds2482_send_cmd_data(pdev, DS2482_CMD_1WIRE_TRIPLET,
> +                                  dbit ? 0xFF : 0)) {
> +		/* Wait until 1WB = 0, grab the status */
> +		status = ds2482_wait_1wire_idle(pdev);
> +	}
> +	up(&pdev->access_lock);
> +
> +	/* Decode the status */
> +	result = (status >> 5);
> +	return(result);
> +}
> +
> +/**
> + * Performs the write byte function.
> + *
> + * @param data       The ds2482 channel pointer
> + * @param byte       The value to write
> + */
> +static void ds2482_w1_write_byte(unsigned long data, u8 byte)
> +{
> +	struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
> +	struct ds2482_data    *pdev = pchan->pdev;
> +
> +	down(&pdev->access_lock);
> +
> +	/* Select the channel */
> +	ds2482_wait_1wire_idle(pdev);
> +	if (pdev->w1_count > 1) {
> +		ds2482_set_channel(pdev, pchan->channel);
> +	}
> +
> +	/* Send the write byte command */
> +	ds2482_send_cmd_data(pdev, DS2482_CMD_1WIRE_WRITE_BYTE, byte);
> +
> +	up(&pdev->access_lock);
> +}
> +
> +/**
> + * Performs the read byte function.
> + *
> + * @param data       The ds2482 channel pointer
> + * @return           The value read
> + */
> +static u8 ds2482_w1_read_byte(unsigned long data)
> +{
> +	struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
> +	struct ds2482_data    *pdev = pchan->pdev;
> +	int result;
> +
> +	down(&pdev->access_lock);
> +
> +	/* Select the channel */
> +	ds2482_wait_1wire_idle(pdev);
> +	if (pdev->w1_count > 1) {
> +		ds2482_set_channel(pdev, pchan->channel);
> +	}
> +
> +	/* Send the read byte command */
> +	ds2482_send_cmd(pdev, DS2482_CMD_1WIRE_READ_BYTE);
> +
> +	/* Wait until 1WB = 0 */
> +	ds2482_wait_1wire_idle(pdev);
> +
> +	/* Select the data register */
> +	ds2482_select_register(pdev, DS2482_PTR_CODE_DATA);
> +
> +	/* Read the data byte */
> +	result = i2c_smbus_read_byte(&pdev->client);
> +
> +	up(&pdev->access_lock);
> +
> +	return((u8)result);
> +}
> +
> +
> +/**
> + * Sends a reset on the 1-wire interface
> + *
> + * @param data    The ds2482 channel pointer
> + * @return        0Þvice present, 1=No device present or error
> + */
> +static u8 ds2482_w1_reset_bus(unsigned long data)
> +{
> +	struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
> +	struct ds2482_data    *pdev = pchan->pdev;
> +	int err;
> +	u8 retval = 1;
> +
> +	down(&pdev->access_lock);
> +
> +	/* Select the channel */
> +	ds2482_wait_1wire_idle(pdev);
> +	if (pdev->w1_count > 1) {
> +		ds2482_set_channel(pdev, pchan->channel);
> +	}
> +
> +	/* Send the reset command */
> +	err = ds2482_send_cmd(pdev, DS2482_CMD_1WIRE_RESET);
> +	if (err >= 0) {
> +		/* Wait until the reset is complete */
> +		err = ds2482_wait_1wire_idle(pdev);
> +		retval = (err & DS2482_REG_STS_PPD) ? 0 : 1;
> +
> +		/* If the chip did reset since detect, re-config it */
> +		if (err & DS2482_REG_STS_RST) {
> +			ds2482_send_cmd_data(pdev, DS2482_CMD_WRITE_CONFIG,
> +                                             0xF0);
> +		}
> +	}
> +
> +	up(&pdev->access_lock);
> +
> +	return(retval);
> +}
> +
> +
> +/**
> + * Called to see if the device exists on an i2c bus.
> + */
> +static int ds2482_attach_adapter(struct i2c_adapter *adapter)
> +{
> +	return(i2c_probe(adapter, &addr_data, ds2482_detect));
> +}
> +
> +
> +/*
> + * The following function does more than just detection. If detection
> + * succeeds, it also registers the new chip.
> + */
> +static int ds2482_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> +	struct ds2482_data *data;
> +	struct i2c_client  *new_client;
> +	int err = 0;
> +	int temp1;
> +	int idx;
> +
> +	if ( !i2c_check_functionality(adapter,
> +				      I2C_FUNC_SMBUS_BYTE_DATA |
> +				      I2C_FUNC_SMBUS_BYTE) ) {
> +		goto exit;
> +	}
> +
> +	if ( !(data = kmalloc(sizeof(struct ds2482_data), GFP_KERNEL)) ) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +	memset(data, 0, sizeof(struct ds2482_data));
> +
> +	new_client = &data->client;
> +	i2c_set_clientdata(new_client, data);
> +	new_client->addr = address;
> +	new_client->adapter = adapter;
> +	new_client->driver = &ds2482_driver;
> +	new_client->flags = 0;
> +
> +	/* Reset the device (sets the read_ptr to status) */
> +	if ( ds2482_send_cmd(data, DS2482_CMD_RESET) < 0 ) {
> +		dev_dbg(&adapter->dev, "DS2482 reset failed at 0x%02x.\n",
> +			address);
> +		goto exit_free;
> +	}
> +
> +	/* Sleep at least 525ns to allow the reset to complete */
> +	ndelay(525);
> +
> +	/* Read the status byte - expect reset bit and line to be set */
> +	temp1 = i2c_smbus_read_byte(new_client);
> +	if (temp1 != (DS2482_REG_STS_LL | DS2482_REG_STS_RST)) {
> +		dev_dbg(&adapter->dev, "DS2482 (0x%02x) reset status "
> +			"0x%02X - not a DS2482\n", address, temp1);
> +		goto exit_free;
> +	}
> +
> +	/* Detect the 8-port version */
> +	data->w1_count = 1;
> +	if (ds2482_set_channel(data, 7) = 0) {
> +		data->w1_count = 8;
> +	}
> +
> +	/* Set all config items to 0 (off) */
> +	ds2482_send_cmd_data(data, DS2482_CMD_WRITE_CONFIG, 0xF0);
> +
> +	/* We can fill in the remaining client fields */
> +	sprintf(new_client->name, "ds2482-%d00", data->w1_count);
> +
> +	init_MUTEX(&data->access_lock);
> +
> +	/* Tell the I2C layer a new client has arrived */
> +	if ( (err = i2c_attach_client(new_client)) )
> +		goto exit_free;
> +
> +	/* Register 1-wire interface(s) */
> +	for (idx = 0; idx < data->w1_count; idx++) {
> +		data->w1_ch[idx].pdev = data;
> +		data->w1_ch[idx].channel = idx;
> +
> +		/* Populate all the w1 bus master stuff */
> +		data->w1_ch[idx].w1_bus_master.data = (unsigned long)&data->w1_ch[idx];
> +		data->w1_ch[idx].w1_bus_master.read_byte  = ds2482_w1_read_byte;
> +		data->w1_ch[idx].w1_bus_master.write_byte = ds2482_w1_write_byte;
> +		data->w1_ch[idx].w1_bus_master.touch_bit  = ds2482_w1_touch_bit;
> +		data->w1_ch[idx].w1_bus_master.triplet    = ds2482_w1_triplet;
> +		data->w1_ch[idx].w1_bus_master.reset_bus  = ds2482_w1_reset_bus;
> +
> +		err = w1_add_master_device(&data->w1_ch[idx].w1_bus_master);
> +		if ( err ) {
> +			data->w1_ch[idx].pdev = NULL;
> +			goto exit_w1_remove;
> +		}
> +	}
> +
> +	return(0);
> +
> +exit_w1_remove:
> +	for (idx = 0; idx < data->w1_count; idx++) {
> +		if (data->w1_ch[idx].pdev != NULL) {
> +			w1_remove_master_device(&data->w1_ch[idx].w1_bus_master);
> +		}
> +	}
> +exit_free:
> +	kfree(data);
> +exit:
> +	return(err);
> +}
> +
> +static int ds2482_detach_client(struct i2c_client *client)
> +{
> +	struct ds2482_data   *data = i2c_get_clientdata(client);
> +	int err, idx;
> +
> +	/* Unregister the 1-wire bridge(s) */
> +	for (idx = 0; idx < data->w1_count; idx++) {
> +		if (data->w1_ch[idx].pdev != NULL) {
> +			w1_remove_master_device(&data->w1_ch[idx].w1_bus_master);
> +		}
> +	}
> +
> +	/* Detach the i2c device */
> +	if ( (err = i2c_detach_client(client)) ) {
> +		dev_err(&client->dev,
> +			"Client deregistration failed, client not detached.\n");
> +		return(err);
> +	}
> +
> +	/* Free the memory */
> +	kfree(data);
> +	return(0);
> +}
> +
> +static int __init sensors_ds2482_init(void)
> +{
> +	return(i2c_add_driver(&ds2482_driver));
> +}
> +
> +static void __exit sensors_ds2482_exit(void)
> +{
> +	i2c_del_driver(&ds2482_driver);
> +}
> +
> +MODULE_AUTHOR("Ben Gardner <bgardner@wabtec.com>");
> +MODULE_DESCRIPTION("DS2482 driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(sensors_ds2482_init);
> +module_exit(sensors_ds2482_exit);
> Index: linux-2.6.15-rc3-mm1/drivers/i2c/chips/Kconfig
> =================================> --- linux-2.6.15-rc3-mm1.orig/drivers/i2c/chips/Kconfig
> +++ linux-2.6.15-rc3-mm1/drivers/i2c/chips/Kconfig
> @@ -135,4 +135,16 @@ config RTC_X1205_I2C
>  	  This driver can also be built as a module. If so, the module
>  	  will be called x1205.
>  
> +config SENSORS_DS2482
> +	tristate "Maxim DS2482 I2C to 1-Wire bridge"
> +	depends on I2C && EXPERIMENTAL
> +	select W1

"select" is for very small and commond code pathes.
You need here 
depends on I2C && W1 && EXPERIMENTAL

> +	help
> +	  If you say yes here you get support for the Maxim DS2482
> +	  I2C to 1-Wire bridge.
> +	  This, obviously, requires the 1-Wire subsystem (W1)
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called ds2482.
> +
>  endmenu
> 
> 
> 
> 


-- 
	Evgeniy Polyakov

[lm-sensors] Re: [patch] ds2482: i2c to 1-wire bridge

[Ben Gardner]

Evgeniy,

I will incorporate your suggestions and repost the patch.
 - remove unecessary () in return statements
 - remove { } from around simple, single statement if statement bodies
 - fix indentation of #defines
 - add timeout for ds2482_wait_1wire_idle (good catch, BTW)
 - fix Kconfig stuff

Thank you for your time!

Ben

[lm-sensors] Re: [patch] ds2482: i2c to 1-wire bridge

[Jean Delvare]

Hi Ben,

> This patch adds support for the Maxim DS2482-100 and DS2482-800 chips,
> which are i2c devices that provide 1 or 8 1-wire masters.
> 
> I put the code under drivers/i2c/chips, but perhaps it should go under
> drivers/w1.

Yes, I think it should go under drivers/w1. SMBus masters which are PCI
devices go under drivers/i2c/busses rather than drivers/pci, and in
general it sounds saner to care about what the driver provides rather
than the technology it relies on. drivers/i2c/chips is an exception to
that rule, but drivers should only go there if there is really no other
place where they fit. Ideally this directory should be empty ;)

> + * The DS2482 is a sensor chip made by Dallis Semiconductor (Maxim).

Typo: Dallas.

> + * The complete datasheet can be obtained from MAXIM's website at:
> + *   http://www.maxim-ic.com

Can you please point to the device page? Maxim's site is well done and
each chip or family of chips has a page you can easily point to.

> +/**
> + * Address is selected using 2 pins, resulting in 4 possible addresses.
> + *  0x18, 0x19, 0x1a, 0x1b
> + * However, this chip is rare and should not be detected, so use the force
> + * module parameter.
> + */

I guess you mean "cannot be detected" rather than "should not be
detected"?

Hopefully, the upcoming new method to explicitely attach i2c drivers to
devices should make that case easier to deal with.

> +/**
> + * Configure Register bit definitions
> + * The top 4 bits always read 0.
> + * To write, the top nibble must be the 1's compl. of the low nibble.
> + */
> +#define DS2482_REG_CFG_1WS             0x08
> +#define DS2482_REG_CFG_SPU             0x04
> +#define DS2482_REG_CFG_PPM             0x02
> +#define DS2482_REG_CFG_APU             0x01
> +
> +/* Write and verify codes for the CHANNEL_SELECT command (DS2482-800 only) */
> +static u8 ds2482_chan_wr[8] = { 0xF0, 0xE1, 0xD2, 0xC3, 0xB4, 0xA5, 0x96, 0x87};
> +static u8 ds2482_chan_rd[8] = { 0xB8, 0xB1, 0xAA, 0xA3, 0x9C, 0x95, 0x8E, 0x87};

This second static array (ds2482_chan_rd) looks contradictory with the
above statement that the top 4 bits always read 0. Clarification needed?

BTW, please align all define values using tabs, not spaces. And spaces
are missing before closing curly braces.

> +static struct i2c_driver ds2482_driver = {
> +	.owner		= THIS_MODULE,
> +	.name		= "ds2482",
> +	.flags		= I2C_DF_NOTIFY,
> +	.attach_adapter	= ds2482_attach_adapter,
> +	.detach_client	= ds2482_detach_client,
> +};

Note that there are ongoing changes related to this structure which
will hit next -mm. This will need to look like this there:

static struct i2c_driver ds2482_driver = {
	.driver = {
		.owner	= THIS_MODULE,
		.name	= "ds2482",
	},
	.attach_adapter	= ds2482_attach_adapter,
	.detach_client	= ds2482_detach_client,
};

I guess Andrew will deal with this just fine anyway.

> +	if ( pdev->read_prt != read_ptr ) {

No spaces inside parentheses please! Ever!

> +			return(-1);

No parentheses on simple returns.

> +static inline int ds2482_set_channel(struct ds2482_data *pdev, u8 channel)
> +{
> +	if (channel >= 8)
> +		return -1;

As I understand it, this cannot happen unless you did a programming
error. Thus this should be made a DEBUG statement, if not plain dropped
once your driver is fully tested.

> +static u8 ds2482_w1_touch_bit(unsigned long data, u8 bit)
> +{
> +	struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;

That's an ugly cast. Can't it be avoided?

> +static u8 ds2482_w1_read_byte(unsigned long data)
> +{
> (...)
> +	return((u8)result);
> +}

If this cast is ever useful, then your code is broken. Let alone the
fact that the cast is implicit anyway.

> +		retval = (err & DS2482_REG_STS_PPD) ? 0 : 1;

I suspect that:

		retval = !(err & DS2482_REG_STS_PPD);

would be more efficient. But the compiler might generate the same code
after all, I don't know for sure.

> +	if ( !i2c_check_functionality(adapter,
> +				      I2C_FUNC_SMBUS_BYTE_DATA |
> +				      I2C_FUNC_SMBUS_BYTE) ) {
> +		goto exit;
> +	}

You don't seem to use i2c_smbus_read_byte_data, so you should test for
I2C_FUNC_SMBUS_WRITE_BYTE_DATA instead of I2C_FUNC_SMBUS_BYTE_DATA.

> +	if ( !(data = kmalloc(sizeof(struct ds2482_data), GFP_KERNEL)) ) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +	memset(data, 0, sizeof(struct ds2482_data));

Please use kzalloc().

> +	new_client->flags = 0;

Not needed thanks to kzalloc (or memset).

> +	/* Read the status byte - expect reset bit and line to be set */
> +	temp1 = i2c_smbus_read_byte(new_client);
> +	if (temp1 != (DS2482_REG_STS_LL | DS2482_REG_STS_RST)) {

You test for more than the comment says (you test that all other bits
are *not* set).

> +	/* We can fill in the remaining client fields */
> +	sprintf(new_client->name, "ds2482-%d00", data->w1_count);

Unsafe, please use snprintf.

> +exit_w1_remove:
> +	for (idx = 0; idx < data->w1_count; idx++) {
> +		if (data->w1_ch[idx].pdev != NULL) {
> +			w1_remove_master_device(&data->w1_ch[idx].w1_bus_master);
> +		}
> +	}
> +exit_free:
> +	kfree(data);

Isn't there an i2c_detach_client(new_client) missing in this error path?

-- 
Jean Delvare

[lm-sensors] [patch] ds2482: i2c to 1-wire bridge

[Ben Gardner]

If there aren't any objections, I'll follow the i2c layout and create
a drivers/w1/busses folder and put the driver there.
Should I also create a few patches to move the matrox_w1 and
ds_w1_bridge stuff to that folder?
Should I also create a drivers/w1/chips folder and move the family drivers?

> > +static u8 ds2482_w1_touch_bit(unsigned long data, u8 bit)
> > +{
> > +     struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
>
> That's an ugly cast. Can't it be avoided?

Sure, if we change the w1 subsystem to use 'void *' as the user-data
type instead of 'unsigned long'.
Eugeniy - are you OK with that change?

Thanks,
Ben

[lm-sensors] [patch] ds2482: i2c to 1-wire bridge

[Evgeniy Polyakov]

On Mon, Dec 05, 2005 at 09:40:20AM -0600, Ben Gardner (gardner.ben at gmail.com) wrote:
> If there aren't any objections, I'll follow the i2c layout and create
> a drivers/w1/busses folder and put the driver there.
> Should I also create a few patches to move the matrox_w1 and
> ds_w1_bridge stuff to that folder?
> Should I also create a drivers/w1/chips folder and move the family drivers?

Hmm, any other names? There are no at least busses in the w1 world.
Maybe masters/bus_masters/controllers/something...?

> > > +static u8 ds2482_w1_touch_bit(unsigned long data, u8 bit)
> > > +{
> > > +     struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
> >
> > That's an ugly cast. Can't it be avoided?
> 
> Sure, if we change the w1 subsystem to use 'void *' as the user-data
> type instead of 'unsigned long'.
> Eugeniy - are you OK with that change?

No problem, but make it in a separate patch.

> Thanks,
> Ben

-- 
	Evgeniy Polyakov

[lm-sensors] [patch] ds2482: i2c to 1-wire bridge

[Ben Gardner]

It seems that most w1 documentation refers to them as masters and
slaves, so how about:
drivers/w1/masters
drivers/w1/slaves

I'll create three patch series:
 - change 'unsigned long' to 'void *'
 - move master and slave code into subfolders
 - add ds2482

Does that sound OK?

Thanks,
Ben

On 12/5/05, Evgeniy Polyakov <johnpol at 2ka.mipt.ru> wrote:
> On Mon, Dec 05, 2005 at 09:40:20AM -0600, Ben Gardner (gardner.ben at gmail.com) wrote:
> > If there aren't any objections, I'll follow the i2c layout and create
> > a drivers/w1/busses folder and put the driver there.
> > Should I also create a few patches to move the matrox_w1 and
> > ds_w1_bridge stuff to that folder?
> > Should I also create a drivers/w1/chips folder and move the family drivers?
>
> Hmm, any other names? There are no at least busses in the w1 world.
> Maybe masters/bus_masters/controllers/something...?
>
> > > > +static u8 ds2482_w1_touch_bit(unsigned long data, u8 bit)
> > > > +{
> > > > +     struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
> > >
> > > That's an ugly cast. Can't it be avoided?
> >
> > Sure, if we change the w1 subsystem to use 'void *' as the user-data
> > type instead of 'unsigned long'.
> > Eugeniy - are you OK with that change?
>
> No problem, but make it in a separate patch.
>
> > Thanks,
> > Ben
>
> --
>         Evgeniy Polyakov
>

[lm-sensors] [patch] ds2482: i2c to 1-wire bridge

[Evgeniy Polyakov]

On Mon, Dec 05, 2005 at 10:10:37AM -0600, Ben Gardner (gardner.ben at gmail.com) wrote:
> It seems that most w1 documentation refers to them as masters and
> slaves, so how about:
> drivers/w1/masters
> drivers/w1/slaves
> 
> I'll create three patch series:
>  - change 'unsigned long' to 'void *'
>  - move master and slave code into subfolders
>  - add ds2482
> 
> Does that sound OK?

Yes, it definitely does.
Thank you.

> Thanks,
> Ben
> 
> On 12/5/05, Evgeniy Polyakov <johnpol at 2ka.mipt.ru> wrote:
> > On Mon, Dec 05, 2005 at 09:40:20AM -0600, Ben Gardner (gardner.ben at gmail.com) wrote:
> > > If there aren't any objections, I'll follow the i2c layout and create
> > > a drivers/w1/busses folder and put the driver there.
> > > Should I also create a few patches to move the matrox_w1 and
> > > ds_w1_bridge stuff to that folder?
> > > Should I also create a drivers/w1/chips folder and move the family drivers?
> >
> > Hmm, any other names? There are no at least busses in the w1 world.
> > Maybe masters/bus_masters/controllers/something...?
> >
> > > > > +static u8 ds2482_w1_touch_bit(unsigned long data, u8 bit)
> > > > > +{
> > > > > +     struct ds2482_w1_chan *pchan = (struct ds2482_w1_chan *)data;
> > > >
> > > > That's an ugly cast. Can't it be avoided?
> > >
> > > Sure, if we change the w1 subsystem to use 'void *' as the user-data
> > > type instead of 'unsigned long'.
> > > Eugeniy - are you OK with that change?
> >
> > No problem, but make it in a separate patch.
> >
> > > Thanks,
> > > Ben
> >
> > --
> >         Evgeniy Polyakov
> >

-- 
	Evgeniy Polyakov