[Re-send: PATCH i2c-next 0/6] i2c: mux: mlxcpld: Extend driver functionality and update licenses

[Re-send: PATCH i2c-next 0/6] i2c: mux: mlxcpld: Extend driver functionality and update licenses

[Vadim Pasternak]

The patchset adds new features for the existing Mellanox systems.

Patches #1-#2 update license to SPDX-License.
Patch #3  moves header file out of x86 realm.
Patch #4 converts driver to platform driver.
Patch #5 adds support for word address space devices.
Patch #6 extends mux number supported by driver.

Vadim Pasternak (6):
  i2c: mux: mlxcpld: Update module license
  platform/x86: mlxcpld: Update module license
  i2c: mux: mlxcpld: Move header file out of x86 realm
  i2c: mux: mlxcpld: Convert driver to platform driver
  i2c: mux: mlxcpld: Extend driver to support word address space devices
  i2c: mux: mlxcpld: Extend supported mux number

 drivers/i2c/muxes/i2c-mux-mlxcpld.c       | 153 ++++++++++++++----------------
 include/linux/platform_data/mlxcpld.h     |  29 ++++++
 include/linux/platform_data/x86/mlxcpld.h |  52 ----------
 3 files changed, 102 insertions(+), 132 deletions(-)
 create mode 100644 include/linux/platform_data/mlxcpld.h
 delete mode 100644 include/linux/platform_data/x86/mlxcpld.h

-- 
2.11.0

[Re-send: PATCH i2c-next 1/6] i2c: mux: mlxcpld: Update module license

[Vadim Pasternak]

Update license to SPDX-License.

Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c | 33 +++------------------------------
 1 file changed, 3 insertions(+), 30 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
index 5ed55ca4fe93..53bce81cf5c9 100644
--- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
+++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
@@ -1,35 +1,8 @@
+// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
 /*
- * drivers/i2c/muxes/i2c-mux-mlxcpld.c
- * Copyright (c) 2016 Mellanox Technologies. All rights reserved.
- * Copyright (c) 2016 Michael Shych <michaels@mellanox.com>
+ * Mellanox i2c mux driver
  *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are met:
- *
- * 1. Redistributions of source code must retain the above copyright
- *    notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- *    notice, this list of conditions and the following disclaimer in the
- *    documentation and/or other materials provided with the distribution.
- * 3. Neither the names of the copyright holders nor the names of its
- *    contributors may be used to endorse or promote products derived from
- *    this software without specific prior written permission.
- *
- * Alternatively, this software may be distributed under the terms of the
- * GNU General Public License ("GPL") version 2 as published by the Free
- * Software Foundation.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
- * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
- * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
- * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
- * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
- * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
- * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
- * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
- * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
- * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
- * POSSIBILITY OF SUCH DAMAGE.
+ * Copyright (C) 2016-2020 Mellanox Technologies
  */
 
 #include <linux/device.h>
-- 
2.11.0

[Re-send: PATCH i2c-next 2/6] platform/x86: mlxcpld: Update module license

[Vadim Pasternak]

Update license to SPDX-License.

Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
---
 include/linux/platform_data/x86/mlxcpld.h | 34 +++----------------------------
 1 file changed, 3 insertions(+), 31 deletions(-)

diff --git a/include/linux/platform_data/x86/mlxcpld.h b/include/linux/platform_data/x86/mlxcpld.h
index b08dcb183fca..e6c18bf017dd 100644
--- a/include/linux/platform_data/x86/mlxcpld.h
+++ b/include/linux/platform_data/x86/mlxcpld.h
@@ -1,36 +1,8 @@
+/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */
 /*
- * mlxcpld.h - Mellanox I2C multiplexer support in CPLD
+ * Mellanox I2C multiplexer support in CPLD
  *
- * Copyright (c) 2016 Mellanox Technologies. All rights reserved.
- * Copyright (c) 2016 Michael Shych <michaels@mellanox.com>
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are met:
- *
- * 1. Redistributions of source code must retain the above copyright
- *    notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- *    notice, this list of conditions and the following disclaimer in the
- *    documentation and/or other materials provided with the distribution.
- * 3. Neither the names of the copyright holders nor the names of its
- *    contributors may be used to endorse or promote products derived from
- *    this software without specific prior written permission.
- *
- * Alternatively, this software may be distributed under the terms of the
- * GNU General Public License ("GPL") version 2 as published by the Free
- * Software Foundation.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
- * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
- * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
- * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
- * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
- * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
- * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
- * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
- * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
- * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
- * POSSIBILITY OF SUCH DAMAGE.
+ * Copyright (C) 2016-2020 Mellanox Technologies
  */
 
 #ifndef _LINUX_I2C_MLXCPLD_H
-- 
2.11.0

[Re-send: PATCH i2c-next 3/6] i2c: mux: mlxcpld: Move header file out of x86 realm

[Vadim Pasternak]

Move out header file from include/linux/platform_data/x86/ to
include/linux/platform_data/, since it does not depend on x86
architecture.

Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
Reviewed-by: Michael Shych <michaelsh@nvidia.com>
---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c             | 2 +-
 include/linux/platform_data/{x86 => }/mlxcpld.h | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename include/linux/platform_data/{x86 => }/mlxcpld.h (100%)

diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
index 53bce81cf5c9..3d894cfb19df 100644
--- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
+++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
@@ -11,7 +11,7 @@
 #include <linux/io.h>
 #include <linux/init.h>
 #include <linux/module.h>
-#include <linux/platform_data/x86/mlxcpld.h>
+#include <linux/platform_data/mlxcpld.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
diff --git a/include/linux/platform_data/x86/mlxcpld.h b/include/linux/platform_data/mlxcpld.h
similarity index 100%
rename from include/linux/platform_data/x86/mlxcpld.h
rename to include/linux/platform_data/mlxcpld.h
-- 
2.11.0

[Re-send: PATCH i2c-next 4/6] i2c: mux: mlxcpld: Convert driver to platform driver

[Vadim Pasternak]

Convert driver from 'i2c' to 'platform'.
The motivation is to avoid I2C addressing conflict between
‘i2c-mux-cpld’ driver, providing mux selection and deselection through
CPLD ‘mux control’ register, and CPLD host driver. The CPLD is I2C
device and is multi-functional device performing logic for different
components, like LED, ‘hwmon’, interrupt control, watchdog etcetera.
For such configuration CPLD should be host I2C device, connected to the
relevant I2C bus with the relevant I2C address and all others component
drivers are supposed to be its children.
The hierarchy in such case will be like in the below example:
ls /sys/bus/i2c/devices/44-0032
i2c-mux-mlxcpld.44  leds-mlxreg.44  mlxreg-io.44
ls /sys/bus/i2c/devices/44-0032/i2c-mux-mlxcpld.44
channel-0, …,  channel-X

