linux-kernel - Re: [PATCH RFC leds + net-next v2 1/1] net: phy: marvell: add support for PHY LEDs via LED class

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: <20200723213531.GK1553578@lunn.ch>
Date:   Thu, 23 Jul 2020 23:35:31 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Marek Behún <marek.behun@....cz>
Cc:     linux-leds@...r.kernel.org, Pavel Machek <pavel@....cz>,
        jacek.anaszewski@...il.com, Dan Murphy <dmurphy@...com>,
        Ondřej Jirman <megous@...ous.com>,
        netdev@...r.kernel.org, Russell King <linux@...linux.org.uk>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Gregory Clement <gregory.clement@...tlin.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC leds + net-next v2 1/1] net: phy: marvell: add
 support for PHY LEDs via LED class

On Thu, Jul 23, 2020 at 08:13:19PM +0200, Marek Behún wrote:
> This patch adds support for controlling the LEDs connected to several
> families of Marvell PHYs via Linux' LED API. These families are:
> 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can be
> added.
> 
> The code reads LEDs definitions from the device-tree node of the PHY.
> 
> Since the LEDs can be controlled by hardware, we add one LED-private LED
> trigger named "hw-control". This trigger is private and displayed only
> for Marvell PHY LEDs.
> 
> When this driver is activated, another sysfs file is created in that
> LEDs sysfs directory, names "hw_control". This file contains space
> separated list of possible HW controlled modes for this LED. The one
> which is selected is enclosed by square brackets. To change HW control
> mode the user has to write the name of desired mode to this "hw_control"
> file.
> 
> This patch does not yet add support for compound LED modes. This could
> be achieved via the LED multicolor framework (which is not yet in
> upstream).
> 
> Settings such as HW blink rate or pulse stretch duration are not yet
> supported, nor are LED polarity settings.

Hi Marek

I expect some of this should be moved into the phylib core. We don't
want each PHY inventing its own way to do this. The core should
provide a framework and the PHY driver fills in the gaps.

Take a look at for example mscc_main.c and its LED information. It has
pretty similar hardware to the Marvell. And microchip.c also has LED
handling, etc.

> +static int _marvell_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness,
> +				       bool check_trigger)

Please avoid _ functions. 

> +{
> +	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
> +	struct marvell_phy_led *led = to_marvell_phy_led(cdev);
> +	u8 val;
> +
> +	/* don't do anything if HW control is enabled */
> +	if (check_trigger && cdev->trigger == &marvell_hw_led_trigger)
> +		return 0;

I thought the brightness file disappeared when a trigger takes
over. So is this possible?

      Andrew