Re: [PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter

[Wolfram Sang]

Hi,

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2ac87fa..6afc17e 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1021,4 +1021,14 @@ config SCx200_ACB
>  	  This support is also available as a module.  If so, the module
>  	  will be called scx200_acb.
>  
> +config I2C_DLN2
> +       tristate "Diolan DLN-2 USB I2C adapter"
> +       depends on MFD_DLN2
> +       help
> +         If you say yes to this option, support will be included for Diolan
> +         DLN2, a USB to I2C interface.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called i2c-dln2.
> +

Please keep to the existing sorting.


>  endmenu
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 49bf07e..3118fea 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -100,5 +100,6 @@ obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
>  obj-$(CONFIG_I2C_PCA_ISA)	+= i2c-pca-isa.o
>  obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
>  obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
> +obj-$(CONFIG_I2C_DLN2)		+= i2c-dln2.o

Ditto!

> +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
> +{
> +	int ret;
> +	u16 cmd;
> +
> +	if (enable)
> +		cmd = DLN2_I2C_ENABLE;
> +	else
> +		cmd = DLN2_I2C_DISABLE;

Ternary operator?

> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
> +			    NULL, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int dln2_i2c_set_frequency(struct dln2_i2c *dln2, u32 freq)

Here you have 'set_frequency'...

> +{
> +	int ret;
> +	struct tx_data {
> +		u8 port;
> +		__le32 freq;
> +	} __packed tx;
> +
> +	tx.port = dln2->port;
> +	tx.freq = cpu_to_le32(freq);
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_I2C_SET_FREQUENCY, &tx, sizeof(tx),
> +			    NULL, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	dln2->freq = freq;
> +
> +	return 0;
> +}
> +
> +static int dln2_i2c_get_freq(struct dln2_i2c *dln2, u16 cmd, u32 *res)

... here 'get_freq'. Please keep it consistent.

> +{
> +	int ret;
> +	__le32 freq;
> +	unsigned len = sizeof(freq);
> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
> +			    &freq, &len);
> +	if (ret < 0)
> +		return ret;
> +	if (len < sizeof(freq))
> +		return -EPROTO;
> +
> +	*res = le32_to_cpu(freq);
> +
> +	return 0;
> +}

...

> +
> +static int dln2_i2c_xfer(struct i2c_adapter *adapter,
> +			 struct i2c_msg *msgs, int num)
> +{
> +	struct dln2_i2c *dln2 = i2c_get_adapdata(adapter);
> +	struct i2c_msg *pmsg;
> +	int i;
> +
> +	for (i = 0; i < num; i++) {
> +		int ret;
> +
> +		pmsg = &msgs[i];
> +
> +		if (pmsg->len > DLN2_I2C_MAX_XFER_SIZE)
> +			return -EINVAL;

Rather -EOPNOTSUPP. And we should add a warning here since I2C transfers
can be bigger than 256 byte and this is a flaw of the controller.

> +
> +		if (pmsg->flags & I2C_M_RD) {
> +			ret = dln2_i2c_read(dln2, pmsg->addr, pmsg->buf,
> +					    pmsg->len);
> +			if (ret < 0)
> +				return ret;
> +
> +			pmsg->len = ret;
> +		} else {
> +			ret = dln2_i2c_write(dln2, pmsg->addr, pmsg->buf,
> +					     pmsg->len);
> +			if (ret != pmsg->len)
> +				return -EPROTO;
> +		}
> +	}
> +
> +	return num;
> +}
> +

Thanks,

   Wolfram

Attachment: signature.asc
Description: Digital signature