Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 11 Aug 2023 14:14:20 +0200
From:   Simon Horman <horms@kernel.org>
To:     Daniel Golle <daniel@makrotopia.org>
Cc:     Qingfang Deng <dqfext@gmail.com>,
        SkyLake Huang <SkyLake.Huang@mediatek.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org
Subject: Re: [PATCH net-next] net: phy: mediatek-ge-soc: support PHY LEDs
Message-ID: <ZNYmHFkC5ZxySq1h@vergenet.net>
References: <a21288bf80f26dda6c9729edb5b25d0995df5e38.1691724757.git.daniel@makrotopia.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a21288bf80f26dda6c9729edb5b25d0995df5e38.1691724757.git.daniel@makrotopia.org>
Precedence: bulk

On Fri, Aug 11, 2023 at 04:35:38AM +0100, Daniel Golle wrote:
> Implement netdev trigger and primitive bliking offloading as well as
> simple set_brigthness function for both PHY LEDs of the in-SoC PHYs
> found in MT7981 and MT7988.
> 
> On MT7988 it is necessary to read the boottrap register and apply LED
> polarities accordingly to get uniform behavior from all LEDs.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>

...

> +static int mt798x_phy_hw_led_on_set(struct phy_device *phydev, u8 index,
> +				    bool on)
> +{
> +	struct mtk_socphy_priv *priv = phydev->priv;
> +	u32 mask = MTK_PHY_LED_STATE_FORCE_ON << (index ? 16 : 0);
> +	bool changed;
> +
> +	if (on)
> +		changed = (test_and_set_bit(mask, &priv->led_state) != mask);
> +	else
> +		changed = !!test_and_clear_bit(mask, &priv->led_state);

Hi Daniel,

are you sure the first parameter to test_and_set_bit() and
test_and_clear_bit() is correct here and below?

Smatch warns that: test_and_clear_bit() takes a bit number

I.e. the first argument should be a bit number, not a mask.

> +
> +	changed |= !!test_and_clear_bit(MTK_PHY_LED_STATE_NETDEV <<
> +					(index ? 16 : 0), &priv->led_state);
> +	if (changed)
> +		return phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
> +				      MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL,
> +				      MTK_PHY_LED_ON_MASK,
> +				      on ? MTK_PHY_LED_ON_FORCE_ON : 0);
> +	else
> +		return 0;
> +}
> +
> +static int mt798x_phy_hw_led_blink_set(struct phy_device *phydev, u8 index,
> +				       bool blinking)
> +{
> +	struct mtk_socphy_priv *priv = phydev->priv;
> +	u32 mask = MTK_PHY_LED_STATE_FORCE_BLINK << (index ? 16 : 0);
> +	bool changed;
> +
> +	if (blinking)
> +		changed = (test_and_set_bit(mask, &priv->led_state) != mask);
> +	else
> +		changed = !!test_and_clear_bit(mask, &priv->led_state);
> +
> +	changed |= !!test_bit(MTK_PHY_LED_STATE_NETDEV << (index ? 16 : 0), &priv->led_state);
> +	if (changed)
> +		return phy_write_mmd(phydev, MDIO_MMD_VEND2, index ?
> +				     MTK_PHY_LED1_BLINK_CTRL : MTK_PHY_LED0_BLINK_CTRL,
> +				     blinking ? MTK_PHY_LED_BLINK_FORCE_BLINK : 0);
> +	else
> +		return 0;
> +}

...

