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 ?
> 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" */
...
> + /* 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?
> + 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.
> + 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.
> + 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.
> + dev_dbg(dev, "Removing ATR child bus %d\n", i2c_adapter_id(adap));
> +
> + 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);
> +
> + atr->adapter[chan_id] = NULL;
> +
> + fwnode_handle_put(fwnode);
> + mutex_destroy(&chan->orig_addrs_lock);
> + kfree(chan->orig_addrs);
> + kfree(chan);
> +}
...
> +void i2c_atr_set_driver_data(struct i2c_atr *atr, void *data)
> +{
> + atr->priv = data;
> +}
> +EXPORT_SYMBOL_NS_GPL(i2c_atr_set_driver_data, I2C_ATR);
> +
> +void *i2c_atr_get_driver_data(struct i2c_atr *atr)
> +{
> + return atr->priv;
> +}
> +EXPORT_SYMBOL_NS_GPL(i2c_atr_get_driver_data, I2C_ATR);
Just to be sure: Is it really _driver_ data and not _device instance_ data?
--
With Best Regards,
Andy Shevchenko
