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

[Cosmin Tanislav]

On 4/30/25 5:33 PM, Luca Ceresoli wrote:
> On Mon, 28 Apr 2025 13:25:08 +0300
> Cosmin Tanislav <demonsingur@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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