| Message ID | 20240725-dev-mule-i2c-mux-v6-0-f9f6d7b60fb2@cherry.de |
|---|---|
| Headers | show |
| Series | Add tsd,mule-i2c-mux support | expand |
On 7/25/24 06:27, Farouk Bouabid wrote: > Theobroma Systems Mule is an MCU that emulates a set of I2C devices, > among which is an amc6821 and other 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) > > Implement "tsd,mule" compatible to instantiate the I2C-mux platform device > when probing the amc6821. > > Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> lgtm For my reference: Reviewed-by: Guenter Roeck <linux@roeck-us.net>
On Thu, Jul 25, 2024 at 03:27:50PM +0200, Farouk Bouabid wrote: > Theobroma Systems Mule is an MCU that emulates a set of I2C devices, > among which is an amc6821 and other 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) > > Implement "tsd,mule" compatible to instantiate the I2C-mux platform device > when probing the amc6821. > > Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> Applied. Thanks, Guenter
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
On 31/07/2024 17:12, Guenter Roeck wrote: > On Thu, Jul 25, 2024 at 03:27:50PM +0200, Farouk Bouabid wrote: >> Theobroma Systems Mule is an MCU that emulates a set of I2C devices, >> among which is an amc6821 and other 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) >> >> Implement "tsd,mule" compatible to instantiate the I2C-mux platform device >> when probing the amc6821. >> >> Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> >> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > Applied. Eh, there is undocumented dependency on I2C here. Next has warning because of this. Farouk, please *always mention* the dependencies between patches. Best regards, Krzysztof
Hi Krzysztof, On 8/12/24 1:38 PM, Krzysztof Kozlowski wrote: > [Some people who received this message don't often get email from krzk@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > On 31/07/2024 17:12, Guenter Roeck wrote: >> On Thu, Jul 25, 2024 at 03:27:50PM +0200, Farouk Bouabid wrote: >>> Theobroma Systems Mule is an MCU that emulates a set of I2C devices, >>> among which is an amc6821 and other 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) >>> >>> Implement "tsd,mule" compatible to instantiate the I2C-mux platform device >>> when probing the amc6821. >>> >>> Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> >>> Reviewed-by: Guenter Roeck <linux@roeck-us.net> >> >> Applied. > > Eh, there is undocumented dependency on I2C here. Next has warning > because of this. > I think you meant to comment this on https://lore.kernel.org/linux-i2c/20240725-dev-mule-i2c-mux-v6-0-f9f6d7b60fb2@cherry.de/T/#mdb7976f1dc16fce0b7db9abee6fd0b1fd0a2e2ba (patch 3 and not 4 of the series). This patch (4) is fine on its own I believe, no dependency on anything else. (well, except if we expect bindings to be absolutely merged before the drivers? I think what matters is the Device Tree changes making use of the new binding be merged after dt-binding changes?). I agree that there's a somewhat non-obvious dependency between patch 1 and 3 (the dt-bindings) and 5-8 with everything before, we could have made this more explicit. > Farouk, please *always mention* the dependencies between patches. > I wasn't aware of that rule, my apologies for not catching this before upstream submission. For anyone wondering the rule is made explicit here: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes "If one patch depends on another patch in order for a change to be complete, that is OK. Simply note “this patch depends on patch X” in your patch description." Question about b4 workflow though. I encourage using b4 to avoid as many mistakes as possible and make the workflow as painless as possible. I believe b4 doesn't allow you to have per-patch notes, only in the cover-letter. a) is this dependency list in cover-letter acceptable, or b) need to add it to the patch note (below the ---), or c) can add it to the patch commit log I've seen subsystem keep vX changelogs in commit logs, and some who do not want it, so maybe there's no one rule here? Cheers, Quentin
> 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 13:58, Quentin Schulz wrote: > Hi Krzysztof, > > On 8/12/24 1:38 PM, Krzysztof Kozlowski wrote: >> [Some people who received this message don't often get email from krzk@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] >> >> On 31/07/2024 17:12, Guenter Roeck wrote: >>> On Thu, Jul 25, 2024 at 03:27:50PM +0200, Farouk Bouabid wrote: >>>> Theobroma Systems Mule is an MCU that emulates a set of I2C devices, >>>> among which is an amc6821 and other 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) >>>> >>>> Implement "tsd,mule" compatible to instantiate the I2C-mux platform device >>>> when probing the amc6821. >>>> >>>> Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> >>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net> >>> >>> Applied. >> >> Eh, there is undocumented dependency on I2C here. Next has warning >> because of this. >> > > I think you meant to comment this on > https://lore.kernel.org/linux-i2c/20240725-dev-mule-i2c-mux-v6-0-f9f6d7b60fb2@cherry.de/T/#mdb7976f1dc16fce0b7db9abee6fd0b1fd0a2e2ba > (patch 3 and not 4 of the series). This patch (4) is fine on its own I > believe, no dependency on anything else. (well, except if we expect > bindings to be absolutely merged before the drivers? I think what > matters is the Device Tree changes making use of the new binding be > merged after dt-binding changes?). Yeah, this was about DT binding. > > I agree that there's a somewhat non-obvious dependency between patch 1 > and 3 (the dt-bindings) and 5-8 with everything before, we could have > made this more explicit. > >> Farouk, please *always mention* the dependencies between patches. >> > > I wasn't aware of that rule, my apologies for not catching this before > upstream submission. > > For anyone wondering the rule is made explicit here: > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes > > "If one patch depends on another patch in order for a change to be > complete, that is OK. Simply note “this patch depends on patch X” in > your patch description." > > Question about b4 workflow though. I encourage using b4 to avoid as many > mistakes as possible and make the workflow as painless as possible. I > believe b4 doesn't allow you to have per-patch notes, only in the > cover-letter. "Patch description" or "per patch notes" is whatever you write in changelog, so under ---. > a) is this dependency list in cover-letter acceptable, or > b) need to add it to the patch note (below the ---), or One of above should be enough, both are more welcomed because many maintainers ignore completely cover letters. > c) can add it to the patch commit log No, if patches go through separate trees then it would be just confusing and not helping at all. Best regards, Krzysztof
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 04:38, Krzysztof Kozlowski wrote: > On 31/07/2024 17:12, Guenter Roeck wrote: >> On Thu, Jul 25, 2024 at 03:27:50PM +0200, Farouk Bouabid wrote: >>> Theobroma Systems Mule is an MCU that emulates a set of I2C devices, >>> among which is an amc6821 and other 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) >>> >>> Implement "tsd,mule" compatible to instantiate the I2C-mux platform device >>> when probing the amc6821. >>> >>> Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> >>> Reviewed-by: Guenter Roeck <linux@roeck-us.net> >> >> Applied. > > Eh, there is undocumented dependency on I2C here. Next has warning > because of this. > > Farouk, please *always mention* the dependencies between patches. > > Best regards, > Krzysztof > > Sorry, I wasn't aware that all bindings have to be in the tree before I apply patches, and I somehow had the apparently wrong impression that the bindings were approved. I'll drop the two patches (this one and the DT patch for amc6821). Someone may need to remind me to re-apply them after all pre-dependencies are in the tree. Guenter
On 12/08/2024 15:21, Guenter Roeck wrote: > On 8/12/24 04:38, Krzysztof Kozlowski wrote: >> On 31/07/2024 17:12, Guenter Roeck wrote: >>> On Thu, Jul 25, 2024 at 03:27:50PM +0200, Farouk Bouabid wrote: >>>> Theobroma Systems Mule is an MCU that emulates a set of I2C devices, >>>> among which is an amc6821 and other 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) >>>> >>>> Implement "tsd,mule" compatible to instantiate the I2C-mux platform device >>>> when probing the amc6821. >>>> >>>> Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> >>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net> >>> >>> Applied. >> >> Eh, there is undocumented dependency on I2C here. Next has warning >> because of this. >> >> Farouk, please *always mention* the dependencies between patches. >> >> Best regards, >> Krzysztof >> >> > Sorry, I wasn't aware that all bindings have to be in the tree before I apply > patches, and I somehow had the apparently wrong impression that the bindings None of us were aware of it and bindings were in fact approved, so I would do the same as you - picked up patches. > were approved. I'll drop the two patches (this one and the DT patch for > amc6821). Someone may need to remind me to re-apply them after all > pre-dependencies are in the tree. Thanks, that would solve the issue. 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.
Hi, On Thu, Jul 25, 2024 at 03:27:48PM +0200, 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. Seems like DT maintainers are happy with the approach. From the I2C perspective, this seems suitable as well. Just a few minor comments. Can be fixed incrementally, from my POV. But basically: Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > +static inline int __mux_select(struct regmap *regmap, u32 dev) > +{ > + return regmap_write(regmap, MUX_CONFIG_REG, dev); > +} Does this really need to be a seperate function? I'd vote for merging it into 'mux_select'. Also the __-prefix often means unlocked versions of some call, so it is also a bit misleading. > +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"); alloc_functions usually print something when failing. > + 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); The 'add_adapter' functions for sure print something when failing. Thanks! Wolfram
Theobroma Systems Mule is an MCU that emulates a set of I2C devices which 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 (0xff) that is not used by amc6821 logic. This required us to add a new compatible to the amc6821 driver, from which, the new platform device "tsd,mule-i2c-mux" is probed. The selected device on the mux can be accessed for reading and writing at I2C address 0x6f. +--------+----------------+------------------------------+ | Mule | 0x18 | +------------------+ | --------+----->| amc6821 | | | | +------------------+ | | +----->| tsd,mule-i2c-mux |---+ | | +------------------+ | | | V__ +---------+ | | | \-------->| isl1208 | | | | | +---------+ | 0x6f | | M |-------->| dev #1 | | ------------------------------------>| U | +---------+ | | | X |-------->| dev #2 | | | | | +---------+ | | | /-------->| dev #3 | | | |__/ +---------+ | +--------------------------------------------------------+ This patch-series adds support for the tsd,mule-i2c multiplexer as part of rk3399-puma, px30-ringneck, rk3588-tiger and rk3588-jaguar boards. Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> Changes in v6: - Move ti,amc6821 from trivial-devices into its own dt-bindings - Use same regmap config structure for both tsd,mule and ti,amc6821 - Remove max_register from regmap config structure for amc6821 - Link to v5: https://lore.kernel.org/r/20240708-dev-mule-i2c-mux-v5-0-71446d3f0b8d@cherry.de Changes in v5: - Drop the mfd implementation of v4 - Add more dev_probe_err callbacks to tsd,mule-i2c-mux - Instantiate tsd,mule-i2c-mux as a platform device from amc6821 driver - add "Theobroma Systems" when describing mule. - Link to v4: https://lore.kernel.org/lkml/20240618-dev-mule-i2c-mux-v4-0-5462d28354c8@cherry.de/ Changes in v4: - Drop the previously added i2c adapter quirks - Add platform driver probe to amc6821. - Change mule-i2c-mux driver to a platform driver - Add dev_probe_err in mule-i2c-mux driver - Add support for tsd,mule in simple-mfd-i2c - Add tsd,mule mfd to supported dts - Link to v3: https://lore.kernel.org/r/20240611-dev-mule-i2c-mux-v3-0-08d26a28e001@cherry.de Changes in v3: - Change "i2c" in comments/commit-logs to "I2C" - Fix long line-length - Warn when "share_addr_with_children" is set and the Mux is not an I2C device - Fix/stop propagating "I2C_AQ_SKIP_ADDR_CHECK" flag if "share_addr_with_children" is not set. - Fix "old_fw" variable is used to indicate the reversed meaning. - Link to v2: https://lore.kernel.org/r/20240506-dev-mule-i2c-mux-v2-0-a91c954f65d7@cherry.de Changes in v2: - Add i2c-adapter quirks to skip checking for conflict between the mux core and a child device address. - Rename dt-binding to "tsd,mule-i2c-mux.yaml" - Add Mule description to kconfig - Fix indentation - Move device table after probe - Link to v1: https://lore.kernel.org/r/20240426-dev-mule-i2c-mux-v1-0-045a482f6ffb@theobroma-systems.com --- Farouk Bouabid (8): dt-bindings: i2c: add support for tsd,mule-i2c-mux i2c: muxes: add support for tsd,mule-i2c multiplexer dt-bindings: hwmon: add support for ti,amc6821 hwmon: (amc6821) add support for tsd,mule arm64: dts: rockchip: add tsd,mule-i2c-mux on rk3588-jaguar arm64: dts: rockchip: add tsd,mule-i2c-mux on rk3399-puma arm64: dts: rockchip: add tsd,mule-i2c-mux on rk3588-tiger arm64: dts: rockchip: add tsd,mule-i2c-mux on px30-ringneck .../devicetree/bindings/hwmon/ti,amc6821.yaml | 86 ++++++++++++ .../devicetree/bindings/i2c/tsd,mule-i2c-mux.yaml | 69 +++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi | 24 +++- arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi | 24 +++- arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts | 25 +++- arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi | 23 ++- drivers/hwmon/amc6821.c | 12 +- drivers/i2c/muxes/Kconfig | 16 +++ drivers/i2c/muxes/Makefile | 1 + drivers/i2c/muxes/i2c-mux-mule.c | 155 +++++++++++++++++++++ 11 files changed, 411 insertions(+), 26 deletions(-) --- base-commit: 5d09c33f705191f1be376ecd1bc41fd32798370c change-id: 20240404-dev-mule-i2c-mux-9103cde07021 Best regards,