On 18/01/2023 16:23, Andy Shevchenko wrote:
On Wed, Jan 18, 2023 at 02:40:25PM +0200, Tomi Valkeinen wrote:
From: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>
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
is is ?
Is is a typo, I'll fix it.
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.
...
+A typical example follows.
+
+Topology::
+
+ Slave X @ 0x10
+ .-----. |
+ .-----. | |---+---- B
+ | CPU |--A--| ATR |
+ `-----' | |---+---- C
+ `-----' |
+ Slave Y @ 0x10
+
+Alias table:
+
+.. 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
I'm not sure I got the real / virtual status of the adapters. Are the B and C
virtual ones, while A is the real?
...
+#define ATR_MAX_ADAPTERS 99 /* Just a sanity limit */
Hmm... It's not clear why this is not 100, for example, and how 99 below is
related to that, assuming channel numbering is started from 0.
+#define ATR_MAX_SYMLINK_LEN 16 /* Longest name is 10 chars: "channel-99" */
Right, it should be 100.
I think I set the ATR_MAX_SYMLINK_LEN to 16 just for alignment, but that
probably doesn't make sense and I should just set ATR_MAX_SYMLINK_LEN to
11 (10 + zero byte).
...
+ /* Ensure we have enough room to save the original addresses */
+ if (unlikely(chan->orig_addrs_size < num)) {
+ u16 *new_buf;
+
+ new_buf = kmalloc_array(num, sizeof(*new_buf), GFP_KERNEL);
I remember that I asked why we don't use krealloc_array() here... Perhaps
that we don't need to copy the old mapping table? Can we put a short comment
to clarify this in the code?
Yes, we don't care about the old data, we just require the buffer to be
large enough.
I'm not sure what kind of comment you want here. Isn't this a common
idiom, where you have a buffer for temporary data, but you might need to
resize at some point if you need a larger one?
+ if (!new_buf)
+ return -ENOMEM;
+
+ kfree(chan->orig_addrs);
+ chan->orig_addrs = new_buf;
+ chan->orig_addrs_size = num;
+ }
...
+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;
+ int ret;
+
+ if (max_adapters > ATR_MAX_ADAPTERS)
+ return ERR_PTR(-EINVAL);
+
+ if (!ops || !ops->attach_client || !ops->detach_client)
+ return ERR_PTR(-EINVAL);
+ atr = devm_kzalloc(dev, struct_size(atr, adapter, max_adapters),
+ GFP_KERNEL);
How do you know (or why do we limit) that the scope of this function will be
only in ->probe()? Even though, I would replace devm_ by non-devm_ since there
is the tear-down function has to be called by the user anyway.
That's a very good point. I don't know why devm_kzalloc is used here.
+ 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;
+
+ atr->i2c_nb.notifier_call = i2c_atr_bus_notifier_call;
+ ret = bus_register_notifier(&i2c_bus_type, &atr->i2c_nb);
+ if (ret) {
+ mutex_destroy(&atr->lock);
+ return ERR_PTR(ret);
+ }
+
+ return atr;
+}
...
+void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id)
+{
+ char symlink_name[ATR_MAX_SYMLINK_LEN];
+
Redundant blank line.
Right.
+ struct i2c_adapter *adap = atr->adapter[chan_id];
+ struct i2c_atr_chan *chan = adap->algo_data;
+ struct fwnode_handle *fwnode = dev_fwnode(&adap->dev);
+ struct device *dev = atr->dev;
+ if (!adap)
+ return;
Redundant check (it will be optimized out by compiler) or wrong assignments
above.
Indeed.
The doc for the function says "If no I2C bus has been added this
function is a no-op", so we need the check, and I need to fix the
assignments.
Although to be honest, I don't usually like this kind of "do nothing if
given wrong parameters".
Tomi