Currently this driver is not activated by any kernel driver,
so this conversion doesn’t affect any user.

Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
Reviewed-by: Michael Shych <michaelsh@nvidia.com>
---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c | 62 +++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 34 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
index 3d894cfb19df..6bb8caecf8e8 100644
--- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
+++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
@@ -20,10 +20,12 @@
 /* mlxcpld_mux - mux control structure:
  * @last_chan - last register value
  * @client - I2C device client
+ * @pdata: platform data
  */
 struct mlxcpld_mux {
 	u8 last_chan;
 	struct i2c_client *client;
+	struct mlxcpld_mux_plat_data pdata;
 };
 
 /* MUX logic description.
@@ -54,37 +56,30 @@ struct mlxcpld_mux {
  *
  */
 
-static const struct i2c_device_id mlxcpld_mux_id[] = {
-	{ "mlxcpld_mux_module", 0 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, mlxcpld_mux_id);
-
 /* Write to mux register. Don't use i2c_transfer() and i2c_smbus_xfer()
  * for this as they will try to lock adapter a second time.
  */
 static int mlxcpld_mux_reg_write(struct i2c_adapter *adap,
-				 struct i2c_client *client, u8 val)
+				 struct mlxcpld_mux *mux, u8 val)
 {
-	struct mlxcpld_mux_plat_data *pdata = dev_get_platdata(&client->dev);
+	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, pdata->sel_reg_addr,
+				I2C_SMBUS_WRITE, mux->pdata.sel_reg_addr,
 				I2C_SMBUS_BYTE_DATA, &data);
 }
 
 static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32 chan)
 {
-	struct mlxcpld_mux *data = i2c_mux_priv(muxc);
-	struct i2c_client *client = data->client;
+	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 (data->last_chan != regval) {
-		err = mlxcpld_mux_reg_write(muxc->parent, client, regval);
-		data->last_chan = err < 0 ? 0 : regval;
+	if (mux->last_chan != regval) {
+		err = mlxcpld_mux_reg_write(muxc->parent, mux, regval);
+		mux->last_chan = err < 0 ? 0 : regval;
 	}
 
 	return err;
@@ -92,21 +87,19 @@ static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32 chan)
 
 static int mlxcpld_mux_deselect(struct i2c_mux_core *muxc, u32 chan)
 {
-	struct mlxcpld_mux *data = i2c_mux_priv(muxc);
-	struct i2c_client *client = data->client;
+	struct mlxcpld_mux *mux = i2c_mux_priv(muxc);
 
 	/* Deselect active channel */
-	data->last_chan = 0;
+	mux->last_chan = -1;
 
-	return mlxcpld_mux_reg_write(muxc->parent, client, data->last_chan);
+	return mlxcpld_mux_reg_write(muxc->parent, mux, mux->last_chan);
 }
 
 /* Probe/reomove functions */
-static int mlxcpld_mux_probe(struct i2c_client *client,
-			     const struct i2c_device_id *id)
+static int mlxcpld_mux_probe(struct platform_device *pdev)
 {
-	struct i2c_adapter *adap = client->adapter;
-	struct mlxcpld_mux_plat_data *pdata = dev_get_platdata(&client->dev);
+	struct mlxcpld_mux_plat_data *pdata = dev_get_platdata(&pdev->dev);
+	struct i2c_client *client = to_i2c_client(pdev->dev.parent);
 	struct i2c_mux_core *muxc;
 	int num, force;
 	struct mlxcpld_mux *data;
@@ -115,18 +108,20 @@ static int mlxcpld_mux_probe(struct i2c_client *client,
 	if (!pdata)
 		return -EINVAL;
 
-	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
 		return -ENODEV;
 
-	muxc = i2c_mux_alloc(adap, &client->dev, CPLD_MUX_MAX_NCHANS,
+	muxc = i2c_mux_alloc(client->adapter, &pdev->dev, CPLD_MUX_MAX_NCHANS,
 			     sizeof(*data), 0, mlxcpld_mux_select_chan,
 			     mlxcpld_mux_deselect);
 	if (!muxc)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, muxc);
 	data = i2c_mux_priv(muxc);
-	i2c_set_clientdata(client, muxc);
 	data->client = client;
+	memcpy(&data->pdata, pdata, sizeof(*pdata));
 	data->last_chan = 0; /* force the first selection */
 
 	/* Create an adapter for each channel. */
@@ -149,24 +144,23 @@ static int mlxcpld_mux_probe(struct i2c_client *client,
 	return err;
 }
 
-static int mlxcpld_mux_remove(struct i2c_client *client)
+static int mlxcpld_mux_remove(struct platform_device *pdev)
 {
-	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
+	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
 
 	i2c_mux_del_adapters(muxc);
 	return 0;
 }
 
-static struct i2c_driver mlxcpld_mux_driver = {
-	.driver		= {
-		.name	= "mlxcpld-mux",
+static struct platform_driver mlxcpld_mux_driver = {
+	.driver = {
+		.name = "i2c-mux-mlxcpld",
 	},
-	.probe		= mlxcpld_mux_probe,
-	.remove		= mlxcpld_mux_remove,
-	.id_table	= mlxcpld_mux_id,
+	.probe = mlxcpld_mux_probe,
+	.remove = mlxcpld_mux_remove,
 };
 
-module_i2c_driver(mlxcpld_mux_driver);
+module_platform_driver(mlxcpld_mux_driver);
 
 MODULE_AUTHOR("Michael Shych (michaels@mellanox.com)");
 MODULE_DESCRIPTION("Mellanox I2C-CPLD-MUX driver");
-- 
2.11.0

[Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[Vadim Pasternak]

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 +++++++++++++++++++++++++++--------
 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];
 };
 
 /* 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];
+		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);
+	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++;
+	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;
+		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
  */
 struct mlxcpld_mux_plat_data {
 	int *adap_ids;
 	int num_adaps;
 	int sel_reg_addr;
+	u8 reg_size;
 };
 
 #endif /* _LINUX_I2C_MLXCPLD_H */
-- 
2.11.0

[Re-send: PATCH i2c-next 6/6] i2c: mux: mlxcpld: Extend supported mux number

[Vadim Pasternak]

Allow to extend mux number supported by driver.
Currently it is limited by eight, which is not enough for new coming
Mellanox modular system with line cards, which require up to 64 mux
support.

Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
Reviewed-by: Michael Shych <michaelsh@nvidia.com>
---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c   | 17 +++++------------
 include/linux/platform_data/mlxcpld.h |  3 +++
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
index c76180919fc3..760636b507fa 100644
--- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
+++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
@@ -15,8 +15,6 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
-#define CPLD_MUX_MAX_NCHANS	8
-
 /* mlxcpld_mux - mux control structure:
  * @last_chan - last register value
  * @client - I2C device client
@@ -24,7 +22,7 @@
  * @sel_buf: I2C message buffer for mux select 16 bits transactions
  */
 struct mlxcpld_mux {
-	u8 last_chan;
+	int last_chan;
 	struct i2c_client *client;
 	struct mlxcpld_mux_plat_data pdata;
 	u8 sel_buf[3];
@@ -94,7 +92,6 @@ static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32 chan)
 	int err = 0;
 
 	/* Only select the channel if its different from the last channel */
-	chan++;
 	if (mux->last_chan != chan) {
 		err = mlxcpld_mux_reg_write(muxc->parent, mux, chan);
 		mux->last_chan = err < 0 ? 0 : chan;
@@ -143,7 +140,7 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
 	if (!i2c_check_functionality(client->adapter, func))
 		return -ENODEV;
 
-	muxc = i2c_mux_alloc(client->adapter, &pdev->dev, CPLD_MUX_MAX_NCHANS,
+	muxc = i2c_mux_alloc(client->adapter, &pdev->dev, pdata->num_adaps,
 			     sizeof(*data), 0, mlxcpld_mux_select_chan,
 			     mlxcpld_mux_deselect);
 	if (!muxc)
@@ -158,13 +155,9 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
 	data->last_chan = 0; /* force the first selection */
 
 	/* Create an adapter for each channel. */
-	for (num = 0; num < CPLD_MUX_MAX_NCHANS; num++) {
-		if (num >= pdata->num_adaps)
-			/* discard unconfigured channels */
-			break;
-
-		force = pdata->adap_ids[num];
-
+	for (num = 0; num < pdata->num_adaps; num++) {
+		force = pdata->base_nr ? (pdata->base_nr +
+			pdata->adap_ids[num]) : pdata->adap_ids[num];
 		err = i2c_mux_add_adapter(muxc, force, num, 0);
 		if (err)
 			goto virt_reg_failed;
diff --git a/include/linux/platform_data/mlxcpld.h b/include/linux/platform_data/mlxcpld.h
index da4f7e8f5721..ea88817b3b35 100644
--- a/include/linux/platform_data/mlxcpld.h
+++ b/include/linux/platform_data/mlxcpld.h
@@ -11,12 +11,15 @@
 /* Platform data for the CPLD I2C multiplexers */
 
 /* mlxcpld_mux_plat_data - per mux data, used with i2c_register_board_info
+ * @base_nr: base I2C bus number to number adapters from or zero for setting
+ *	     to adap_ids vector
  * @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
  */
 struct mlxcpld_mux_plat_data {
+	int base_nr;
 	int *adap_ids;
 	int num_adaps;
 	int sel_reg_addr;
-- 
2.11.0

Re: [Re-send: PATCH i2c-next 0/6] i2c: mux: mlxcpld: Extend driver functionality and update licenses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 564 bytes --]

On Wed, Nov 18, 2020 at 04:44:10PM +0200, Vadim Pasternak wrote:
> The patchset adds new features for the existing Mellanox systems.
> 
> Patches #1-#2 update license to SPDX-License.
> Patch #3  moves header file out of x86 realm.
> Patch #4 converts driver to platform driver.
> Patch #5 adds support for word address space devices.
> Patch #6 extends mux number supported by driver.

Hi Peter,

I hope you had a good start in 2021 and all things are good. Do you have
time to review this series in the near future?

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Re-send: PATCH i2c-next 4/6] i2c: mux: mlxcpld: Convert driver to platform driver

[Peter Rosin]

Hi!

I appologize for the late review. Sorry about that.

On 2020-11-18 15:44, Vadim Pasternak wrote:
> Convert driver from 'i2c' to 'platform'.
> The motivation is to avoid I2C addressing conflict between
> ‘i2c-mux-cpld’ driver, providing mux selection and deselection through
> CPLD ‘mux control’ register, and CPLD host driver. The CPLD is I2C
> device and is multi-functional device performing logic for different
> components, like LED, ‘hwmon’, interrupt control, watchdog etcetera.
> For such configuration CPLD should be host I2C device, connected to the
> relevant I2C bus with the relevant I2C address and all others component
> drivers are supposed to be its children.
> The hierarchy in such case will be like in the below example:
> ls /sys/bus/i2c/devices/44-0032
> i2c-mux-mlxcpld.44  leds-mlxreg.44  mlxreg-io.44
> ls /sys/bus/i2c/devices/44-0032/i2c-mux-mlxcpld.44
> channel-0, …,  channel-X
> 
> Currently this driver is not activated by any kernel driver,
> so this conversion doesn’t affect any user.
> 
> Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
> Reviewed-by: Michael Shych <michaelsh@nvidia.com>
> ---
>  drivers/i2c/muxes/i2c-mux-mlxcpld.c | 62 +++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> index 3d894cfb19df..6bb8caecf8e8 100644
> --- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> @@ -20,10 +20,12 @@
>  /* mlxcpld_mux - mux control structure:
>   * @last_chan - last register value
>   * @client - I2C device client
> + * @pdata: platform data
>   */
>  struct mlxcpld_mux {
>  	u8 last_chan;
>  	struct i2c_client *client;
> +	struct mlxcpld_mux_plat_data pdata;
>  };
>  
>  /* MUX logic description.
> @@ -54,37 +56,30 @@ struct mlxcpld_mux {
>   *
>   */
>  
> -static const struct i2c_device_id mlxcpld_mux_id[] = {
> -	{ "mlxcpld_mux_module", 0 },
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(i2c, mlxcpld_mux_id);
> -
>  /* Write to mux register. Don't use i2c_transfer() and i2c_smbus_xfer()
>   * for this as they will try to lock adapter a second time.
>   */
>  static int mlxcpld_mux_reg_write(struct i2c_adapter *adap,
> -				 struct i2c_client *client, u8 val)
> +				 struct mlxcpld_mux *mux, u8 val)
>  {
> -	struct mlxcpld_mux_plat_data *pdata = dev_get_platdata(&client->dev);
> +	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, pdata->sel_reg_addr,
> +				I2C_SMBUS_WRITE, mux->pdata.sel_reg_addr,
>  				I2C_SMBUS_BYTE_DATA, &data);
>  }
>  
>  static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32 chan)
>  {
> -	struct mlxcpld_mux *data = i2c_mux_priv(muxc);
> -	struct i2c_client *client = data->client;
> +	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 (data->last_chan != regval) {
> -		err = mlxcpld_mux_reg_write(muxc->parent, client, regval);
> -		data->last_chan = err < 0 ? 0 : regval;
> +	if (mux->last_chan != regval) {
> +		err = mlxcpld_mux_reg_write(muxc->parent, mux, regval);
> +		mux->last_chan = err < 0 ? 0 : regval;
>  	}
>  
>  	return err;
> @@ -92,21 +87,19 @@ static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32 chan)
>  
>  static int mlxcpld_mux_deselect(struct i2c_mux_core *muxc, u32 chan)
>  {
> -	struct mlxcpld_mux *data = i2c_mux_priv(muxc);
> -	struct i2c_client *client = data->client;
> +	struct mlxcpld_mux *mux = i2c_mux_priv(muxc);
>  
>  	/* Deselect active channel */
> -	data->last_chan = 0;
> +	mux->last_chan = -1;

Why do you change from 0 to -1 here? That change seems totally unrelated and
such a subtle change deserves its own commit with its own motivation.

Cheers,
Peter

>  
> -	return mlxcpld_mux_reg_write(muxc->parent, client, data->last_chan);
> +	return mlxcpld_mux_reg_write(muxc->parent, mux, mux->last_chan);
>  }
>  
>  /* Probe/reomove functions */
> -static int mlxcpld_mux_probe(struct i2c_client *client,
> -			     const struct i2c_device_id *id)
> +static int mlxcpld_mux_probe(struct platform_device *pdev)
>  {
> -	struct i2c_adapter *adap = client->adapter;
> -	struct mlxcpld_mux_plat_data *pdata = dev_get_platdata(&client->dev);
> +	struct mlxcpld_mux_plat_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct i2c_client *client = to_i2c_client(pdev->dev.parent);
>  	struct i2c_mux_core *muxc;
>  	int num, force;
>  	struct mlxcpld_mux *data;
> @@ -115,18 +108,20 @@ static int mlxcpld_mux_probe(struct i2c_client *client,
>  	if (!pdata)
>  		return -EINVAL;
>  
> -	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
>  		return -ENODEV;
>  
> -	muxc = i2c_mux_alloc(adap, &client->dev, CPLD_MUX_MAX_NCHANS,
> +	muxc = i2c_mux_alloc(client->adapter, &pdev->dev, CPLD_MUX_MAX_NCHANS,
>  			     sizeof(*data), 0, mlxcpld_mux_select_chan,
>  			     mlxcpld_mux_deselect);
>  	if (!muxc)
>  		return -ENOMEM;
>  
> +	platform_set_drvdata(pdev, muxc);
>  	data = i2c_mux_priv(muxc);
> -	i2c_set_clientdata(client, muxc);
>  	data->client = client;
> +	memcpy(&data->pdata, pdata, sizeof(*pdata));
>  	data->last_chan = 0; /* force the first selection */
>  
>  	/* Create an adapter for each channel. */
> @@ -149,24 +144,23 @@ static int mlxcpld_mux_probe(struct i2c_client *client,
>  	return err;
>  }
>  
> -static int mlxcpld_mux_remove(struct i2c_client *client)
> +static int mlxcpld_mux_remove(struct platform_device *pdev)
>  {
> -	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> +	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
>  
>  	i2c_mux_del_adapters(muxc);
>  	return 0;
>  }
>  
> -static struct i2c_driver mlxcpld_mux_driver = {
> -	.driver		= {
> -		.name	= "mlxcpld-mux",
> +static struct platform_driver mlxcpld_mux_driver = {
> +	.driver = {
> +		.name = "i2c-mux-mlxcpld",
>  	},
> -	.probe		= mlxcpld_mux_probe,
> -	.remove		= mlxcpld_mux_remove,
> -	.id_table	= mlxcpld_mux_id,
> +	.probe = mlxcpld_mux_probe,
> +	.remove = mlxcpld_mux_remove,
>  };
>  
> -module_i2c_driver(mlxcpld_mux_driver);
> +module_platform_driver(mlxcpld_mux_driver);
>  
>  MODULE_AUTHOR("Michael Shych (michaels@mellanox.com)");
>  MODULE_DESCRIPTION("Mellanox I2C-CPLD-MUX driver");
> 

Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[Peter Rosin]

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@nvidia.com>
> Reviewed-by: Michael Shych <michaelsh@nvidia.com>
> ---
>  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 */
> 

Re: [Re-send: PATCH i2c-next 6/6] i2c: mux: mlxcpld: Extend supported mux number

[Peter Rosin]

Hi!

Sorry for the late review.

On 2020-11-18 15:44, Vadim Pasternak wrote:
> Allow to extend mux number supported by driver.
> Currently it is limited by eight, which is not enough for new coming
> Mellanox modular system with line cards, which require up to 64 mux
> support.
> 
> Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
> Reviewed-by: Michael Shych <michaelsh@nvidia.com>
> ---
>  drivers/i2c/muxes/i2c-mux-mlxcpld.c   | 17 +++++------------
>  include/linux/platform_data/mlxcpld.h |  3 +++
>  2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> index c76180919fc3..760636b507fa 100644
> --- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> @@ -15,8 +15,6 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
> -#define CPLD_MUX_MAX_NCHANS	8
> -
>  /* mlxcpld_mux - mux control structure:
>   * @last_chan - last register value
>   * @client - I2C device client
> @@ -24,7 +22,7 @@
>   * @sel_buf: I2C message buffer for mux select 16 bits transactions
>   */
>  struct mlxcpld_mux {
> -	u8 last_chan;
> +	int last_chan;
>  	struct i2c_client *client;
>  	struct mlxcpld_mux_plat_data pdata;
>  	u8 sel_buf[3];
> @@ -94,7 +92,6 @@ static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32 chan)
>  	int err = 0;
>  
>  	/* Only select the channel if its different from the last channel */
> -	chan++;

This breaks a property of _deselect which previously always triggered a write to the
register. It's also a subtle change without any motivation. How is this connected to
extending the mux channel count?

>  	if (mux->last_chan != chan) {
>  		err = mlxcpld_mux_reg_write(muxc->parent, mux, chan);
>  		mux->last_chan = err < 0 ? 0 : chan;
> @@ -143,7 +140,7 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
>  	if (!i2c_check_functionality(client->adapter, func))
>  		return -ENODEV;
>  
> -	muxc = i2c_mux_alloc(client->adapter, &pdev->dev, CPLD_MUX_MAX_NCHANS,
> +	muxc = i2c_mux_alloc(client->adapter, &pdev->dev, pdata->num_adaps,
>  			     sizeof(*data), 0, mlxcpld_mux_select_chan,
>  			     mlxcpld_mux_deselect);
>  	if (!muxc)
> @@ -158,13 +155,9 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
>  	data->last_chan = 0; /* force the first selection */
>  
>  	/* Create an adapter for each channel. */
> -	for (num = 0; num < CPLD_MUX_MAX_NCHANS; num++) {
> -		if (num >= pdata->num_adaps)
> -			/* discard unconfigured channels */
> -			break;
> -
> -		force = pdata->adap_ids[num];
> -
> +	for (num = 0; num < pdata->num_adaps; num++) {
> +		force = pdata->base_nr ? (pdata->base_nr +
> +			pdata->adap_ids[num]) : pdata->adap_ids[num];
>  		err = i2c_mux_add_adapter(muxc, force, num, 0);
>  		if (err)
>  			goto virt_reg_failed;
> diff --git a/include/linux/platform_data/mlxcpld.h b/include/linux/platform_data/mlxcpld.h
> index da4f7e8f5721..ea88817b3b35 100644
> --- a/include/linux/platform_data/mlxcpld.h
> +++ b/include/linux/platform_data/mlxcpld.h
> @@ -11,12 +11,15 @@
>  /* Platform data for the CPLD I2C multiplexers */
>  
>  /* mlxcpld_mux_plat_data - per mux data, used with i2c_register_board_info
> + * @base_nr: base I2C bus number to number adapters from or zero for setting
> + *	     to adap_ids vector
>   * @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
>   */
>  struct mlxcpld_mux_plat_data {
> +	int base_nr;

The changes related to adding a base is not mentioned in the commit message. It's
totally unrelated to extending the number of supported channels, AFAICT.

Cheers,
Peter

>  	int *adap_ids;
>  	int num_adaps;
>  	int sel_reg_addr;
> 

RE: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[Vadim Pasternak]

Hi Peter,

Thank you very much for review.

> -----Original Message-----
> From: Peter Rosin <peda@axentia.se>
> 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:
> > 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
> +++++++++++++++++++++++++++--------
> >  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.

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.

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,
	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]
             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]
             cat-17623   [004] .... 1152583.811281: i2c_read: i2c-1 #1 a=062 f=0001 l=2
             cat-17623   [004] .... 1152583.811700: i2c_reply: i2c-1 #1 a=062 f=0001 l=2 [2f-f0]
             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]

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

So I am using  preconfigured buffer with mux address, which I want to right, here it is 0x25dc.
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?

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

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

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

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

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

Will fix it.

Thank you very much,
Vadim.

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

Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[Peter Rosin]

Hi!

On 2021-01-07 21:43, Vadim Pasternak wrote:
> Hi Peter,
> 
> Thank you very much for review.
> 
>> -----Original Message-----
>> From: Peter Rosin <peda@axentia.se>
>> 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:
>>> 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
>> +++++++++++++++++++++++++++--------
>>>  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.
> 
> 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.
> 
> 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,
> 	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]
>              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]
>              cat-17623   [004] .... 1152583.811281: i2c_read: i2c-1 #1 a=062 f=0001 l=2
>              cat-17623   [004] .... 1152583.811700: i2c_reply: i2c-1 #1 a=062 f=0001 l=2 [2f-f0]
>              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]
> 
> 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?

> So I am using  preconfigured buffer with mux address, which I want to right, here it is 0x25dc.
> 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.

>>
>>> +		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.
> 
> I will return back 'regval' and make assignment base on register size.
> 
>>
>>> +	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.
> 
> Yes. I have to check for I2C functionality, so it should I2C_FUNC_I2C, yes?

Yes.

Cheers,
Peter

>>
>>> +		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...
> 
> Will fix it.
> 
> Thank you very much,
> Vadim.
> 
>>
>> 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 */
>>>

RE: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[Vadim Pasternak]

Hi Peter,

> -----Original Message-----
> From: Peter Rosin <peda@axentia.se>
> 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:
> > Hi Peter,
> >
> > Thank you very much for review.
> >
> >> -----Original Message-----
> >> From: Peter Rosin <peda@axentia.se>
> >> 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:
> >>> 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
> >> +++++++++++++++++++++++++++--------
> >>>  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.
> >
> > 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.
> >
> > 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,
> > 	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]
> >              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]
> >              cat-17623   [004] .... 1152583.811281: i2c_read: i2c-1 #1 a=062 f=0001
> l=2
> >              cat-17623   [004] .... 1152583.811700: i2c_reply: i2c-1 #1 a=062 f=0001
> l=2 [2f-f0]
> >              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]
> >
> > 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?

> 
> > So I am using  preconfigured buffer with mux address, which I want to right,
> here it is 0x25dc.
> > 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.
> 
> >>
> >>> +		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.
> >
> > I will return back 'regval' and make assignment base on register size.
> >
> >>
> >>> +	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.
> >
> > Yes. I have to check for I2C functionality, so it should I2C_FUNC_I2C, yes?
> 
> Yes.
> 
> Cheers,
> Peter
> 
> >>
> >>> +		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...
> >
> > Will fix it.
> >
> > Thank you very much,
> > Vadim.
> >
> >>
> >> 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 */
> >>>

Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[Peter Rosin]

Hi!

On 2021-01-11 19:11, Vadim Pasternak wrote:
> Hi Peter,
> 
>> -----Original Message-----
>> From: Peter Rosin <peda@axentia.se>
>> 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:
>>> Hi Peter,
>>>
>>> Thank you very much for review.
>>>
>>>> -----Original Message-----
>>>> From: Peter Rosin <peda@axentia.se>
>>>> 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:
>>>>> 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
>>>> +++++++++++++++++++++++++++--------
>>>>>  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.
>>>
>>> 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.
>>>
>>> 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,
>>> 	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]
>>>              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]
>>>              cat-17623   [004] .... 1152583.811281: i2c_read: i2c-1 #1 a=062 f=0001
>> l=2
>>>              cat-17623   [004] .... 1152583.811700: i2c_reply: i2c-1 #1 a=062 f=0001
>> l=2 [2f-f0]
>>>              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]
>>>
>>> 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?

What convention are you talking about? What makes it interesting to force specific
adapter IDs? I just don't see the point. I would do

		err = i2c_mux_add_adapter(muxc, 0, pdata->chan_ids[num], 0);

and let the adapted ID be whatever the I2C core makes up.

What's wrong with that?

Trying to force specific adapater IDs risks failure whenever any of those IDs
happen to be taken, and you have no way of preallocating some range the I2C
core should not use. The only way to do what you do is to select some
high enough ID range and hope for the best. But what if someone else does the
same thing? It's just a slippery slope. So, why?

Cheers,
Peter

RE: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[Vadim Pasternak]

Hi Peter,

> -----Original Message-----
> From: Peter Rosin <peda@axentia.se>
> Sent: Monday, January 11, 2021 11:23 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!
> 
> On 2021-01-11 19:11, Vadim Pasternak wrote:
> > Hi Peter,
> >
> >> -----Original Message-----
> >> From: Peter Rosin <peda@axentia.se>
> >> 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:
> >>> Hi Peter,
> >>>
> >>> Thank you very much for review.
> >>>
> >>>> -----Original Message-----
> >>>> From: Peter Rosin <peda@axentia.se>
> >>>> 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:
> >>>>> 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
> >>>> +++++++++++++++++++++++++++--------
> >>>>>  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.
> >>>
> >>> 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.
> >>>
> >>> 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,
> >>> 	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]
> >>>              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]
> >>>              cat-17623   [004] .... 1152583.811281: i2c_read: i2c-1 #1 a=062
> f=0001
> >> l=2
> >>>              cat-17623   [004] .... 1152583.811700: i2c_reply: i2c-1 #1 a=062
> f=0001
> >> l=2 [2f-f0]
> >>>              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]
> >>>
> >>> 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?
> 
> What convention are you talking about? What makes it interesting to force
> specific adapter IDs? I just don't see the point. I would do
> 
> 		err = i2c_mux_add_adapter(muxc, 0, pdata->chan_ids[num],
> 0);
> 
> and let the adapted ID be whatever the I2C core makes up.
> 
> What's wrong with that?

The motivation it to provide support for new modular systems which
could be equipped with the different types of replaceable line cards
and management board.

The line cards could be of different types and could have different
I2C topolgy:
- Line cards with 16x100Gbe QSFP28 Ethernet ports.
- Line cards with 8x200Gbe QSFP28 Ethernet ports.
- Line cards with 4x400Gbe QSFP-DD Ethernet ports.
- Smart cards equipped with Nvidia ARM CPU for offloading and for fast
  access to the storage (EBoF).
- Fabric InfiniBand cards for inter-connection.

The first version of modular system will be equipped with 8 slots.

With no enforcement, for example, it could be the next bus assignments:
- if system is booted with empty slot number one, and with line card at slot
  number 2, i2c devices i2c-{n1} - i2c-{n2} will be created for line card at the slot 2.
- if system is booted with line cards at slot 1 and at slot 2, devices i2c-{n1} - i2c-{n2}
  this time will be associated with line card at slot 1, while i2c-{n2+1} - i2c-{n2*2} will
  be associated with line card at slot 2.
- line cards could are removed and then re-inserted in some random order, and it'll
  also could change bus indexes for line card inserted at the same slot.   

It'll make a big challenge for any user application, which wants to use /dev/i2c-{x}.

With enforcement I can avoid this situation.
So, for fixed system it would be fine to have base_nr equal zero, but for modular
I'd like to have base_nr = f(slot), f.e. 100 * slot.

> 
> Trying to force specific adapater IDs risks failure whenever any of those IDs
> happen to be taken, and you have no way of preallocating some range the I2C
> core should not use. The only way to do what you do is to select some high
> enough ID range and hope for the best. But what if someone else does the
> same thing? It's just a slippery slope. So, why?

Yes, it could happen with fixed system, but in this case I expect base_nr to be
zero and IDs will be allocated just from free pool.
But for modular system availability of specific IDs could be granted.

Otherwise I'll have IDs reordering, each time when some devices are removed/
inserted.

Cheers,
Vadim.

> 
> Cheers,
> Peter

Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[Peter Rosin]

Hi!

Wolfram, question for you below!

On 2021-01-12 00:24, Vadim Pasternak wrote:
> Hi Peter,
> 
>> -----Original Message-----
>> From: Peter Rosin <peda@axentia.se>
>> Sent: Monday, January 11, 2021 11:23 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!
>>
>> On 2021-01-11 19:11, Vadim Pasternak wrote:
>>> Hi Peter,
>>>
>>>> -----Original Message-----
>>>> From: Peter Rosin <peda@axentia.se>
>>>> 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:
>>>>> Hi Peter,
>>>>>
>>>>> Thank you very much for review.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Peter Rosin <peda@axentia.se>
>>>>>> 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:
>>>>>>> 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
>>>>>> +++++++++++++++++++++++++++--------
>>>>>>>  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.
>>>>>
>>>>> 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.
>>>>>
>>>>> 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,
>>>>> 	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]
>>>>>              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]
>>>>>              cat-17623   [004] .... 1152583.811281: i2c_read: i2c-1 #1 a=062
>> f=0001
>>>> l=2
>>>>>              cat-17623   [004] .... 1152583.811700: i2c_reply: i2c-1 #1 a=062
>> f=0001
>>>> l=2 [2f-f0]
>>>>>              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]
>>>>>
>>>>> 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?
>>
>> What convention are you talking about? What makes it interesting to force
>> specific adapter IDs? I just don't see the point. I would do
>>
>> 		err = i2c_mux_add_adapter(muxc, 0, pdata->chan_ids[num],
>> 0);
>>
>> and let the adapted ID be whatever the I2C core makes up.
>>
>> What's wrong with that?
> 
> The motivation it to provide support for new modular systems which
> could be equipped with the different types of replaceable line cards
> and management board.
> 
> The line cards could be of different types and could have different
> I2C topolgy:
> - Line cards with 16x100Gbe QSFP28 Ethernet ports.
> - Line cards with 8x200Gbe QSFP28 Ethernet ports.
> - Line cards with 4x400Gbe QSFP-DD Ethernet ports.
> - Smart cards equipped with Nvidia ARM CPU for offloading and for fast
>   access to the storage (EBoF).
> - Fabric InfiniBand cards for inter-connection.
> 
> The first version of modular system will be equipped with 8 slots.
> 
> With no enforcement, for example, it could be the next bus assignments:
> - if system is booted with empty slot number one, and with line card at slot
>   number 2, i2c devices i2c-{n1} - i2c-{n2} will be created for line card at the slot 2.
> - if system is booted with line cards at slot 1 and at slot 2, devices i2c-{n1} - i2c-{n2}
>   this time will be associated with line card at slot 1, while i2c-{n2+1} - i2c-{n2*2} will
>   be associated with line card at slot 2.
> - line cards could are removed and then re-inserted in some random order, and it'll
>   also could change bus indexes for line card inserted at the same slot.   
> 
> It'll make a big challenge for any user application, which wants to use /dev/i2c-{x}.
> 
> With enforcement I can avoid this situation.
> So, for fixed system it would be fine to have base_nr equal zero, but for modular
> I'd like to have base_nr = f(slot), f.e. 100 * slot.

