i2c: mux: mlxcpld: Extend driver functionality and update licenses

---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c | 33 +++------------------------------
 1 file changed, 3 insertions(+), 30 deletions(-)

drivers/i2c/muxes/i2c-mux-mlxcpld.c +3 ~~-30~~

index 5ed55ca4fe93..53bce81cf5c9 100644
@@ -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 <[email protected]>
+ * 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>

Signature · 1 line

2.11.0

Peter Rosin

Acked-by on v2 1/7

2021-01-28 10:10 UTC #2 lore raw

Re: [PATCH i2c-next v2 1/7] i2c: mux: mlxcpld: Update module license

Quoted 3 lines · Vadim Pasternak

Update license to SPDX-License.
Signed-off-by Vadim Pasternak <[email protected]>

Acked-by Peter Rosin <[email protected]>

Cheers,
Peter

Wolfram Sang

Applied

2021-02-01 21:57 UTC #3 lore raw

Re: [PATCH i2c-next v2 1/7] i2c: mux: mlxcpld: Update module license

Quoted 3 lines · Vadim Pasternak

Update license to SPDX-License.
Signed-off-by Vadim Pasternak <[email protected]>

Applied to for-next, thanks!

Vadim Pasternak

author maintainer

2021-01-22 22:38 UTC #4 lore raw

PATCH v2 2/7 platform/x86: mlxcpld: Update module license

Update license to SPDX-License.

---
 include/linux/platform_data/x86/mlxcpld.h | 34 +++----------------------------
 1 file changed, 3 insertions(+), 31 deletions(-)

include/linux/platform_data/x86/mlxcpld.h +3 ~~-31~~

index b08dcb183fca..e6c18bf017dd 100644
@@ -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 <[email protected]>
- *
- * 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

Signature · 1 line

2.11.0

Peter Rosin

Acked-by on v2 2/7

2021-01-28 10:11 UTC #5 lore raw

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

Quoted 3 lines · Vadim Pasternak

Update license to SPDX-License.
Signed-off-by Vadim Pasternak <[email protected]>

Acked-by Peter Rosin <[email protected]>

Cheers,
Peter

Wolfram Sang

Applied

2021-02-01 21:57 UTC #6 lore raw

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

Quoted 3 lines · Vadim Pasternak

Update license to SPDX-License.
Signed-off-by Vadim Pasternak <[email protected]>

Applied to for-next, thanks!

Vadim Pasternak

author maintainer

2021-01-22 22:38 UTC #7 lore raw

PATCH v2 3/7 i2c: mux: mlxcpld: Move header file out of x86 realm

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

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

drivers/i2c/muxes/i2c-mux-mlxcpld.c +1 -1

index 53bce81cf5c9..3d894cfb19df 100644
@@ -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>

include/linux/platform_data/mlxcpld.h renamed from include/linux/platform_data/x86/mlxcpld.h +0 -0

similarity index 100%
rename from include/linux/platform_data/x86/mlxcpld.h
rename to include/linux/platform_data/mlxcpld.h

Signature · 1 line

2.11.0

Peter Rosin

Acked-by on v2 3/7

2021-01-28 10:12 UTC #8 lore raw

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

Quoted 6 lines · 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 <[email protected]>
Reviewed-by Michael Shych <[email protected]>

Acked-by Peter Rosin <[email protected]>

Cheers,
Peter

Wolfram Sang

Applied

2021-02-01 21:57 UTC #9 lore raw

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

Quoted 6 lines · 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 <[email protected]>
Reviewed-by Michael Shych <[email protected]>

Applied to for-next, thanks!

Vadim Pasternak

author maintainer

2021-01-22 19:51 UTC #10 lore raw

PATCH v2 4/7 i2c: mux: mlxcpld: Convert driver to platform driver

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.

---
v1->v2:
Comments pointed out by Peter:
- Drop change for last channel setting in mlxcpld_mux_deselect().

---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c | 62 +++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 34 deletions(-)

drivers/i2c/muxes/i2c-mux-mlxcpld.c +28 ~~-34~~

index 3d894cfb19df..b53f1479272d 100644
@@ -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 = 0;
 
