Re: [PATCH v4 3/9] i2c: atr: split up i2c_atr_get_mapping_by_addr()

[Cosmin Tanislav <demonsingur@gmail.com>]

On 4/30/25 5:33 PM, Luca Ceresoli wrote:
> On Mon, 28 Apr 2025 13:25:08 +0300
> Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 
>> The i2c_atr_get_mapping_by_addr() function handles three separate
>> usecases: finding an existing mapping, creating a new mapping, or
>> replacing an existing mapping if a new mapping cannot be created
>> because there aren't enough aliases available.
>>
>> Split up the function into three different functions handling its
>> individual usecases to prepare for better usage of each one.
>>
>> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> 
> This function has become quite complex over time, so this looks like a
> good cleanup by itself even not counting the advantages coming with the
> following patches.
> 
> I have only one small remark, see below.
> 
>> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
>> index 939fb95fe781..184c57c31e60 100644
>> --- a/drivers/i2c/i2c-atr.c
>> +++ b/drivers/i2c/i2c-atr.c
>> @@ -239,9 +239,23 @@ static void i2c_atr_release_alias(struct i2c_atr_alias_pool *alias_pool, u16 ali
>>   	spin_unlock(&alias_pool->lock);
>>   }
>>   
>> -/* Must be called with alias_pairs_lock held */
>>   static struct i2c_atr_alias_pair *
>> -i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
>> +i2c_atr_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
>> +{
>> +	struct i2c_atr_alias_pair *c2a;
>> +
>> +	lockdep_assert_held(&chan->alias_pairs_lock);
>> +
>> +	list_for_each_entry(c2a, &chan->alias_pairs, node) {
>> +		if (c2a->addr == addr)
>> +			return c2a;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static struct i2c_atr_alias_pair *
>> +i2c_atr_replace_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
>>   {
>>   	struct i2c_atr *atr = chan->atr;
>>   	struct i2c_atr_alias_pair *c2a;
>> @@ -254,41 +268,57 @@ i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
>>   
>>   	alias_pairs = &chan->alias_pairs;
>>   
>> -	list_for_each_entry(c2a, alias_pairs, node) {
>> -		if (c2a->addr == addr)
>> -			return c2a;
>> +	if (unlikely(list_empty(alias_pairs)))
>> +		return NULL;
>> +
>> +	list_for_each_entry_reverse(c2a, alias_pairs, node) {
>> +		if (!c2a->fixed) {
>> +			found = true;
>> +			break;
>> +		}
>>   	}
>>   
>> +	if (!found)
>> +		return NULL;
>> +
>> +	atr->ops->detach_addr(atr, chan->chan_id, c2a->addr);
>> +	c2a->addr = addr;
>> +
>> +	list_move(&c2a->node, alias_pairs);
>> +
>> +	alias = c2a->alias;
>> +
>> +	ret = atr->ops->attach_addr(atr, chan->chan_id, c2a->addr, c2a->alias);
>> +	if (ret) {
>> +		dev_err(atr->dev, "failed to attach 0x%02x on channel %d: err %d\n",
>> +			addr, chan->chan_id, ret);
>> +		i2c_atr_destroy_c2a(&c2a);
>> +		i2c_atr_release_alias(chan->alias_pool, alias);
>> +		return NULL;
>> +	}
>> +
>> +	return c2a;
>> +}
>> +
>> +static struct i2c_atr_alias_pair *
>> +i2c_atr_create_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
> 
> I would move the _create function before the _replace one, because
> that's the logical order in which they are called.
> 

Sadly the diff actually becomes bigger by doing this.
before: 78 insertions(+), 32 deletions(-)
after: 84 insertions(+), 38 deletions(-)

If we were to put things in a logical order then should we put _find()
after create(), or after replace()? There's no specific order in that
case. I think we should keep things as-is as it matches the previous
branches of the code, just split into separate functions.

> As a nice side effect, this might make the diff more readable.
> 
> Luca
>