netdev - Re: [PATCH net-next v2] net: phy: add driver for MaxLinear MxL86110 PHY

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: <de1514f8-7612-4a26-a74e-cf87ce3c8819@lunn.ch>
Date: Mon, 19 May 2025 14:35:15 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Stefano Radaelli <stefano.radaelli21@...il.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Xu Liang <lxu@...linear.com>
Subject: Re: [PATCH net-next v2] net: phy: add driver for MaxLinear MxL86110
 PHY

> +static int mxl86110_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> +{
> +	struct net_device *netdev;
> +	const u8 *mac;
> +	int ret = 0;
> +
> +	phy_lock_mdio_bus(phydev);
> +
> +	if (wol->wolopts & WAKE_MAGIC) {
> +		netdev = phydev->attached_dev;
> +		if (!netdev) {
> +			ret = -ENODEV;
> +			goto error;
> +		}

...

> +
> +	phy_unlock_mdio_bus(phydev);
> +	return 0;
> +error:
> +	phy_unlock_mdio_bus(phydev);
> +	return ret;
> +}

You should be able to simplify this. If you have not had an error, ret
should be 0. So you can also return ret. You have the same pattern in
other places.

> +/**
> + * mxl86110_synce_clk_cfg() - applies syncE/clk output configuration
> + * @phydev: pointer to the phy_device
> + *
> + * Custom settings can be defined in custom config section of the driver
> + * returns 0 or negative errno code
> + */

Maybe add a comment that the bus is expected to be locked.

> +static int mxl86110_synce_clk_cfg(struct phy_device *phydev)
> +{
> +	u16 mask = 0, value = 0;
> +	int ret = 0;
> +
> +	/*
> +	 * Configures the clock output to its default setting as per the datasheet.
> +	 * This results in a 25MHz clock output being selected in the
> +	 * COM_EXT_SYNCE_CFG register for SyncE configuration.
> +	 */
> +	value = MXL86110_EXT_SYNCE_CFG_EN_SYNC_E |
> +			FIELD_PREP(MXL86110_EXT_SYNCE_CFG_CLK_SRC_SEL_MASK,
> +				   MXL86110_EXT_SYNCE_CFG_CLK_SRC_SEL_25M);
> +	mask = MXL86110_EXT_SYNCE_CFG_EN_SYNC_E |
> +	       MXL86110_EXT_SYNCE_CFG_CLK_SRC_SEL_MASK |
> +	       MXL86110_EXT_SYNCE_CFG_CLK_FRE_SEL;
> +
> +	/* Write clock output configuration */
> +	ret = mxl86110_modify_extended_reg(phydev, MXL86110_EXT_SYNCE_CFG_REG,
> +					   mask, value);
> +
> +	return ret;
> +}
> +
> +/**
> + * mxl86110_broadcast_cfg - Configure MDIO broadcast setting for PHY
> + * @phydev: Pointer to the PHY device structure
> + *
> + * This function configures the MDIO broadcast behavior of the MxL86110 PHY.
> + * Currently, broadcast mode is explicitly disabled by clearing the EPA0 bit
> + * in the RGMII_MDIO_CFG extended register.

here as well.

> + *
> + * Return: 0 on success or a negative errno code on failure.
> + */
> +static int mxl86110_broadcast_cfg(struct phy_device *phydev)
> +{
> +	int ret = 0;
> +	u16 val;
> +
> +	val = mxl86110_read_extended_reg(phydev, MXL86110_EXT_RGMII_MDIO_CFG);
> +	if (val < 0)
> +		return val;
> +
> +	val &= ~MXL86110_EXT_RGMII_MDIO_CFG_EPA0_MASK;
> +	ret = mxl86110_write_extended_reg(phydev, MXL86110_EXT_RGMII_MDIO_CFG, val);

Could _modify_ be used here?

> +
> +	return ret;
> +}
> +
> +/**
> + * mxl86110_enable_led_activity_blink - Enable LEDs activity blink on PHY
> + * @phydev: Pointer to the PHY device structure
> + *
> + * Configure all PHY LEDs to blink on traffic activity regardless of their
> + * ON or OFF state. This behavior allows each LED to serve as a pure activity
> + * indicator, independently of its use as a link status indicator.
> + *
> + * By default, each LED blinks only when it is also in the ON state. This function
> + * modifies the appropriate registers (LABx fields) to enable blinking even
> + * when the LEDs are OFF, to allow the LED to be used as a traffic indicator
> + * without requiring it to also serve as a link status LED.
> + *
> + * NOTE: Any further LED customization can be performed via the
> + * /sys/class/led interface; the functions led_hw_is_supported, led_hw_control_get, and
> + * led_hw_control_set are used to support this mechanism.
> + *
> + * Return: 0 on success or a negative errno code on failure.
> + */
> +static int mxl86110_enable_led_activity_blink(struct phy_device *phydev)
> +{
> +	int ret, index;
> +	u16 val = 0;
> +
> +	for (index = 0; index < MXL86110_MAX_LEDS; index++) {
> +		val = mxl86110_read_extended_reg(phydev, MXL86110_LED0_CFG_REG + index);
> +		if (val < 0)
> +			return val;
> +
> +		val |= MXL86110_LEDX_CFG_TRAFFIC_ACT_BLINK_IND;
> +		ret = mxl86110_write_extended_reg(phydev, MXL86110_LED0_CFG_REG + index, val);
> +		if (ret < 0)
> +			return ret;

_modify_ ?

Getting pretty close to finished now. Thanks for keeping working on
it.

	Andrew