-	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 ([email protected])");
 MODULE_DESCRIPTION("Mellanox I2C-CPLD-MUX driver");

Signature · 1 line

2.11.0

Vadim Pasternak

author maintainer

2021-01-22 19:51 UTC #11 lore raw

PATCH v2 5/7 i2c: mux: mlxcpld: Extend driver to support word address space devices

Extend driver to allow I2C routing control through CPLD devices with word address space. Till now only CPLD devices with byte address space have been supported.

---
v1->v2:
Comments pointed out by Peter:
- Remove data buffer allocation from 'mlxcpld_mux' structure, do it on stack instead.
- Do not use array pdata.adap_ids[] in mlxcpld_mux_reg_write() for channel assignment.
- Return back 'regval' variable, used for channel assignment in mlxcpld_mux_select_chan().
- Fix functionality validation in mlxcpld_mux_probe().
- Fix comment for 'reg_size' field in mlxcpld_mux_plat_data' structure. Added by Vadim:
- Change type of register select address to '__be16' to align with type in assignment in cpu_to_be16().

---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c   | 54 ++++++++++++++++++++++++++++-------
 include/linux/platform_data/mlxcpld.h |  2 ++
 2 files changed, 46 insertions(+), 10 deletions(-)

drivers/i2c/muxes/i2c-mux-mlxcpld.c +44 ~~-10~~

index b53f1479272d..4848dd4ff41a 100644
@@ -21,11 +21,13 @@
  * @last_chan - last register value
  * @client - I2C device client
  * @pdata: platform data
