| Message ID | 20240902-dev-mule-i2c-mux-v7-0-bf7b8f5385ed@cherry.de |
|---|---|
| Headers | show |
| Series | Add tsd,mule-i2c-mux support | expand |
Hi Farouk, Before jumping into the review, who is going to take this and the previous patch? Peter shall I take it? Now to the review :-) On Mon, Sep 02, 2024 at 06:38:15PM GMT, 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 Along the driver I expressed some concern about the prefixes. You should avoid prefixes such as mux_* or MUX_* because they don't belong to your driver. You should always use your driver's name: 1. mule_* 2. mule_mux_* 3. mule_i2c_mux_* You have used the 3rd, I'd rather prefer the 1st. Because when you are in i2c/muxex/ it's implied that you are an i2c mux device. But it's a matter of personal taste. Other than this, there is still, one major error down below. > support for the mux will be added in a later commit. > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> ... > +#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 Please define these as MULE_I2C_MUX_* > + > +struct mule_i2c_reg_mux { > + struct regmap *regmap; > +}; > + > +static int mux_select(struct i2c_mux_core *muxc, u32 dev) > +{ > + struct mule_i2c_reg_mux *mux = muxc->priv; > + > + return regmap_write(mux->regmap, MUX_CONFIG_REG, 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) Please call these mule_i2c_mux_*(), the mux_ prefix doesn't belong to this driver. > +{ > + struct i2c_mux_core *muxc = data; > + > + i2c_mux_del_adapters(muxc); > + > + mux_deselect(muxc, MUX_DEFAULT_DEV); > +} ... > + /* 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 ret; do we need to delete the adapters we added in previous cycles? > + } > + > + mux_deselect(muxc, MUX_DEFAULT_DEV); > + > + return 0; > +} > + > +static const struct of_device_id mule_i2c_mux_of_match[] = { > + {.compatible = "tsd,mule-i2c-mux",}, if you are going to resend, can you leave one space after the '{' and before the '}' > + {}, > +}; > +MODULE_DEVICE_TABLE(of, mule_i2c_mux_of_match); > + > +static struct platform_driver mule_i2c_mux_driver = { > + .driver = { I don't see the need for this '\t' here, the alignment is too far. It just looks bad. Your choice, though. Thanks, Andi > + .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"); > > -- > 2.34.1 >
Hi! 2024-09-03 at 17:13, Andi Shyti wrote: > Hi Farouk, > > Before jumping into the review, who is going to take this and the > previous patch? > > Peter shall I take it? Please do. Thanks, and as always, sorry for my low bandwidth... > > Now to the review :-) > > On Mon, Sep 02, 2024 at 06:38:15PM GMT, 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 > > Along the driver I expressed some concern about the prefixes. > > You should avoid prefixes such as mux_* or MUX_* because they > don't belong to your driver. You should always use your driver's > name: > > 1. mule_* > 2. mule_mux_* > 3. mule_i2c_mux_* > > You have used the 3rd, I'd rather prefer the 1st. Because when > you are in i2c/muxex/ it's implied that you are an i2c mux > device. But it's a matter of personal taste. > > Other than this, there is still, one major error down below. > >> support for the mux will be added in a later commit. >> >> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> >> Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> > > ... > >> +#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 > > Please define these as MULE_I2C_MUX_* > >> + >> +struct mule_i2c_reg_mux { >> + struct regmap *regmap; >> +}; >> + >> +static int mux_select(struct i2c_mux_core *muxc, u32 dev) >> +{ >> + struct mule_i2c_reg_mux *mux = muxc->priv; >> + >> + return regmap_write(mux->regmap, MUX_CONFIG_REG, 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) > > Please call these mule_i2c_mux_*(), the mux_ prefix doesn't > belong to this driver. > >> +{ >> + struct i2c_mux_core *muxc = data; >> + >> + i2c_mux_del_adapters(muxc); >> + >> + mux_deselect(muxc, MUX_DEFAULT_DEV); >> +} > > ... > >> + /* 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 ret; > > do we need to delete the adapters we added in previous cycles? Yes, nicely spotted. A call to i2c_mux_del_adapters(muxc) is needed on failure if any i2c_mux_add_adapter() call has succeeded (but it's safe to call it on any i2c_mux_add_adapter() failure). Cheers, Peter > >> + } >> + >> + mux_deselect(muxc, MUX_DEFAULT_DEV); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id mule_i2c_mux_of_match[] = { >> + {.compatible = "tsd,mule-i2c-mux",}, > > if you are going to resend, can you leave one space after the > '{' and before the '}' > >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, mule_i2c_mux_of_match); >> + >> +static struct platform_driver mule_i2c_mux_driver = { >> + .driver = { > > I don't see the need for this '\t' here, the alignment is too > far. It just looks bad. Your choice, though. > > Thanks, > Andi > >> + .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"); >> >> -- >> 2.34.1 >>
Hi Andi, On 03.09.24 17:13, Andi Shyti wrote: [...] >> + /* 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 ret; > do we need to delete the adapters we added in previous cycles? > We calldevm_action_or_reset() before the loop to add adapter-removal to the error path. I think that does the job for us or am I missing something ? Thanks, Farouk
Hi! 2024-09-04 at 10:35, Farouk Bouabid wrote: > Hi Andi, > > On 03.09.24 17:13, Andi Shyti wrote: > > [...] > >>> + ret = i2c_mux_add_adapter(muxc, 0, reg); >>> + if (ret) >>> + return ret; >> do we need to delete the adapters we added in previous cycles? >> > > We calldevm_action_or_reset() before the loop to add adapter-removal to the error path. I think that does the job > > for us or am I missing something ? I missed that too, but it LGTM. It's safe to call i2c_mux_del_adapters() as soon the mux core has been allocated, so there is no risk it is called too early or something. With that said, I agree with Andi on the naming and the nitpicks. Cheers, Peter
Hi Andi, On 03.09.24 17:13, Andi Shyti 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 > Along the driver I expressed some concern about the prefixes. > > You should avoid prefixes such as mux_* or MUX_* because they > don't belong to your driver. You should always use your driver's > name: > > 1. mule_* > 2. mule_mux_* > 3. mule_i2c_mux_* > > You have used the 3rd, I'd rather prefer the 1st. Because when > you are in i2c/muxex/ it's implied that you are an i2c mux > device. But it's a matter of personal taste. > Are you here referring to the commit log, module name or function prefixes ? (because later you suggested that we use "mule_i2c_mux_*" for functions) "Mule" is a chip that requires multiple drivers that will be added later on, and I suppose we don't want conflict with other module names ? Thanks, Farouk
Hi, On Wed, Sep 04, 2024 at 10:59:47AM GMT, Peter Rosin wrote: > 2024-09-04 at 10:35, Farouk Bouabid wrote: > > On 03.09.24 17:13, Andi Shyti wrote: > > > > [...] > > > >>> + ret = i2c_mux_add_adapter(muxc, 0, reg); > >>> + if (ret) > >>> + return ret; > >> do we need to delete the adapters we added in previous cycles? > >> > > > > We calldevm_action_or_reset() before the loop to add adapter-removal to the error path. I think that does the job > > > > for us or am I missing something ? > > I missed that too, but it LGTM. It's safe to call i2c_mux_del_adapters() as > soon the mux core has been allocated, so there is no risk it is called too > early or something. Just a question, still: is it the same calling i2c_mux_add_adapter() and calling mux_remove()? Thanks, Andi > With that said, I agree with Andi on the naming and the > nitpicks. > > Cheers, > Peter
Hi Farouk, On Wed, Sep 04, 2024 at 12:23:56PM GMT, Farouk Bouabid wrote: > On 03.09.24 17:13, Andi Shyti 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 > > Along the driver I expressed some concern about the prefixes. > > > > You should avoid prefixes such as mux_* or MUX_* because they > > don't belong to your driver. You should always use your driver's > > name: > > > > 1. mule_* > > 2. mule_mux_* > > 3. mule_i2c_mux_* > > > > You have used the 3rd, I'd rather prefer the 1st. Because when > > you are in i2c/muxex/ it's implied that you are an i2c mux > > device. But it's a matter of personal taste. > > > > Are you here referring to the commit log, module name or function prefixes ? > (because later you suggested that we use "mule_i2c_mux_*" for functions) I made a general comment that applies to all the functions, defines, and global variables you've made here. My suggestion to use mule_i2c_mux_* is based on the fact that it's the most commonly used prefix in your code, but you don't necessarily need to use it. That's why I listed a few options. > "Mule" is a chip that requires multiple drivers that will be added later on, > and I suppose we don't want conflict with other module names ? It's an unwritten rule that you should avoid using overly generic terms as prefixes in your driver, like "smbus_read()" or "i2c_read()". Instead, you should incorporate to the prefix the chip identifier as named by the vendor. If this device is called 'Theobroma Systems Mule,' you should stick to that naming as much as possible. Using the correct prefix might seem like overkill, but it's essential for debugging, grepping, and avoiding conflicts in cases where other developers haven’t used unique identifiers in their modules. Lastly, if you're working within the i2c/muxes directory, you can omit the 'mux' prefix. It’s already clear that you're working with an I2C mux device. Thanks, Andi
Hi Andi, On 05.09.24 22:20, Andi Shyti wrote: > Hi, > > On Wed, Sep 04, 2024 at 10:59:47AM GMT, Peter Rosin wrote: >> 2024-09-04 at 10:35, Farouk Bouabid wrote: >>> On 03.09.24 17:13, Andi Shyti wrote: >>> >>> [...] >>> >>>>> + ret = i2c_mux_add_adapter(muxc, 0, reg); >>>>> + if (ret) >>>>> + return ret; >>>> do we need to delete the adapters we added in previous cycles? >>>> >>> We calldevm_action_or_reset() before the loop to add adapter-removal to the error path. I think that does the job >>> >>> for us or am I missing something ? >> I missed that too, but it LGTM. It's safe to call i2c_mux_del_adapters() as >> soon the mux core has been allocated, so there is no risk it is called too >> early or something. > Just a question, still: is it the same calling > i2c_mux_add_adapter() and calling mux_remove()? > We are basically wrapping i2c_mux_adapters_del() by mux_remove() (implemented in our driver) which is registered it as devm_add_action_or_reset and would be called for a non-0 return of the probe. i2c_mux_adapters_del() will then remove added adapters (do nothing if none is added). Thanks, Farouk
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. Please merge patch 1 before patch 3 Please merge patches 2, 3 and 4 (and 1) before patches 5, 6, 7, 8 Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de> Changes in v7: - Merged __mux_select and mux_select functions in patch 2 - Removed unnecessary prints after mux_alloc and add_adapter calls in patch 2 - Added Guenter's Reviewed-by tag - Added Rob's Reviewed-by tag - Added Wolfram's Reviewed/Acked-by tags - Link to v6: https://lore.kernel.org/r/20240725-dev-mule-i2c-mux-v6-0-f9f6d7b60fb2@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 | 148 +++++++++++++++++++++ 11 files changed, 404 insertions(+), 26 deletions(-) --- base-commit: 67784a74e258a467225f0e68335df77acd67b7ab change-id: 20240404-dev-mule-i2c-mux-9103cde07021 Best regards,