* Re: [patch 2/2] i2c: mux: mellanox: add driver
2016-08-24 13:56 ` vadimp
@ 2016-08-25 6:50 ` kbuild test robot
2016-08-25 6:50 ` kbuild test robot
1 sibling, 0 replies; 10+ messages in thread
From: kbuild test robot @ 2016-08-25 6:50 UTC (permalink / raw)
To: vadimp
Cc: kbuild-all, wsa, linux-i2c, linux-kernel, jiri, Vadim Pasternak,
Michael Shych
Hi Vadim,
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v4.8-rc3 next-20160824]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]
url: https://github.com/0day-ci/linux/commits/vadimp-mellanox-com/i2c-add-master-driver-for-mellanox-systems/20160824-200057
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
coccinelle warnings: (new ones prefixed by >>)
>> drivers/i2c/muxes/i2c-mux-mlxcpld.c:329:3-8: No need to set .owner here. The core will do it.
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [patch 2/2] i2c: mux: mellanox: add driver @ 2016-08-25 6:50 ` kbuild test robot 0 siblings, 0 replies; 10+ messages in thread From: kbuild test robot @ 2016-08-25 6:50 UTC (permalink / raw) Cc: kbuild-all, wsa, linux-i2c, linux-kernel, jiri, Vadim Pasternak, Michael Shych Hi Vadim, [auto build test WARNING on wsa/i2c/for-next] [also build test WARNING on v4.8-rc3 next-20160824] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on] [Check https://git-scm.com/docs/git-format-patch for more information] url: https://github.com/0day-ci/linux/commits/vadimp-mellanox-com/i2c-add-master-driver-for-mellanox-systems/20160824-200057 base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next coccinelle warnings: (new ones prefixed by >>) >> drivers/i2c/muxes/i2c-mux-mlxcpld.c:329:3-8: No need to set .owner here. The core will do it. Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation ^ permalink raw reply [flat|nested] 10+ messages in thread
* [patch 2/2] i2c: mux: mellanox: add driver @ 2016-08-24 13:56 ` vadimp 2016-08-24 13:54 ` Peter Rosin 2016-08-25 6:50 ` kbuild test robot 0 siblings, 2 replies; 10+ messages in thread From: vadimp @ 2016-08-24 13:56 UTC (permalink / raw) To: wsa; +Cc: linux-i2c, linux-kernel, jiri, Vadim Pasternak, Michael Shych From: Vadim Pasternak <vadimp@mellanox.com> This driver allows I2C routing controlled through CPLD select registers on wide range of Mellanox systems (CPLD Lattice device). MUX selection is provided by digital and analog HW. Analog part is not under SW control. Digital part is under CPLD control (channel selection/de-selection). MUX logic description. Mux selector can control 256 mux (channels), if utilized one CPLD register (8 bits) as select register - register value specifies mux id. Mux selector can control n*256 mux, if utilized n CPLD registers as select registers. The number of registers within the same CPLD can be combined to support mux hierarchy. This logic can be applied for LPC attached CPLD and fro I2C attached CPLD. Driver can support different mux control logic, according to CPLD implementation. Connectivity schema. i2c-mlxcpld Digital Analog driver *--------* * -> mux1 (virt bus2) -> mux -> | | I2CLPC | i2c physical * -> mux2 (virt bus3) -> mux -> | | bridge | bus 1 *---------* | | logic |---------------------> * mux reg * | | in CPLD| *---------* | *--------* i2c-mux-mlxpcld ^ * -> muxn (virt busn) -> mux -> | | driver | | | *---------------* | Devices | * CPLD (LPC bus)* select | | * registers for *--------* | * mux selection * deselect | *---------------* | | <--------> <-----------> i2c cntrl Board cntrl reg reg space space (mux select, | IO, LED, WD, info) | | *-----* *-----* *------------- LPC bus --------------| PCH |---| CPU | *-----* *-----* i2c-mux-mlxpcld does not necessary required i2c-mlxcpld. It can be use along with another bus driver, and still control i2c routing through CPLD mux selection, in case the system is equipped with CPLD capable of mux selection control. The Kconfig currently controlling compilation of this code is: drivers/i2c/muxes/Kconfig:config I2C_MUX_MLXCPLD Signed-off-by: Michael Shych <michaelsh@mellanox.com> Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> Reviewed-by: Jiri Pirko <jiri@mellanox.com> --- drivers/i2c/muxes/Kconfig | 11 ++ drivers/i2c/muxes/Makefile | 1 + drivers/i2c/muxes/i2c-mux-mlxcpld.c | 352 ++++++++++++++++++++++++++++++++++++ include/linux/i2c/mlxcpld.h | 67 +++++++ 4 files changed, 431 insertions(+) create mode 100644 drivers/i2c/muxes/i2c-mux-mlxcpld.c create mode 100644 include/linux/i2c/mlxcpld.h diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig index e280c8e..b7ab144 100644 --- a/drivers/i2c/muxes/Kconfig +++ b/drivers/i2c/muxes/Kconfig @@ -81,4 +81,15 @@ config I2C_DEMUX_PINCTRL demultiplexer that uses the pinctrl subsystem. This is useful if you want to change the I2C master at run-time depending on features. +config I2C_MUX_MLXCPLD + tristate "Mellanox CPLD based I2C multiplexer" + help + If you say yes to this option, support will be included for a + CPLD based I2C multiplexer. This driver provides access to + I2C busses connected through a MUX, which is controlled + by a CPLD registers. + + This driver can also be built as a module. If so, the module + will be called i2c-mux-mlxcpld. + endmenu diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile index 7c267c2..e5c990e 100644 --- a/drivers/i2c/muxes/Makefile +++ b/drivers/i2c/muxes/Makefile @@ -10,5 +10,6 @@ obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o obj-$(CONFIG_I2C_MUX_PINCTRL) += i2c-mux-pinctrl.o obj-$(CONFIG_I2C_MUX_REG) += i2c-mux-reg.o +obj-$(CONFIG_I2C_MUX_MLXCPLD) += i2c-mux-mlxcpld.o ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c new file mode 100644 index 0000000..ae860de --- /dev/null +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c @@ -0,0 +1,352 @@ +/* + * drivers/i2c/muxes/i2c-mux-mlxcpld.c + * 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. + */ + +#include <linux/module.h> +#include <linux/version.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/i2c-mux.h> +#include <linux/io.h> +#include <linux/platform_device.h> +#include <linux/i2c/mlxcpld.h> + +#define CPLD_MUX_MAX_NCHANS 8 +#define CPLD_MUX_EXT_MAX_NCHANS 24 + +/* + * mlxcpld_mux types - kind of mux supported by driver: + * @mlxcpld_mux_tor - LPC access; 8 channels/legs; select/deselect - + * channel=first defined channel(2/10) + channel/leg + * @mlxcpld_mux_mgmt - LPC access; 8 channels/legs; select/deselect - + * channel=1 + channel/leg + * @mlxcpld_mux_mgmt_ext - LPC access; 24 channels/legs; select/deselect - + * channel=1 + channel/leg + * @mlxcpld_mux_module - I2C access; 8 channels/legs; select/deselect - + * channel=1 + channel/leg + */ +enum mlxcpld_mux_type { + mlxcpld_mux_tor, + mlxcpld_mux_mgmt, + mlxcpld_mux_mgmt_ext, + mlxcpld_mux_module, +}; + +/* mlxcpld_mux_type - underlying physical bus, to which device is connected: + * @lpc_access - LPC connected CPLD device + * @i2c_access - I2C connected CPLD device + */ +enum mlxcpld_mux_access_type { + lpc_access, + i2c_access, +}; + +/* mlxcpld_mux - mux control structure: + * @type - mux type + * @last_chan - last register value + * @client - I2C device client + */ +struct mlxcpld_mux { + enum mlxcpld_mux_type type; + u8 last_chan; + struct i2c_client *client; +}; + +/* mlxcpld_mux_desc - mux descriptor structure: + * @nchans - number of channels + * @muxtype - physical mux type (LPC or I2C) + */ +struct mlxcpld_mux_desc { + u8 nchans; + enum mlxcpld_mux_access_type muxtype; +}; + +/* MUX logic description. + * Mux selector can control 256 mux (channels), if utilized one CPLD register + * (8 bits) as select register - register value specifies mux id. + * Mux selector can control n*256 mux, if utilized n CPLD registers as select + * registers. + * The number of registers within the same CPLD can be combined to support + * mux hierarchy. + * This logic can be applied for LPC attached CPLD and fro I2C attached CPLD. + * Driver can support different mux control logic, according to CPLD + * implementation. + * + * Connectivity schema. + * + * i2c-mlxcpld Digital Analog + * driver + * *--------* * -> mux1 (virt bus2) -> mux -> | + * | I2CLPC | i2c physical * -> mux2 (virt bus3) -> mux -> | + * | bridge | bus 1 *---------* | + * | logic |---------------------> * mux reg * | + * | in CPLD| *---------* | + * *--------* i2c-mux-mlxpcld ^ * -> muxn (virt busn) -> mux -> | + * | driver | | + * | *---------------* | Devices + * | * CPLD (LPC bus)* select | + * | * registers for *--------* + * | * mux selection * deselect + * | *---------------* + * | | + * <--------> <-----------> + * i2c cntrl Board cntrl reg + * reg space space (mux select, + * | IO, LED, WD, info) + * | | *-----* *-----* + * *------------- LPC bus --------------| PCH |---| CPU | + * *-----* *-----* + * + * i2c-mux-mlxpcld does not necessary required i2c-mlxcpld. It can be use along + * with another bus driver, and still control i2c routing through CPLD mux + * selection, in case the system is equipped with CPLD capable of mux selection + * control. + */ +static const struct mlxcpld_mux_desc muxes[] = { + [mlxcpld_mux_tor] = { + .nchans = CPLD_MUX_MAX_NCHANS, + .muxtype = lpc_access, + }, + [mlxcpld_mux_mgmt] = { + .nchans = CPLD_MUX_MAX_NCHANS, + .muxtype = lpc_access, + }, + [mlxcpld_mux_mgmt_ext] = { + .nchans = CPLD_MUX_EXT_MAX_NCHANS, + .muxtype = lpc_access, + }, + [mlxcpld_mux_module] = { + .nchans = CPLD_MUX_MAX_NCHANS, + .muxtype = i2c_access, + }, +}; + +static const struct i2c_device_id mlxcpld_mux_id[] = { + { "mlxcpld_mux_tor", mlxcpld_mux_tor }, + { "mlxcpld_mux_mgmt", mlxcpld_mux_mgmt }, + { "mlxcpld_mux_mgmt_ext", mlxcpld_mux_mgmt_ext }, + { "mlxcpld_mux_module", mlxcpld_mux_module }, + { } +}; +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, + enum mlxcpld_mux_access_type muxtype) +{ + struct mlxcpld_mux_platform_data *pdata = + dev_get_platdata(&client->dev); + int ret = -ENODEV; + + switch (muxtype) { + case lpc_access: + outb(val, pdata->addr); /* addr = CPLD base + offset */ + ret = 1; + break; + + case i2c_access: + if (adap->algo->master_xfer) { + struct i2c_msg msg; + u8 msgbuf[] = {pdata->sel_reg_addr, val}; + + msg.addr = pdata->addr; + msg.flags = 0; + msg.len = 2; + msg.buf = msgbuf; + return adap->algo->master_xfer(adap, &msg, 1); + } + dev_err(&client->dev, "SMBus isn't supported on this adapter\n"); + break; + + default: + dev_err(&client->dev, "Incorrect muxtype %d\n", muxtype); + } + + return ret; +} + +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_platform_data *pdata = + dev_get_platdata(&client->dev); + const struct mlxcpld_mux_desc *mux = &muxes[data->type]; + u8 regval; + int err; + + switch (data->type) { + case mlxcpld_mux_tor: + regval = pdata->first_channel + chan; + break; + + case mlxcpld_mux_mgmt: + case mlxcpld_mux_mgmt_ext: + case mlxcpld_mux_module: + regval = chan + 1; + break; + + default: + return -ENXIO; + } + + /* 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, + mux->muxtype); + data->last_chan = regval; + } + + return 0; +} + +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; + const struct mlxcpld_mux_desc *mux = &muxes[data->type]; + + /* Deselect active channel */ + data->last_chan = 0; + + return mlxcpld_mux_reg_write(muxc->parent, client, data->last_chan, + mux->muxtype); +} + +/* I2C init/probing/exit functions */ +static int mlxcpld_mux_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent); + struct mlxcpld_mux_platform_data *pdata = + dev_get_platdata(&client->dev); + struct i2c_mux_core *muxc; + int num, force; + u8 nchans; + struct mlxcpld_mux *data; + int err; + + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE)) + return -ENODEV; + + switch (id->driver_data) { + case mlxcpld_mux_tor: + case mlxcpld_mux_mgmt: + case mlxcpld_mux_mgmt_ext: + case mlxcpld_mux_module: + nchans = muxes[id->driver_data].nchans; + break; + default: + return -EINVAL; + } + + muxc = i2c_mux_alloc(adap, &client->dev, nchans, sizeof(*data), 0, + mlxcpld_mux_select_chan, mlxcpld_mux_deselect); + if (!muxc) + return -ENOMEM; + + data = i2c_mux_priv(muxc); + i2c_set_clientdata(client, muxc); + data->client = client; + data->type = id->driver_data; + data->last_chan = 0; /* force the first selection */ + + /* Only in mlxcpld_mux_tor first_channel can be different. + * In other mlxcpld_mux types channel numbering begin from 1 + * Now create an adapter for each channel + */ + for (num = 0; num < muxes[data->type].nchans; num++) { + force = 0; /* dynamic adap number */ + if (pdata) { + if (num < pdata->num_modes) + force = pdata->first_channel + num; + else + /* discard unconfigured channels */ + break; + } + + err = i2c_mux_add_adapter(muxc, force, num, 0); + if (err) { + dev_err(&client->dev, "failed to register multiplexed adapter %d as bus %d\n", + num, force); + goto virt_reg_failed; + } + } + + return 0; + +virt_reg_failed: + i2c_mux_del_adapters(muxc); + return err; +} + +static int mlxcpld_mux_remove(struct i2c_client *client) +{ + struct i2c_mux_core *muxc = i2c_get_clientdata(client); + + i2c_mux_del_adapters(muxc); + return 0; +} + +static struct i2c_driver mlxcpld_mux_driver = { + .driver = { + .name = "mlxcpld-mux", + .owner = THIS_MODULE, + }, + .probe = mlxcpld_mux_probe, + .remove = mlxcpld_mux_remove, + .id_table = mlxcpld_mux_id, +}; + +static int __init mlxcpld_mux_init(void) +{ + return i2c_add_driver(&mlxcpld_mux_driver); +} + +static void __exit mlxcpld_mux_exit(void) +{ + i2c_del_driver(&mlxcpld_mux_driver); +} + +module_init(mlxcpld_mux_init); +module_exit(mlxcpld_mux_exit); + +MODULE_AUTHOR("Michael Shych (michaels@mellanox.com)"); +MODULE_DESCRIPTION("Mellanox I2C-CPLD-MUX driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:i2c-mux-mlxcpld"); diff --git a/include/linux/i2c/mlxcpld.h b/include/linux/i2c/mlxcpld.h new file mode 100644 index 0000000..cc3236e --- /dev/null +++ b/include/linux/i2c/mlxcpld.h @@ -0,0 +1,67 @@ +/* + * mlxcpld.h - 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. + */ + +#ifndef _LINUX_I2C_MLXCPLD_H +#define _LINUX_I2C_MLXCPLD_H + +/* Platform data for the CPLD I2C multiplexers */ + +/* mlxcpld_mux_platform_mode - per channel initialisation data: + * @adap_id: bus number for the adapter. 0 = don't care + * @deselect_on_exit: set this entry to 1, if your H/W needs deselection + * of this channel after transaction. + */ +struct mlxcpld_mux_platform_mode { + int adap_id; + unsigned int deselect_on_exit; +}; + +/* mlxcpld_mux_platform_data - per mux data, used with i2c_register_board_info + * @modes - mux configuration model + * @num_modes - number of adapters + * @sel_reg_addr - mux select register offset in CPLD space + * @first_channel - first channel to start virtual busses vector + * @addr - address of mux device - set to mux select register offset on LPC + * connected CPLDs or to I2C address on I2C conncted CPLDs + */ +struct mlxcpld_mux_platform_data { + struct mlxcpld_mux_platform_mode *modes; + int num_modes; + int sel_reg_addr; + int first_channel; + unsigned short addr; +}; + +#endif /* _LINUX_I2C_MLXCPLD_H */ -- 2.1.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch 2/2] i2c: mux: mellanox: add driver 2016-08-24 13:56 ` vadimp @ 2016-08-24 13:54 ` Peter Rosin 2016-08-25 6:50 ` kbuild test robot 1 sibling, 0 replies; 10+ messages in thread From: Peter Rosin @ 2016-08-24 13:54 UTC (permalink / raw) To: vadimp, wsa; +Cc: linux-i2c, linux-kernel, jiri, Michael Shych On 2016-08-24 15:56, Vadim Pasternak wrote: > From: Vadim Pasternak <vadimp@mellanox.com> > > This driver allows I2C routing controlled through CPLD select registers on wide > range of Mellanox systems (CPLD Lattice device). > MUX selection is provided by digital and analog HW. Analog part is not under SW > control. Digital part is under CPLD control (channel selection/de-selection). > > MUX logic description. > Mux selector can control 256 mux (channels), if utilized one CPLD register > (8 bits) as select register - register value specifies mux id. > Mux selector can control n*256 mux, if utilized n CPLD registers as select > registers. > The number of registers within the same CPLD can be combined to support mux > hierarchy. > This logic can be applied for LPC attached CPLD and fro I2C attached CPLD. > Driver can support different mux control logic, according to CPLD > implementation. This paragraph is strangely wrapped. And please limit to 75 chars per line as per checkpatch. Also, I have a hard time getting the grips on the actual number of mux channels that can be controlled. You talk about n * 256 channels if you have n registers. But the code below appears to only deal with one register. So why complicate things in the comments and the commit message? It is also not entirely clear to me if you support 8 channels or if you really support 256 channels. That would be huge number of pins. Because from the text above, I would have guessed 256, but below... > Connectivity schema. > i2c-mlxcpld Digital Analog > driver > *--------* * -> mux1 (virt bus2) -> mux -> | > | I2CLPC | i2c physical * -> mux2 (virt bus3) -> mux -> | > | bridge | bus 1 *---------* | > | logic |---------------------> * mux reg * | > | in CPLD| *---------* | > *--------* i2c-mux-mlxpcld ^ * -> muxn (virt busn) -> mux -> | > | driver | | > | *---------------* | Devices > | * CPLD (LPC bus)* select | > | * registers for *--------* > | * mux selection * deselect > | *---------------* > | | > <--------> <-----------> > i2c cntrl Board cntrl reg > reg space space (mux select, > | IO, LED, WD, info) > | | *-----* *-----* > *------------- LPC bus --------------| PCH |---| CPU | > *-----* *-----* > > i2c-mux-mlxpcld does not necessary required i2c-mlxcpld. It can be use along > with another bus driver, and still control i2c routing through CPLD mux > selection, in case the system is equipped with CPLD capable of mux selection > control. > > The Kconfig currently controlling compilation of this code is: > drivers/i2c/muxes/Kconfig:config I2C_MUX_MLXCPLD > > Signed-off-by: Michael Shych <michaelsh@mellanox.com> > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> > Reviewed-by: Jiri Pirko <jiri@mellanox.com> > --- > drivers/i2c/muxes/Kconfig | 11 ++ > drivers/i2c/muxes/Makefile | 1 + > drivers/i2c/muxes/i2c-mux-mlxcpld.c | 352 ++++++++++++++++++++++++++++++++++++ > include/linux/i2c/mlxcpld.h | 67 +++++++ > 4 files changed, 431 insertions(+) > create mode 100644 drivers/i2c/muxes/i2c-mux-mlxcpld.c > create mode 100644 include/linux/i2c/mlxcpld.h > > diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig > index e280c8e..b7ab144 100644 > --- a/drivers/i2c/muxes/Kconfig > +++ b/drivers/i2c/muxes/Kconfig > @@ -81,4 +81,15 @@ config I2C_DEMUX_PINCTRL > demultiplexer that uses the pinctrl subsystem. This is useful if you > want to change the I2C master at run-time depending on features. > > +config I2C_MUX_MLXCPLD > + tristate "Mellanox CPLD based I2C multiplexer" > + help > + If you say yes to this option, support will be included for a > + CPLD based I2C multiplexer. This driver provides access to > + I2C busses connected through a MUX, which is controlled > + by a CPLD registers. > + > + This driver can also be built as a module. If so, the module > + will be called i2c-mux-mlxcpld. > + > endmenu > diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile > index 7c267c2..e5c990e 100644 > --- a/drivers/i2c/muxes/Makefile > +++ b/drivers/i2c/muxes/Makefile > @@ -10,5 +10,6 @@ obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o > obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o > obj-$(CONFIG_I2C_MUX_PINCTRL) += i2c-mux-pinctrl.o > obj-$(CONFIG_I2C_MUX_REG) += i2c-mux-reg.o > +obj-$(CONFIG_I2C_MUX_MLXCPLD) += i2c-mux-mlxcpld.o > > ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG > diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c > new file mode 100644 > index 0000000..ae860de > --- /dev/null > +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c > @@ -0,0 +1,352 @@ > +/* > + * drivers/i2c/muxes/i2c-mux-mlxcpld.c > + * 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. > + */ > + > +#include <linux/module.h> > +#include <linux/version.h> > +#include <linux/init.h> > +#include <linux/slab.h> > +#include <linux/device.h> > +#include <linux/i2c.h> > +#include <linux/i2c-mux.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > +#include <linux/i2c/mlxcpld.h> > + > +#define CPLD_MUX_MAX_NCHANS 8 > +#define CPLD_MUX_EXT_MAX_NCHANS 24 ...here, you say 8. Or 24 for some ext thingy, which is not mentioned in the commit message. So, what's going on? Why mess things up by mentioning 256? > + > +/* > + * mlxcpld_mux types - kind of mux supported by driver: > + * @mlxcpld_mux_tor - LPC access; 8 channels/legs; select/deselect - > + * channel=first defined channel(2/10) + channel/leg > + * @mlxcpld_mux_mgmt - LPC access; 8 channels/legs; select/deselect - > + * channel=1 + channel/leg > + * @mlxcpld_mux_mgmt_ext - LPC access; 24 channels/legs; select/deselect - > + * channel=1 + channel/leg > + * @mlxcpld_mux_module - I2C access; 8 channels/legs; select/deselect - > + * channel=1 + channel/leg > + */ Channels? Legs? If they are the same, then pick one name, and use it throughout. I prefer channels in that case. If you mean channels per leg, then you need to define what a leg is. > +enum mlxcpld_mux_type { > + mlxcpld_mux_tor, > + mlxcpld_mux_mgmt, > + mlxcpld_mux_mgmt_ext, > + mlxcpld_mux_module, > +}; > + > +/* mlxcpld_mux_type - underlying physical bus, to which device is connected: > + * @lpc_access - LPC connected CPLD device > + * @i2c_access - I2C connected CPLD device > + */ > +enum mlxcpld_mux_access_type { > + lpc_access, > + i2c_access, > +}; > + > +/* mlxcpld_mux - mux control structure: > + * @type - mux type > + * @last_chan - last register value > + * @client - I2C device client > + */ > +struct mlxcpld_mux { > + enum mlxcpld_mux_type type; > + u8 last_chan; > + struct i2c_client *client; > +}; > + > +/* mlxcpld_mux_desc - mux descriptor structure: > + * @nchans - number of channels > + * @muxtype - physical mux type (LPC or I2C) > + */ > +struct mlxcpld_mux_desc { > + u8 nchans; > + enum mlxcpld_mux_access_type muxtype; > +}; > + > +/* MUX logic description. > + * Mux selector can control 256 mux (channels), if utilized one CPLD register > + * (8 bits) as select register - register value specifies mux id. > + * Mux selector can control n*256 mux, if utilized n CPLD registers as select > + * registers. > + * The number of registers within the same CPLD can be combined to support > + * mux hierarchy. > + * This logic can be applied for LPC attached CPLD and fro I2C attached CPLD. > + * Driver can support different mux control logic, according to CPLD > + * implementation. > + * > + * Connectivity schema. > + * > + * i2c-mlxcpld Digital Analog > + * driver > + * *--------* * -> mux1 (virt bus2) -> mux -> | > + * | I2CLPC | i2c physical * -> mux2 (virt bus3) -> mux -> | > + * | bridge | bus 1 *---------* | > + * | logic |---------------------> * mux reg * | > + * | in CPLD| *---------* | > + * *--------* i2c-mux-mlxpcld ^ * -> muxn (virt busn) -> mux -> | > + * | driver | | > + * | *---------------* | Devices > + * | * CPLD (LPC bus)* select | > + * | * registers for *--------* > + * | * mux selection * deselect > + * | *---------------* > + * | | > + * <--------> <-----------> > + * i2c cntrl Board cntrl reg > + * reg space space (mux select, > + * | IO, LED, WD, info) > + * | | *-----* *-----* > + * *------------- LPC bus --------------| PCH |---| CPU | > + * *-----* *-----* > + * > + * i2c-mux-mlxpcld does not necessary required i2c-mlxcpld. It can be use along > + * with another bus driver, and still control i2c routing through CPLD mux > + * selection, in case the system is equipped with CPLD capable of mux selection > + * control. > + */ > +static const struct mlxcpld_mux_desc muxes[] = { > + [mlxcpld_mux_tor] = { > + .nchans = CPLD_MUX_MAX_NCHANS, > + .muxtype = lpc_access, > + }, > + [mlxcpld_mux_mgmt] = { > + .nchans = CPLD_MUX_MAX_NCHANS, > + .muxtype = lpc_access, > + }, > + [mlxcpld_mux_mgmt_ext] = { > + .nchans = CPLD_MUX_EXT_MAX_NCHANS, > + .muxtype = lpc_access, > + }, > + [mlxcpld_mux_module] = { > + .nchans = CPLD_MUX_MAX_NCHANS, > + .muxtype = i2c_access, > + }, > +}; > + > +static const struct i2c_device_id mlxcpld_mux_id[] = { > + { "mlxcpld_mux_tor", mlxcpld_mux_tor }, > + { "mlxcpld_mux_mgmt", mlxcpld_mux_mgmt }, > + { "mlxcpld_mux_mgmt_ext", mlxcpld_mux_mgmt_ext }, > + { "mlxcpld_mux_module", mlxcpld_mux_module }, > + { } > +}; > +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, > + enum mlxcpld_mux_access_type muxtype) > +{ > + struct mlxcpld_mux_platform_data *pdata = > + dev_get_platdata(&client->dev); > + int ret = -ENODEV; > + > + switch (muxtype) { > + case lpc_access: > + outb(val, pdata->addr); /* addr = CPLD base + offset */ > + ret = 1; > + break; > + > + case i2c_access: > + if (adap->algo->master_xfer) { > + struct i2c_msg msg; > + u8 msgbuf[] = {pdata->sel_reg_addr, val}; You assume there is pdata for all muxes connected via i2c. Is that certain? > + > + msg.addr = pdata->addr; > + msg.flags = 0; > + msg.len = 2; > + msg.buf = msgbuf; > + return adap->algo->master_xfer(adap, &msg, 1); > + } > + dev_err(&client->dev, "SMBus isn't supported on this adapter\n"); > + break; > + > + default: > + dev_err(&client->dev, "Incorrect muxtype %d\n", muxtype); > + } > + > + return ret; > +} > + > +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_platform_data *pdata = > + dev_get_platdata(&client->dev); > + const struct mlxcpld_mux_desc *mux = &muxes[data->type]; > + u8 regval; > + int err; > + > + switch (data->type) { > + case mlxcpld_mux_tor: > + regval = pdata->first_channel + chan; You assume that there is pdata for all muxes of type mlxcpld_mux_tor. Is that certain? > + break; > + > + case mlxcpld_mux_mgmt: > + case mlxcpld_mux_mgmt_ext: > + case mlxcpld_mux_module: > + regval = chan + 1; > + break; > + > + default: > + return -ENXIO; > + } > + > + /* 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, > + mux->muxtype); You are not making use of err, and you then set last_chan as if no error could ever occur. Is that a problem? > + data->last_chan = regval; > + } > + > + return 0; > +} > + > +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; > + const struct mlxcpld_mux_desc *mux = &muxes[data->type]; > + > + /* Deselect active channel */ > + data->last_chan = 0; > + > + return mlxcpld_mux_reg_write(muxc->parent, client, data->last_chan, > + mux->muxtype); > +} > + > +/* I2C init/probing/exit functions */ > +static int mlxcpld_mux_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent); > + struct mlxcpld_mux_platform_data *pdata = > + dev_get_platdata(&client->dev); > + struct i2c_mux_core *muxc; > + int num, force; > + u8 nchans; > + struct mlxcpld_mux *data; > + int err; > + > + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE)) > + return -ENODEV; > + > + switch (id->driver_data) { > + case mlxcpld_mux_tor: > + case mlxcpld_mux_mgmt: > + case mlxcpld_mux_mgmt_ext: > + case mlxcpld_mux_module: > + nchans = muxes[id->driver_data].nchans; > + break; > + default: > + return -EINVAL; > + } > + > + muxc = i2c_mux_alloc(adap, &client->dev, nchans, sizeof(*data), 0, > + mlxcpld_mux_select_chan, mlxcpld_mux_deselect); > + if (!muxc) > + return -ENOMEM; > + > + data = i2c_mux_priv(muxc); > + i2c_set_clientdata(client, muxc); > + data->client = client; > + data->type = id->driver_data; > + data->last_chan = 0; /* force the first selection */ > + > + /* Only in mlxcpld_mux_tor first_channel can be different. > + * In other mlxcpld_mux types channel numbering begin from 1 > + * Now create an adapter for each channel > + */ > + for (num = 0; num < muxes[data->type].nchans; num++) { > + force = 0; /* dynamic adap number */ > + if (pdata) { > + if (num < pdata->num_modes) > + force = pdata->first_channel + num; It looks as if you, as soon as pdata is supplied, tie the adapter number to the needed register value to select the mux channel. That's not good. At all. Looks like you need to dig into pdata->modes[num].adap_id > + else > + /* discard unconfigured channels */ > + break; > + } > + > + err = i2c_mux_add_adapter(muxc, force, num, 0); > + if (err) { > + dev_err(&client->dev, "failed to register multiplexed adapter %d as bus %d\n", > + num, force); > + goto virt_reg_failed; > + } > + } > + > + return 0; > + > +virt_reg_failed: > + i2c_mux_del_adapters(muxc); > + return err; > +} > + > +static int mlxcpld_mux_remove(struct i2c_client *client) > +{ > + struct i2c_mux_core *muxc = i2c_get_clientdata(client); > + > + i2c_mux_del_adapters(muxc); > + return 0; > +} > + > +static struct i2c_driver mlxcpld_mux_driver = { > + .driver = { > + .name = "mlxcpld-mux", > + .owner = THIS_MODULE, .owner is not needed. > + }, > + .probe = mlxcpld_mux_probe, > + .remove = mlxcpld_mux_remove, > + .id_table = mlxcpld_mux_id, > +}; > + Replace from here... > +static int __init mlxcpld_mux_init(void) > +{ > + return i2c_add_driver(&mlxcpld_mux_driver); > +} > + > +static void __exit mlxcpld_mux_exit(void) > +{ > + i2c_del_driver(&mlxcpld_mux_driver); > +} > + > +module_init(mlxcpld_mux_init); > +module_exit(mlxcpld_mux_exit); ...to here, with module_i2c_driver(mlxcpld_mux_driver); > + > +MODULE_AUTHOR("Michael Shych (michaels@mellanox.com)"); > +MODULE_DESCRIPTION("Mellanox I2C-CPLD-MUX driver"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:i2c-mux-mlxcpld"); > diff --git a/include/linux/i2c/mlxcpld.h b/include/linux/i2c/mlxcpld.h > new file mode 100644 > index 0000000..cc3236e > --- /dev/null > +++ b/include/linux/i2c/mlxcpld.h > @@ -0,0 +1,67 @@ > +/* > + * mlxcpld.h - 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. > + */ > + > +#ifndef _LINUX_I2C_MLXCPLD_H > +#define _LINUX_I2C_MLXCPLD_H > + > +/* Platform data for the CPLD I2C multiplexers */ > + > +/* mlxcpld_mux_platform_mode - per channel initialisation data: > + * @adap_id: bus number for the adapter. 0 = don't care > + * @deselect_on_exit: set this entry to 1, if your H/W needs deselection > + * of this channel after transaction. > + */ > +struct mlxcpld_mux_platform_mode { > + int adap_id; > + unsigned int deselect_on_exit; > +}; deselect_on_exit is not used anywhere... > + > +/* mlxcpld_mux_platform_data - per mux data, used with i2c_register_board_info > + * @modes - mux configuration model > + * @num_modes - number of adapters > + * @sel_reg_addr - mux select register offset in CPLD space > + * @first_channel - first channel to start virtual busses vector > + * @addr - address of mux device - set to mux select register offset on LPC > + * connected CPLDs or to I2C address on I2C conncted CPLDs > + */ > +struct mlxcpld_mux_platform_data { > + struct mlxcpld_mux_platform_mode *modes; ...in fact, this entire member is not used at all, which makes the above struct mlxcpld_mux_platform_mode useless. Unless you intend to fix the above problem with mixing channel number with adapter number. > + int num_modes; > + int sel_reg_addr; > + int first_channel; > + unsigned short addr; > +}; > + > +#endif /* _LINUX_I2C_MLXCPLD_H */ > I'm missing an entry in MAINTAINERS. Who will fix problems? I'm missing devicetree bindings. Is that not applicable? Cheers, Peter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch 2/2] i2c: mux: mellanox: add driver @ 2016-08-24 13:54 ` Peter Rosin 0 siblings, 0 replies; 10+ messages in thread From: Peter Rosin @ 2016-08-24 13:54 UTC (permalink / raw) To: vadimp, wsa; +Cc: linux-i2c, linux-kernel, jiri, Michael Shych On 2016-08-24 15:56, Vadim Pasternak wrote: > From: Vadim Pasternak <vadimp@mellanox.com> > > This driver allows I2C routing controlled through CPLD select registers on wide > range of Mellanox systems (CPLD Lattice device). > MUX selection is provided by digital and analog HW. Analog part is not under SW > control. Digital part is under CPLD control (channel selection/de-selection). > > MUX logic description. > Mux selector can control 256 mux (channels), if utilized one CPLD register > (8 bits) as select register - register value specifies mux id. > Mux selector can control n*256 mux, if utilized n CPLD registers as select > registers. > The number of registers within the same CPLD can be combined to support mux > hierarchy. > This logic can be applied for LPC attached CPLD and fro I2C attached CPLD. > Driver can support different mux control logic, according to CPLD > implementation. This paragraph is strangely wrapped. And please limit to 75 chars per line as per checkpatch. Also, I have a hard time getting the grips on the actual number of mux channels that can be controlled. You talk about n * 256 channels if you have n registers. But the code below appears to only deal with one register. So why complicate things in the comments and the commit message? It is also not entirely clear to me if you support 8 channels or if you really support 256 channels. That would be huge number of pins. Because from the text above, I would have guessed 256, but below... > Connectivity schema. > i2c-mlxcpld Digital Analog > driver > *--------* * -> mux1 (virt bus2) -> mux -> | > | I2CLPC | i2c physical * -> mux2 (virt bus3) -> mux -> | > | bridge | bus 1 *---------* | > | logic |---------------------> * mux reg * | > | in CPLD| *---------* | > *--------* i2c-mux-mlxpcld ^ * -> muxn (virt busn) -> mux -> | > | driver | | > | *---------------* | Devices > | * CPLD (LPC bus)* select | > | * registers for *--------* > | * mux selection * deselect > | *---------------* > | | > <--------> <-----------> > i2c cntrl Board cntrl reg > reg space space (mux select, > | IO, LED, WD, info) > | | *-----* *-----* > *------------- LPC bus --------------| PCH |---| CPU | > *-----* *-----* > > i2c-mux-mlxpcld does not necessary required i2c-mlxcpld. It can be use along > with another bus driver, and still control i2c routing through CPLD mux > selection, in case the system is equipped with CPLD capable of mux selection > control. > > The Kconfig currently controlling compilation of this code is: > drivers/i2c/muxes/Kconfig:config I2C_MUX_MLXCPLD > > Signed-off-by: Michael Shych <michaelsh@mellanox.com> > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> > Reviewed-by: Jiri Pirko <jiri@mellanox.com> > --- > drivers/i2c/muxes/Kconfig | 11 ++ > drivers/i2c/muxes/Makefile | 1 + > drivers/i2c/muxes/i2c-mux-mlxcpld.c | 352 ++++++++++++++++++++++++++++++++++++ > include/linux/i2c/mlxcpld.h | 67 +++++++ > 4 files changed, 431 insertions(+) > create mode 100644 drivers/i2c/muxes/i2c-mux-mlxcpld.c > create mode 100644 include/linux/i2c/mlxcpld.h > > diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig > index e280c8e..b7ab144 100644 > --- a/drivers/i2c/muxes/Kconfig > +++ b/drivers/i2c/muxes/Kconfig > @@ -81,4 +81,15 @@ config I2C_DEMUX_PINCTRL > demultiplexer that uses the pinctrl subsystem. This is useful if you > want to change the I2C master at run-time depending on features. > > +config I2C_MUX_MLXCPLD > + tristate "Mellanox CPLD based I2C multiplexer" > + help > + If you say yes to this option, support will be included for a > + CPLD based I2C multiplexer. This driver provides access to > + I2C busses connected through a MUX, which is controlled > + by a CPLD registers. > + > + This driver can also be built as a module. If so, the module > + will be called i2c-mux-mlxcpld. > + > endmenu > diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile > index 7c267c2..e5c990e 100644 > --- a/drivers/i2c/muxes/Makefile > +++ b/drivers/i2c/muxes/Makefile > @@ -10,5 +10,6 @@ obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o > obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o > obj-$(CONFIG_I2C_MUX_PINCTRL) += i2c-mux-pinctrl.o > obj-$(CONFIG_I2C_MUX_REG) += i2c-mux-reg.o > +obj-$(CONFIG_I2C_MUX_MLXCPLD) += i2c-mux-mlxcpld.o > > ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG > diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c > new file mode 100644 > index 0000000..ae860de > --- /dev/null > +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c > @@ -0,0 +1,352 @@ > +/* > + * drivers/i2c/muxes/i2c-mux-mlxcpld.c > + * 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. > + */ > + > +#include <linux/module.h> > +#include <linux/version.h> > +#include <linux/init.h> > +#include <linux/slab.h> > +#include <linux/device.h> > +#include <linux/i2c.h> > +#include <linux/i2c-mux.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > +#include <linux/i2c/mlxcpld.h> > + > +#define CPLD_MUX_MAX_NCHANS 8 > +#define CPLD_MUX_EXT_MAX_NCHANS 24 ...here, you say 8. Or 24 for some ext thingy, which is not mentioned in the commit message. So, what's going on? Why mess things up by mentioning 256? > + > +/* > + * mlxcpld_mux types - kind of mux supported by driver: > + * @mlxcpld_mux_tor - LPC access; 8 channels/legs; select/deselect - > + * channel=first defined channel(2/10) + channel/leg > + * @mlxcpld_mux_mgmt - LPC access; 8 channels/legs; select/deselect - > + * channel=1 + channel/leg > + * @mlxcpld_mux_mgmt_ext - LPC access; 24 channels/legs; select/deselect - > + * channel=1 + channel/leg > + * @mlxcpld_mux_module - I2C access; 8 channels/legs; select/deselect - > + * channel=1 + channel/leg > + */ Channels? Legs? If they are the same, then pick one name, and use it throughout. I prefer channels in that case. If you mean channels per leg, then you need to define what a leg is. > +enum mlxcpld_mux_type { > + mlxcpld_mux_tor, > + mlxcpld_mux_mgmt, > + mlxcpld_mux_mgmt_ext, > + mlxcpld_mux_module, > +}; > + > +/* mlxcpld_mux_type - underlying physical bus, to which device is connected: > + * @lpc_access - LPC connected CPLD device > + * @i2c_access - I2C connected CPLD device > + */ > +enum mlxcpld_mux_access_type { > + lpc_access, > + i2c_access, > +}; > + > +/* mlxcpld_mux - mux control structure: > + * @type - mux type > + * @last_chan - last register value > + * @client - I2C device client > + */ > +struct mlxcpld_mux { > + enum mlxcpld_mux_type type; > + u8 last_chan; > + struct i2c_client *client; > +}; > + > +/* mlxcpld_mux_desc - mux descriptor structure: > + * @nchans - number of channels > + * @muxtype - physical mux type (LPC or I2C) > + */ > +struct mlxcpld_mux_desc { > + u8 nchans; > + enum mlxcpld_mux_access_type muxtype; > +}; > + > +/* MUX logic description. > + * Mux selector can control 256 mux (channels), if utilized one CPLD register > + * (8 bits) as select register - register value specifies mux id. > + * Mux selector can control n*256 mux, if utilized n CPLD registers as select > + * registers. > + * The number of registers within the same CPLD can be combined to support > + * mux hierarchy. > + * This logic can be applied for LPC attached CPLD and fro I2C attached CPLD. > + * Driver can support different mux control logic, according to CPLD > + * implementation. > + * > + * Connectivity schema. > + * > + * i2c-mlxcpld Digital Analog > + * driver > + * *--------* * -> mux1 (virt bus2) -> mux -> | > + * | I2CLPC | i2c physical * -> mux2 (virt bus3) -> mux -> | > + * | bridge | bus 1 *---------* | > + * | logic |---------------------> * mux reg * | > + * | in CPLD| *---------* | > + * *--------* i2c-mux-mlxpcld ^ * -> muxn (virt busn) -> mux -> | > + * | driver | | > + * | *---------------* | Devices > + * | * CPLD (LPC bus)* select | > + * | * registers for *--------* > + * | * mux selection * deselect > + * | *---------------* > + * | | > + * <--------> <-----------> > + * i2c cntrl Board cntrl reg > + * reg space space (mux select, > + * | IO, LED, WD, info) > + * | | *-----* *-----* > + * *------------- LPC bus --------------| PCH |---| CPU | > + * *-----* *-----* > + * > + * i2c-mux-mlxpcld does not necessary required i2c-mlxcpld. It can be use along > + * with another bus driver, and still control i2c routing through CPLD mux > + * selection, in case the system is equipped with CPLD capable of mux selection > + * control. > + */ > +static const struct mlxcpld_mux_desc muxes[] = { > + [mlxcpld_mux_tor] = { > + .nchans = CPLD_MUX_MAX_NCHANS, > + .muxtype = lpc_access, > + }, > + [mlxcpld_mux_mgmt] = { > + .nchans = CPLD_MUX_MAX_NCHANS, > + .muxtype = lpc_access, > + }, > + [mlxcpld_mux_mgmt_ext] = { > + .nchans = CPLD_MUX_EXT_MAX_NCHANS, > + .muxtype = lpc_access, > + }, > + [mlxcpld_mux_module] = { > + .nchans = CPLD_MUX_MAX_NCHANS, > + .muxtype = i2c_access, > + }, > +}; > + > +static const struct i2c_device_id mlxcpld_mux_id[] = { > + { "mlxcpld_mux_tor", mlxcpld_mux_tor }, > + { "mlxcpld_mux_mgmt", mlxcpld_mux_mgmt }, > + { "mlxcpld_mux_mgmt_ext", mlxcpld_mux_mgmt_ext }, > + { "mlxcpld_mux_module", mlxcpld_mux_module }, > + { } > +}; > +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, > + enum mlxcpld_mux_access_type muxtype) > +{ > + struct mlxcpld_mux_platform_data *pdata = > + dev_get_platdata(&client->dev); > + int ret = -ENODEV; > + > + switch (muxtype) { > + case lpc_access: > + outb(val, pdata->addr); /* addr = CPLD base + offset */ > + ret = 1; > + break; > + > + case i2c_access: > + if (adap->algo->master_xfer) { > + struct i2c_msg msg; > + u8 msgbuf[] = {pdata->sel_reg_addr, val}; You assume there is pdata for all muxes connected via i2c. Is that certain? > + > + msg.addr = pdata->addr; > + msg.flags = 0; > + msg.len = 2; > + msg.buf = msgbuf; > + return adap->algo->master_xfer(adap, &msg, 1); > + } > + dev_err(&client->dev, "SMBus isn't supported on this adapter\n"); > + break; > + > + default: > + dev_err(&client->dev, "Incorrect muxtype %d\n", muxtype); > + } > + > + return ret; > +} > + > +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_platform_data *pdata = > + dev_get_platdata(&client->dev); > + const struct mlxcpld_mux_desc *mux = &muxes[data->type]; > + u8 regval; > + int err; > + > + switch (data->type) { > + case mlxcpld_mux_tor: > + regval = pdata->first_channel + chan; You assume that there is pdata for all muxes of type mlxcpld_mux_tor. Is that certain? > + break; > + > + case mlxcpld_mux_mgmt: > + case mlxcpld_mux_mgmt_ext: > + case mlxcpld_mux_module: > + regval = chan + 1; > + break; > + > + default: > + return -ENXIO; > + } > + > + /* 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, > + mux->muxtype); You are not making use of err, and you then set last_chan as if no error could ever occur. Is that a problem? > + data->last_chan = regval; > + } > + > + return 0; > +} > + > +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; > + const struct mlxcpld_mux_desc *mux = &muxes[data->type]; > + > + /* Deselect active channel */ > + data->last_chan = 0; > + > + return mlxcpld_mux_reg_write(muxc->parent, client, data->last_chan, > + mux->muxtype); > +} > + > +/* I2C init/probing/exit functions */ > +static int mlxcpld_mux_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent); > + struct mlxcpld_mux_platform_data *pdata = > + dev_get_platdata(&client->dev); > + struct i2c_mux_core *muxc; > + int num, force; > + u8 nchans; > + struct mlxcpld_mux *data; > + int err; > + > + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE)) > + return -ENODEV; > + > + switch (id->driver_data) { > + case mlxcpld_mux_tor: > + case mlxcpld_mux_mgmt: > + case mlxcpld_mux_mgmt_ext: > + case mlxcpld_mux_module: > + nchans = muxes[id->driver_data].nchans; > + break; > + default: > + return -EINVAL; > + } > + > + muxc = i2c_mux_alloc(adap, &client->dev, nchans, sizeof(*data), 0, > + mlxcpld_mux_select_chan, mlxcpld_mux_deselect); > + if (!muxc) > + return -ENOMEM; > + > + data = i2c_mux_priv(muxc); > + i2c_set_clientdata(client, muxc); > + data->client = client; > + data->type = id->driver_data; > + data->last_chan = 0; /* force the first selection */ > + > + /* Only in mlxcpld_mux_tor first_channel can be different. > + * In other mlxcpld_mux types channel numbering begin from 1 > + * Now create an adapter for each channel > + */ > + for (num = 0; num < muxes[data->type].nchans; num++) { > + force = 0; /* dynamic adap number */ > + if (pdata) { > + if (num < pdata->num_modes) > + force = pdata->first_channel + num; It looks as if you, as soon as pdata is supplied, tie the adapter number to the needed register value to select the mux channel. That's not good. At all. Looks like you need to dig into pdata->modes[num].adap_id > + else > + /* discard unconfigured channels */ > + break; > + } > + > + err = i2c_mux_add_adapter(muxc, force, num, 0); > + if (err) { > + dev_err(&client->dev, "failed to register multiplexed adapter %d as bus %d\n", > + num, force); > + goto virt_reg_failed; > + } > + } > + > + return 0; > + > +virt_reg_failed: > + i2c_mux_del_adapters(muxc); > + return err; > +} > + > +static int mlxcpld_mux_remove(struct i2c_client *client) > +{ > + struct i2c_mux_core *muxc = i2c_get_clientdata(client); > + > + i2c_mux_del_adapters(muxc); > + return 0; > +} > + > +static struct i2c_driver mlxcpld_mux_driver = { > + .driver = { > + .name = "mlxcpld-mux", > + .owner = THIS_MODULE, .owner is not needed. > + }, > + .probe = mlxcpld_mux_probe, > + .remove = mlxcpld_mux_remove, > + .id_table = mlxcpld_mux_id, > +}; > + Replace from here... > +static int __init mlxcpld_mux_init(void) > +{ > + return i2c_add_driver(&mlxcpld_mux_driver); > +} > + > +static void __exit mlxcpld_mux_exit(void) > +{ > + i2c_del_driver(&mlxcpld_mux_driver); > +} > + > +module_init(mlxcpld_mux_init); > +module_exit(mlxcpld_mux_exit); ...to here, with module_i2c_driver(mlxcpld_mux_driver); > + > +MODULE_AUTHOR("Michael Shych (michaels@mellanox.com)"); > +MODULE_DESCRIPTION("Mellanox I2C-CPLD-MUX driver"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:i2c-mux-mlxcpld"); > diff --git a/include/linux/i2c/mlxcpld.h b/include/linux/i2c/mlxcpld.h > new file mode 100644 > index 0000000..cc3236e > --- /dev/null > +++ b/include/linux/i2c/mlxcpld.h > @@ -0,0 +1,67 @@ > +/* > + * mlxcpld.h - 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. > + */ > + > +#ifndef _LINUX_I2C_MLXCPLD_H > +#define _LINUX_I2C_MLXCPLD_H > + > +/* Platform data for the CPLD I2C multiplexers */ > + > +/* mlxcpld_mux_platform_mode - per channel initialisation data: > + * @adap_id: bus number for the adapter. 0 = don't care > + * @deselect_on_exit: set this entry to 1, if your H/W needs deselection > + * of this channel after transaction. > + */ > +struct mlxcpld_mux_platform_mode { > + int adap_id; > + unsigned int deselect_on_exit; > +}; deselect_on_exit is not used anywhere... > + > +/* mlxcpld_mux_platform_data - per mux data, used with i2c_register_board_info > + * @modes - mux configuration model > + * @num_modes - number of adapters > + * @sel_reg_addr - mux select register offset in CPLD space > + * @first_channel - first channel to start virtual busses vector > + * @addr - address of mux device - set to mux select register offset on LPC > + * connected CPLDs or to I2C address on I2C conncted CPLDs > + */ > +struct mlxcpld_mux_platform_data { > + struct mlxcpld_mux_platform_mode *modes; ...in fact, this entire member is not used at all, which makes the above struct mlxcpld_mux_platform_mode useless. Unless you intend to fix the above problem with mixing channel number with adapter number. > + int num_modes; > + int sel_reg_addr; > + int first_channel; > + unsigned short addr; > +}; > + > +#endif /* _LINUX_I2C_MLXCPLD_H */ > I'm missing an entry in MAINTAINERS. Who will fix problems? I'm missing devicetree bindings. Is that not applicable? Cheers, Peter ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [patch 2/2] i2c: mux: mellanox: add driver 2016-08-24 13:54 ` Peter Rosin (?) @ 2016-08-24 16:20 ` Vadim Pasternak 2016-08-25 8:53 ` Peter Rosin -1 siblings, 1 reply; 10+ messages in thread From: Vadim Pasternak @ 2016-08-24 16:20 UTC (permalink / raw) To: Peter Rosin, wsa; +Cc: linux-i2c, linux-kernel, jiri, Michael Shych Hi Peter, Thank you very much for your review. > -----Original Message----- > From: Peter Rosin [mailto:peda@axentia.se] > Sent: Wednesday, August 24, 2016 4:55 PM > To: Vadim Pasternak <vadimp@mellanox.com>; wsa@the-dreams.de > Cc: linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; jiri@resnulli.us; > Michael Shych <michaelsh@mellanox.com> > Subject: Re: [patch 2/2] i2c: mux: mellanox: add driver > > On 2016-08-24 15:56, Vadim Pasternak wrote: > > From: Vadim Pasternak <vadimp@mellanox.com> > > > > This driver allows I2C routing controlled through CPLD select > > registers on wide range of Mellanox systems (CPLD Lattice device). > > MUX selection is provided by digital and analog HW. Analog part is not > > under SW control. Digital part is under CPLD control (channel selection/de- > selection). > > > > MUX logic description. > > Mux selector can control 256 mux (channels), if utilized one CPLD > > register > > (8 bits) as select register - register value specifies mux id. > > Mux selector can control n*256 mux, if utilized n CPLD registers as > > select registers. > > The number of registers within the same CPLD can be combined to > > support mux hierarchy. > > This logic can be applied for LPC attached CPLD and fro I2C attached CPLD. > > Driver can support different mux control logic, according to CPLD > > implementation. > > This paragraph is strangely wrapped. And please limit to 75 chars per line as per > checkpatch. > > Also, I have a hard time getting the grips on the actual number of mux channels > that can be controlled. You talk about n * 256 channels if you have n registers. > But the code below appears to only deal with one register. So why complicate > things in the comments and the commit message? It is also not entirely clear to > me if you support 8 channels or if you really support 256 channels. That would > be huge number of pins. In CPLD we have three channel selection registers, which one can support up to 256 channels. CPLD could be programmed with the bigger number of selection registers. I tried to explain it in description. Since it's misleading, I'll remove it. > > Because from the text above, I would have guessed 256, but below... > > > Connectivity schema. > > i2c-mlxcpld Digital Analog > > driver > > *--------* * -> mux1 (virt bus2) -> mux -> | > > | I2CLPC | i2c physical * -> mux2 (virt bus3) -> mux -> | > > | bridge | bus 1 *---------* | > > | logic |---------------------> * mux reg * | > > | in CPLD| *---------* | > > *--------* i2c-mux-mlxpcld ^ * -> muxn (virt busn) -> mux -> | > > | driver | | > > | *---------------* | Devices > > | * CPLD (LPC bus)* select | > > | * registers for *--------* > > | * mux selection * deselect > > | *---------------* > > | | > > <--------> <-----------> > > i2c cntrl Board cntrl reg > > reg space space (mux select, > > | IO, LED, WD, info) > > | | *-----* *-----* > > *------------- LPC bus --------------| PCH |---| CPU | > > *-----* *-----* > > > > i2c-mux-mlxpcld does not necessary required i2c-mlxcpld. It can be use > > along with another bus driver, and still control i2c routing through > > CPLD mux selection, in case the system is equipped with CPLD capable > > of mux selection control. > > > > The Kconfig currently controlling compilation of this code is: > > drivers/i2c/muxes/Kconfig:config I2C_MUX_MLXCPLD > > > > Signed-off-by: Michael Shych <michaelsh@mellanox.com> > > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> > > Reviewed-by: Jiri Pirko <jiri@mellanox.com> > > --- > > drivers/i2c/muxes/Kconfig | 11 ++ > > drivers/i2c/muxes/Makefile | 1 + > > drivers/i2c/muxes/i2c-mux-mlxcpld.c | 352 > ++++++++++++++++++++++++++++++++++++ > > include/linux/i2c/mlxcpld.h | 67 +++++++ > > 4 files changed, 431 insertions(+) > > create mode 100644 drivers/i2c/muxes/i2c-mux-mlxcpld.c > > create mode 100644 include/linux/i2c/mlxcpld.h > > > > diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig > > index e280c8e..b7ab144 100644 > > --- a/drivers/i2c/muxes/Kconfig > > +++ b/drivers/i2c/muxes/Kconfig > > @@ -81,4 +81,15 @@ config I2C_DEMUX_PINCTRL > > demultiplexer that uses the pinctrl subsystem. This is useful if you > > want to change the I2C master at run-time depending on features. > > > > +config I2C_MUX_MLXCPLD > > + tristate "Mellanox CPLD based I2C multiplexer" > > + help > > + If you say yes to this option, support will be included for a > > + CPLD based I2C multiplexer. This driver provides access to > > + I2C busses connected through a MUX, which is controlled > > + by a CPLD registers. > > + > > + This driver can also be built as a module. If so, the module > > + will be called i2c-mux-mlxcpld. > > + > > endmenu > > diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile > > index 7c267c2..e5c990e 100644 > > --- a/drivers/i2c/muxes/Makefile > > +++ b/drivers/i2c/muxes/Makefile > > @@ -10,5 +10,6 @@ obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux- > pca9541.o > > obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o > > obj-$(CONFIG_I2C_MUX_PINCTRL) += i2c-mux-pinctrl.o > > obj-$(CONFIG_I2C_MUX_REG) += i2c-mux-reg.o > > +obj-$(CONFIG_I2C_MUX_MLXCPLD) += i2c-mux-mlxcpld.o > > > > ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git > > a/drivers/i2c/muxes/i2c-mux-mlxcpld.c > > b/drivers/i2c/muxes/i2c-mux-mlxcpld.c > > new file mode 100644 > > index 0000000..ae860de > > --- /dev/null > > +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c > > @@ -0,0 +1,352 @@ > > +/* > > + * drivers/i2c/muxes/i2c-mux-mlxcpld.c > > + * 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. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/version.h> > > +#include <linux/init.h> > > +#include <linux/slab.h> > > +#include <linux/device.h> > > +#include <linux/i2c.h> > > +#include <linux/i2c-mux.h> > > +#include <linux/io.h> > > +#include <linux/platform_device.h> > > +#include <linux/i2c/mlxcpld.h> > > + > > +#define CPLD_MUX_MAX_NCHANS 8 > > +#define CPLD_MUX_EXT_MAX_NCHANS 24 > > ...here, you say 8. Or 24 for some ext thingy, which is not mentioned in the > commit message. So, what's going on? Why mess things up by mentioning 256? > Yes, we have systems only with 8 and 24 channels. Since through one register it's possible to control up to 256 channels, I mentioned it in description. I'll remove it. > > + > > +/* > > + * mlxcpld_mux types - kind of mux supported by driver: > > + * @mlxcpld_mux_tor - LPC access; 8 channels/legs; select/deselect - > > + * channel=first defined channel(2/10) + channel/leg > > + * @mlxcpld_mux_mgmt - LPC access; 8 channels/legs; select/deselect - > > + * channel=1 + channel/leg > > + * @mlxcpld_mux_mgmt_ext - LPC access; 24 channels/legs; select/deselect > - > > + * channel=1 + channel/leg > > + * @mlxcpld_mux_module - I2C access; 8 channels/legs; select/deselect - > > + * channel=1 + channel/leg > > + */ > > Channels? Legs? If they are the same, then pick one name, and use it > throughout. I prefer channels in that case. Channels. > > If you mean channels per leg, then you need to define what a leg is. > > > +enum mlxcpld_mux_type { > > + mlxcpld_mux_tor, > > + mlxcpld_mux_mgmt, > > + mlxcpld_mux_mgmt_ext, > > + mlxcpld_mux_module, > > +}; > > + > > +/* mlxcpld_mux_type - underlying physical bus, to which device is connected: > > + * @lpc_access - LPC connected CPLD device > > + * @i2c_access - I2C connected CPLD device */ enum > > +mlxcpld_mux_access_type { > > + lpc_access, > > + i2c_access, > > +}; > > + > > +/* mlxcpld_mux - mux control structure: > > + * @type - mux type > > + * @last_chan - last register value > > + * @client - I2C device client > > + */ > > +struct mlxcpld_mux { > > + enum mlxcpld_mux_type type; > > + u8 last_chan; > > + struct i2c_client *client; > > +}; > > + > > +/* mlxcpld_mux_desc - mux descriptor structure: > > + * @nchans - number of channels > > + * @muxtype - physical mux type (LPC or I2C) */ struct > > +mlxcpld_mux_desc { > > + u8 nchans; > > + enum mlxcpld_mux_access_type muxtype; }; > > + > > +/* MUX logic description. > > + * Mux selector can control 256 mux (channels), if utilized one CPLD > > +register > > + * (8 bits) as select register - register value specifies mux id. > > + * Mux selector can control n*256 mux, if utilized n CPLD registers > > +as select > > + * registers. > > + * The number of registers within the same CPLD can be combined to > > +support > > + * mux hierarchy. > > + * This logic can be applied for LPC attached CPLD and fro I2C attached CPLD. > > + * Driver can support different mux control logic, according to CPLD > > + * implementation. > > + * > > + * Connectivity schema. > > + * > > + * i2c-mlxcpld Digital Analog > > + * driver > > + * *--------* * -> mux1 (virt bus2) -> mux -> | > > + * | I2CLPC | i2c physical * -> mux2 (virt bus3) -> mux -> | > > + * | bridge | bus 1 *---------* | > > + * | logic |---------------------> * mux reg * | > > + * | in CPLD| *---------* | > > + * *--------* i2c-mux-mlxpcld ^ * -> muxn (virt busn) -> mux -> | > > + * | driver | | > > + * | *---------------* | Devices > > + * | * CPLD (LPC bus)* select | > > + * | * registers for *--------* > > + * | * mux selection * deselect > > + * | *---------------* > > + * | | > > + * <--------> <-----------> > > + * i2c cntrl Board cntrl reg > > + * reg space space (mux select, > > + * | IO, LED, WD, info) > > + * | | *-----* *-----* > > + * *------------- LPC bus --------------| PCH |---| CPU | > > + * *-----* *-----* > > + * > > + * i2c-mux-mlxpcld does not necessary required i2c-mlxcpld. It can be > > +use along > > + * with another bus driver, and still control i2c routing through > > +CPLD mux > > + * selection, in case the system is equipped with CPLD capable of mux > > +selection > > + * control. > > + */ > > +static const struct mlxcpld_mux_desc muxes[] = { > > + [mlxcpld_mux_tor] = { > > + .nchans = CPLD_MUX_MAX_NCHANS, > > + .muxtype = lpc_access, > > + }, > > + [mlxcpld_mux_mgmt] = { > > + .nchans = CPLD_MUX_MAX_NCHANS, > > + .muxtype = lpc_access, > > + }, > > + [mlxcpld_mux_mgmt_ext] = { > > + .nchans = CPLD_MUX_EXT_MAX_NCHANS, > > + .muxtype = lpc_access, > > + }, > > + [mlxcpld_mux_module] = { > > + .nchans = CPLD_MUX_MAX_NCHANS, > > + .muxtype = i2c_access, > > + }, > > +}; > > + > > +static const struct i2c_device_id mlxcpld_mux_id[] = { > > + { "mlxcpld_mux_tor", mlxcpld_mux_tor }, > > + { "mlxcpld_mux_mgmt", mlxcpld_mux_mgmt }, > > + { "mlxcpld_mux_mgmt_ext", mlxcpld_mux_mgmt_ext }, > > + { "mlxcpld_mux_module", mlxcpld_mux_module }, > > + { } > > +}; > > +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, > > + enum mlxcpld_mux_access_type muxtype) { > > + struct mlxcpld_mux_platform_data *pdata = > > + dev_get_platdata(&client- > >dev); > > + int ret = -ENODEV; > > + > > + switch (muxtype) { > > + case lpc_access: > > + outb(val, pdata->addr); /* addr = CPLD base + offset */ > > + ret = 1; > > + break; > > + > > + case i2c_access: > > + if (adap->algo->master_xfer) { > > + struct i2c_msg msg; > > + u8 msgbuf[] = {pdata->sel_reg_addr, val}; > > You assume there is pdata for all muxes connected via i2c. Is that certain? Yes > > > + > > + msg.addr = pdata->addr; > > + msg.flags = 0; > > + msg.len = 2; > > + msg.buf = msgbuf; > > + return adap->algo->master_xfer(adap, &msg, 1); > > + } > > + dev_err(&client->dev, "SMBus isn't supported on this > adapter\n"); > > + break; > > + > > + default: > > + dev_err(&client->dev, "Incorrect muxtype %d\n", muxtype); > > + } > > + > > + return ret; > > +} > > + > > +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_platform_data *pdata = > > + dev_get_platdata(&client- > >dev); > > + const struct mlxcpld_mux_desc *mux = &muxes[data->type]; > > + u8 regval; > > + int err; > > + > > + switch (data->type) { > > + case mlxcpld_mux_tor: > > + regval = pdata->first_channel + chan; > > You assume that there is pdata for all muxes of type mlxcpld_mux_tor. Is that > certain? Yes > > > + break; > > + > > + case mlxcpld_mux_mgmt: > > + case mlxcpld_mux_mgmt_ext: > > + case mlxcpld_mux_module: > > + regval = chan + 1; > > + break; > > + > > + default: > > + return -ENXIO; > > + } > > + > > + /* 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, > > + mux->muxtype); > > You are not making use of err, and you then set last_chan as if no error could > ever occur. Is that a problem? Missed. Should return err; > > > + data->last_chan = regval; > > + } > > + > > + return 0; > > +} > > + > > +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; > > + const struct mlxcpld_mux_desc *mux = &muxes[data->type]; > > + > > + /* Deselect active channel */ > > + data->last_chan = 0; > > + > > + return mlxcpld_mux_reg_write(muxc->parent, client, data->last_chan, > > + mux->muxtype); > > +} > > + > > +/* I2C init/probing/exit functions */ static int > > +mlxcpld_mux_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) { > > + struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent); > > + struct mlxcpld_mux_platform_data *pdata = > > + dev_get_platdata(&client- > >dev); > > + struct i2c_mux_core *muxc; > > + int num, force; > > + u8 nchans; > > + struct mlxcpld_mux *data; > > + int err; > > + > > + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE)) > > + return -ENODEV; > > + > > + switch (id->driver_data) { > > + case mlxcpld_mux_tor: > > + case mlxcpld_mux_mgmt: > > + case mlxcpld_mux_mgmt_ext: > > + case mlxcpld_mux_module: > > + nchans = muxes[id->driver_data].nchans; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + muxc = i2c_mux_alloc(adap, &client->dev, nchans, sizeof(*data), 0, > > + mlxcpld_mux_select_chan, mlxcpld_mux_deselect); > > + if (!muxc) > > + return -ENOMEM; > > + > > + data = i2c_mux_priv(muxc); > > + i2c_set_clientdata(client, muxc); > > + data->client = client; > > + data->type = id->driver_data; > > + data->last_chan = 0; /* force the first selection */ > > + > > + /* Only in mlxcpld_mux_tor first_channel can be different. > > + * In other mlxcpld_mux types channel numbering begin from 1 > > + * Now create an adapter for each channel > > + */ > > + for (num = 0; num < muxes[data->type].nchans; num++) { > > + force = 0; /* dynamic adap number */ > > + if (pdata) { > > + if (num < pdata->num_modes) > > + force = pdata->first_channel + num; > > It looks as if you, as soon as pdata is supplied, tie the adapter number to the > needed register value to select the mux channel. > That's not good. At all. > > Looks like you need to dig into pdata->modes[num].adap_id Do you suggest to replace force = pdata->first_channel + num; with force = pdata->modes[num].adap_id; Right? > > > + else > > + /* discard unconfigured channels */ > > + break; > > + } > > + > > + err = i2c_mux_add_adapter(muxc, force, num, 0); > > + if (err) { > > + dev_err(&client->dev, "failed to register multiplexed > adapter %d as bus %d\n", > > + num, force); > > + goto virt_reg_failed; > > + } > > + } > > + > > + return 0; > > + > > +virt_reg_failed: > > + i2c_mux_del_adapters(muxc); > > + return err; > > +} > > + > > +static int mlxcpld_mux_remove(struct i2c_client *client) { > > + struct i2c_mux_core *muxc = i2c_get_clientdata(client); > > + > > + i2c_mux_del_adapters(muxc); > > + return 0; > > +} > > + > > +static struct i2c_driver mlxcpld_mux_driver = { > > + .driver = { > > + .name = "mlxcpld-mux", > > + .owner = THIS_MODULE, > > .owner is not needed. > > > + }, > > + .probe = mlxcpld_mux_probe, > > + .remove = mlxcpld_mux_remove, > > + .id_table = mlxcpld_mux_id, > > +}; > > + > > Replace from here... > > > +static int __init mlxcpld_mux_init(void) { > > + return i2c_add_driver(&mlxcpld_mux_driver); > > +} > > + > > +static void __exit mlxcpld_mux_exit(void) { > > + i2c_del_driver(&mlxcpld_mux_driver); > > +} > > + > > +module_init(mlxcpld_mux_init); > > +module_exit(mlxcpld_mux_exit); > > ...to here, with > > module_i2c_driver(mlxcpld_mux_driver); > > > + > > +MODULE_AUTHOR("Michael Shych (michaels@mellanox.com)"); > > +MODULE_DESCRIPTION("Mellanox I2C-CPLD-MUX driver"); > > +MODULE_LICENSE("GPL v2"); MODULE_ALIAS("platform:i2c-mux-mlxcpld"); > > diff --git a/include/linux/i2c/mlxcpld.h b/include/linux/i2c/mlxcpld.h > > new file mode 100644 index 0000000..cc3236e > > --- /dev/null > > +++ b/include/linux/i2c/mlxcpld.h > > @@ -0,0 +1,67 @@ > > +/* > > + * mlxcpld.h - 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. > > + */ > > + > > +#ifndef _LINUX_I2C_MLXCPLD_H > > +#define _LINUX_I2C_MLXCPLD_H > > + > > +/* Platform data for the CPLD I2C multiplexers */ > > + > > +/* mlxcpld_mux_platform_mode - per channel initialisation data: > > + * @adap_id: bus number for the adapter. 0 = don't care > > + * @deselect_on_exit: set this entry to 1, if your H/W needs deselection > > + * of this channel after transaction. > > + */ > > +struct mlxcpld_mux_platform_mode { > > + int adap_id; > > + unsigned int deselect_on_exit; > > +}; > > deselect_on_exit is not used anywhere... > > > + > > +/* mlxcpld_mux_platform_data - per mux data, used with > > +i2c_register_board_info > > + * @modes - mux configuration model > > + * @num_modes - number of adapters > > + * @sel_reg_addr - mux select register offset in CPLD space > > + * @first_channel - first channel to start virtual busses vector > > + * @addr - address of mux device - set to mux select register offset on LPC > > + * connected CPLDs or to I2C address on I2C conncted CPLDs > > + */ > > +struct mlxcpld_mux_platform_data { > > + struct mlxcpld_mux_platform_mode *modes; > > ...in fact, this entire member is not used at all, which makes the above struct > mlxcpld_mux_platform_mode useless. Unless you intend to fix the above > problem with mixing channel number with adapter number. I'll remove mlxcpld_mux_platform_data and replace *modes with *adap_id. When select registers is written out, the previous value is overwritten. > > > + int num_modes; > > + int sel_reg_addr; > > + int first_channel; > > + unsigned short addr; > > +}; > > + > > +#endif /* _LINUX_I2C_MLXCPLD_H */ > > > > I'm missing an entry in MAINTAINERS. Who will fix problems? I'll add at myself. > I'm missing devicetree bindings. Is that not applicable? It could be something very simple, like cpldmux@da { compatible = "i2c-mux-mlxcpld"; #address-cells = <1>; #size-cells = <0>; i2c@0 { ... } } Do you think it worth adding? Also most of our systems is based x86 (but also PPC). So I'll must to add #ifdef CONFIG_OF static const struct of_device_id i2c_mux_mlxcpld_of_match[] = { { .compatible = " i2c-mux-mlxcpld ", }, {}, }; MODULE_DEVICE_TABLE(of, i2c_mux_mlxcpld_of_match); #endif Do you th9ink it's OK to have such ifdef? > > Cheers, > Peter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch 2/2] i2c: mux: mellanox: add driver 2016-08-24 16:20 ` Vadim Pasternak @ 2016-08-25 8:53 ` Peter Rosin 2016-08-25 10:08 ` Vadim Pasternak 0 siblings, 1 reply; 10+ messages in thread From: Peter Rosin @ 2016-08-25 8:53 UTC (permalink / raw) To: Vadim Pasternak, wsa; +Cc: linux-i2c, linux-kernel, jiri, Michael Shych On 2016-08-24 18:20, Vadim Pasternak wrote: > Hi Peter, > > Thank you very much for your review. > >> -----Original Message----- >> From: Peter Rosin [mailto:peda@axentia.se] >> Sent: Wednesday, August 24, 2016 4:55 PM >> To: Vadim Pasternak <vadimp@mellanox.com>; wsa@the-dreams.de >> Cc: linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; jiri@resnulli.us; >> Michael Shych <michaelsh@mellanox.com> >> Subject: Re: [patch 2/2] i2c: mux: mellanox: add driver >> >> On 2016-08-24 15:56, Vadim Pasternak wrote: >>> From: Vadim Pasternak <vadimp@mellanox.com> >>> >>> This driver allows I2C routing controlled through CPLD select >>> registers on wide range of Mellanox systems (CPLD Lattice device). >>> MUX selection is provided by digital and analog HW. Analog part is not >>> under SW control. Digital part is under CPLD control (channel selection/de- >> selection). >>> >>> MUX logic description. >>> Mux selector can control 256 mux (channels), if utilized one CPLD >>> register >>> (8 bits) as select register - register value specifies mux id. >>> Mux selector can control n*256 mux, if utilized n CPLD registers as >>> select registers. >>> The number of registers within the same CPLD can be combined to >>> support mux hierarchy. >>> This logic can be applied for LPC attached CPLD and fro I2C attached CPLD. >>> Driver can support different mux control logic, according to CPLD >>> implementation. >> >> This paragraph is strangely wrapped. And please limit to 75 chars per line as per >> checkpatch. >> >> Also, I have a hard time getting the grips on the actual number of mux channels >> that can be controlled. You talk about n * 256 channels if you have n registers. >> But the code below appears to only deal with one register. So why complicate >> things in the comments and the commit message? It is also not entirely clear to >> me if you support 8 channels or if you really support 256 channels. That would >> be huge number of pins. > > In CPLD we have three channel selection registers, which one can support up to 256 channels. > CPLD could be programmed with the bigger number of selection registers. > I tried to explain it in description. > Since it's misleading, I'll remove it. I'm a curious bastard though, and now I want to know how it works :-) So, the CPLD supports up to 768 channels, using three registers, but there is no HW that makes use of all of the options of this building block? At least there's no such "big" system yet? But then I wonder if that wouldn't be three parallel muxes, i.e. one mux for each register? Otherwise you would only need two more bits to get three times the number of channels... >> >> Because from the text above, I would have guessed 256, but below... >> >>> Connectivity schema. >>> i2c-mlxcpld Digital Analog >>> driver >>> *--------* * -> mux1 (virt bus2) -> mux -> | >>> | I2CLPC | i2c physical * -> mux2 (virt bus3) -> mux -> | >>> | bridge | bus 1 *---------* | >>> | logic |---------------------> * mux reg * | >>> | in CPLD| *---------* | >>> *--------* i2c-mux-mlxpcld ^ * -> muxn (virt busn) -> mux -> | >>> | driver | | >>> | *---------------* | Devices >>> | * CPLD (LPC bus)* select | >>> | * registers for *--------* >>> | * mux selection * deselect >>> | *---------------* >>> | | >>> <--------> <-----------> >>> i2c cntrl Board cntrl reg >>> reg space space (mux select, >>> | IO, LED, WD, info) >>> | | *-----* *-----* >>> *------------- LPC bus --------------| PCH |---| CPU | >>> *-----* *-----* >>> >>> i2c-mux-mlxpcld does not necessary required i2c-mlxcpld. It can be use >>> along with another bus driver, and still control i2c routing through >>> CPLD mux selection, in case the system is equipped with CPLD capable >>> of mux selection control. >>> >>> The Kconfig currently controlling compilation of this code is: >>> drivers/i2c/muxes/Kconfig:config I2C_MUX_MLXCPLD >>> >>> Signed-off-by: Michael Shych <michaelsh@mellanox.com> >>> Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> >>> Reviewed-by: Jiri Pirko <jiri@mellanox.com> >>> --- >>> drivers/i2c/muxes/Kconfig | 11 ++ >>> drivers/i2c/muxes/Makefile | 1 + >>> drivers/i2c/muxes/i2c-mux-mlxcpld.c | 352 >> ++++++++++++++++++++++++++++++++++++ >>> include/linux/i2c/mlxcpld.h | 67 +++++++ >>> 4 files changed, 431 insertions(+) >>> create mode 100644 drivers/i2c/muxes/i2c-mux-mlxcpld.c >>> create mode 100644 include/linux/i2c/mlxcpld.h >>> >>> a/drivers/i2c/muxes/i2c-mux-mlxcpld.c >>> b/drivers/i2c/muxes/i2c-mux-mlxcpld.c >>> new file mode 100644 >>> index 0000000..ae860de >>> --- /dev/null >>> +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c >>> @@ -0,0 +1,352 @@ *snip* >>> +#include <linux/device.h> >>> +#include <linux/i2c.h> >>> +#include <linux/i2c-mux.h> >>> +#include <linux/io.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/i2c/mlxcpld.h> >>> + >>> +#define CPLD_MUX_MAX_NCHANS 8 >>> +#define CPLD_MUX_EXT_MAX_NCHANS 24 >> >> ...here, you say 8. Or 24 for some ext thingy, which is not mentioned in the >> commit message. So, what's going on? Why mess things up by mentioning 256? >> > Yes, we have systems only with 8 and 24 channels. > Since through one register it's possible to control up to 256 channels, I mentioned it in description. > I'll remove it. >>> + >>> +/* >>> + * mlxcpld_mux types - kind of mux supported by driver: >>> + * @mlxcpld_mux_tor - LPC access; 8 channels/legs; select/deselect - >>> + * channel=first defined channel(2/10) + channel/leg >>> + * @mlxcpld_mux_mgmt - LPC access; 8 channels/legs; select/deselect - >>> + * channel=1 + channel/leg >>> + * @mlxcpld_mux_mgmt_ext - LPC access; 24 channels/legs; select/deselect >> - >>> + * channel=1 + channel/leg >>> + * @mlxcpld_mux_module - I2C access; 8 channels/legs; select/deselect - >>> + * channel=1 + channel/leg >>> + */ >> >> Channels? Legs? If they are the same, then pick one name, and use it >> throughout. I prefer channels in that case. > > Channels. > >> >> If you mean channels per leg, then you need to define what a leg is. >> >>> +enum mlxcpld_mux_type { >>> + mlxcpld_mux_tor, >>> + mlxcpld_mux_mgmt, >>> + mlxcpld_mux_mgmt_ext, >>> + mlxcpld_mux_module, >>> +}; *snip* >>> + >>> +/* 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, >>> + enum mlxcpld_mux_access_type muxtype) { >>> + struct mlxcpld_mux_platform_data *pdata = >>> + dev_get_platdata(&client- >>> dev); >>> + int ret = -ENODEV; >>> + >>> + switch (muxtype) { >>> + case lpc_access: >>> + outb(val, pdata->addr); /* addr = CPLD base + offset */ >>> + ret = 1; >>> + break; >>> + >>> + case i2c_access: >>> + if (adap->algo->master_xfer) { >>> + struct i2c_msg msg; >>> + u8 msgbuf[] = {pdata->sel_reg_addr, val}; >> >> You assume there is pdata for all muxes connected via i2c. Is that certain? > > Yes > >> >>> + >>> + msg.addr = pdata->addr; >>> + msg.flags = 0; >>> + msg.len = 2; >>> + msg.buf = msgbuf; >>> + return adap->algo->master_xfer(adap, &msg, 1); Forgot last time, __i2c_transfer is better for unlocked transfers. >>> + } >>> + dev_err(&client->dev, "SMBus isn't supported on this >> adapter\n"); >>> + break; >>> + >>> + default: >>> + dev_err(&client->dev, "Incorrect muxtype %d\n", muxtype); >>> + } >>> + >>> + return ret; >>> +} *snip* >>> +/* I2C init/probing/exit functions */ static int >>> +mlxcpld_mux_probe(struct i2c_client *client, >>> + const struct i2c_device_id *id) { >>> + struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent); >>> + struct mlxcpld_mux_platform_data *pdata = >>> + dev_get_platdata(&client- >>> dev); >>> + struct i2c_mux_core *muxc; >>> + int num, force; >>> + u8 nchans; >>> + struct mlxcpld_mux *data; >>> + int err; >>> + >>> + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE)) >>> + return -ENODEV; >>> + >>> + switch (id->driver_data) { >>> + case mlxcpld_mux_tor: >>> + case mlxcpld_mux_mgmt: >>> + case mlxcpld_mux_mgmt_ext: >>> + case mlxcpld_mux_module: >>> + nchans = muxes[id->driver_data].nchans; >>> + break; >>> + default: >>> + return -EINVAL; >>> + } >>> + >>> + muxc = i2c_mux_alloc(adap, &client->dev, nchans, sizeof(*data), 0, >>> + mlxcpld_mux_select_chan, mlxcpld_mux_deselect); >>> + if (!muxc) >>> + return -ENOMEM; >>> + >>> + data = i2c_mux_priv(muxc); >>> + i2c_set_clientdata(client, muxc); >>> + data->client = client; >>> + data->type = id->driver_data; >>> + data->last_chan = 0; /* force the first selection */ >>> + >>> + /* Only in mlxcpld_mux_tor first_channel can be different. >>> + * In other mlxcpld_mux types channel numbering begin from 1 >>> + * Now create an adapter for each channel >>> + */ >>> + for (num = 0; num < muxes[data->type].nchans; num++) { >>> + force = 0; /* dynamic adap number */ >>> + if (pdata) { >>> + if (num < pdata->num_modes) >>> + force = pdata->first_channel + num; >> >> It looks as if you, as soon as pdata is supplied, tie the adapter number to the >> needed register value to select the mux channel. >> That's not good. At all. >> >> Looks like you need to dig into pdata->modes[num].adap_id > > Do you suggest to replace > force = pdata->first_channel + num; with > force = pdata->modes[num].adap_id; > Right? Right, then you have the option to number the adapters however you like. Of course, you could still fill in adap_id so that they match first_channel + num, but you also have the option of setting adap_id to zero and have the system allocate the adapter number at runtime. *snip* >>> diff --git a/include/linux/i2c/mlxcpld.h b/include/linux/i2c/mlxcpld.h >>> new file mode 100644 index 0000000..cc3236e >>> --- /dev/null >>> +++ b/include/linux/i2c/mlxcpld.h >>> @@ -0,0 +1,67 @@ *snip* >>> +#ifndef _LINUX_I2C_MLXCPLD_H >>> +#define _LINUX_I2C_MLXCPLD_H >>> + >>> +/* Platform data for the CPLD I2C multiplexers */ >>> + >>> +/* mlxcpld_mux_platform_mode - per channel initialisation data: >>> + * @adap_id: bus number for the adapter. 0 = don't care >>> + * @deselect_on_exit: set this entry to 1, if your H/W needs deselection >>> + * of this channel after transaction. >>> + */ >>> +struct mlxcpld_mux_platform_mode { >>> + int adap_id; >>> + unsigned int deselect_on_exit; >>> +}; >> >> deselect_on_exit is not used anywhere... >> >>> + >>> +/* mlxcpld_mux_platform_data - per mux data, used with >>> +i2c_register_board_info >>> + * @modes - mux configuration model >>> + * @num_modes - number of adapters >>> + * @sel_reg_addr - mux select register offset in CPLD space >>> + * @first_channel - first channel to start virtual busses vector >>> + * @addr - address of mux device - set to mux select register offset on LPC >>> + * connected CPLDs or to I2C address on I2C conncted CPLDs >>> + */ >>> +struct mlxcpld_mux_platform_data { >>> + struct mlxcpld_mux_platform_mode *modes; >> >> ...in fact, this entire member is not used at all, which makes the above struct >> mlxcpld_mux_platform_mode useless. Unless you intend to fix the above >> problem with mixing channel number with adapter number. > > I'll remove mlxcpld_mux_platform_data and replace *modes with *adap_id. > When select registers is written out, the previous value is overwritten. If writing a value with no corresponding channel to the select register causes all real channels to be deselected, that could be used to support deselect_on_exit. If you like? >> >>> + int num_modes; >>> + int sel_reg_addr; >>> + int first_channel; >>> + unsigned short addr; >>> +}; >>> + >>> +#endif /* _LINUX_I2C_MLXCPLD_H */ >>> >> >> I'm missing an entry in MAINTAINERS. Who will fix problems? > > I'll add at myself. > >> I'm missing devicetree bindings. Is that not applicable? > > It could be something very simple, like > cpldmux@da { > compatible = "i2c-mux-mlxcpld"; > #address-cells = <1>; > #size-cells = <0>; > i2c@0 { > ... > } > } > Do you think it worth adding? compatible should have a vendor prefix (present in Documentation/devicetree/bindings/vendor-prefixes.txt), and you'd need a way to distinguish the four different variants, e.g. "mellanox,i2c-mux-cpld-tor" You'd also need some properties to fill in the info that you would otherwise get from pdata. But if the device can't exist on any system that uses devicetree, then I don't think devicetree support makes much sense. And it can always be added later... > Also most of our systems is based x86 (but also PPC). > So I'll must to add > #ifdef CONFIG_OF > static const struct of_device_id i2c_mux_mlxcpld_of_match[] = { > { .compatible = " i2c-mux-mlxcpld ", }, > {}, > }; > MODULE_DEVICE_TABLE(of, i2c_mux_mlxcpld_of_match); > #endif > > Do you th9ink it's OK to have such ifdef? Such an ifdef is ok, but you'd need four entries here as well, BTW, if you don't have need for devicetree, how are you instantiating the driver? And where is the code that fills in pdata, because it feels like pdata is unconditional? Cheers, Peter ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [patch 2/2] i2c: mux: mellanox: add driver 2016-08-25 8:53 ` Peter Rosin @ 2016-08-25 10:08 ` Vadim Pasternak 0 siblings, 0 replies; 10+ messages in thread From: Vadim Pasternak @ 2016-08-25 10:08 UTC (permalink / raw) To: Peter Rosin, wsa; +Cc: linux-i2c, linux-kernel, jiri, Michael Shych > -----Original Message----- > From: Peter Rosin [mailto:peda@axentia.se] > Sent: Thursday, August 25, 2016 11:53 AM > To: Vadim Pasternak <vadimp@mellanox.com>; wsa@the-dreams.de > Cc: linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; jiri@resnulli.us; > Michael Shych <michaelsh@mellanox.com> > Subject: Re: [patch 2/2] i2c: mux: mellanox: add driver > > On 2016-08-24 18:20, Vadim Pasternak wrote: > > Hi Peter, > > > > Thank you very much for your review. > > > >> -----Original Message----- > >> From: Peter Rosin [mailto:peda@axentia.se] > >> Sent: Wednesday, August 24, 2016 4:55 PM > >> To: Vadim Pasternak <vadimp@mellanox.com>; wsa@the-dreams.de > >> Cc: linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; > >> jiri@resnulli.us; Michael Shych <michaelsh@mellanox.com> > >> Subject: Re: [patch 2/2] i2c: mux: mellanox: add driver > >> > >> On 2016-08-24 15:56, Vadim Pasternak wrote: > >>> From: Vadim Pasternak <vadimp@mellanox.com> > >>> > >>> This driver allows I2C routing controlled through CPLD select > >>> registers on wide range of Mellanox systems (CPLD Lattice device). > >>> MUX selection is provided by digital and analog HW. Analog part is > >>> not under SW control. Digital part is under CPLD control (channel > >>> selection/de- > >> selection). > >>> > >>> MUX logic description. > >>> Mux selector can control 256 mux (channels), if utilized one CPLD > >>> register > >>> (8 bits) as select register - register value specifies mux id. > >>> Mux selector can control n*256 mux, if utilized n CPLD registers as > >>> select registers. > >>> The number of registers within the same CPLD can be combined to > >>> support mux hierarchy. > >>> This logic can be applied for LPC attached CPLD and fro I2C attached CPLD. > >>> Driver can support different mux control logic, according to CPLD > >>> implementation. > >> > >> This paragraph is strangely wrapped. And please limit to 75 chars per > >> line as per checkpatch. > >> > >> Also, I have a hard time getting the grips on the actual number of > >> mux channels that can be controlled. You talk about n * 256 channels if you > have n registers. > >> But the code below appears to only deal with one register. So why > >> complicate things in the comments and the commit message? It is also > >> not entirely clear to me if you support 8 channels or if you really > >> support 256 channels. That would be huge number of pins. > > > > In CPLD we have three channel selection registers, which one can support up > to 256 channels. > > CPLD could be programmed with the bigger number of selection registers. > > I tried to explain it in description. > > Since it's misleading, I'll remove it. > > I'm a curious bastard though, and now I want to know how it works :-) So, the > CPLD supports up to 768 channels, using three registers, but there is no HW that > makes use of all of the options of this building block? > At least there's no such "big" system yet? > Right. On our TOR systems we are using only 16 channels, controlled by two registers - one for channels on main board, the other on main board. We also have director systems, that we are using on management board up to 48 channels (on such systems we have up-to 648 100G ports. On these systems we also have such CPLD on modules (leafs, spines), which controls devices on modules (also on each modules we have 16 channels). On management board of such system we have 113 channels (adapters from i2c1 to i2c-113), where 16 channels are on top of headachy: i2c-1: has multiplexed buses from 1 to 16 i2c-3: has multiplexed buses bus 42 to 65 i2c-4: has multiplexed buses bus 66 to 89 i2c-4: has multiplexed buses bus 90 to 113 These channels go to the modules (PSU, FAN, leaf, spine). On such system we have 5 registers in CPLD for channel selection, two of top headachy (on main board and on switch board), each controls 8 channels. Three of 2-nd level of hierarchy, each controls 24 channels. Each register of 8 bytes, so theoretically you can support 255 channels through 1 register. In CPLD you can program each register as channel selection (if necessary and if you have spares). CPLD can be connected to LPC, I2C (we don't have other connections). We are using Lattice devices. > But then I wonder if that wouldn't be three parallel muxes, i.e. one mux for each > register? Otherwise you would only need two more bits to get three times the > number of channels... It's all depends on system topology. Practically you can't control all channels from one register, because of system configuration (as in my example for TOR systems, when you have two register for 8 channels, and not one for 16). In initial system we used Philips pca9548, but the we replaced it with CPLD mux control as a part of cost reduction. > > >> > >> Because from the text above, I would have guessed 256, but below... > >> > >>> Connectivity schema. > >>> i2c-mlxcpld Digital Analog > >>> driver > >>> *--------* * -> mux1 (virt bus2) -> mux -> | > >>> | I2CLPC | i2c physical * -> mux2 (virt bus3) -> mux -> | > >>> | bridge | bus 1 *---------* | > >>> | logic |---------------------> * mux reg * | > >>> | in CPLD| *---------* | > >>> *--------* i2c-mux-mlxpcld ^ * -> muxn (virt busn) -> mux -> | > >>> | driver | | > >>> | *---------------* | Devices > >>> | * CPLD (LPC bus)* select | > >>> | * registers for *--------* > >>> | * mux selection * deselect > >>> | *---------------* > >>> | | > >>> <--------> <-----------> > >>> i2c cntrl Board cntrl reg > >>> reg space space (mux select, > >>> | IO, LED, WD, info) > >>> | | *-----* *-----* > >>> *------------- LPC bus --------------| PCH |---| CPU | > >>> *-----* *-----* > >>> > >>> i2c-mux-mlxpcld does not necessary required i2c-mlxcpld. It can be > >>> use along with another bus driver, and still control i2c routing > >>> through CPLD mux selection, in case the system is equipped with CPLD > >>> capable of mux selection control. > >>> > >>> The Kconfig currently controlling compilation of this code is: > >>> drivers/i2c/muxes/Kconfig:config I2C_MUX_MLXCPLD > >>> > >>> Signed-off-by: Michael Shych <michaelsh@mellanox.com> > >>> Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> > >>> Reviewed-by: Jiri Pirko <jiri@mellanox.com> > >>> --- > >>> drivers/i2c/muxes/Kconfig | 11 ++ > >>> drivers/i2c/muxes/Makefile | 1 + > >>> drivers/i2c/muxes/i2c-mux-mlxcpld.c | 352 > >> ++++++++++++++++++++++++++++++++++++ > >>> include/linux/i2c/mlxcpld.h | 67 +++++++ > >>> 4 files changed, 431 insertions(+) > >>> create mode 100644 drivers/i2c/muxes/i2c-mux-mlxcpld.c > >>> create mode 100644 include/linux/i2c/mlxcpld.h > >>> > >>> a/drivers/i2c/muxes/i2c-mux-mlxcpld.c > >>> b/drivers/i2c/muxes/i2c-mux-mlxcpld.c > >>> new file mode 100644 > >>> index 0000000..ae860de > >>> --- /dev/null > >>> +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c > >>> @@ -0,0 +1,352 @@ > > *snip* > > >>> +#include <linux/device.h> > >>> +#include <linux/i2c.h> > >>> +#include <linux/i2c-mux.h> > >>> +#include <linux/io.h> > >>> +#include <linux/platform_device.h> > >>> +#include <linux/i2c/mlxcpld.h> > >>> + > >>> +#define CPLD_MUX_MAX_NCHANS 8 > >>> +#define CPLD_MUX_EXT_MAX_NCHANS 24 > >> > >> ...here, you say 8. Or 24 for some ext thingy, which is not mentioned > >> in the commit message. So, what's going on? Why mess things up by > mentioning 256? > >> > > Yes, we have systems only with 8 and 24 channels. > > Since through one register it's possible to control up to 256 channels, I > mentioned it in description. > > I'll remove it. > >>> + > >>> +/* > >>> + * mlxcpld_mux types - kind of mux supported by driver: > >>> + * @mlxcpld_mux_tor - LPC access; 8 channels/legs; select/deselect - > >>> + * channel=first defined channel(2/10) + channel/leg > >>> + * @mlxcpld_mux_mgmt - LPC access; 8 channels/legs; select/deselect - > >>> + * channel=1 + channel/leg > >>> + * @mlxcpld_mux_mgmt_ext - LPC access; 24 channels/legs; > >>> +select/deselect > >> - > >>> + * channel=1 + channel/leg > >>> + * @mlxcpld_mux_module - I2C access; 8 channels/legs; select/deselect - > >>> + * channel=1 + channel/leg > >>> + */ > >> > >> Channels? Legs? If they are the same, then pick one name, and use it > >> throughout. I prefer channels in that case. > > > > Channels. > > > >> > >> If you mean channels per leg, then you need to define what a leg is. > >> > >>> +enum mlxcpld_mux_type { > >>> + mlxcpld_mux_tor, > >>> + mlxcpld_mux_mgmt, > >>> + mlxcpld_mux_mgmt_ext, > >>> + mlxcpld_mux_module, > >>> +}; > > *snip* > > >>> + > >>> +/* 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, > >>> + enum mlxcpld_mux_access_type muxtype) { > >>> + struct mlxcpld_mux_platform_data *pdata = > >>> + dev_get_platdata(&client- > >>> dev); > >>> + int ret = -ENODEV; > >>> + > >>> + switch (muxtype) { > >>> + case lpc_access: > >>> + outb(val, pdata->addr); /* addr = CPLD base + offset */ > >>> + ret = 1; > >>> + break; > >>> + > >>> + case i2c_access: > >>> + if (adap->algo->master_xfer) { > >>> + struct i2c_msg msg; > >>> + u8 msgbuf[] = {pdata->sel_reg_addr, val}; > >> > >> You assume there is pdata for all muxes connected via i2c. Is that certain? > > > > Yes > > > >> > >>> + > >>> + msg.addr = pdata->addr; > >>> + msg.flags = 0; > >>> + msg.len = 2; > >>> + msg.buf = msgbuf; > >>> + return adap->algo->master_xfer(adap, &msg, 1); > > Forgot last time, __i2c_transfer is better for unlocked transfers. > > >>> + } > >>> + dev_err(&client->dev, "SMBus isn't supported on this > >> adapter\n"); > >>> + break; > >>> + > >>> + default: > >>> + dev_err(&client->dev, "Incorrect muxtype %d\n", muxtype); > >>> + } > >>> + > >>> + return ret; > >>> +} > > *snip* > > >>> +/* I2C init/probing/exit functions */ static int > >>> +mlxcpld_mux_probe(struct i2c_client *client, > >>> + const struct i2c_device_id *id) { > >>> + struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent); > >>> + struct mlxcpld_mux_platform_data *pdata = > >>> + dev_get_platdata(&client- > >>> dev); > >>> + struct i2c_mux_core *muxc; > >>> + int num, force; > >>> + u8 nchans; > >>> + struct mlxcpld_mux *data; > >>> + int err; > >>> + > >>> + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE)) > >>> + return -ENODEV; > >>> + > >>> + switch (id->driver_data) { > >>> + case mlxcpld_mux_tor: > >>> + case mlxcpld_mux_mgmt: > >>> + case mlxcpld_mux_mgmt_ext: > >>> + case mlxcpld_mux_module: > >>> + nchans = muxes[id->driver_data].nchans; > >>> + break; > >>> + default: > >>> + return -EINVAL; > >>> + } > >>> + > >>> + muxc = i2c_mux_alloc(adap, &client->dev, nchans, sizeof(*data), 0, > >>> + mlxcpld_mux_select_chan, mlxcpld_mux_deselect); > >>> + if (!muxc) > >>> + return -ENOMEM; > >>> + > >>> + data = i2c_mux_priv(muxc); > >>> + i2c_set_clientdata(client, muxc); > >>> + data->client = client; > >>> + data->type = id->driver_data; > >>> + data->last_chan = 0; /* force the first selection */ > >>> + > >>> + /* Only in mlxcpld_mux_tor first_channel can be different. > >>> + * In other mlxcpld_mux types channel numbering begin from 1 > >>> + * Now create an adapter for each channel > >>> + */ > >>> + for (num = 0; num < muxes[data->type].nchans; num++) { > >>> + force = 0; /* dynamic adap number */ > >>> + if (pdata) { > >>> + if (num < pdata->num_modes) > >>> + force = pdata->first_channel + num; > >> > >> It looks as if you, as soon as pdata is supplied, tie the adapter > >> number to the needed register value to select the mux channel. > >> That's not good. At all. > >> > >> Looks like you need to dig into pdata->modes[num].adap_id > > > > Do you suggest to replace > > force = pdata->first_channel + num; with force = > > pdata->modes[num].adap_id; Right? > > Right, then you have the option to number the adapters however you like. Of > course, you could still fill in adap_id so that they match first_channel + num, but > you also have the option of setting adap_id to zero and have the system allocate > the adapter number at runtime. > > *snip* > > >>> diff --git a/include/linux/i2c/mlxcpld.h > >>> b/include/linux/i2c/mlxcpld.h new file mode 100644 index > >>> 0000000..cc3236e > >>> --- /dev/null > >>> +++ b/include/linux/i2c/mlxcpld.h > >>> @@ -0,0 +1,67 @@ > > *snip* > > >>> +#ifndef _LINUX_I2C_MLXCPLD_H > >>> +#define _LINUX_I2C_MLXCPLD_H > >>> + > >>> +/* Platform data for the CPLD I2C multiplexers */ > >>> + > >>> +/* mlxcpld_mux_platform_mode - per channel initialisation data: > >>> + * @adap_id: bus number for the adapter. 0 = don't care > >>> + * @deselect_on_exit: set this entry to 1, if your H/W needs deselection > >>> + * of this channel after transaction. > >>> + */ > >>> +struct mlxcpld_mux_platform_mode { > >>> + int adap_id; > >>> + unsigned int deselect_on_exit; > >>> +}; > >> > >> deselect_on_exit is not used anywhere... > >> > >>> + > >>> +/* mlxcpld_mux_platform_data - per mux data, used with > >>> +i2c_register_board_info > >>> + * @modes - mux configuration model > >>> + * @num_modes - number of adapters > >>> + * @sel_reg_addr - mux select register offset in CPLD space > >>> + * @first_channel - first channel to start virtual busses vector > >>> + * @addr - address of mux device - set to mux select register offset on LPC > >>> + * connected CPLDs or to I2C address on I2C conncted CPLDs > >>> + */ > >>> +struct mlxcpld_mux_platform_data { > >>> + struct mlxcpld_mux_platform_mode *modes; > >> > >> ...in fact, this entire member is not used at all, which makes the > >> above struct mlxcpld_mux_platform_mode useless. Unless you intend to > >> fix the above problem with mixing channel number with adapter number. > > > > I'll remove mlxcpld_mux_platform_data and replace *modes with *adap_id. > > When select registers is written out, the previous value is overwritten. > > If writing a value with no corresponding channel to the select register causes all > real channels to be deselected, that could be used to support deselect_on_exit. > If you like? > > >> > >>> + int num_modes; > >>> + int sel_reg_addr; > >>> + int first_channel; > >>> + unsigned short addr; > >>> +}; > >>> + > >>> +#endif /* _LINUX_I2C_MLXCPLD_H */ > >>> > >> > >> I'm missing an entry in MAINTAINERS. Who will fix problems? > > > > I'll add at myself. > > > >> I'm missing devicetree bindings. Is that not applicable? > > > > It could be something very simple, like cpldmux@da { > > compatible = "i2c-mux-mlxcpld"; > > #address-cells = <1>; > > #size-cells = <0>; > > i2c@0 { > > ... > > } > > } > > Do you think it worth adding? > > compatible should have a vendor prefix (present in > Documentation/devicetree/bindings/vendor-prefixes.txt), and you'd need a way > to distinguish the four different variants, e.g. > "mellanox,i2c-mux-cpld-tor" > > You'd also need some properties to fill in the info that you would otherwise get > from pdata. > > But if the device can't exist on any system that uses devicetree, then I don't think > devicetree support makes much sense. And it can always be added later... > OK, if you say so, I'd prefer to add such support at next submittion and not put all to the initial one. Also at the moment we focused only on x86 systems support. > > Also most of our systems is based x86 (but also PPC). > > So I'll must to add > > #ifdef CONFIG_OF > > static const struct of_device_id i2c_mux_mlxcpld_of_match[] = { > > { .compatible = " i2c-mux-mlxcpld ", }, > > {}, > > }; > > MODULE_DEVICE_TABLE(of, i2c_mux_mlxcpld_of_match); #endif > > > > Do you th9ink it's OK to have such ifdef? > > Such an ifdef is ok, but you'd need four entries here as well, > > BTW, if you don't have need for devicetree, how are you instantiating the > driver? And where is the code that fills in pdata, because it feels like pdata is > unconditional? I have another module, which I am going to submit after this one and also fater another module i2c-mlxcpld, which I sent for review. This is arch/x86/platform/mellanox/mlx-platform.c module, which has among other platform initialization stuff code, like below: struct mlxcpld_mux_platform_mode mlxplat_mux_modes[] = { {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, }; /* Platform mux data */ struct mlxcpld_mux_platform_data mlxcpld_mux_data[] = { { .modes = mlxplat_mux_modes, .num_modes = ARRAY_SIZE(mlxplat_mux_modes), .sel_reg_addr = MLXPLAT_CPLD_LPC_REG_BASE_ADRR + MLXPLAT_CPLD_LPC_REG_OFF, .addr = MLXPLAT_CPLD_LPC_REG_BASE_ADRR + MLXPLAT_CPLD_LPC_REG_OFF, .first_channel = 2, }, { .modes = mlxplat_mux_modes, .num_modes = ARRAY_SIZE(mlxplat_mux_modes), .sel_reg_addr = MLXPLAT_CPLD_LPC_REG_BASE_ADRR + MLXPLAT_CPLD_LPC_I2C_OFF, .addr = MLXPLAT_CPLD_LPC_REG_BASE_ADRR + MLXPLAT_CPLD_LPC_I2C_OFF, .first_channel = 10, }, }; /* mlxplat_topology - board entry data, used with i2c_register_board_info * @parent_channel - parent channel for upper connection * @adapter - adapter for parent channel * @client - client for mux * @brdinfo - board specific info */ struct mlxplat_topology { int parent_channel; struct i2c_adapter *adapter; struct i2c_client *client; struct i2c_board_info brdinfo; }; /* Platform topology */ struct mlxplat_topology mlxplat_topo[] = { { .parent_channel = 1, .brdinfo = { I2C_BOARD_INFO("mlxcpld_mux_tor", 0xb), .platform_data = &mlxcpld_mux_data[0], }, }, { .parent_channel = 1, .brdinfo = { I2C_BOARD_INFO("mlxcpld_mux_tor", 0xa), .platform_data = &mlxcpld_mux_data[1], }, }, }; When I have i2c stuff submitted, I'll use here include for mlxcpld.h. > > Cheers, > Peter Cheers, Vadim. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] i2c: mux: mellanox: fix platform_no_drv_owner.cocci warnings 2016-08-24 13:56 ` vadimp @ 2016-08-25 6:50 ` kbuild test robot 2016-08-25 6:50 ` kbuild test robot 1 sibling, 0 replies; 10+ messages in thread From: kbuild test robot @ 2016-08-25 6:50 UTC (permalink / raw) To: vadimp Cc: kbuild-all, wsa, linux-i2c, linux-kernel, jiri, Vadim Pasternak, Michael Shych drivers/i2c/muxes/i2c-mux-mlxcpld.c:329:3-8: No need to set .owner here. The core will do it. Remove .owner field if calls are used which set it automatically Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci CC: Vadim Pasternak <vadimp@mellanox.com> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> --- i2c-mux-mlxcpld.c | 1 - 1 file changed, 1 deletion(-) --- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c @@ -326,7 +326,6 @@ static int mlxcpld_mux_remove(struct i2c static struct i2c_driver mlxcpld_mux_driver = { .driver = { .name = "mlxcpld-mux", - .owner = THIS_MODULE, }, .probe = mlxcpld_mux_probe, .remove = mlxcpld_mux_remove, ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] i2c: mux: mellanox: fix platform_no_drv_owner.cocci warnings @ 2016-08-25 6:50 ` kbuild test robot 0 siblings, 0 replies; 10+ messages in thread From: kbuild test robot @ 2016-08-25 6:50 UTC (permalink / raw) Cc: kbuild-all, wsa, linux-i2c, linux-kernel, jiri, Vadim Pasternak, Michael Shych drivers/i2c/muxes/i2c-mux-mlxcpld.c:329:3-8: No need to set .owner here. The core will do it. Remove .owner field if calls are used which set it automatically Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci CC: Vadim Pasternak <vadimp@mellanox.com> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> --- i2c-mux-mlxcpld.c | 1 - 1 file changed, 1 deletion(-) --- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c @@ -326,7 +326,6 @@ static int mlxcpld_mux_remove(struct i2c static struct i2c_driver mlxcpld_mux_driver = { .driver = { .name = "mlxcpld-mux", - .owner = THIS_MODULE, }, .probe = mlxcpld_mux_probe, .remove = mlxcpld_mux_remove, ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-08-25 23:38 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-25 6:50 [patch 2/2] i2c: mux: mellanox: add driver kbuild test robot 2016-08-25 6:50 ` kbuild test robot 2016-08-24 13:56 ` vadimp 2016-08-24 13:54 ` Peter Rosin 2016-08-24 13:54 ` Peter Rosin 2016-08-24 16:20 ` Vadim Pasternak 2016-08-25 8:53 ` Peter Rosin 2016-08-25 10:08 ` Vadim Pasternak 2016-08-25 6:50 ` [PATCH] i2c: mux: mellanox: fix platform_no_drv_owner.cocci warnings kbuild test robot 2016-08-25 6:50 ` kbuild test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.