Re: [PATCH v3 2/9] media: i2c: ds90ub960: Replace aliased clients list with bitmap

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi,

On 25/11/2024 10:45, Romain Gantois wrote:
> The ds90ub960 driver currently uses a list of i2c_client structs to keep
> track of used I2C address translator (ATR) alias slots for each RX port.
> 
> Keeping these i2c_client structs in the alias slot list isn't actually
> needed, the driver only needs to know if a specific alias slot is already
> in use or not.
> 
> Convert the aliased_clients list to a bitmap named "alias_use_mask". This
> will allow removing the "client" parameter from the i2c-atr callbacks in a
> future patch.
> 
> Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
> ---
>   drivers/media/i2c/ds90ub960.c | 23 +++++++++--------------
>   1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index ffe5f25f8647624be005da33a6412da2493413b4..f86028894c78187257efc8fd70812387000796f7 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -468,7 +468,7 @@ struct ub960_rxport {
>   		};
>   	} eq;
>   
> -	const struct i2c_client *aliased_clients[UB960_MAX_PORT_ALIASES];
> +	DECLARE_BITMAP(alias_use_mask, UB960_MAX_PORT_ALIASES);
>   };
>   
>   struct ub960_asd {
> @@ -1032,17 +1032,13 @@ static int ub960_atr_attach_client(struct i2c_atr *atr, u32 chan_id,
>   	struct device *dev = &priv->client->dev;
>   	unsigned int reg_idx;
>   
> -	for (reg_idx = 0; reg_idx < ARRAY_SIZE(rxport->aliased_clients); reg_idx++) {
> -		if (!rxport->aliased_clients[reg_idx])
> -			break;
> -	}
> -
> -	if (reg_idx == ARRAY_SIZE(rxport->aliased_clients)) {
> +	reg_idx = find_first_zero_bit(rxport->alias_use_mask, UB960_MAX_PORT_ALIASES);
> +	if (reg_idx >= UB960_MAX_PORT_ALIASES) {
>   		dev_err(dev, "rx%u: alias pool exhausted\n", rxport->nport);
>   		return -EADDRNOTAVAIL;
>   	}
>   
> -	rxport->aliased_clients[reg_idx] = client;
> +	set_bit(reg_idx, rxport->alias_use_mask);
>   
>   	ub960_rxport_write(priv, chan_id, UB960_RR_SLAVE_ID(reg_idx),
>   			   client->addr << 1);
> @@ -1063,18 +1059,15 @@ static void ub960_atr_detach_client(struct i2c_atr *atr, u32 chan_id,
>   	struct device *dev = &priv->client->dev;
>   	unsigned int reg_idx;
>   
> -	for (reg_idx = 0; reg_idx < ARRAY_SIZE(rxport->aliased_clients); reg_idx++) {
> -		if (rxport->aliased_clients[reg_idx] == client)
> -			break;
> -	}
> +	reg_idx = find_first_zero_bit(rxport->alias_use_mask, UB960_MAX_PORT_ALIASES);

The old code went through the alias table to find the matching client, 
so that it can be removed. The new code... Tries to find the first 
unused entry in the mask, to... free it?

I'm not sure how this is supposed to work, or how the driver even could 
manage with just a bit mask. The driver needs to remove the one that was 
assigned in ub960_atr_attach_addr(), so it somehow has to find the same 
entry using the address or the alias.

  Tomi