Re: [net-next PATCH 12/13] drivers: net: dsa: qca8k: add support for pws config reg

[Ansuel Smith <ansuelsmth@gmail.com>]

On Thu, Oct 07, 2021 at 02:41:10AM +0200, Andrew Lunn wrote:
> > +static int
> > +qca8k_setup_of_pws_reg(struct qca8k_priv *priv)
> > +{
> > +	struct device_node *node = priv->dev->of_node;
> > +	u32 val = 0;
> > +
> > +	if (priv->switch_id == QCA8K_ID_QCA8327)
> > +		if (of_property_read_bool(node, "qca,package48"))
> > +			val |= QCA8327_PWS_PACKAGE48_EN;
> 
> What does this actually do? How is PACKAGE48 different to normal mode?
>

I actually made a typo.
Anyway the difference is that they made 2 different package version of
the qca8327. One with 176 pin and one with 148 pin. Setting the wrong
layout cause the switch malfunction (no traffic, we found this on one
xiaomi device). This is from Documenation and it does toggle the MAC
interface configuration for the 2 different package.

> > +
> > +	if (of_property_read_bool(node, "qca,power-on-sel"))
> > +		val |= QCA8K_PWS_POWER_ON_SEL;
> 
> What happens if you unconditionally do this? Why is a DT property
> required?
> 

This is needed to bypass the power on strapping and use the regs config.
The switch can use hardware pin to set eeprom presence and leds open
drain. Setting this bit on bypass the hardware strapping and sets these
2 thing based on the regs. We didn't add the eeprom binding as we didn't
find any switch using it and we don't have any support for it.

> > +
> > +	if (of_property_read_bool(node, "qca,led-open-drain"))
> > +		/* POWER_ON_SEL needs to be set when configuring led to open drain */
> > +		val |= QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL;
> 
> This is getting into territory of adding LED support for PHYs, which
> we want to do via the LED subsystem.
> 

Don't know if it would be the correct way. Without this the switch leds
chaese to work. I think this should be handled in a dedicated way than
defined in a binding in the leds configuration. But I could be wrong.

>    Andrew

-- 
	Ansuel