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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