+ * @sel_reg_addr: mux select/deselect register address
  */
 struct mlxcpld_mux {
 	u8 last_chan;
 	struct i2c_client *client;
 	struct mlxcpld_mux_plat_data pdata;
+	__be16 sel_reg_addr;
 };
 
 /* MUX logic description.

@@ -60,24 +62,43 @@ struct mlxcpld_mux {
  * for this as they will try to lock adapter a second time.
  */
 static int mlxcpld_mux_reg_write(struct i2c_adapter *adap,
-				 struct mlxcpld_mux *mux, u8 val)
+				 struct mlxcpld_mux *mux, int chan)
 {
 	struct i2c_client *client = mux->client;
-	union i2c_smbus_data data = { .byte = val };
-
-	return __i2c_smbus_xfer(adap, client->addr, client->flags,
-				I2C_SMBUS_WRITE, mux->pdata.sel_reg_addr,
-				I2C_SMBUS_BYTE_DATA, &data);
+	union i2c_smbus_data data;
+	struct i2c_msg msg;
+	u8 buf[3];
+
+	switch (mux->pdata.reg_size) {
+	case 1:
+		data.byte = (chan < 0) ? 0 : chan;
+		return __i2c_smbus_xfer(adap, client->addr, client->flags,
+					I2C_SMBUS_WRITE, mux->pdata.sel_reg_addr,
+					I2C_SMBUS_BYTE_DATA, &data);
+	case 2:
+		memcpy(buf, &mux->sel_reg_addr, 2);
+		buf[2] = chan;
+		msg.addr = client->addr;
+		msg.buf = buf;
+		msg.len = mux->pdata.reg_size + 1;
+		msg.flags = 0;
+		return __i2c_transfer(adap, &msg, 1);
+	default:
+		return -EINVAL;
+	}
 }
 
 static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32 chan)
 {
 	struct mlxcpld_mux *mux = i2c_mux_priv(muxc);
-	u8 regval = chan + 1;
+	u8 regval = chan;
 	int err = 0;
 
+	if (mux->pdata.reg_size == 1)
+		regval += 1;
+
 	/* Only select the channel if its different from the last channel */
-	if (mux->last_chan != regval) {
+	if (mux->last_chan != chan) {
 		err = mlxcpld_mux_reg_write(muxc->parent, mux, regval);
 		mux->last_chan = err < 0 ? 0 : regval;
 	}

@@ -103,13 +124,24 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
 	struct i2c_mux_core *muxc;
 	int num, force;
 	struct mlxcpld_mux *data;
+	u32 func;
 	int err;
 
 	if (!pdata)
 		return -EINVAL;
 
-	if (!i2c_check_functionality(client->adapter,
-				     I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
+	switch (pdata->reg_size) {
+	case 1:
+		func = I2C_FUNC_SMBUS_WRITE_BYTE_DATA;
+		break;
+	case 2:
+		func = I2C_FUNC_I2C;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (!i2c_check_functionality(client->adapter, func))
 		return -ENODEV;
 
 	muxc = i2c_mux_alloc(client->adapter, &pdev->dev, CPLD_MUX_MAX_NCHANS,

@@ -122,6 +154,8 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
 	data = i2c_mux_priv(muxc);
 	data->client = client;
 	memcpy(&data->pdata, pdata, sizeof(*pdata));
+	/* Save mux select address for 16 bits transaction size. */
+	data->sel_reg_addr = cpu_to_be16(pdata->sel_reg_addr);
 	data->last_chan = 0; /* force the first selection */
 
 	/* Create an adapter for each channel. */

include/linux/platform_data/mlxcpld.h +2 -0

index e6c18bf017dd..9cb2c3d8293e 100644
@@ -14,11 +14,13 @@
  * @adap_ids - adapter array
  * @num_adaps - number of adapters
  * @sel_reg_addr - mux select register offset in CPLD space
+ * @reg_size: register size in bytes
  */
 struct mlxcpld_mux_plat_data {
 	int *adap_ids;
 	int num_adaps;
 	int sel_reg_addr;
+	u8 reg_size;
 };
 
 #endif /* _LINUX_I2C_MLXCPLD_H */

Signature · 1 line

2.11.0

Peter Rosin

2021-01-25 21:49 UTC #12 lore raw

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

Hi!

Quoted 44 lines · 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 <[email protected]>
Reviewed-by Michael Shych <[email protected]>
---
v1->v2:
Comments pointed out by Peter:
- Remove data buffer allocation from 'mlxcpld_mux' structure, do it on stack instead.
- Do not use array pdata.adap_ids[] in mlxcpld_mux_reg_write() for channel assignment.
- Return back 'regval' variable, used for channel assignment in mlxcpld_mux_select_chan().
- Fix functionality validation in mlxcpld_mux_probe().
- Fix comment for 'reg_size' field in mlxcpld_mux_plat_data' structure. Added by Vadim:
- Change type of register select address to '__be16' to align with type in assignment in cpu_to_be16().
---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c   | 54 ++++++++++++++++++++++++++++-------
 include/linux/platform_data/mlxcpld.h |  2 ++
 2 files changed, 46 insertions(+), 10 deletions(-)
drivers/i2c/muxes/i2c-mux-mlxcpld.c +2 -0
index b53f1479272d..4848dd4ff41a 100644
@@ -21,11 +21,13 @@
  * @last_chan - last register value
  * @client - I2C device client
  * @pdata: platform data
+ * @sel_reg_addr: mux select/deselect register address
  */
 struct mlxcpld_mux {
 	u8 last_chan;
 	struct i2c_client *client;
 	struct mlxcpld_mux_plat_data pdata;
+	__be16 sel_reg_addr;
 };
 
 /* MUX logic description.

drivers/i2c/muxes/i2c-mux-mlxcpld.c struct mlxcpld_mux { · line 65

Quoted 5 lines

drivers/i2c/muxes/i2c-mux-mlxcpld.c +1 -1

@@ -60,24 +62,43 @@ struct mlxcpld_mux {
  * for this as they will try to lock adapter a second time.
  */
 static int mlxcpld_mux_reg_write(struct i2c_adapter *adap,
-				 struct mlxcpld_mux *mux, u8 val)
+				 struct mlxcpld_mux *mux, int chan)

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

Quoted 9 lines

drivers/i2c/muxes/i2c-mux-mlxcpld.c +2 -5

 {
 	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;

drivers/i2c/muxes/i2c-mux-mlxcpld.c struct mlxcpld_mux { · line 79

Quoted 10 lines

drivers/i2c/muxes/i2c-mux-mlxcpld.c +10 -0

+	u8 buf[3];
+
+	switch (mux->pdata.reg_size) {
+	case 1:
+		data.byte = (chan < 0) ? 0 : chan;
+		return __i2c_smbus_xfer(adap, client->addr, client->flags,
+					I2C_SMBUS_WRITE, mux->pdata.sel_reg_addr,
+					I2C_SMBUS_BYTE_DATA, &data);
+	case 2:
+		memcpy(buf, &mux->sel_reg_addr, 2);

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

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

Quoted 14 lines

drivers/i2c/muxes/i2c-mux-mlxcpld.c +9 -0

+		buf[2] = chan;
+		msg.addr = client->addr;
+		msg.buf = buf;
+		msg.len = mux->pdata.reg_size + 1;
+		msg.flags = 0;
+		return __i2c_transfer(adap, &msg, 1);
+	default:
+		return -EINVAL;
+	}
 }
 
 static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32 chan)
 {
 	struct mlxcpld_mux *mux = i2c_mux_priv(muxc);

drivers/i2c/muxes/i2c-mux-mlxcpld.c struct mlxcpld_mux { · line 101

Quoted 10 lines

drivers/i2c/muxes/i2c-mux-mlxcpld.c +5 -2

-	u8 regval = chan + 1;
+	u8 regval = chan;
 	int err = 0;
 
+	if (mux->pdata.reg_size == 1)
+		regval += 1;
+
 	/* Only select the channel if its different from the last channel */
-	if (mux->last_chan != regval) {
+	if (mux->last_chan != chan) {

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

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

drivers/i2c/muxes/i2c-mux-mlxcpld.c struct mlxcpld_mux { · line 103

Quoted 2 lines

drivers/i2c/muxes/i2c-mux-mlxcpld.c +0 -0

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

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

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

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

Cheers,
Peter

Quoted 56 lines

drivers/i2c/muxes/i2c-mux-mlxcpld.c +15 -2

@@ -103,13 +124,24 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
 	struct i2c_mux_core *muxc;
 	int num, force;
 	struct mlxcpld_mux *data;
+	u32 func;
 	int err;
 
 	if (!pdata)
 		return -EINVAL;
 
-	if (!i2c_check_functionality(client->adapter,
-				     I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
+	switch (pdata->reg_size) {
+	case 1:
+		func = I2C_FUNC_SMBUS_WRITE_BYTE_DATA;
+		break;
+	case 2:
+		func = I2C_FUNC_I2C;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (!i2c_check_functionality(client->adapter, func))
 		return -ENODEV;
 
 	muxc = i2c_mux_alloc(client->adapter, &pdev->dev, CPLD_MUX_MAX_NCHANS,

@@ -122,6 +154,8 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
 	data = i2c_mux_priv(muxc);
 	data->client = client;
 	memcpy(&data->pdata, pdata, sizeof(*pdata));
+	/* Save mux select address for 16 bits transaction size. */
+	data->sel_reg_addr = cpu_to_be16(pdata->sel_reg_addr);
 	data->last_chan = 0; /* force the first selection */
 
 	/* Create an adapter for each channel. */

include/linux/platform_data/mlxcpld.h +2 -0

index e6c18bf017dd..9cb2c3d8293e 100644
@@ -14,11 +14,13 @@
  * @adap_ids - adapter array
  * @num_adaps - number of adapters
  * @sel_reg_addr - mux select register offset in CPLD space
+ * @reg_size: register size in bytes
  */
 struct mlxcpld_mux_plat_data {
 	int *adap_ids;
 	int num_adaps;
 	int sel_reg_addr;
+	u8 reg_size;
 };
 
 #endif /* _LINUX_I2C_MLXCPLD_H */

A message that is not in the archive (sent off-list or not yet received): DM6PR12MB38987A5C2B8A8429289BA6CDAFBA9@DM6PR12MB3898.namprd12.prod.outlook.com

Peter Rosin

2021-01-29 23:01 UTC #14 lore raw

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

Hi Vadim,

Quoted 2 lines · Vadim Pasternak

Hi Peter,

*snip*

Quoted 2 lines

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

Please no, that's backwards.

Quoted 3 lines

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

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

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

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

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

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

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

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

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

Cheers,
Peter

Vadim Pasternak

author maintainer

2021-01-22 19:51 UTC #15 lore raw

PATCH v2 6/7 i2c: mux: mlxcpld: Extend supported mux number

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.

---
v1->v2:
Comments pointed out by Peter:
- Remove introducing of 'base_nr' field.
- Drop chan increment in mlxcpld_mux_select_chan(). Added by Vadim:
- Rename 'adaps_ids' array to 'chan_ids' array.
- Drop forcing of adapter 'nr'.

---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c   | 19 +++++--------------
 include/linux/platform_data/mlxcpld.h |  4 ++--
 2 files changed, 7 insertions(+), 16 deletions(-)

drivers/i2c/muxes/i2c-mux-mlxcpld.c +5 ~~-14~~

index 4848dd4ff41a..7b37d59dc23b 100644
@@ -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_reg_addr: mux select/deselect register address
  */
 struct mlxcpld_mux {
-	u8 last_chan;
+	int last_chan;
 	struct i2c_client *client;
 	struct mlxcpld_mux_plat_data pdata;
 	__be16 sel_reg_addr;

@@ -122,10 +120,9 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
 	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;
+	int num, err;
 	u32 func;
-	int err;
 
 	if (!pdata)
 		return -EINVAL;

@@ -144,7 +141,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)

@@ -159,14 +156,8 @@ 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];
-
-		err = i2c_mux_add_adapter(muxc, force, num, 0);
+	for (num = 0; num < pdata->num_adaps; num++) {
+		err = i2c_mux_add_adapter(muxc, 0, pdata->chan_ids[num], 0);
 		if (err)
 			goto virt_reg_failed;
 	}

include/linux/platform_data/mlxcpld.h +2 -2

index 9cb2c3d8293e..a7bee798d991 100644
@@ -11,13 +11,13 @@
 /* Platform data for the CPLD I2C multiplexers */
 
 /* mlxcpld_mux_plat_data - per mux data, used with i2c_register_board_info
- * @adap_ids - adapter array
+ * @chan_ids - channels array
  * @num_adaps - number of adapters
  * @sel_reg_addr - mux select register offset in CPLD space
  * @reg_size: register size in bytes
  */
 struct mlxcpld_mux_plat_data {
-	int *adap_ids;
+	int *chan_ids;
 	int num_adaps;
 	int sel_reg_addr;
 	u8 reg_size;

Signature · 1 line

2.11.0

Vadim Pasternak

author maintainer

2021-01-22 19:51 UTC #16 lore raw

PATCH v2 7/7 i2c: mux: mlxcpld: Add callback to notify mux creation completion

Add notification to inform caller that mux objects array has been created. It allows to user, invoked platform device registration for "i2c-mux-mlxcpld" driver, to be notified that mux infrastructure is available, and thus some devices could be connected to this infrastructure.

---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c   | 4 ++++
 include/linux/platform_data/mlxcpld.h | 5 +++++
 2 files changed, 9 insertions(+)

drivers/i2c/muxes/i2c-mux-mlxcpld.c +4 -0

index 7b37d59dc23b..5e0b910790ce 100644
@@ -162,6 +162,10 @@ static int mlxcpld_mux_probe(struct platform_device *pdev)
 			goto virt_reg_failed;
 	}
 
+	/* Notify caller when all channels' adapters are created. */
+	if (pdata->completion_notify)
+		pdata->completion_notify(pdata->handle, muxc->parent, muxc->adapter);
+
 	return 0;
 
 virt_reg_failed:

include/linux/platform_data/mlxcpld.h +5 -0

index a7bee798d991..d7610b528856 100644
@@ -15,12 +15,17 @@
  * @num_adaps - number of adapters
  * @sel_reg_addr - mux select register offset in CPLD space
  * @reg_size: register size in bytes
+ * @handle: handle to be passed by callback
+ * @completion_notify: callback to notify when all the adapters are created
  */
 struct mlxcpld_mux_plat_data {
 	int *chan_ids;
 	int num_adaps;
 	int sel_reg_addr;
 	u8 reg_size;
+	void *handle;
+	int (*completion_notify)(void *handle, struct i2c_adapter *parent,
+				 struct i2c_adapter *adapters[]);
 };
 
 #endif /* _LINUX_I2C_MLXCPLD_H */

Signature · 1 line

2.11.0

Wolfram Sang

2021-01-28 08:54 UTC #17 lore raw