| Message ID | 20241125-fpc202-v3-2-34e86bcb5b56@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | misc: Support TI FPC202 dual-port controller | expand |
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
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,
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); - if (reg_idx == ARRAY_SIZE(rxport->aliased_clients)) { + if (reg_idx >= UB960_MAX_PORT_ALIASES) { dev_err(dev, "rx%u: client 0x%02x is not mapped!\n", rxport->nport, client->addr); return; } - rxport->aliased_clients[reg_idx] = NULL; + clear_bit(reg_idx, rxport->alias_use_mask); ub960_rxport_write(priv, chan_id, UB960_RR_SLAVE_ALIAS(reg_idx), 0); @@ -3404,6 +3397,8 @@ static int ub960_parse_dt_rxport(struct ub960_data *priv, unsigned int nport, if (ret) goto err_put_remote_fwnode; + bitmap_zero(rxport->alias_use_mask, UB960_MAX_PORT_ALIASES); + return 0; err_put_remote_fwnode:
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(-)