My confusion stems from your response to my note "I get the feeling that you are
desperatly trying to get some specific numbering in user space", when you said:

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

So, you had me confused. :-)

Wolfram, is there a better way to get something stable for user space to
interact with? Is there maybe a way to do this with aliases or something?
Setting up an ad-hoc scheme for forcing the adapter IDs feels a bit outdated.

>> Trying to force specific adapater IDs risks failure whenever any of those IDs
>> happen to be taken, and you have no way of preallocating some range the I2C
>> core should not use. The only way to do what you do is to select some high
>> enough ID range and hope for the best. But what if someone else does the
>> same thing? It's just a slippery slope. So, why?
> 
> Yes, it could happen with fixed system, but in this case I expect base_nr to be
> zero and IDs will be allocated just from free pool.
> But for modular system availability of specific IDs could be granted.

With your latest suggested code, setting base_nr to zero will not trigger
allocation from the free pool. Well, the first channel would be, but
that's ... not practical.

Cheers,
Peter

> Otherwise I'll have IDs reordering, each time when some devices are removed/
> inserted.

Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[wsa]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]


> Wolfram, is there a better way to get something stable for user space to
> interact with? Is there maybe a way to do this with aliases or something?
> Setting up an ad-hoc scheme for forcing the adapter IDs feels a bit outdated.

