Re: [PATCH v3 3/5] net: phy: qcom: at803x: Add Qualcomm IPQ5018 Internal PHY support

[Rob Herring <robh@kernel.org>]

On Mon, Jun 02, 2025 at 02:41:56PM +0200, Andrew Lunn wrote:
> > +	/* PHY DAC values are optional and only set in a PHY to PHY link architecture */
> > +	if (priv->set_short_cable_dac) {
> > +		/* setting MDAC (Multi-level Digital-to-Analog Converter) in MMD1 */
> > +		phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, IPQ5018_PHY_MMD1_MDAC,
> > +			       IPQ5018_PHY_DAC_MASK, IPQ5018_PHY_MMD1_MDAC_VAL);
> > +
> > +		/* setting EDAC (Error-detection and Correction) in debug register */
> > +		at803x_debug_reg_mask(phydev, IPQ5018_PHY_DEBUG_EDAC,
> > +				      IPQ5018_PHY_DAC_MASK, IPQ5018_PHY_DEBUG_EDAC_VAL);
> > +	}
> 
> In the binding you say:
> 
> 
> +            If not set, it is assumed the MDI output pins of this PHY are directly
> +            connected to an RJ45 connector and default DAC values will be used.
> 
> So shouldn't there be an else clause here setting these two values to
> their default, undoing what the bootloader might of done etc.
> 
> Or you can change the binding, and say something like:
> 
> +            If not set, DAC values are not modified.
> 
> We often need a tristate in DT, set something, unset something, leave
> it as it is. But that does not exist in DT in an easy form :-(

I'm happy to define that and have thought about it. That does up the 
minimum version of dtschema required, but we pretty much expect people 
to use the latest anyways.

Rob