| Message ID | 20240725-dev-mule-i2c-mux-v6-4-f9f6d7b60fb2@cherry.de (mailing list archive) |
|---|---|
| State | Accepted |
| 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
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
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 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
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c index ec94392fcb65..a3fdbcf01ecd 100644 --- a/drivers/hwmon/amc6821.c +++ b/drivers/hwmon/amc6821.c @@ -22,6 +22,7 @@ #include <linux/minmax.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of_platform.h> #include <linux/regmap.h> #include <linux/slab.h> @@ -897,7 +898,6 @@ static bool amc6821_volatile_reg(struct device *dev, unsigned int reg) static const struct regmap_config amc6821_regmap_config = { .reg_bits = 8, .val_bits = 8, - .max_register = AMC6821_REG_CONF3, .volatile_reg = amc6821_volatile_reg, .cache_type = REGCACHE_MAPLE, }; @@ -924,6 +924,13 @@ static int amc6821_probe(struct i2c_client *client) if (err) return err; + if (of_device_is_compatible(dev->of_node, "tsd,mule")) { + err = devm_of_platform_populate(dev); + if (err) + return dev_err_probe(dev, err, + "Failed to create sub-devices\n"); + } + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, &amc6821_chip_info, amc6821_groups); @@ -941,6 +948,9 @@ static const struct of_device_id __maybe_unused amc6821_of_match[] = { { .compatible = "ti,amc6821", }, + { + .compatible = "tsd,mule", + }, { } };
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> --- drivers/hwmon/amc6821.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)