* Re: [PATCH v6 01/24] i2c-mux: add common data for every i2c-mux instance @ 2016-04-15 15:52 Peter Rosin 0 siblings, 0 replies; 5+ messages in thread From: Peter Rosin @ 2016-04-15 15:52 UTC (permalink / raw) To: Wolfram Sang, Peter Rosin Cc: linux-kernel, Jonathan Corbet, Peter Korsgaard, Guenter Roeck, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Antti Palosaari, Mauro Carvalho Chehab, Rob Herring, Frank Rowand, Grant Likely, Andrew Morton, Greg Kroah-Hartman, David S. Miller, Kalle Valo, Joe Perches, Jiri Slaby, Daniel Baluta, Adriana Reus, Lucas De Marchi, Matt Ranostay, Krzysztof Kozlowski, Terry Heo, Hans Verkuil, Arnd Bergmann, Tommi Rantala, linux-i2c, linux-doc, linux-iio, linux-media, devicetree Wolfram Sang wrote: > > > wonder even more if we couldn't supply num_adapters to i2c_mux_alloc() > > > and reserve the memory statically. i2c busses are not > > > dynamic/hot-pluggable so that should be good enough? > > > > Yes, that would work, but it would take some restructuring in some of > > the drivers that currently don't know how many child adapters they > > are going to need when they call i2c_mux_alloc. > > Which ones? If you look at i2c-mux-reg.c, it currently allocates its private struct regmux, then fills it with various platform things and then when it knows how many children it needs it allocates them. After v6 it first allocates a mux core and private struct regmux in one go using i2c_mux_alloc, then continues in much the same way as before. If the number of children is needed for the i2c_mux_alloc call, then this is certainly doable, and it would probably not be all that bad, but the simplest approach would probably be to allocate the private struct regmux first, then dig through the platform data, then allocate the mux core when the number of children is known. Which would still be two allocations separated by the platform data dig. So, your suggestion would basically move the mux core allocation from generally being done early together with other private data to later when the driver has figured out how many children it's going to create. The restructuring I thought about is needed if the intention of this was to reduce number of allocations, but maybe you just wanted what I described above? Because what I did in v6 and what you are suggesting is quite similar in complexity, but your version has the advantage of not having the need for realloc. So, I have made this change locally (and the adapters->num_adapters change) and I like it. I haven't even compile-tested it yet though, but I'll get back when I have done some testing. > > Because you thought about removing i2c_mux_reserve_adapters completely, > > and not provide any means of adding more adapters than specified in > > the i2c_mux_alloc call, right? > > Yes. I assumed I2C to be static enough that such information is known in > advance. > > > > Ignoring the 80 char limit here makes the code more readable. > > > > That is only true if you actually have more than 80 characters, so I don't > > agree. Are you adamant about it? (I'm not) > > No. Keep it if you prefer it. > > > >> +EXPORT_SYMBOL_GPL(i2c_mux_one_adapter); > > > > > > Are you sure the above function pays off? Its argument list is very > > > complex and it doesn't save a lot of code. Having seperate calls is > > > probably more understandable in drivers? Then again, I assume it makes > > > the conversion of existing drivers easier. > > > > I added it in v4, you can check earlier versions if you like. Without > > it most gate-muxes (i.e. typically the muxes in drivers/media) grew > > since the i2c_add_mux_adapter call got replaced by two calls, i.e. > > i2c_mux_alloc followed by i2c_max_add_adapter, and coupled with > > error checks made it look more complex than before. So, this wasn't > > much of a cleanup from the point of those drivers. > > Hmm, v3 didn't have the driver patches posted with it. Can you push it > to your branch? I am also not too strong with this one, but having a > look how it looks without would be nice. Although I'm not sure what you meant by "driver patches", I have pushed mux-core-and-locking-2 and mux-core-and-locking-3 to https://github.com/peda-r/i2c-mux/ (note that these are the branches as they where when I posted v2 and v3 to the list, i.e. w/o fixups) Those early versions updated all drivers with each change, making each patch big, so if that was what you meant by missing "driver patches" then there simply were no driver patches. If you meant the follow-up patches to relax locking in the media drivers etc, I only compile-tested them using throwaway branches back then (if I even had branches). So, I don't have anything ready to push, sorry. Cheers, Peter ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v6 00/24] i2c mux cleanup and locking update @ 2016-04-03 8:52 Peter Rosin 2016-04-03 8:52 ` [PATCH v6 01/24] i2c-mux: add common data for every i2c-mux instance Peter Rosin 0 siblings, 1 reply; 5+ messages in thread From: Peter Rosin @ 2016-04-03 8:52 UTC (permalink / raw) To: linux-kernel Cc: Peter Rosin, Wolfram Sang, Jonathan Corbet, Peter Korsgaard, Guenter Roeck, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Antti Palosaari, Mauro Carvalho Chehab, Rob Herring, Frank Rowand, Grant Likely, Andrew Morton, Greg Kroah-Hartman, David S. Miller, Kalle Valo, Joe Perches, Jiri Slaby, Daniel Baluta, Adriana Reus, Lucas De Marchi, Matt Ranostay, Krzysztof Kozlowski, Terry Heo, Hans Verkuil, Arnd Bergmann, Tommi Rantala, linux-i2c, linux-doc, linux-iio, linux-media, devicetree, Peter Rosin From: Peter Rosin <peda@axentia.se> Hi! I have a pair of boards with this i2c topology: GPIO ---| ------ BAT1 | v / I2C -----+------B---+---- MUX | \ EEPROM ------ BAT2 (B denotes the boundary between the boards) The problem with this is that the GPIO controller sits on the same i2c bus that it MUXes. For pca954x devices this is worked around by using unlocked transfers when updating the MUX. I have no such luck as the GPIO is a general purpose IO expander and the MUX is just a random bidirectional MUX, unaware of the fact that it is muxing an i2c bus. Extending unlocked transfers into the GPIO subsystem is too ugly to even think about. But the general hw approach is sane in my opinion, with the number of connections between the two boards minimized. To put it plainly, I need support for it. So, I observe that while it is needed to have the i2c bus locked during the actual MUX update in order to avoid random garbage on the slave side, it is not strictly a must to have it locked over the whole sequence of a full select-transfer-deselect operation. The MUX itself needs to be locked, so transfers to clients behind the mux are serialized, and the MUX needs to be stable during all i2c traffic (otherwise individual mux slave segments might see garbage). This series accomplishes this by adding code to i2c-mux-gpio and i2c-mux-pinctrl that determines if all involved devices used to update the mux are controlled by the same root i2c adapter that is muxed. When this is the case, the select-transfer-deselect operations should be locked individually to avoid the deadlock. The i2c bus *is* still locked during muxing, since the muxing happens as part of i2c transfers. This is true even if the MUX is updated with several transfers to the GPIO (at least as long as *all* MUX changes are using the i2c master bus). A lock is added to i2c adapters that muxes on that adapter grab, so that transfers through the muxes are serialized. Concerns: - The locking is perhaps too complex? - I worry about the priority inheritance aspect of the adapter lock. When the transfers behind the mux are divided into select-transfer-deselect all locked individually, low priority transfers get more chances to interfere with high priority transfers. - When doing an i2c_transfer() in_atomic() context or with irqs_disabled(), there is a higher possibility that the mux is not returned to its idle state after a failed (-EAGAIN) transfer due to trylock. - Is the detection of i2c-controlled gpios and pinctrls sane (i.e. the usage of the new i2c_root_adapter() function in 18/24)? To summarize the series, there's some i2c-mux infrastructure cleanup work first (I think that part stands by itself as desireable regardless), the locking changes are in 16/24 and after with the real meat in 18/24. There is some documentation added in 19/24 while 20/24 and after are cleanups to existing drivers utilizing the new stuff. PS. needs a bunch of testing, I do not have access to all the involved hw. Specifically, thank you Antti for testing v5, but I did not add any Tested-by for v6 since I moved the lock from the mux itself to the mux parent adapter. I did this to cope with the situation I described in http://marc.info/?l=linux-i2c&m=145875234525803&w=2 thus making it possible to get rid of the unlocked accesses in the si2168 driver (patch 21/24). I also didn't add any Reviewed-by for the parts of the rtl2832 driver changes that suffered the most from driver updates since v4.5-rc7, so please review that again as well. This series can also be pulled from github, if that is preferred: --------------------- The following changes since commit f55532a0c0b8bb6148f4e07853b876ef73bc69ca: Linux 4.6-rc1 (2016-03-26 16:03:24 -0700) are available in the git repository at: https://github.com/peda-r/i2c-mux.git mux-core-and-locking-6 for you to fetch changes up to 81830e43de2bc849848b939166103217ac444df5: [media] rtl2832: regmap is aware of lockdep, drop local locking hack (2016-04-03 09:35:52 +0200) --------------------- v6 compared to v5: - Rebase on top of v4.6-rc1 - Adjust to gpio subsystem overhaul. - Adjust to changes in the inv_mpu6050 driver. - Adjust to changes in the rtl2832 driver. - Fix some new trivial checkpatch issues. - Rename "self-locked" muxes "mux-locked" instead, since the lock has been moved to the parent adapter and is common for all muxes with the same parent adapter. The advantage is that address collisions behind sibling muxes are handled. Parent-locked muxes also grab this new mux-lock so that parent-locked and mux-locked siblings interact better. - Firmware mutex added to the si2168 driver. v5 compared to v4 (only published as a git branch): - Rebase on top of v4.5-rc7. - A new patch making me maintainer of i2c muxes (also sent separately). - A new file Documentation/i2c/i2c-topology that describes various muxing issues. - Rename "i2c-controlled" muxes "self-locked" instead, as it is perfectly reasonable to have i2c-controlled muxes that use the pre-existing locking scheme. The pre-existing locking scheme for i2c muxes is from here on called "parent-locked". - Rename i2c-mux.c:i2c_mux_master_xfer to __i2c_mux_master_xfer since it calls __i2c_transfer, which leaves room for a new i2c_mux_master_xfer that calls i2c_transfer. Similar rename shuffle for i2c_mux_smbus_xfer. - Use sizeof(*priv) instead of sizeof(struct i2c_mux_priv). One instance. - Some follow-up patches that were posted in response to v2-v4 cleaning up and simplifying various i2c muxes outside drivers/i2c/, among those is an unrelated cleanup patch to drivers/media/dvb-frontends/rtl2832.c that I carry here since it conflicts (trivially) with this series. That unrelated patch is (currently) the last patch in the series. v4 compared to v3: - Rebase on top of v4.5-rc6. - Update to add new i2c-mux interfaces in 01/18 including glue to implement the old interfaces in terms of the new interfaces, then change the mux users over to the new interfaces one by one (in 02/18 through 14/18), and finally removing the old interfaces in 15/18. I.e. the first 15 patches of v4 replaces the first 5 patches of v3, with the following points describing changes in the end result. Each patch is now touching only one subsystem. - Rename i2c_add_mux_adapter and i2c_del_mux_adapters to i2c_mux_add_adapter and i2c_mux_del_adapters (so that the old functions can live on during the transition). - Make i2c_mux_alloc take a parent and the select/deselect ops as arguments. Also add a flags argument to prevent churn later on. - Add a new interface i2c_mux_one_adapter(). Make use of it in suitable mux users with a single child adapter. - Adjust to a rename in struct gpio_chip. - Update a couple of comments to match the new code. v3 compared to v2: - Fix devm_kfree of a NULL pointer in i2c_mux_reserve_adapters(). - Remove device tree "i2c-controlled" property and determine this by walking the dev tree instead. - Fix compile problems with inv_mpu_acpi.c - Wait with adding the client pointer to patch 2/8 for pca9541 and pca954x. v2 compared to v1: - Allocate mux core and (optional) priv in a combined allocation. - Kill dev_err messages triggered by memory allocation failure. - Fix the device specific i2c muxes that I had overlooked. - Rebase on top of v4.4-rc8 (was based on v4.4-rc6 previously). - Drop the last two patches in the series. Cheers, Peter Antti Palosaari (1): [media] si2168: change the i2c gate to be mux-locked Peter Rosin (23): i2c-mux: add common data for every i2c-mux instance i2c: i2c-mux-gpio: convert to use an explicit i2c mux core i2c: i2c-mux-pinctrl: convert to use an explicit i2c mux core i2c: i2c-arb-gpio-challenge: convert to use an explicit i2c mux core i2c: i2c-mux-pca9541: convert to use an explicit i2c mux core i2c: i2c-mux-pca954x: convert to use an explicit i2c mux core i2c: i2c-mux-reg: convert to use an explicit i2c mux core iio: imu: inv_mpu6050: convert to use an explicit i2c mux core [media] m88ds3103: convert to use an explicit i2c mux core [media] rtl2830: convert to use an explicit i2c mux core [media] rtl2832: convert to use an explicit i2c mux core [media] si2168: convert to use an explicit i2c mux core [media] cx231xx: convert to use an explicit i2c mux core of/unittest: convert to use an explicit i2c mux core i2c-mux: drop old unused i2c-mux api i2c: allow adapter drivers to override the adapter locking i2c: muxes always lock the parent adapter i2c-mux: relax locking of the top i2c adapter during mux-locked muxing i2c-mux: document i2c muxes and elaborate on parent-/mux-locked muxes iio: imu: inv_mpu6050: change the i2c gate to be mux-locked [media] rtl2832: change the i2c gate to be mux-locked [media] rtl2832_sdr: get rid of empty regmap wrappers [media] rtl2832: regmap is aware of lockdep, drop local locking hack Documentation/i2c/i2c-topology | 370 +++++++++++++++++++++++++++ MAINTAINERS | 1 + drivers/i2c/i2c-core.c | 66 +++-- drivers/i2c/i2c-mux.c | 350 ++++++++++++++++++++----- drivers/i2c/muxes/i2c-arb-gpio-challenge.c | 47 ++-- drivers/i2c/muxes/i2c-mux-gpio.c | 72 +++--- drivers/i2c/muxes/i2c-mux-pca9541.c | 55 ++-- drivers/i2c/muxes/i2c-mux-pca954x.c | 64 ++--- drivers/i2c/muxes/i2c-mux-pinctrl.c | 124 +++++---- drivers/i2c/muxes/i2c-mux-reg.c | 63 ++--- drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c | 2 +- drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 1 - drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 78 ++---- drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 3 +- drivers/media/dvb-frontends/m88ds3103.c | 18 +- drivers/media/dvb-frontends/m88ds3103_priv.h | 2 +- drivers/media/dvb-frontends/rtl2830.c | 17 +- drivers/media/dvb-frontends/rtl2830_priv.h | 2 +- drivers/media/dvb-frontends/rtl2832.c | 241 +++-------------- drivers/media/dvb-frontends/rtl2832.h | 4 +- drivers/media/dvb-frontends/rtl2832_priv.h | 3 +- drivers/media/dvb-frontends/rtl2832_sdr.c | 303 ++++++++++------------ drivers/media/dvb-frontends/rtl2832_sdr.h | 5 +- drivers/media/dvb-frontends/si2168.c | 103 +++----- drivers/media/dvb-frontends/si2168_priv.h | 3 +- drivers/media/usb/cx231xx/cx231xx-core.c | 6 +- drivers/media/usb/cx231xx/cx231xx-i2c.c | 47 ++-- drivers/media/usb/cx231xx/cx231xx.h | 4 +- drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 5 +- drivers/of/unittest.c | 40 ++- include/linux/i2c-mux.h | 64 ++++- include/linux/i2c.h | 29 ++- 32 files changed, 1277 insertions(+), 915 deletions(-) create mode 100644 Documentation/i2c/i2c-topology -- 2.1.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v6 01/24] i2c-mux: add common data for every i2c-mux instance 2016-04-03 8:52 [PATCH v6 00/24] i2c mux cleanup and locking update Peter Rosin @ 2016-04-03 8:52 ` Peter Rosin 2016-04-11 20:46 ` Wolfram Sang 0 siblings, 1 reply; 5+ messages in thread From: Peter Rosin @ 2016-04-03 8:52 UTC (permalink / raw) To: linux-kernel Cc: Peter Rosin, Wolfram Sang, Jonathan Corbet, Peter Korsgaard, Guenter Roeck, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Antti Palosaari, Mauro Carvalho Chehab, Rob Herring, Frank Rowand, Grant Likely, Andrew Morton, Greg Kroah-Hartman, David S. Miller, Kalle Valo, Joe Perches, Jiri Slaby, Daniel Baluta, Adriana Reus, Lucas De Marchi, Matt Ranostay, Krzysztof Kozlowski, Terry Heo, Hans Verkuil, Arnd Bergmann, Tommi Rantala, linux-i2c, linux-doc, linux-iio, linux-media, devicetree, Peter Rosin From: Peter Rosin <peda@axentia.se> All i2c-muxes have a parent adapter and one or many child adapters. A mux also has some means of selection. Previously, this was stored per child adapter, but it is only needed to keep track of this per mux. Add an i2c mux core, that keeps track of this consistently. Also add some glue for users of the old interface, which will create one implicit mux core per child adapter. Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/i2c/i2c-mux.c | 238 ++++++++++++++++++++++++++++++++++++++---------- include/linux/i2c-mux.h | 47 ++++++++++ 2 files changed, 237 insertions(+), 48 deletions(-) diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c index d4022878b2f0..d95eb66e11bf 100644 --- a/drivers/i2c/i2c-mux.c +++ b/drivers/i2c/i2c-mux.c @@ -28,33 +28,34 @@ #include <linux/slab.h> /* multiplexer per channel data */ +struct i2c_mux_priv_old { + void *mux_priv; + int (*select)(struct i2c_adapter *, void *mux_priv, u32 chan_id); + int (*deselect)(struct i2c_adapter *, void *mux_priv, u32 chan_id); +}; + struct i2c_mux_priv { struct i2c_adapter adap; struct i2c_algorithm algo; - - struct i2c_adapter *parent; - struct device *mux_dev; - void *mux_priv; + struct i2c_mux_core *muxc; u32 chan_id; - - int (*select)(struct i2c_adapter *, void *mux_priv, u32 chan_id); - int (*deselect)(struct i2c_adapter *, void *mux_priv, u32 chan_id); }; static int i2c_mux_master_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) { struct i2c_mux_priv *priv = adap->algo_data; - struct i2c_adapter *parent = priv->parent; + struct i2c_mux_core *muxc = priv->muxc; + struct i2c_adapter *parent = muxc->parent; int ret; /* Switch to the right mux port and perform the transfer. */ - ret = priv->select(parent, priv->mux_priv, priv->chan_id); + ret = muxc->select(muxc, priv->chan_id); if (ret >= 0) ret = __i2c_transfer(parent, msgs, num); - if (priv->deselect) - priv->deselect(parent, priv->mux_priv, priv->chan_id); + if (muxc->deselect) + muxc->deselect(muxc, priv->chan_id); return ret; } @@ -65,17 +66,18 @@ static int i2c_mux_smbus_xfer(struct i2c_adapter *adap, int size, union i2c_smbus_data *data) { struct i2c_mux_priv *priv = adap->algo_data; - struct i2c_adapter *parent = priv->parent; + struct i2c_mux_core *muxc = priv->muxc; + struct i2c_adapter *parent = muxc->parent; int ret; /* Select the right mux port and perform the transfer. */ - ret = priv->select(parent, priv->mux_priv, priv->chan_id); + ret = muxc->select(muxc, priv->chan_id); if (ret >= 0) ret = parent->algo->smbus_xfer(parent, addr, flags, read_write, command, size, data); - if (priv->deselect) - priv->deselect(parent, priv->mux_priv, priv->chan_id); + if (muxc->deselect) + muxc->deselect(muxc, priv->chan_id); return ret; } @@ -84,7 +86,7 @@ static int i2c_mux_smbus_xfer(struct i2c_adapter *adap, static u32 i2c_mux_functionality(struct i2c_adapter *adap) { struct i2c_mux_priv *priv = adap->algo_data; - struct i2c_adapter *parent = priv->parent; + struct i2c_adapter *parent = priv->muxc->parent; return parent->algo->functionality(parent); } @@ -102,30 +104,80 @@ static unsigned int i2c_mux_parent_classes(struct i2c_adapter *parent) return class; } -struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent, - struct device *mux_dev, - void *mux_priv, u32 force_nr, u32 chan_id, - unsigned int class, - int (*select) (struct i2c_adapter *, - void *, u32), - int (*deselect) (struct i2c_adapter *, - void *, u32)) +int i2c_mux_reserve_adapters(struct i2c_mux_core *muxc, int adapters) +{ + struct i2c_adapter **adapter; + + if (adapters <= muxc->max_adapters) + return 0; + + adapter = devm_kmalloc_array(muxc->dev, + adapters, sizeof(*adapter), + GFP_KERNEL); + if (!adapter) + return -ENOMEM; + + if (muxc->adapter) { + memcpy(adapter, muxc->adapter, + muxc->max_adapters * sizeof(*adapter)); + devm_kfree(muxc->dev, muxc->adapter); + } + + muxc->adapter = adapter; + muxc->max_adapters = adapters; + return 0; +} +EXPORT_SYMBOL_GPL(i2c_mux_reserve_adapters); + +struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent, + struct device *dev, int sizeof_priv, + u32 flags, + int (*select)(struct i2c_mux_core *, u32), + int (*deselect)(struct i2c_mux_core *, u32)) { + struct i2c_mux_core *muxc; + + muxc = devm_kzalloc(dev, sizeof(*muxc) + sizeof_priv, GFP_KERNEL); + if (!muxc) + return NULL; + if (sizeof_priv) + muxc->priv = muxc + 1; + + muxc->parent = parent; + muxc->dev = dev; + muxc->select = select; + muxc->deselect = deselect; + + return muxc; +} +EXPORT_SYMBOL_GPL(i2c_mux_alloc); + +int i2c_mux_add_adapter(struct i2c_mux_core *muxc, + u32 force_nr, u32 chan_id, + unsigned int class) +{ + struct i2c_adapter *parent = muxc->parent; struct i2c_mux_priv *priv; char symlink_name[20]; int ret; - priv = kzalloc(sizeof(struct i2c_mux_priv), GFP_KERNEL); + if (muxc->adapters >= muxc->max_adapters) { + int new_max = 2 * muxc->max_adapters; + + if (!new_max) + new_max = 1; + ret = i2c_mux_reserve_adapters(muxc, new_max); + if (ret) + return ret; + } + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); if (!priv) - return NULL; + return -ENOMEM; /* Set up private adapter data */ - priv->parent = parent; - priv->mux_dev = mux_dev; - priv->mux_priv = mux_priv; + priv->muxc = muxc; priv->chan_id = chan_id; - priv->select = select; - priv->deselect = deselect; /* Need to do algo dynamically because we don't know ahead * of time what sort of physical adapter we'll be dealing with. @@ -159,11 +211,11 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent, * Try to populate the mux adapter's of_node, expands to * nothing if !CONFIG_OF. */ - if (mux_dev->of_node) { + if (muxc->dev->of_node) { struct device_node *child; u32 reg; - for_each_child_of_node(mux_dev->of_node, child) { + for_each_child_of_node(muxc->dev->of_node, child) { ret = of_property_read_u32(child, "reg", ®); if (ret) continue; @@ -177,8 +229,9 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent, /* * Associate the mux channel with an ACPI node. */ - if (has_acpi_companion(mux_dev)) - acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev), + if (has_acpi_companion(muxc->dev)) + acpi_preset_companion(&priv->adap.dev, + ACPI_COMPANION(muxc->dev), chan_id); if (force_nr) { @@ -192,33 +245,122 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent, "failed to add mux-adapter (error=%d)\n", ret); kfree(priv); - return NULL; + return ret; } - WARN(sysfs_create_link(&priv->adap.dev.kobj, &mux_dev->kobj, "mux_device"), - "can't create symlink to mux device\n"); + WARN(sysfs_create_link(&priv->adap.dev.kobj, &muxc->dev->kobj, + "mux_device"), + "can't create symlink to mux device\n"); snprintf(symlink_name, sizeof(symlink_name), "channel-%u", chan_id); - WARN(sysfs_create_link(&mux_dev->kobj, &priv->adap.dev.kobj, symlink_name), - "can't create symlink for channel %u\n", chan_id); + WARN(sysfs_create_link(&muxc->dev->kobj, &priv->adap.dev.kobj, + symlink_name), + "can't create symlink for channel %u\n", chan_id); dev_info(&parent->dev, "Added multiplexed i2c bus %d\n", i2c_adapter_id(&priv->adap)); - return &priv->adap; + muxc->adapter[muxc->adapters++] = &priv->adap; + return 0; +} +EXPORT_SYMBOL_GPL(i2c_mux_add_adapter); + +struct i2c_mux_core *i2c_mux_one_adapter(struct i2c_adapter *parent, + struct device *dev, int sizeof_priv, + u32 flags, u32 force_nr, + u32 chan_id, unsigned int class, + int (*select)(struct i2c_mux_core *, + u32), + int (*deselect)(struct i2c_mux_core *, + u32)) +{ + struct i2c_mux_core *muxc; + int ret; + + muxc = i2c_mux_alloc(parent, dev, sizeof_priv, flags, select, deselect); + if (!muxc) + return ERR_PTR(-ENOMEM); + + ret = i2c_mux_add_adapter(muxc, force_nr, chan_id, class); + if (ret) { + devm_kfree(dev, muxc); + return ERR_PTR(ret); + } + + return muxc; +} +EXPORT_SYMBOL_GPL(i2c_mux_one_adapter); + +static int i2c_mux_select(struct i2c_mux_core *muxc, u32 chan) +{ + struct i2c_mux_priv_old *priv = i2c_mux_priv(muxc); + + return priv->select(muxc->parent, priv->mux_priv, chan); +} + +static int i2c_mux_deselect(struct i2c_mux_core *muxc, u32 chan) +{ + struct i2c_mux_priv_old *priv = i2c_mux_priv(muxc); + + return priv->deselect(muxc->parent, priv->mux_priv, chan); +} + +struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent, + struct device *mux_dev, void *mux_priv, + u32 force_nr, u32 chan_id, + unsigned int class, + int (*select)(struct i2c_adapter *, + void *, u32), + int (*deselect)(struct i2c_adapter *, + void *, u32)) +{ + struct i2c_mux_core *muxc; + struct i2c_mux_priv_old *priv; + + muxc = i2c_mux_one_adapter(parent, mux_dev, sizeof(*priv), 0, + force_nr, chan_id, class, + i2c_mux_select, + deselect ? i2c_mux_deselect : NULL); + if (IS_ERR(muxc)) + return NULL; + + priv = i2c_mux_priv(muxc); + priv->select = select; + priv->deselect = deselect; + priv->mux_priv = mux_priv; + + return muxc->adapter[0]; } EXPORT_SYMBOL_GPL(i2c_add_mux_adapter); -void i2c_del_mux_adapter(struct i2c_adapter *adap) +void i2c_mux_del_adapters(struct i2c_mux_core *muxc) { - struct i2c_mux_priv *priv = adap->algo_data; char symlink_name[20]; - snprintf(symlink_name, sizeof(symlink_name), "channel-%u", priv->chan_id); - sysfs_remove_link(&priv->mux_dev->kobj, symlink_name); + while (muxc->adapters) { + struct i2c_adapter *adap = muxc->adapter[--muxc->adapters]; + struct i2c_mux_priv *priv = adap->algo_data; + + muxc->adapter[muxc->adapters] = NULL; + + snprintf(symlink_name, sizeof(symlink_name), + "channel-%u", priv->chan_id); + sysfs_remove_link(&muxc->dev->kobj, symlink_name); + + sysfs_remove_link(&priv->adap.dev.kobj, "mux_device"); + i2c_del_adapter(adap); + kfree(priv); + } +} +EXPORT_SYMBOL_GPL(i2c_mux_del_adapters); + +void i2c_del_mux_adapter(struct i2c_adapter *adap) +{ + struct i2c_mux_priv *priv = adap->algo_data; + struct i2c_mux_core *muxc = priv->muxc; - sysfs_remove_link(&priv->adap.dev.kobj, "mux_device"); - i2c_del_adapter(adap); - kfree(priv); + i2c_mux_del_adapters(muxc); + devm_kfree(muxc->dev, muxc->adapter); + devm_kfree(muxc->dev, muxc); } EXPORT_SYMBOL_GPL(i2c_del_mux_adapter); diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h index b5f9a007a3ab..0d97d7a3f03c 100644 --- a/include/linux/i2c-mux.h +++ b/include/linux/i2c-mux.h @@ -27,6 +27,32 @@ #ifdef __KERNEL__ +struct i2c_mux_core { + struct i2c_adapter *parent; + struct i2c_adapter **adapter; + int adapters; + int max_adapters; + struct device *dev; + + void *priv; + + int (*select)(struct i2c_mux_core *, u32 chan_id); + int (*deselect)(struct i2c_mux_core *, u32 chan_id); +}; + +struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent, + struct device *dev, int sizeof_priv, + u32 flags, + int (*select)(struct i2c_mux_core *, u32), + int (*deselect)(struct i2c_mux_core *, u32)); + +static inline void *i2c_mux_priv(struct i2c_mux_core *muxc) +{ + return muxc->priv; +} + +int i2c_mux_reserve_adapters(struct i2c_mux_core *muxc, int adapters); + /* * Called to create a i2c bus on a multiplexed bus segment. * The mux_dev and chan_id parameters are passed to the select @@ -41,8 +67,29 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent, void *mux_dev, u32 chan_id), int (*deselect) (struct i2c_adapter *, void *mux_dev, u32 chan_id)); +/* + * Called to create a i2c bus on a multiplexed bus segment. + * The chan_id parameter is passed to the select and deselect + * callback functions to perform hardware-specific mux control. + */ +int i2c_mux_add_adapter(struct i2c_mux_core *muxc, + u32 force_nr, u32 chan_id, + unsigned int class); + +/* + * Allocate an i2c_mux_core and add one adapter with one call. + */ +struct i2c_mux_core *i2c_mux_one_adapter(struct i2c_adapter *parent, + struct device *dev, int sizeof_priv, + u32 flags, u32 force_nr, + u32 chan_id, unsigned int class, + int (*select)(struct i2c_mux_core *, + u32), + int (*deselect)(struct i2c_mux_core *, + u32)); void i2c_del_mux_adapter(struct i2c_adapter *adap); +void i2c_mux_del_adapters(struct i2c_mux_core *muxc); #endif /* __KERNEL__ */ -- 2.1.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v6 01/24] i2c-mux: add common data for every i2c-mux instance 2016-04-03 8:52 ` [PATCH v6 01/24] i2c-mux: add common data for every i2c-mux instance Peter Rosin @ 2016-04-11 20:46 ` Wolfram Sang 2016-04-13 13:37 ` Peter Rosin 0 siblings, 1 reply; 5+ messages in thread From: Wolfram Sang @ 2016-04-11 20:46 UTC (permalink / raw) To: Peter Rosin Cc: linux-kernel, Peter Rosin, Jonathan Corbet, Peter Korsgaard, Guenter Roeck, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Antti Palosaari, Mauro Carvalho Chehab, Rob Herring, Frank Rowand, Grant Likely, Andrew Morton, Greg Kroah-Hartman, David S. Miller, Kalle Valo, Joe Perches, Jiri Slaby, Daniel Baluta, Adriana Reus, Lucas De Marchi, Matt Ranostay, Krzysztof Kozlowski, Terry Heo, Hans Verkuil, Arnd Bergmann, Tommi Rantala, linux-i2c, linux-doc, linux-iio, linux-media, devicetree [-- Attachment #1: Type: text/plain, Size: 3137 bytes --] Hi Peter, first high-level review: > +int i2c_mux_reserve_adapters(struct i2c_mux_core *muxc, int adapters) I'd suggest to rename 'adapters' into 'num_adapters' throughout this patch. I think it makes the code a lot easier to understand. > +{ > + struct i2c_adapter **adapter; > + > + if (adapters <= muxc->max_adapters) > + return 0; > + > + adapter = devm_kmalloc_array(muxc->dev, > + adapters, sizeof(*adapter), > + GFP_KERNEL); > + if (!adapter) > + return -ENOMEM; > + > + if (muxc->adapter) { > + memcpy(adapter, muxc->adapter, > + muxc->max_adapters * sizeof(*adapter)); > + devm_kfree(muxc->dev, muxc->adapter); > + } > + > + muxc->adapter = adapter; > + muxc->max_adapters = adapters; > + return 0; > +} > +EXPORT_SYMBOL_GPL(i2c_mux_reserve_adapters); Despite that I wonder why not using some of the realloc functions, I wonder even more if we couldn't supply num_adapters to i2c_mux_alloc() and reserve the memory statically. i2c busses are not dynamic/hot-pluggable so that should be good enough? > - WARN(sysfs_create_link(&priv->adap.dev.kobj, &mux_dev->kobj, "mux_device"), > - "can't create symlink to mux device\n"); > + WARN(sysfs_create_link(&priv->adap.dev.kobj, &muxc->dev->kobj, > + "mux_device"), Ignoring the 80 char limit here makes the code more readable. > + "can't create symlink to mux device\n"); > > snprintf(symlink_name, sizeof(symlink_name), "channel-%u", chan_id); > - WARN(sysfs_create_link(&mux_dev->kobj, &priv->adap.dev.kobj, symlink_name), > - "can't create symlink for channel %u\n", chan_id); > + WARN(sysfs_create_link(&muxc->dev->kobj, &priv->adap.dev.kobj, > + symlink_name), ditto. > + "can't create symlink for channel %u\n", chan_id); > dev_info(&parent->dev, "Added multiplexed i2c bus %d\n", > i2c_adapter_id(&priv->adap)); > > - return &priv->adap; > + muxc->adapter[muxc->adapters++] = &priv->adap; > + return 0; > +} > +EXPORT_SYMBOL_GPL(i2c_mux_add_adapter); > + > +struct i2c_mux_core *i2c_mux_one_adapter(struct i2c_adapter *parent, > + struct device *dev, int sizeof_priv, > + u32 flags, u32 force_nr, > + u32 chan_id, unsigned int class, > + int (*select)(struct i2c_mux_core *, > + u32), ditto > + int (*deselect)(struct i2c_mux_core *, > + u32)) ditto > +{ > + struct i2c_mux_core *muxc; > + int ret; > + > + muxc = i2c_mux_alloc(parent, dev, sizeof_priv, flags, select, deselect); > + if (!muxc) > + return ERR_PTR(-ENOMEM); > + > + ret = i2c_mux_add_adapter(muxc, force_nr, chan_id, class); > + if (ret) { > + devm_kfree(dev, muxc); > + return ERR_PTR(ret); > + } > + > + return muxc; > +} > +EXPORT_SYMBOL_GPL(i2c_mux_one_adapter); Are you sure the above function pays off? Its argument list is very complex and it doesn't save a lot of code. Having seperate calls is probably more understandable in drivers? Then again, I assume it makes the conversion of existing drivers easier. So much for now. Thanks! Wolfram [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v6 01/24] i2c-mux: add common data for every i2c-mux instance 2016-04-11 20:46 ` Wolfram Sang @ 2016-04-13 13:37 ` Peter Rosin 2016-04-15 11:23 ` Wolfram Sang 0 siblings, 1 reply; 5+ messages in thread From: Peter Rosin @ 2016-04-13 13:37 UTC (permalink / raw) To: Wolfram Sang Cc: linux-kernel, Peter Rosin, Jonathan Corbet, Peter Korsgaard, Guenter Roeck, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Antti Palosaari, Mauro Carvalho Chehab, Rob Herring, Frank Rowand, Grant Likely, Andrew Morton, Greg Kroah-Hartman, David S. Miller, Kalle Valo, Joe Perches, Jiri Slaby, Daniel Baluta, Adriana Reus, Lucas De Marchi, Matt Ranostay, Krzysztof Kozlowski, Terry Heo, Hans Verkuil, Arnd Bergmann, Tommi Rantala, linux-i2c, linux-doc, linux-iio, linux-media, devicetree Hi! On 2016-04-11 22:46, Wolfram Sang wrote: > Hi Peter, > > first high-level review: > >> +int i2c_mux_reserve_adapters(struct i2c_mux_core *muxc, int adapters) > > I'd suggest to rename 'adapters' into 'num_adapters' throughout this > patch. I think it makes the code a lot easier to understand. Hmm, you mean just the variable names, right? And not function names such as i2c_mux_reserve_(num_)adapters? >> +{ >> + struct i2c_adapter **adapter; >> + >> + if (adapters <= muxc->max_adapters) >> + return 0; >> + >> + adapter = devm_kmalloc_array(muxc->dev, >> + adapters, sizeof(*adapter), >> + GFP_KERNEL); >> + if (!adapter) >> + return -ENOMEM; >> + >> + if (muxc->adapter) { >> + memcpy(adapter, muxc->adapter, >> + muxc->max_adapters * sizeof(*adapter)); >> + devm_kfree(muxc->dev, muxc->adapter); >> + } >> + >> + muxc->adapter = adapter; >> + muxc->max_adapters = adapters; >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(i2c_mux_reserve_adapters); > > Despite that I wonder why not using some of the realloc functions, I When I wrote it, I found no devm_ version of realloc. I'm not finding anything now either... > wonder even more if we couldn't supply num_adapters to i2c_mux_alloc() > and reserve the memory statically. i2c busses are not > dynamic/hot-pluggable so that should be good enough? Yes, that would work, but it would take some restructuring in some of the drivers that currently don't know how many child adapters they are going to need when they call i2c_mux_alloc. Because you thought about removing i2c_mux_reserve_adapters completely, and not provide any means of adding more adapters than specified in the i2c_mux_alloc call, right? >> - WARN(sysfs_create_link(&priv->adap.dev.kobj, &mux_dev->kobj, "mux_device"), >> - "can't create symlink to mux device\n"); >> + WARN(sysfs_create_link(&priv->adap.dev.kobj, &muxc->dev->kobj, >> + "mux_device"), > > Ignoring the 80 char limit here makes the code more readable. That is only true if you actually have more than 80 characters, so I don't agree. Are you adamant about it? (I'm not) >> + "can't create symlink to mux device\n"); >> >> snprintf(symlink_name, sizeof(symlink_name), "channel-%u", chan_id); >> - WARN(sysfs_create_link(&mux_dev->kobj, &priv->adap.dev.kobj, symlink_name), >> - "can't create symlink for channel %u\n", chan_id); >> + WARN(sysfs_create_link(&muxc->dev->kobj, &priv->adap.dev.kobj, >> + symlink_name), > > ditto. > >> + "can't create symlink for channel %u\n", chan_id); >> dev_info(&parent->dev, "Added multiplexed i2c bus %d\n", >> i2c_adapter_id(&priv->adap)); >> >> - return &priv->adap; >> + muxc->adapter[muxc->adapters++] = &priv->adap; >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(i2c_mux_add_adapter); >> + >> +struct i2c_mux_core *i2c_mux_one_adapter(struct i2c_adapter *parent, >> + struct device *dev, int sizeof_priv, >> + u32 flags, u32 force_nr, >> + u32 chan_id, unsigned int class, >> + int (*select)(struct i2c_mux_core *, >> + u32), > > ditto > >> + int (*deselect)(struct i2c_mux_core *, >> + u32)) > > ditto > >> +{ >> + struct i2c_mux_core *muxc; >> + int ret; >> + >> + muxc = i2c_mux_alloc(parent, dev, sizeof_priv, flags, select, deselect); >> + if (!muxc) >> + return ERR_PTR(-ENOMEM); >> + >> + ret = i2c_mux_add_adapter(muxc, force_nr, chan_id, class); >> + if (ret) { >> + devm_kfree(dev, muxc); >> + return ERR_PTR(ret); >> + } >> + >> + return muxc; >> +} >> +EXPORT_SYMBOL_GPL(i2c_mux_one_adapter); > > Are you sure the above function pays off? Its argument list is very > complex and it doesn't save a lot of code. Having seperate calls is > probably more understandable in drivers? Then again, I assume it makes > the conversion of existing drivers easier. I added it in v4, you can check earlier versions if you like. Without it most gate-muxes (i.e. typically the muxes in drivers/media) grew since the i2c_add_mux_adapter call got replaced by two calls, i.e. i2c_mux_alloc followed by i2c_max_add_adapter, and coupled with error checks made it look more complex than before. So, this wasn't much of a cleanup from the point of those drivers. > So much for now. Thanks! Yeah, thanks! Cheers, Peter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v6 01/24] i2c-mux: add common data for every i2c-mux instance 2016-04-13 13:37 ` Peter Rosin @ 2016-04-15 11:23 ` Wolfram Sang 0 siblings, 0 replies; 5+ messages in thread From: Wolfram Sang @ 2016-04-15 11:23 UTC (permalink / raw) To: Peter Rosin Cc: linux-kernel, Peter Rosin, Jonathan Corbet, Peter Korsgaard, Guenter Roeck, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Antti Palosaari, Mauro Carvalho Chehab, Rob Herring, Frank Rowand, Grant Likely, Andrew Morton, Greg Kroah-Hartman, David S. Miller, Kalle Valo, Joe Perches, Jiri Slaby, Daniel Baluta, Adriana Reus, Lucas De Marchi, Matt Ranostay, Krzysztof Kozlowski, Terry Heo, Hans Verkuil, Arnd Bergmann, Tommi Rantala, linux-i2c, linux-doc, linux-iio, linux-media, devicetree [-- Attachment #1: Type: text/plain, Size: 2343 bytes --] > > I'd suggest to rename 'adapters' into 'num_adapters' throughout this > > patch. I think it makes the code a lot easier to understand. > > Hmm, you mean just the variable names, right? And not function names > such as i2c_mux_reserve_(num_)adapters? Yes, only variable names. > > Despite that I wonder why not using some of the realloc functions, I > > When I wrote it, I found no devm_ version of realloc. I'm not finding > anything now either... Right, there is no devm_ version of it :( > > wonder even more if we couldn't supply num_adapters to i2c_mux_alloc() > > and reserve the memory statically. i2c busses are not > > dynamic/hot-pluggable so that should be good enough? > > Yes, that would work, but it would take some restructuring in some of > the drivers that currently don't know how many child adapters they > are going to need when they call i2c_mux_alloc. Which ones? > Because you thought about removing i2c_mux_reserve_adapters completely, > and not provide any means of adding more adapters than specified in > the i2c_mux_alloc call, right? Yes. I assumed I2C to be static enough that such information is known in advance. > > Ignoring the 80 char limit here makes the code more readable. > > That is only true if you actually have more than 80 characters, so I don't > agree. Are you adamant about it? (I'm not) No. Keep it if you prefer it. > >> +EXPORT_SYMBOL_GPL(i2c_mux_one_adapter); > > > > Are you sure the above function pays off? Its argument list is very > > complex and it doesn't save a lot of code. Having seperate calls is > > probably more understandable in drivers? Then again, I assume it makes > > the conversion of existing drivers easier. > > I added it in v4, you can check earlier versions if you like. Without > it most gate-muxes (i.e. typically the muxes in drivers/media) grew > since the i2c_add_mux_adapter call got replaced by two calls, i.e. > i2c_mux_alloc followed by i2c_max_add_adapter, and coupled with > error checks made it look more complex than before. So, this wasn't > much of a cleanup from the point of those drivers. Hmm, v3 didn't have the driver patches posted with it. Can you push it to your branch? I am also not too strong with this one, but having a look how it looks without would be nice. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-04-15 23:10 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-15 15:52 [PATCH v6 01/24] i2c-mux: add common data for every i2c-mux instance Peter Rosin -- strict thread matches above, loose matches on Subject: below -- 2016-04-03 8:52 [PATCH v6 00/24] i2c mux cleanup and locking update Peter Rosin 2016-04-03 8:52 ` [PATCH v6 01/24] i2c-mux: add common data for every i2c-mux instance Peter Rosin 2016-04-11 20:46 ` Wolfram Sang 2016-04-13 13:37 ` Peter Rosin 2016-04-15 11:23 ` Wolfram Sang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).