netdev - Re: [RFC] net: phy: air_en8811h: add Airoha AN8811HB support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <497ad08d-2603-4159-a4ce-52bdc5361aed@lunn.ch>
Date: Tue, 2 Dec 2025 17:12:43 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Bjørn Mork <bjorn@...k.no>
Cc: netdev@...r.kernel.org, "Lucien.Jheng" <lucienzx159@...il.com>,
	Daniel Golle <daniel@...rotopia.org>,
	Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [RFC] net: phy: air_en8811h: add Airoha AN8811HB support

>  static int en8811h_load_firmware(struct phy_device *phydev)
>  {
>  	struct en8811h_priv *priv = phydev->priv;
>  	struct device *dev = &phydev->mdio.dev;
>  	const struct firmware *fw1, *fw2;
> +	bool en8811h;
>  	int ret;
>  
> -	ret = request_firmware_direct(&fw1, EN8811H_MD32_DM, dev);
> -	if (ret < 0)
> -		return ret;
> +	switch (phydev->phy_id & PHY_ID_MATCH_MODEL_MASK) {
> +	case EN8811H_PHY_ID & PHY_ID_MATCH_MODEL_MASK:
> +		ret = request_firmware_direct(&fw1, EN8811H_MD32_DM, dev);
> +		if (ret < 0)
> +			return ret;
>  
> -	ret = request_firmware_direct(&fw2, EN8811H_MD32_DSP, dev);
> -	if (ret < 0)
> -		goto en8811h_load_firmware_rel1;
> +		ret = request_firmware_direct(&fw2, EN8811H_MD32_DSP, dev);
> +		if (ret < 0)
> +			goto en8811h_load_firmware_rel1;
> +
> +		en8811h = true;
> +		break;
> +
> +	case AN8811HB_PHY_ID & PHY_ID_MATCH_MODEL_MASK:
> +		ret = request_firmware_direct(&fw1, AN8811HB_MD32_DM, dev);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = request_firmware_direct(&fw2, AN8811HB_MD32_DSP, dev);
> +		if (ret < 0)
> +			goto en8811h_load_firmware_rel1;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
>  
>  	ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
>  				     EN8811H_FW_CTRL_1_START);
> +	if (ret == 0 && en8811h)

Generally, you don't test for 0. If there is an error you return
it. Does this need to be special?

> +		ret = air_buckpbus_reg_modify(phydev, EN8811H_FW_CTRL_2,
> +					      EN8811H_FW_CTRL_2_LOADING,
> +					      EN8811H_FW_CTRL_2_LOADING);
>  	if (ret < 0)
>  		goto en8811h_load_firmware_out;
>  
> -	ret = air_buckpbus_reg_modify(phydev, EN8811H_FW_CTRL_2,
> -				      EN8811H_FW_CTRL_2_LOADING,
> -				      EN8811H_FW_CTRL_2_LOADING);
> +	ret = air_write_buf(phydev, AIR_FW_ADDR_DM,  fw1);
>  	if (ret < 0)
>  		goto en8811h_load_firmware_out;
>  
> -	ret = air_write_buf(phydev, AIR_FW_ADDR_DM,  fw1);
> +	if (ret == 0 && !en8811h)
> +		ret = an8811hb_check_crc(phydev, AN8811HB_CRC_DM_SET1,
> +					 AN8811HB_CRC_DM_MON2,
> +					 AN8811HB_CRC_DM_MON3);

This !en881h and en881h looks a bit ugly. Maybe see if you can
refactor this code into helpers for the two cases? 

> @@ -952,8 +1141,9 @@ static int en8811h_probe(struct phy_device *phydev)
>  	}
>  
>  	priv->phydev = phydev;
> +
>  	/* Co-Clock Output */
> -	ret = en8811h_clk_provider_setup(&phydev->mdio.dev, &priv->hw);
> +	ret = en8811h_clk_provider_setup(phydev, &priv->hw);
>  	if (ret)
>  		return ret;

Maybe look at having two different probe functions, with a helper for
any common code? That might mean you don't need as many switch
statements. And this is a common pattern when dealing with variants of
hardware. You have a collection of helpers which are generic, and then
version specific functions which make use of the helpers are
appropriate.

This patch is also quite large. See if you can break it
up. Refactoring the existing code into helpers can be a patch of its
own.

	Andrew