From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934571AbcHaNKY (ORCPT ); Wed, 31 Aug 2016 09:10:24 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:49399 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934200AbcHaNKV (ORCPT ); Wed, 31 Aug 2016 09:10:21 -0400 X-AuditID: cbfec7f4-f79cb6d000001359-f2-57c6d73982f0 Subject: Re: [patch] leds: add driver for Mellanox systems LEDs To: vadimp@mellanox.com, rpurdie@rpsys.net References: <1472494066-1681-1-git-send-email-vadimp@mellanox.com> Cc: linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org, jiri@resnulli.us, michaelsh@mellanox.com From: Jacek Anaszewski Message-id: <9215a8a3-8dbe-518a-8cfb-8f0e8def5158@samsung.com> Date: Wed, 31 Aug 2016 15:10:16 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-version: 1.0 In-reply-to: <1472494066-1681-1-git-send-email-vadimp@mellanox.com> Content-type: text/plain; charset=windows-1252; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrALMWRmVeSWpSXmKPExsVy+t/xq7qW14+FGyz9YGJx8Wq6xeVdc9gs tr5Zx2jxZ/9DJovdu56yWpxfv5vFgc3j2fTDTB5Xb1Z77Jn/g9Xj8ya5AJYoLpuU1JzMstQi fbsEroz5C7ewF0zqZqxo3/mIqYGxM7+LkZNDQsBEYs+CP+wQtpjEhXvr2boYuTiEBJYySjTd v80E4TxjlNg49zczSJWwgL3E+jNtQAkODhEBLYm2hVIgYSEBJ4l9OyexgdjMAhkSD39tZwWx 2QQMJX6+eM0EYvMK2En8+PoWbAyLgKrE6ZMbwWpEBSIkbq36yAhRIyjxY/I9FhCbU8BZorN7 NxPETFuJBe/XsUDY8hKb17xlnsAoMAtJyywkZbOQlC1gZF7FKJpamlxQnJSea6hXnJhbXJqX rpecn7uJERLOX3YwLj5mdYhRgINRiYc3Y8bRcCHWxLLiytxDjBIczEoivN7XjoUL8aYkVlal FuXHF5XmpBYfYpTmYFES5527632IkEB6YklqdmpqQWoRTJaJg1OqgdEp+H5C+ezt+9c1BW51 XHBgZ9Jbey3dBRnlJ9rfXp+4hTWm+p+tVn2sxuWA9I0XXly67VDuWmZsfnFHupDs3Ps1Mz9V Xn/ztDo9TmDFu8OZ+zzFeKsOrjX7/2zuiWmCnEK+E6OXfZN34FPX9tNPX8b4S8JNPnV2e+va mv0yixosE6e2TTL+/FOJpTgj0VCLuag4EQByJ5SpYwIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Vadim, Thanks for the patch. See my comments below. On 08/29/2016 08:07 PM, vadimp@mellanox.com wrote: > From: Vadim Pasternak > > This makes it possible to create a set of LEDs for Mellanox systems: > "msx6710", "msx6720", "msb7700", "msn2700", "msx1410", "msn2410", > "msb7800", "msn2740", "msn2100". > > Drivers creates led devices according to system configuration, provided s/Drivers/Driver/ and s/led/LED/ here and in case of all other occurrences in the in-driver comments. > through system DMA data, like > fan1:green fan1:red status:green status:red Documentation/leds/leds-class.txt states the LED class device name should match the following pattern: devicename:colour:function > > LED setting is controlled through on board CPLD Lattice device. > For setting particular led off, solid, > blink (blink required ledtrig-timer module): s/required/requires/ > echo 0 > /sys/class/leds/status\:green/brightness > echo 1 > /sys/class/leds/status\:green/brightness > echo timer > /sys/class/leds/status\:green/trigger > > On module probing all leds are set green, on removing - off. > > Last setting overwrites previous, f.e. sequence for > changing led from green - red - green: > echo 1 > /sys/class/leds/psu\:green/brightness > echo 1 > /sys/class/leds/psu\:red/brightness > echo 1 > /sys/class/leds/psu\:green/brightness It seems that there are many LEDs connected to the same iout and they cannot be turned on simultaneously. Am I right? > The Kconfig currently controlling compilation of this code is: > drivers/leds/Kconfig:config LEDS_MLXCPLD > > Signed-off-by: Vadim Pasternak > Reviewed-by: Jiri Pirko > --- > MAINTAINERS | 7 + > drivers/leds/Kconfig | 8 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-mlxcpld.c | 385 ++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 401 insertions(+) > create mode 100644 drivers/leds/leds-mlxcpld.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 0bbe4b1..31cffdd 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7655,6 +7655,13 @@ W: http://www.mellanox.com > Q: http://patchwork.ozlabs.org/project/netdev/list/ > F: drivers/net/ethernet/mellanox/mlxsw/ > > +MELLANOX MLXCPLD LED DRIVER > +M: Vadim Pasternak > +L: linux-kernel@vger.kernel.org s/linux-kernel/linux-leds/ > +S: Supported > +W: http://www.mellanox.com Could you please provide a direct link to the datasheet? > +F: drivers/leds/leds-mlxcpld.c > + > SOFT-ROCE DRIVER (rxe) > M: Moni Shoua > L: linux-rdma@vger.kernel.org > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 9dcc9b1..4cd85be 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -631,6 +631,14 @@ config LEDS_VERSATILE > This option enabled support for the LEDs on the ARM Versatile > and RealView boards. Say Y to enabled these. > > +config LEDS_MLXCPLD > + tristate "LED support for the Mellanox boards" > + depends on X86_64 && DMI > + depends on LEDS_CLASS > + help > + This option enabled support for the LEDs on the Mellanox > + boards. Say Y to enabled these. > + > comment "LED Triggers" > source "drivers/leds/trigger/Kconfig" > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 0684c86..9a2494b 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -68,6 +68,7 @@ obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o > obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o > obj-$(CONFIG_LEDS_SEAD3) += leds-sead3.o > obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o > +obj-$(CONFIG_LEDS_MLXCPLD) += leds-mlxcpld.o > > # LED SPI Drivers > obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o > diff --git a/drivers/leds/leds-mlxcpld.c b/drivers/leds/leds-mlxcpld.c > new file mode 100644 > index 0000000..b79342c > --- /dev/null > +++ b/drivers/leds/leds-mlxcpld.c > @@ -0,0 +1,385 @@ > +/* > + * drivers/leds/leds-mlxcpld.c > + * Copyright (c) 2016 Mellanox Technologies. All rights reserved. > + * Copyright (c) 2016 Vadim Pasternak > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions are met: > + * > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * 3. Neither the names of the copyright holders nor the names of its > + * contributors may be used to endorse or promote products derived from > + * this software without specific prior written permission. > + * > + * Alternatively, this software may be distributed under the terms of the > + * GNU General Public License ("GPL") version 2 as published by the Free > + * Software Foundation. > + * > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE > + * POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Please arrange include directives in alphabetical order. > +#define MLXPLAT_CPLD_LPC_REG_BASE_ADRR 0x2500 /* LPC bus access */ > + > +/* Color codes for leds */ > +#define LED_IS_OFF 0x00 > +#define LED_RED_STATIC_ON 0x05 > +#define LED_RED_BLINK_HALF 0x06 > +#define LED_GREEN_STATIC_ON 0x0D > +#define LED_GREEN_BLINK_HALF 0x0E Why *BLINK* macros aren't used anywhere? > + > +/** > + * mlxcpld_param - > + * @offset - offset for led access in CPLD device > + * @mask - mask for led access in CPLD device > + * @base_color - base color code for led > +**/ > +struct mlxcpld_param { > + u8 offset; > + u8 mask; > + u8 base_color; > +}; > + > +/** > + * mlxcpld_led_priv - > + * @led - led class device pointer It is not a pointer but rather an instance. > + * @param - led CPLD access parameters > +**/ > +struct mlxcpld_led_priv { > + struct led_classdev cdev; > + struct mlxcpld_param param; > +}; Add empty line here. > +#define cdev_to_priv(c) container_of(c, struct mlxcpld_led_priv, cdev) > + > +/** > + * mlxcpld_led_profile (defined per system class) - > + * @offset - offset for led access in CPLD device > + * @mask - mask for led access in CPLD device > + * @base_color - base color code > + * @brightness - default brightness setting (on/off) > + * @name - led name > +**/ > +struct mlxcpld_led_profile { > + u8 offset; > + u8 mask; > + u8 base_color; > + enum led_brightness brightness; > + const char *name; > +}; > + > +/** > + * mlxcpld_led_pdata - > + * @pdev - platform device pointer > + * @led - led class device pointer > + * @trigger - trigger class device pointer Related property in the structure definition is missing. > + * @profile - system configuration profile > + * @num_led_instances - number of system triggers > + * @lock - device access lock > +**/ > +struct mlxcpld_led_pdata { > + struct platform_device *pdev; > + struct mlxcpld_led_priv *pled; > + struct mlxcpld_led_profile *profile; > + int num_led_instances; > + spinlock_t lock; > +}; Add empty line here. > +static struct mlxcpld_led_pdata *mlxcpld_led; > + > +/* Default profile fit the next Mellanox systems: > + * "msx6710", "msx6720", "msb7700", "msn2700", "msx1410", > + * "msn2410", "msb7800", "msn2740" > + */ > +struct mlxcpld_led_profile mlxcpld_led_default_profile[] = { > + { > + 0x21, 0xf0, LED_GREEN_STATIC_ON, LED_FULL, "fan1:green", > + }, You're defining max_brightness to 1 below. s/LED_FULL/1/ > + { > + 0x21, 0xf0, LED_RED_STATIC_ON, LED_OFF, "fan1:red", > + }, > + { > + 0x21, 0x0f, LED_GREEN_STATIC_ON, LED_FULL, "fan2:green", > + }, > + { > + 0x21, 0x0f, LED_RED_STATIC_ON, LED_OFF, "fan2:red", > + }, > + { > + 0x22, 0xf0, LED_GREEN_STATIC_ON, LED_FULL, "fan3:green", > + }, > + { > + 0x22, 0xf0, LED_RED_STATIC_ON, LED_OFF, "fan3:red", > + }, > + { > + 0x22, 0x0f, LED_GREEN_STATIC_ON, LED_FULL, "fan4:green", > + }, > + { > + 0x22, 0x0f, LED_RED_STATIC_ON, LED_OFF, "fan4:red", > + }, > + { > + 0x20, 0x0f, LED_GREEN_STATIC_ON, LED_FULL, "psu:green", > + }, > + { > + 0x20, 0x0f, LED_RED_STATIC_ON, LED_OFF, "psu:red", > + }, > + { > + 0x20, 0xf0, LED_GREEN_STATIC_ON, LED_FULL, "status:green", > + }, > + { > + 0x20, 0xf0, LED_RED_STATIC_ON, LED_OFF, "status:red", > + }, > +}; > + > +/* Profile fit the Mellanox systems based on "msn2100" */ > +struct mlxcpld_led_profile mlxcpld_led_msn2100_profile[] = { > + { > + 0x21, 0xf0, LED_GREEN_STATIC_ON, LED_FULL, "fan:green", > + }, > + { > + 0x21, 0xf0, LED_RED_STATIC_ON, LED_OFF, "fan:red", > + }, > + { > + 0x23, 0xf0, LED_GREEN_STATIC_ON, LED_FULL, "psu1:green", > + }, > + { > + 0x23, 0xf0, LED_RED_STATIC_ON, LED_OFF, "psu1:red", > + }, > + { > + 0x23, 0x0f, LED_GREEN_STATIC_ON, LED_FULL, "psu2:green", > + }, > + { > + 0x23, 0x0f, LED_RED_STATIC_ON, LED_OFF, "psu2:red", > + }, > + { > + 0x20, 0xf0, LED_GREEN_STATIC_ON, LED_FULL, "status:green", > + }, > + { > + 0x20, 0xf0, LED_RED_STATIC_ON, LED_OFF, "status:red", > + }, > + { > + 0x24, 0xf0, LED_GREEN_STATIC_ON, LED_OFF, "uid:blue", > + }, > +}; > + > +enum mlxcpld_led_platform_types { > + mlxcpld_led_platform_default, > + mlxcpld_led_platform_msn2100, Please use capital letters for the enums. > +}; > + > +const char *mlx_product_names[] = { > + "DEFAULT", > + "MSN2100", > +}; > + > +static enum > +mlxcpld_led_platform_types mlxcpld_led_platform_check_sys_type(void) > +{ > + const char *mlx_product_name; > + int i; > + > + mlx_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); > + if (mlx_product_name == NULL) if (!mlx_product_name) > + return mlxcpld_led_platform_default; > + > + for (i = 1; i < ARRAY_SIZE(mlx_product_names); i++) { > + if (strstr(mlx_product_name, mlx_product_names[i])) > + return i; > + } > + > + return mlxcpld_led_platform_default; > +} > + > +static void mlxcpld_led_bus_access_func(u16 base, u8 offset, u8 rw_flag, > + u8 *data) > +{ > + u32 addr = base + offset; > + > + if (rw_flag == 0) > + outb(*data, addr); > + else > + *data = inb(addr); > +} > + > +static void mlxcpld_led_store_hw(u8 mask, u8 off, u8 vset) > +{ > + u8 tmask, val; > + > + spin_lock(&mlxcpld_led->lock); > + tmask = (mask == 0xf0) ? vset : (vset << 4); Could you shed some light on what happens here? What is tmask? > + mlxcpld_led_bus_access_func(MLXPLAT_CPLD_LPC_REG_BASE_ADRR, off, 1, > + &val); > + val = (val & mask) | tmask; > + mlxcpld_led_bus_access_func(MLXPLAT_CPLD_LPC_REG_BASE_ADRR, off, 0, > + &val); > + spin_unlock(&mlxcpld_led->lock); > +} > + > +static void mlxcpld_led_brightness(struct led_classdev *led, > + enum led_brightness value) > +{ > + struct mlxcpld_led_priv *pled = cdev_to_priv(led); > + > + switch (value) { > + case LED_FULL: > + case LED_HALF: The value passed to this callback will never be greater than max_brightness. Since you're setting it to 1 below, than you can get rid of this switch statement in favour of the following: if (!value) { mlxcpld_led_store_hw(pled->param.mask, pled->param.offset, LED_OFF); return; } mlxcpld_led_store_hw(pled->param.mask, pled->param.offset, pled->param.base_color); > + default: > + mlxcpld_led_store_hw(pled->param.mask, pled->param.offset, > + pled->param.base_color); > + break; > + case LED_OFF: > + mlxcpld_led_store_hw(pled->param.mask, pled->param.offset, > + LED_IS_OFF); > + break; > + } > +} > + > +static int mlxcpld_led_blink(struct led_classdev *led, > + unsigned long *delay_on, > + unsigned long *delay_off) > +{ > + struct mlxcpld_led_priv *pled = cdev_to_priv(led); > + > + /* SW blinking is not supported. You're claiming the opposite in the commit message. Nonetheless that is not true, since you don't have any control over how led_set_brightness() will be called. > + * HW supports two types of blinking: full (6KHz) and half (3KHz). > + * Defaul value is 3KHz, which is set for blink request. > + */ > + mlxcpld_led_store_hw(pled->param.mask, pled->param.offset, > + pled->param.base_color + 1); Does base_color + 1 enable hardware blinking at 3kHz? How LED_RED_BLINK_HALF and LED_GREEN_BLINK_HALF values are related to that? Why aren't you supporting 6kHz setting? Besides, please refer to the other LED class drivers that implement blink_set op. They set HW blinking only if passed delay_on and delay_off settings are supported by the hardware, and return -EINVAL otherwise, which enables software blinking fallback at LED core. > + > + return 0; > +} > + > +static int mlxcpld_led_config(struct device *dev, > + struct mlxcpld_led_pdata *cpld) > +{ > + int err = 0, i; > + > + cpld->pled = devm_kzalloc(dev, sizeof(struct mlxcpld_led_priv) * > + cpld->num_led_instances, GFP_KERNEL); > + if (!cpld->pled) > + return -ENOMEM; > + > + for (i = 0; i < cpld->num_led_instances; i++) { > + cpld->pled[i].cdev.name = cpld->profile[i].name; > + cpld->pled[i].cdev.brightness = cpld->profile[i].brightness; > + cpld->pled[i].cdev.max_brightness = 1; > + cpld->pled[i].cdev.brightness_set = mlxcpld_led_brightness; > + cpld->pled[i].cdev.blink_set = mlxcpld_led_blink; > + cpld->pled[i].cdev.flags = LED_CORE_SUSPENDRESUME; > + err = devm_led_classdev_register(dev, &cpld->pled[i].cdev); > + if (err) { > + devm_kfree(dev, cpld->pled); You don't need to call this explicitly. > + return err; > + } > + > + cpld->pled[i].param.offset = mlxcpld_led->profile[i].offset; > + cpld->pled[i].param.mask = mlxcpld_led->profile[i].mask; > + cpld->pled[i].param.base_color = > + mlxcpld_led->profile[i].base_color; > + switch (mlxcpld_led->profile[i].brightness) { > + case LED_HALF: > + case LED_FULL: > + mlxcpld_led_brightness(&cpld->pled[i].cdev, > + mlxcpld_led->profile[i].brightness); > + break; > + default: > + break; > + } Switch statement is not needed here. > + } > + > + return err; > +} > + > +static int __init mlxcpld_led_probe(struct platform_device *pdev) > +{ > + enum mlxcpld_led_platform_types mlxcpld_led_plat = > + mlxcpld_led_platform_check_sys_type(); > + > + mlxcpld_led = devm_kzalloc(&pdev->dev, sizeof(*mlxcpld_led), > + GFP_KERNEL); > + if (!mlxcpld_led) > + return -ENOMEM; Please add empty line here. > + mlxcpld_led->pdev = pdev; > + > + switch (mlxcpld_led_plat) { > + case mlxcpld_led_platform_msn2100: > + mlxcpld_led->profile = mlxcpld_led_msn2100_profile; > + mlxcpld_led->num_led_instances = > + ARRAY_SIZE(mlxcpld_led_msn2100_profile); > + break; > + > + default: > + mlxcpld_led->profile = mlxcpld_led_default_profile; > + mlxcpld_led->num_led_instances = > + ARRAY_SIZE(mlxcpld_led_default_profile); > + break; > + } > + > + spin_lock_init(&mlxcpld_led->lock); > + platform_set_drvdata(pdev, mlxcpld_led); There is no corresponding platform_get_drvdata() call in this driver, so it seems to be redundant. > + > + return mlxcpld_led_config(&pdev->dev, mlxcpld_led); > +} > + > +static struct platform_driver mlxcpld_led_driver = { > + .driver = { > + .name = KBUILD_MODNAME, > + }, > +}; > + > +static int __init mlxcpld_led_init(void) > +{ > + struct platform_device *pdev; > + int err; > + > + pdev = platform_device_register_simple(KBUILD_MODNAME, -1, NULL, 0); > + if (!pdev) { > + pr_err("Device allocation failed\n"); > + return -ENOMEM; > + } > + > + err = platform_driver_probe(&mlxcpld_led_driver, mlxcpld_led_probe); > + if (err) { > + pr_err("Probe platform driver failed\n"); > + platform_device_unregister(pdev); > + } > + > + return err; > +} > + > +static void __exit mlxcpld_led_exit(void) > +{ > + platform_device_unregister(mlxcpld_led->pdev); > + platform_driver_unregister(&mlxcpld_led_driver); > +} > + > +module_init(mlxcpld_led_init); > +module_exit(mlxcpld_led_exit); > + > +MODULE_AUTHOR("Vadim Pasternak (vadimp@mellanox.com)"); > +MODULE_DESCRIPTION("Mellanox board LED driver"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:leds-mlxcpld"); > -- Best regards, Jacek Anaszewski