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

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

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.

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

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com