Hi Peter,

-----Original Message-----
From: Peter Rosin <redacted>
Sent: Friday, January 08, 2021 10:02 AM
To: Vadim Pasternak <vadimp@nvidia.com>; wsa@the-dreams.de
Cc: linux-i2c@vger.kernel.org
Subject: Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to
support word address space devices

Hi!

On 2021-01-07 21:43, Vadim Pasternak wrote:

quoted

Hi Peter,

Thank you very much for review.

quoted

-----Original Message-----
From: Peter Rosin <redacted>
Sent: Thursday, January 07, 2021 12:04 PM
To: Vadim Pasternak <vadimp@nvidia.com>; wsa@the-dreams.de
Cc: linux-i2c@vger.kernel.org
Subject: Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend
driver to support word address space devices

Hi!

Again, sorry for the late review.

On 2020-11-18 15:44, Vadim Pasternak wrote:

quoted

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>
---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c   | 57

+++++++++++++++++++++++++++--------

quoted

 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.

quoted

quoted

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.

quoted

 };

 /* 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.

OK, I will explain what I am trying to get.
This is not something related to the user space.

I want to access some device, located on a line card, which is replaceable.
This is for modular system, which can be equipped with the different
type of line cards.

I have mux selector register in line card CPLD, which is located at
some offset in CPLD register space, f.e. 0x25dc. On other systems it could

different offset.

quoted

For this line card type in pdata.adap_ids[] channels mapping looks like:
{
	0x04, 0x05, 0x06, 0x07, 0x08, 0x10, 0x20, 0x21, 0x22, 0x23, 0x40, 0x41,
	0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, 0x49, 0x4a, 0x4b, 0x4c, 0x4d,
	0x4e, 0x4f
};
Ids from 0x01 - 0x0f are used for access to devices like  voltage regulators,

hotswap,

quoted

	EEPROMs, iio, etcetera.
Ids from 0x10 are used for FPGAs.
Ids from 0x20 are used for gearboxes.
Ids from 0x40 are used for QSFP.
On other line card type it could be different device tree, but it
still will follow the same convention.

CPLD is connected to some upper adapter at address 0x32, and device on
line card is connected to adapter = base_nr * slot + pdata.adap_ids[channel].
For example, base_nr is 100, slot, at which line card is inserted  is
1, channel 0 will be be configured for adapter 104.

And access will be as below:
cat /sys/bus/i2c/devices/104-0062/hwmon/hwmon5/in1_input
11750

             cat-17623   [004] .... 1152583.810824: i2c_write: i2c-1 #0 a=032 f=0000

l=3 [25-dc-04]

quoted

             cat-17623   [004] .... 1152583.811276: i2c_result: i2c-1 n=1 ret=1
             cat-17623   [004] .... 1152583.811281: i2c_write: i2c-1 #0 a=062 f=0000

l=1 [88]

quoted

             cat-17623   [004] .... 1152583.811281: i2c_read: i2c-1 #1 a=062 f=0001

l=2

quoted

             cat-17623   [004] .... 1152583.811700: i2c_reply: i2c-1 #1 a=062 f=0001

l=2 [2f-f0]

quoted

             cat-17623   [004] .... 1152583.811700: i2c_result: i2c-1 n=2 ret=2
             cat-17623   [004] .... 1152583.811704: i2c_write: i2c-1 #0 a=032 f=0000

l=3 [25-dc-00]

quoted

When the same line card is inserted for example at slot 3, the
adapter, to which this device is connected will be 304.

Yes, I think I get it. You are however not introducing base_nr until 6/6, so at
this point the code makes no sense. But even after 6/6 with base_nr in place, I
suggest the following:

- The adap_ids array is for forceing the system global adapter id. Leave this
variable
  alone and let it continue to do what it does, and only that. Or...
- Since you stated somewhere that there are no users of this drivers, I'd be
happy to just
  see the adap_ids variable deleted, i.e. I see no need to force the adapter id.
- Instead of reusing adap_ids, intruduce a new "channel" array and fill it with
the same
  values that you provide in adap_ids, and then have the driver feed them to
the 3rd arg
  of i2c_mux_add_adapter(), i.e. chan_id.

Would that work for you? Or do you somehow depend on predictable adapter
ids?

I can drop adap_id[]s, use chan_ids[] instead and modify loop for adding
Adapters like:
	for (num = 0; num < pdata->num_adaps; num++) {;
		err = i2c_mux_add_adapter(muxc, pdata->base_nr + num,
					  pdata->chan_ids[num], 0);
		if (err)
			goto virt_reg_failed;
	}

In such way I can keep convention for adapters numbering for card in slot 'n',
nrs will be:
Form 100 * n + 1 - for voltage  regulators, hotswap, EEPROMs, iio, ...
From 100 *n + 16 - for FPGAs.
From 100 * n + 32 - for gearboxes.
From 100 * n + 64 - for QSFP.

Would it be OK?

quoted

So I am using  preconfigured buffer with mux address, which I want to right,

here it is 0x25dc.

quoted

On select I write channel Id to this register (sel_buf[] = { 0x25 0xdc
<adap_ids[channel]>} ), on deselect zero (sel_buf[] = { 0x25 0dc 0x00 }).

I can have a buffer on stack and set it each time
mlxcpld_mux_reg_write() is called, as you suggested.

Which API you suggest to use here for sending I2C transaction?

I suspect that this driver will only be used with a very limited list of I2C
adapters, and that all of those support whatever method you use. I also
supsect that in practice, the i2c_check_functionality() checks will always
succeed because of this, so my comments in regard to this are probably mainly
cosmetic. But it's easier to read code when things fit, and problems like that
tend to "escape" when someone reuses the code.

So, use whatever suits you, but make it consistent. :-)

However, SMBUS has 8-bit commands/registers, so it doesn't really fit. You
could still shoe-horn your xfers in there, if you desperately needed to support
some SMBUS-only adapter, but I think I would have stayed with
__i2c_transfer() for the 16-bit case.

quoted

quoted

quoted

+		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...

quoted

+	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.

I will return back 'regval' and make assignment base on register size.

quoted

quoted

+	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)

quoted

 	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.

quoted

quoted

WRITE_WORD_DATA is 8-bit register and 16-bit data. You have the

opposite.

quoted

quoted

So, this check is broken.

Yes. I have to check for I2C functionality, so it should I2C_FUNC_I2C, yes?

Yes.

Cheers,
Peter

quoted

quoted

quoted

+		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)

quoted

 	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...

Will fix it.

Thank you very much,
Vadim.

quoted

Cheers,
Peter

quoted

  */
 struct mlxcpld_mux_plat_data {
 	int *adap_ids;
 	int num_adaps;
 	int sel_reg_addr;
+	u8 reg_size;
 };

 #endif /* _LINUX_I2C_MLXCPLD_H */