RE: [v7,1/1] i2c: add master driver for mellanox systems

[Vadim Pasternak]

Hi Wolfram,

Thank you for review.

> -----Original Message-----
> From: Wolfram Sang [mailto:wsa-dev@xxxxxxxxxxxxxxxxxxxx]
> Sent: Friday, November 18, 2016 12:15 AM
> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> Cc: wsa@xxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; jiri@xxxxxxxxxxx; Michael Shych
> <michaelsh@xxxxxxxxxxxx>
> Subject: Re: [v7,1/1] i2c: add master driver for mellanox systems
> 
> Hi,
> 
> thanks for the patch and the prompt reworks. Also thank you to Vladimir for
> initial reviews!
> 
> > Device supports:
> >  - Master mode
> >  - One physical bus
> >  - Polling mode
> 
> Out of interest: is there any interrupt at all?

Yes, it's possible to configure interrupt mode in HW, but we've never used it.

> 
> > diff --git a/MAINTAINERS b/MAINTAINERS index 411e3b8..26d05f8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7881,6 +7881,14 @@ W:	http://www.mellanox.com
> >  Q:	http://patchwork.ozlabs.org/project/netdev/list/
> >  F:	drivers/net/ethernet/mellanox/mlxsw/
> >
> > +MELLANOX MLXCPLD I2C DRIVER
> > +M:	Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> > +M:	Michael Shych <michaelsh@xxxxxxxxxxxx>
> > +L:	linux-i2c@xxxxxxxxxxxxxxx
> > +S:	Supported
> > +F:	drivers/i2c/busses/i2c-mlxcpld.c
> > +F:	Documentation/i2c/busses/i2c-mlxcpld
> > +
> 
> No need to change right now, but I think you could simplify all your drivers in
> one entry with
> 
> F: *mlxcpld*
> 
> or something similar to keep MAINTAINERS compact.
> 
> > +/**
> > + * struct  mlxcpld_i2c_curr_xfer - current transaction parameters:
> > + * @cmd: command;
> > + * @addr_width: address width;
> > + * @data_len: data length;
> > + * @cmd: command register;
> > + * @msg_num: message number;
> > + * @msg: pointer to message buffer;
> > + */
> 
> While I value effort to describe struct members, this is so self-explaining that I
> think it could be left out.
> 
> > +/**
> > + * struct mlxcpld_i2c_priv - private controller data:
> > + * @adap: i2c adapter;
> > + * @base_addr: base IO address;
> > + * @lock: bus access lock;
> > + * @xfer: current i2c transfer block;
> > + * @dev: device handle;
> > + */
> 
> ditto
> 
> > +/*
> > + * Check validity of current i2c message and all transfer.
> > + * Calculate also common length of all i2c messages in transfer.
> > + */
> > +static int mlxcpld_i2c_invalid_len(struct mlxcpld_i2c_priv *priv,
> > +				   const struct i2c_msg *msg, u8 *comm_len) {
> > +	u8 max_len = msg->flags == I2C_M_RD ? MLXCPLD_I2C_DATA_REG_SZ -
> > +		     MLXCPLD_I2C_MAX_ADDR_LEN :
> MLXCPLD_I2C_DATA_REG_SZ;
> > +
> > +	if (msg->len > max_len)
> > +		return -EINVAL;
> 
> If you populate a 'struct i2c_adapter_quirks' the core will do this check for you.
> 
> > +	*comm_len += msg->len;
> > +	if (*comm_len > MLXCPLD_I2C_DATA_REG_SZ)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Check validity of received i2c messages parameters.
> > + * Returns 0 if OK, other - in case of invalid parameters
> > + * or common length of data that should be passed to CPLD  */ static
> > +int mlxcpld_i2c_check_msg_params(struct mlxcpld_i2c_priv *priv,
> > +					struct i2c_msg *msgs, int num,
> > +					u8 *comm_len)
> > +{
> > +	int i;
> > +
> > +	if (!num) {
> > +		dev_err(priv->dev, "Incorrect 0 num of messages\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (unlikely(msgs[0].addr > 0x7f)) {
> > +		dev_err(priv->dev, "Invalid address 0x%03x\n",
> > +			msgs[0].addr);
> > +		return -EINVAL;
> > +	}
> > +
> > +	for (i = 0; i < num; ++i) {
> > +		if (unlikely(!msgs[i].buf)) {
> > +			dev_err(priv->dev, "Invalid buf in msg[%d]\n",
> > +				i);
> > +			return -EINVAL;
> > +		}
> 
> I was about to write "the core will check this for you", but wow, we are not
> good at this... I will try to come up with some patches soon. No need for you to
> changes this right now.
> 
> ...
> 
> > +	case MLXCPLD_LPCI2C_ACK_IND:
> > +		if (priv->xfer.cmd != I2C_M_RD)
> > +			return (priv->xfer.addr_width + priv->xfer.data_len);
> > +
> > +		if (priv->xfer.msg_num == 1)
> > +			i = 0;
> > +		else
> > +			i = 1;
> > +
> > +		if (!priv->xfer.msg[i].buf)
> > +			return -EINVAL;
> > +
> > +		/*
> > +		 * Actual read data len will be always the same as
> > +		 * requested len. 0xff (line pull-up) will be returned
> > +		 * if slave has no data to return. Thus don't read
> > +		 * MLXCPLD_LPCI2C_NUM_DAT_REG reg from CPLD.
> > +		 */
> > +		datalen = priv->xfer.data_len;
> > +
> > +		mlxcpld_i2c_read_comm(priv, MLXCPLD_LPCI2C_DATA_REG,
> > +				      priv->xfer.msg[i].buf, datalen);
> > +
> > +		return datalen;
> > +
> > +	case MLXCPLD_LPCI2C_NACK_IND:
> > +		return -EAGAIN;
> 
> -EAGAIN is for arbitration lost. -ENXIO is for NACK. See
> Documentation/i2c/fault-codes.
> 
> What kind of testing have you done with the driver?
> 

Actually we are using these driver on a wide range of different Mellanox systems (about 10 systems, TORs and modular systems), which we have on the filed about two years.
After updating it with the upstream review comments, I performed some basic set of the functional testing on a couple of our systems - access to all the devices (1, 2, 4 byte width), access to broken device (transaction TO), some kind of stress test.

> Thanks,
> 
>    Wolfram

Thanks,

Vadim.