Yeah, it feels rightfully outdated IMO. Bringing such policy into the
kernel is frowned upon. I think the proper way is a udev rule to act on
the newly created I2C adapter. This even could provide a really stable
symlink for userspace to consume. The above scheme is only stable per
"block" but inside the block, there is still randomness. Or?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[Peter Rosin]

Hi!

On 2021-01-12 11:11, wsa@the-dreams.de wrote:
> 
>> Wolfram, is there a better way to get something stable for user space to
>> interact with? Is there maybe a way to do this with aliases or something?
>> Setting up an ad-hoc scheme for forcing the adapter IDs feels a bit outdated.
> 
> Yeah, it feels rightfully outdated IMO. Bringing such policy into the
> kernel is frowned upon. I think the proper way is a udev rule to act on
> the newly created I2C adapter. This even could provide a really stable
> symlink for userspace to consume. The above scheme is only stable per
> "block" but inside the block, there is still randomness. Or?

Right, that makes sense. Thanks! Vadim, is there any reason to not solve this
with udev? Doing that with care could perhaps provide stable names even if
you swap slots?

Cheers,
Peter

RE: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[Vadim Pasternak]

Hi Peter and Wolfram,

Thank you for your comments.

> -----Original Message-----
> From: Peter Rosin <peda@axentia.se>
> Sent: Thursday, January 14, 2021 9:49 AM
> To: wsa@the-dreams.de; Vadim Pasternak <vadimp@nvidia.com>; 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-12 11:11, wsa@the-dreams.de wrote:
> >
> >> Wolfram, is there a better way to get something stable for user space
> >> to interact with? Is there maybe a way to do this with aliases or something?
> >> Setting up an ad-hoc scheme for forcing the adapter IDs feels a bit
> outdated.
> >
> > Yeah, it feels rightfully outdated IMO. Bringing such policy into the
> > kernel is frowned upon. I think the proper way is a udev rule to act
> > on the newly created I2C adapter. This even could provide a really
> > stable symlink for userspace to consume. The above scheme is only
> > stable per "block" but inside the block, there is still randomness. Or?
> 
> Right, that makes sense. Thanks! Vadim, is there any reason to not solve this
> with udev? Doing that with care could perhaps provide stable names even if
> you swap slots?

