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

["Arınç ÜNAL" <arinc.unal@arinc9.com>]

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ç