Re: [PATCH v2 4/9] i2c: atr: split up i2c_atr_get_mapping_by_addr()

[Romain Gantois <romain.gantois@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 4846 bytes --]

On mardi 25 février 2025 12:39:32 heure normale d’Europe centrale Cosmin 
Tanislav 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>
> ---
>  drivers/i2c/i2c-atr.c | 108 ++++++++++++++++++++++++++++++------------
>  1 file changed, 79 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index f2485d1670a2..9c4e9e8ec802 100644
> --- a/drivers/i2c/i2c-atr.c
> +++ b/drivers/i2c/i2c-atr.c
> @@ -239,9 +239,26 @@ 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;
> +	struct list_head *alias_pairs;
> +
> +	lockdep_assert_held(&chan->alias_pairs_lock);
> +
> +	alias_pairs = &chan->alias_pairs;

This variable isn't really needed since it's only being used once.

> +
> +	list_for_each_entry(c2a, 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;
> @@ -253,38 +270,55 @@ 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)
> +			break;
> +
> +	if (c2a->fixed)
> +		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);
> +		goto err_del_c2a;

I think this goto should be removed, the corresponding label code is only used 
here anyway.

>  	}
> 
> +	return c2a;
> +
> +err_del_c2a:
> +	i2c_atr_destroy_c2a(&c2a);
> +	i2c_atr_release_alias(chan->alias_pool, alias);
> +	return NULL;
> +}
> +
> +static struct i2c_atr_alias_pair *
> +i2c_atr_create_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
> +{
> +	struct i2c_atr *atr = chan->atr;
> +	struct i2c_atr_alias_pair *c2a;
> +	u16 alias;
> +	int ret;
> +
>  	ret = i2c_atr_reserve_alias(chan->alias_pool);
> -	if (ret < 0) {
> -		// If no free aliases are left, replace an existing one
> -		if (unlikely(list_empty(alias_pairs)))
> -			return NULL;
> +	if (ret < 0)
> +		return NULL;
> 
> -		list_for_each_entry_reverse(c2a, alias_pairs, node)
> -			if (!c2a->fixed)
> -				break;
> +	alias = ret;
> 
> -		if (c2a->fixed)
> -			return NULL;
> -
> -		atr->ops->detach_addr(atr, chan->chan_id, c2a->addr);
> -		c2a->addr = addr;
> -
> -		// Move updated entry to beginning of list
> -		list_move(&c2a->node, alias_pairs);
> -
> -		alias = c2a->alias;
> -	} else {
> -		alias = ret;
> -
> -		c2a = i2c_atr_create_c2a(chan, alias, addr);
> -		if (!c2a)
> -			goto err_release_alias;
> -	}
> +	c2a = i2c_atr_create_c2a(chan, alias, addr);
> +	if (!c2a)
> +		goto err_release_alias;
> 
>  	ret = atr->ops->attach_addr(atr, chan->chan_id, c2a->addr, c2a-
>alias);
>  	if (ret) {
> @@ -302,6 +336,22 @@ i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan,
> u16 addr) return NULL;
>  }
> 
> +static struct i2c_atr_alias_pair *
> +i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
> +{
> +	struct i2c_atr_alias_pair *c2a;
> +
> +	c2a = i2c_atr_find_mapping_by_addr(chan, addr);
> +	if (c2a)
> +		return c2a;
> +
> +	c2a = i2c_atr_create_mapping_by_addr(chan, addr);
> +	if (c2a)
> +		return c2a;
> +
> +	return i2c_atr_replace_mapping_by_addr(chan, addr);
> +}
> +
>  /*
>   * Replace all message addresses with their aliases, saving the original
>   * addresses.

Best Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]