Yes, I can manage it by udev and provide some names like "i2c-lc1-fpga1",
which maybe will be more clear for user, then name like "i2c-132".

I have another, not user space problem and maybe you can
suggest some solution.

In line card driver I planned to create I2C infrastructure for the
specific line card, like:

static int mlxreg_lc_chan[] = {
	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
};

static struct mlxcpld_mux_plat_data mlxreg_lc_mux_data[] = {
	{
		.chan_ids = mlxreg_lc_chan,
		.num_adaps = ARRAY_SIZE(mlxreg_lc_chan),
		.sel_reg_addr = MLXREG_LC_CHANNEL_I2C_REG,
		.reg_size = 2,
	},
};

	mlxreg_lc->mux = platform_device_register_resndata(dev, "i2c-mux-mlxcpld", parent_nr,
							   NULL, 0, &mlxreg_lc_mux_data,
							   sizeof(mlxreg_lc_mux_data));

And after this infrastructure is ready - to attach from this drive line
card devices from 'i2c_board_info', like:

static struct i2c_board_info mlxreg_lc_main_pwr_devices[] = {
	{
		I2C_BOARD_INFO("mp2975", 0x62),
	},
	{
		I2C_BOARD_INFO("mp2975", 0x64),
	},
	{
		I2C_BOARD_INFO("max11603", 0x6d),
	},
	{
		I2C_BOARD_INFO("lm25066", 0x15),
	},
};

