| Message ID | 20220206115939.3091265-3-luca@lucaceresoli.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hi, | expand |
On Mon, Feb 7, 2022 at 7:55 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > An ATR is a device that looks similar to an i2c-mux: it has an I2C > slave "upstream" port and N master "downstream" ports, and forwards > transactions from upstream to the appropriate downstream port. But is > is different in that the forwarded transaction has a different slave > address. The address used on the upstream bus is called the "alias" > and is (potentially) different from the physical slave address of the > downstream chip. > > Add a helper file (just like i2c-mux.c for a mux or switch) to allow > implementing ATR features in a device driver. The helper takes care or > adapter creation/destruction and translates addresses at each transaction. Why I2C mux driver can't be updated to support this feature? ... > RFCv1 was implemented inside i2c-mux.c and added yet more complexity > there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR > features are not in a MUX and vice versa, the overlapping is low. This was > almost a complete rewrite, but for the records here are the main > differences from the old implementation: While this is from a code perspective, maybe i2c mux and this one can still share some parts? ... > +config I2C_ATR > + tristate "I2C Address Translator (ATR) support" > + help > + Enable support for I2C Address Translator (ATR) chips. > + > + An ATR allows accessing multiple I2C busses from a single > + physical bus via address translation instead of bus selection as > + i2c-muxes do. What would be the module name? ... > +/** Is this a kernel doc formatted documentation? Haven't you got a warning? > + * I2C Address Translator > + * > + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net> 2019,2022? > + * > + * An I2C Address Translator (ATR) is a device with an I2C slave parent > + * ("upstream") port and N I2C master child ("downstream") ports, and > + * forwards transactions from upstream to the appropriate downstream port > + * with a modified slave address. The address used on the parent bus is > + * called the "alias" and is (potentially) different from the physical > + * slave address of the child bus. Address translation is done by the > + * hardware. > + * > + * An ATR looks similar to an i2c-mux except: > + * - the address on the parent and child busses can be different > + * - there is normally no need to select the child port; the alias used on > + * the parent bus implies it > + * > + * The ATR functionality can be provided by a chip with many other > + * features. This file provides a helper to implement an ATR within your > + * driver. > + * > + * The ATR creates a new I2C "child" adapter on each child bus. Adding > + * devices on the child bus ends up in invoking the driver code to select > + * an available alias. Maintaining an appropriate pool of available aliases > + * and picking one for each new device is up to the driver implementer. The > + * ATR maintains an table of currently assigned alias and uses it to modify > + * all I2C transactions directed to devices on the child buses. > + * > + * A typical example follows. > + * > + * Topology: > + * > + * Slave X @ 0x10 > + * .-----. | > + * .-----. | |---+---- B > + * | CPU |--A--| ATR | > + * `-----' | |---+---- C > + * `-----' | > + * Slave Y @ 0x10 > + * > + * Alias table: > + * > + * Client Alias > + * ------------- > + * X 0x20 > + * Y 0x30 > + * > + * Transaction: > + * > + * - Slave X driver sends a transaction (on adapter B), slave address 0x10 > + * - ATR driver rewrites messages with address 0x20, forwards to adapter A > + * - Physical I2C transaction on bus A, slave address 0x20 > + * - ATR chip propagates transaction on bus B with address translated to 0x10 > + * - Slave X chip replies on bus B > + * - ATR chip forwards reply on bus A > + * - ATR driver rewrites messages with address 0x10 > + * - Slave X driver gets back the msgs[], with reply and address 0x10 > + * > + * Usage: > + * > + * 1. In your driver (typically in the probe function) add an ATR by > + * calling i2c_atr_new() passing your attach/detach callbacks > + * 2. When the attach callback is called pick an appropriate alias, > + * configure it in your chip and return the chosen alias in the > + * alias_id parameter > + * 3. When the detach callback is called, deconfigure the alias from > + * your chip and put it back in the pool for later usage > + * > + * Originally based on i2c-mux.c > + */ Shouldn't this comment be somewhere under Documentation/ ? ... > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/slab.h> > +static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, > + struct i2c_msg msgs[], int num) foo[] makes not much sense in the function parameter. *foo is what will be used and it's explicit. Can this be located on one line (similar question to make compact the rest of the function declarations)? > + Redundant blank line. ... > + /* Ensure we have enough room to save the original addresses */ > + if (unlikely(chan->orig_addrs_size < num)) { > + void *new_buf = kmalloc(num * sizeof(chan->orig_addrs[0]), > + GFP_KERNEL); Use kmalloc_array() > + if (new_buf == NULL) > + return -ENOMEM; > + > + kfree(chan->orig_addrs); Hmm... is it a reimplementation of krealloc_array()? > + chan->orig_addrs = new_buf; > + chan->orig_addrs_size = num; > + } ... > + if (c2a) { > + msgs[i].addr = c2a->alias; > + } else { > + dev_err(atr->dev, "client 0x%02x not mapped!\n", > + msgs[i].addr); > + return -ENXIO; > + } 'else' would be redundant if you switch to the traditional pattern, i.e. check for errors first. ... > +/* > + * Restore all message address aliases with the original addresses. > + * > + * This function is internal for use in i2c_atr_master_xfer(). > + * > + * @see i2c_atr_map_msgs() > + */ Too sparse formatting of the comment. Can you make it compact? ... > + int ret = 0; Unneeded assignment. > + /* Switch to the right atr port */ > + if (atr->ops->select) { > + ret = atr->ops->select(atr, chan->chan_id); > + if (ret < 0) > + goto out; > + } > + > + /* Translate addresses */ > + mutex_lock(&chan->orig_addrs_lock); > + ret = i2c_atr_map_msgs(chan, msgs, num); > + if (ret < 0) { > + mutex_unlock(&chan->orig_addrs_lock); > + goto out; goto out_unlock_deselect; > + } > + > + /* Perform the transfer */ > + ret = i2c_transfer(parent, msgs, num); > + > + /* Restore addresses */ > + i2c_atr_unmap_msgs(chan, msgs, num); out_unlock_deselct: > + mutex_unlock(&chan->orig_addrs_lock); > +out: out_deselect: > + if (atr->ops->deselect) > + atr->ops->deselect(atr, chan->chan_id); > + > + return ret; > +} ... > + int err = 0; Be consistent with ret vs. err across the functions. > + if (atr->ops->select) > + err = atr->ops->select(atr, chan->chan_id); > + if (!err) Perhaps int ret; ret = 0; if (atr->ops->select) ret = atr->ops->select(atr, chan->chan_id); if (ret) goto out_deselect; > + err = i2c_smbus_xfer(parent, c2a->alias, flags, > + read_write, command, size, data); out_deselect: > + if (atr->ops->deselect) > + atr->ops->deselect(atr, chan->chan_id); > + > + return err; > +} ... > + int err = 0; Same as above: naming, useless assignment. ... > + c2a = kzalloc(sizeof(struct i2c_atr_cli2alias_pair), GFP_KERNEL); sizeof(*c2a) > + if (!c2a) { > + err = -ENOMEM; > + goto err_alloc; Useless label, return directly. > + } ... > + c2a = i2c_atr_find_mapping_by_client(&chan->alias_list, client); > + if (c2a != NULL) { if (c2a) > + list_del(&c2a->node); > + kfree(c2a); > + } ... > + char symlink_name[20]; Why 20? Do we have a predefined constant for that? > + if (dev->of_node) { This check can be dropped, also please use device property and fwnode APIs. No good of having OF-centric generic modules nowadays. > + struct device_node *atr_node; > + struct device_node *child; > + u32 reg; > + > + atr_node = of_get_child_by_name(dev->of_node, "i2c-atr"); atr_node = device_get_named_child_node(...); fwnode_for_each_child_node() { } > + for_each_child_of_node(atr_node, child) { > + err = of_property_read_u32(child, "reg", ®); > + if (err) > + continue; > + if (chan_id == reg) > + break; > + } > + > + chan->adap.dev.of_node = child; > + of_node_put(atr_node); > + } On the second thought can you utilize the parser from I2C mux? ... > + WARN(sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"), > + "can't create symlink to atr device\n"); > + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", chan_id); > + WARN(sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name), > + "can't create symlink for channel %u\n", chan_id); Doesn't sysfs already has a warning when it's really needed? ... > + if (atr->adapter[chan_id] == NULL) { > + dev_err(dev, "Adapter %d does not exist\n", chan_id); Noisy message. On freeing we usually don't issue such when we try to free already freeed resource. > + return; > + } ... > + atr = devm_kzalloc(dev, sizeof(*atr) > + + max_adapters * sizeof(atr->adapter[0]), > + GFP_KERNEL); Check overflow.h and use respective macro here. > + if (!atr) > + return ERR_PTR(-ENOMEM); ... > +/** It's not a kernel doc. > + * drivers/i2c/i2c-atr.h -- I2C Address Translator Please, no names of the files inside the files. > + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net> 2019,2022 ? > + * Based on i2c-mux.h > + */ ... > +#ifdef __KERNEL__ Why? ... > +#include <linux/i2c.h> > +#include <linux/mutex.h> Missed types.h Missed struct device;
Hi Andy, thank you for the _very_ detailed review and apologies for not having found the time to reply until now. I'm OK with most of your comments, so I'm not commenting on them for brevity. Below my comments on the remaining topics. On 08/02/22 12:16, Andy Shevchenko wrote: > On Mon, Feb 7, 2022 at 7:55 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: >> >> An ATR is a device that looks similar to an i2c-mux: it has an I2C >> slave "upstream" port and N master "downstream" ports, and forwards >> transactions from upstream to the appropriate downstream port. But is >> is different in that the forwarded transaction has a different slave >> address. The address used on the upstream bus is called the "alias" >> and is (potentially) different from the physical slave address of the >> downstream chip. >> >> Add a helper file (just like i2c-mux.c for a mux or switch) to allow >> implementing ATR features in a device driver. The helper takes care or >> adapter creation/destruction and translates addresses at each transaction. > > Why I2C mux driver can't be updated to support this feature? My first version did that. But it was very complex to shoehorn the ATR features in the i2c-mux code which already handles various [corner] cases. If memory serves, code reuse was limited to the trivial code: allocations, cleanups and the like. The root reason is that an atr and a mux have a similar electric topology, but they do very different things. An mux need to be commanded to switch from one downstream bus to another, an atr does not. An atr modifies the transaction, including the speed, a mux does not. >> RFCv1 was implemented inside i2c-mux.c and added yet more complexity >> there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR >> features are not in a MUX and vice versa, the overlapping is low. This was >> almost a complete rewrite, but for the records here are the main >> differences from the old implementation: > > While this is from a code perspective, maybe i2c mux and this one can > still share some parts? Possibly. I'd have to look into that in more detail. I must say having a separate file allowed me to be free to implement whatever is best for atr. With that done I would certainly make sense to check whether there are still enough commonalities to share code, maybe in a .c file with shared functions. >> +config I2C_ATR >> + tristate "I2C Address Translator (ATR) support" >> + help >> + Enable support for I2C Address Translator (ATR) chips. >> + >> + An ATR allows accessing multiple I2C busses from a single >> + physical bus via address translation instead of bus selection as >> + i2c-muxes do. > > What would be the module name? Isn't the module name written in Kconfig files just to avoid checkpatch complain about "too few doc lines"? :) Oook, it's i2s-atr anyway. >> +/** > > Is this a kernel doc formatted documentation? > Haven't you got a warning? Not from checkpatch, but I got one from the kernel test robot. Will fix. [...] >> + * >> + * An I2C Address Translator (ATR) is a device with an I2C slave parent >> + * ("upstream") port and N I2C master child ("downstream") ports, and >> + * forwards transactions from upstream to the appropriate downstream port >> + * with a modified slave address. The address used on the parent bus is >> + * called the "alias" and is (potentially) different from the physical >> + * slave address of the child bus. Address translation is done by the >> + * hardware. >> + * >> + * An ATR looks similar to an i2c-mux except: >> + * - the address on the parent and child busses can be different >> + * - there is normally no need to select the child port; the alias used on >> + * the parent bus implies it >> + * >> + * The ATR functionality can be provided by a chip with many other >> + * features. This file provides a helper to implement an ATR within your >> + * driver. >> + * >> + * The ATR creates a new I2C "child" adapter on each child bus. Adding >> + * devices on the child bus ends up in invoking the driver code to select >> + * an available alias. Maintaining an appropriate pool of available aliases >> + * and picking one for each new device is up to the driver implementer. The >> + * ATR maintains an table of currently assigned alias and uses it to modify >> + * all I2C transactions directed to devices on the child buses. >> + * >> + * A typical example follows. >> + * >> + * Topology: >> + * >> + * Slave X @ 0x10 >> + * .-----. | >> + * .-----. | |---+---- B >> + * | CPU |--A--| ATR | >> + * `-----' | |---+---- C >> + * `-----' | >> + * Slave Y @ 0x10 >> + * >> + * Alias table: >> + * >> + * Client Alias >> + * ------------- >> + * X 0x20 >> + * Y 0x30 >> + * >> + * Transaction: >> + * >> + * - Slave X driver sends a transaction (on adapter B), slave address 0x10 >> + * - ATR driver rewrites messages with address 0x20, forwards to adapter A >> + * - Physical I2C transaction on bus A, slave address 0x20 >> + * - ATR chip propagates transaction on bus B with address translated to 0x10 >> + * - Slave X chip replies on bus B >> + * - ATR chip forwards reply on bus A >> + * - ATR driver rewrites messages with address 0x10 >> + * - Slave X driver gets back the msgs[], with reply and address 0x10 >> + * >> + * Usage: >> + * >> + * 1. In your driver (typically in the probe function) add an ATR by >> + * calling i2c_atr_new() passing your attach/detach callbacks >> + * 2. When the attach callback is called pick an appropriate alias, >> + * configure it in your chip and return the chosen alias in the >> + * alias_id parameter >> + * 3. When the detach callback is called, deconfigure the alias from >> + * your chip and put it back in the pool for later usage >> + * >> + * Originally based on i2c-mux.c >> + */ > > Shouldn't this comment be somewhere under Documentation/ ? Uhm, yes, I agree it's a good idea to move this entire comment there. >> + if (dev->of_node) { > > This check can be dropped, also please use device property and fwnode > APIs. No good of having OF-centric generic modules nowadays. Sure! This code was written in another decade and I didn't update it... As you noticed elsewhere it also honors the old, strict 80-chars per line limit in various places where it makes no sense anymore. >> + WARN(sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"), >> + "can't create symlink to atr device\n"); >> + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", chan_id); >> + WARN(sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name), >> + "can't create symlink for channel %u\n", chan_id); > > Doesn't sysfs already has a warning when it's really needed? I have to check that. I usually don't add unnecessary log messages. [...] >> +#include <linux/i2c.h> >> +#include <linux/mutex.h> > > Missed types.h > > Missed struct device; Not sure I got your point here. This file has some 'struct device *', which do not need a declaration, and has zero non-pointer uses of 'struct device'.
On 2/16/22 10:40, Luca Ceresoli wrote: > On 08/02/22 12:16, Andy Shevchenko wrote: >> On Mon, Feb 7, 2022 at 7:55 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>> +config I2C_ATR >>> + tristate "I2C Address Translator (ATR) support" >>> + help >>> + Enable support for I2C Address Translator (ATR) chips. >>> + >>> + An ATR allows accessing multiple I2C busses from a single >>> + physical bus via address translation instead of bus selection as >>> + i2c-muxes do. >> >> What would be the module name? > > Isn't the module name written in Kconfig files just to avoid checkpatch > complain about "too few doc lines"? :) Oook, it's i2s-atr anyway. Thanks Luca! I have always wondered why people keep adding this seemingly unnecessary boilerplate. Now I finally get the purpose! --Matti
Hi dee Ho peeps! On 2/6/22 13:59, Luca Ceresoli wrote: > An ATR is a device that looks similar to an i2c-mux: it has an I2C > slave "upstream" port and N master "downstream" ports, and forwards > transactions from upstream to the appropriate downstream port. But is > is different in that the forwarded transaction has a different slave > address. The address used on the upstream bus is called the "alias" > and is (potentially) different from the physical slave address of the > downstream chip. > > Add a helper file (just like i2c-mux.c for a mux or switch) to allow > implementing ATR features in a device driver. The helper takes care or > adapter creation/destruction and translates addresses at each transaction. > snip > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig > index 438905e2a1d0..c6d1a345ea6d 100644 > --- a/drivers/i2c/Kconfig > +++ b/drivers/i2c/Kconfig > @@ -71,6 +71,15 @@ config I2C_MUX > > source "drivers/i2c/muxes/Kconfig" > > +config I2C_ATR > + tristate "I2C Address Translator (ATR) support" > + help > + Enable support for I2C Address Translator (ATR) chips. > + > + An ATR allows accessing multiple I2C busses from a single > + physical bus via address translation instead of bus selection as > + i2c-muxes do. > + I continued playing with the ROHM (de-)serializer and ended up having .config where the I2C_ATR was ='m', while my ATR driver was ='y' even though it selects the I2C_ATR. Yep, most probably my error somewhere. Anyways, this made me think that most of the I2C_ATR users are likely to just silently select the I2C_ATR, right? The I2C_ATR has no much reason to be compiled in w/o users, right? So perhaps the menu entry for selecting the I2C_ATR could be dropped(?) Do we really need this entry in already long list of configs to be manually picked? snip > +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev, > + const struct i2c_atr_ops *ops, int max_adapters) > +{ > + struct i2c_atr *atr; > + > + if (!ops || !ops->attach_client || !ops->detach_client) > + return ERR_PTR(-EINVAL); > + I believe that most of the attach_client implementations will have similar approach of allocating and populating an address-pool and searching for first unused address. As a 'further dev' it'd be great to see a common helper implementation for attach/detach - perhaps so that the atr drivers would only need to specify the slave-address configuration register(s) / mask and the use a 'generic' attach/detach helpers. Well, just thinking how to reduce the code from actual IC drivers but this is really not something that is required during this initial series :) Also, devm-variants would be great - although that falls to the same category of things that do not need to be done immediately - but would perhaps be worth considering in the future. so, perhaps reconsider the Kconfig but for what-ever it is worth: Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Yours Matti
Hi Matti, On 16/03/22 15:11, Vaittinen, Matti wrote: > Hi dee Ho peeps! > > On 2/6/22 13:59, Luca Ceresoli wrote: >> An ATR is a device that looks similar to an i2c-mux: it has an I2C >> slave "upstream" port and N master "downstream" ports, and forwards >> transactions from upstream to the appropriate downstream port. But is >> is different in that the forwarded transaction has a different slave >> address. The address used on the upstream bus is called the "alias" >> and is (potentially) different from the physical slave address of the >> downstream chip. >> >> Add a helper file (just like i2c-mux.c for a mux or switch) to allow >> implementing ATR features in a device driver. The helper takes care or >> adapter creation/destruction and translates addresses at each transaction. >> > > snip > >> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig >> index 438905e2a1d0..c6d1a345ea6d 100644 >> --- a/drivers/i2c/Kconfig >> +++ b/drivers/i2c/Kconfig >> @@ -71,6 +71,15 @@ config I2C_MUX >> >> source "drivers/i2c/muxes/Kconfig" >> >> +config I2C_ATR >> + tristate "I2C Address Translator (ATR) support" >> + help >> + Enable support for I2C Address Translator (ATR) chips. >> + >> + An ATR allows accessing multiple I2C busses from a single >> + physical bus via address translation instead of bus selection as >> + i2c-muxes do. >> + > > I continued playing with the ROHM (de-)serializer and ended up having > .config where the I2C_ATR was ='m', while my ATR driver was ='y' even > though it selects the I2C_ATR. > > Yep, most probably my error somewhere. > > Anyways, this made me think that most of the I2C_ATR users are likely to > just silently select the I2C_ATR, right? The I2C_ATR has no much reason > to be compiled in w/o users, right? So perhaps the menu entry for > selecting the I2C_ATR could be dropped(?) Do we really need this entry > in already long list of configs to be manually picked? Maybe we could make it a blind option, sure. The only reason it could be useful that it's visible is that one might implement a user driver could be written out of tree. I don't care very much about that, but it is possible. Maybe it's the reason for I2C_MUX to be a visible option too. Peter? >> +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev, >> + const struct i2c_atr_ops *ops, int max_adapters) >> +{ >> + struct i2c_atr *atr; >> + >> + if (!ops || !ops->attach_client || !ops->detach_client) >> + return ERR_PTR(-EINVAL); >> + > > I believe that most of the attach_client implementations will have > similar approach of allocating and populating an address-pool and > searching for first unused address. As a 'further dev' it'd be great to > see a common helper implementation for attach/detach - perhaps so that > the atr drivers would only need to specify the slave-address > configuration register(s) / mask and the use a 'generic' attach/detach > helpers. Well, just thinking how to reduce the code from actual IC > drivers but this is really not something that is required during this > initial series :) > > Also, devm-variants would be great - although that falls to the same > category of things that do not need to be done immediately - but would > perhaps be worth considering in the future. Both of your proposals make sense, however I did deliberately not generalize too much because I knew only one chipset. I don't like trying to generalize for an unpredictable future use case, it generally leads (me) to generalizing in the wrong direction. That means you'd be very welcome to propose helpers and/or devm variants, possibly in the same patchset as the first Rohm serdes driver. ;) > Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Thanks for your review!
diff --git a/MAINTAINERS b/MAINTAINERS index 69a2935daf6c..7383aec87e4a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8961,6 +8961,13 @@ L: linux-acpi@vger.kernel.org S: Maintained F: drivers/i2c/i2c-core-acpi.c +I2C ADDRESS TRANSLATOR (ATR) +M: Luca Ceresoli <luca@lucaceresoli.net> +L: linux-i2c@vger.kernel.org +S: Maintained +F: drivers/i2c/i2c-atr.c +F: include/linux/i2c-atr.h + I2C CONTROLLER DRIVER FOR NVIDIA GPU M: Ajay Gupta <ajayg@nvidia.com> L: linux-i2c@vger.kernel.org diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig index 438905e2a1d0..c6d1a345ea6d 100644 --- a/drivers/i2c/Kconfig +++ b/drivers/i2c/Kconfig @@ -71,6 +71,15 @@ config I2C_MUX source "drivers/i2c/muxes/Kconfig" +config I2C_ATR + tristate "I2C Address Translator (ATR) support" + help + Enable support for I2C Address Translator (ATR) chips. + + An ATR allows accessing multiple I2C busses from a single + physical bus via address translation instead of bus selection as + i2c-muxes do. + config I2C_HELPER_AUTO bool "Autoselect pertinent helper modules" default y diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile index c1d493dc9bac..3f71ce4711e3 100644 --- a/drivers/i2c/Makefile +++ b/drivers/i2c/Makefile @@ -13,6 +13,7 @@ i2c-core-$(CONFIG_OF) += i2c-core-of.o obj-$(CONFIG_I2C_SMBUS) += i2c-smbus.o obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o obj-$(CONFIG_I2C_MUX) += i2c-mux.o +obj-$(CONFIG_I2C_ATR) += i2c-atr.o obj-y += algos/ busses/ muxes/ obj-$(CONFIG_I2C_STUB) += i2c-stub.o obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c new file mode 100644 index 000000000000..f44a5696f7d8 --- /dev/null +++ b/drivers/i2c/i2c-atr.c @@ -0,0 +1,557 @@ +// SPDX-License-Identifier: GPL-2.0 +/** + * I2C Address Translator + * + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net> + * + * An I2C Address Translator (ATR) is a device with an I2C slave parent + * ("upstream") port and N I2C master child ("downstream") ports, and + * forwards transactions from upstream to the appropriate downstream port + * with a modified slave address. The address used on the parent bus is + * called the "alias" and is (potentially) different from the physical + * slave address of the child bus. Address translation is done by the + * hardware. + * + * An ATR looks similar to an i2c-mux except: + * - the address on the parent and child busses can be different + * - there is normally no need to select the child port; the alias used on + * the parent bus implies it + * + * The ATR functionality can be provided by a chip with many other + * features. This file provides a helper to implement an ATR within your + * driver. + * + * The ATR creates a new I2C "child" adapter on each child bus. Adding + * devices on the child bus ends up in invoking the driver code to select + * an available alias. Maintaining an appropriate pool of available aliases + * and picking one for each new device is up to the driver implementer. The + * ATR maintains an table of currently assigned alias and uses it to modify + * all I2C transactions directed to devices on the child buses. + * + * A typical example follows. + * + * Topology: + * + * Slave X @ 0x10 + * .-----. | + * .-----. | |---+---- B + * | CPU |--A--| ATR | + * `-----' | |---+---- C + * `-----' | + * Slave Y @ 0x10 + * + * Alias table: + * + * Client Alias + * ------------- + * X 0x20 + * Y 0x30 + * + * Transaction: + * + * - Slave X driver sends a transaction (on adapter B), slave address 0x10 + * - ATR driver rewrites messages with address 0x20, forwards to adapter A + * - Physical I2C transaction on bus A, slave address 0x20 + * - ATR chip propagates transaction on bus B with address translated to 0x10 + * - Slave X chip replies on bus B + * - ATR chip forwards reply on bus A + * - ATR driver rewrites messages with address 0x10 + * - Slave X driver gets back the msgs[], with reply and address 0x10 + * + * Usage: + * + * 1. In your driver (typically in the probe function) add an ATR by + * calling i2c_atr_new() passing your attach/detach callbacks + * 2. When the attach callback is called pick an appropriate alias, + * configure it in your chip and return the chosen alias in the + * alias_id parameter + * 3. When the detach callback is called, deconfigure the alias from + * your chip and put it back in the pool for later usage + * + * Originally based on i2c-mux.c + */ + +#include <linux/i2c.h> +#include <linux/i2c-atr.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/slab.h> + +/** + * struct i2c_atr_cli2alias_pair - Hold the alias assigned to a client. + * @node: List node + * @client: Pointer to the client on the child bus + * @alias: I2C alias address assigned by the driver. + * This is the address that will be used to issue I2C transactions + * on the parent (physical) bus. + */ +struct i2c_atr_cli2alias_pair { + struct list_head node; + const struct i2c_client *client; + u16 alias; +}; + +/* + * Data for each channel (child bus) + */ +struct i2c_atr_chan { + struct i2c_adapter adap; + struct i2c_atr *atr; + u32 chan_id; + + struct list_head alias_list; + + u16 *orig_addrs; + unsigned int orig_addrs_size; + struct mutex orig_addrs_lock; /* Lock orig_addrs during xfer */ +}; + +static struct i2c_atr_cli2alias_pair * +i2c_atr_find_mapping_by_client(const struct list_head *list, + const struct i2c_client *client) +{ + struct i2c_atr_cli2alias_pair *c2a; + + list_for_each_entry(c2a, list, node) { + if (c2a->client == client) + return c2a; + } + + return NULL; +} + +static struct i2c_atr_cli2alias_pair * +i2c_atr_find_mapping_by_addr(const struct list_head *list, + u16 phys_addr) +{ + struct i2c_atr_cli2alias_pair *c2a; + + list_for_each_entry(c2a, list, node) { + if (c2a->client->addr == phys_addr) + return c2a; + } + + return NULL; +} + +/* + * Replace all message addresses with their aliases, saving the original + * addresses. + * + * This function is internal for use in i2c_atr_master_xfer(). It must be + * followed by i2c_atr_unmap_msgs() to restore the original addresses. + */ +static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, + struct i2c_msg msgs[], int num) + +{ + struct i2c_atr *atr = chan->atr; + static struct i2c_atr_cli2alias_pair *c2a; + int i; + + /* Ensure we have enough room to save the original addresses */ + if (unlikely(chan->orig_addrs_size < num)) { + void *new_buf = kmalloc(num * sizeof(chan->orig_addrs[0]), + GFP_KERNEL); + if (new_buf == NULL) + return -ENOMEM; + + kfree(chan->orig_addrs); + chan->orig_addrs = new_buf; + chan->orig_addrs_size = num; + } + + for (i = 0; i < num; i++) { + chan->orig_addrs[i] = msgs[i].addr; + + c2a = i2c_atr_find_mapping_by_addr(&chan->alias_list, + msgs[i].addr); + if (c2a) { + msgs[i].addr = c2a->alias; + } else { + dev_err(atr->dev, "client 0x%02x not mapped!\n", + msgs[i].addr); + return -ENXIO; + } + } + + return 0; +} + +/* + * Restore all message address aliases with the original addresses. + * + * This function is internal for use in i2c_atr_master_xfer(). + * + * @see i2c_atr_map_msgs() + */ +static void i2c_atr_unmap_msgs(struct i2c_atr_chan *chan, + struct i2c_msg msgs[], int num) +{ + int i; + + for (i = 0; i < num; i++) + msgs[i].addr = chan->orig_addrs[i]; +} + +static int i2c_atr_master_xfer(struct i2c_adapter *adap, + struct i2c_msg msgs[], int num) +{ + struct i2c_atr_chan *chan = adap->algo_data; + struct i2c_atr *atr = chan->atr; + struct i2c_adapter *parent = atr->parent; + int ret = 0; + + /* Switch to the right atr port */ + if (atr->ops->select) { + ret = atr->ops->select(atr, chan->chan_id); + if (ret < 0) + goto out; + } + + /* Translate addresses */ + mutex_lock(&chan->orig_addrs_lock); + ret = i2c_atr_map_msgs(chan, msgs, num); + if (ret < 0) { + mutex_unlock(&chan->orig_addrs_lock); + goto out; + } + + /* Perform the transfer */ + ret = i2c_transfer(parent, msgs, num); + + /* Restore addresses */ + i2c_atr_unmap_msgs(chan, msgs, num); + mutex_unlock(&chan->orig_addrs_lock); + +out: + if (atr->ops->deselect) + atr->ops->deselect(atr, chan->chan_id); + + return ret; +} + +static int i2c_atr_smbus_xfer(struct i2c_adapter *adap, + u16 addr, unsigned short flags, + char read_write, u8 command, + int size, union i2c_smbus_data *data) +{ + struct i2c_atr_chan *chan = adap->algo_data; + struct i2c_atr *atr = chan->atr; + struct i2c_adapter *parent = atr->parent; + struct i2c_atr_cli2alias_pair *c2a; + int err = 0; + + c2a = i2c_atr_find_mapping_by_addr(&chan->alias_list, addr); + if (!c2a) { + dev_err(atr->dev, "client 0x%02x not mapped!\n", addr); + return -ENXIO; + } + + if (atr->ops->select) + err = atr->ops->select(atr, chan->chan_id); + if (!err) + err = i2c_smbus_xfer(parent, c2a->alias, flags, + read_write, command, size, data); + if (atr->ops->deselect) + atr->ops->deselect(atr, chan->chan_id); + + return err; +} + +static u32 i2c_atr_functionality(struct i2c_adapter *adap) +{ + struct i2c_atr_chan *chan = adap->algo_data; + struct i2c_adapter *parent = chan->atr->parent; + + return parent->algo->functionality(parent); +} + +static void i2c_atr_lock_bus(struct i2c_adapter *adapter, unsigned int flags) +{ + struct i2c_atr_chan *chan = adapter->algo_data; + struct i2c_atr *atr = chan->atr; + + mutex_lock(&atr->lock); +} + +static int i2c_atr_trylock_bus(struct i2c_adapter *adapter, unsigned int flags) +{ + struct i2c_atr_chan *chan = adapter->algo_data; + struct i2c_atr *atr = chan->atr; + + return mutex_trylock(&atr->lock); +} + +static void i2c_atr_unlock_bus(struct i2c_adapter *adapter, unsigned int flags) +{ + struct i2c_atr_chan *chan = adapter->algo_data; + struct i2c_atr *atr = chan->atr; + + mutex_unlock(&atr->lock); +} + +static const struct i2c_lock_operations i2c_atr_lock_ops = { + .lock_bus = i2c_atr_lock_bus, + .trylock_bus = i2c_atr_trylock_bus, + .unlock_bus = i2c_atr_unlock_bus, +}; + +static int i2c_atr_attach_client(struct i2c_adapter *adapter, + const struct i2c_board_info *info, + const struct i2c_client *client) +{ + struct i2c_atr_chan *chan = adapter->algo_data; + struct i2c_atr *atr = chan->atr; + struct i2c_atr_cli2alias_pair *c2a; + u16 alias_id = 0; + int err = 0; + + c2a = kzalloc(sizeof(struct i2c_atr_cli2alias_pair), GFP_KERNEL); + if (!c2a) { + err = -ENOMEM; + goto err_alloc; + } + + err = atr->ops->attach_client(atr, chan->chan_id, info, client, + &alias_id); + if (err) + goto err_attach; + if (alias_id == 0) { + err = -EINVAL; + goto err_attach; + } + + c2a->client = client; + c2a->alias = alias_id; + list_add(&c2a->node, &chan->alias_list); + + return 0; + +err_attach: + kfree(c2a); +err_alloc: + return err; +} + +static void i2c_atr_detach_client(struct i2c_adapter *adapter, + const struct i2c_client *client) +{ + struct i2c_atr_chan *chan = adapter->algo_data; + struct i2c_atr *atr = chan->atr; + struct i2c_atr_cli2alias_pair *c2a; + + atr->ops->detach_client(atr, chan->chan_id, client); + + c2a = i2c_atr_find_mapping_by_client(&chan->alias_list, client); + if (c2a != NULL) { + list_del(&c2a->node); + kfree(c2a); + } +} + +static const struct i2c_attach_operations i2c_atr_attach_ops = { + .attach_client = i2c_atr_attach_client, + .detach_client = i2c_atr_detach_client, +}; + +/** + * i2c_atr_add_adapter - Create a child ("downstream") I2C bus. + * @atr: The I2C ATR + * @chan_id: Index of the new adapter (0 .. max_adapters-1). This value is + * passed to the callbacks in `struct i2c_atr_ops`. + * + * After calling this function a new i2c bus will appear. Adding and + * removing devices on the downstream bus will result in calls to the + * `attach_client` and `detach_client` callbacks for the driver to assign + * an alias to the device. + * + * If there is a device tree node under "i2c-atr" whose "reg" property + * equals chan_id, the new adapter will receive that node and perhaps start + * adding devices under it. The callbacks for those additions will be made + * before i2c_atr_add_adapter() returns. + * + * Call i2c_atr_del_adapter() to remove the adapter. + * + * Return: 0 on success, a negative error code otherwise. + */ +int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id) +{ + struct i2c_adapter *parent = atr->parent; + struct device *dev = atr->dev; + struct i2c_atr_chan *chan; + char symlink_name[20]; + int err; + + if (chan_id >= atr->max_adapters) + return -EINVAL; + + if (atr->adapter[chan_id]) { + dev_err(dev, "Adapter %d already present\n", chan_id); + return -EEXIST; + } + + chan = kzalloc(sizeof(*chan), GFP_KERNEL); + if (!chan) + return -ENOMEM; + + chan->atr = atr; + chan->chan_id = chan_id; + INIT_LIST_HEAD(&chan->alias_list); + mutex_init(&chan->orig_addrs_lock); + + snprintf(chan->adap.name, sizeof(chan->adap.name), + "i2c-%d-atr-%d", i2c_adapter_id(parent), chan_id); + chan->adap.owner = THIS_MODULE; + chan->adap.algo = &atr->algo; + chan->adap.algo_data = chan; + chan->adap.dev.parent = dev; + chan->adap.retries = parent->retries; + chan->adap.timeout = parent->timeout; + chan->adap.quirks = parent->quirks; + chan->adap.lock_ops = &i2c_atr_lock_ops; + chan->adap.attach_ops = &i2c_atr_attach_ops; + + if (dev->of_node) { + struct device_node *atr_node; + struct device_node *child; + u32 reg; + + atr_node = of_get_child_by_name(dev->of_node, "i2c-atr"); + + for_each_child_of_node(atr_node, child) { + err = of_property_read_u32(child, "reg", ®); + if (err) + continue; + if (chan_id == reg) + break; + } + + chan->adap.dev.of_node = child; + of_node_put(atr_node); + } + + err = i2c_add_adapter(&chan->adap); + if (err) { + dev_err(dev, "failed to add atr-adapter %u (error=%d)\n", + chan_id, err); + goto err_add_adapter; + } + + WARN(sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"), + "can't create symlink to atr device\n"); + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", chan_id); + WARN(sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name), + "can't create symlink for channel %u\n", chan_id); + + dev_info(dev, "Added ATR child bus %d\n", i2c_adapter_id(&chan->adap)); + + atr->adapter[chan_id] = &chan->adap; + return 0; + +err_add_adapter: + mutex_destroy(&chan->orig_addrs_lock); + kfree(chan); + return err; +} +EXPORT_SYMBOL_GPL(i2c_atr_add_adapter); + +/** + * i2c_atr_del_adapter - Remove a child ("downstream") I2C bus added by + * i2c_atr_del_adapter(). + * @atr: The I2C ATR + * @chan_id: Index of the `adapter to be removed (0 .. max_adapters-1) + */ +void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id) +{ + char symlink_name[20]; + + struct i2c_adapter *adap = atr->adapter[chan_id]; + struct i2c_atr_chan *chan = adap->algo_data; + struct device_node *np = adap->dev.of_node; + struct device *dev = atr->dev; + + if (atr->adapter[chan_id] == NULL) { + dev_err(dev, "Adapter %d does not exist\n", chan_id); + return; + } + + dev_info(dev, "Removing ATR child bus %d\n", i2c_adapter_id(adap)); + + atr->adapter[chan_id] = NULL; + + snprintf(symlink_name, sizeof(symlink_name), + "channel-%u", chan->chan_id); + sysfs_remove_link(&dev->kobj, symlink_name); + sysfs_remove_link(&chan->adap.dev.kobj, "atr_device"); + + i2c_del_adapter(adap); + of_node_put(np); + mutex_destroy(&chan->orig_addrs_lock); + kfree(chan); +} +EXPORT_SYMBOL_GPL(i2c_atr_del_adapter); + +/** + * i2c_atr_new() - Allocate and initialize an I2C ATR helper. + * @parent: The parent (upstream) adapter + * @dev: The device acting as an ATR + * @ops: Driver-specific callbacks + * @max_adapters: Maximum number of child adapters + * + * The new ATR helper is connected to the parent adapter but has no child + * adapters. Call i2c_atr_add_adapter() to add some. + * + * Call i2c_atr_delete() to remove. + * + * Return: pointer to the new ATR helper object, or ERR_PTR + */ +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev, + const struct i2c_atr_ops *ops, int max_adapters) +{ + struct i2c_atr *atr; + + if (!ops || !ops->attach_client || !ops->detach_client) + return ERR_PTR(-EINVAL); + + atr = devm_kzalloc(dev, sizeof(*atr) + + max_adapters * sizeof(atr->adapter[0]), + GFP_KERNEL); + if (!atr) + return ERR_PTR(-ENOMEM); + + mutex_init(&atr->lock); + + atr->parent = parent; + atr->dev = dev; + atr->ops = ops; + atr->max_adapters = max_adapters; + + if (parent->algo->master_xfer) + atr->algo.master_xfer = i2c_atr_master_xfer; + if (parent->algo->smbus_xfer) + atr->algo.smbus_xfer = i2c_atr_smbus_xfer; + atr->algo.functionality = i2c_atr_functionality; + + return atr; +} +EXPORT_SYMBOL_GPL(i2c_atr_new); + +/** + * i2c_atr_delete - Delete an I2C ATR helper. + * @atr: I2C ATR helper to be deleted. + * + * Precondition: all the adapters added with i2c_atr_add_adapter() mumst be + * removed by calling i2c_atr_del_adapter(). + */ +void i2c_atr_delete(struct i2c_atr *atr) +{ + mutex_destroy(&atr->lock); +} +EXPORT_SYMBOL_GPL(i2c_atr_delete); + +MODULE_AUTHOR("Luca Ceresoli <luca@lucaceresoli.net>"); +MODULE_DESCRIPTION("I2C Address Translator"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h new file mode 100644 index 000000000000..019816e5a50c --- /dev/null +++ b/include/linux/i2c-atr.h @@ -0,0 +1,82 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/** + * drivers/i2c/i2c-atr.h -- I2C Address Translator + * + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net> + * + * Based on i2c-mux.h + */ + +#ifndef _LINUX_I2C_ATR_H +#define _LINUX_I2C_ATR_H + +#ifdef __KERNEL__ + +#include <linux/i2c.h> +#include <linux/mutex.h> + +struct i2c_atr; + +/** + * struct i2c_atr_ops - Callbacks from ATR to the device driver. + * @select: Ask the driver to select a child bus (optional) + * @deselect: Ask the driver to deselect a child bus (optional) + * @attach_client: Notify the driver of a new device connected on a child + * bus. The driver must choose an I2C alias, configure the + * hardware to use it and return it in `alias_id`. + * @detach_client: Notify the driver of a device getting disconnected. The + * driver must configure the hardware to stop using the + * alias. + * + * All these functions return 0 on success, a negative error code otherwise. + */ +struct i2c_atr_ops { + int (*select)(struct i2c_atr *atr, u32 chan_id); + int (*deselect)(struct i2c_atr *atr, u32 chan_id); + int (*attach_client)(struct i2c_atr *atr, u32 chan_id, + const struct i2c_board_info *info, + const struct i2c_client *client, + u16 *alias_id); + void (*detach_client)(struct i2c_atr *atr, u32 chan_id, + const struct i2c_client *client); +}; + +/** + * Helper to add I2C ATR features to a device driver. + */ +struct i2c_atr { + /* private: internal use only */ + + struct i2c_adapter *parent; + struct device *dev; + const struct i2c_atr_ops *ops; + + void *priv; + + struct i2c_algorithm algo; + struct mutex lock; + int max_adapters; + + struct i2c_adapter *adapter[0]; +}; + +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev, + const struct i2c_atr_ops *ops, int max_adapters); +void i2c_atr_delete(struct i2c_atr *atr); + +static inline void i2c_atr_set_clientdata(struct i2c_atr *atr, void *data) +{ + atr->priv = data; +} + +static inline void *i2c_atr_get_clientdata(struct i2c_atr *atr) +{ + return atr->priv; +} + +int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id); +void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id); + +#endif /* __KERNEL__ */ + +#endif /* _LINUX_I2C_ATR_H */
An ATR is a device that looks similar to an i2c-mux: it has an I2C slave "upstream" port and N master "downstream" ports, and forwards transactions from upstream to the appropriate downstream port. But is is different in that the forwarded transaction has a different slave address. The address used on the upstream bus is called the "alias" and is (potentially) different from the physical slave address of the downstream chip. Add a helper file (just like i2c-mux.c for a mux or switch) to allow implementing ATR features in a device driver. The helper takes care or adapter creation/destruction and translates addresses at each transaction. Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> --- Changes RFCv2 -> v3: - fix const-related warnings Changes RFCv1 -> RFCv2: RFCv1 was implemented inside i2c-mux.c and added yet more complexity there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR features are not in a MUX and vice versa, the overlapping is low. This was almost a complete rewrite, but for the records here are the main differences from the old implementation: - change bus description - remove I2C_ATR_ARBITRATOR and I2C_ATR_GATE support - select() optional - rename i2c_atr_alloc -> i2c_atr_new, add i2c_atr_delete, move to bottom - lock the ATR, not the adapter or the muxes on the adapter - remove parent-locked code - remove I2C_MUX_LOCKED flag, now unused - remove I2C_ATR_ATR flag (always true) - translation in i2c_atr_smbus_xfer too - i2c_atr_map_msgs: don't ignore mapping errors - always require the "i2c-atr" DT node, no magic - remove ACPI support - one algo in the atrc, not one per adapter - remove unneeded i2c_atr_root_adapter - ditch force_nr - don't allocate private user memory, just provide a plain userdata pointer - consolidate all ops in a single struct, simplify naming - remove adapters individually, allocate in atrc->adapter[chan_id] --- MAINTAINERS | 7 + drivers/i2c/Kconfig | 9 + drivers/i2c/Makefile | 1 + drivers/i2c/i2c-atr.c | 557 ++++++++++++++++++++++++++++++++++++++++ include/linux/i2c-atr.h | 82 ++++++ 5 files changed, 656 insertions(+) create mode 100644 drivers/i2c/i2c-atr.c create mode 100644 include/linux/i2c-atr.h