From mboxrd@z Thu Jan 1 00:00:00 1970 From: Helmut Schaa Subject: Re: [PATCH 2/2] drivers: net: phy: at803x: LED control via led subsystem Date: Thu, 11 Jul 2013 14:57:47 +0200 Message-ID: References: <1373543855-31670-1-git-send-email-helmut.schaa@googlemail.com> <1373543855-31670-2-git-send-email-helmut.schaa@googlemail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Cc: netdev , David Miller To: Florian Fainelli Return-path: Received: from mail-ie0-f179.google.com ([209.85.223.179]:63158 "EHLO mail-ie0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754306Ab3GKM5r (ORCPT ); Thu, 11 Jul 2013 08:57:47 -0400 Received: by mail-ie0-f179.google.com with SMTP id c10so18358816ieb.10 for ; Thu, 11 Jul 2013 05:57:47 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Jul 11, 2013 at 2:15 PM, Florian Fainelli wrote: > Hello Helmut, > > 2013/7/11 Helmut Schaa : >> The AT8035 PHY allows to control the LED PIN behavior from software. >> Expose this functionality by registering an LED device. >> >> Signed-off-by: Helmut Schaa >> --- >> drivers/net/phy/Kconfig | 7 ++++ >> drivers/net/phy/at803x.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 94 insertions(+) >> >> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig >> index 3a316b3..5e855b7 100644 >> --- a/drivers/net/phy/Kconfig >> +++ b/drivers/net/phy/Kconfig >> @@ -19,6 +19,13 @@ config AT803X_PHY >> ---help--- >> Currently supports the AT8030 and AT8035 model >> >> +config AT803X_LEDS >> + bool "Enable access to PHY connected LEDs" >> + depends on AT803X_PHY && LEDS_CLASS >> + ---help--- >> + Select this to be able to control LEDs connected to the >> + PHY from userspace >> + >> config AMD_PHY >> tristate "Drivers for the AMD PHYs" >> ---help--- >> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c >> index ac22283..49ef6306 100644 >> --- a/drivers/net/phy/at803x.c >> +++ b/drivers/net/phy/at803x.c >> @@ -16,6 +16,7 @@ >> #include >> #include >> #include >> +#include >> >> #define AT803X_INTR_ENABLE 0x12 >> #define AT803X_INTR_STATUS 0x13 >> @@ -31,6 +32,8 @@ >> #define AT803X_DEBUG_DATA 0x1E >> #define AT803X_DEBUG_SYSTEM_MODE_CTRL 0x05 >> #define AT803X_DEBUG_RGMII_TX_CLK_DLY BIT(8) >> +#define AT8035_LED_CTRL 0x18 >> +#define AT8035_LED_CTRL_OFF BIT(15) >> >> MODULE_DESCRIPTION("Atheros 803x PHY driver"); >> MODULE_AUTHOR("Matus Ujhelyi"); >> @@ -152,6 +155,86 @@ static int at803x_config_init(struct phy_device *phydev) >> return 0; >> } >> >> +#ifdef CONFIG_AT803X_LEDS >> + >> +struct at8035_priv { >> + struct led_classdev led; >> + struct phy_device *phydev; >> + struct work_struct led_work; >> + bool stop; >> +}; >> + >> +static void at8035_led_work(struct work_struct *work) >> +{ >> + struct at8035_priv *priv = >> + container_of(work, struct at8035_priv, led_work); >> + struct phy_device *phydev = priv->phydev; >> + int val; >> + >> + if (priv->stop) >> + return; >> + >> + val = phy_read(phydev, AT8035_LED_CTRL); >> + >> + if (priv->led.brightness > 0) >> + phy_write(phydev, AT8035_LED_CTRL, val & ~AT8035_LED_CTRL_OFF); >> + else >> + phy_write(phydev, AT8035_LED_CTRL, val | AT8035_LED_CTRL_OFF); >> +} > > I do like this and I believe that there is nothing specific that would > prevent you from making software controllable LEDs from being used > with other PHYs. How about you create a small LED class helper that > PHY drivers supporting controllable LEDs can hook into and provide > their own callback and LED name for setting the LED state? Hmm, nice idea. However, there is not much complexity in the code. The only thing worth to make generic would be the delayed register access via workqueue. The rest is just registering the led device :) I thought about moving the code into the generic phy code and using a new phy_driver callback for setting the LED state but there might be more then one LED pin on some PHYs (activity, speed, duplexmode etc.). >> + >> +static void at8035_led_brightness_set(struct led_classdev *led, >> + enum led_brightness brightness) >> +{ >> + struct at8035_priv *priv = container_of(led, struct at8035_priv, led); >> + >> + /* MDIO bus can only be used from process context */ >> + if (!priv->stop) >> + schedule_work(&priv->led_work); > > It seems to me that you could remove the stop boolean if you move > led_classdev_unregister upper, provided that it guarantees it will not > schedule the workqueue. led_classdev_unregister will try to set brightness to 0 and thus schedule the work. Thanks, Helmut