> +static int mt798x_phy_led_hw_control_get(struct phy_device *phydev, u8 index,
> +					 unsigned long *rules)
> +{
> +	u32 blink_mask = MTK_PHY_LED_STATE_FORCE_BLINK << (index ? 16 : 0);
> +	u32 netdev_mask = MTK_PHY_LED_STATE_NETDEV << (index ? 16 : 0);
> +	u32 on_mask = MTK_PHY_LED_STATE_FORCE_ON << (index ? 16 : 0);
> +	struct mtk_socphy_priv *priv = phydev->priv;
> +	int on, blink;
> +
> +	if (index > 1)
> +		return -EINVAL;
> +
> +	on = phy_read_mmd(phydev, MDIO_MMD_VEND2,
> +			  index ? MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL);
> +
> +	if (on < 0)
> +		return -EIO;
> +
> +	blink = phy_read_mmd(phydev, MDIO_MMD_VEND2,
> +			     index ? MTK_PHY_LED1_BLINK_CTRL :
> +				     MTK_PHY_LED0_BLINK_CTRL);
> +	if (blink < 0)
> +		return -EIO;
> +
> +	if ((on & (MTK_PHY_LED_ON_LINK1000 | MTK_PHY_LED_ON_LINK100 |
> +		   MTK_PHY_LED_ON_LINK10)) ||
> +	    (blink & (MTK_PHY_LED_BLINK_1000RX | MTK_PHY_LED_BLINK_100RX |
> +		      MTK_PHY_LED_BLINK_10RX | MTK_PHY_LED_BLINK_1000TX |
> +		      MTK_PHY_LED_BLINK_100TX | MTK_PHY_LED_BLINK_10TX)))
> +		set_bit(netdev_mask, &priv->led_state);
> +	else
> +		clear_bit(netdev_mask, &priv->led_state);
> +
> +	if (on & MTK_PHY_LED_ON_FORCE_ON)
> +		set_bit(on_mask, &priv->led_state);
> +	else
> +		clear_bit(on_mask, &priv->led_state);
> +
> +	if (blink & MTK_PHY_LED_BLINK_FORCE_BLINK)
> +		set_bit(blink_mask, &priv->led_state);
> +	else
> +		clear_bit(blink_mask, &priv->led_state);
> +
> +	if (!rules)
> +		return 0;
> +
> +	if (on & (MTK_PHY_LED_ON_LINK1000 | MTK_PHY_LED_ON_LINK100 | MTK_PHY_LED_ON_LINK10))
> +		*rules |= BIT(TRIGGER_NETDEV_LINK);
> +
> +	if (on & MTK_PHY_LED_ON_LINK10)
> +		*rules |= BIT(TRIGGER_NETDEV_LINK_10);
> +
> +	if (on & MTK_PHY_LED_ON_LINK100)
> +		*rules |= BIT(TRIGGER_NETDEV_LINK_100);
> +
> +	if (on & MTK_PHY_LED_ON_LINK1000)
> +		*rules |= BIT(TRIGGER_NETDEV_LINK_1000);
> +
> +	if (on & MTK_PHY_LED_ON_FDX)
> +		*rules |= BIT(TRIGGER_NETDEV_FULL_DUPLEX);
> +
> +	if (on & MTK_PHY_LED_ON_HDX)
> +		*rules |= BIT(TRIGGER_NETDEV_HALF_DUPLEX);
> +
> +	if (blink & (MTK_PHY_LED_BLINK_1000RX | MTK_PHY_LED_BLINK_100RX | MTK_PHY_LED_BLINK_10RX))
> +		*rules |= BIT(TRIGGER_NETDEV_RX);
> +
> +	if (blink & (MTK_PHY_LED_BLINK_1000TX | MTK_PHY_LED_BLINK_100TX | MTK_PHY_LED_BLINK_10TX))
> +		*rules |= BIT(TRIGGER_NETDEV_TX);
> +
> +	return 0;
> +};
> +
> +static int mt798x_phy_led_hw_control_set(struct phy_device *phydev, u8 index,
> +					 unsigned long rules)
> +{
> +	u32 mask = MTK_PHY_LED_STATE_NETDEV << (index ? 16 : 0);
> +	struct mtk_socphy_priv *priv = phydev->priv;
> +	u16 on = 0, blink = 0;
> +	int ret;
> +
> +	if (index > 1)
> +		return -EINVAL;
> +
> +	if (rules & BIT(TRIGGER_NETDEV_FULL_DUPLEX))
> +		on |= MTK_PHY_LED_ON_FDX;
> +
> +	if (rules & BIT(TRIGGER_NETDEV_HALF_DUPLEX))
> +		on |= MTK_PHY_LED_ON_HDX;
> +
> +	if (rules & (BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK)))
> +		on |= MTK_PHY_LED_ON_LINK10;
> +
> +	if (rules & (BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK)))
> +		on |= MTK_PHY_LED_ON_LINK100;
> +
> +	if (rules & (BIT(TRIGGER_NETDEV_LINK_1000) | BIT(TRIGGER_NETDEV_LINK)))
> +		on |= MTK_PHY_LED_ON_LINK1000;
> +
> +	if (rules & BIT(TRIGGER_NETDEV_RX)) {
> +		blink |= MTK_PHY_LED_BLINK_10RX  |
> +			 MTK_PHY_LED_BLINK_100RX |
> +			 MTK_PHY_LED_BLINK_1000RX;
> +	}
> +
> +	if (rules & BIT(TRIGGER_NETDEV_TX)) {
> +		blink |= MTK_PHY_LED_BLINK_10TX  |
> +			 MTK_PHY_LED_BLINK_100TX |
> +			 MTK_PHY_LED_BLINK_1000TX;
> +	}
> +
> +	if (blink || on)
> +		set_bit(mask, &priv->led_state);
> +	else
> +		clear_bit(mask, &priv->led_state);
> +
> +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
> +				MTK_PHY_LED1_ON_CTRL :
> +				MTK_PHY_LED0_ON_CTRL,
> +			     MTK_PHY_LED_ON_FDX     |
> +			     MTK_PHY_LED_ON_HDX     |
> +			     MTK_PHY_LED_ON_LINK10  |
> +			     MTK_PHY_LED_ON_LINK100 |
> +			     MTK_PHY_LED_ON_LINK1000,
> +			     on);
> +
> +	if (ret)
> +		return ret;
> +
> +	return phy_write_mmd(phydev, MDIO_MMD_VEND2, index ?
> +				MTK_PHY_LED1_BLINK_CTRL :
> +				MTK_PHY_LED0_BLINK_CTRL, blink);
> +};

...