From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752396AbcIIHVY (ORCPT ); Fri, 9 Sep 2016 03:21:24 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:37209 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751448AbcIIHVU (ORCPT ); Fri, 9 Sep 2016 03:21:20 -0400 X-AuditID: cbfec7f4-f79cb6d000001359-da-57d262ed83fb Subject: Re: [PATCH v2] leds: Introduce userspace leds driver To: David Lechner , Richard Purdie References: <1473361440-9668-1-git-send-email-david@lechnology.com> Cc: linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org, Marcel Holtmann From: Jacek Anaszewski Message-id: <84510542-4faa-9763-611a-be3890f3f1f7@samsung.com> Date: Fri, 09 Sep 2016 09:21: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: <1473361440-9668-1-git-send-email-david@lechnology.com> Content-type: text/plain; charset=windows-1252; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrNLMWRmVeSWpSXmKPExsVy+t/xy7pvky6FG2zbI26xqEHM4vKuOWwW W9+sY7T49ukXo8XuXU9ZHVg91u9ezu7xqf8kq8ee+T9YPT5vkgtgieKySUnNySxLLdK3S+DK mLrhJUtBV0bFtt457A2MewK6GDk5JARMJBbtu8wOYYtJXLi3nq2LkYtDSGApo8SUF9OYIJxn jBKnpn0AqxIWsJX4+fY3C4gtIuAjsfDsArC4kICzxJuu3UBxDg5mgVSJlZtcQMJsAoYSP1+8 ZgKxeQXsJD78amQFsVkEVCX+X3kHNkZUIELi1qqPjBA1ghI/Jt8Di3MKuEi077/KBmIzA61d 8H4dC4QtL7F5zVvmCYwCs5C0zEJSNgtJ2QJG5lWMoqmlyQXFSem5hnrFibnFpXnpesn5uZsY IUH8ZQfj4mNWhxgFOBiVeHgf5F0MF2JNLCuuzD3EKMHBrCTC2xV7KVyINyWxsiq1KD++qDQn tfgQozQHi5I479xd70OEBNITS1KzU1MLUotgskwcnFINjFw3FvHJG3xbMsWzuzNnSs6a/seH o9fr/dI+c9AmpON+0pY9eird5wKuy/W+21fwctfKZb+/GZqfOhN9Ru88Y6H6AaWbnCc+f9// 59EHCcZlPPIL5TprT6VMedi+8sLzSW71Vt3LpU32CLtvuBGvWsuk23jB6Alzd6tL3Xf2k1Nd NTWzPtten6PEUpyRaKjFXFScCAD1gDKtXgIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi David, Thanks for the update. Even more nitpicking below. Please also rebase the patch on top of current linux-leds.git, for-next branch. On 09/08/2016 09:04 PM, David Lechner wrote: > This driver creates a userspace leds driver similar to uinput. > > New leds are created by opening /dev/uleds and writing a uleds_user_dev > struct. A new leds class device is registered with the name given in the > struct. Reading will return a single byte that is the current brightness. > The poll() syscall is also supported. It will be triggered whenever the > brightness changes. Closing the file handle to /dev/uleds will remove > the leds class device. > > Signed-off-by: David Lechner > --- > > v2 changes: > > * sort #includes > * fix typo in Kconfig > * Add Documentation text file > > Documentation/leds/uleds.txt | 105 ++++++++++++++++++++ > drivers/leds/Kconfig | 8 ++ > drivers/leds/Makefile | 3 + > drivers/leds/uleds.c | 224 +++++++++++++++++++++++++++++++++++++++++++ > include/uapi/linux/Kbuild | 1 + > include/uapi/linux/uleds.h | 23 +++++ > 6 files changed, 364 insertions(+) > create mode 100644 Documentation/leds/uleds.txt > create mode 100644 drivers/leds/uleds.c > create mode 100644 include/uapi/linux/uleds.h > > diff --git a/Documentation/leds/uleds.txt b/Documentation/leds/uleds.txt > new file mode 100644 > index 0000000..1c033d8 > --- /dev/null > +++ b/Documentation/leds/uleds.txt > @@ -0,0 +1,105 @@ > +Userspace LEDs > +============== > + > +The uleds driver supports userspace LEDs. This can be useful for testing > +triggers and can also be used to implement virtual LEDs. > + > + > +Usage > +===== > + > +When the driver is loaded, a character devices is created at /dev/uleds. To > +create a new leds class device, open /dev/uleds and write a uleds_user_dev > +structure to it. > + > + #define ULEDS_MAX_NAME_SIZE 80 > + > + struct uleds_user_dev { > + char name[ULEDS_MAX_NAME_SIZE]; > + }; Maybe it would be worth to mention that these definitions are available in the public kernel API header. > + > +A new leds class device will be created with the name given. The name can be s/leds/LED/ > +any valid file name, but consider using the leds class convention of s/leds class/LED class device naming convention/ > +"devicename:color:function". > + > +The current brightness is found by reading a single byte from the character > +device. Values are unsigned: 0 to 255. Reading does not block and always returns > +the most recent brightness value. The device node can also be polled to notify > +when the brightness value changes. > + > +The leds class device will be removed when the open file handle to /dev/uleds s/leds/LED/ > +is closed. > + > + > +Example > +======= > + > +/* > + * uledmon.c > + * > + * This program creates a new userspace leds class device and monitor it. A s/leds/LED/ > + * timestamp and brightness value is printed each time the brightness changes. > + * > + * Usage: uledmon > + * > + * is the name of the leds class device to be created. Pressing s/leds/LED/ > + * CTRL+C will exit. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +int main(int argc, char const *argv[]) > +{ > + struct uleds_user_dev uleds_dev; > + int fd, ret; > + struct pollfd pfd; > + unsigned char brightness; > + struct timespec ts; > + > + if (argc != 2) { > + fprintf(stderr, "Requires argument\n"); > + return 1; > + } > + > + strncpy(uleds_dev.name, argv[1], ULEDS_MAX_NAME_SIZE); > + > + fd = open("/dev/uleds", O_RDWR); > + if (fd == -1) { > + perror("Failed to open /dev/uleds"); > + return 1; > + } > + > + ret = write(fd, &uleds_dev, sizeof(uleds_dev)); > + if (ret == -1) { > + perror("Failed to write to /dev/uleds"); > + close(fd); > + return 1; > + } > + > + pfd.fd = fd; > + pfd.events = POLLIN; > + pfd.revents = 0; > + > + while (!(pfd.revents & (POLLERR | POLLHUP | POLLNVAL))) { > + ret = read(fd, &brightness, 1); > + if (ret == -1) { > + perror("Failed to read from /dev/uleds"); > + close(fd); > + return 1; > + } > + clock_gettime(CLOCK_MONOTONIC, &ts); > + printf("[%ld.%09ld] %u\n", ts.tv_sec, ts.tv_nsec, brightness); > + poll(&pfd, 1, -1); > + } > + > + close(fd); > + > + return 0; > +} > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 9dcc9b1..94550f6 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_USER > + tristate "Userspace LED support" > + depends on LEDS_CLASS > + help > + This option enables support for userspace LEDs. Say 'y' to enable this > + support in kernel. To compile this driver as a module, choose 'm' here: > + the module will be called uleds. > + > comment "LED Triggers" > source "drivers/leds/trigger/Kconfig" > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 0684c86..d723eeb 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -72,5 +72,8 @@ obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o > # LED SPI Drivers > obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o > > +# LED Userspace Drivers > +obj-$(CONFIG_LEDS_USER) += uleds.o > + > # LED Triggers > obj-$(CONFIG_LEDS_TRIGGERS) += trigger/ > diff --git a/drivers/leds/uleds.c b/drivers/leds/uleds.c > new file mode 100644 > index 0000000..77a0bee > --- /dev/null > +++ b/drivers/leds/uleds.c > @@ -0,0 +1,224 @@ > +/* > + * Userspace driver for leds subsystem > + * > + * Copyright (C) 2016 David Lechner > + * > + * Based on uinput.c: Aristeu Sergio Rozanski Filho > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#define ULEDS_NAME "uleds" > + > +enum uleds_state { > + ULEDS_STATE_UNKNOWN, > + ULEDS_STATE_REGISTERED, > +}; > + > +struct uleds_device { > + struct uleds_user_dev user_dev; > + struct led_classdev led_cdev; > + struct mutex mutex; > + enum uleds_state state; > + wait_queue_head_t waitq; > + unsigned char brightness; > + unsigned char new_data; > +}; > + > +static void uleds_brightness_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) > +{ > + struct uleds_device *udev = container_of(led_cdev, struct uleds_device, > + led_cdev); > + > + if (udev->brightness != brightness) { > + udev->brightness = brightness; > + udev->new_data = 1; > + wake_up_interruptible(&udev->waitq); > + } > +} > + > +static int uleds_open(struct inode *inode, struct file *file) > +{ > + struct uleds_device *udev; > + > + udev = kzalloc(sizeof(*udev), GFP_KERNEL); > + if (!udev) > + return -ENOMEM; > + > + udev->led_cdev.name = udev->user_dev.name; > + udev->led_cdev.max_brightness = LED_FULL; > + udev->led_cdev.brightness_set = uleds_brightness_set; > + > + mutex_init(&udev->mutex); > + init_waitqueue_head(&udev->waitq); > + udev->state = ULEDS_STATE_UNKNOWN; > + > + file->private_data = udev; > + nonseekable_open(inode, file); > + > + return 0; > +} > + > +static ssize_t uleds_write(struct file *file, const char __user *buffer, > + size_t count, loff_t *ppos) > +{ > + struct uleds_device *udev = file->private_data; > + int ret; > + > + if (count == 0) > + return 0; > + > + ret = mutex_lock_interruptible(&udev->mutex); > + if (ret) > + return ret; > + > + if (udev->state == ULEDS_STATE_REGISTERED) { > + ret = -EBUSY; > + goto out; > + } > + > + if (count != sizeof(struct uleds_user_dev)) { > + ret = -EINVAL; > + goto out; > + } > + > + if (copy_from_user(&udev->user_dev, buffer, > + sizeof(struct uleds_user_dev))) { > + ret = -EFAULT; > + goto out; > + } > + > + if (!udev->user_dev.name[0]) { > + ret = -EINVAL; > + goto out; > + } > + > + ret = led_classdev_register(NULL, &udev->led_cdev); > + if (ret < 0) > + goto out; > + > + udev->state = ULEDS_STATE_REGISTERED; > + ret = count; > + > +out: > + mutex_unlock(&udev->mutex); > + > + return ret; > +} > + > +static ssize_t uleds_read(struct file *file, char __user *buffer, size_t count, > + loff_t *ppos) > +{ > + struct uleds_device *udev = file->private_data; > + ssize_t retval; > + > + if (count == 0) > + return 0; > + > + if (count != 1) > + return -EINVAL; > + > + do { > + retval = mutex_lock_interruptible(&udev->mutex); > + if (retval) > + return retval; > + > + if (udev->state != ULEDS_STATE_REGISTERED) { > + retval = -ENODEV; > + } else if (!udev->new_data && (file->f_flags & O_NONBLOCK)) { > + retval = -EAGAIN; > + } else { > + retval = copy_to_user(buffer, &udev->brightness, 1); > + udev->new_data = 0; > + retval = 1; > + } > + > + mutex_unlock(&udev->mutex); > + > + if (retval || count == 0) > + break; > + > + if (!(file->f_flags & O_NONBLOCK)) > + retval = wait_event_interruptible(udev->waitq, > + udev->new_data || > + udev->state != ULEDS_STATE_REGISTERED); > + } while (retval == 0); > + > + return retval; > +} > + > +static unsigned int uleds_poll(struct file *file, poll_table *wait) > +{ > + struct uleds_device *udev = file->private_data; > + > + poll_wait(file, &udev->waitq, wait); > + > + if (udev->new_data) > + return POLLIN | POLLRDNORM; > + > + return 0; > +} > + > +static int uleds_release(struct inode *inode, struct file *file) > +{ > + struct uleds_device *udev = file->private_data; > + > + if (udev->state == ULEDS_STATE_REGISTERED) { > + udev->state = ULEDS_STATE_UNKNOWN; > + led_classdev_unregister(&udev->led_cdev); > + } > + kfree(udev); > + > + return 0; > +} > + > +static const struct file_operations uleds_fops = { > + .owner = THIS_MODULE, > + .open = uleds_open, > + .release = uleds_release, > + .read = uleds_read, > + .write = uleds_write, > + .poll = uleds_poll, > + .llseek = no_llseek, > +}; > + > +static struct miscdevice uleds_misc = { > + .fops = &uleds_fops, > + .minor = MISC_DYNAMIC_MINOR, > + .name = ULEDS_NAME, > +}; > + > +static int __init uleds_init(void) > +{ > + return misc_register(&uleds_misc); > +} > +module_init(uleds_init); > + > +static void __exit uleds_exit(void) > +{ > + misc_deregister(&uleds_misc); > +} > +module_exit(uleds_exit); > + > +MODULE_AUTHOR("David Lechner "); > +MODULE_DESCRIPTION("Userspace driver for leds subsystem"); > +MODULE_LICENSE("GPL"); > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild > index 185f8ea..416f5e6 100644 > --- a/include/uapi/linux/Kbuild > +++ b/include/uapi/linux/Kbuild > @@ -421,6 +421,7 @@ header-y += udp.h > header-y += uhid.h > header-y += uinput.h > header-y += uio.h > +header-y += uleds.h > header-y += ultrasound.h > header-y += un.h > header-y += unistd.h > diff --git a/include/uapi/linux/uleds.h b/include/uapi/linux/uleds.h > new file mode 100644 > index 0000000..e78ed46 > --- /dev/null > +++ b/include/uapi/linux/uleds.h > @@ -0,0 +1,23 @@ > +/* > + * Userspace driver support for leds subsystem > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#ifndef _UAPI__ULEDS_H_ > +#define _UAPI__ULEDS_H_ > + > +#define ULEDS_MAX_NAME_SIZE 80 > + > +struct uleds_user_dev { > + char name[ULEDS_MAX_NAME_SIZE]; > +}; > + > +#endif /* _UAPI__ULEDS_H_ */ > -- Best regards, Jacek Anaszewski