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

Luca Ceresoli
2025-04-30T14:33:11Z

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