From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 481A6CD343F for ; Thu, 7 May 2026 21:43:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=aX36hdB/vSHiUML6vh4mSgGpvQJsS+iDpluEkRtR6aE=; b=X1FI8+emVWSaEXxCjxYXRJe1QB poEvcIW2todPN5XKVu/4+y2qE3xfbviy7b/cAxYs0U1a6YzY6kOvbdvHJCOX6NikPnI5uJ1nyAJNn Xc6PLvSwZhjD4Mnc5WbFCPZQYvBF1oRhM2TYZfx2kyiBsiRuOCuU3XcDBqz4F/5YetgmmjjEFjj5u yMmCRvQCGmtdpwiApn0ClRB6uaZw5yMVYUZewHw5ChetFguEh1gg6AAoBoukBudQZpEE96yRR8837 BO03Vm4KCxPeTp51cyVxyzFyxm90UG/nZ6XM5poLzYZtHZG4+OV0NWkUQH5s5W0M8Nge2KfERpTpN ay25jPUg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1wL6VL-00000004yEX-13Mm; Thu, 07 May 2026 21:43:35 +0000 Received: from vps0.lunn.ch ([156.67.10.101]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1wL6VJ-00000004yDo-02ek; Thu, 07 May 2026 21:43:34 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch; s=20171124; h=In-Reply-To:Content-Transfer-Encoding:Content-Disposition: Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:From: Sender:Reply-To:Subject:Date:Message-ID:To:Cc:MIME-Version:Content-Type: Content-Transfer-Encoding:Content-ID:Content-Description:Content-Disposition: In-Reply-To:References; bh=aX36hdB/vSHiUML6vh4mSgGpvQJsS+iDpluEkRtR6aE=; b=ea atdxi3jr/eOn1JZiw5IZVBQjFj2PpSS0+PANzy7hG8LX3Cof1djFbbSkIoFJKTsaNiESIybpaHIIe buaeR2RXDSAyOuFwT5gM/AM+ocltCTsm9NhZjsJHbKdR5+QZuKl6QS5yDCxRxujASK7zvvB9tHBCI S7vBzmszdEb7zrQ=; Received: from andrew by vps0.lunn.ch with local (Exim 4.94.2) (envelope-from ) id 1wL6V6-001s5C-G8; Thu, 07 May 2026 23:43:20 +0200 Date: Thu, 7 May 2026 23:43:20 +0200 From: Andrew Lunn To: Louis-Alexis Eyraud Cc: Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Rob Herring , Krzysztof Kozlowski , Conor Dooley , AngeloGioacchino Del Regno , Heiner Kallweit , Russell King , kevin-kw.huang@airoha.com, macpaul.lin@mediatek.com, matthias.bgg@gmail.com, kernel@collabora.com, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next v2 4/4] net: phy: Introduce Airoha AN8801/R Gigabit Ethernet PHY driver Message-ID: <27ca6b71-18df-4e47-9117-0e503f7e7d5f@lunn.ch> References: <20260326-add-airoha-an8801-support-v2-0-1a42d6b6050f@collabora.com> <20260326-add-airoha-an8801-support-v2-4-1a42d6b6050f@collabora.com> <3688a285-7f98-4afa-80ad-697094cd7b97@lunn.ch> <2c441d51f6a865ddb6e67b63cd26a651ed3ff058.camel@collabora.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2c441d51f6a865ddb6e67b63cd26a651ed3ff058.camel@collabora.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.9.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20260507_144333_073827_B03BE784 X-CRM114-Status: GOOD ( 22.80 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org > > > +static int an8801r_of_init_leds(struct phy_device *phydev, u8 > > > *led_cfg) > > > +{ > > > + struct device *dev = &phydev->mdio.dev; > > > + struct device_node *np = dev->of_node; > > > + struct device_node *leds; > > > + u32 function_enum_idx; > > > + int ret; > > > + > > > + if (!np) > > > + return 0; > > > + > > > + /* If devicetree is present, leds configuration is > > > required */ > > > + leds = of_get_child_by_name(np, "leds"); > > > + if (!leds) > > > + return 0; > > > + > > > + for_each_available_child_of_node_scoped(leds, led) { > > > + u32 led_idx; > > > + > > > + ret = of_property_read_u32(led, "reg", &led_idx); > > > + if (ret) > > > + goto out; > > > + > > > + if (led_idx >= AN8801R_NUM_LEDS) { > > > + ret = -EINVAL; > > > + goto out; > > > + } > > > + > > > + ret = of_property_read_u32(led, "function- > > > enumerator", > > > +    &function_enum_idx); > > > + if (ret) > > > + function_enum_idx = AN8801R_LED_FN_NONE; > > > + > > > > What is this doing? Is this documented in the binding? > The `function-enumerator` property is only documented in the led common > dt-binding file. The an8801 dt-bindings inherits this property from the > ethernet-phy dt-bindings. > > We aimed to have this PHY have its led behaviour (how many to enable > and what their role shall be) configurable using devicetree and not to > rely on a default configuration, hard-coded in the driver (like the > air_en8811h driver did) and also make use of the led hardware > offloading (for functions like 100/1000, activity blinking, and others) > that this PHY is capable of. What other drivers do is leave the configuration with its reset default. They are often sensible. When the netdev trigger loads, it should ask the LED how it is configured, and the values in sysfs will reflect it. After that you can change it, via udev rules, etc. You have to be careful about what you put in DT. DT describes hardware, not configuration or policy. How the LED blinks is probably configuration, so it does not belong in DT. > > > +static int an8801r_read_status(struct phy_device *phydev) > > > +{ > > > + int prev_speed, ret; > > > + u32 val; > > > + > > > + prev_speed = phydev->speed; > > > + > > > + ret = genphy_read_status(phydev); > > > + if (ret) > > > + return ret; > > > + > > > + if (phydev->link && prev_speed != phydev->speed) { > > > + val = phydev->speed == SPEED_1000 ? > > > +       AN8801_BPBUS_LINK_MODE_1000 : 0; > > > + > > > + return an8801_buckpbus_reg_rmw(phydev, > > > +        > > > AN8801_BPBUS_REG_LINK_MODE, > > > +        > > > AN8801_BPBUS_LINK_MODE_1000, > > > +        val); > > > + }; > > > > This is unusual. What is it doing? Please add a comment. > This call is to ensure that the PHY switches to the expected 1Gbps > speed when available.  So this is an errata workaround? Please add this in a patch of its own, described the problem in the commit message, list the errata etc. Andrew