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

[Andrew Lunn <andrew@lunn.ch>]

> Anyway the difference is that they made 2 different package version of
> the qca8327. One with 176 pin and one with 148 pin.

I assume they have different product numbers. So you can quote them in
the DT binding? Are they BGP or QFP? Can somebody easily count the
pins?

> > > +
> > > +	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.

So first off, it sounds like you have the DT property named wrong. It
should be 'qca,ignore-power-on-sel'.

However, why do you even need this? Generally, strapping gives you the
defaults. Registers get loaded with a value determined by the
strapping. But after that, you can change the value, based on
additional information. Or are you saying the register is read only
when strapping is used?

> > > +	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;

At minimum, you need to clearly document that qca,led-open-drain
implies 'qca,ignore-power-on-sel'. I would probably go further and
return -EINVAL if qca,led-open-drain is set and
'qca,ignore-power-on-sel' is not.

	Andrew