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? > 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? Thanks, Wolfram
Attachment:
signature.asc
Description: PGP signature
