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

Cosmin Tanislav
2025-05-05T10:27:01Z

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
>