diff mbox series

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

Message ID 20241125-fpc202-v3-2-34e86bcb5b56@bootlin.com (mailing list archive)
State New
Headers show
Series misc: Support TI FPC202 dual-port controller | expand

Commit Message

Romain Gantois Nov. 25, 2024, 8:45 a.m. UTC 
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(-)

Comments

Tomi Valkeinen Nov. 29, 2024, 1:46 p.m. UTC | #1 
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
Romain Gantois Dec. 3, 2024, 8:48 a.m. UTC | #2 
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 mbox series

Patch

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: