Hi!
Again, sorry for the late review.
On 2020-11-18 15:44, Vadim Pasternak wrote:
> Extend driver to allow I2C routing control through CPLD devices with
> word address space. Till now only CPLD devices with byte address space
> have been supported.
>
> Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxx>
> Reviewed-by: Michael Shych <michaelsh@xxxxxxxxxx>
> ---
> drivers/i2c/muxes/i2c-mux-mlxcpld.c | 57 +++++++++++++++++++++++++++--------
> include/linux/platform_data/mlxcpld.h | 2 ++
> 2 files changed, 47 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> index 6bb8caecf8e8..c76180919fc3 100644
> --- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> @@ -21,11 +21,13 @@
> * @last_chan - last register value
> * @client - I2C device client
> * @pdata: platform data
> + * @sel_buf: I2C message buffer for mux select 16 bits transactions
> */
> struct mlxcpld_mux {
> u8 last_chan;
> struct i2c_client *client;
> struct mlxcpld_mux_plat_data pdata;
> + u8 sel_buf[3];
I think it's a mistake to have this buffer here. I'd rather create a buffer on
the stack in mlxcpld_mux_reg_write() and fill it with values for every xfer.
Sure, I bet there are external locks that prevent any clobbering of the buffer,
but it's so small that it really can be on the stack.
> };
>
> /* MUX logic description.
> @@ -60,26 +62,42 @@ struct mlxcpld_mux {
> * for this as they will try to lock adapter a second time.
> */
> static int mlxcpld_mux_reg_write(struct i2c_adapter *adap,
> - struct mlxcpld_mux *mux, u8 val)
> + struct mlxcpld_mux *mux, int chan)
> {
> struct i2c_client *client = mux->client;
> - union i2c_smbus_data data = { .byte = val };
> -
> - return __i2c_smbus_xfer(adap, client->addr, client->flags,
> - I2C_SMBUS_WRITE, mux->pdata.sel_reg_addr,
> - I2C_SMBUS_BYTE_DATA, &data);
> + union i2c_smbus_data data;
> + struct i2c_msg msg;
> +
> + switch (mux->pdata.reg_size) {
> + case 1:
> + data.byte = (chan < 0) ? 0 : chan;
> + return __i2c_smbus_xfer(adap, client->addr, client->flags,
> + I2C_SMBUS_WRITE,
> + mux->pdata.sel_reg_addr,
> + I2C_SMBUS_BYTE_DATA, &data);
> + case 2:
> + mux->sel_buf[mux->pdata.reg_size] = (chan < 0) ? 0 :
> + mux->pdata.adap_ids[chan];
I get the feeling that you are desperatly trying to get some specific numbering in
user space.
The adapter id is one thing.
The mux channel is one thing.
The value in the register is one thing.
Often, it can make things easier with an easy mapping between the latter two,
but you program the system global I2C adapter id into the channel selection
register of the mux. That is problematic. Just don't.
> + msg.addr = client->addr;
> + msg.buf = mux->sel_buf;
> + msg.len = mux->pdata.reg_size + 1;
> + msg.flags = 0;
> + return __i2c_transfer(adap, &msg, 1);
Here you use I2C xfers for the 16-bit case...
> + default:
> + return -EINVAL;
> + }
> }
>
> static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32 chan)
> {
> struct mlxcpld_mux *mux = i2c_mux_priv(muxc);
> - u8 regval = chan + 1;
> int err = 0;
>
> /* Only select the channel if its different from the last channel */
> - if (mux->last_chan != regval) {
> - err = mlxcpld_mux_reg_write(muxc->parent, mux, regval);
> - mux->last_chan = err < 0 ? 0 : regval;
> + chan++;
I question the removal of the regval variable. See above.
> + if (mux->last_chan != chan) {
> + err = mlxcpld_mux_reg_write(muxc->parent, mux, chan);
> + mux->last_chan = err < 0 ? 0 : chan;
> }
>
> return err;
> @@ -103,13 +121,26 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
> struct i2c_mux_core *muxc;
> int num, force;
> struct mlxcpld_mux *data;
> + u16 sel_reg_addr = 0;
> + u32 func;
> int err;
>
> if (!pdata)
> return -EINVAL;
>
> - if (!i2c_check_functionality(client->adapter,
> - I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
> + switch (pdata->reg_size) {
> + case 1:
> + func = I2C_FUNC_SMBUS_WRITE_BYTE_DATA;
> + break;
> + case 2:
> + func = I2C_FUNC_SMBUS_WRITE_WORD_DATA;
...and here you setup to check for SMBUS for the 16-bit case. And the type of
SMBUS xfer is not compatible with the xfer that is actually taking place.
WRITE_WORD_DATA is 8-bit register and 16-bit data. You have the opposite.
So, this check is broken.
> + sel_reg_addr = cpu_to_be16(pdata->sel_reg_addr);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (!i2c_check_functionality(client->adapter, func))
> return -ENODEV;
>
> muxc = i2c_mux_alloc(client->adapter, &pdev->dev, CPLD_MUX_MAX_NCHANS,
> @@ -122,6 +153,8 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
> data = i2c_mux_priv(muxc);
> data->client = client;
> memcpy(&data->pdata, pdata, sizeof(*pdata));
> + /* Save mux select address for 16 bits transaction size. */
> + memcpy(data->sel_buf, &sel_reg_addr, 2);
> data->last_chan = 0; /* force the first selection */
>
> /* Create an adapter for each channel. */
> diff --git a/include/linux/platform_data/mlxcpld.h b/include/linux/platform_data/mlxcpld.h
> index e6c18bf017dd..da4f7e8f5721 100644
> --- a/include/linux/platform_data/mlxcpld.h
> +++ b/include/linux/platform_data/mlxcpld.h
> @@ -14,11 +14,13 @@
> * @adap_ids - adapter array
> * @num_adaps - number of adapters
> * @sel_reg_addr - mux select register offset in CPLD space
> + * @reg_size: register size in bytes (default 0 - 1 byte data, 1 - 2 bytes data
The reg_size isn't in bytes according to the brackeded info. Missing end bracket
as well...
Cheers,
Peter
> */
> struct mlxcpld_mux_plat_data {
> int *adap_ids;
> int num_adaps;
> int sel_reg_addr;
> + u8 reg_size;
> };
>
> #endif /* _LINUX_I2C_MLXCPLD_H */
>
