mbox series

[net-next,v1,0/3] Add support for LEDs on Marvell PHYs

Message ID 20200908000300.6982-1-marek.behun@nic.cz
Headers show
Series Add support for LEDs on Marvell PHYs | expand

Message

Marek Behún Sept. 8, 2020, 12:02 a.m. UTC
Hi,

after 4 RFC versions I am now sending these patches for real.

The code applies on net-next.

Changes since RFC v4:
- added device-tree binding documentation 
- the OF code now checks for linux,default-hw-mode property so that
  default HW mode can be set in device tree (like linux,default-trigger)
  (this was suggested by Andrew)
- the OF code also checks for enable-active-high and led-open-drain
  properties, and the marvell PHY driver now understands uses these
  settings when initializing the LEDs
- the LED operations were moved to their own struct phy_device_led_ops
- a new member was added into struct phy_device: phyindex. This is an
  incrementing integer, new for each registered phy_device. This is used
  for a simple naming scheme for the devicename part of a LED, as was
  suggested in a discussion by Andrew and Pavel. A PHY controlled LED
  now has a name in form:
    phy%i:color:function
  When a PHY is attached to a netdevice, userspace can control available
  PHY controlled LEDs via /sys/class/net/<ifname>/phydev/leds/
- legacy LED configuration in Marvell PHY driver (in function
  marvell_config_led) now writes only to registers which do not
  correspond to any registered LED

Changes since RFC v3:
- addressed some of Andrew's suggestions
- phy_hw_led_mode.c renamed to phy_led.c
- the DT reading code is now also generic, moved to phy_led.c and called
  from phy_probe
- the function registering the phydev-hw-mode trigger is now called from
  phy_device.c function phy_init before registering genphy drivers
- PHY LED functionality now depends on CONFIG_LEDS_TRIGGERS

Changes since RFC v2:
- to share code with other drivers which may want to also offer PHY HW
  control of LEDs some of the code was refactored and now resides in
  phy_hw_led_mode.c. This code is compiled in when config option
  LED_TRIGGER_PHY_HW is enabled. Drivers wanting to offer PHY HW control
  of LEDs should depend on this option.
- the "hw-control" trigger is renamed to "phydev-hw-mode" and is
  registered by the code in phy_hw_led_mode.c
- the "hw_control" sysfs file is renamed to "hw_mode"
- struct phy_driver is extended by three methods to support PHY HW LED
  control
- I renamed the various HW control modes offeret by Marvell PHYs to
  conform to other Linux mode names, for example the "1000/100/10/else"
  mode was renamed to "1Gbps/100Mbps/10Mbps", or "recv/else" was renamed
  to "rx" (this is the name of the mode in netdev trigger).

Marek Behún (3):
  dt-bindings: net: ethernet-phy: add description for PHY LEDs
  net: phy: add API for LEDs controlled by ethernet PHY chips
  net: phy: marvell: add support for LEDs controlled by Marvell PHYs

 .../devicetree/bindings/net/ethernet-phy.yaml |  31 ++
 drivers/net/phy/Kconfig                       |   4 +
 drivers/net/phy/Makefile                      |   1 +
 drivers/net/phy/marvell.c                     | 309 +++++++++++++++++-
 drivers/net/phy/phy_device.c                  |  28 +-
 drivers/net/phy/phy_led.c                     | 224 +++++++++++++
 include/linux/phy.h                           |  91 ++++++
 7 files changed, 679 insertions(+), 9 deletions(-)
 create mode 100644 drivers/net/phy/phy_led.c

Comments

Marek Behún Sept. 8, 2020, 10:02 p.m. UTC | #1
Please ignore this patch, still refactoring...
Pavel Machek Oct. 4, 2020, 9:58 a.m. UTC | #2
Hi!

> Many an ethernet PHY supports various HW control modes for LEDs
> connected directly to the PHY chip.
> 
> This patch adds code for registering such LEDs when described in device
> tree and also adds a new private LED trigger called phydev-hw-mode.
> When this trigger is enabled for a LED, the various HW control modes
> which are supported by the PHY for given LED cat be get/set via hw_mode
> sysfs file.
> 
> A PHY driver wishing to utilize this API needs to implement all the
> methods in the phy_device_led_ops structure.
> 
> Signed-off-by: Marek Beh??n <marek.behun@nic.cz>


>  	select MDIO_I2C
>  
> +config PHY_LEDS
> +	bool
> +	default y if LEDS_TRIGGERS
> +
>  comment "MII PHY device drivers"
>  
>  config AMD_PHY

> +/* drivers/net/phy/phy_hw_led_mode.c
> + *

Stale comment.

> +	init_data.fwnode = &np->fwnode;
> +	init_data.devname_mandatory = true;
> +	snprintf(devicename, sizeof(devicename), "phy%d", phydev->phyindex);
> +	init_data.devicename = devicename;
> +
> +	ret = phydev->led_ops->led_init(phydev, led, &pdata);
> +	if (ret < 0)
> +		goto err_free;
> +
> +	ret = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data);
> +	if (ret < 0)
> +		goto err_free;
> +
> +	led->flags |= PHY_DEVICE_LED_REGISTERED;
> +
> +	return 0;
> +err_free:
> +	devm_kfree(&phydev->mdio.dev, led);
> +	return ret;

devm should take care of freeing, right?

Plus, format comments to 80 colums. checkpatch no longer warns, but rule still exists.

Best regards,
									Pavel