| Message ID | 20240426-dev-mule-i2c-mux-v1-3-045a482f6ffb@theobroma-systems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Mule I2C multiplexer support | expand |
On 26/04/2024 18:49, Farouk Bouabid wrote: > Mule is an mcu that emulates a set of i2c devices which are reacheable > through an i2c-mux. > > The emulated devices share a single i2c address with the mux itself where > the requested register is what determines which logic is executed (mux or > device): > > 1- The devices on the mux can be selected (mux function) by writing the > appropriate device number to an i2c config register (0xff) that is not > used by any device logic. > > 2- Any access to a register other than the config register will be > handled by the previously selected device. > > Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com> > --- > drivers/i2c/muxes/Kconfig | 11 +++ > drivers/i2c/muxes/Makefile | 1 + > drivers/i2c/muxes/i2c-mux-mule.c | 157 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 169 insertions(+) > > diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig > index db1b9057612a..593a20a6ac51 100644 > --- a/drivers/i2c/muxes/Kconfig > +++ b/drivers/i2c/muxes/Kconfig > @@ -119,4 +119,15 @@ config I2C_MUX_MLXCPLD > This driver can also be built as a module. If so, the module > will be called i2c-mux-mlxcpld. > > +config I2C_MUX_MULE > + tristate "Mule I2C device multiplexer" > + depends on OF > + help > + If you say yes to this option, support will be included for a > + Mule I2C device multiplexer. This driver provides access to > + I2C devices connected on the Mule I2C mux. Describe what is Mule. Here and in bindings documentation. > + > + This driver can also be built as a module. If so, the module > + will be called i2c-mux-mule. > + > endmenu > diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile > index 6d9d865e8518..4b24f49515a7 100644 > --- a/drivers/i2c/muxes/Makefile > +++ b/drivers/i2c/muxes/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_I2C_MUX_GPIO) += i2c-mux-gpio.o > obj-$(CONFIG_I2C_MUX_GPMUX) += i2c-mux-gpmux.o > obj-$(CONFIG_I2C_MUX_LTC4306) += i2c-mux-ltc4306.o > obj-$(CONFIG_I2C_MUX_MLXCPLD) += i2c-mux-mlxcpld.o > +obj-$(CONFIG_I2C_MUX_MULE) += i2c-mux-mule.o > 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 > diff --git a/drivers/i2c/muxes/i2c-mux-mule.c b/drivers/i2c/muxes/i2c-mux-mule.c > new file mode 100644 > index 000000000000..da2a9526522e > --- /dev/null > +++ b/drivers/i2c/muxes/i2c-mux-mule.c > @@ -0,0 +1,157 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Mule I2C device multiplexer > + * > + * Copyright (C) 2024 Theobroma Systems Design und Consulting GmbH > + */ > + > +#include <linux/i2c-mux.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > + > +#define MUX_CONFIG_REG 0xff > +#define MUX_DEFAULT_DEV 0x0 > + > +struct mule_i2c_reg_mux { > + struct regmap *regmap; > +}; > + > +static const struct regmap_config mule_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > +}; > + > +static const struct of_device_id mule_i2c_mux_of_match[] = { > + {.compatible = "tsd,mule-i2c-mux",}, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, mule_i2c_mux_of_match); This goes after or before probe. Don't introduce unusual coding style. > + ... > +static void mux_remove(void *data) > +{ > + struct i2c_mux_core *muxc = data; > + > + i2c_mux_del_adapters(muxc); > + > + mux_deselect(muxc, MUX_DEFAULT_DEV); > +} > + > +static int mule_i2c_mux_probe(struct i2c_client *client) > +{ > + struct i2c_adapter *adap = client->adapter; > + struct mule_i2c_reg_mux *priv; > + struct i2c_mux_core *muxc; > + struct device_node *dev; > + unsigned int readback; > + bool old_fw; > + int ndev, ret; > + > + /* Count devices on the mux */ > + ndev = of_get_child_count(client->dev.of_node); > + dev_dbg(&client->dev, "%u devices on the mux\n", ndev); > + > + muxc = i2c_mux_alloc(adap, &client->dev, > + ndev, sizeof(*priv), > + I2C_MUX_LOCKED, > + mux_select, mux_deselect); Very odd alignment. This is absolutely unreadable. Please properly align with opening (. > + if (!muxc) > + return -ENOMEM; > + > + muxc->share_addr_with_children = 1; > + priv = i2c_mux_priv(muxc); > + > + priv->regmap = devm_regmap_init_i2c(client, &mule_regmap_config); > + if (IS_ERR(priv->regmap)) > + return dev_err_probe(&client->dev, PTR_ERR(priv->regmap), > + "Failed to allocate i2c register map\n"); > + > + i2c_set_clientdata(client, muxc); > + > + /* > + * Mux 0 is guaranteed to exist on all old and new mule fw. > + * mule fw without mux support will accept write ops to the > + * config register, but readback returns 0xff (register not updated). > + */ > + ret = mux_select(muxc, 0); > + if (ret) > + return ret; > + > + ret = regmap_read(priv->regmap, MUX_CONFIG_REG, &readback); > + if (ret) > + return ret; > + > + old_fw = (readback == 0); > + > + ret = devm_add_action_or_reset(&client->dev, mux_remove, muxc); This is really odd. Why do you call remove callback as devm action? I have serious doubts this was really tested. > + if (ret) > + return ret; > + > + /* Create device adapters */ > + for_each_child_of_node(client->dev.of_node, dev) { > + u32 reg; > + > + ret = of_property_read_u32(dev, "reg", ®); > + if (ret) { > + dev_err(&client->dev, "No reg property found for %s: %d\n", > + of_node_full_name(dev), ret); Very odd alignment. Please properly align with opening (. > + return ret; > + } > + > + if (!old_fw && reg != 0) { > + dev_warn(&client->dev, > + "Mux %d not supported, please update Mule FW\n", reg); > + continue; > + } > + > + ret = mux_select(muxc, reg); > + if (ret) { > + dev_warn(&client->dev, > + "Mux %d not supported, please update Mule FW\n", reg); > + continue; > + } > + > + ret = i2c_mux_add_adapter(muxc, 0, reg, 0); > + if (ret) { > + dev_err(&client->dev, "Failed to add i2c mux adapter %d: %d\n", reg, ret); > + return ret; > + } > + } > + > + mux_deselect(muxc, MUX_DEFAULT_DEV); > + > + return 0; > +} > + > +static struct i2c_driver mule_i2c_mux_driver = { > + .driver = { > + .name = "mule-i2c-mux", > + .of_match_table = mule_i2c_mux_of_match, > + }, > + .probe = mule_i2c_mux_probe, > +}; > + Anyway, all this looks like i2c-mux-reg. Please provide rationale in commit msg WHY you need one more driver. Best regards, Krzysztof
Hi Krzysztof, On 29.04.24 08:33, Krzysztof Kozlowski wrote: > [You don't often get email from krzysztof.kozlowski@linaro.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > On 26/04/2024 18:49, Farouk Bouabid wrote: >> Mule is an mcu that emulates a set of i2c devices which are reacheable >> through an i2c-mux. >> >> The emulated devices share a single i2c address with the mux itself where >> the requested register is what determines which logic is executed (mux or >> device): >> >> 1- The devices on the mux can be selected (mux function) by writing the >> appropriate device number to an i2c config register (0xff) that is not >> used by any device logic. >> >> 2- Any access to a register other than the config register will be >> handled by the previously selected device. >> >> Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com> >> --- >> drivers/i2c/muxes/Kconfig | 11 +++ >> drivers/i2c/muxes/Makefile | 1 + >> drivers/i2c/muxes/i2c-mux-mule.c | 157 +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 169 insertions(+) >> >> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig >> index db1b9057612a..593a20a6ac51 100644 >> --- a/drivers/i2c/muxes/Kconfig >> +++ b/drivers/i2c/muxes/Kconfig >> @@ -119,4 +119,15 @@ config I2C_MUX_MLXCPLD >> This driver can also be built as a module. If so, the module >> will be called i2c-mux-mlxcpld. >> >> +config I2C_MUX_MULE >> + tristate "Mule I2C device multiplexer" >> + depends on OF >> + help >> + If you say yes to this option, support will be included for a >> + Mule I2C device multiplexer. This driver provides access to >> + I2C devices connected on the Mule I2C mux. > Describe what is Mule. Here and in bindings documentation. Is the description in bindings documentation good enough to be copied here ? If not, any suggestions? [snip] >> + if (!muxc) >> + return -ENOMEM; >> + >> + muxc->share_addr_with_children = 1; >> + priv = i2c_mux_priv(muxc); >> + >> + priv->regmap = devm_regmap_init_i2c(client, &mule_regmap_config); >> + if (IS_ERR(priv->regmap)) >> + return dev_err_probe(&client->dev, PTR_ERR(priv->regmap), >> + "Failed to allocate i2c register map\n"); >> + >> + i2c_set_clientdata(client, muxc); >> + >> + /* >> + * Mux 0 is guaranteed to exist on all old and new mule fw. >> + * mule fw without mux support will accept write ops to the >> + * config register, but readback returns 0xff (register not updated). >> + */ >> + ret = mux_select(muxc, 0); >> + if (ret) >> + return ret; >> + >> + ret = regmap_read(priv->regmap, MUX_CONFIG_REG, &readback); >> + if (ret) >> + return ret; >> + >> + old_fw = (readback == 0); >> + >> + ret = devm_add_action_or_reset(&client->dev, mux_remove, muxc); > This is really odd. Why do you call remove callback as devm action? > > I have serious doubts this was really tested. This was tested in both scenarios where the probe fails and while removing the driver. The remove function is added as devm action to the unwinding path, which will basically be called when removing the driver OR when the probe fails after this call. https://elixir.bootlin.com/linux/latest/source/drivers/base/devres.c#L734 [snip] > Anyway, all this looks like i2c-mux-reg. Please provide rationale in > commit msg WHY you need one more driver. We are basically using an i2c device (mux) and an i2c register to handle the devices behind the mux which is not what the "i2c-mux-reg" offers, where the mux is a platform device and the register used is part of the io-memory. Also to check that backward compatibility is valid on older Mule versions, we perform some tests in the probe function that are specific to Mule. Did I miss something? Best regards Farouk
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig index db1b9057612a..593a20a6ac51 100644 --- a/drivers/i2c/muxes/Kconfig +++ b/drivers/i2c/muxes/Kconfig @@ -119,4 +119,15 @@ config I2C_MUX_MLXCPLD This driver can also be built as a module. If so, the module will be called i2c-mux-mlxcpld. +config I2C_MUX_MULE + tristate "Mule I2C device multiplexer" + depends on OF + help + If you say yes to this option, support will be included for a + Mule I2C device multiplexer. This driver provides access to + I2C devices connected on the Mule I2C mux. + + This driver can also be built as a module. If so, the module + will be called i2c-mux-mule. + endmenu diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile index 6d9d865e8518..4b24f49515a7 100644 --- a/drivers/i2c/muxes/Makefile +++ b/drivers/i2c/muxes/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_I2C_MUX_GPIO) += i2c-mux-gpio.o obj-$(CONFIG_I2C_MUX_GPMUX) += i2c-mux-gpmux.o obj-$(CONFIG_I2C_MUX_LTC4306) += i2c-mux-ltc4306.o obj-$(CONFIG_I2C_MUX_MLXCPLD) += i2c-mux-mlxcpld.o +obj-$(CONFIG_I2C_MUX_MULE) += i2c-mux-mule.o 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 diff --git a/drivers/i2c/muxes/i2c-mux-mule.c b/drivers/i2c/muxes/i2c-mux-mule.c new file mode 100644 index 000000000000..da2a9526522e --- /dev/null +++ b/drivers/i2c/muxes/i2c-mux-mule.c @@ -0,0 +1,157 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Mule I2C device multiplexer + * + * Copyright (C) 2024 Theobroma Systems Design und Consulting GmbH + */ + +#include <linux/i2c-mux.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/property.h> +#include <linux/regmap.h> + +#define MUX_CONFIG_REG 0xff +#define MUX_DEFAULT_DEV 0x0 + +struct mule_i2c_reg_mux { + struct regmap *regmap; +}; + +static const struct regmap_config mule_regmap_config = { + .reg_bits = 8, + .val_bits = 8, +}; + +static const struct of_device_id mule_i2c_mux_of_match[] = { + {.compatible = "tsd,mule-i2c-mux",}, + {}, +}; +MODULE_DEVICE_TABLE(of, mule_i2c_mux_of_match); + +static inline int __mux_select(struct regmap *regmap, u32 dev) +{ + return regmap_write(regmap, MUX_CONFIG_REG, dev); +} + +static int mux_select(struct i2c_mux_core *muxc, u32 dev) +{ + struct mule_i2c_reg_mux *mux = muxc->priv; + + return __mux_select(mux->regmap, dev); +} + +static int mux_deselect(struct i2c_mux_core *muxc, u32 dev) +{ + return mux_select(muxc, MUX_DEFAULT_DEV); +} + +static void mux_remove(void *data) +{ + struct i2c_mux_core *muxc = data; + + i2c_mux_del_adapters(muxc); + + mux_deselect(muxc, MUX_DEFAULT_DEV); +} + +static int mule_i2c_mux_probe(struct i2c_client *client) +{ + struct i2c_adapter *adap = client->adapter; + struct mule_i2c_reg_mux *priv; + struct i2c_mux_core *muxc; + struct device_node *dev; + unsigned int readback; + bool old_fw; + int ndev, ret; + + /* Count devices on the mux */ + ndev = of_get_child_count(client->dev.of_node); + dev_dbg(&client->dev, "%u devices on the mux\n", ndev); + + muxc = i2c_mux_alloc(adap, &client->dev, + ndev, sizeof(*priv), + I2C_MUX_LOCKED, + mux_select, mux_deselect); + if (!muxc) + return -ENOMEM; + + muxc->share_addr_with_children = 1; + priv = i2c_mux_priv(muxc); + + priv->regmap = devm_regmap_init_i2c(client, &mule_regmap_config); + if (IS_ERR(priv->regmap)) + return dev_err_probe(&client->dev, PTR_ERR(priv->regmap), + "Failed to allocate i2c register map\n"); + + i2c_set_clientdata(client, muxc); + + /* + * Mux 0 is guaranteed to exist on all old and new mule fw. + * mule fw without mux support will accept write ops to the + * config register, but readback returns 0xff (register not updated). + */ + ret = mux_select(muxc, 0); + if (ret) + return ret; + + ret = regmap_read(priv->regmap, MUX_CONFIG_REG, &readback); + if (ret) + return ret; + + old_fw = (readback == 0); + + ret = devm_add_action_or_reset(&client->dev, mux_remove, muxc); + if (ret) + return ret; + + /* Create device adapters */ + for_each_child_of_node(client->dev.of_node, dev) { + u32 reg; + + ret = of_property_read_u32(dev, "reg", ®); + if (ret) { + dev_err(&client->dev, "No reg property found for %s: %d\n", + of_node_full_name(dev), ret); + return ret; + } + + if (!old_fw && reg != 0) { + dev_warn(&client->dev, + "Mux %d not supported, please update Mule FW\n", reg); + continue; + } + + ret = mux_select(muxc, reg); + if (ret) { + dev_warn(&client->dev, + "Mux %d not supported, please update Mule FW\n", reg); + continue; + } + + ret = i2c_mux_add_adapter(muxc, 0, reg, 0); + if (ret) { + dev_err(&client->dev, "Failed to add i2c mux adapter %d: %d\n", reg, ret); + return ret; + } + } + + mux_deselect(muxc, MUX_DEFAULT_DEV); + + return 0; +} + +static struct i2c_driver mule_i2c_mux_driver = { + .driver = { + .name = "mule-i2c-mux", + .of_match_table = mule_i2c_mux_of_match, + }, + .probe = mule_i2c_mux_probe, +}; + +module_i2c_driver(mule_i2c_mux_driver); + +MODULE_AUTHOR("Farouk Bouabid <farouk.bouabid@theobroma-systems.com>"); +MODULE_DESCRIPTION("I2C mux driver for Mule"); +MODULE_LICENSE("GPL");
Mule is an mcu that emulates a set of i2c devices which are reacheable through an i2c-mux. The emulated devices share a single i2c address with the mux itself where the requested register is what determines which logic is executed (mux or device): 1- The devices on the mux can be selected (mux function) by writing the appropriate device number to an i2c config register (0xff) that is not used by any device logic. 2- Any access to a register other than the config register will be handled by the previously selected device. Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com> --- drivers/i2c/muxes/Kconfig | 11 +++ drivers/i2c/muxes/Makefile | 1 + drivers/i2c/muxes/i2c-mux-mule.c | 157 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+)