static struct mlxreg_hotplug_device mlxreg_lc_main_pwr_brdinfo[] = {
	{
		.brdinfo = &mlxreg_lc_main_pwr_devices[0],
		.nr = 4,
	},
	{
		.brdinfo = &mlxreg_lc_main_pwr_devices[1],
		.nr = 4,
	},
	{
		.brdinfo = &mlxreg_lc_main_pwr_devices[2],
		.nr = 5,
	},
	{
		.brdinfo = &mlxreg_lc_main_pwr_devices[3],
		.nr = 6,
	},
};

Where the above 'nr's are from 'mlxreg_lc_chan'.

And then create with i2c_new_client_device() all the above devices from
workqueue, which will be running until all the 'mlxreg_lc_chan' related
adapters are created.
With forcing base nr, I know the number of last nr, which should be
created by "i2c-mux-mlxcpld".

Without it I'll need some ability to find with nrs have been created by
"i2c-mux-mlxcpld".
Do you have any suggestions for that?

I understand that I can also do it through udev, but I'd prefer to create
all on-board (line card) devices from the kernel, if possible.

Cheers,
Vadim.

> 
> Cheers,
> Peter

Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[Peter Rosin]

Hi!

On 2021-01-14 19:43, Vadim Pasternak wrote:
> Hi Peter and Wolfram,
> 
> Thank you for your comments.
> 
>> -----Original Message-----
>> From: Peter Rosin <peda@axentia.se>
>> Sent: Thursday, January 14, 2021 9:49 AM
>> To: wsa@the-dreams.de; Vadim Pasternak <vadimp@nvidia.com>; 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-12 11:11, wsa@the-dreams.de wrote:
>>>
>>>> Wolfram, is there a better way to get something stable for user space
>>>> to interact with? Is there maybe a way to do this with aliases or something?
>>>> Setting up an ad-hoc scheme for forcing the adapter IDs feels a bit
>> outdated.
>>>
>>> Yeah, it feels rightfully outdated IMO. Bringing such policy into the
>>> kernel is frowned upon. I think the proper way is a udev rule to act
>>> on the newly created I2C adapter. This even could provide a really
>>> stable symlink for userspace to consume. The above scheme is only
>>> stable per "block" but inside the block, there is still randomness. Or?
>>
>> Right, that makes sense. Thanks! Vadim, is there any reason to not solve this
>> with udev? Doing that with care could perhaps provide stable names even if
>> you swap slots?
> 
> Yes, I can manage it by udev and provide some names like "i2c-lc1-fpga1",
> which maybe will be more clear for user, then name like "i2c-132".
> 
> I have another, not user space problem and maybe you can
> suggest some solution.
> 
> In line card driver I planned to create I2C infrastructure for the
> specific line card, like:
> 
> static int mlxreg_lc_chan[] = {
> 	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
> };
> 
> static struct mlxcpld_mux_plat_data mlxreg_lc_mux_data[] = {
> 	{
> 		.chan_ids = mlxreg_lc_chan,
> 		.num_adaps = ARRAY_SIZE(mlxreg_lc_chan),
> 		.sel_reg_addr = MLXREG_LC_CHANNEL_I2C_REG,
> 		.reg_size = 2,
> 	},
> };
> 
> 	mlxreg_lc->mux = platform_device_register_resndata(dev, "i2c-mux-mlxcpld", parent_nr,
> 							   NULL, 0, &mlxreg_lc_mux_data,
> 							   sizeof(mlxreg_lc_mux_data));
> 
> And after this infrastructure is ready - to attach from this drive line
> card devices from 'i2c_board_info', like:
> 
> static struct i2c_board_info mlxreg_lc_main_pwr_devices[] = {
> 	{
> 		I2C_BOARD_INFO("mp2975", 0x62),
> 	},
> 	{
> 		I2C_BOARD_INFO("mp2975", 0x64),
> 	},
> 	{
> 		I2C_BOARD_INFO("max11603", 0x6d),
> 	},
> 	{
> 		I2C_BOARD_INFO("lm25066", 0x15),
> 	},
> };
> 
> static struct mlxreg_hotplug_device mlxreg_lc_main_pwr_brdinfo[] = {
> 	{
> 		.brdinfo = &mlxreg_lc_main_pwr_devices[0],
> 		.nr = 4,
> 	},
> 	{
> 		.brdinfo = &mlxreg_lc_main_pwr_devices[1],
> 		.nr = 4,
> 	},
> 	{
> 		.brdinfo = &mlxreg_lc_main_pwr_devices[2],
> 		.nr = 5,
> 	},
> 	{
> 		.brdinfo = &mlxreg_lc_main_pwr_devices[3],
> 		.nr = 6,
> 	},
> };
> 
> Where the above 'nr's are from 'mlxreg_lc_chan'.
> 
> And then create with i2c_new_client_device() all the above devices from
> workqueue, which will be running until all the 'mlxreg_lc_chan' related
> adapters are created.
> With forcing base nr, I know the number of last nr, which should be
> created by "i2c-mux-mlxcpld".
> 
> Without it I'll need some ability to find with nrs have been created by
> "i2c-mux-mlxcpld".
> Do you have any suggestions for that?
> 
> I understand that I can also do it through udev, but I'd prefer to create
> all on-board (line card) devices from the kernel, if possible.

