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

[Romain Gantois]

Hi,

On vendredi 29 novembre 2024 14:46:38 heure normale d’Europe centrale Tomi 
Valkeinen wrote:
> 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.
...
> >   		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.

Indeed, there is an issue here. Tracking client addresses is still required, 
so the correct change would be to convert aliased_clients to aliased_addrs.

Thanks,

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