| Message ID | 20200926135514.26189-1-luka.kovacic@sartura.hr |
|---|---|
| Headers | show |
| Series | Add support for the iEi Puzzle-M801 board | expand |
On Sat, Sep 26, 2020 at 03:55:14PM +0200, Luka Kovacic wrote: > Add initial support for the iEi Puzzle-M801 1U Rackmount Network > Appliance board. > > The board is based on the quad-core Marvell Armada 8040 SoC and supports > up to 16 GB of DDR4 2400 MHz ECC RAM. It has a PCIe x16 slot (x2 lanes > only) and an M.2 type B slot. > > Main system hardware: > 2x USB 3.0 > 4x Gigabit Ethernet > 2x SFP+ > 1x SATA 3.0 > 1x M.2 type B > 1x RJ45 UART > 1x SPI flash > 1x iEi WT61P803 PUZZLE Microcontroller > 1x EPSON RX8010 RTC (used instead of the integrated Marvell RTC controller) > 6x SFP+ LED > 1x HDD LED > > All of the hardware listed above is supported and tested in this port. > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > Cc: Luka Perkov <luka.perkov@sartura.hr> > Cc: Robert Marko <robert.marko@sartura.hr> I don't know this Marvell SoC too well, but what i see looks O.K. Reviewed-by: Andrew Lunn <andrew@lunn.ch> To get this merged, you probably need to break this patchset up and send the DT part to Gregory. It could be Lee will take the rest if there are ACKed by from the LED and HWMON maintainer. Andrew
On Sat, 26 Sep 2020 15:55:14 +0200 Luka Kovacic <luka.kovacic@sartura.hr> wrote: > + leds { > + compatible = "gpio-leds"; > + status = "okay"; > + pinctrl-0 = <&cp0_sfpplus_led_pins &cp1_sfpplus_led_pins>; > + pinctrl-names = "default"; > + > + led0 { > + function = LED_FUNCTION_STATUS; > + label = "p2_act"; > + gpios = <&cp1_gpio1 6 GPIO_ACTIVE_LOW>; > + }; There should be a dash in LED node name, please pass this dts via dt_binding_check led-0 { ... }; Also why not add the `color` property to the LED? This is DTS for a specific device, right? `label` is obsolete. The LED subsystem creates a name in form [device:]color:function If this LED should blink for activity on port 2 (is this an ethernet port?), the function should be LED_FUNCTION_LAN and function-enumerator should be <2> (or function should be LED_FUNCTION_ACTIVITY, depending on how the LED subsystem goes forward with this, but certainly not LED_FUNCTION_STATUS), and trigger-sources should be set to point to the ethernet port. Luka, are you willing to change this once we solve this API properly in LED subsystem? > + led6 { > + function = LED_FUNCTION_STATUS; > + linux,default-trigger = "disk-activity"; > + label = "front-hdd-led"; > + gpios = <&cp0_gpio2 22 GPIO_ACTIVE_HIGH>; > + }; led-6. LED_FUNCTION_DISK. `label` deprecated. > + leds { > + compatible = "iei,wt61p803-puzzle-leds"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + led@0 { > + reg = <0>; > + color = <LED_COLOR_ID_BLUE>; > + label = "front-power-led"; > + }; Again, `label` is deprecated. Rather use function = <LED_FUNCTION_POWER>; Marek
On Sat, 26 Sep 2020 15:55:08 +0200 Luka Kovacic <luka.kovacic@sartura.hr> wrote: > diff --git a/Documentation/devicetree/bindings/leds/iei,wt61p803-puzzle-leds.yaml b/Documentation/devicetree/bindings/leds/iei,wt61p803-puzzle-leds.yaml > new file mode 100644 > index 000000000000..502d97630ecc > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/iei,wt61p803-puzzle-leds.yaml > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/leds/iei,wt61p803-puzzle-leds.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: iEi WT61P803 PUZZLE MCU LED module from IEI Integration Corp. > + > +maintainers: > + - Luka Kovacic <luka.kovacic@sartura.hr> > + > +description: | > + This module is a part of the iEi WT61P803 PUZZLE MFD device. For more details > + see Documentation/devicetree/bindings/mfd/iei,wt61p803-puzzle.yaml. > + > + The LED module is a sub-node of the MCU node in the Device Tree. > + > +properties: > + compatible: > + const: iei,wt61p803-puzzle-leds > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0 > + > +patternProperties: > + "^led@0$": > + type: object Here should be a ref to LED common.yaml: $ref: common.yaml# > + description: | > + Properties for a single LED. > + > + properties: > + reg: > + description: > + Index of the LED. Only one LED is supported at the moment. > + minimum: 0 > + maximum: 0 > + > + label: true > + > + linux,default-trigger: true > + label is obsolete, linux,default-trigger as well. > +required: > + - compatible > + - "#address-cells" > + - "#size-cells" > diff --git a/Documentation/devicetree/bindings/mfd/iei,wt61p803-puzzle.yaml b/Documentation/devicetree/bindings/mfd/iei,wt61p803-puzzle.yaml > new file mode 100644 > index 000000000000..38846c758372 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/iei,wt61p803-puzzle.yaml > @@ -0,0 +1,82 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/iei,wt61p803-puzzle.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: iEi WT61P803 PUZZLE MCU from IEI Integration Corp. > + > +maintainers: > + - Luka Kovacic <luka.kovacic@sartura.hr> > + > +description: | > + iEi WT61P803 PUZZLE MCU is embedded in some iEi Puzzle series boards. > + It's used for controlling system power states, fans, LEDs and temperature > + sensors. > + > + For Device Tree bindings of other sub-modules (HWMON, LEDs) refer to the > + binding documents under the respective subsystem directories. > + > +properties: > + compatible: > + const: iei,wt61p803-puzzle > + > + current-speed: > + description: > + Serial bus speed in bps > + maxItems: 1 What does this mean? Is this connected via uart? Why not name it `baud`? I realize that `current-speed` is used in other device trees (and `baud` as well), but what "current" is this? I don't suppose this means electric current :) Have you passed these via dt_binding_check? Marek
On Sat, 26 Sep 2020 15:55:11 +0200 Luka Kovacic <luka.kovacic@sartura.hr> wrote: > Add support for the iEi WT61P803 PUZZLE LED driver. > Currently only the front panel power LED is supported. > > This driver depends on the iEi WT61P803 PUZZLE MFD driver. > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > Cc: Luka Perkov <luka.perkov@sartura.hr> > Cc: Robert Marko <robert.marko@sartura.hr> > --- > drivers/leds/Kconfig | 8 ++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-iei-wt61p803-puzzle.c | 174 ++++++++++++++++++++++++ > 3 files changed, 183 insertions(+) > create mode 100644 drivers/leds/leds-iei-wt61p803-puzzle.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 1c181df24eae..8a25fb753dec 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -332,6 +332,14 @@ config LEDS_IPAQ_MICRO > Choose this option if you want to use the notification LED on > Compaq/HP iPAQ h3100 and h3600. > > +config LEDS_IEI_WT61P803_PUZZLE > + tristate "LED Support for the iEi WT61P803 PUZZLE MCU" > + depends on LEDS_CLASS > + depends on MFD_IEI_WT61P803_PUZZLE > + help > + This option enables support for LEDs controlled by the iEi WT61P803 > + M801 MCU. > + > config LEDS_HP6XX > tristate "LED Support for the HP Jornada 6xx" > depends on LEDS_CLASS > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index c2c7d7ade0d0..cd362437fefd 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -34,6 +34,7 @@ obj-$(CONFIG_LEDS_HP6XX) += leds-hp6xx.o > obj-$(CONFIG_LEDS_INTEL_SS4200) += leds-ss4200.o > obj-$(CONFIG_LEDS_IP30) += leds-ip30.o > obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o > +obj-$(CONFIG_LEDS_IEI_WT61P803_PUZZLE) += leds-iei-wt61p803-puzzle.o > obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o > obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o > obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o > diff --git a/drivers/leds/leds-iei-wt61p803-puzzle.c b/drivers/leds/leds-iei-wt61p803-puzzle.c > new file mode 100644 > index 000000000000..b9a977575a23 > --- /dev/null > +++ b/drivers/leds/leds-iei-wt61p803-puzzle.c > @@ -0,0 +1,174 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* iEi WT61P803 PUZZLE MCU LED Driver > + * > + * Copyright (C) 2020 Sartura Ltd. > + * Author: Luka Kovacic <luka.kovacic@sartura.hr> > + */ > + > +#include <linux/leds.h> > +#include <linux/mfd/iei-wt61p803-puzzle.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/property.h> > +#include <linux/slab.h> > + > +enum iei_wt61p803_puzzle_led_state { > + IEI_LED_OFF = 0x30, > + IEI_LED_ON = 0x31, > + IEI_LED_BLINK_5HZ = 0x32, > + IEI_LED_BLINK_1HZ = 0x33, > +}; > + > +/** > + * struct iei_wt61p803_puzzle_led - MCU LED Driver > + * > + * @mcu: MCU struct pointer > + * @response_buffer Global MCU response buffer allocation > + * @lock: General mutex lock for LED operations > + * @led_power_state: State of the front panel power LED > + */ > +struct iei_wt61p803_puzzle_led { > + struct iei_wt61p803_puzzle *mcu; > + unsigned char *response_buffer; > + struct mutex lock; > + int led_power_state; > +}; > + > +static inline struct iei_wt61p803_puzzle_led *cdev_to_iei_wt61p803_puzzle_led > + (struct led_classdev *led_cdev) > +{ > + return dev_get_drvdata(led_cdev->dev->parent); > +} > + > +static int iei_wt61p803_puzzle_led_brightness_set_blocking(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + struct iei_wt61p803_puzzle_led *mcu_led = > + cdev_to_iei_wt61p803_puzzle_led(cdev); > + unsigned char led_power_cmd[5] = { > + IEI_WT61P803_PUZZLE_CMD_HEADER_START, > + IEI_WT61P803_PUZZLE_CMD_LED, > + IEI_WT61P803_PUZZLE_CMD_LED_POWER, > + (char)IEI_LED_OFF > + }; > + unsigned char *resp_buf = mcu_led->response_buffer; > + size_t reply_size; > + > + mutex_lock(&mcu_led->lock); > + if (brightness == LED_OFF) { > + led_power_cmd[3] = (char)IEI_LED_OFF; > + mcu_led->led_power_state = LED_OFF; > + } else { > + led_power_cmd[3] = (char)IEI_LED_ON; > + mcu_led->led_power_state = LED_ON; > + } > + mutex_unlock(&mcu_led->lock); > + > + return iei_wt61p803_puzzle_write_command(mcu_led->mcu, led_power_cmd, > + sizeof(led_power_cmd), resp_buf, &reply_size); > +} > + > +static enum led_brightness > +iei_wt61p803_puzzle_led_brightness_get(struct led_classdev *cdev) > +{ > + struct iei_wt61p803_puzzle_led *mcu_led = > + cdev_to_iei_wt61p803_puzzle_led(cdev); > + int led_state; > + > + mutex_lock(&mcu_led->lock); > + led_state = mcu_led->led_power_state; > + mutex_unlock(&mcu_led->lock); > + > + return led_state; > +} > + > +static int iei_wt61p803_puzzle_led_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev->parent); > + struct iei_wt61p803_puzzle_led *mcu_led; > + struct fwnode_handle *child; > + const char *label; > + int ret; > + > + mcu_led = devm_kzalloc(dev, sizeof(*mcu_led), GFP_KERNEL); > + if (!mcu_led) > + return -ENOMEM; > + > + mcu_led->response_buffer = devm_kzalloc(dev, > + IEI_WT61P803_PUZZLE_BUF_SIZE, GFP_KERNEL); > + if (!mcu_led->response_buffer) > + return -ENOMEM; > + > + mcu_led->mcu = mcu; > + mcu_led->led_power_state = 1; > + mutex_init(&mcu_led->lock); > + dev_set_drvdata(dev, mcu_led); > + > + device_for_each_child_node(dev, child) { > + struct led_classdev *led; > + u32 reg; > + > + led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL); Avoid multiple allocations. Please look at https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=4351dceba0ff929f667867f9bb69210f08f61717 To avoid multiple allocations, please use flexible array members. Does this controller support multiple LEDs? The device tree you provided only defines one. If it supports multiple LED: Rename the mcu_led to mcu_leds, or chip, or (semantically best in this driver) priv. Add member struct led_classdev leds[]; to that structure. Then allocate by count = device_get_child_node_count(dev); priv = devm_kzalloc(dev, struct_size(priv, leds, count), GFP_KERNEL); If the device supports only one LED, just put struct led_classdev cdev; to the private structure of this driver. And don't use device_for_each_child_node, just check whether there is exactly one child node (device_get_child_node_count), get it via child = device_get_next_child_node(dev, NULL); and after registering the LED fwnode_handle_put(child); This was done in: https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=e92e7e8aa066904def9a5d584ece7fb6ae512dbd > + if (!led) { > + ret = -ENOMEM; > + goto err_child_node; > + } > + > + ret = fwnode_property_read_u32(child, "reg", ®); > + if (ret || reg > 1) { > + dev_err(dev, "Could not register 'reg' (%lu)\n", (unsigned long)reg); > + ret = -EINVAL; > + goto err_child_node; > + } > + > + if (fwnode_property_read_string(child, "label", &label)) { > + led->name = "iei-wt61p803-puzzle-led::"; > + } else { > + led->name = devm_kasprintf(dev, GFP_KERNEL, > + "iei-wt61p803-puzzle-led:%s", label); > + if (!led->name) { > + ret = -ENOMEM; > + goto err_child_node; > + } > + } Parsing of label is done by LED core if you use devm_led_classdev_register_ext. Also, label is obsolete. The LED name should be composed from color, function and device. Also please dont pass devicename "iei-wt61p803-puzzle-led" here. We want to make the LED subsystem derive the device name somehow, and afterwards we would need to change this. Also the devicename should refer to the device the LED is triggering to (ie. if the LED is set in devicetree to trigger on activity on eth0, the devicename should be eth0 or something, not the name of this driver). Just remove this code and let devm_led_classdev_register_ext do its thing. > + > + fwnode_property_read_string(child, "linux,default-trigger", > + &led->default_trigger); > + Parsing of linux,default-trigger is done by LED core if you use devm_led_classdev_register_ext. > + led->brightness_set_blocking = iei_wt61p803_puzzle_led_brightness_set_blocking; > + led->brightness_get = iei_wt61p803_puzzle_led_brightness_get; > + led->max_brightness = 1; > + > + ret = devm_led_classdev_register(dev, led); Please use extended LED registration API, with devm_led_classdev_register_ext. Pass init_data with fwnode member set to child. > + if (ret) { > + dev_err(dev, "Could not register %s\n", led->name); > + goto err_child_node; > + } > + } > + return 0; > +err_child_node: > + fwnode_handle_put(child); > + return ret; > +} > + > +static const struct of_device_id iei_wt61p803_puzzle_led_of_match[] = { > + { .compatible = "iei,wt61p803-puzzle-leds" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, iei_wt61p803_puzzle_led_of_match); > + > +static struct platform_driver iei_wt61p803_puzzle_led_driver = { > + .driver = { > + .name = "iei-wt61p803-puzzle-led", > + .of_match_table = iei_wt61p803_puzzle_led_of_match, > + }, > + .probe = iei_wt61p803_puzzle_led_probe, > +}; > +module_platform_driver(iei_wt61p803_puzzle_led_driver); > + > +MODULE_DESCRIPTION("iEi WT61P803 PUZZLE front panel LED driver"); > +MODULE_AUTHOR("Luka Kovacic <luka.kovacic@sartura.hr>"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:leds-iei-wt61p803-puzzle"); Marek
On Sat, 26 Sep 2020 15:55:12 +0200 Luka Kovacic <luka.kovacic@sartura.hr> wrote: > Add the iei-wt61p803-puzzle driver sysfs interface documentation to allow > monitoring and control of the microcontroller from user space. > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > Cc: Luka Perkov <luka.perkov@sartura.hr> > Cc: Robert Marko <robert.marko@sartura.hr> > --- > .../stable/sysfs-driver-iei-wt61p803-puzzle | 65 +++++++++++++++++++ > 1 file changed, 65 insertions(+) > create mode 100644 Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle > > diff --git a/Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle b/Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle > new file mode 100644 > index 000000000000..36fca70d66ef > --- /dev/null > +++ b/Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle I think this should go to testing, not stable. It should go to stable only after it is stable for some time. > @@ -0,0 +1,65 @@ > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/mac_address_* > +Date: September 2020 > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > +Description: Read the internal iEi WT61P803 PUZZLE MCU MAC address values. > + These are factory assigned and can be changed. > + > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/serial_number > +Date: September 2020 > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > +Description: Read the internal iEi WT61P803 PUZZLE MCU serial number. > + This value is factory assigned and can be changed. > + Please use (RO) and (RW) prefixes before the Description, instead of writing "This value is read only", i.e.: Description: (RO) Internal ... serial number. JFI: Why can these values be changed? Shouldn't they be burned into OTP? Marek > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/version > +Date: September 2020 > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > +Description: Read the internal iEi WT61P803 PUZZLE MCU version. > + This value is read only. > + > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/protocol_version > +Date: September 2020 > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > +Description: Read the internal iEi WT61P803 PUZZLE MCU protocol version. > + This value is read only. > + > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_loss_recovery > +Date: September 2020 > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > +Description: Read the iEi WT61P803 PUZZLE MCU power loss recovery value. > + This value is read write. > + Value mapping: 0 - Always-On, 1 - Always-Off, 2 - Always-AC, 3 - Always-WA > + > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/bootloader_mode > +Date: September 2020 > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > +Description: Read whether the MCU is in bootloader mode. > + This value is read only. > + > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status > +Date: September 2020 > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > +Description: Read the iEi WT61P803 PUZZLE MCU power status. Power status indicates > + the power on method. > + This value is read only. > + Value mapping (bitwise list): > + 0x80 - Null > + 0x40 - Firmware flag > + 0x20 - Power loss detection flag (powered off) > + 0x10 - Power loss detection flag (AC mode) > + 0x08 - Button power on > + 0x04 - WOL power on > + 0x02 - RTC alarm power on > + 0x01 - AC recover power on > + > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/build_info > +Date: September 2020 > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > +Description: Read the iEi WT61P803 PUZZLE MCU firmware build date. > + This value is read only. > + Format: yyyy/mm/dd hh:mm > + > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/ac_recovery_status > +Date: September 2020 > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > +Description: Read the iEi WT61P803 PUZZLE MCU AC recovery status. > + This value is read only.
Hello Andrew and Marek, I will break the new patchset up and also add Gregory to the DT conversation. Should I exclude this patch from this patchset or can I just add him to Cc? First six LEDs are used to indicate port status and activity on the SFP+ ports. Certainly, I will change this once the API is solved. There are currently many similar boards with no real solution for the network LED triggers. I'll add the color and correct the function properties for the LEDs. Kind regards, Luka On Sat, Sep 26, 2020 at 7:50 PM Marek Behun <marek.behun@nic.cz> wrote: > > On Sat, 26 Sep 2020 15:55:14 +0200 > Luka Kovacic <luka.kovacic@sartura.hr> wrote: > > > + leds { > > + compatible = "gpio-leds"; > > + status = "okay"; > > + pinctrl-0 = <&cp0_sfpplus_led_pins &cp1_sfpplus_led_pins>; > > + pinctrl-names = "default"; > > + > > + led0 { > > + function = LED_FUNCTION_STATUS; > > + label = "p2_act"; > > + gpios = <&cp1_gpio1 6 GPIO_ACTIVE_LOW>; > > + }; > > There should be a dash in LED node name, please pass this dts via > dt_binding_check > led-0 { > ... > }; > > Also why not add the `color` property to the LED? This is DTS for a > specific device, right? > `label` is obsolete. The LED subsystem creates a name in form > [device:]color:function > If this LED should blink for activity on port 2 (is this an ethernet > port?), the function should be LED_FUNCTION_LAN and function-enumerator > should be <2> (or function should be LED_FUNCTION_ACTIVITY, depending > on how the LED subsystem goes forward with this, but certainly not > LED_FUNCTION_STATUS), and trigger-sources should be set to point to the > ethernet port. > > Luka, are you willing to change this once we solve this API properly > in LED subsystem? > > > > > + led6 { > > + function = LED_FUNCTION_STATUS; > > + linux,default-trigger = "disk-activity"; > > + label = "front-hdd-led"; > > + gpios = <&cp0_gpio2 22 GPIO_ACTIVE_HIGH>; > > + }; > > led-6. LED_FUNCTION_DISK. `label` deprecated. > > > + leds { > > + compatible = "iei,wt61p803-puzzle-leds"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + led@0 { > > + reg = <0>; > > + color = <LED_COLOR_ID_BLUE>; > > + label = "front-power-led"; > > + }; > > Again, `label` is deprecated. Rather use function = > <LED_FUNCTION_POWER>; > > Marek
Hi, The microcontroller on this platform is currently only physically wired to one LED, but should support multiple LEDs in software (this could possibly be used in future platforms). Thus I'd like to keep the support for multiple LEDs prepared. As I am not able to test multiple LEDs functionality, I will just keep the framework prepared. Also I'll use the devm_led_classdev_register_ext() API, thanks for letting me know. Kind regards, Luka On Sat, Sep 26, 2020 at 8:09 PM Marek Behun <marek.behun@nic.cz> wrote: > > On Sat, 26 Sep 2020 15:55:11 +0200 > Luka Kovacic <luka.kovacic@sartura.hr> wrote: > > > Add support for the iEi WT61P803 PUZZLE LED driver. > > Currently only the front panel power LED is supported. > > > > This driver depends on the iEi WT61P803 PUZZLE MFD driver. > > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > > Cc: Luka Perkov <luka.perkov@sartura.hr> > > Cc: Robert Marko <robert.marko@sartura.hr> > > --- > > drivers/leds/Kconfig | 8 ++ > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-iei-wt61p803-puzzle.c | 174 ++++++++++++++++++++++++ > > 3 files changed, 183 insertions(+) > > create mode 100644 drivers/leds/leds-iei-wt61p803-puzzle.c > > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > > index 1c181df24eae..8a25fb753dec 100644 > > --- a/drivers/leds/Kconfig > > +++ b/drivers/leds/Kconfig > > @@ -332,6 +332,14 @@ config LEDS_IPAQ_MICRO > > Choose this option if you want to use the notification LED on > > Compaq/HP iPAQ h3100 and h3600. > > > > +config LEDS_IEI_WT61P803_PUZZLE > > + tristate "LED Support for the iEi WT61P803 PUZZLE MCU" > > + depends on LEDS_CLASS > > + depends on MFD_IEI_WT61P803_PUZZLE > > + help > > + This option enables support for LEDs controlled by the iEi WT61P803 > > + M801 MCU. > > + > > config LEDS_HP6XX > > tristate "LED Support for the HP Jornada 6xx" > > depends on LEDS_CLASS > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > index c2c7d7ade0d0..cd362437fefd 100644 > > --- a/drivers/leds/Makefile > > +++ b/drivers/leds/Makefile > > @@ -34,6 +34,7 @@ obj-$(CONFIG_LEDS_HP6XX) += leds-hp6xx.o > > obj-$(CONFIG_LEDS_INTEL_SS4200) += leds-ss4200.o > > obj-$(CONFIG_LEDS_IP30) += leds-ip30.o > > obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o > > +obj-$(CONFIG_LEDS_IEI_WT61P803_PUZZLE) += leds-iei-wt61p803-puzzle.o > > obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o > > obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o > > obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o > > diff --git a/drivers/leds/leds-iei-wt61p803-puzzle.c b/drivers/leds/leds-iei-wt61p803-puzzle.c > > new file mode 100644 > > index 000000000000..b9a977575a23 > > --- /dev/null > > +++ b/drivers/leds/leds-iei-wt61p803-puzzle.c > > @@ -0,0 +1,174 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* iEi WT61P803 PUZZLE MCU LED Driver > > + * > > + * Copyright (C) 2020 Sartura Ltd. > > + * Author: Luka Kovacic <luka.kovacic@sartura.hr> > > + */ > > + > > +#include <linux/leds.h> > > +#include <linux/mfd/iei-wt61p803-puzzle.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/property.h> > > +#include <linux/slab.h> > > + > > +enum iei_wt61p803_puzzle_led_state { > > + IEI_LED_OFF = 0x30, > > + IEI_LED_ON = 0x31, > > + IEI_LED_BLINK_5HZ = 0x32, > > + IEI_LED_BLINK_1HZ = 0x33, > > +}; > > + > > +/** > > + * struct iei_wt61p803_puzzle_led - MCU LED Driver > > + * > > + * @mcu: MCU struct pointer > > + * @response_buffer Global MCU response buffer allocation > > + * @lock: General mutex lock for LED operations > > + * @led_power_state: State of the front panel power LED > > + */ > > +struct iei_wt61p803_puzzle_led { > > + struct iei_wt61p803_puzzle *mcu; > > + unsigned char *response_buffer; > > + struct mutex lock; > > + int led_power_state; > > +}; > > + > > +static inline struct iei_wt61p803_puzzle_led *cdev_to_iei_wt61p803_puzzle_led > > + (struct led_classdev *led_cdev) > > +{ > > + return dev_get_drvdata(led_cdev->dev->parent); > > +} > > + > > +static int iei_wt61p803_puzzle_led_brightness_set_blocking(struct led_classdev *cdev, > > + enum led_brightness brightness) > > +{ > > + struct iei_wt61p803_puzzle_led *mcu_led = > > + cdev_to_iei_wt61p803_puzzle_led(cdev); > > + unsigned char led_power_cmd[5] = { > > + IEI_WT61P803_PUZZLE_CMD_HEADER_START, > > + IEI_WT61P803_PUZZLE_CMD_LED, > > + IEI_WT61P803_PUZZLE_CMD_LED_POWER, > > + (char)IEI_LED_OFF > > + }; > > + unsigned char *resp_buf = mcu_led->response_buffer; > > + size_t reply_size; > > + > > + mutex_lock(&mcu_led->lock); > > + if (brightness == LED_OFF) { > > + led_power_cmd[3] = (char)IEI_LED_OFF; > > + mcu_led->led_power_state = LED_OFF; > > + } else { > > + led_power_cmd[3] = (char)IEI_LED_ON; > > + mcu_led->led_power_state = LED_ON; > > + } > > + mutex_unlock(&mcu_led->lock); > > + > > + return iei_wt61p803_puzzle_write_command(mcu_led->mcu, led_power_cmd, > > + sizeof(led_power_cmd), resp_buf, &reply_size); > > +} > > + > > +static enum led_brightness > > +iei_wt61p803_puzzle_led_brightness_get(struct led_classdev *cdev) > > +{ > > + struct iei_wt61p803_puzzle_led *mcu_led = > > + cdev_to_iei_wt61p803_puzzle_led(cdev); > > + int led_state; > > + > > + mutex_lock(&mcu_led->lock); > > + led_state = mcu_led->led_power_state; > > + mutex_unlock(&mcu_led->lock); > > + > > + return led_state; > > +} > > + > > +static int iei_wt61p803_puzzle_led_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev->parent); > > + struct iei_wt61p803_puzzle_led *mcu_led; > > + struct fwnode_handle *child; > > + const char *label; > > + int ret; > > + > > + mcu_led = devm_kzalloc(dev, sizeof(*mcu_led), GFP_KERNEL); > > + if (!mcu_led) > > + return -ENOMEM; > > + > > + mcu_led->response_buffer = devm_kzalloc(dev, > > + IEI_WT61P803_PUZZLE_BUF_SIZE, GFP_KERNEL); > > + if (!mcu_led->response_buffer) > > + return -ENOMEM; > > + > > + mcu_led->mcu = mcu; > > + mcu_led->led_power_state = 1; > > + mutex_init(&mcu_led->lock); > > + dev_set_drvdata(dev, mcu_led); > > + > > + device_for_each_child_node(dev, child) { > > + struct led_classdev *led; > > + u32 reg; > > + > > + led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL); > > Avoid multiple allocations. > > Please look > at > https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=4351dceba0ff929f667867f9bb69210f08f61717 > > To avoid multiple allocations, please use flexible array members. > > Does this controller support multiple LEDs? The device tree you > provided only defines one. > > If it supports multiple LED: > Rename the mcu_led to mcu_leds, or chip, or (semantically best in this > driver) priv. > Add member > struct led_classdev leds[]; > to that structure. > Then allocate by > count = device_get_child_node_count(dev); > priv = devm_kzalloc(dev, struct_size(priv, leds, count), GFP_KERNEL); > > If the device supports only one LED, just put > struct led_classdev cdev; > to the private structure of this driver. And don't use > device_for_each_child_node, just check whether there is exactly one > child node (device_get_child_node_count), get it via > child = device_get_next_child_node(dev, NULL); > and after registering the LED > fwnode_handle_put(child); > This was done in: > https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=e92e7e8aa066904def9a5d584ece7fb6ae512dbd > > > + if (!led) { > > + ret = -ENOMEM; > > + goto err_child_node; > > + } > > + > > + ret = fwnode_property_read_u32(child, "reg", ®); > > + if (ret || reg > 1) { > > + dev_err(dev, "Could not register 'reg' (%lu)\n", (unsigned long)reg); > > + ret = -EINVAL; > > + goto err_child_node; > > + } > > + > > + if (fwnode_property_read_string(child, "label", &label)) { > > + led->name = "iei-wt61p803-puzzle-led::"; > > + } else { > > + led->name = devm_kasprintf(dev, GFP_KERNEL, > > + "iei-wt61p803-puzzle-led:%s", label); > > + if (!led->name) { > > + ret = -ENOMEM; > > + goto err_child_node; > > + } > > + } > > Parsing of label is done by LED core if you use > devm_led_classdev_register_ext. Also, label is obsolete. The LED name > should be composed from color, function and device. > Also please dont pass devicename "iei-wt61p803-puzzle-led" here. We > want to make the LED subsystem derive the device name somehow, and > afterwards we would need to change this. Also the devicename should > refer to the device the LED is triggering to (ie. if the LED is set in > devicetree to trigger on activity on eth0, the devicename should be > eth0 or something, not the name of this driver). > > Just remove this code and let devm_led_classdev_register_ext do its > thing. > > > + > > + fwnode_property_read_string(child, "linux,default-trigger", > > + &led->default_trigger); > > + > > Parsing of linux,default-trigger is done by LED core if you use > devm_led_classdev_register_ext. > > > + led->brightness_set_blocking = iei_wt61p803_puzzle_led_brightness_set_blocking; > > + led->brightness_get = iei_wt61p803_puzzle_led_brightness_get; > > + led->max_brightness = 1; > > + > > + ret = devm_led_classdev_register(dev, led); > > Please use extended LED registration API, with > devm_led_classdev_register_ext. Pass init_data with fwnode member set > to child. > > > + if (ret) { > > + dev_err(dev, "Could not register %s\n", led->name); > > + goto err_child_node; > > + } > > + } > > + return 0; > > +err_child_node: > > + fwnode_handle_put(child); > > + return ret; > > +} > > + > > +static const struct of_device_id iei_wt61p803_puzzle_led_of_match[] = { > > + { .compatible = "iei,wt61p803-puzzle-leds" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, iei_wt61p803_puzzle_led_of_match); > > + > > +static struct platform_driver iei_wt61p803_puzzle_led_driver = { > > + .driver = { > > + .name = "iei-wt61p803-puzzle-led", > > + .of_match_table = iei_wt61p803_puzzle_led_of_match, > > + }, > > + .probe = iei_wt61p803_puzzle_led_probe, > > +}; > > +module_platform_driver(iei_wt61p803_puzzle_led_driver); > > + > > +MODULE_DESCRIPTION("iEi WT61P803 PUZZLE front panel LED driver"); > > +MODULE_AUTHOR("Luka Kovacic <luka.kovacic@sartura.hr>"); > > +MODULE_LICENSE("GPL"); > > +MODULE_ALIAS("platform:leds-iei-wt61p803-puzzle"); > > Marek
I agree, I'll move this over to testing. These values are stored in the microcontroller EEPROM and can be modified. Some test units might be shipped with unpopulated MAC address and S/N values so I exposed this functionality to enable the user to use some internal value there. Kind regards, Luka On Sat, Sep 26, 2020 at 8:25 PM Marek Behun <marek.behun@nic.cz> wrote: > > On Sat, 26 Sep 2020 15:55:12 +0200 > Luka Kovacic <luka.kovacic@sartura.hr> wrote: > > > Add the iei-wt61p803-puzzle driver sysfs interface documentation to allow > > monitoring and control of the microcontroller from user space. > > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > > Cc: Luka Perkov <luka.perkov@sartura.hr> > > Cc: Robert Marko <robert.marko@sartura.hr> > > --- > > .../stable/sysfs-driver-iei-wt61p803-puzzle | 65 +++++++++++++++++++ > > 1 file changed, 65 insertions(+) > > create mode 100644 Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle > > > > diff --git a/Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle b/Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle > > new file mode 100644 > > index 000000000000..36fca70d66ef > > --- /dev/null > > +++ b/Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle > > I think this should go to testing, not stable. It should go to stable > only after it is stable for some time. > > > @@ -0,0 +1,65 @@ > > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/mac_address_* > > +Date: September 2020 > > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > > +Description: Read the internal iEi WT61P803 PUZZLE MCU MAC address values. > > + These are factory assigned and can be changed. > > + > > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/serial_number > > +Date: September 2020 > > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > > +Description: Read the internal iEi WT61P803 PUZZLE MCU serial number. > > + This value is factory assigned and can be changed. > > + > > Please use (RO) and (RW) prefixes before the Description, instead of > writing "This value is read only", i.e.: > Description: (RO) Internal ... serial number. > > JFI: Why can these values be changed? Shouldn't they be burned into OTP? > > Marek > > > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/version > > +Date: September 2020 > > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > > +Description: Read the internal iEi WT61P803 PUZZLE MCU version. > > + This value is read only. > > + > > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/protocol_version > > +Date: September 2020 > > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > > +Description: Read the internal iEi WT61P803 PUZZLE MCU protocol version. > > + This value is read only. > > + > > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_loss_recovery > > +Date: September 2020 > > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > > +Description: Read the iEi WT61P803 PUZZLE MCU power loss recovery value. > > + This value is read write. > > + Value mapping: 0 - Always-On, 1 - Always-Off, 2 - Always-AC, 3 - Always-WA > > + > > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/bootloader_mode > > +Date: September 2020 > > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > > +Description: Read whether the MCU is in bootloader mode. > > + This value is read only. > > + > > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status > > +Date: September 2020 > > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > > +Description: Read the iEi WT61P803 PUZZLE MCU power status. Power status indicates > > + the power on method. > > + This value is read only. > > + Value mapping (bitwise list): > > + 0x80 - Null > > + 0x40 - Firmware flag > > + 0x20 - Power loss detection flag (powered off) > > + 0x10 - Power loss detection flag (AC mode) > > + 0x08 - Button power on > > + 0x04 - WOL power on > > + 0x02 - RTC alarm power on > > + 0x01 - AC recover power on > > + > > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/build_info > > +Date: September 2020 > > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > > +Description: Read the iEi WT61P803 PUZZLE MCU firmware build date. > > + This value is read only. > > + Format: yyyy/mm/dd hh:mm > > + > > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/ac_recovery_status > > +Date: September 2020 > > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > > +Description: Read the iEi WT61P803 PUZZLE MCU AC recovery status. > > + This value is read only. >
This patchset adds support for the iEi Puzzle-M801 1U Rackmount Network Appliance and for the iEi WT61P803 PUZZLE microcontroller, which enables some board specific features like fan and LED control, system power management and temperature sensor reading. The platform is based on the popular Marvell Armada 8040 SoC and supports up to 16 GB of DDR4 2400 MHz ECC RAM. It has a PCIe x16 slot (x2 lanes only) and an M.2 type B slot. External chassis ports: 2x 10 GbE SFP+ 4x 1 GbE (Marvell 88E1512P) 2x USB 3.0 1x RJ45 serial port All notable board components are supported in this patchset. Changes for v2: - Use LAAs for local-mac-address and match reg values - Code styling changes - Error handling moved to the end of the function - Define all magic numbers in the main header file - Convert the driver to make it OF independent - Refactor hwmon to use devm_hwmon_device_register_with_info() - Reduce the number of mutex locks - Allocate memory once for the response buffer - Reduce managed memory allocations Luka Kovacic (7): dt-bindings: Add iEi vendor prefix and iEi WT61P803 PUZZLE driver bindings drivers: mfd: Add a driver for iEi WT61P803 PUZZLE MCU drivers: hwmon: Add the iEi WT61P803 PUZZLE HWMON driver drivers: leds: Add the iEi WT61P803 PUZZLE LED driver Documentation/ABI: Add iei-wt61p803-puzzle driver sysfs interface documentation MAINTAINERS: Add an entry for the iEi WT61P803 PUZZLE driver arm64: dts: marvell: Add a device tree for the iEi Puzzle-M801 board .../stable/sysfs-driver-iei-wt61p803-puzzle | 65 + .../hwmon/iei,wt61p803-puzzle-hwmon.yaml | 41 + .../leds/iei,wt61p803-puzzle-leds.yaml | 48 + .../bindings/mfd/iei,wt61p803-puzzle.yaml | 82 ++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 13 + arch/arm64/boot/dts/marvell/Makefile | 1 + .../dts/marvell/armada-8040-puzzle-m801.dts | 519 ++++++++ drivers/hwmon/Kconfig | 8 + drivers/hwmon/Makefile | 1 + drivers/hwmon/iei-wt61p803-puzzle-hwmon.c | 511 ++++++++ drivers/leds/Kconfig | 8 + drivers/leds/Makefile | 1 + drivers/leds/leds-iei-wt61p803-puzzle.c | 174 +++ drivers/mfd/Kconfig | 8 + drivers/mfd/Makefile | 1 + drivers/mfd/iei-wt61p803-puzzle.c | 1069 +++++++++++++++++ include/linux/mfd/iei-wt61p803-puzzle.h | 69 ++ 18 files changed, 2621 insertions(+) create mode 100644 Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle create mode 100644 Documentation/devicetree/bindings/hwmon/iei,wt61p803-puzzle-hwmon.yaml create mode 100644 Documentation/devicetree/bindings/leds/iei,wt61p803-puzzle-leds.yaml create mode 100644 Documentation/devicetree/bindings/mfd/iei,wt61p803-puzzle.yaml create mode 100644 arch/arm64/boot/dts/marvell/armada-8040-puzzle-m801.dts create mode 100644 drivers/hwmon/iei-wt61p803-puzzle-hwmon.c create mode 100644 drivers/leds/leds-iei-wt61p803-puzzle.c create mode 100644 drivers/mfd/iei-wt61p803-puzzle.c create mode 100644 include/linux/mfd/iei-wt61p803-puzzle.h