You could add a callback function to struct mlxcpld_mux_plat_data, and have the
driver call you back with the mapping so that you know what adapter ID you got
for each platform data (or channel if needed) you instantiate.

Would that do it?

Cheers,
Peter

RE: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to support word address space devices

[Vadim Pasternak]

> -----Original Message-----
> From: Peter Rosin <peda@axentia.se>
> Sent: Thursday, January 14, 2021 11:10 PM
> To: Vadim Pasternak <vadimp@nvidia.com>; wsa@the-dreams.de; 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-14 19:43, Vadim Pasternak wrote:
> > Hi Peter and Wolfram,
> >
> > Thank you for your comments.
> >
> >> -----Original Message-----
> >> From: Peter Rosin <peda@axentia.se>
> >> Sent: Thursday, January 14, 2021 9:49 AM
> >> To: wsa@the-dreams.de; Vadim Pasternak <vadimp@nvidia.com>; 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-12 11:11, wsa@the-dreams.de wrote:
> >>>
> >>>> Wolfram, is there a better way to get something stable for user
> >>>> space to interact with? Is there maybe a way to do this with aliases or
> something?
> >>>> Setting up an ad-hoc scheme for forcing the adapter IDs feels a bit
> >> outdated.
> >>>
> >>> Yeah, it feels rightfully outdated IMO. Bringing such policy into
> >>> the kernel is frowned upon. I think the proper way is a udev rule to
> >>> act on the newly created I2C adapter. This even could provide a
> >>> really stable symlink for userspace to consume. The above scheme is
> >>> only stable per "block" but inside the block, there is still randomness. Or?
> >>
> >> Right, that makes sense. Thanks! Vadim, is there any reason to not
> >> solve this with udev? Doing that with care could perhaps provide
> >> stable names even if you swap slots?
> >
> > Yes, I can manage it by udev and provide some names like
> > "i2c-lc1-fpga1", which maybe will be more clear for user, then name like "i2c-
> 132".
> >
> > I have another, not user space problem and maybe you can suggest some
> > solution.
> >
> > In line card driver I planned to create I2C infrastructure for the
> > specific line card, like:
> >
> > static int mlxreg_lc_chan[] = {
> > 	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
> > };
> >
> > static struct mlxcpld_mux_plat_data mlxreg_lc_mux_data[] = {
> > 	{
> > 		.chan_ids = mlxreg_lc_chan,
> > 		.num_adaps = ARRAY_SIZE(mlxreg_lc_chan),
> > 		.sel_reg_addr = MLXREG_LC_CHANNEL_I2C_REG,
> > 		.reg_size = 2,
> > 	},
> > };
> >
> > 	mlxreg_lc->mux = platform_device_register_resndata(dev, "i2c-mux-
> mlxcpld", parent_nr,
> > 							   NULL, 0,
> &mlxreg_lc_mux_data,
> >
> sizeof(mlxreg_lc_mux_data));
> >
> > And after this infrastructure is ready - to attach from this drive
> > line card devices from 'i2c_board_info', like:
> >
> > static struct i2c_board_info mlxreg_lc_main_pwr_devices[] = {
> > 	{
> > 		I2C_BOARD_INFO("mp2975", 0x62),
> > 	},
> > 	{
> > 		I2C_BOARD_INFO("mp2975", 0x64),
> > 	},
> > 	{
> > 		I2C_BOARD_INFO("max11603", 0x6d),
> > 	},
> > 	{
> > 		I2C_BOARD_INFO("lm25066", 0x15),
> > 	},
> > };
> >
> > static struct mlxreg_hotplug_device mlxreg_lc_main_pwr_brdinfo[] = {
> > 	{
> > 		.brdinfo = &mlxreg_lc_main_pwr_devices[0],
> > 		.nr = 4,
> > 	},
> > 	{
> > 		.brdinfo = &mlxreg_lc_main_pwr_devices[1],
> > 		.nr = 4,
> > 	},
> > 	{
> > 		.brdinfo = &mlxreg_lc_main_pwr_devices[2],
> > 		.nr = 5,
> > 	},
> > 	{
> > 		.brdinfo = &mlxreg_lc_main_pwr_devices[3],
> > 		.nr = 6,
> > 	},
> > };
> >
> > Where the above 'nr's are from 'mlxreg_lc_chan'.
> >
> > And then create with i2c_new_client_device() all the above devices
> > from workqueue, which will be running until all the 'mlxreg_lc_chan'
> > related adapters are created.
> > With forcing base nr, I know the number of last nr, which should be
> > created by "i2c-mux-mlxcpld".
> >
> > Without it I'll need some ability to find with nrs have been created
> > by "i2c-mux-mlxcpld".
> > Do you have any suggestions for that?
> >
> > I understand that I can also do it through udev, but I'd prefer to
> > create all on-board (line card) devices from the kernel, if possible.
> 
> You could add a callback function to struct mlxcpld_mux_plat_data, and have
> the driver call you back with the mapping so that you know what adapter ID
> you got for each platform data (or channel if needed) you instantiate.
> 
> Would that do it?

