Subject: Re: [PATCH net-next v2 2/2] net: pcs: rzn1-miic: Add support for PHY link active-level configuration

From: "Russell King (Oracle)" <[email-masked]>
Date: Fri, 9 Jan 2026 15:50:58 +0000

On Fri, Jan 09, 2026 at 02:22:50PM +0000, Prabhakar wrote:
> +static void miic_configure_phylink(struct miic *miic, u32 conf,
> +				   u32 port, bool active_low)
> +{
> +	bool polarity_active_high;
> +	u32 mask, val;
> +	int shift;
> +
> +	/* determine shift and polarity for this conf */
> +	if (miic->of_data->type == MIIC_TYPE_RZN1) {
> +		switch (conf) {
> +		/* switch ports => bits [3:0] (shift 0), active when low */
> +		case MIIC_SWITCH_PORTA:
> +		case MIIC_SWITCH_PORTB:
> +		case MIIC_SWITCH_PORTC:
> +		case MIIC_SWITCH_PORTD:
> +			shift = 0;
> +			polarity_active_high = false;
> +			break;
> +
> +		/* EtherCAT ports => bits [7:4] (shift 4), active when high */
> +		case MIIC_ETHERCAT_PORTA:
> +		case MIIC_ETHERCAT_PORTB:
> +		case MIIC_ETHERCAT_PORTC:
> +			shift = 4;
> +			polarity_active_high = true;
> +			break;
> +
> +		/* Sercos ports => bits [11:8] (shift 8), active when high */
> +		case MIIC_SERCOS_PORTA:
> +		case MIIC_SERCOS_PORTB:
> +			shift = 8;
> +			polarity_active_high = true;
> +			break;
> +
> +		default:
> +			return;
> +		}
> +	} else {
> +		switch (conf) {
> +		/* ETHSW ports => bits [3:0] (shift 0), active when low */
> +		case ETHSS_ETHSW_PORT0:
> +		case ETHSS_ETHSW_PORT1:
> +		case ETHSS_ETHSW_PORT2:
> +			shift = 0;
> +			polarity_active_high = false;
> +			break;
> +
> +		/* ESC ports => bits [7:4] (shift 4), active when high */
> +		case ETHSS_ESC_PORT0:
> +		case ETHSS_ESC_PORT1:
> +		case ETHSS_ESC_PORT2:
> +			shift = 4;
> +			polarity_active_high = true;
> +			break;
> +
> +		default:
> +			return;
> +		}
> +	}
> +
> +	mask = BIT(port) << shift;
> +
> +	if (polarity_active_high)
> +		val = (active_low ? 0 : BIT(port)) << shift;
> +	else
> +		val = (active_low ? BIT(port) : 0) << shift;

Looking closer at this, I think this is confusing.

The underlying purpose here is to set mask and val to change the state of
a single bit in the PHY link register for each call to this function,
accumulating the changes in your misnamed "struct phylink".

Given that "mask" can be used to compute the value to describe the bit,
and that is made up of "shift" that describes the start of the bitfield
and "port" that describes the bit within the bitfield, then surely:

	mask = BIT(port + shift);

would be saner?

Next, the creation of "val". This is either zero or the same value of
mask depending on active_low and polarity_active_high. The truth table
here is:

polarity_active_high	active_low	result
0			0		0
0			1		mask
1			0		mask
1			1		0

This is a classical an exclusive-or truth table in the world of logic,
or could be regarded as an inquality relationship (result is mask
when polarity_active_high differs from active_low, otherwise zero).

Thus:

	/* Set the bit when polarity_active_high differs from active_low */
	val = polarity_active_high != active_low ? mask : 0;

Or, even simpler, this could become overall:

	mask = BIT(port + shift);

	miic->phylink.mask |= mask;
	if (polarity_active_high != active_low)
		miic->phylink.val |= mask;
	else
		miic->phylink.val &= ~mask;

> @@ -605,8 +698,15 @@ static int miic_parse_dt(struct miic *miic, u32 *mode_cfg)
>  
>  		/* Adjust for 0 based index */
>  		port += !miic->of_data->miic_port_start;
> -		if (of_property_read_u32(conv, "renesas,miic-input", &conf) == 0)
> -			dt_val[port] = conf;
> +		if (of_property_read_u32(conv, "renesas,miic-input", &conf))
> +			continue;
> +
> +		dt_val[port] = conf;
> +
> +		active_low = of_property_read_bool(conv, "renesas,miic-phylink-active-low");
> +
> +		miic_configure_phylink(miic, conf, port - !miic->of_data->miic_port_start,
> +				       active_low);

I think this is also over-complicated. Wouldn't it be better to only
deal with the miic_port_start at the one place that it matters?

                if (of_property_read_u32(conv, "reg", &port))
                        continue;

                if (of_property_read_u32(conv, "renesas,miic-input", &conf))
                        continue;

                dt_val[port + !miic->of_data->miic_port_start] = conf;

                active_low = of_property_read_bool(conv, "renesas,miic-phylink-active-low");

                miic_configure_phylink(miic, conf, port, active_low);

?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!