Hello Cosmin,
On lundi 3 février 2025 13:15:17 heure normale d’Europe centrale Cosmin
Tanislav wrote:
Some I2C ATRs can have other I2C ATRs as children. The I2C messages of
the child ATRs need to be forwarded as-is since the parent I2C ATR can
only do address remapping for the direct children.
In the case of GMSL, the deserializer I2C ATR actually doesn't have I2C
address remapping hardware capabilities, but it is able to select which
GMSL link to talk to, allowing it to change the address of the
serializer.
The child ATRs need to have their alias pools defined in such a way to
prevent overlapping addresses between them, but there's no way around
this without orchestration between multiple ATR instances.
To allow for this use-case, add a flag that allows unmapped addresses
to be passed through, since they are already remapped by the child ATRs,
and disables dynamic remapping, since devices that need passthrough
messages to be forwarded as-is, can only handle remapping for their
direct children.
There's no case where a non-remapped address will hit the parent ATR.
I'm having trouble understanding this, because it seems like there's a
contradiction with your previous statement:
add a flag that allows unmapped addresses to be passed through
Unmapped addresses are "non-remapped" by definition right? And they can hit the
parent ATR since we're adding a flag to allow them to pass through...
Signed-off-by: Cosmin Tanislav <demonsingur@xxxxxxxxx>
---
drivers/i2c/i2c-atr.c | 26 ++++++++++++++++++--------
include/linux/i2c-atr.h | 20 +++++++++++++++++---
2 files changed, 35 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 13f7e07fd8e87..5f0e8f1cf69f7 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -106,6 +106,7 @@ struct i2c_atr_chan {
* @lock: Lock for the I2C bus segment (see &struct
i2c_lock_operations) * @lock_key: Lock key for @lock
* @max_adapters: Maximum number of adapters this I2C ATR can have
+ * @flags: Flags for ATR
* @alias_pool: Optional common pool of available client aliases
* @i2c_nb: Notifier for remote client add & del events
* @adapter: Array of adapters
@@ -122,6 +123,7 @@ struct i2c_atr {
struct mutex lock;
struct lock_class_key lock_key;
int max_adapters;
+ u32 flags;
struct i2c_atr_alias_pool *alias_pool;
@@ -241,7 +243,7 @@ static void i2c_atr_release_alias(struct
i2c_atr_alias_pool *alias_pool, u16 ali
/* Must be called with alias_pairs_lock held */
static struct i2c_atr_alias_pair *
-i2c_atr_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
+i2c_atr_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr, bool
new_addr) {
IMO the "new_addr" naming is quite confusing.
After this patch is applied, the expected behavior is:
i2c_atr_find_mapping_by_addr() called from i2c_atr_attach_addr():
1. find existing mapping, return it
2. OR find free alias, create mapping and return it
3. OR remap used alias, return mapping
4. OR fail
i2c_atr_find_mapping_by_addr(), called from anywhere else:
1. find existing mapping, return it
2. OR find free alias, create mapping and return it
3. OR if the ATR has PASSTHROUGH set, fail
4. OR remap used alias, return mapping
5. OR fail
To me, the proposed code doesn't make it immediately obvious why the
PASSTHROUGH flag should have anything to do with not attempting alias
remapping.
Moreover, if we truly want to ignore *all* unmapped addresses, then shouldn't
we also give up on step 2.? (the one that tries to map a free alias to the
requested address).
In that case, I think something like this would be clearer:
in i2c_atr_smbus_xfer() and i2c_atr_map_msgs():
```
#never attempts to create a new mapping, only to find an existing one
c2a = i2c_atr_find_mapping_by_addr(chan, msgs[i].addr);
if (!c2a) {
if (PASSTHROUGH)
# Since passthrough is set, we ignore unmapped addresses
goto success or whatever;
c2a = i2c_atr_create_mapping(chan, msgs[i].addr);
if (!c2a)
fail;
}
```
in i2c_atr_attach_addr():
```
c2a = i2c_atr_find_mapping_by_addr(chan, msgs[i].addr);
if (!c2a) {
c2a = i2c_atr_create_mapping(chan, msgs[i].addr);
if (!c2a)
fail;
}
```
So what I'm suggesting is to remove all c2a mapping creation logic from
find_mapping_by_addr() entirely, and to move it to a separate function.
Please let me know what you think.
Thanks,