[i2c] [PATCH] [2.6.21] [v2] PA Semi SMBus driver

Jean Delvare khali at linux-fr.org
Mon Jan 29 20:54:26 CET 2007


Hi Olof,

On Sun, 28 Jan 2007 22:11:29 -0600, Olof Johansson wrote:
> Driver for PA Semi SMBus interface.
> 
> Signed-off-by: Olof Johansson <olof at lixom.net>

OK, it's really good this time as you fixed all the problems I had
found last time. There are some problems related to the implementation
of master_xfer you added.

> +static int pasemi_i2c_xfer_msg(struct i2c_adapter *adapter,
> +			      struct i2c_msg *msg, int stop)
> +{
> +	struct pasemi_smbus *smbus = adapter->algo_data;
> +	int read, i;
> +	u32 rd;
> +
> +	read = msg->flags & I2C_M_RD ? 1 : 0;
> +
> +	TXFIFO_WR(smbus, MTXFIFO_START | (msg->addr << 1) | read);
> +
> +	if (read) {
> +		TXFIFO_WR(smbus, msg->len | MTXFIFO_READ |
> +				(stop ? MTXFIFO_STOP : 0));
> +
> +		for (i = 0; i < msg->len; i++) {
> +			pasemi_smb_waitready(smbus);

In the previous version, the code handling block reads didn't include
that call to pasemi_smb_waitready() inside the loop, but before. Why is
it different here?

> +			rd = RXFIFO_RD(smbus);
> +			if (rd & MRXFIFO_EMPTY)
> +				return -ENODATA;
> +			msg->buf[i] = rd & MRXFIFO_DATA_M;
> +		}
> +	} else {
> +		for (i = 0; i < msg->len - 1; i++)
> +			TXFIFO_WR(smbus, msg->buf[i]);
> +
> +		TXFIFO_WR(smbus, msg->buf[i] | (stop ? MTXFIFO_STOP : 0));

Maybe the intent would be clearer using [msg->len] instead of [i]?
(Although I agree it's the same value.)

> +	}
> +
> +	return 0;
> +}
> +
> +static int pasemi_i2c_xfer(struct i2c_adapter *adapter,
> +			   struct i2c_msg *msgs, int num)
> +{
> +	struct pasemi_smbus *smbus = adapter->algo_data;
> +	int ret, i;
> +
> +	pasemi_smb_clear(smbus);
> +
> +	ret = 0;
> +
> +	for (i = 0; i < num && !ret; i++)
> +		ret = pasemi_i2c_xfer_msg(adapter, &msgs[i], (i == (num - 1)));
> +
> +	return ret ? ret : num;
> +}

You're not checking the message sizes. Does the hardware support
arbitrarily large messages? If not, you really want to add a check here
and return an error if the message length exceeds what the hardware can
handle.

Also, the smbus_xfer() function takes care of reinitializing the
hardware in case of error, shouldn't you do the same here?

> +
> +static int pasemi_smb_xfer(struct i2c_adapter *adapter,
> +		u16 addr, unsigned short flags, char read_write, u8 command,
> +		int size, union i2c_smbus_data *data)
> +{
> +	struct pasemi_smbus *smbus = adapter->algo_data;
> +	unsigned int rd;
> +	int read_flag, err;
> +
> +	/* All our ops take 8-bit shifted addresses */
> +	addr <<= 1;
> +	read_flag = read_write == I2C_SMBUS_READ;
> +
> +	pasemi_smb_clear(smbus);
> +
> +	switch (size) {
> +	case I2C_SMBUS_QUICK:
> +		TXFIFO_WR(smbus, addr | read_flag | MTXFIFO_START |
> +			  MTXFIFO_STOP);
> +		break;
> +	case I2C_SMBUS_BYTE:
> +		TXFIFO_WR(smbus, addr | read_flag | MTXFIFO_START);
> +		if (read_write)
> +			TXFIFO_WR(smbus, 1 | MTXFIFO_STOP | MTXFIFO_READ);
> +		else
> +			TXFIFO_WR(smbus, MTXFIFO_STOP | command);
> +		break;
> +	case I2C_SMBUS_BYTE_DATA:
> +		TXFIFO_WR(smbus, addr | MTXFIFO_START);
> +		TXFIFO_WR(smbus, command);
> +		if (read_write) {
> +			TXFIFO_WR(smbus, addr | I2C_SMBUS_READ | MTXFIFO_START);
> +			TXFIFO_WR(smbus, 1 | MTXFIFO_READ | MTXFIFO_STOP);
> +		} else {
> +			TXFIFO_WR(smbus, MTXFIFO_STOP | data->byte);
> +		}
> +		break;
> +	case I2C_SMBUS_WORD_DATA:
> +		TXFIFO_WR(smbus, addr | MTXFIFO_START);
> +		TXFIFO_WR(smbus, command);
> +		if (read_write) {
> +			TXFIFO_WR(smbus, addr | I2C_SMBUS_READ | MTXFIFO_START);
> +			TXFIFO_WR(smbus, 2 | MTXFIFO_READ | MTXFIFO_STOP);
> +		} else {
> +			TXFIFO_WR(smbus, data->word & MTXFIFO_DATA_M);
> +			TXFIFO_WR(smbus, MTXFIFO_STOP | (data->word >> 8));
> +		}
> +		break;
> +	default:
> +		dev_warn(&adapter->dev, "Unsupported transaction %d\n", size);
> +		return -EINVAL;
> +	}
> +
> +	err = pasemi_smb_waitready(smbus);
> +
> +	if (err)
> +		goto reset_out;
> +
> +	if (!read_write)
> +		return 0;
> +
> +	switch (size) {
> +	case I2C_SMBUS_BYTE:
> +	case I2C_SMBUS_BYTE_DATA:
> +		rd = RXFIFO_RD(smbus);
> +		if (rd & MRXFIFO_EMPTY) {
> +			err = -ENODATA;
> +			goto reset_out;
> +		}
> +		data->byte = rd & MRXFIFO_DATA_M;
> +		break;
> +	case I2C_SMBUS_WORD_DATA:
> +		rd = RXFIFO_RD(smbus);
> +		if (rd & MRXFIFO_EMPTY) {
> +			err = -ENODATA;
> +			goto reset_out;
> +		}
> +		data->word = rd & MRXFIFO_DATA_M;
> +		rd = RXFIFO_RD(smbus);
> +		if (rd & MRXFIFO_EMPTY) {
> +			err = -ENODATA;
> +			goto reset_out;
> +		}
> +		data->word |= (rd & MRXFIFO_DATA_M) << 8;
> +		break;
> +	}
> +
> +	return 0;
> +
> +reset_out:
> +	reg_write(smbus, REG_CTL, (CTL_MTR | CTL_MRR |
> +		  (CLK_100K_DIV & CTL_CLK_M)));
> +	return err;
> +}

I see you removed support for some transactions from this function
compared to the previous code, presumably under the assumption that
pasemi_i2c_xfer() would handle them. There are two problems with this
approach. First, SMBus block read and SMBus block process call are
special transactions because the read length is received from the
slave chip itself. This currently cannot be done under I2C emulation.
(The code doing that used to exist but was never ported to Linux 2.6.)
Second, i2c-core isn't smart enough to detect which transactions are
not implemented by pasemi_smb_xfer(), and will call it for all
SMBus-level transactions.

I admit this situation isn't handled conveniently by i2c-core at the
moment, we never bothered because drivers which implement both
master_xfer and smbus_xfer are very rare.

I see four possible paths you can walk from here:

1* Restore pasemi_smb_xfer() as it was before, delete pasemi_i2c_xfer()
and fix i2c-ds1307 to use I2C block transactions at the SMBus-level
interface.

2* Restore pasemi_smb_xfer() as it was before, and keep
pasemi_i2c_xfer(). That's the most simple option, the only drawback
being some code duplication.

3* Forward-port the missing i2c-core code so that SMBus block read and
SMBus block process call can be emulated as well. It shouldn't be too
hard, and I can help you (not before next week though). Then you should
be able to extend pasemi_i2c_xfer() to emulate everything and delete
pasemi_smb_xfer(). That's clean and there's no code duplication at all.

4* Explicitely call pasemi_i2c_xfer() from pasemi_smb_xfer() for the
transactions you did not implement in from pasemi_smb_xfer(). This will
require i2c_smbus_xfer_emulated() to be exported from i2c-core, which
isn't the case at the moment. We can export it if you need it though,
that's a valid case. Or we may decide that smbus_xfer should return for
example -EAGAIN when the driver wants master_xfer to be called instead
for a given transaction, in which case i2c-core can do it automatically.

I'd rather avoid 4* for now as it'll require some interface changes,
which should never be done lightly. For the rest, just pick what you
prefer.

> +
> +static u32 pasemi_smb_func(struct i2c_adapter *adapter)
> +{
> +	return	I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
> +		I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> +		I2C_FUNC_I2C;
> +
> +}

Extra blank line ;)

