| Message ID | 20240902-dev-mule-i2c-mux-v7-2-bf7b8f5385ed@cherry.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
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 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 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
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..e4e8992d4a09 --- /dev/null +++ b/drivers/i2c/muxes/i2c-mux-mule.c @@ -0,0 +1,148 @@ +// 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 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) +{ + 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 -ENOMEM; + + 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 ret; + } + + 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");