| Message ID | 20230222132907.594690-1-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
On 22/02/2023 16:17, Andy Shevchenko wrote: > On Wed, Feb 22, 2023 at 04:15:50PM +0200, Andy Shevchenko wrote: >> On Wed, Feb 22, 2023 at 03:28:59PM +0200, Tomi Valkeinen wrote: >>> Hi, >>> >>> You can find v9 from: >>> >>> https://lore.kernel.org/all/20230216140747.445477-1-tomi.valkeinen@ideasonboard.com/ >>> >>> Diff to v9 included below. >>> >>> Main changes in v10: >>> - Switch pre-increments to post-increments >>> - Add macros for FPD3_RX_ID lengths >>> - Use regmap_bulk_read in 16 bit reg accessors >> >> Thanks! >> I have no more remarks, nice job! >> >> One thing below to just look at and if you want / have time / chance update. > > ... > >>> + ret = regmap_bulk_read(priv->regmap, reg, &__v, 2); >> >> sizeof() >> >> ... >> >>> + ret = regmap_bulk_read(priv->regmap, reg, &__v, 2); >> >> Ditto. > > Here is a formal tag for patches 1, 2, 6, 7, and 8 > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thanks for the thorough reviews! I'll do the changes above. I also got kernel test bot mails about the "ID '%.*s'\n" printks (have to typecast the sizeof to int), which I also need to fix. Tomi
Hi Wolfram, On 22/02/2023 15:29, Tomi Valkeinen wrote: > From: Luca Ceresoli <luca@lucaceresoli.net> > > 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 it > 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> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > Documentation/i2c/index.rst | 1 + > Documentation/i2c/muxes/i2c-atr.rst | 97 +++++ > MAINTAINERS | 8 + > drivers/i2c/Kconfig | 9 + > drivers/i2c/Makefile | 1 + > drivers/i2c/i2c-atr.c | 548 ++++++++++++++++++++++++++++ > include/linux/i2c-atr.h | 116 ++++++ > 7 files changed, 780 insertions(+) > create mode 100644 Documentation/i2c/muxes/i2c-atr.rst > create mode 100644 drivers/i2c/i2c-atr.c > create mode 100644 include/linux/i2c-atr.h Wolfram, do you have any comments on this? Things have been calming down, I think, and I'd like to merge the series soon if nothing major comes up. The easiest way would be to merge the whole series via linux-media, as most of the patches are for media. If this looks good, can you ack it and I'll send a pull request to linux-media maintainers? Tomi > diff --git a/Documentation/i2c/index.rst b/Documentation/i2c/index.rst > index 6270f1fd7d4e..aaf33d1315f4 100644 > --- a/Documentation/i2c/index.rst > +++ b/Documentation/i2c/index.rst > @@ -16,6 +16,7 @@ Introduction > instantiating-devices > busses/index > i2c-topology > + muxes/i2c-atr > muxes/i2c-mux-gpio > i2c-sysfs > > diff --git a/Documentation/i2c/muxes/i2c-atr.rst b/Documentation/i2c/muxes/i2c-atr.rst > new file mode 100644 > index 000000000000..da226fd4de63 > --- /dev/null > +++ b/Documentation/i2c/muxes/i2c-atr.rst > @@ -0,0 +1,97 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +===================== > +Kernel driver i2c-atr > +===================== > + > +Author: Luca Ceresoli <luca@lucaceresoli.net> > +Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > + > +Description > +----------- > + > +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: > + > +A, B and C are three physical I2C busses, electrically independent from > +each other. The ATR receives the transactions initiated on bus A and > +propagates them on bus B or bus C or none depending on the device address > +in the transaction and based on the alias table. > + > +Alias table: > + > +.. table:: > + > + =============== ===== > + Client Alias > + =============== ===== > + X (bus B, 0x10) 0x20 > + Y (bus C, 0x10) 0x30 > + =============== ===== > + > +Transaction: > + > + - Slave X driver sends a transaction (on adapter B), slave address 0x10 > + - ATR driver finds slave X is on bus B and has alias 0x20, rewrites > + messages with address 0x20, forwards to adapter A > + - Physical I2C transaction on bus A, slave address 0x20 > + - ATR chip detects transaction on address 0x20, finds it in table, > + propagates transaction on bus B with address translated to 0x10, > + keeps clock streched on bus A waiting for reply > + - Slave X chip (on bus B) detects transaction at its own physical > + address 0x10 and replies normally > + - ATR chip stops clock stretching and forwards reply on bus A, > + with address translated back to 0x20 > + - ATR driver receives the reply, rewrites messages with address 0x10 > + as they were initially > + - 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 > + > +I2C ATR functions and data structures > +------------------------------------- > + > +.. kernel-doc:: include/linux/i2c-atr.h > diff --git a/MAINTAINERS b/MAINTAINERS > index 7f4678dba495..375e13ee05a2 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9688,6 +9688,14 @@ L: linux-acpi@vger.kernel.org > S: Maintained > F: drivers/i2c/i2c-core-acpi.c > > +I2C ADDRESS TRANSLATOR (ATR) > +M: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > +R: Luca Ceresoli <luca.ceresoli@bootlin.com> > +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..5ab890b83670 > --- /dev/null > +++ b/drivers/i2c/i2c-atr.c > @@ -0,0 +1,548 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * I2C Address Translator > + * > + * Copyright (c) 2019,2022 Luca Ceresoli <luca@lucaceresoli.net> > + * Copyright (c) 2022,2023 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > + * > + * Originally based on i2c-mux.c > + */ > + > +#include <linux/fwnode.h> > +#include <linux/i2c-atr.h> > +#include <linux/i2c.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/slab.h> > + > +#define ATR_MAX_ADAPTERS 100 /* Just a sanity limit */ > +#define ATR_MAX_SYMLINK_LEN 11 /* Longest name is 10 chars: "channel-99" */ > + > +/** > + * struct i2c_atr_cli2alias_pair - Holds 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; > +}; > + > +/** > + * struct i2c_atr_chan - Data for a channel. > + * @adap: The &struct i2c_adapter for the channel > + * @atr: The parent I2C ATR > + * @chan_id: The ID of this channel > + * @alias_list: List of @struct i2c_atr_cli2alias_pair containing the > + * assigned aliases > + * @orig_addrs_lock: Mutex protecting @orig_addrs > + * @orig_addrs: Buffer used to store the original addresses during transmit > + * @orig_addrs_size: Size of @orig_addrs > + */ > +struct i2c_atr_chan { > + struct i2c_adapter adap; > + struct i2c_atr *atr; > + u32 chan_id; > + > + struct list_head alias_list; > + > + /* Lock orig_addrs during xfer */ > + struct mutex orig_addrs_lock; > + u16 *orig_addrs; > + unsigned int orig_addrs_size; > +}; > + > +/** > + * struct i2c_atr - The I2C ATR instance > + * @parent: The parent &struct i2c_adapter > + * @dev: The device that owns the I2C ATR instance > + * @ops: &struct i2c_atr_ops > + * @priv: Private driver data, set with i2c_atr_set_driver_data() > + * @algo: The &struct i2c_algorithm for adapters > + * @lock: Lock for the I2C bus segment (see &struct i2c_lock_operations) > + * @max_adapters: Maximum number of adapters this I2C ATR can have > + * @adapter: Array of adapters > + */ > +struct i2c_atr { > + struct i2c_adapter *parent; > + struct device *dev; > + const struct i2c_atr_ops *ops; > + > + void *priv; > + > + struct i2c_algorithm algo; > + /* lock for the I2C bus segment (see struct i2c_lock_operations) */ > + struct mutex lock; > + int max_adapters; > + > + struct notifier_block i2c_nb; > + > + struct i2c_adapter *adapter[]; > +}; > + > +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)) { > + u16 *new_buf; > + > + /* We don't care about old data, hence no realloc() */ > + new_buf = kmalloc_array(num, sizeof(*new_buf), GFP_KERNEL); > + if (!new_buf) > + 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) { > + dev_err(atr->dev, "client 0x%02x not mapped!\n", > + msgs[i].addr); > + return -ENXIO; > + } > + > + msgs[i].addr = c2a->alias; > + } > + > + 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; > + > + /* Translate addresses */ > + mutex_lock(&chan->orig_addrs_lock); > + > + ret = i2c_atr_map_msgs(chan, msgs, num); > + if (ret < 0) > + goto err_unlock; > + > + /* Perform the transfer */ > + ret = i2c_transfer(parent, msgs, num); > + > + /* Restore addresses */ > + i2c_atr_unmap_msgs(chan, msgs, num); > + > +err_unlock: > + mutex_unlock(&chan->orig_addrs_lock); > + > + 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; > + > + 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; > + } > + > + return i2c_smbus_xfer(parent, c2a->alias, flags, read_write, command, > + size, data); > +} > + > +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_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; > + int ret; > + > + c2a = kzalloc(sizeof(*c2a), GFP_KERNEL); > + if (!c2a) > + return -ENOMEM; > + > + ret = atr->ops->attach_client(atr, chan->chan_id, client, &alias_id); > + if (ret) > + goto err_free; > + > + dev_dbg(atr->dev, "chan%u: client 0x%02x mapped at alias 0x%02x (%s)\n", > + chan->chan_id, client->addr, alias_id, client->name); > + > + c2a->client = client; > + c2a->alias = alias_id; > + list_add(&c2a->node, &chan->alias_list); > + > + return 0; > + > +err_free: > + kfree(c2a); > + > + return ret; > +} > + > +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) { > + /* This should never happen */ > + dev_warn(atr->dev, "Unable to find address mapping\n"); > + return; > + } > + > + dev_dbg(atr->dev, > + "chan%u: client 0x%02x unmapped from alias 0x%02x (%s)\n", > + chan->chan_id, client->addr, c2a->alias, client->name); > + > + list_del(&c2a->node); > + kfree(c2a); > +} > + > +static int i2c_atr_bus_notifier_call(struct notifier_block *nb, > + unsigned long event, void *device) > +{ > + struct i2c_atr *atr = container_of(nb, struct i2c_atr, i2c_nb); > + struct device *dev = device; > + struct i2c_client *client; > + u32 chan_id; > + int ret; > + > + client = i2c_verify_client(dev); > + if (!client) > + return NOTIFY_DONE; > + > + /* Is the client in one of our adapters? */ > + for (chan_id = 0; chan_id < atr->max_adapters; ++chan_id) { > + if (client->adapter == atr->adapter[chan_id]) > + break; > + } > + > + if (chan_id == atr->max_adapters) > + return NOTIFY_DONE; > + > + switch (event) { > + case BUS_NOTIFY_ADD_DEVICE: > + ret = i2c_atr_attach_client(client->adapter, client); > + if (ret) > + dev_err(atr->dev, > + "Failed to attach remote client '%s': %d\n", > + dev_name(dev), ret); > + break; > + > + case BUS_NOTIFY_DEL_DEVICE: > + i2c_atr_detach_client(client->adapter, client); > + break; > + > + default: > + break; > + } > + > + return NOTIFY_DONE; > +} > + > +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 = kzalloc(struct_size(atr, adapter, max_adapters), 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; > + > + 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; > +} > +EXPORT_SYMBOL_NS_GPL(i2c_atr_new, I2C_ATR); > + > +void i2c_atr_delete(struct i2c_atr *atr) > +{ > + bus_unregister_notifier(&i2c_bus_type, &atr->i2c_nb); > + mutex_destroy(&atr->lock); > + kfree(atr); > +} > +EXPORT_SYMBOL_NS_GPL(i2c_atr_delete, I2C_ATR); > + > +int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id, > + struct device *adapter_parent, > + struct fwnode_handle *bus_handle) > +{ > + struct i2c_adapter *parent = atr->parent; > + struct device *dev = atr->dev; > + struct i2c_atr_chan *chan; > + char symlink_name[ATR_MAX_SYMLINK_LEN]; > + int ret; > + > + if (chan_id >= atr->max_adapters) { > + dev_err(dev, "No room for more i2c-atr adapters\n"); > + 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; > + > + if (!adapter_parent) > + adapter_parent = dev; > + > + 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 = adapter_parent; > + chan->adap.retries = parent->retries; > + chan->adap.timeout = parent->timeout; > + chan->adap.quirks = parent->quirks; > + chan->adap.lock_ops = &i2c_atr_lock_ops; > + > + if (bus_handle) { > + device_set_node(&chan->adap.dev, fwnode_handle_get(bus_handle)); > + } else { > + struct fwnode_handle *atr_node; > + struct fwnode_handle *child; > + u32 reg; > + > + atr_node = device_get_named_child_node(dev, "i2c-atr"); > + > + fwnode_for_each_child_node(atr_node, child) { > + ret = fwnode_property_read_u32(child, "reg", ®); > + if (ret) > + continue; > + if (chan_id == reg) > + break; > + } > + > + device_set_node(&chan->adap.dev, child); > + fwnode_handle_put(atr_node); > + } > + > + atr->adapter[chan_id] = &chan->adap; > + > + ret = i2c_add_adapter(&chan->adap); > + if (ret) { > + dev_err(dev, "failed to add atr-adapter %u (error=%d)\n", > + chan_id, ret); > + goto err_fwnode_put; > + } > + > + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", > + chan->chan_id); > + > + ret = sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"); > + if (ret) > + dev_warn(dev, "can't create symlink to atr device\n"); > + ret = sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name); > + if (ret) > + dev_warn(dev, "can't create symlink for channel %u\n", chan_id); > + > + dev_dbg(dev, "Added ATR child bus %d\n", i2c_adapter_id(&chan->adap)); > + > + return 0; > + > +err_fwnode_put: > + fwnode_handle_put(dev_fwnode(&chan->adap.dev)); > + mutex_destroy(&chan->orig_addrs_lock); > + kfree(chan); > + return ret; > +} > +EXPORT_SYMBOL_NS_GPL(i2c_atr_add_adapter, I2C_ATR); > + > +void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id) > +{ > + char symlink_name[ATR_MAX_SYMLINK_LEN]; > + struct i2c_adapter *adap; > + struct i2c_atr_chan *chan; > + struct fwnode_handle *fwnode; > + struct device *dev = atr->dev; > + > + adap = atr->adapter[chan_id]; > + if (!adap) > + return; > + > + chan = adap->algo_data; > + fwnode = dev_fwnode(&adap->dev); > + > + 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); > +} > +EXPORT_SYMBOL_NS_GPL(i2c_atr_del_adapter, I2C_ATR); > + > +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); > + > +MODULE_AUTHOR("Luca Ceresoli <luca.ceresoli@bootlin.com>"); > +MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>"); > +MODULE_DESCRIPTION("I2C Address Translator"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h > new file mode 100644 > index 000000000000..7596f70ce1ab > --- /dev/null > +++ b/include/linux/i2c-atr.h > @@ -0,0 +1,116 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * I2C Address Translator > + * > + * Copyright (c) 2019,2022 Luca Ceresoli <luca@lucaceresoli.net> > + * Copyright (c) 2022,2023 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > + * > + * Based on i2c-mux.h > + */ > + > +#ifndef _LINUX_I2C_ATR_H > +#define _LINUX_I2C_ATR_H > + > +#include <linux/i2c.h> > +#include <linux/types.h> > + > +struct device; > +struct fwnode_handle; > +struct i2c_atr; > + > +/** > + * struct i2c_atr_ops - Callbacks from ATR to the device driver. > + * @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 (*attach_client)(struct i2c_atr *atr, u32 chan_id, > + const struct i2c_client *client, u16 *alias_id); > + void (*detach_client)(struct i2c_atr *atr, u32 chan_id, > + const struct i2c_client *client); > +}; > + > +/** > + * 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); > + > +/** > + * 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); > + > +/** > + * 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`. > + * @adapter_parent: The device used as the parent of the new i2c adapter, or NULL > + * to use the i2c-atr device as the parent. > + * @bus_handle: The fwnode handle that points to the adapter's i2c > + * peripherals, or NULL. > + * > + * After calling this function a new i2c bus will appear. Adding and removing > + * devices on the downstream bus will result in calls to the > + * &i2c_atr_ops->attach_client and &i2c_atr_ops->detach_client callbacks for the > + * driver to assign an alias to the device. > + * > + * The adapter's fwnode is set to @bus_handle, or if @bus_handle is NULL the > + * function looks for a child node whose 'reg' property matches the chan_id > + * under the i2c-atr device's 'i2c-atr' node. > + * > + * 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 device *adapter_parent, > + struct fwnode_handle *bus_handle); > + > +/** > + * i2c_atr_del_adapter - Remove a child ("downstream") I2C bus added by > + * i2c_atr_add_adapter(). If no I2C bus has been added > + * this function is a no-op. > + * @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); > + > +/** > + * i2c_atr_set_driver_data - Set private driver data to the i2c-atr instance. > + * @atr: The I2C ATR > + * @data: Pointer to the data to store > + */ > +void i2c_atr_set_driver_data(struct i2c_atr *atr, void *data); > + > +/** > + * i2c_atr_get_driver_data - Get the stored drive data. > + * @atr: The I2C ATR > + * > + * Return: Pointer to the stored data > + */ > +void *i2c_atr_get_driver_data(struct i2c_atr *atr); > + > +#endif /* _LINUX_I2C_ATR_H */
Some inline comments below. Regards Matthias Am 22.02.23 um 14:29 schrieb Tomi Valkeinen: > From: Luca Ceresoli <luca@lucaceresoli.net> > > 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 it > 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> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > Documentation/i2c/index.rst | 1 + > Documentation/i2c/muxes/i2c-atr.rst | 97 +++++ > MAINTAINERS | 8 + > drivers/i2c/Kconfig | 9 + > drivers/i2c/Makefile | 1 + > drivers/i2c/i2c-atr.c | 548 ++++++++++++++++++++++++++++ > include/linux/i2c-atr.h | 116 ++++++ > 7 files changed, 780 insertions(+) > create mode 100644 Documentation/i2c/muxes/i2c-atr.rst > create mode 100644 drivers/i2c/i2c-atr.c > create mode 100644 include/linux/i2c-atr.h > [...] > diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c > new file mode 100644 > index 000000000000..5ab890b83670 > --- /dev/null > +++ b/drivers/i2c/i2c-atr.c > @@ -0,0 +1,548 @@ [...] > + > +/* > + * 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)) { > + u16 *new_buf; > + > + /* We don't care about old data, hence no realloc() */ > + new_buf = kmalloc_array(num, sizeof(*new_buf), GFP_KERNEL); > + if (!new_buf) > + 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) { > + dev_err(atr->dev, "client 0x%02x not mapped!\n", > + msgs[i].addr); > + return -ENXIO; I miss the roll-back of previously modified msgs[].addr values. > + } > + > + msgs[i].addr = c2a->alias; > + } > + > + 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]; Does this code needs null and size checks for orig_addrs/orig_addrs_size to protect from oopses? This cannot happen now as i2c_atr_master_xfer returns early when i2c_atr_map_msgs fails. > +} > + > +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; > + > + /* Translate addresses */ > + mutex_lock(&chan->orig_addrs_lock); > + > + ret = i2c_atr_map_msgs(chan, msgs, num); > + if (ret < 0) > + goto err_unlock; > + > + /* Perform the transfer */ > + ret = i2c_transfer(parent, msgs, num); > + > + /* Restore addresses */ > + i2c_atr_unmap_msgs(chan, msgs, num); > + > +err_unlock: > + mutex_unlock(&chan->orig_addrs_lock); > + > + return ret; > +} > + [...]
Hello Matthias, thanks for the in-depth review! On Mon, 20 Mar 2023 07:34:34 +0100 zzam@gentoo.org wrote: > Some inline comments below. > > Regards > Matthias > > Am 22.02.23 um 14:29 schrieb Tomi Valkeinen: > > From: Luca Ceresoli <luca@lucaceresoli.net> > > > > 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 it > > 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> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > --- > > Documentation/i2c/index.rst | 1 + > > Documentation/i2c/muxes/i2c-atr.rst | 97 +++++ > > MAINTAINERS | 8 + > > drivers/i2c/Kconfig | 9 + > > drivers/i2c/Makefile | 1 + > > drivers/i2c/i2c-atr.c | 548 ++++++++++++++++++++++++++++ > > include/linux/i2c-atr.h | 116 ++++++ > > 7 files changed, 780 insertions(+) > > create mode 100644 Documentation/i2c/muxes/i2c-atr.rst > > create mode 100644 drivers/i2c/i2c-atr.c > > create mode 100644 include/linux/i2c-atr.h > > > [...] > > diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c > > new file mode 100644 > > index 000000000000..5ab890b83670 > > --- /dev/null > > +++ b/drivers/i2c/i2c-atr.c > > @@ -0,0 +1,548 @@ > [...] > > + > > +/* > > + * 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)) { > > + u16 *new_buf; > > + > > + /* We don't care about old data, hence no realloc() */ > > + new_buf = kmalloc_array(num, sizeof(*new_buf), GFP_KERNEL); > > + if (!new_buf) > > + 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) { > > + dev_err(atr->dev, "client 0x%02x not mapped!\n", > > + msgs[i].addr); > > + return -ENXIO; > I miss the roll-back of previously modified msgs[].addr values. Indeed you have a point. There is a subtle error in case all of the following happen in a single i2c_atr_master_xfer() call: * there are 2+ messages, having different addresses * msg[0] is mapped correctly * msg[n] (n > 0) fails mapping It's very unlikely, but in this case we'd get back to the caller with an error and modified addresses for the first n messages. Which in turn is unlikely to create any problems, but it could. Tomi, do you agree? This looks like a simple solution: if (!c2a) { + i2c_atr_unmap_msgs(chan, msgs, i); ... } While there, maybe switching to dev_err_probe would make code cleaner. > > +/* > > + * 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]; > Does this code needs null and size checks for orig_addrs/orig_addrs_size > to protect from oopses? > This cannot happen now as i2c_atr_master_xfer returns early when > i2c_atr_map_msgs fails. The map/unmap functions are really a part of i2c_atr_master_xfer() that has been extracted for code readability, as the comments say, and I can't think of a different use for them. So I think this code is OK as is. However a small comment might help future readers, especially in case code will change and these functions gain new use cases. E.g. This function is internal for use in i2c_atr_master_xfer() + and for this reason it needs no null and size checks on orig_addr. It must be followed by i2c_atr_unmap_msgs() to restore the original addresses. Regards, Luca
On 20/03/2023 10:28, Luca Ceresoli wrote: > Hello Matthias, > > thanks for the in-depth review! > > On Mon, 20 Mar 2023 07:34:34 +0100 > zzam@gentoo.org wrote: > >> Some inline comments below. >> >> Regards >> Matthias >> >> Am 22.02.23 um 14:29 schrieb Tomi Valkeinen: >>> From: Luca Ceresoli <luca@lucaceresoli.net> >>> >>> 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 it >>> 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> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> --- >>> Documentation/i2c/index.rst | 1 + >>> Documentation/i2c/muxes/i2c-atr.rst | 97 +++++ >>> MAINTAINERS | 8 + >>> drivers/i2c/Kconfig | 9 + >>> drivers/i2c/Makefile | 1 + >>> drivers/i2c/i2c-atr.c | 548 ++++++++++++++++++++++++++++ >>> include/linux/i2c-atr.h | 116 ++++++ >>> 7 files changed, 780 insertions(+) >>> create mode 100644 Documentation/i2c/muxes/i2c-atr.rst >>> create mode 100644 drivers/i2c/i2c-atr.c >>> create mode 100644 include/linux/i2c-atr.h >>> >> [...] >>> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c >>> new file mode 100644 >>> index 000000000000..5ab890b83670 >>> --- /dev/null >>> +++ b/drivers/i2c/i2c-atr.c >>> @@ -0,0 +1,548 @@ >> [...] >>> + >>> +/* >>> + * 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)) { >>> + u16 *new_buf; >>> + >>> + /* We don't care about old data, hence no realloc() */ >>> + new_buf = kmalloc_array(num, sizeof(*new_buf), GFP_KERNEL); >>> + if (!new_buf) >>> + 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) { >>> + dev_err(atr->dev, "client 0x%02x not mapped!\n", >>> + msgs[i].addr); >>> + return -ENXIO; >> I miss the roll-back of previously modified msgs[].addr values. > > Indeed you have a point. There is a subtle error in case all of the > following happen in a single i2c_atr_master_xfer() call: > > * there are 2+ messages, having different addresses > * msg[0] is mapped correctly > * msg[n] (n > 0) fails mapping > > It's very unlikely, but in this case we'd get back to the caller with > an error and modified addresses for the first n messages. Which in turn > is unlikely to create any problems, but it could. > > Tomi, do you agree? > > This looks like a simple solution: > > if (!c2a) { > + i2c_atr_unmap_msgs(chan, msgs, i); > ... > } Wouldn't that possibly restore the address from orig_addrs[x] also for messages we haven't handled yet? I think a simple while (i--) msgs[i].addr = chan->orig_addrs[i]; should do here. It is also, perhaps, a bit more clear this way, as you can see the assignments to msgs[i].addr nearby, and the rollback here with the above code. Instead of seeing a call to an unmap function, having to go and see what exactly it will do. > While there, maybe switching to dev_err_probe would make code cleaner. The while loop above has to be done after the print, if we use the same i variable in both. dev_err_probe could still be used, but... I don't know if it's worth trying to push it in. >>> +/* >>> + * 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]; >> Does this code needs null and size checks for orig_addrs/orig_addrs_size >> to protect from oopses? >> This cannot happen now as i2c_atr_master_xfer returns early when >> i2c_atr_map_msgs fails. > > The map/unmap functions are really a part of i2c_atr_master_xfer() that > has been extracted for code readability, as the comments say, and I > can't think of a different use for them. So I think this code is OK as > is. > > However a small comment might help future readers, especially in case > code will change and these functions gain new use cases. > E.g. > > This function is internal for use in i2c_atr_master_xfer() > + and for this reason it needs no null and size checks on orig_addr. > It must be followed by i2c_atr_unmap_msgs() to restore the original addresses. I can add a comment. as Luca said, it's an internal helper function, I don't think we need to check the parameters there for cases which can't happen. Tomi
Hi Tomi, > Wolfram, do you have any comments on this? Not yet. I need to dive into the previous discussions again to understand what we agreed on and what potential problems we had to face. However, holiday season is near, it could be that I won't have really time for this until Mid-April or so. I'll try earlier but no promises :/ > Things have been calming down, I think, and I'd like to merge the series > soon if nothing major comes up. The easiest way would be to merge the whole > series via linux-media, as most of the patches are for media. If this looks > good, can you ack it and I'll send a pull request to linux-media > maintainers? I'd think this is a too elemental (is this a word?) change for someone else to pull it. But no worries, I would offer an immutable branch right when I am done with reviewing so other subsystems can pull it. Or are there other technical reasons I missed? Sorry for not having better news, Wolfram
Hi Wolfram, On 20/03/2023 19:00, Wolfram Sang wrote: > Hi Tomi, > >> Wolfram, do you have any comments on this? > > Not yet. I need to dive into the previous discussions again to > understand what we agreed on and what potential problems we had to face. > However, holiday season is near, it could be that I won't have really > time for this until Mid-April or so. I'll try earlier but no promises :/ > >> Things have been calming down, I think, and I'd like to merge the series >> soon if nothing major comes up. The easiest way would be to merge the whole >> series via linux-media, as most of the patches are for media. If this looks >> good, can you ack it and I'll send a pull request to linux-media >> maintainers? > > I'd think this is a too elemental (is this a word?) change for someone > else to pull it. But no worries, I would offer an immutable branch right > when I am done with reviewing so other subsystems can pull it. Or are > there other technical reasons I missed? An immutable branch is fine too. Tomi
Hi Tomi, On Mon, 20 Mar 2023 14:12:32 +0200 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > On 20/03/2023 10:28, Luca Ceresoli wrote: > > Hello Matthias, > > > > thanks for the in-depth review! > > > > On Mon, 20 Mar 2023 07:34:34 +0100 > > zzam@gentoo.org wrote: > > > >> Some inline comments below. > >> > >> Regards > >> Matthias > >> > >> Am 22.02.23 um 14:29 schrieb Tomi Valkeinen: > >>> From: Luca Ceresoli <luca@lucaceresoli.net> > >>> > >>> 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 it > >>> 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> > >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >>> --- > >>> Documentation/i2c/index.rst | 1 + > >>> Documentation/i2c/muxes/i2c-atr.rst | 97 +++++ > >>> MAINTAINERS | 8 + > >>> drivers/i2c/Kconfig | 9 + > >>> drivers/i2c/Makefile | 1 + > >>> drivers/i2c/i2c-atr.c | 548 ++++++++++++++++++++++++++++ > >>> include/linux/i2c-atr.h | 116 ++++++ > >>> 7 files changed, 780 insertions(+) > >>> create mode 100644 Documentation/i2c/muxes/i2c-atr.rst > >>> create mode 100644 drivers/i2c/i2c-atr.c > >>> create mode 100644 include/linux/i2c-atr.h > >>> > >> [...] > >>> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c > >>> new file mode 100644 > >>> index 000000000000..5ab890b83670 > >>> --- /dev/null > >>> +++ b/drivers/i2c/i2c-atr.c > >>> @@ -0,0 +1,548 @@ > >> [...] > >>> + > >>> +/* > >>> + * 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)) { > >>> + u16 *new_buf; > >>> + > >>> + /* We don't care about old data, hence no realloc() */ > >>> + new_buf = kmalloc_array(num, sizeof(*new_buf), GFP_KERNEL); > >>> + if (!new_buf) > >>> + 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) { > >>> + dev_err(atr->dev, "client 0x%02x not mapped!\n", > >>> + msgs[i].addr); > >>> + return -ENXIO; > >> I miss the roll-back of previously modified msgs[].addr values. > > > > Indeed you have a point. There is a subtle error in case all of the > > following happen in a single i2c_atr_master_xfer() call: > > > > * there are 2+ messages, having different addresses > > * msg[0] is mapped correctly > > * msg[n] (n > 0) fails mapping > > > > It's very unlikely, but in this case we'd get back to the caller with > > an error and modified addresses for the first n messages. Which in turn > > is unlikely to create any problems, but it could. > > > > Tomi, do you agree? > > > > This looks like a simple solution: > > > > if (!c2a) { > > + i2c_atr_unmap_msgs(chan, msgs, i); > > ... > > } > > Wouldn't that possibly restore the address from orig_addrs[x] also for > messages we haven't handled yet? No, because there is 'i' as the 3rd argument, not 'num'. But... > > I think a simple > > while (i--) > msgs[i].addr = chan->orig_addrs[i]; > > should do here. It is also, perhaps, a bit more clear this way, as you > can see the assignments to msgs[i].addr nearby, and the rollback here > with the above code. Instead of seeing a call to an unmap function, > having to go and see what exactly it will do. ...sure, this would work. If I had connected my brain at the appropriate time I would have realized it's two lines only. And definitely less spaghetti-coded that what I had suggested. Luca
> + i2c-alias-pool: > + $ref: /schemas/types.yaml#/definitions/uint16-array > + description: > + I2C alias pool is a pool of I2C addresses on the main I2C bus that can be > + used to access the remote peripherals on the serializer's I2C bus. The > + addresses must be available, not used by any other peripheral. Each > + remote peripheral is assigned an alias from the pool, and transactions to > + that address will be forwarded to the remote peripheral, with the address > + translated to the remote peripheral's real address. This property is not > + needed if there are no I2C addressable remote peripherals. After some initial discussion with Tomi on IRC, this question is probably more for Luca: Why is "i2c-alias-pool" in the drivers binding and not a regular i2c binding? Same question for the implementation of the alias-pool handling. Shouldn't this be in the i2c-atr library? I'd think managing the list of aliases would look all the same in the drivers otherwise?
Hi Tomi, hi Luca, as mentioned on IRC already, good move to use bus notifiers here and drop the generic attach/detach callbacks. Those were a show stopper for me. This version is nicely self contained. I like that! > diff --git a/Documentation/i2c/index.rst b/Documentation/i2c/index.rst > index 6270f1fd7d4e..aaf33d1315f4 100644 > --- a/Documentation/i2c/index.rst > +++ b/Documentation/i2c/index.rst > @@ -16,6 +16,7 @@ Introduction > instantiating-devices > busses/index > i2c-topology > + muxes/i2c-atr The muxes-dir is only for the description of mux drivers. I'd prefer to have this document not in the sub-dir. Also, renaming the document to "address-translations.rst" might be worth discussing. > muxes/i2c-mux-gpio > i2c-sysfs > > diff --git a/Documentation/i2c/muxes/i2c-atr.rst b/Documentation/i2c/muxes/i2c-atr.rst > new file mode 100644 > index 000000000000..da226fd4de63 > --- /dev/null > +++ b/Documentation/i2c/muxes/i2c-atr.rst > @@ -0,0 +1,97 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +===================== > +Kernel driver i2c-atr Maybe "I2C address translations"? > +===================== > + > +Author: Luca Ceresoli <luca@lucaceresoli.net> > +Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > + > +Description > +----------- > + > +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 I'd like to get rid of all "your". Maybe "client driver" here? > +driver. ... > +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 Remove all "your", please. Some can simply go, I'd say. The others replaced by "the". > + > +I2C ATR functions and data structures > +------------------------------------- > + ... > +/** > + * struct i2c_atr_cli2alias_pair - Holds the alias assigned to a client. I stumbled over this one because "cli" is "command line interface" for me... The long version isn't much longer: 'i2c_atr_client_alias_pair' But I'd be also fine with: 'i2c_atr_alias_pair' > + * @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. > + */ > +EXPORT_SYMBOL_NS_GPL(i2c_atr_add_adapter, I2C_ATR); EXPORT_SYMBOL_GPL, please. We can then later think about using an I2C namespace for all I2C symbols. Pretty high level comments only so far. I'll keep at it this week and might come back with more detailed comments. But in general, this looks quite good to go. Moving the alias pool handling to here is the biggest question I have. Thank you for your patience! Wolfram
Hi Wolfram, Tomi, On Tue, 18 Apr 2023 16:25:02 +0200 Wolfram Sang <wsa@kernel.org> wrote: > Hi Tomi, hi Luca, > > as mentioned on IRC already, good move to use bus notifiers here and > drop the generic attach/detach callbacks. Those were a show stopper for > me. This version is nicely self contained. I like that! > > > diff --git a/Documentation/i2c/index.rst b/Documentation/i2c/index.rst > > index 6270f1fd7d4e..aaf33d1315f4 100644 > > --- a/Documentation/i2c/index.rst > > +++ b/Documentation/i2c/index.rst > > @@ -16,6 +16,7 @@ Introduction > > instantiating-devices > > busses/index > > i2c-topology > > + muxes/i2c-atr > > The muxes-dir is only for the description of mux drivers. I'd prefer to > have this document not in the sub-dir. Also, renaming the document to > "address-translations.rst" might be worth discussing. > > > muxes/i2c-mux-gpio > > i2c-sysfs > > > > diff --git a/Documentation/i2c/muxes/i2c-atr.rst b/Documentation/i2c/muxes/i2c-atr.rst > > new file mode 100644 > > index 000000000000..da226fd4de63 > > --- /dev/null > > +++ b/Documentation/i2c/muxes/i2c-atr.rst > > @@ -0,0 +1,97 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > +===================== > > +Kernel driver i2c-atr > > Maybe "I2C address translations"? Even better: "I2C address translator dirvers", or just "I2C address translators"? Here we document a facility to implement a driver for an address translator, and discussion on "address translation" in general is just a part of it. Same for the filename. Uh, I guess this was a journey in the realm of nitpicking, sorry... :) > > +Description > > +----------- > > + > > +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 > > I'd like to get rid of all "your". Maybe "client driver" here? I agree. > > + > > +I2C ATR functions and data structures > > +------------------------------------- > > + > > ... > > > +/** > > + * struct i2c_atr_cli2alias_pair - Holds the alias assigned to a client. > > I stumbled over this one because "cli" is "command line interface" for > me... The long version isn't much longer: 'i2c_atr_client_alias_pair' > But I'd be also fine with: 'i2c_atr_alias_pair' The short one looks better to me. The only "alias" involved in ATRs is the client alias, thus no ambiguity. Best regards, Luca
Hi Wolfram, Tomi, On Tue, 18 Apr 2023 15:06:10 +0200 Wolfram Sang <wsa@kernel.org> wrote: > > + i2c-alias-pool: > > + $ref: /schemas/types.yaml#/definitions/uint16-array > > + description: > > + I2C alias pool is a pool of I2C addresses on the main I2C bus that can be > > + used to access the remote peripherals on the serializer's I2C bus. The > > + addresses must be available, not used by any other peripheral. Each > > + remote peripheral is assigned an alias from the pool, and transactions to > > + that address will be forwarded to the remote peripheral, with the address > > + translated to the remote peripheral's real address. This property is not > > + needed if there are no I2C addressable remote peripherals. > > After some initial discussion with Tomi on IRC, this question is > probably more for Luca: > > Why is "i2c-alias-pool" in the drivers binding and not a regular i2c > binding? Same question for the implementation of the alias-pool > handling. Shouldn't this be in the i2c-atr library? I'd think managing > the list of aliases would look all the same in the drivers otherwise? I think that this _was_ the plan, as it looks obviously cleaner, but then we agreed that we should remove the pool entirely, so I didn't bother moving it. Best regards, Luca
> > Why is "i2c-alias-pool" in the drivers binding and not a regular i2c > > binding? Same question for the implementation of the alias-pool > > handling. Shouldn't this be in the i2c-atr library? I'd think managing > > the list of aliases would look all the same in the drivers otherwise? > > I think that this _was_ the plan, as it looks obviously cleaner, but > then we agreed that we should remove the pool entirely, so I didn't > bother moving it. Ah, you mean we agreed on that at the Plumbers BoF? I think we can conclude this is obsolete meanwhile. GMSL encodes the target addresses in DT. Rob is also fine with the binding here to encode the pool in DT. Let's follow that road, I'd say.
Hi Wolfram, On Wed, 19 Apr 2023 10:05:54 +0200 Wolfram Sang <wsa@kernel.org> wrote: > > > Why is "i2c-alias-pool" in the drivers binding and not a regular i2c > > > binding? Same question for the implementation of the alias-pool > > > handling. Shouldn't this be in the i2c-atr library? I'd think managing > > > the list of aliases would look all the same in the drivers otherwise? > > > > I think that this _was_ the plan, as it looks obviously cleaner, but > > then we agreed that we should remove the pool entirely, so I didn't > > bother moving it. > > Ah, you mean we agreed on that at the Plumbers BoF? I think we can > conclude this is obsolete meanwhile. GMSL encodes the target addresses > in DT. Rob is also fine with the binding here to encode the pool in DT. > Let's follow that road, I'd say. Sure, I'm not questioning that. Apologies if it did look like. I was just trying to explain (to myself as well) why this hadn't been done previously. Best regards, Luca
Hi Luca, > > Ah, you mean we agreed on that at the Plumbers BoF? I think we can > > conclude this is obsolete meanwhile. GMSL encodes the target addresses > > in DT. Rob is also fine with the binding here to encode the pool in DT. > > Let's follow that road, I'd say. > > Sure, I'm not questioning that. Apologies if it did look like. I was > just trying to explain (to myself as well) why this hadn't been done > previously. All fine. I didn't understand it this way and mainly wanted to summarize what happened since the BoF. To myself as well ;) And also for Tomi so he knows what happened.
On 18/04/2023 16:06, Wolfram Sang wrote: > >> + i2c-alias-pool: >> + $ref: /schemas/types.yaml#/definitions/uint16-array >> + description: >> + I2C alias pool is a pool of I2C addresses on the main I2C bus that can be >> + used to access the remote peripherals on the serializer's I2C bus. The >> + addresses must be available, not used by any other peripheral. Each >> + remote peripheral is assigned an alias from the pool, and transactions to >> + that address will be forwarded to the remote peripheral, with the address >> + translated to the remote peripheral's real address. This property is not >> + needed if there are no I2C addressable remote peripherals. > > After some initial discussion with Tomi on IRC, this question is > probably more for Luca: > > Why is "i2c-alias-pool" in the drivers binding and not a regular i2c Where should be the binding documented? A new Documentation/devicetree/bindings/i2c/i2c-atr.yaml file that only contains the i2c-alias-pool? > binding? Same question for the implementation of the alias-pool > handling. Shouldn't this be in the i2c-atr library? I'd think managing > the list of aliases would look all the same in the drivers otherwise? I think this is fine, but I also think that we need to keep the door open to other kinds of alias management. We only have a single user for this for now. A driver/device might have other requirements for its i2c-atr. Say, a pool per link, or perhaps runtime events may affect the pool. If we dictate the use of i2c-alias-pool property and the i2c-atr will automatically get an alias from that pool, i2c-atr won't be usable for the hypothetical drivers that have other needs. With that in mind the current binding and i2c-atr.c is safe, as the i2c-atr.c isn't even aware of the pool. We can easily re-arrange the code later if and when we get more users and understand their needs. But the bindings are important to get right(-ish) now. So: - Is the "i2c-alias-pool" property a driver property or a common property for all drivers using i2c-atr? - It the property mandatory or optional? It must be optional, as a setup (meaning, e.g., what cameras you happen to connect) might not have any i2c addressable remote devices, in which case the driver doesn't even need i2c-atr (even if it supports i2c-atr). But is it optional even in the case where the driver needs i2c-atr? In other words, do we allow some other way to manage the aliases? How does this sound: - If "i2c-alias-pool" is present in the DT data of the device passed to i2c_atr_new(), i2c_atr_new() will parse the property. i2c-atr.c will export functions to get a new alias and to release a previously reserved alias. The driver can use those functions in attach/detach_client() callbacks. In other words, the alias pool management wouldn't be fully automatic inside the i2c-atr, but it would provide helpers for the driver to do the common work. - If "i2c-alias-pool" is not present, i2c-atr.c will behave as it does now, and expects the driver to manage the aliases. Also, looking at the ub960 code... I don't think this will simplify the attach/detach_client callbacks much. Most of the code in those functions is about managing the UB960's registers related to ATR, not managing the address pool itself. However, it will remove the probe time "i2c-alias-pool" parsing from the driver, which is nice. Tomi
Hi Tomi, > How does this sound: > > - If "i2c-alias-pool" is present in the DT data of the device passed to > i2c_atr_new(), i2c_atr_new() will parse the property. i2c-atr.c will export > functions to get a new alias and to release a previously reserved alias. The > driver can use those functions in attach/detach_client() callbacks. In other > words, the alias pool management wouldn't be fully automatic inside the > i2c-atr, but it would provide helpers for the driver to do the common work. > > - If "i2c-alias-pool" is not present, i2c-atr.c will behave as it does now, > and expects the driver to manage the aliases. So, how does a driver manage the aliases without a pool of available addresses? I can't imagine another way right now. In general, your above proposal sounds good to me. With my lack of imagination regarding a different alias handling, I could also see that i2c-atr already provides the alias to the attach callback. But if you teach me another way of alias handling, then I could agree that your proposal makes sense as is. And yes, the "i2c-alias-pool" is definately optional. Happy hacking, Wolfram
On 20/04/2023 21:47, Wolfram Sang wrote: > Hi Tomi, > >> How does this sound: >> >> - If "i2c-alias-pool" is present in the DT data of the device passed to >> i2c_atr_new(), i2c_atr_new() will parse the property. i2c-atr.c will export >> functions to get a new alias and to release a previously reserved alias. The >> driver can use those functions in attach/detach_client() callbacks. In other >> words, the alias pool management wouldn't be fully automatic inside the >> i2c-atr, but it would provide helpers for the driver to do the common work. >> >> - If "i2c-alias-pool" is not present, i2c-atr.c will behave as it does now, >> and expects the driver to manage the aliases. > > So, how does a driver manage the aliases without a pool of available > addresses? I can't imagine another way right now. > > In general, your above proposal sounds good to me. With my lack of > imagination regarding a different alias handling, I could also see that > i2c-atr already provides the alias to the attach callback. But if you > teach me another way of alias handling, then I could agree that your > proposal makes sense as is. Oh, my imagination doesn't go so far to give you concrete examples. If the driver has to do runtime decisions on the pool, a fixed pool handled by the i2c-atr won't work. But why exactly would there be runtime events affecting the pool... I don't know. Maybe that doesn't matter here. We can start with the i2c-atr providing the alias to the attach callback, and if we ever need something else, the (kernel internal) API can be changed accordingly. The DT bindings should be fine in either case. Tomi
diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c index a41e941ad972..5ab890b83670 100644 --- a/drivers/i2c/i2c-atr.c +++ b/drivers/i2c/i2c-atr.c @@ -298,8 +298,11 @@ static void i2c_atr_detach_client(struct i2c_adapter *adapter, atr->ops->detach_client(atr, chan->chan_id, client); c2a = i2c_atr_find_mapping_by_client(&chan->alias_list, client); - if (!c2a) - return; /* This shouldn't happen */ + if (!c2a) { + /* This should never happen */ + dev_warn(atr->dev, "Unable to find address mapping\n"); + return; + } dev_dbg(atr->dev, "chan%u: client 0x%02x unmapped from alias 0x%02x (%s)\n", diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c index f7696bce7c77..203f7cceae23 100644 --- a/drivers/media/i2c/ds90ub913.c +++ b/drivers/media/i2c/ds90ub913.c @@ -108,7 +108,7 @@ static const struct ub913_format_info *ub913_find_format(u32 incode) { unsigned int i; - for (i = 0; i < ARRAY_SIZE(ub913_formats); ++i) { + for (i = 0; i < ARRAY_SIZE(ub913_formats); i++) { if (ub913_formats[i].incode == incode) return &ub913_formats[i]; } @@ -276,7 +276,7 @@ static int _ub913_set_routing(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, struct v4l2_subdev_krouting *routing) { - static const struct v4l2_mbus_framefmt informat = { + static const struct v4l2_mbus_framefmt in_format = { .width = 640, .height = 480, .code = MEDIA_BUS_FMT_UYVY8_2X8, @@ -286,7 +286,7 @@ static int _ub913_set_routing(struct v4l2_subdev *sd, .quantization = V4L2_QUANTIZATION_LIM_RANGE, .xfer_func = V4L2_XFER_FUNC_SRGB, }; - static const struct v4l2_mbus_framefmt outformat = { + static const struct v4l2_mbus_framefmt out_format = { .width = 640, .height = 480, .code = MEDIA_BUS_FMT_UYVY8_1X16, @@ -319,11 +319,11 @@ static int _ub913_set_routing(struct v4l2_subdev *sd, stream_configs = &state->stream_configs; - for (i = 0; i < stream_configs->num_configs; ++i) { + for (i = 0; i < stream_configs->num_configs; i++) { if (stream_configs->configs[i].pad == UB913_PAD_SINK) - stream_configs->configs[i].fmt = informat; + stream_configs->configs[i].fmt = in_format; else - stream_configs->configs[i].fmt = outformat; + stream_configs->configs[i].fmt = out_format; } return 0; @@ -374,7 +374,7 @@ static int ub913_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, if (route->source_pad != pad) continue; - for (i = 0; i < source_fd.num_entries; ++i) { + for (i = 0; i < source_fd.num_entries; i++) { if (source_fd.entry[i].stream == route->sink_stream) break; } diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c index 6efe1460c976..a77908da5c38 100644 --- a/drivers/media/i2c/ds90ub953.c +++ b/drivers/media/i2c/ds90ub953.c @@ -87,6 +87,7 @@ #define UB953_REG_IND_ACC_DATA 0xb2 #define UB953_REG_FPD3_RX_ID(n) (0xf0 + (n)) +#define UB953_REG_FPD3_RX_ID_LEN 6 /* Indirect register blocks */ #define UB953_IND_TARGET_PAT_GEN 0x00 @@ -272,14 +273,14 @@ static int ub953_write_ind(struct ub953_data *priv, u8 block, u8 reg, u8 val) ret = ub953_select_ind_reg_block(priv, block); if (ret) - goto out; + goto out_unlock; ret = regmap_write(priv->regmap, UB953_REG_IND_ACC_ADDR, reg); if (ret) { dev_err(&priv->client->dev, "Write to IND_ACC_ADDR failed when writing %u:%x02x: %d\n", block, reg, ret); - goto out; + goto out_unlock; } ret = regmap_write(priv->regmap, UB953_REG_IND_ACC_DATA, val); @@ -289,7 +290,7 @@ static int ub953_write_ind(struct ub953_data *priv, u8 block, u8 reg, u8 val) block, reg, ret); } -out: +out_unlock: mutex_unlock(&priv->reg_lock); return ret; @@ -498,7 +499,7 @@ static int ub953_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, if (route->source_pad != pad) continue; - for (i = 0; i < source_fd.num_entries; ++i) { + for (i = 0; i < source_fd.num_entries; i++) { if (source_fd.entry[i].stream == route->sink_stream) { source_entry = &source_fd.entry[i]; break; @@ -591,16 +592,15 @@ static int ub953_log_status(struct v4l2_subdev *sd) struct device *dev = &priv->client->dev; u8 v = 0, v1 = 0, v2 = 0; unsigned int i; - char id[7]; + char id[UB953_REG_FPD3_RX_ID_LEN]; u8 gpio_local_data; u8 gpio_input_ctrl; u8 gpio_pin_sts; - for (i = 0; i < 6; ++i) + for (i = 0; i < sizeof(id); i++) ub953_read(priv, UB953_REG_FPD3_RX_ID(i), &id[i]); - id[6] = 0; - dev_info(dev, "ID '%s'\n", id); + dev_info(dev, "ID '%.*s'\n", sizeof(id), id); ub953_read(priv, UB953_REG_GENERAL_STATUS, &v); dev_info(dev, "GENERAL_STATUS %#02x\n", v); @@ -638,7 +638,7 @@ static int ub953_log_status(struct v4l2_subdev *sd) ub953_read(priv, UB953_REG_GPIO_INPUT_CTRL, &gpio_input_ctrl); ub953_read(priv, UB953_REG_GPIO_PIN_STS, &gpio_pin_sts); - for (i = 0; i < UB953_NUM_GPIOS; ++i) { + for (i = 0; i < UB953_NUM_GPIOS; i++) { dev_info(dev, "GPIO%u: remote: %u is_input: %u is_output: %u val: %u sts: %u\n", i, diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index 1ae0c7cda7c7..c9dfe2ea0dfb 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -7,7 +7,7 @@ */ /* - * (Possible) TODOs + * (Possible) TODOs: * * - PM for serializer and remote peripherals. We need to manage: * - VPOC @@ -347,6 +347,7 @@ #define UB960_RR_CHANNEL_MODE 0xe4 /* UB9702 */ #define UB960_SR_FPD3_RX_ID(n) (0xf0 + (n)) +#define UB960_SR_FPD3_RX_ID_LEN 6 #define UB960_SR_I2C_RX_ID(n) (0xf8 + (n)) /* < UB960_FPD_RX_NPORTS */ @@ -487,10 +488,11 @@ struct ub960_txport { struct atr_alias_table_entry { u16 alias_id; /* Alias ID from DT */ - bool in_use; u8 nport; u8 slave_id; /* i2c client's local i2c address */ u8 port_reg_idx; + + bool in_use; }; struct ub960_data { @@ -595,7 +597,7 @@ static const struct ub960_format_info *ub960_find_format(u32 code) { unsigned int i; - for (i = 0; i < ARRAY_SIZE(ub960_formats); ++i) { + for (i = 0; i < ARRAY_SIZE(ub960_formats); i++) { if (ub960_formats[i].code == code) return &ub960_formats[i]; } @@ -667,26 +669,19 @@ static int ub960_update_bits(struct ub960_data *priv, u8 reg, u8 mask, u8 val) static int ub960_read16(struct ub960_data *priv, u8 reg, u16 *val) { struct device *dev = &priv->client->dev; - unsigned int v1, v2; + __be16 __v; int ret; mutex_lock(&priv->reg_lock); - ret = regmap_read(priv->regmap, reg, &v1); + ret = regmap_bulk_read(priv->regmap, reg, &__v, 2); if (ret) { dev_err(dev, "%s: cannot read register 0x%02x (%d)!\n", __func__, reg, ret); goto out_unlock; } - ret = regmap_read(priv->regmap, reg + 1, &v2); - if (ret) { - dev_err(dev, "%s: cannot read register 0x%02x (%d)!\n", - __func__, reg + 1, ret); - goto out_unlock; - } - - *val = (v1 << 8) | v2; + *val = be16_to_cpu(__v); out_unlock: mutex_unlock(&priv->reg_lock); @@ -793,8 +788,7 @@ static int ub960_rxport_read16(struct ub960_data *priv, u8 nport, u8 reg, u16 *val) { struct device *dev = &priv->client->dev; - unsigned int v1; - unsigned int v2; + __be16 __v; int ret; mutex_lock(&priv->reg_lock); @@ -803,21 +797,14 @@ static int ub960_rxport_read16(struct ub960_data *priv, u8 nport, u8 reg, if (ret) goto out_unlock; - ret = regmap_read(priv->regmap, reg, &v1); + ret = regmap_bulk_read(priv->regmap, reg, &__v, 2); if (ret) { dev_err(dev, "%s: cannot read register 0x%02x (%d)!\n", __func__, reg, ret); goto out_unlock; } - ret = regmap_read(priv->regmap, reg + 1, &v2); - if (ret) { - dev_err(dev, "%s: cannot read register 0x%02x (%d)!\n", - __func__, reg + 1, ret); - goto out_unlock; - } - - *val = (v1 << 8) | v2; + *val = be16_to_cpu(__v); out_unlock: mutex_unlock(&priv->reg_lock); @@ -1072,7 +1059,7 @@ static int ub960_atr_attach_client(struct i2c_atr *atr, u32 chan_id, * 2. Construct a bitmask of port's used alias entries */ - for (pool_idx = 0; pool_idx < priv->atr.num_aliases; ++pool_idx) { + for (pool_idx = 0; pool_idx < priv->atr.num_aliases; pool_idx++) { struct atr_alias_table_entry *e; e = &priv->atr.aliases[pool_idx]; @@ -1133,7 +1120,7 @@ static void ub960_atr_detach_client(struct i2c_atr *atr, u32 chan_id, /* Find alias mapped to this client */ - for (pool_idx = 0; pool_idx < priv->atr.num_aliases; ++pool_idx) { + for (pool_idx = 0; pool_idx < priv->atr.num_aliases; pool_idx++) { entry = &priv->atr.aliases[pool_idx]; if (entry->in_use && entry->nport == rxport->nport && @@ -1271,7 +1258,7 @@ static int ub960_rxport_enable_vpocs(struct ub960_data *priv) unsigned int nport; int ret; - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport]; if (!rxport || !rxport->vpoc) @@ -1301,7 +1288,7 @@ static void ub960_rxport_disable_vpocs(struct ub960_data *priv) { unsigned int nport; - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport]; if (!rxport || !rxport->vpoc) @@ -1331,7 +1318,7 @@ static void ub960_clear_rx_errors(struct ub960_data *priv) { unsigned int nport; - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) ub960_rxport_clear_errors(priv, nport); } @@ -1768,7 +1755,7 @@ static int ub960_rxport_add_serializers(struct ub960_data *priv) unsigned int nport; int ret; - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport]; if (!rxport) @@ -1798,7 +1785,7 @@ static void ub960_rxport_remove_serializers(struct ub960_data *priv) { unsigned int nport; - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport]; if (!rxport) @@ -1875,7 +1862,7 @@ static int ub960_init_tx_ports(struct ub960_data *priv) } } - for (nport = 0; nport < priv->hw_data->num_txports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_txports; nport++) { struct ub960_txport *txport = priv->txports[nport]; if (!txport) @@ -2204,7 +2191,7 @@ static int ub960_init_rx_ports(struct ub960_data *priv) { unsigned int nport; - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport]; if (!rxport) @@ -2432,11 +2419,11 @@ static int ub960_validate_stream_vcs(struct ub960_data *priv) unsigned int nport; unsigned int i; - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport]; struct v4l2_mbus_frame_desc desc; int ret; - u8 cur_vc; + u8 vc; if (!rxport) continue; @@ -2449,15 +2436,13 @@ static int ub960_validate_stream_vcs(struct ub960_data *priv) if (desc.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) continue; - for (i = 0; i < desc.num_entries; ++i) { - u8 vc = desc.entry[i].bus.csi2.vc; + if (desc.num_entries == 0) + continue; - if (i == 0) { - cur_vc = vc; - continue; - } + vc = desc.entry[0].bus.csi2.vc; - if (vc == cur_vc) + for (i = 1; i < desc.num_entries; i++) { + if (vc == desc.entry[i].bus.csi2.vc) continue; dev_err(&priv->client->dev, @@ -2550,7 +2535,7 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv, fwd_ctl = 0; - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport]; u8 vc = vc_map[nport]; @@ -2587,7 +2572,7 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv, unsigned int i; /* Map all VCs from this port to VC(nport) */ - for (i = 0; i < 8; ++i) + for (i = 0; i < 8; i++) ub960_rxport_write(priv, nport, UB960_RR_VC_ID_MAP(i), nport); @@ -2615,7 +2600,7 @@ static void ub960_update_streaming_status(struct ub960_data *priv) { unsigned int i; - for (i = 0; i < UB960_MAX_NPORTS; ++i) { + for (i = 0; i < UB960_MAX_NPORTS; i++) { if (priv->stream_enable_mask[i]) break; } @@ -2665,7 +2650,7 @@ static int ub960_enable_streams(struct v4l2_subdev *sd, sink_streams[nport] |= BIT_ULL(route->sink_stream); } - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { if (!sink_streams[nport]) continue; @@ -2703,7 +2688,7 @@ static int ub960_enable_streams(struct v4l2_subdev *sd, return 0; err: - for (nport = 0; nport < failed_port; ++nport) { + for (nport = 0; nport < failed_port; nport++) { if (!sink_streams[nport]) continue; @@ -2759,7 +2744,7 @@ static int ub960_disable_streams(struct v4l2_subdev *sd, sink_streams[nport] |= BIT_ULL(route->sink_stream); } - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { if (!sink_streams[nport]) continue; @@ -2886,7 +2871,7 @@ static int ub960_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, goto out_unlock; } - for (i = 0; i < source_fd.num_entries; ++i) { + for (i = 0; i < source_fd.num_entries; i++) { if (source_fd.entry[i].stream == route->sink_stream) { source_entry = &source_fd.entry[i]; break; @@ -3025,17 +3010,16 @@ static int ub960_log_status(struct v4l2_subdev *sd) unsigned int i; u16 v16 = 0; u8 v = 0; - u8 id[7]; + u8 id[UB960_SR_FPD3_RX_ID_LEN]; state = v4l2_subdev_lock_and_get_active_state(sd); - for (i = 0; i < 6; ++i) + for (i = 0; i < sizeof(id); i++) ub960_read(priv, UB960_SR_FPD3_RX_ID(i), &id[i]); - id[6] = 0; - dev_info(dev, "ID '%s'\n", id); + dev_info(dev, "ID '%.*s'\n", sizeof(id), id); - for (nport = 0; nport < priv->hw_data->num_txports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_txports; nport++) { struct ub960_txport *txport = priv->txports[nport]; dev_info(dev, "TX %u\n", nport); @@ -3062,7 +3046,7 @@ static int ub960_log_status(struct v4l2_subdev *sd) dev_info(dev, "\tline error counter %u\n", v16); } - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport]; u8 eq_level; s8 strobe_pos; @@ -3141,7 +3125,7 @@ static int ub960_log_status(struct v4l2_subdev *sd) dev_info(dev, "\tEQ level %u\n", eq_level); /* GPIOs */ - for (i = 0; i < UB960_NUM_BC_GPIOS; ++i) { + for (i = 0; i < UB960_NUM_BC_GPIOS; i++) { u8 ctl_reg; u8 ctl_shift; @@ -3201,12 +3185,12 @@ static irqreturn_t ub960_handle_events(int irq, void *arg) dev_dbg(&priv->client->dev, "FWD_STS %#02x\n", fwd_sts); - for (i = 0; i < priv->hw_data->num_txports; ++i) { + for (i = 0; i < priv->hw_data->num_txports; i++) { if (int_sts & UB960_SR_INTERRUPT_STS_IS_CSI_TX(i)) ub960_csi_handle_events(priv, i); } - for (i = 0; i < priv->hw_data->num_rxports; ++i) { + for (i = 0; i < priv->hw_data->num_rxports; i++) { if (!priv->rxports[i]) continue; @@ -3233,7 +3217,7 @@ static void ub960_txport_free_ports(struct ub960_data *priv) { unsigned int nport; - for (nport = 0; nport < priv->hw_data->num_txports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_txports; nport++) { struct ub960_txport *txport = priv->txports[nport]; if (!txport) @@ -3248,7 +3232,7 @@ static void ub960_rxport_free_ports(struct ub960_data *priv) { unsigned int nport; - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport]; if (!rxport) @@ -3284,7 +3268,7 @@ static int ub960_parse_dt_base(struct ub960_data *priv) return 0; priv->atr.aliases = devm_kcalloc(dev, table_size, - sizeof(struct atr_alias_table_entry), + sizeof(*priv->atr.aliases), GFP_KERNEL); if (!priv->atr.aliases) return -ENOMEM; @@ -3302,7 +3286,7 @@ static int ub960_parse_dt_base(struct ub960_data *priv) return ret; } - for (i = 0; i < table_size; ++i) + for (i = 0; i < table_size; i++) priv->atr.aliases[i].alias_id = aliases[i]; kfree(aliases); @@ -3387,12 +3371,15 @@ ub960_parse_dt_rxport_link_properties(struct ub960_data *priv, "ti,strobe-pos", ret); return ret; } - } else if (strobe_pos < UB960_MIN_MANUAL_STROBE_POS || - strobe_pos > UB960_MAX_MANUAL_STROBE_POS) { - dev_err(dev, "rx%u: illegal 'strobe-pos' value: %d\n", nport, - strobe_pos); } else { - /* NOTE: ignored unless global manual strobe pos is set */ + if (strobe_pos < UB960_MIN_MANUAL_STROBE_POS || + strobe_pos > UB960_MAX_MANUAL_STROBE_POS) { + dev_err(dev, "rx%u: illegal 'strobe-pos' value: %d\n", + nport, strobe_pos); + return -EINVAL; + } + + /* NOTE: ignored unless global manual strobe pos is also set */ rxport->eq.strobe_pos = strobe_pos; if (!priv->strobe.manual) dev_warn(dev, @@ -3407,10 +3394,13 @@ ub960_parse_dt_rxport_link_properties(struct ub960_data *priv, "ti,eq-level", ret); return ret; } - } else if (eq_level > UB960_MAX_EQ_LEVEL) { - dev_err(dev, "rx%u: illegal 'ti,eq-level' value: %d\n", nport, - eq_level); } else { + if (eq_level > UB960_MAX_EQ_LEVEL) { + dev_err(dev, "rx%u: illegal 'ti,eq-level' value: %d\n", + nport, eq_level); + return -EINVAL; + } + rxport->eq.manual_eq = true; rxport->eq.manual.eq_level = eq_level; } @@ -3486,8 +3476,9 @@ static int ub960_parse_dt_rxport(struct ub960_data *priv, unsigned int nport, struct fwnode_handle *link_fwnode, struct fwnode_handle *ep_fwnode) { - static const char *vpoc_names[UB960_MAX_RX_NPORTS] = { "vpoc0", "vpoc1", - "vpoc2", "vpoc3" }; + static const char *vpoc_names[UB960_MAX_RX_NPORTS] = { + "vpoc0", "vpoc1", "vpoc2", "vpoc3" + }; struct device *dev = &priv->client->dev; struct ub960_rxport *rxport; int ret; @@ -3576,7 +3567,7 @@ static int ub960_parse_dt_rxports(struct ub960_data *priv) priv->strobe.manual = fwnode_property_read_bool(links_fwnode, "ti,manual-strobe"); - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct fwnode_handle *link_fwnode; struct fwnode_handle *ep_fwnode; @@ -3619,7 +3610,7 @@ static int ub960_parse_dt_txports(struct ub960_data *priv) u32 nport; int ret; - for (nport = 0; nport < priv->hw_data->num_txports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_txports; nport++) { unsigned int port = nport + priv->hw_data->num_rxports; struct fwnode_handle *ep_fwnode; @@ -3696,7 +3687,7 @@ static int ub960_notify_bound(struct v4l2_async_notifier *notifier, return ret; } - for (i = 0; i < priv->hw_data->num_rxports; ++i) { + for (i = 0; i < priv->hw_data->num_rxports; i++) { if (priv->rxports[i] && !priv->rxports[i]->source.sd) { dev_dbg(dev, "Waiting for more subdevs to be bound\n"); return 0; @@ -3728,7 +3719,7 @@ static int ub960_v4l2_notifier_register(struct ub960_data *priv) v4l2_async_nf_init(&priv->notifier); - for (i = 0; i < priv->hw_data->num_rxports; ++i) { + for (i = 0; i < priv->hw_data->num_rxports; i++) { struct ub960_rxport *rxport = priv->rxports[i]; struct ub960_asd *asd; @@ -3791,7 +3782,7 @@ static int ub960_create_subdev(struct ub960_data *priv) priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; priv->sd.entity.ops = &ub960_entity_ops; - for (i = 0; i < priv->hw_data->num_rxports + priv->hw_data->num_txports; ++i) { + for (i = 0; i < priv->hw_data->num_rxports + priv->hw_data->num_txports; i++) { priv->pads[i].flags = ub960_pad_is_sink(priv, i) ? MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE; @@ -4055,7 +4046,7 @@ static int ub960_probe(struct i2c_client *client) port_mask = 0; - for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { + for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport]; if (!rxport)