On Mon, Feb 7, 2022 at 7:55 PM Luca Ceresoli [off-list ref] 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 [off-list ref]

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", &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 [off-list ref]

2019,2022 ?

+ * Based on i2c-mux.h
+ */

...

+#ifdef __KERNEL__

Why?

...

+#include <linux/i2c.h>
+#include <linux/mutex.h>

Missed types.h

Missed struct device;

-- 
With Best Regards,
Andy Shevchenko