[i2c-next,v2,5/7] i2c: mux: mlxcpld: Extend driver to support word address space devices

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@nvidia.com>
Reviewed-by: Michael Shych <michaelsh@nvidia.com>
---
v1->v2:
 Comments pointed out by Peter:
 - Remove data buffer allocation from 'mlxcpld_mux' structure, do it on
   stack instead.
 - Do not use array pdata.adap_ids[] in mlxcpld_mux_reg_write() for
   channel assignment.
 - Return back 'regval' variable, used for channel assignment in
   mlxcpld_mux_select_chan().
 - Fix functionality validation in mlxcpld_mux_probe().
 - Fix comment for 'reg_size' field in mlxcpld_mux_plat_data' structure.
  Added by Vadim:
  - Change type of register select address to '__be16' to align with
    type in assignment in cpu_to_be16().
---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c   | 54 ++++++++++++++++++++++++++++-------
 include/linux/platform_data/mlxcpld.h |  2 ++
 2 files changed, 46 insertions(+), 10 deletions(-)

Hi!

On 2021-01-22 20:25, 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@nvidia.com>
> Reviewed-by: Michael Shych <michaelsh@nvidia.com>
> ---
> v1->v2:
>  Comments pointed out by Peter:
>  - Remove data buffer allocation from 'mlxcpld_mux' structure, do it on
>    stack instead.
>  - Do not use array pdata.adap_ids[] in mlxcpld_mux_reg_write() for
>    channel assignment.
>  - Return back 'regval' variable, used for channel assignment in
>    mlxcpld_mux_select_chan().
>  - Fix functionality validation in mlxcpld_mux_probe().
>  - Fix comment for 'reg_size' field in mlxcpld_mux_plat_data' structure.
>   Added by Vadim:
>   - Change type of register select address to '__be16' to align with
>     type in assignment in cpu_to_be16().
> ---
>  drivers/i2c/muxes/i2c-mux-mlxcpld.c   | 54 ++++++++++++++++++++++++++++-------
>  include/linux/platform_data/mlxcpld.h |  2 ++
>  2 files changed, 46 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> index b53f1479272d..4848dd4ff41a 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_reg_addr: mux select/deselect register address
>   */
>  struct mlxcpld_mux {
>  	u8 last_chan;
>  	struct i2c_client *client;
>  	struct mlxcpld_mux_plat_data pdata;
> +	__be16 sel_reg_addr;
>  };
>  
>  /* MUX logic description.
> @@ -60,24 +62,43 @@ 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)

Changing val to chan is misleading for the one-byte case, where they
are not equal.

>  {
>  	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;
> +	u8 buf[3];
> +
> +	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:
> +		memcpy(buf, &mux->sel_reg_addr, 2);

Instead of precalculating these two bytes with cpu_to_be16 and storing the extra
copy, why not just write this as:

		buf[0] = mux->pdata.sel_reg_addr >> 8;
		buf[1] = mux->pdata.sel_reg_addr;

> +		buf[2] = chan;
> +		msg.addr = client->addr;
> +		msg.buf = buf;
> +		msg.len = mux->pdata.reg_size + 1;
> +		msg.flags = 0;
> +		return __i2c_transfer(adap, &msg, 1);
> +	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;
> +	u8 regval = chan;
>  	int err = 0;
>  
> +	if (mux->pdata.reg_size == 1)
> +		regval += 1;
> +
>  	/* Only select the channel if its different from the last channel */
> -	if (mux->last_chan != regval) {
> +	if (mux->last_chan != chan) {

This is broken. last_chan doesn't store the last channel, it stores the
last regval (or 0). I.e., you should keep comparing with regval.

If you are not comfortable with the somewhat strange name of the last_chan
variable when it in fact records the last register value, then maybe you
should change the name of it to last_regval?

>  		err = mlxcpld_mux_reg_write(muxc->parent, mux, regval);
>  		mux->last_chan = err < 0 ? 0 : regval;

Note that 0 is used to mark a special state of no selected mux, which
ensures a write to the register on the next select request. That is now
broken if the next select request happens to be for chan 0 (because that
channel now has a regval that collides with the special state, i.e. 0).

Since you cannot avoid chan 0 until patch 6/7, this is broken at this
stage.

Also, after reading this and 6/7, I wonder why the one-byte case is
preserved at all? If noone is using it now, and apparently never has,
why not just ditch it? Or am I missing something?

Cheers,
Peter

>  	}
> @@ -103,13 +124,24 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
>  	struct i2c_mux_core *muxc;
>  	int num, force;
>  	struct mlxcpld_mux *data;
> +	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_I2C;
> +		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 +154,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. */
> +	data->sel_reg_addr = cpu_to_be16(pdata->sel_reg_addr);
>  	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..9cb2c3d8293e 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
>   */
>  struct mlxcpld_mux_plat_data {
>  	int *adap_ids;
>  	int num_adaps;
>  	int sel_reg_addr;
> +	u8 reg_size;
>  };
>  
>  #endif /* _LINUX_I2C_MLXCPLD_H */
>

Hi Vadim,

On 2021-01-28 20:29, Vadim Pasternak wrote:
> Hi Peter,
> 

*snip*

> I can merge 5 and 6 to one patch.
> Would it be OK?

Please no, that's backwards.

> I would like to not drop one-byte register support.
> We have InfiniBand modular system with leaves and spines devices on which
> I need one-byte case. Code for those device was not upstream-ed yet.

If you could have dropped the one-byte code, you'd have had a lot more
freedom. If that support has to remain, then I have to assume that it
should be preserved. So, you need to make it as clear as possible that
the one-byte code still works as before. The current series makes this
hard to see. I have to read patches out of order, which should not be
needed.

Look, when you add a new driver the most important thing to look out for
is that the new code isn't adding bad interfaces or doing stupid things
like double free etc. If the hardware works or not is not really important.
If it doesn't work, nothing is lost because it didn't work before either.
But when you extend a driver to support new hardware, you need to make it
clear that the old hardware keeps working. In my experience, the best way
to do that is to, if needed, first make the existing code more flexible
but without adding any support for new hardware. I.e. first prepare the
driver. Then, when the code is in such a shape that adding the support for
the new hardware is easy, that final patch will be as easy to take for the
maintainer as a completely new driver would be.

The difficulty will always be in the preparatory patches that make the
driver more flexible while preserving the old functionality.

In this case, you have a couple of issus that you need to address before
you can add support for your new hardware.

One issue is that the one-byte code is never writing regval 0 to the
register. It is instead using that value as a hint to not short out the
*next* write of the regval. You seem to need your new two-byte code to
be able to program regval 0 to the mux register. I would solve this by
changing the 0 key to some other number, probably -1, so that the same
mechanism could be used in both cases. For that to happen last_chan
needs to be able to hold -1, so it needs to change type from u8. This
change should be done in one patch, before adding the two-byte support.
Then this issue becomes a non-issue when adding the two-byte support,
because the driver already fits the need.

One issue is that the current mechanism for selecting the actual regval
to program is inflexible; you want to be able to not create all
the child adapters, only the ones you actually need. I.e. the change from
adap_id to chan_id. That change should be done in a preparatory patch,
before adding two-byte support.

One issue is that the driver hardcodes 8 maximum child adapters, and you
need to make that more flexible. Do that change before you add two-byte
support. That could possibly be in the same patch as the above adap_id
to chan_id change, but it would not be wrong to do it as separate steps.
As a reviewer I would probably prefer it in a separate patch.

Etc, take small steps and do one logical change in each patch. That makes
reviewing much more pleasant than reviewing everything mashed together.

Cheers,
Peter