> +
> +static const struct i2c_algorithm smbus_algorithm = {
> +	.master_xfer	= pasemi_i2c_xfer,
> +	.smbus_xfer	= pasemi_smb_xfer,
> +	.functionality	= pasemi_smb_func,
> +};
> +
> +
> +static int __devinit pasemi_smb_probe(struct pci_dev *dev,
> +				      const struct pci_device_id *id)
> +{
> +	struct pasemi_smbus *smbus;
> +	int error;
> +
> +	if (!(pci_resource_flags(dev, 0) & IORESOURCE_IO))
> +		return -ENODEV;
> +
> +	smbus = kzalloc(sizeof(struct pasemi_smbus), GFP_KERNEL);
> +	if (!smbus)
> +		return -ENOMEM;
> +
> +	smbus->dev = dev;
> +	smbus->base = pci_resource_start(dev, 0);
> +	smbus->size = pci_resource_len(dev, 0);
> +
> +	if (!request_region(smbus->base, smbus->size,
> +			    pasemi_smb_driver.name)) {
> +		error = -EBUSY;
> +		goto out_kfree;
> +	}
> +
> +	smbus->adapter.owner = THIS_MODULE;
> +	snprintf(smbus->adapter.name, I2C_NAME_SIZE,
> +		"PA Semi SMBus adapter at 0x%lx", smbus->base);
> +	smbus->adapter.class = I2C_CLASS_HWMON;
> +	smbus->adapter.algo = &smbus_algorithm;
> +	smbus->adapter.algo_data = smbus;
> +
> +	/* set up the driverfs linkage to our parent device */
> +	smbus->adapter.dev.parent = &dev->dev;
> +
> +	reg_write(smbus, REG_CTL, (CTL_MTR | CTL_MRR |
> +		  (CLK_100K_DIV & CTL_CLK_M)));
> +
> +	error = i2c_add_adapter(&smbus->adapter);
> +	if (error)
> +		goto out_release_region;
> +
> +	pci_set_drvdata(dev, smbus);
> +
> +	return 0;
> +
> + out_release_region:
> +	release_region(smbus->base, smbus->size);
> + out_kfree:
> +	kfree(smbus);
> +	return error;
> +}
> +
> +
> +static void __devexit pasemi_smb_remove(struct pci_dev *dev)
> +{
> +	struct pasemi_smbus *smbus = pci_get_drvdata(dev);
> +
> +	i2c_del_adapter(&smbus->adapter);
> +	release_region(smbus->base, smbus->size);
> +	kfree(smbus);
> +}
> +
> +static struct pci_device_id pasemi_smb_ids[] = {
> +	{ PCI_DEVICE(0x1959, 0xa003) },
> +	{ 0, }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, pasemi_smb_ids);
> +
> +static struct pci_driver pasemi_smb_driver = {
> +	.name		= "i2c-pasemi",
> +	.id_table	= pasemi_smb_ids,
> +	.probe		= pasemi_smb_probe,
> +	.remove		= __devexit_p(pasemi_smb_remove),
> +};
> +
> +static int __init pasemi_smb_init(void)
> +{
> +	return pci_register_driver(&pasemi_smb_driver);
> +}
> +
> +static void __exit pasemi_smb_exit(void)
> +{
> +	pci_unregister_driver(&pasemi_smb_driver);
> +}
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR ("Olof Johansson <olof at lixom.net>");
> +MODULE_DESCRIPTION("PA Semi PWRficient SMBus driver");
> +
> +module_init(pasemi_smb_init);
> +module_exit(pasemi_smb_exit);
> Index: merge/MAINTAINERS
> ===================================================================
> --- merge.orig/MAINTAINERS
> +++ merge/MAINTAINERS
> @@ -2484,6 +2484,12 @@ L:	orinoco-devel at lists.sourceforge.net
>  W:	http://www.nongnu.org/orinoco/
>  S:	Maintained
>  
> +PA SEMI SMBUS DRIVER
> +P:	Olof Johansson
> +M:	olof at lixom.net
> +L:	i2c at lm-sensors.org
> +S:	Maintained
> +
>  PARALLEL PORT SUPPORT
>  P:	Phil Blundell
>  M:	philb at gnu.org

Otherwise it's fine, great work.

-- 
Jean Delvare



More information about the i2c mailing list