Re: [PATCH 12/19] media: i2c: ds90ub960: Add RX port iteration support

[Sakari Ailus]

Moi,

On Fri, Jan 24, 2025 at 03:34:44PM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 15/01/2025 16:23, Sakari Ailus wrote:
> > Moi,
> > 
> > On Fri, Jan 10, 2025 at 11:14:12AM +0200, Tomi Valkeinen wrote:
> > > The driver does a lot of iteration over the RX ports with for loops. In
> > > most cases the driver will skip unused RX ports. Also, in the future
> > > patches the FPD-Link IV support will be refreshed with TI's latest init
> > > sequences which involves a lot of additional iterations over the RX
> > > ports, often only for FPD-Link IV ports.
> > > 
> > > To make the iteration simpler and to make it clearer what we're
> > > iterating over (all or only-active, all or only-fpd4), add macros and
> > > support functions for iterating the RX ports. Use the macros in the
> > > driver, replacing the for loops.
> > > 
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/media/i2c/ds90ub960.c | 260 ++++++++++++++++++++++--------------------
> > >   1 file changed, 135 insertions(+), 125 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> > > index bca858172942..02e22ae813fa 100644
> > > --- a/drivers/media/i2c/ds90ub960.c
> > > +++ b/drivers/media/i2c/ds90ub960.c
> > > @@ -649,6 +649,63 @@ static const struct ub960_format_info *ub960_find_format(u32 code)
> > >   	return NULL;
> > >   }
> > > +struct ub960_rxport_iter {
> > > +	unsigned int nport;
> > > +	struct ub960_rxport *rxport;
> > > +};
> > > +
> > > +enum ub960_iter_flags {
> > > +	UB960_ITER_ACTIVE_ONLY = BIT(0),
> > > +	UB960_ITER_FPD4_ONLY = BIT(1),
> > > +};
> > > +
> > > +static struct ub960_rxport_iter ub960_iter_rxport(struct ub960_data *priv,
> > > +						  struct ub960_rxport_iter it,
> > > +						  enum ub960_iter_flags flags)
> > > +{
> > > +	for (; it.nport < priv->hw_data->num_rxports; it.nport++) {
> > > +		it.rxport = priv->rxports[it.nport];
> > > +
> > > +		if ((flags & UB960_ITER_ACTIVE_ONLY) && !it.rxport)
> > > +			continue;
> > > +
> > > +		if ((flags & UB960_ITER_FPD4_ONLY) &&
> > > +		    it.rxport->cdr_mode != RXPORT_CDR_FPD4)
> > > +			continue;
> > > +
> > > +		return it;
> > > +	}
> > > +
> > > +	it.rxport = NULL;
> > > +
> > > +	return it;
> > > +}
> > > +
> > > +#define for_each_rxport(priv)                                                 \
> > 
> > it should be also an argument to the macro as it's visible outside it.
> > 
> > And wouldn't it be reasonable to use a pointer instead for the purpsoe?
> 
> You mean something like:
> 
>   struct ub960_rxport_iter it = { 0 };
> 
>   for_each_rxport(priv, &it) { }
> 
> Then we leak the iterator, and I really hate it. I've fixed numerous bugs
> caused by such cases.

You can still define it for the loop only, just give it a name instead of
using a pre-determined name.

Could you also pass the iterator by reference instead of the value?

> 
>  Tomi
> 
> > 
> > > +	for (struct ub960_rxport_iter it =                                    \
> > > +		     ub960_iter_rxport(priv, (struct ub960_rxport_iter){ 0 }, \
> > > +				       0);                                    \
> > > +	     it.nport < (priv)->hw_data->num_rxports;                         \
> > > +	     it.nport++, it = ub960_iter_rxport(priv, it, 0))
> > > +
> > > +#define for_each_active_rxport(priv)                                          \
> > > +	for (struct ub960_rxport_iter it =                                    \
> > > +		     ub960_iter_rxport(priv, (struct ub960_rxport_iter){ 0 }, \
> > > +				       UB960_ITER_ACTIVE_ONLY);               \
> > > +	     it.nport < (priv)->hw_data->num_rxports;                         \
> > > +	     it.nport++, it = ub960_iter_rxport(priv, it,                     \
> > > +						UB960_ITER_ACTIVE_ONLY))
> > > +
> > > +#define for_each_active_rxport_fpd4(priv)                                     \
> > > +	for (struct ub960_rxport_iter it =                                    \
> > > +		     ub960_iter_rxport(priv, (struct ub960_rxport_iter){ 0 }, \
> > > +				       UB960_ITER_ACTIVE_ONLY |               \
> > > +					       UB960_ITER_FPD4_ONLY);         \
> > > +	     it.nport < (priv)->hw_data->num_rxports;                         \
> > > +	     it.nport++, it = ub960_iter_rxport(priv, it,                     \
> > > +						UB960_ITER_ACTIVE_ONLY |      \
> > > +							UB960_ITER_FPD4_ONLY))
> > 
> 

-- 
Terveisin,

Sakari Ailus