From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vadim Pasternak Subject: RE: [patch v6 1/1] leds: add driver for support Mellanox regmap LEDs for BMC and x86 platform Date: Tue, 13 Feb 2018 21:43:28 +0000 Message-ID: References: <1518466530-132967-1-git-send-email-vadimp@mellanox.com> <1518466530-132967-2-git-send-email-vadimp@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-eopbgr00041.outbound.protection.outlook.com ([40.107.0.41]:42403 "EHLO EUR02-AM5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S965844AbeBMVnd (ORCPT ); Tue, 13 Feb 2018 16:43:33 -0500 In-Reply-To: Content-Language: en-US Sender: linux-leds-owner@vger.kernel.org List-Id: linux-leds@vger.kernel.org To: Jacek Anaszewski , "rpurdie@rpsys.net" , "pavel@ucw.cz" Cc: "linux-leds@vger.kernel.org" , "jiri@resnulli.us" > -----Original Message----- > From: Jacek Anaszewski [mailto:jacek.anaszewski@gmail.com] > Sent: Tuesday, February 13, 2018 11:16 PM > To: Vadim Pasternak ; rpurdie@rpsys.net; > pavel@ucw.cz > Cc: linux-leds@vger.kernel.org; jiri@resnulli.us > Subject: Re: [patch v6 1/1] leds: add driver for support Mellanox regmap = LEDs > for BMC and x86 platform >=20 > Hi Vadim, >=20 > Thanks for the update. Hi Jacek, Thank you very much for reply. >=20 > On 02/12/2018 09:15 PM, Vadim Pasternak wrote: > > Driver obtains LED devices according to system configuration and > > creates devices in form: "devicename:color:function", like The full > > path is to be: > > /sys/class/leds/mlxreg\:status\:amber/brightness > > After timer trigger activation: > > echo timer > /sys/class/leds/mlxreg\:status\:amber/trigger > > Attributes for LED blinking will appaer in sysfs infrastructure: > > /sys/class/leds/mlxreg\:status\:amber/delay_off > > /sys/class/leds/mlxreg\:status\:amber/delay_on > > > > LED setting is controlled through the on-board programmable devices, > > which exports its register map. This device could be attached to any > > bus type, for which register mapping is supported. > > > > Signed-off-by: Vadim Pasternak > > Acked-by: Pavel Machek > > --- > > v5->v6 > > Changes added by Vadim: > > - Change year to 2018 in Copyright; > > - Change structure mlxreg_core_led_platform_data name to common name > > mlxreg_core_platform_data, as it has been pushed to > > git://git.infradead.org/linux-platform-drivers-x86.git to branch, > > commit 1f976f6978bf6156ce822eb279ac86c519b10329 (1f976f6), file > > include/linux/platform_data/mlxreg.h > > - Add orange color, as alternative to red (some systems use orange, > > instead of red); > > - Drop mlxreg_led_dt_match, since the driver is to be activated by an= other > > common Mellanox platform driver, mlx-platform or mlx-platform-of; > > v4->v5 > > Comments pointed out by Jacek: > > - Make consistent naming in mlxreg_led_brightness_set, > > mlxreg_led_brightness_get, mlxreg_led_config; > > - Remove unnecessary data assignment in mlxreg_led_config; Changes > > added by Vadim: > > - Add internal structure mlxreg_led_data to leds-mlxreg.c; > > v3->v4 > > Comments pointed out by Jacek: > > - add led in declaration mlxreg_led_store_hw and mlxreg_led_get_hw; > > - rearrange local variables names in mlxreg_led_config; Comments > > pointed out by Pavel: > > - replace _HALF/_FULL with BLINK_3HZ/BLINK_6HZ; > > - in mlxreg_led_get_hw just return after access error; > > - explicit check for green led is not added, since control device can = be > > programmed for blue LED and this case it reuses masks of green LED. > > v2->v3: > > Comments pointed out by Jacek: > > - fix order in includes; > > - add declaration pdata in mlxreg_led_store_hw and mlxreg_led_get_hw; > > - add declaration of led_data in mlxreg_led_config; > > - use LED_ON in assignment instead of 1; > > - fix mutex call in mlxreg_led_remove; > > v1->v2: > > Comments pointed out by Jacek: > > - remove macros MLXREG_MAP and MLXREG_CDEV; > > - brightness_set_blocking insteaf of brightness_set, this fix allows > > removing of context validation and workqueue and also three fields f= rom > > struct mlxreg_core_led_data in mlxreg.h; > > - remove MLXREG_LED_NAME; > > - extend Kconfig description; > > Changes added by Vadim: > > - remove unnecessary includes after dropping workqueue; > > --- > > MAINTAINERS | 1 + > > drivers/leds/Kconfig | 10 ++ > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-mlxreg.c | 311 > > +++++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 323 insertions(+) > > create mode 100644 drivers/leds/leds-mlxreg.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS index 95c3fa1..96625b2 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -8916,6 +8916,7 @@ M: Vadim Pasternak > > L: linux-leds@vger.kernel.org > > S: Supported > > F: drivers/leds/leds-mlxcpld.c > > +F: drivers/leds/leds-mlxreg.c > > F: Documentation/leds/leds-mlxcpld.txt > > > > MELLANOX PLATFORM DRIVER > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index > > 3e763d2..c98abe1 100644 > > --- a/drivers/leds/Kconfig > > +++ b/drivers/leds/Kconfig > > @@ -694,6 +694,16 @@ config LEDS_MLXCPLD > > This option enabled support for the LEDs on the Mellanox > > boards. Say Y to enabled these. > > > > +config LEDS_MLXREG > > + tristate "LED support for the Mellanox switches management control" > > + depends on LEDS_CLASS > > + help > > + This option enabled support for the LEDs on the Mellanox Ethernet > > +and >=20 > s/enabled/enables/ Ack. Should I sent a fix for LEDS_MLXCPLD in a separate patch? >=20 > Just noticed that we have the same typo in two other places in this Kconf= ig. >=20 > > + InfiniBand switches. The driver can be activated from the device > > +tree >=20 > You don't seem to use OF API in the driver. >=20 > > + or by the direct platform device add call. >=20 > Could you please explain how it is supposed to work? Who is responsible f= or > regmap initialization? Do you plan on adding corresponding platform_data = to > drivers/platform/x86/mlx-platform.c or so? Yes. I am going to submit a patch with LED introduction to mlx-platform right after leds-mlxreg is accepted for the next. And also I am going to have some kind of mlx-of-platform driver in=20 drivers/platform/mellanox for ARM which will use OF API itself. But you are right, the sentence in Kconfig "The driver can be activated fro= m the device Tree" is not correct, because even with mlx-of-platform, mlxreg-led driver = will not use OF API by itself. I'll drop it. >=20 > > Say Y to enabled these. To > > + compile this driver as a module, choose 'M' here: the module will b= e > > + called leds-mlxreg. > > + > > config LEDS_USER > > tristate "Userspace LED support" > > depends on LEDS_CLASS > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index > > 987884a..91eca81 100644 > > --- a/drivers/leds/Makefile > > +++ b/drivers/leds/Makefile > > @@ -72,6 +72,7 @@ obj-$(CONFIG_LEDS_IS31FL319X) +=3D leds- > is31fl319x.o > > obj-$(CONFIG_LEDS_IS31FL32XX) +=3D leds-is31fl32xx.o > > obj-$(CONFIG_LEDS_PM8058) +=3D leds-pm8058.o > > obj-$(CONFIG_LEDS_MLXCPLD) +=3D leds-mlxcpld.o > > +obj-$(CONFIG_LEDS_MLXREG) +=3D leds-mlxreg.o > > obj-$(CONFIG_LEDS_NIC78BX) +=3D leds-nic78bx.o > > obj-$(CONFIG_LEDS_MT6323) +=3D leds-mt6323.o > > obj-$(CONFIG_LEDS_LM3692X) +=3D leds-lm3692x.o > > diff --git a/drivers/leds/leds-mlxreg.c b/drivers/leds/leds-mlxreg.c > > new file mode 100644 index 0000000..4dd2bd8 > > --- /dev/null > > +++ b/drivers/leds/leds-mlxreg.c > > @@ -0,0 +1,311 @@ > > +/* > > + * Copyright (c) 2018 Mellanox Technologies. All rights reserved. > > + * Copyright (c) 2018 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 copyrigh= t > > + * notice, this list of conditions and the following disclaimer in = the > > + * documentation and/or other materials provided with the distribut= ion. > > + * 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 > > + > > +/* Codes for LEDs. */ > > +#define MLXREG_LED_OFFSET_BLINK_3HZ 0x01 /* Offset from solid: 3Hz > blink */ > > +#define MLXREG_LED_OFFSET_BLINK_6HZ 0x02 /* Offset from solid: 6Hz > blink */ > > +#define MLXREG_LED_IS_OFF 0x00 /* Off */ > > +#define MLXREG_LED_RED_SOLID 0x05 /* Solid red */ > > +#define MLXREG_LED_GREEN_SOLID 0x0D /* Solid green */ > > +#define MLXREG_LED_AMBER_SOLID 0x09 /* Solid amber */ > > +#define MLXREG_LED_BLINK_3HZ 167 /* ~167 msec off/on - HW > support */ > > +#define MLXREG_LED_BLINK_6HZ 83 /* ~83 msec off/on - HW > support */ > > + > > +/** > > + * struct mlxreg_led_data - led control data: > > + * > > + * @data: led configuration data; > > + * @led_classdev: led class data; > > + * @base_color: base led color (other colors have constant offset > > +from base); > > + * @led_data: led data; > > + * @data_parent: pointer to private device control data of parent; > > +*/ struct mlxreg_led_data { > > + struct mlxreg_core_data *data; > > + struct led_classdev led_cdev; > > + u8 base_color; > > + void *data_parent; > > + char led_cdev_name[MLXREG_CORE_LABEL_MAX_SIZE]; > > +}; > > + > > +#define cdev_to_priv(c) container_of(c, struct mlxreg_led_data, > > +led_cdev) > > + > > +/** > > + * struct mlxreg_led_priv_data - platform private data: > > + * > > + * @pdev: platform device; > > + * @pdata: platform data; > > + * @access_lock: mutex for attribute IO access; */ struct > > +mlxreg_led_priv_data { > > + struct platform_device *pdev; > > + struct mlxreg_core_platform_data *pdata; > > + struct mutex access_lock; /* protect IO operations */ }; > > + > > +static int > > +mlxreg_led_store_hw(struct mlxreg_led_data *led_data, u8 vset) { > > + struct mlxreg_led_priv_data *priv =3D led_data->data_parent; > > + struct mlxreg_core_platform_data *led_pdata =3D priv->pdata; > > + struct mlxreg_core_data *data =3D led_data->data; > > + u32 regval; > > + u32 nib; > > + int ret; > > + > > + /* > > + * Each LED is controlled through low or high nibble of the relevant > > + * register byte. Register offset is specified by off parameter. > > + * Parameter vset provides color code: 0x0 for off, 0x5 for solid red= , > > + * 0x6 for 3Hz blink red, 0xd for solid green, 0xe for 3Hz blink > > + * green. > > + * Parameter mask specifies which nibble is used for specific LED: ma= sk > > + * 0xf0 - lower nibble is to be used (bits from 0 to 3), mask 0x0f - > > + * higher nibble (bits from 4 to 7). > > + */ > > + mutex_lock(&priv->access_lock); > > + > > + ret =3D regmap_read(led_pdata->regmap, data->reg, ®val); > > + if (ret) > > + goto access_error; > > + > > + nib =3D (ror32(data->mask, data->bit) =3D=3D 0xf0) ? rol32(vset, data= ->bit) : > > + rol32(vset, data->bit + 4); > > + regval =3D (regval & data->mask) | nib; > > + > > + ret =3D regmap_write(led_pdata->regmap, data->reg, regval); > > + > > +access_error: > > + mutex_unlock(&priv->access_lock); > > + > > + return ret; > > +} > > + > > +static enum led_brightness > > +mlxreg_led_get_hw(struct mlxreg_led_data *led_data) { > > + struct mlxreg_led_priv_data *priv =3D led_data->data_parent; > > + struct mlxreg_core_platform_data *led_pdata =3D priv->pdata; > > + struct mlxreg_core_data *data =3D led_data->data; > > + u32 regval; > > + int ret; > > + > > + /* > > + * Each LED is controlled through low or high nibble of the relevant > > + * register byte. Register offset is specified by off parameter. > > + * Parameter vset provides color code: 0x0 for off, 0x5 for solid red= , > > + * 0x6 for 3Hz blink red, 0xd for solid green, 0xe for 3Hz blink > > + * green. > > + * Parameter mask specifies which nibble is used for specific LED: ma= sk > > + * 0xf0 - lower nibble is to be used (bits from 0 to 3), mask 0x0f - > > + * higher nibble (bits from 4 to 7). > > + */ > > + ret =3D regmap_read(led_pdata->regmap, data->reg, ®val); > > + if (ret < 0) { > > + dev_warn(led_data->led_cdev.dev, "Failed to get current > brightness, error: %d\n", > > + ret); > > + /* Assume the LED is OFF */ > > + return LED_OFF; > > + } > > + > > + regval =3D regval & ~data->mask; > > + regval =3D (ror32(data->mask, data->bit) =3D=3D 0xf0) ? ror32(regval, > > + data->bit) : ror32(regval, data->bit + 4); > > + if (regval >=3D led_data->base_color && > > + regval <=3D (led_data->base_color + > MLXREG_LED_OFFSET_BLINK_6HZ)) > > + ret =3D LED_FULL; > > + else > > + ret =3D LED_OFF; > > + > > + return ret; > > +} > > + > > +static int > > +mlxreg_led_brightness_set(struct led_classdev *cled, enum > > +led_brightness value) { > > + struct mlxreg_led_data *led_data =3D cdev_to_priv(cled); > > + > > + if (value) > > + return mlxreg_led_store_hw(led_data, led_data->base_color); > > + else > > + return mlxreg_led_store_hw(led_data, MLXREG_LED_IS_OFF); } > > + > > +static enum led_brightness > > +mlxreg_led_brightness_get(struct led_classdev *cled) { > > + struct mlxreg_led_data *led_data =3D cdev_to_priv(cled); > > + > > + return mlxreg_led_get_hw(led_data); > > +} > > + > > +static int > > +mlxreg_led_blink_set(struct led_classdev *cled, unsigned long *delay_o= n, > > + unsigned long *delay_off) > > +{ > > + struct mlxreg_led_data *led_data =3D cdev_to_priv(cled); > > + int err; > > + > > + /* > > + * HW supports two types of blinking: full (6Hz) and half (3Hz). > > + * For delay on/off zero LED is setting to solid color. For others > > + * combination blinking is to be controlled by the software timer. > > + */ > > + if (!(*delay_on =3D=3D 0 && *delay_off =3D=3D 0) && > > + !(*delay_on =3D=3D MLXREG_LED_BLINK_3HZ && > > + *delay_off =3D=3D MLXREG_LED_BLINK_3HZ) && > > + !(*delay_on =3D=3D MLXREG_LED_BLINK_6HZ && > > + *delay_off =3D=3D MLXREG_LED_BLINK_6HZ)) > > + return -EINVAL; > > + > > + if (*delay_on =3D=3D MLXREG_LED_BLINK_6HZ) > > + err =3D mlxreg_led_store_hw(led_data, led_data->base_color + > > + MLXREG_LED_OFFSET_BLINK_6HZ); > > + else if (*delay_on =3D=3D MLXREG_LED_BLINK_3HZ) > > + err =3D mlxreg_led_store_hw(led_data, led_data->base_color + > > + MLXREG_LED_OFFSET_BLINK_3HZ); > > + else > > + err =3D mlxreg_led_store_hw(led_data, led_data->base_color); > > + > > + return err; > > +} > > + > > +static int mlxreg_led_config(struct mlxreg_led_priv_data *priv) { > > + struct mlxreg_core_platform_data *led_pdata =3D priv->pdata; > > + struct mlxreg_core_data *data =3D led_pdata->data; > > + struct mlxreg_led_data *led_data; > > + struct led_classdev *led_cdev; > > + int brightness; > > + int i; > > + int err; > > + > > + for (i =3D 0; i < led_pdata->counter; i++, data++) { > > + led_data =3D devm_kzalloc(&priv->pdev->dev, sizeof(*led_data), > > + GFP_KERNEL); > > + if (!led_data) > > + return -ENOMEM; > > + > > + led_cdev =3D &led_data->led_cdev; > > + led_data->data_parent =3D priv; > > + if (strstr(data->label, "red") || > > + strstr(data->label, "orange")) { > > + brightness =3D LED_OFF; > > + led_data->base_color =3D MLXREG_LED_RED_SOLID; > > + } else if (strstr(data->label, "amber")) { > > + brightness =3D LED_OFF; > > + led_data->base_color =3D MLXREG_LED_AMBER_SOLID; > > + } else { > > + brightness =3D LED_OFF; > > + led_data->base_color =3D MLXREG_LED_GREEN_SOLID; > > + } > > + sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg", > > + data->label); > > + led_cdev->name =3D led_data->led_cdev_name; > > + led_cdev->brightness =3D brightness; > > + led_cdev->max_brightness =3D LED_ON; > > + led_cdev->brightness_set_blocking =3D > > + mlxreg_led_brightness_set; > > + led_cdev->brightness_get =3D mlxreg_led_brightness_get; > > + led_cdev->blink_set =3D mlxreg_led_blink_set; > > + led_cdev->flags =3D LED_CORE_SUSPENDRESUME; > > + led_data->data =3D data; > > + err =3D devm_led_classdev_register(&priv->pdev->dev, led_cdev); > > + if (err) > > + return err; > > + > > + if (led_cdev->brightness) > > + mlxreg_led_brightness_set(led_cdev, > > + led_cdev->brightness); > > + dev_info(led_cdev->dev, "label: %s, mask: 0x%02x, > offset:0x%02x\n", > > + data->label, data->mask, data->reg); > > + } > > + > > + return 0; > > +} > > + > > +static int mlxreg_led_probe(struct platform_device *pdev) { > > + struct mlxreg_core_platform_data *led_pdata; > > + struct mlxreg_led_priv_data *priv; > > + > > + led_pdata =3D dev_get_platdata(&pdev->dev); > > + if (!led_pdata) { > > + dev_err(&pdev->dev, "Failed to get platform data.\n"); > > + return -EINVAL; > > + } > > + > > + priv =3D devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + mutex_init(&priv->access_lock); > > + priv->pdev =3D pdev; > > + priv->pdata =3D led_pdata; > > + > > + return mlxreg_led_config(priv); > > +} > > + > > +static int mlxreg_led_remove(struct platform_device *pdev) { > > + struct mlxreg_led_priv_data *priv =3D dev_get_drvdata(&pdev->dev); > > + > > + mutex_destroy(&priv->access_lock); > > + > > + return 0; > > +} > > + > > +static struct platform_driver mlxreg_led_driver =3D { > > + .driver =3D { > > + .name =3D "leds-mlxreg", > > + }, > > + .probe =3D mlxreg_led_probe, > > + .remove =3D mlxreg_led_remove, > > +}; > > + > > +module_platform_driver(mlxreg_led_driver); > > + > > +MODULE_AUTHOR("Vadim Pasternak "); > > +MODULE_DESCRIPTION("Mellanox LED regmap driver"); > > +MODULE_LICENSE("Dual BSD/GPL"); MODULE_ALIAS("platform:leds- > mlxreg"); > > >=20 > -- > Best regards, > Jacek Anaszewski Thank you, Vadim.