Great!!!
I will do it in this way.

Thank you very much for help.

I'll add it to v2 patchset.

Cheers,
Vadim.

> 
> Cheers,
> Peter

[Re-send: PATCH i2c-next 2/6] platform/x86: mlxcpld: Update module license

[Vadim Pasternak]

Update license to SPDX-License.

Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
---
 include/linux/platform_data/x86/mlxcpld.h | 34 +++----------------------------
 1 file changed, 3 insertions(+), 31 deletions(-)

diff --git a/include/linux/platform_data/x86/mlxcpld.h b/include/linux/platform_data/x86/mlxcpld.h
index b08dcb183fca..e6c18bf017dd 100644
--- a/include/linux/platform_data/x86/mlxcpld.h
+++ b/include/linux/platform_data/x86/mlxcpld.h
@@ -1,36 +1,8 @@
+/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */
 /*
- * mlxcpld.h - Mellanox I2C multiplexer support in CPLD
+ * Mellanox I2C multiplexer support in CPLD
  *
- * Copyright (c) 2016 Mellanox Technologies. All rights reserved.
- * Copyright (c) 2016 Michael Shych <michaels@mellanox.com>
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are met:
- *
- * 1. Redistributions of source code must retain the above copyright
- *    notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- *    notice, this list of conditions and the following disclaimer in the
- *    documentation and/or other materials provided with the distribution.
- * 3. Neither the names of the copyright holders nor the names of its
- *    contributors may be used to endorse or promote products derived from
- *    this software without specific prior written permission.
- *
- * Alternatively, this software may be distributed under the terms of the
- * GNU General Public License ("GPL") version 2 as published by the Free
- * Software Foundation.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
- * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
- * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
- * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
- * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
- * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
- * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
- * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
- * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
- * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
- * POSSIBILITY OF SUCH DAMAGE.
+ * Copyright (C) 2016-2020 Mellanox Technologies
  */
 
 #ifndef _LINUX_I2C_MLXCPLD_H
-- 
2.11.0