Re: [PATCH i2c-mux-next 1/1] i2c: mux: Add register map based mux driver

Yann Sionneau
2023-08-23T15:00:26Z

Hi,

On 23/08/2023 16:41, Vadim Pasternak wrote:
> Add 'regmap' mux driver to allow virtual bus switching by setting a
> single selector register.
> The 'regmap' is supposed to be passed to driver within a platform data
> by parent platform driver.
>
> The register can be on type of bus or mapped memory, supported by
> 'regmap' infrastructure.
>
> Motivation is to support indirect access to register space.
> For example, Lattice FPGA LFD2NX-40 device, being connected through
> PCIe bus provides SPI or LPC bus logic through PCIe-to-SPI or
> PCIe-to-LPC bridging. Thus, FPGA operates as host controller and some
> slave devices can be connected to it. For example:
> CPU (PCIe) -> FPGA (PCIe-to-SPI bridge) -> CPLD or another FPGA
> CPU (PCIe) -> FPGA (PCIe-to-LPC bridge) -> CPLD or another FPGA
> where 1-st FPGA connected to PCIe is located on carrier board, while
> 2-nd programming logic device is located on some switch board and
> cannot be connected to CPU PCIe root complex.
>
> In case mux selector register is located within the 2-nd device, SPI or
> LPC transaction is prepared sent by indirect access, through some
> pre-defined protocol.
> To support such protocol reg_read()/reg_write() callbacks are provided
> to 'regmap' object and these callback implements required indirect
> access.
> Thus, 'regmap' performs all hardware specific access making it
> transparent to the mux driver.
>
> Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
> Reviewed-by: Michael Shych <michaelsh@nvidia.com>
> ---
> Documentation/i2c/muxes/i2c-mux-regmap.rst | 168 +++++++++++++++++++
> drivers/i2c/muxes/Kconfig | 12 ++
> drivers/i2c/muxes/Makefile | 1 +
> drivers/i2c/muxes/i2c-mux-regmap.c | 127 ++++++++++++++
> include/linux/platform_data/i2c-mux-regmap.h | 34 ++++
> 5 files changed, 342 insertions(+)
> create mode 100644 Documentation/i2c/muxes/i2c-mux-regmap.rst
> create mode 100644 drivers/i2c/muxes/i2c-mux-regmap.c
> create mode 100644 include/linux/platform_data/i2c-mux-regmap.h
>
> diff --git a/Documentation/i2c/muxes/i2c-mux-regmap.rst b/Documentation/i2c/muxes/i2c-mux-regmap.rst
> new file mode 100644
> index 000000000000..b062c1c5c02c
> --- /dev/null
> +++ b/Documentation/i2c/muxes/i2c-mux-regmap.rst
> @@ -0,0 +1,168 @@
> +.. SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +============================
> +Kernel driver i2c-mux-regmap
> +============================
> +
> +Author: Vadim Pasternak <vadimp@nvidia.com>
> +
> +Description
> +-----------
> +
> +i2c-mux-regmap is an i2c mux driver providing access to I2C bus segments
> +from a master I2C bus and a hardware MUX controlled through FPGA device
> +with indirect access to register space.
> +
> +For example, Lattice FPGA LFD2NX-40 device, being connected through PCIe
> +bus provides SPI or LPC logic through PCIe-to-SPI or PCIe-to-LPC
> +bridging.
> +Thus, FPGA operates as host controller and some slave devices can be
> +connected to it. For example:
> +- CPU (PCIe) -> FPGA (PCIe-to-SPI bridge) -> CPLD or another FPGA
> +- CPU (PCIe) -> FPGA (PCIe-to-LPC bridge) -> CPLD or another FPGA
> +where 1-st FPGA connected to PCIe is located on carrier board, while 2-nd
> +programming logic device is located on some switch board and cannot be
> +connected to CPU PCIe root complex.
> +
> +E.G.::
> + ------------------------ ---------------------------------------
> +| COME board | | Switch board |
> +| | | |
> +| ----- ------ | | ------- Bus channel 1 |
> +| | | | | | | | | *--------------> |
> +| | CPU | | FPGA |------->| CPLD | | |
> +| | | PCIe | | LPC | --- | | Bus channel 2 |
> +| | |------| | | | | |MUX|--->*--------------> Devices |
> +| | | | | | | | |REG| | | |
> +| | | | | | | | --- | | Bus channel n |
> +| | | | | | | | | *--------------> |
> +| ----- ------ | | ------- |
> +| | | |
> + ------------------------ ---------------------------------------
> +
> +SCL/SDA of the master I2C bus is multiplexed to bus segment 1..n
> +according to the settings of the MUX REG or REGS.
> +
> +Access to MUX selector registers is performed through the 'regmap' object,
> +which provides read and write methods, implementing protocol for indirect
> +access.
> +
> +Usage
> +-----
> +
> +i2c-mux-regmap uses the platform bus, so it is necessary to provide a struct
> +platform_device with the platform_data pointing to a struct
> +i2c_mux_regmap_platform_data with:
> +- The I2C adapter number of the master bus.
> +- Channels array and the number of bus channels to create.
> +- MUX select register offset in programming logic device space for bus
> + selection/deselection control.
> +- Optional callback to notify caller when all the adapters are created and
> + handle to be passed to callback.
> +See include/linux/platform_data/i2c-mux-regmap.h for details.
> +
> +Device Registration example
> +---------------------------
> +
> + /* Channels vector for regmap mux. */
> + static int regmap_mux_chan[] = { 1, 2, 3, 4, 5, 6, 7, 8 };
> +
> + /* Platform regmap mux data */
> + static struct i2c_mux_regmap_platform_data regmap_mux_data[] = {
> + {
> + .parent = 1,
> + .chan_ids = regmap_mux_chan,
> + .num_adaps = ARRAY_SIZE(regmap_mux_chan),
> + .sel_reg_addr = 0xdb,
> + },
> + {
> + .parent = 1,
> + .chan_ids = regmap_mux_chan,
> + .num_adaps = ARRAY_SIZE(regmap_mux_chan),
> + .sel_reg_addr = 0xda,
> + },
> + };
> +
> + Create regmap object.
> +
> + struct caller_regmap_context {
> + void __iomem *base;
> + };
> +
> + /* Read callback for indirect register map access */
> + static int fpga_reg_read(void *context, unsigned int reg, unsigned int *val)
> + {
> + /* Verify there is no pending transactions */
> + /* Set address in register space */
> + /* Activate read operation */
> + /* Verify transaction completion */
> + /* Read data */
> + }
> +
> + /* Write callback for indirect register map access */
> + static int reg_write(void *context, unsigned int reg, unsigned int val)
> + {
> + /* Verify there is no pending transactions */
Typo: "*are* no ... translationS" ?
> + /* Set address in register space */
> + /* Set data to be written */
> + /* Activate write operation */
> + /* Verify transaction completion */
> + }
> +
> + static struct caller_regmap_context caller_regmap_ctx;
> +
> + static const struct regmap_config fpga_regmap_config = {
> + .reg_bits = 9,
> + .val_bits = 8,
> + .max_register = 511,
> + .cache_type = REGCACHE_FLAT,
> + .writeable_reg = caller_writeable_reg,
> + .readable_reg = caller_readable_reg,
> + .volatile_reg = caller_volatile_reg,
> + .reg_defaults = caller_regmap_default,
> + .num_reg_defaults = ARRAY_SIZE(caller_regmap_default),
> + /* Methods implementing ptotocol to access PCI-LPC bridge. */
typo: "protocol"
> + .reg_read = fpga_reg_read,
> + .reg_write = fpga_reg_write,
> + };
> +
> + regmap = devm_regmap_init(&dev, NULL, &caller_regmap_ctx,
> + fpga_regmap_config);
> +
> + Remap FPGA base address.
> +
> + caller_regmap_ctx.base = devm_ioremap(&fpga_pci_dev->dev,
> + pci_resource_start(pci_dev, 0),
> + pci_resource_len(pci_dev, 0));
> +
> + For each entry in 'regmap_mux_data' array.
> +
> + mux_regmap_data[i].handle = caller_handle;
> + mux_regmap_data[i].regmap = regmap;
> + mux_regmap_data[i].completion_notify = caller_complition_notify;
> +
> + pdev[i] =
> + platform_device_register_resndata(dev, "i2c-mux-regmap", i, NULL, 0,
> + &regmap_mux_data[i],
> + sizeof(regmap_mux_data[i]));
> +
> + In the above examples two instances of "i2c-mux-regmap" will be created.
> + For each the array of the created adapter will be passed to
> + caller_complition_notify(), if this callback was provided.
> +
> +SYSFS example
> +=============
> + In 'sysfs' the channels will be exposed through path:
> + /sys/devices/platform/<caller-driver>/<i2c-parent-driver.parent-bus>
> + Following the above example it will contain:
> + i2c-mux-regmap.0/channel-1
> + ...
> + i2c-mux-regmap.0/channel-8
> + i2c-mux-regmap.1/channel-1
> + ...
> + i2c-mux-regmap.1/channel-8
> +
> + However, MUX number of each 'i2c-mux-regmap' instance is limited by
> + the size of selector register.
> + Thus, if its size is 1 byte - up to 255 MUX channels can be created,
> + for 2 bytes respectively up to 65535.
> +
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index ea838dbae32e..a509b75bd545 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -99,6 +99,18 @@ config I2C_MUX_REG
> This driver can also be built as a module. If so, the module
> will be called i2c-mux-reg.
>
> +config I2C_MUX_REGMAP
> + tristate "Register map based I2C multiplexer"
> + depends on REGMAP
> + help
> + If you say yes to this option, support will be included for a
> + register map based I2C multiplexer. This driver provides access to
> + I2C busses connected through a MUX, which is controlled
> + by a single register through the regmap.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-mux-regmap.
> +
> config I2C_DEMUX_PINCTRL
> tristate "pinctrl-based I2C demultiplexer"
> depends on PINCTRL && OF
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> index 6d9d865e8518..092dca428a75 100644
> --- a/drivers/i2c/muxes/Makefile
> +++ b/drivers/i2c/muxes/Makefile
> @@ -14,5 +14,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_REGMAP) += i2c-mux-regmap.o
>
> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/muxes/i2c-mux-regmap.c b/drivers/i2c/muxes/i2c-mux-regmap.c
> new file mode 100644
> index 000000000000..0414b0ce9096
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-mux-regmap.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +/*
> + * Regmap i2c mux driver
> + *
> + * Copyright (C) 2023 Nvidia Technologies Ltd.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-mux.h>
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/i2c-mux-regmap.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +/* i2c_mux_regmap - mux control structure:
> + * @last_val - last selected register value or -1 if mux deselected;
> + * @parent - parent I2C adapter;
> + * @pdata: platform data;
> + */
> +struct i2c_mux_regmap {
> + int last_val;
> + struct i2c_adapter *parent;
> + struct i2c_mux_regmap_platform_data pdata;
> +};
> +
> +static int i2c_mux_regmap_select_chan(struct i2c_mux_core *muxc, u32 chan)
> +{
> + struct i2c_mux_regmap *mux = i2c_mux_priv(muxc);
> + int err = 0;
> +
> + /* Only select the channel if its different from the last channel */
Typo: "it's"
> + if (mux->last_val != chan) {
> + err = regmap_write(mux->pdata.regmap, mux->pdata.sel_reg_addr, chan);

nitpick but the last 2 args of regmap_write should be of type 'unsigned int'

Maybe use this type for both `chan` and `sel_reg_addr` ?

> + mux->last_val = err < 0 ? -1 : chan;
> + }
> +
> + return err;
> +}
> +
> +static int i2c_mux_regmap_deselect(struct i2c_mux_core *muxc, u32 chan)
> +{
> + struct i2c_mux_regmap *mux = i2c_mux_priv(muxc);
> +
> + /* Deselect active channel */
> + mux->last_val = -1;
> +
> + return regmap_write(mux->pdata.regmap, mux->pdata.sel_reg_addr, 0);
> +}
> +
> +/* Probe/reomove functions */
Typo "remove"
> +static int i2c_mux_regmap_probe(struct platform_device *pdev)
> +{
> + struct i2c_mux_regmap_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + struct i2c_mux_regmap *mux;
> + struct i2c_mux_core *muxc;
> + int num, err;
> +
> + if (!pdata)
> + return -EINVAL;
> +
> + mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return -ENOMEM;
> +
> + memcpy(&mux->pdata, pdata, sizeof(*pdata));
> + mux->parent = i2c_get_adapter(mux->pdata.parent);
> + if (!mux->parent)
> + return -EPROBE_DEFER;
> +
> + muxc = i2c_mux_alloc(mux->parent, &pdev->dev, pdata->num_adaps, sizeof(*mux), 0,
> + i2c_mux_regmap_select_chan, i2c_mux_regmap_deselect);
> + if (!muxc) {
> + err = -ENOMEM;
> + goto err_i2c_mux_alloc;
> + }
> +
> + platform_set_drvdata(pdev, muxc);
> + muxc->priv = mux;
> + mux->last_val = -1; /* force the first selection */
> +
> + /* Create an adapter for each channel. */
> + for (num = 0; num < pdata->num_adaps; num++) {
> + err = i2c_mux_add_adapter(muxc, 0, pdata->chan_ids[num], 0);
> + if (err)
> + goto err_i2c_mux_add_adapter;
> + }
> +
> + /* Notify caller when all channels' adapters are created. */
> + if (pdata->completion_notify)
> + pdata->completion_notify(pdata->handle, muxc->parent, muxc->adapter);
> +
> + return 0;
> +
> +err_i2c_mux_add_adapter:
> + i2c_mux_del_adapters(muxc);
> +err_i2c_mux_alloc:
> + i2c_put_adapter(mux->parent);
> + return err;
> +}
> +
> +static void i2c_mux_regmap_remove(struct platform_device *pdev)
> +{
> + struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
> + struct i2c_mux_regmap *mux = muxc->priv;
> +
> + i2c_mux_del_adapters(muxc);
> + i2c_put_adapter(mux->parent);
> +}
> +
> +static struct platform_driver i2c_mux_regmap_driver = {
> + .driver = {
> + .name = "i2c-mux-regmap",
> + },
> + .probe = i2c_mux_regmap_probe,
> + .remove_new = i2c_mux_regmap_remove,
> +};
> +
> +module_platform_driver(i2c_mux_regmap_driver);
> +
> +MODULE_AUTHOR("Vadim Pasternak (vadimp@nvidia.com)");
> +MODULE_DESCRIPTION("Regmap I2C multiplexer driver");
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_ALIAS("platform:i2c-mux-regmap");
> diff --git a/include/linux/platform_data/i2c-mux-regmap.h b/include/linux/platform_data/i2c-mux-regmap.h
> new file mode 100644
> index 000000000000..a06614e5edd2
> --- /dev/null
> +++ b/include/linux/platform_data/i2c-mux-regmap.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */
> +/*
> + * Regmap i2c mux driver
> + *
> + * Copyright (C) 2023 Nvidia Technologies Ltd.
> + */
> +
> +#ifndef __LINUX_PLATFORM_DATA_I2C_MUX_REGMAP_H
> +#define __LINUX_PLATFORM_DATA_I2C_MUX_REGMAP_H
> +
> +/**
> + * struct i2c_mux_regmap_platform_data - Platform-dependent data for i2c-mux-regmap
> + * @regmap: register map of parent device;
> + * @parent: Parent I2C bus adapter number
> + * @chan_ids - channels array
> + * @num_adaps - number of adapters
> + * @sel_reg_addr - mux select register offset in CPLD space
> + * @reg_size: register size in bytes
> + * @handle: handle to be passed by callback
> + * @completion_notify: callback to notify when all the adapters are created
> + */
> +struct i2c_mux_regmap_platform_data {
> + void *regmap;
> + int parent;
> + const unsigned int *chan_ids;
> + int num_adaps;
Why not using unsigned int for `num_adaps` as well?
> + int sel_reg_addr;
> + u8 reg_size;
> + void *handle;
> + int (*completion_notify)(void *handle, struct i2c_adapter *parent,
> + struct i2c_adapter *adapters[]);
> +};
> +
> +#endif /* __LINUX_PLATFORM_DATA_I2C_MUX_REGMAP_H */

Regards,

--

Yann