| Message ID | 20240725-dev-mule-i2c-mux-v6-2-f9f6d7b60fb2@cherry.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add tsd,mule-i2c-mux support | expand |
Hi all, Gentle ping. +To Wolfram Sang @Wolfram I see that you're the one merging most patches in in drivers/i2c/muxes recently but ./scripts/get_maintainer.pl doesn't return your mail address for the patches in that folder hence why you weren't explicitly added (we used b4 prep --auto-to-cc) I assume this is due to: https://elixir.bootlin.com/linux/v6.10.2/source/MAINTAINERS#L10339 which according to https://elixir.bootlin.com/linux/v6.10.2/source/MAINTAINERS#L36 means everything that is a direct child from drivers/i2c but not deeper than that. Is this expected? +Cc Andi Shyti, since patches that weren't committed by Wolfram seems to have been by Andi. Though I'm not entirely sure which entry in MAINTAINERS shows that responsibility? Are we missing an update to MAINTAINERS or is there an issue in how we get the maintainers for those patches somehow? Looking forward to receiving feedback on those patches, thanks! For the original link of the patch series if somehow it didn't make it to your inboxes: https://lore.kernel.org/linux-i2c/20240725-dev-mule-i2c-mux-v6-0-f9f6d7b60fb2@cherry.de/ Cheers, Quentin On 7/25/24 3:27 PM, Farouk Bouabid wrote: > Theobroma Systems Mule is an MCU that emulates a set of I2C devices, > among which an amc6821 and devices that are reachable through an I2C-mux. > The devices on the mux can be selected by writing the appropriate device > number to an I2C config register (amc6821 reg 0xff). > > This driver is expected to be probed as a platform device with amc6821 > as its parent i2c device. > > Add support for the mule-i2c-mux platform driver. The amc6821 driver > support for the mux will be added in a later commit. > > Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> > --- > drivers/i2c/muxes/Kconfig | 16 ++++ > drivers/i2c/muxes/Makefile | 1 + > drivers/i2c/muxes/i2c-mux-mule.c | 155 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 172 insertions(+) > > diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig > index db1b9057612a..6d2f66810cdc 100644 > --- a/drivers/i2c/muxes/Kconfig > +++ b/drivers/i2c/muxes/Kconfig > @@ -119,4 +119,20 @@ 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 "Theobroma Systems Mule I2C device multiplexer" > + depends on OF && SENSORS_AMC6821 > + help > + Mule is an MCU that emulates a set of I2C devices, among which > + devices that are reachable through an I2C-mux. The devices on the mux > + can be selected by writing the appropriate device number to an I2C > + configuration register. > + > + If you say yes to this option, support will be included for a > + Theobroma Systems Mule I2C multiplexer. This driver provides access to > + I2C devices connected on this 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..062596869651 > --- /dev/null > +++ b/drivers/i2c/muxes/i2c-mux-mule.c > @@ -0,0 +1,155 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Theobroma Systems 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/platform_device.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 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 platform_device *pdev) > +{ > + struct device *mux_dev = &pdev->dev; > + struct mule_i2c_reg_mux *priv; > + struct i2c_client *client; > + struct i2c_mux_core *muxc; > + struct device_node *dev; > + unsigned int readback; > + int ndev, ret; > + bool old_fw; > + > + /* Count devices on the mux */ > + ndev = of_get_child_count(mux_dev->of_node); > + dev_dbg(mux_dev, "%d devices on the mux\n", ndev); > + > + client = to_i2c_client(mux_dev->parent); > + > + muxc = i2c_mux_alloc(client->adapter, mux_dev, ndev, sizeof(*priv), > + I2C_MUX_LOCKED, mux_select, mux_deselect); > + if (!muxc) > + return dev_err_probe(mux_dev, -ENOMEM, > + "Failed to allocate mux struct\n"); > + > + priv = i2c_mux_priv(muxc); > + > + priv->regmap = dev_get_regmap(mux_dev->parent, NULL); > + if (IS_ERR(priv->regmap)) > + return dev_err_probe(mux_dev, PTR_ERR(priv->regmap), > + "No parent i2c register map\n"); > + > + platform_set_drvdata(pdev, muxc); > + > + /* > + * MUX_DEFAULT_DEV 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, MUX_DEFAULT_DEV); > + if (ret) > + return dev_err_probe(mux_dev, ret, > + "Failed to write config register\n"); > + > + ret = regmap_read(priv->regmap, MUX_CONFIG_REG, &readback); > + if (ret) > + return dev_err_probe(mux_dev, ret, > + "Failed to read config register\n"); > + > + old_fw = (readback != MUX_DEFAULT_DEV); > + > + ret = devm_add_action_or_reset(mux_dev, mux_remove, muxc); > + if (ret) > + return dev_err_probe(mux_dev, ret, > + "Failed to register mux remove\n"); > + > + /* Create device adapters */ > + for_each_child_of_node(mux_dev->of_node, dev) { > + u32 reg; > + > + ret = of_property_read_u32(dev, "reg", ®); > + if (ret) > + return dev_err_probe(mux_dev, ret, > + "No reg property found for %s\n", > + of_node_full_name(dev)); > + > + if (old_fw && reg != 0) { > + dev_warn(mux_dev, > + "Mux is not supported, please update Mule FW\n"); > + continue; > + } > + > + ret = mux_select(muxc, reg); > + if (ret) { > + dev_warn(mux_dev, > + "Device %d not supported, please update Mule FW\n", reg); > + continue; > + } > + > + ret = i2c_mux_add_adapter(muxc, 0, reg); > + if (ret) > + return dev_err_probe(mux_dev, ret, > + "Failed to add i2c mux adapter %d\n", reg); > + } > + > + mux_deselect(muxc, MUX_DEFAULT_DEV); > + > + return 0; > +} > + > +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 struct platform_driver mule_i2c_mux_driver = { > + .driver = { > + .name = "mule-i2c-mux", > + .of_match_table = mule_i2c_mux_of_match, > + }, > + .probe = mule_i2c_mux_probe, > +}; > + > +module_platform_driver(mule_i2c_mux_driver); > + > +MODULE_AUTHOR("Farouk Bouabid <farouk.bouabid@cherry.de>"); > +MODULE_DESCRIPTION("I2C mux driver for Theobroma Systems Mule"); > +MODULE_LICENSE("GPL"); >
On 12/08/2024 11:24, Quentin Schulz wrote: > Hi all, > > Gentle ping. Let's call it real ping, because we have warning in next due to this: Documentation/devicetree/bindings/hwmon/ti,amc6821.example.dtb: /example-1/i2c/fan@18/i2c-mux: failed to match any schema with compatible: ['tsd,mule-i2c-mux'] Can anyone pick up the i2c pieces? And/or fix maintainers for your subsystem? Best regards, Krzysztof
> Let's call it real ping, because we have warning in next due to this: > > Documentation/devicetree/bindings/hwmon/ti,amc6821.example.dtb: /example-1/i2c/fan@18/i2c-mux: failed to match any schema with compatible: ['tsd,mule-i2c-mux'] So someone picked up patches before the dependencies were accepted? Bad idea. > Can anyone pick up the i2c pieces? They need review first. We likely have an issue with resource availabilty here. It is that simple. > And/or fix maintainers for your subsystem? Have you looked at the entries? They look proper to me.
On 12/08/2024 12:06, Wolfram Sang wrote: > >> Let's call it real ping, because we have warning in next due to this: >> >> Documentation/devicetree/bindings/hwmon/ti,amc6821.example.dtb: /example-1/i2c/fan@18/i2c-mux: failed to match any schema with compatible: ['tsd,mule-i2c-mux'] > > So someone picked up patches before the dependencies were accepted? Bad > idea. Yep, but to be fair the patchset did not say anything about dependencies. There is absolutely nothing in cover letter, nothing in the patches, so I do not wonder that this mishap happened. > >> Can anyone pick up the i2c pieces? > > They need review first. We likely have an issue with resource > availabilty here. It is that simple. > >> And/or fix maintainers for your subsystem? > > Have you looked at the entries? They look proper to me. Depends whether you rely on being CC-ed here. Existing entries do not include you, thus you are not cc-ed on maintainers. Peter Rosin is, but it seems Peter does not apply patches. It could be intentional, but then I understand that all pings should go to Peter? Best regards, Krzysztof
> Yep, but to be fair the patchset did not say anything about > dependencies. There is absolutely nothing in cover letter, nothing in > the patches, so I do not wonder that this mishap happened. Still, one shouldn't take DT patches (which are even the last ones in this series) until all other patches are at least in -next, or? Yes, mistakes happen, so no big deal, but i2c is not to blame IMHO. > Depends whether you rely on being CC-ed here. Existing entries do not I don't rely on CC. I rely on patches being on the i2c list. > include you, thus you are not cc-ed on maintainers. Peter Rosin is, but > it seems Peter does not apply patches. It could be intentional, but then > I understand that all pings should go to Peter? Once Peter acks, I apply. He is the maintainer. Yet, he is very busy, so I also apply when someone else I trust does a review. He is fine with that and might chime in later, if needed. This patch here did not get any review, sadly. As I said, resource problem. That being said, these patches are somewhere on my todo list if nobody else steps up (what I would prefer). But please, don't put pressure on me (or any other potential reviewer) just because DT patches ended up upstream too early.
On 12/08/2024 14:21, Wolfram Sang wrote: > >> Yep, but to be fair the patchset did not say anything about >> dependencies. There is absolutely nothing in cover letter, nothing in >> the patches, so I do not wonder that this mishap happened. > > Still, one shouldn't take DT patches (which are even the last ones in > this series) until all other patches are at least in -next, or? Yes, > mistakes happen, so no big deal, but i2c is not to blame IMHO. No, it's not. It was just a ping. The issue is here not describing dependency, allowing Guenter to take the patch and not even telling him that now next has warning. :/ It's like entire weight is on maintainers and contributors care only about getting their patch inside. Once it is inside, not my problem anymore... :( > >> Depends whether you rely on being CC-ed here. Existing entries do not > > I don't rely on CC. I rely on patches being on the i2c list. > >> include you, thus you are not cc-ed on maintainers. Peter Rosin is, but >> it seems Peter does not apply patches. It could be intentional, but then >> I understand that all pings should go to Peter? > > Once Peter acks, I apply. He is the maintainer. Yet, he is very busy, so > I also apply when someone else I trust does a review. He is fine with Sure, that explains, so ping should not really go to you... > that and might chime in later, if needed. This patch here did not get > any review, sadly. As I said, resource problem. That being said, these > patches are somewhere on my todo list if nobody else steps up (what I > would prefer). But please, don't put pressure on me (or any other > potential reviewer) just because DT patches ended up upstream too early. Best regards, Krzysztof
On 8/12/24 06:13, Krzysztof Kozlowski wrote: > On 12/08/2024 14:21, Wolfram Sang wrote: >> >>> Yep, but to be fair the patchset did not say anything about >>> dependencies. There is absolutely nothing in cover letter, nothing in >>> the patches, so I do not wonder that this mishap happened. >> >> Still, one shouldn't take DT patches (which are even the last ones in >> this series) until all other patches are at least in -next, or? Yes, >> mistakes happen, so no big deal, but i2c is not to blame IMHO. > > No, it's not. It was just a ping. The issue is here not describing > dependency, allowing Guenter to take the patch and not even telling him Oh, I knew that the i2c patches were not yet in the tree. I just didn't know that I must not apply patches in this situation (where the actual patches are perfectly fine but assume that something else completely elsewhere is applied). After all, the amc6821 patches do not actually trigger anything in i2c mux, they just trigger instantiation of nested devices. We live and learn. Patches now dropped from linux-next. I do wonder though if the rules for applying a sequence of patches with non-technical dependencies is documented somewhere. Thanks, Guenter
> No, it's not. It was just a ping. The issue is here not describing > dependency, allowing Guenter to take the patch and not even telling him > that now next has warning. :/ It's like entire weight is on maintainers > and contributors care only about getting their patch inside. Once it is > inside, not my problem anymore... :( I totally get the "weight on maintainers" thing. I just also see that for new contributors, it can be hard to see soft dependencies in advance. I mean, even we missed it. I guess Guenter is right, we all make mistakes, so we fix them. Commits got reverted and we will re-apply better.
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig index db1b9057612a..6d2f66810cdc 100644 --- a/drivers/i2c/muxes/Kconfig +++ b/drivers/i2c/muxes/Kconfig @@ -119,4 +119,20 @@ 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 "Theobroma Systems Mule I2C device multiplexer" + depends on OF && SENSORS_AMC6821 + help + Mule is an MCU that emulates a set of I2C devices, among which + devices that are reachable through an I2C-mux. The devices on the mux + can be selected by writing the appropriate device number to an I2C + configuration register. + + If you say yes to this option, support will be included for a + Theobroma Systems Mule I2C multiplexer. This driver provides access to + I2C devices connected on this 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..062596869651 --- /dev/null +++ b/drivers/i2c/muxes/i2c-mux-mule.c @@ -0,0 +1,155 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Theobroma Systems 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/platform_device.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 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 platform_device *pdev) +{ + struct device *mux_dev = &pdev->dev; + struct mule_i2c_reg_mux *priv; + struct i2c_client *client; + struct i2c_mux_core *muxc; + struct device_node *dev; + unsigned int readback; + int ndev, ret; + bool old_fw; + + /* Count devices on the mux */ + ndev = of_get_child_count(mux_dev->of_node); + dev_dbg(mux_dev, "%d devices on the mux\n", ndev); + + client = to_i2c_client(mux_dev->parent); + + muxc = i2c_mux_alloc(client->adapter, mux_dev, ndev, sizeof(*priv), + I2C_MUX_LOCKED, mux_select, mux_deselect); + if (!muxc) + return dev_err_probe(mux_dev, -ENOMEM, + "Failed to allocate mux struct\n"); + + priv = i2c_mux_priv(muxc); + + priv->regmap = dev_get_regmap(mux_dev->parent, NULL); + if (IS_ERR(priv->regmap)) + return dev_err_probe(mux_dev, PTR_ERR(priv->regmap), + "No parent i2c register map\n"); + + platform_set_drvdata(pdev, muxc); + + /* + * MUX_DEFAULT_DEV 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, MUX_DEFAULT_DEV); + if (ret) + return dev_err_probe(mux_dev, ret, + "Failed to write config register\n"); + + ret = regmap_read(priv->regmap, MUX_CONFIG_REG, &readback); + if (ret) + return dev_err_probe(mux_dev, ret, + "Failed to read config register\n"); + + old_fw = (readback != MUX_DEFAULT_DEV); + + ret = devm_add_action_or_reset(mux_dev, mux_remove, muxc); + if (ret) + return dev_err_probe(mux_dev, ret, + "Failed to register mux remove\n"); + + /* Create device adapters */ + for_each_child_of_node(mux_dev->of_node, dev) { + u32 reg; + + ret = of_property_read_u32(dev, "reg", ®); + if (ret) + return dev_err_probe(mux_dev, ret, + "No reg property found for %s\n", + of_node_full_name(dev)); + + if (old_fw && reg != 0) { + dev_warn(mux_dev, + "Mux is not supported, please update Mule FW\n"); + continue; + } + + ret = mux_select(muxc, reg); + if (ret) { + dev_warn(mux_dev, + "Device %d not supported, please update Mule FW\n", reg); + continue; + } + + ret = i2c_mux_add_adapter(muxc, 0, reg); + if (ret) + return dev_err_probe(mux_dev, ret, + "Failed to add i2c mux adapter %d\n", reg); + } + + mux_deselect(muxc, MUX_DEFAULT_DEV); + + return 0; +} + +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 struct platform_driver mule_i2c_mux_driver = { + .driver = { + .name = "mule-i2c-mux", + .of_match_table = mule_i2c_mux_of_match, + }, + .probe = mule_i2c_mux_probe, +}; + +module_platform_driver(mule_i2c_mux_driver); + +MODULE_AUTHOR("Farouk Bouabid <farouk.bouabid@cherry.de>"); +MODULE_DESCRIPTION("I2C mux driver for Theobroma Systems Mule"); +MODULE_LICENSE("GPL");
Theobroma Systems Mule is an MCU that emulates a set of I2C devices, among which an amc6821 and devices that are reachable through an I2C-mux. The devices on the mux can be selected by writing the appropriate device number to an I2C config register (amc6821 reg 0xff). This driver is expected to be probed as a platform device with amc6821 as its parent i2c device. Add support for the mule-i2c-mux platform driver. The amc6821 driver support for the mux will be added in a later commit. Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> --- drivers/i2c/muxes/Kconfig | 16 ++++ drivers/i2c/muxes/Makefile | 1 + drivers/i2c/muxes/i2c-mux-mule.c | 155 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 172 insertions(+)