Message ID | 20230317023125.486-5-ansuelsmth@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:604a:0:0:0:0:0 with SMTP id j10csp106659wrt; Thu, 16 Mar 2023 19:34:39 -0700 (PDT) X-Google-Smtp-Source: AK7set8pMETqTMBk3yU7vkoVmvho1U5oKOEgeZeMKHWHXxQiCIzsFry8vU2G+YdawFKG+0vjd8i7 X-Received: by 2002:a62:1845:0:b0:60d:461a:d03c with SMTP id 66-20020a621845000000b0060d461ad03cmr4834589pfy.27.1679020479400; Thu, 16 Mar 2023 19:34:39 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1679020479; cv=none; d=google.com; s=arc-20160816; b=bAkp++eyHsUAOWuAOUvzvmchvzvdoGE9+sV+IvbZhdfQO2wiNfMqhhpQ+Leay++8JK 8wnx8p0cWJnsS32GEYSiyq+qyBJe1OIqBq7m/13q/GLkGKdFaasDWsZvNQFu6B1FXyAW G19a3p2Ab3mdbdrFpRR/t6msDlEkRqyBJ/uqFSQFxGFNe6HH9fZNT5P+59+8Oyj3jPx7 HV1EqQpiyTV2ug0hYTm7uOZAuTa3K17FIBfblhzYA1c0pr2EXRx3ZC//ZwsFOaQA81Yw 8jipv2tBtYJU2HjACZO8Z/8s0vrL95uTyfcl0mC0M6h3JaZijmTGOk5OAeIg77oTHGTe 7mEQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:to:from :dkim-signature; bh=VM3DoNMLu3F/k6wPO3qYiXVgdAsBr9H+lF3WwRw8JHY=; b=tqvfkI5JhuXHcJElgLN443UeYrohEnw6LzeG65mvN6UQcLj38ktUdU7iyT3XbiCZ4z QMbTUFmLfH+zCnuOYIekRf2s4OHhKrhZKor0IAFXGln4QV1wG3KA2O0OWixeLfTjndQZ Hv8koxCTrNq+B0Xot1n7bbx99WoQQdB/Y7YvcfoEBzFMrIJlG91t2A2cJkMZViQJEihZ Gz4fasdwtrkB0mGKWwndDxQ1N4DW48m1OSPk1xVOABa0UVKn2oYBF410bizk6EcSzXPA Qoeth86oIayfU/khJlJAPWUDZNXKyz2W59Sz+EduvHlitE/sA7yUDDCQyyOP1txb70MV cO7A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=GI3I0nTO; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id p41-20020a056a0026e900b0059b98e82de3si1040291pfw.314.2023.03.16.19.34.26; Thu, 16 Mar 2023 19:34:39 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=GI3I0nTO; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229972AbjCQCdr (ORCPT <rfc822;pwkd43@gmail.com> + 99 others); Thu, 16 Mar 2023 22:33:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40318 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229863AbjCQCda (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 16 Mar 2023 22:33:30 -0400 Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 42FBC3B87F; Thu, 16 Mar 2023 19:33:27 -0700 (PDT) Received: by mail-wm1-x32a.google.com with SMTP id j42-20020a05600c1c2a00b003ed363619ddso2707955wms.1; Thu, 16 Mar 2023 19:33:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1679020405; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:from:to:cc:subject:date:message-id :reply-to; bh=VM3DoNMLu3F/k6wPO3qYiXVgdAsBr9H+lF3WwRw8JHY=; b=GI3I0nTOe0aBeLI0l3/lUpIAY7Pet947XD0nFynMbOz6OX8jfYop8gvJZHiLq1O3Xo rcHANTn9UTVOALoTXuXcKBprhvxFjsmTK3W72xmyTFGPfERQj6rg2Fz7N1I3iCG8a30e Gyz0o2jBWMN4XUnPY0qsSFkk8CogUxAZsdiIrl9ObI0TVov2j2i6l37Ftwtnjdl2s9ei 6n/MEl+U4oEGOoygSea+KB9B9/7ccQYCNDBI5eXRQLyi1nPxMZ3voYJLEJnyrszEXhFX fHmHGKqsc4SM8MJo2Eu659xoog8RDZ+D5EuQ8mW+nSqjIORg/cbzza7H+xNx3Z0R+GR8 KcTA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679020405; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=VM3DoNMLu3F/k6wPO3qYiXVgdAsBr9H+lF3WwRw8JHY=; b=zVN5eh/GBboxcAoVfgZXCMPC0B1aTjPUug9af4qI76fsbSVZg2yiIhsYZOqF7G6oBc BUXdwGDByD8l60ALgbPQfueRWSMEgCL/MCI+uOO8U160ntaWvRocvkreL4MUK5Ui/iZO 1oLCOaKRIiQa2VpLUG5DL5pv0fz30qIggLhAJXxQAMxpx21jraFR7NlapO+LxLWkuv7a 4Fw3atNv6NwKWgx5pqppchvU77jCSlQNVsoroiJ5ah/EI0wRWKYSfj/tgkOoaOngs00Q RROWig2zgg0ZdXqegCJTWebPr/O18Nx/RM0++MiF6Ks8MBe4Ck/7bAJftwEoWV1iMztN KPeg== X-Gm-Message-State: AO0yUKV0YTY8wsdXlNLIqsBPBR6E/NZkG5XKDAbzMIK808A+MAOv2ChA eiWAhk8RLlRTTH9wr7xjB74= X-Received: by 2002:a1c:f701:0:b0:3ed:809b:79ac with SMTP id v1-20020a1cf701000000b003ed809b79acmr405573wmh.19.1679020405234; Thu, 16 Mar 2023 19:33:25 -0700 (PDT) Received: from localhost.localdomain (93-34-89-197.ip49.fastwebnet.it. [93.34.89.197]) by smtp.googlemail.com with ESMTPSA id z15-20020a5d44cf000000b002ce9f0e4a8fsm782313wrr.84.2023.03.16.19.33.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Mar 2023 19:33:24 -0700 (PDT) From: Christian Marangi <ansuelsmth@gmail.com> To: Andrew Lunn <andrew@lunn.ch>, Florian Fainelli <f.fainelli@gmail.com>, Vladimir Oltean <olteanv@gmail.com>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Heiner Kallweit <hkallweit1@gmail.com>, Russell King <linux@armlinux.org.uk>, Gregory Clement <gregory.clement@bootlin.com>, Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>, Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Christian Marangi <ansuelsmth@gmail.com>, John Crispin <john@phrozen.org>, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, Lee Jones <lee@kernel.org>, linux-leds@vger.kernel.org Subject: [net-next PATCH v4 04/14] net: phy: Add a binding for PHY LEDs Date: Fri, 17 Mar 2023 03:31:15 +0100 Message-Id: <20230317023125.486-5-ansuelsmth@gmail.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230317023125.486-1-ansuelsmth@gmail.com> References: <20230317023125.486-1-ansuelsmth@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1760580577800472522?= X-GMAIL-MSGID: =?utf-8?q?1760580577800472522?= |
Series |
net: Add basic LED support for switch/phy
|
|
Commit Message
Christian Marangi
March 17, 2023, 2:31 a.m. UTC
From: Andrew Lunn <andrew@lunn.ch> Define common binding parsing for all PHY drivers with LEDs using phylib. Parse the DT as part of the phy_probe and add LEDs to the linux LED class infrastructure. For the moment, provide a dummy brightness function, which will later be replaced with a call into the PHY driver. Signed-off-by: Andrew Lunn <andrew@lunn.ch> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- drivers/net/phy/Kconfig | 1 + drivers/net/phy/phy_device.c | 75 ++++++++++++++++++++++++++++++++++++ include/linux/phy.h | 16 ++++++++ 3 files changed, 92 insertions(+)
Comments
On Fri, 17 Mar 2023 03:31:15 +0100 Christian Marangi <ansuelsmth@gmail.com> wrote: > + cdev->brightness_set_blocking = phy_led_set_brightness; > + cdev->max_brightness = 1; > + init_data.devicename = dev_name(&phydev->mdio.dev); > + init_data.fwnode = of_fwnode_handle(led); > + > + err = devm_led_classdev_register_ext(dev, cdev, &init_data); Since init_data.devname_mandatory is false, devicename is ignored. Which is probably good, becuse the device name of a mdio device is often ugly, taken from devicetree or switch drivers, for example: f1072004.mdio-mii fixed-0 mv88e6xxx-1 So either don't fill devicename or use devname_mandatory (and maybe fill devicename with something less ugly, but I guess if we don't have much choice if we want to keep persistent names). Without devname_mandatory, the name of the LED classdev will be of the form color:function[-function-enumerator], i.e. green:lan amber:lan-1 With multiple switch ethenret ports all having LAN function, it is worth noting that the function enumerator must be explicitly used in the devicetree, otherwise multiple LEDs will be registered under the same name, and the LED subsystem will add a number at the and of the name (function led_classdev_next_name), resulting in names green:lan green:lan_1 green:lan_2 ... These names are dependent on the order of classdev registration. Marek
On Fri, Mar 17, 2023 at 03:31:15AM +0100, Christian Marangi wrote: > From: Andrew Lunn <andrew@lunn.ch> > > Define common binding parsing for all PHY drivers with LEDs using > phylib. Parse the DT as part of the phy_probe and add LEDs to the > linux LED class infrastructure. For the moment, provide a dummy > brightness function, which will later be replaced with a call into the > PHY driver. > Hi Andrew, Personally, I see no good reason to provide a dummy implementation of "phy_led_set_brightness", especially if you implement it in the next patch. You only use that function only the function pointer in "led_classdev". I think you can just skip it in this patch. Please find the rest of my comments inline. Thanks, Michal > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > drivers/net/phy/Kconfig | 1 + > drivers/net/phy/phy_device.c | 75 ++++++++++++++++++++++++++++++++++++ > include/linux/phy.h | 16 ++++++++ > 3 files changed, 92 insertions(+) > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index f5df2edc94a5..666efa6b1c8e 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -16,6 +16,7 @@ config PHYLINK > menuconfig PHYLIB > tristate "PHY Device support and infrastructure" > depends on NETDEVICES > + depends on LEDS_CLASS > select MDIO_DEVICE > select MDIO_DEVRES > help > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 9ba8f973f26f..ee800f93c8c3 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -19,10 +19,12 @@ > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/kernel.h> > +#include <linux/list.h> > #include <linux/mdio.h> > #include <linux/mii.h> > #include <linux/mm.h> > #include <linux/module.h> > +#include <linux/of.h> > #include <linux/netdevice.h> > #include <linux/phy.h> > #include <linux/phy_led_triggers.h> > @@ -658,6 +660,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, > device_initialize(&mdiodev->dev); > > dev->state = PHY_DOWN; > + INIT_LIST_HEAD(&dev->leds); > > mutex_init(&dev->lock); > INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine); > @@ -2964,6 +2967,73 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv) > return phydrv->config_intr && phydrv->handle_interrupt; > } > > +/* Dummy implementation until calls into PHY driver are added */ > +static int phy_led_set_brightness(struct led_classdev *led_cdev, > + enum led_brightness value) > +{ > + return 0; > +} It can be removed from this patch. > + > +static int of_phy_led(struct phy_device *phydev, > + struct device_node *led) > +{ > + struct device *dev = &phydev->mdio.dev; > + struct led_init_data init_data = {}; > + struct led_classdev *cdev; > + struct phy_led *phyled; > + int err; > + > + phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL); > + if (!phyled) > + return -ENOMEM; > + > + cdev = &phyled->led_cdev; > + > + err = of_property_read_u32(led, "reg", &phyled->index); > + if (err) > + return err; Memory leak. 'phyled' is not freed in case of error. > + > + cdev->brightness_set_blocking = phy_led_set_brightness; Please move this initialization to the patch where you are actually implementing this callback. > + cdev->max_brightness = 1; > + init_data.devicename = dev_name(&phydev->mdio.dev); > + init_data.fwnode = of_fwnode_handle(led); > + > + err = devm_led_classdev_register_ext(dev, cdev, &init_data); > + if (err) > + return err; Another memory leak. > + > + list_add(&phyled->list, &phydev->leds); Where do you free the memory allocated for phy_led structure? > + > + return 0; > +} > + > +static int of_phy_leds(struct phy_device *phydev) > +{ > + struct device_node *node = phydev->mdio.dev.of_node; > + struct device_node *leds, *led; > + int err; > + > + if (!IS_ENABLED(CONFIG_OF_MDIO)) > + return 0; > + > + if (!node) > + return 0; > + > + leds = of_get_child_by_name(node, "leds"); > + if (!leds) > + return 0; > + > + for_each_available_child_of_node(leds, led) { > + err = of_phy_led(phydev, led); > + if (err) { > + of_node_put(led); > + return err; > + } > + } > + > + return 0; > +} > + > /** > * fwnode_mdio_find_device - Given a fwnode, find the mdio_device > * @fwnode: pointer to the mdio_device's fwnode > @@ -3142,6 +3212,11 @@ static int phy_probe(struct device *dev) > /* Set the state to READY by default */ > phydev->state = PHY_READY; > > + /* Get the LEDs from the device tree, and instantiate standard > + * LEDs for them. > + */ > + err = of_phy_leds(phydev); > + > out: > /* Assert the reset signal */ > if (err) > diff --git a/include/linux/phy.h b/include/linux/phy.h > index fbeba4fee8d4..88a77ff60be9 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -14,6 +14,7 @@ > #include <linux/compiler.h> > #include <linux/spinlock.h> > #include <linux/ethtool.h> > +#include <linux/leds.h> > #include <linux/linkmode.h> > #include <linux/netlink.h> > #include <linux/mdio.h> > @@ -595,6 +596,7 @@ struct macsec_ops; > * @phy_num_led_triggers: Number of triggers in @phy_led_triggers > * @led_link_trigger: LED trigger for link up/down > * @last_triggered: last LED trigger for link speed > + * @leds: list of PHY LED structures > * @master_slave_set: User requested master/slave configuration > * @master_slave_get: Current master/slave advertisement > * @master_slave_state: Current master/slave configuration > @@ -690,6 +692,7 @@ struct phy_device { > > struct phy_led_trigger *led_link_trigger; > #endif > + struct list_head leds; > > /* > * Interrupt number for this PHY > @@ -825,6 +828,19 @@ struct phy_plca_status { > bool pst; > }; > > +/** > + * struct phy_led: An LED driven by the PHY > + * > + * @list: List of LEDs > + * @led_cdev: Standard LED class structure > + * @index: Number of the LED > + */ > +struct phy_led { > + struct list_head list; > + struct led_classdev led_cdev; > + u32 index; > +}; > + > /** > * struct phy_driver - Driver structure for a particular PHY type > * > -- > 2.39.2 >
On Fri, Mar 17, 2023 at 08:45:19AM +0100, Marek Behún wrote: > On Fri, 17 Mar 2023 03:31:15 +0100 > Christian Marangi <ansuelsmth@gmail.com> wrote: > > > + cdev->brightness_set_blocking = phy_led_set_brightness; > > + cdev->max_brightness = 1; > > + init_data.devicename = dev_name(&phydev->mdio.dev); > > + init_data.fwnode = of_fwnode_handle(led); > > + > > + err = devm_led_classdev_register_ext(dev, cdev, &init_data); > > Since init_data.devname_mandatory is false, devicename is ignored. > Which is probably good, becuse the device name of a mdio device is > often ugly, taken from devicetree or switch drivers, for example: > f1072004.mdio-mii > fixed-0 > mv88e6xxx-1 > So either don't fill devicename or use devname_mandatory (and maybe > fill devicename with something less ugly, but I guess if we don't have > much choice if we want to keep persistent names). > > Without devname_mandatory, the name of the LED classdev will be of the > form > color:function[-function-enumerator], > i.e. > green:lan > amber:lan-1 > > With multiple switch ethenret ports all having LAN function, it is > worth noting that the function enumerator must be explicitly used in the > devicetree, otherwise multiple LEDs will be registered under the same > name, and the LED subsystem will add a number at the and of the name > (function led_classdev_next_name), resulting in names > green:lan > green:lan_1 > green:lan_2 > ... I'm testing on a Marvell RDK, with limited LEDs. It has one LED on the front port to represent the WAN port. The DT patch is at the end of the series. With that, i end up with: root@370rd:/sys/class/leds# ls -l total 0 lrwxrwxrwx 1 root root 0 Mar 17 01:10 f1072004.mdio-mii:00:WAN -> ../../devices/platform/soc/soc:interna l-regs/f1072004.mdio/mdio_bus/f1072004.mdio-mii/f1072004.mdio-mii:00/leds/f1072004.mdio-mii:00:WAN I also have: root@370rd:/sys/class/net/eth0/phydev/leds# ls f1072004.mdio-mii:00:WAN f1072004.mdio-mii:00: is not nice, but it is unique to a netdev. The last part then comes from the label property. Since there is only one LED, i went with what the port is intended to be used as. If there had been more LEDs, i would of probably used labels like "LINK" and "ACTIVITY", since that is often what they reset default to. Alternatively, you could names the "Left" and "Right", which does suggest they can be given any function. I don't actually think the name is too important, so long as it is unique. You are going to find it via /sys/class/net. MAC LEDs should be /sys/class/net/eth42/leds, and PHY LEDs will be /sys/class/net/phydev/leds. It has been discussed in the past to either extend ethtool to understand this, or write a new little tool to make it easier to manipulate these LEDs. Andrew
On Fri, Mar 17, 2023 at 02:38:15PM +0100, Michal Kubiak wrote: > On Fri, Mar 17, 2023 at 03:31:15AM +0100, Christian Marangi wrote: > > From: Andrew Lunn <andrew@lunn.ch> > > > > Define common binding parsing for all PHY drivers with LEDs using > > phylib. Parse the DT as part of the phy_probe and add LEDs to the > > linux LED class infrastructure. For the moment, provide a dummy > > brightness function, which will later be replaced with a call into the > > PHY driver. > > > > Hi Andrew, > > Personally, I see no good reason to provide a dummy implementation > of "phy_led_set_brightness", especially if you implement it in the next > patch. You only use that function only the function pointer in > "led_classdev". I think you can just skip it in this patch. Hi Michal The basic code for this patch has been sitting in my tree for a long time. It used to be, if you did not have a set_brightness method in cdev, the registration failed. That made it hard to test this patch on its own during development work, did i have the link list correct, can i unload the PHY driver without it exploding etc. I need to check if it is still mandatory. > > +static int of_phy_led(struct phy_device *phydev, > > + struct device_node *led) > > +{ > > + struct device *dev = &phydev->mdio.dev; > > + struct led_init_data init_data = {}; > > + struct led_classdev *cdev; > > + struct phy_led *phyled; > > + int err; > > + > > + phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL); > > + if (!phyled) > > + return -ENOMEM; > > + > > + cdev = &phyled->led_cdev; > > + > > + err = of_property_read_u32(led, "reg", &phyled->index); > > + if (err) > > + return err; > > Memory leak. 'phyled' is not freed in case of error. devm_ API, so it gets freed when the probe fails. > > + > > + cdev->brightness_set_blocking = phy_led_set_brightness; > > Please move this initialization to the patch where you are actually > implementing this callback. > > > + cdev->max_brightness = 1; > > + init_data.devicename = dev_name(&phydev->mdio.dev); > > + init_data.fwnode = of_fwnode_handle(led); > > + > > + err = devm_led_classdev_register_ext(dev, cdev, &init_data); > > + if (err) > > + return err; > > Another memory leak. Ah, maybe you don't know about devm_ ? devm_ allocations and actions register an action to be taken when the device is removed, either because the probe failed, or when the device is unregistered. For memory allocation, the memory is freed automagically. For actions like registering an LED, requesting an interrupt etc, an unregister/release is performed. This makes cleanup less buggy since the core does it. Andrew
On Fri, 17 Mar 2023 14:55:11 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > On Fri, Mar 17, 2023 at 08:45:19AM +0100, Marek Behún wrote: > > On Fri, 17 Mar 2023 03:31:15 +0100 > > Christian Marangi <ansuelsmth@gmail.com> wrote: > > > > > + cdev->brightness_set_blocking = phy_led_set_brightness; > > > + cdev->max_brightness = 1; > > > + init_data.devicename = dev_name(&phydev->mdio.dev); > > > + init_data.fwnode = of_fwnode_handle(led); > > > + > > > + err = devm_led_classdev_register_ext(dev, cdev, &init_data); > > > > Since init_data.devname_mandatory is false, devicename is ignored. > > Which is probably good, becuse the device name of a mdio device is > > often ugly, taken from devicetree or switch drivers, for example: > > f1072004.mdio-mii > > fixed-0 > > mv88e6xxx-1 > > So either don't fill devicename or use devname_mandatory (and maybe > > fill devicename with something less ugly, but I guess if we don't have > > much choice if we want to keep persistent names). > > > > Without devname_mandatory, the name of the LED classdev will be of the > > form > > color:function[-function-enumerator], > > i.e. > > green:lan > > amber:lan-1 > > > > With multiple switch ethenret ports all having LAN function, it is > > worth noting that the function enumerator must be explicitly used in the > > devicetree, otherwise multiple LEDs will be registered under the same > > name, and the LED subsystem will add a number at the and of the name > > (function led_classdev_next_name), resulting in names > > green:lan > > green:lan_1 > > green:lan_2 > > ... > > I'm testing on a Marvell RDK, with limited LEDs. It has one LED on the > front port to represent the WAN port. The DT patch is at the end of > the series. With that, i end up with: > > root@370rd:/sys/class/leds# ls -l > total 0 > lrwxrwxrwx 1 root root 0 Mar 17 01:10 f1072004.mdio-mii:00:WAN -> ../../devices/platform/soc/soc:interna > l-regs/f1072004.mdio/mdio_bus/f1072004.mdio-mii/f1072004.mdio-mii:00/leds/f1072004.mdio-mii:00:WAN > > I also have: > > root@370rd:/sys/class/net/eth0/phydev/leds# ls > f1072004.mdio-mii:00:WAN Hmm, yes I see. If label is specified, devicename is used even if devname_mandatory is false. > f1072004.mdio-mii:00: is not nice, but it is unique to a netdev. The > last part then comes from the label property. Since there is only one > LED, i went with what the port is intended to be used as. If there had > been more LEDs, i would of probably used labels like "LINK" and > "ACTIVITY", since that is often what they reset default > to. Alternatively, you could names the "Left" and "Right", which does > suggest they can be given any function. > > I don't actually think the name is too important, so long as it is > unique. You are going to find it via /sys/class/net. MAC LEDs should > be /sys/class/net/eth42/leds, and PHY LEDs will be > /sys/class/net/phydev/leds. Maybe the name may not be that important from the perspective of a user who just wants to find the LED for a given phy, yes, but the proposal of how LED classdev should be named was done in good faith and accepted years ago. The documentation still defines the name format and until that part of documenation is changed, I think we should at least try to follow it. Also, the label DT property has been deprecated for LEDs. IMO it should be removed from that last patch of this series. > It has been discussed in the past to either extend ethtool to > understand this, or write a new little tool to make it easier to > manipulate these LEDs. Yes, and this would solve the problem for a user who wants to change the behaviour of a LED for a given PHY. But a user who wants to list all available LEDs by listing /sys/class/leds can also retrieve a nice list of names that make sense, if the documented format is followed. Marek
On Fri, Mar 17, 2023 at 03:03:32PM +0100, Andrew Lunn wrote: > > Hi Andrew, > > > > Personally, I see no good reason to provide a dummy implementation > > of "phy_led_set_brightness", especially if you implement it in the next > > patch. You only use that function only the function pointer in > > "led_classdev". I think you can just skip it in this patch. > > Hi Michal > > The basic code for this patch has been sitting in my tree for a long > time. It used to be, if you did not have a set_brightness method in > cdev, the registration failed. That made it hard to test this patch on > its own during development work, did i have the link list correct, can > i unload the PHY driver without it exploding etc. I need to check if > it is still mandatory. > Thank you for the explanation. I was not aware of failing registration in case of undefined "cdev->brightness_set_blocking". I think it is a good reason of defining the dummy function. (The only alternative would be to squash two commits, but I think it is easier to review smaller chunks of code). > > > +static int of_phy_led(struct phy_device *phydev, > > > + struct device_node *led) > > > +{ > > > + struct device *dev = &phydev->mdio.dev; > > > + struct led_init_data init_data = {}; > > > + struct led_classdev *cdev; > > > + struct phy_led *phyled; > > > + int err; > > > + > > > + phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL); > > > + if (!phyled) > > > + return -ENOMEM; > > > + > > > + cdev = &phyled->led_cdev; > > > + > > > + err = of_property_read_u32(led, "reg", &phyled->index); > > > + if (err) > > > + return err; > > > > Memory leak. 'phyled' is not freed in case of error. > > devm_ API, so it gets freed when the probe fails. > > > > + > > > + cdev->brightness_set_blocking = phy_led_set_brightness; > > > > Please move this initialization to the patch where you are actually > > implementing this callback. > > > > > + cdev->max_brightness = 1; > > > + init_data.devicename = dev_name(&phydev->mdio.dev); > > > + init_data.fwnode = of_fwnode_handle(led); > > > + > > > + err = devm_led_classdev_register_ext(dev, cdev, &init_data); > > > + if (err) > > > + return err; > > > > Another memory leak. > > Ah, maybe you don't know about devm_ ? devm_ allocations and actions > register an action to be taken when the device is removed, either > because the probe failed, or when the device is unregistered. For > memory allocation, the memory is freed automagically. For actions like > registering an LED, requesting an interrupt etc, an unregister/release > is performed. This makes cleanup less buggy since the core does it. > > Andrew Yeah, it is my fault, I apologize for that. I didn't consider neither the probe() context, nor the lifetime of the list. You are right - I had no experience with using this devm_ API, so I looked at it as a standard memory allocation. Thank you for your patience and this piece of knowledge. Thanks, Michal
> Yes, and this would solve the problem for a user who wants to change > the behaviour of a LED for a given PHY. But a user who wants to list > all available LEDs by listing /sys/class/leds can also retrieve a nice > list of names that make sense, if the documented format is followed. Please make a concrete proposal. Also, keep in mind, ethernet device names change at runtime, and are not unique. Function also changes at run time, which is in fact the whole purpose of this collection of patchsets. Andrew
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index f5df2edc94a5..666efa6b1c8e 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -16,6 +16,7 @@ config PHYLINK menuconfig PHYLIB tristate "PHY Device support and infrastructure" depends on NETDEVICES + depends on LEDS_CLASS select MDIO_DEVICE select MDIO_DEVRES help diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 9ba8f973f26f..ee800f93c8c3 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -19,10 +19,12 @@ #include <linux/interrupt.h> #include <linux/io.h> #include <linux/kernel.h> +#include <linux/list.h> #include <linux/mdio.h> #include <linux/mii.h> #include <linux/mm.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/netdevice.h> #include <linux/phy.h> #include <linux/phy_led_triggers.h> @@ -658,6 +660,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, device_initialize(&mdiodev->dev); dev->state = PHY_DOWN; + INIT_LIST_HEAD(&dev->leds); mutex_init(&dev->lock); INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine); @@ -2964,6 +2967,73 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv) return phydrv->config_intr && phydrv->handle_interrupt; } +/* Dummy implementation until calls into PHY driver are added */ +static int phy_led_set_brightness(struct led_classdev *led_cdev, + enum led_brightness value) +{ + return 0; +} + +static int of_phy_led(struct phy_device *phydev, + struct device_node *led) +{ + struct device *dev = &phydev->mdio.dev; + struct led_init_data init_data = {}; + struct led_classdev *cdev; + struct phy_led *phyled; + int err; + + phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL); + if (!phyled) + return -ENOMEM; + + cdev = &phyled->led_cdev; + + err = of_property_read_u32(led, "reg", &phyled->index); + if (err) + return err; + + cdev->brightness_set_blocking = phy_led_set_brightness; + cdev->max_brightness = 1; + init_data.devicename = dev_name(&phydev->mdio.dev); + init_data.fwnode = of_fwnode_handle(led); + + err = devm_led_classdev_register_ext(dev, cdev, &init_data); + if (err) + return err; + + list_add(&phyled->list, &phydev->leds); + + return 0; +} + +static int of_phy_leds(struct phy_device *phydev) +{ + struct device_node *node = phydev->mdio.dev.of_node; + struct device_node *leds, *led; + int err; + + if (!IS_ENABLED(CONFIG_OF_MDIO)) + return 0; + + if (!node) + return 0; + + leds = of_get_child_by_name(node, "leds"); + if (!leds) + return 0; + + for_each_available_child_of_node(leds, led) { + err = of_phy_led(phydev, led); + if (err) { + of_node_put(led); + return err; + } + } + + return 0; +} + /** * fwnode_mdio_find_device - Given a fwnode, find the mdio_device * @fwnode: pointer to the mdio_device's fwnode @@ -3142,6 +3212,11 @@ static int phy_probe(struct device *dev) /* Set the state to READY by default */ phydev->state = PHY_READY; + /* Get the LEDs from the device tree, and instantiate standard + * LEDs for them. + */ + err = of_phy_leds(phydev); + out: /* Assert the reset signal */ if (err) diff --git a/include/linux/phy.h b/include/linux/phy.h index fbeba4fee8d4..88a77ff60be9 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -14,6 +14,7 @@ #include <linux/compiler.h> #include <linux/spinlock.h> #include <linux/ethtool.h> +#include <linux/leds.h> #include <linux/linkmode.h> #include <linux/netlink.h> #include <linux/mdio.h> @@ -595,6 +596,7 @@ struct macsec_ops; * @phy_num_led_triggers: Number of triggers in @phy_led_triggers * @led_link_trigger: LED trigger for link up/down * @last_triggered: last LED trigger for link speed + * @leds: list of PHY LED structures * @master_slave_set: User requested master/slave configuration * @master_slave_get: Current master/slave advertisement * @master_slave_state: Current master/slave configuration @@ -690,6 +692,7 @@ struct phy_device { struct phy_led_trigger *led_link_trigger; #endif + struct list_head leds; /* * Interrupt number for this PHY @@ -825,6 +828,19 @@ struct phy_plca_status { bool pst; }; +/** + * struct phy_led: An LED driven by the PHY + * + * @list: List of LEDs + * @led_cdev: Standard LED class structure + * @index: Number of the LED + */ +struct phy_led { + struct list_head list; + struct led_classdev led_cdev; + u32 index; +}; + /** * struct phy_driver - Driver structure for a particular PHY type *