Re: [PATCH net-next v2 1/7] net: dsa: mt7530: always trap frames to active CPU port on MT7530

[Arınç ÜNAL]

On 4.01.2024 18:22, Vladimir Oltean wrote:
> On Wed, Dec 27, 2023 at 07:43:41AM +0300, Arınç ÜNAL wrote:
> > @@ -3075,6 +3071,38 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
> >   	return 0;
> >   }
> >   
> > +static void
> > +mt753x_conduit_state_change(struct dsa_switch *ds,
> > +			    const struct net_device *conduit,
> > +			    bool operational)
> > +{
> > +	struct dsa_port *cpu_dp = conduit->dsa_ptr;
> > +	struct mt7530_priv *priv = ds->priv;
> > +	u8 mask;
> > +	int val = 0;
>
> Longest line first.
>
> > +
> > +	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
> > +	 * forwarded to the numerically smallest CPU port which the DSA conduit
> > +	 * interface its affine to is up.
>
> "first CPU port whose conduit interface is up"
>
> > +	 */
> > +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> > +		return;
> > +
> > +	mask = BIT(cpu_dp->index);
> > +
> > +	if (operational)
> > +		priv->active_cpu_ports |= mask;
> > +	else
> > +		priv->active_cpu_ports &= ~mask;
> > +
> > +	if (priv->active_cpu_ports)
> > +		val =
> > +		    CPU_EN |
> > +		    CPU_PORT(__ffs((unsigned long)priv->active_cpu_ports));
>
> I don't think the type cast is necessary (implicit type promotion takes place).
>
> Also, it is customary to put {} for multi-line "if" blocks, even if they are
> made up of a single expression.
>
> But without the type cast, it could look like this.
>
> 	if (priv->active_cpu_ports)
> 		val = CPU_EN | CPU_PORT(__ffs(priv->active_cpu_ports));

Will do. Thanks for dealing with my rookie mistakes. :)

Arınç