From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965905AbcIHJ7D (ORCPT ); Thu, 8 Sep 2016 05:59:03 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:59025 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932342AbcIHJ7B (ORCPT ); Thu, 8 Sep 2016 05:59:01 -0400 X-AuditID: cbfec7f5-f792e6d0000013f5-16-57d13660e245 Subject: Re: [PATCH] leds: Introduce userspace leds driver To: David Lechner , Richard Purdie References: <1473128158-26915-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: <04c5ee0d-9bfb-6e0a-2616-6cf720fc8763@samsung.com> Date: Thu, 08 Sep 2016 11:58:55 +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: <1473128158-26915-1-git-send-email-david@lechnology.com> Content-type: text/plain; charset=windows-1252; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrJLMWRmVeSWpSXmKPExsVy+t/xq7oJZhfDDZ40WVksahCzuLxrDpvF 1jfrGC2+ffrFaLF711NWB1aP9buXs3t86j/J6rFn/g9Wj8+b5AJYorhsUlJzMstSi/TtErgy Jp/fzV7wK7Ti7sJTbA2MB5y7GDk5JARMJNrWbGeHsMUkLtxbzwZiCwksZZSYMyu3i5ELyH7G KPF04n/GLkYODmEBK4meddwgNSICPhILzy5gh6h3kXh8qYkdpIRZIFVi5SYXkDCbgKHEzxev mUBsXgE7if2blzOC2CwCqhJtDzeD2aICERK3Vn1khKgRlPgx+R4LiM0p4CpxYv0VsHOYBWwl FrxfxwJhy0tsXvOWeQKjwCwkLbOQlM1CUraAkXkVo2hqaXJBcVJ6rpFecWJucWleul5yfu4m RkgAf93BuPSY1SFGAQ5GJR5eDukL4UKsiWXFlbmHGCU4mJVEeP+ZXAwX4k1JrKxKLcqPLyrN SS0+xCjNwaIkzjtz1/sQIYH0xJLU7NTUgtQimCwTB6dUA2NQmNWJg5cudBz2bvz+Q36GL7Pc y2j9zCUnMio/3K768o77WmIx6yL9tOAP14rqPNfKnP7HZhdaJ810ie25n1tSbsS/xOYPdrFr Hz7JnCJluTDbr+B4t5x5M3+RUcr0qPcdK69VrDx4bMctNvUN8xoyn7zzkcxumpn+OUn+2d7S /3tqpto1uCuxFGckGmoxFxUnAgDdQg53XAIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi David, Thanks for the patch. It is very nice. I have only one minor remark in the code. I think that it would be good to add a documentation for this driver to Documentation/leds, with exemplary C program instead of python one. The program could poll the dev node in a loop, which allows to conveniently check the impact of setting particular triggers and all other brightness change events. On 09/06/2016 04:15 AM, 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. > > Signed-off-by: David Lechner > --- > > Here is a sample python program that I used to test that it works as expected. > > #!/usr/bin/env python3 > > import select > from threading import Timer > > ULEDS_MAX_NAME_SIZE = 80 > > name = 'test' > > > def change_brightness(b): > with open('/sys/class/leds/{}/brightness'.format(name) , 'w') as f: > f.write(str(b)) > > > with open('/dev/uleds', 'rb+', 0) as uleds: > bname = name.encode("ascii") > # create the leds class device > uleds.write(bname + b'\0' * (ULEDS_MAX_NAME_SIZE - len(bname))) > > # test that it works > print(uleds.read(1)) > > # schedule some brighness changes at 1 second intervals > Timer(1, change_brightness, (1,)).start() > # this won't trigger poll since brighness does not change > Timer(2, change_brightness, (1,)).start() > Timer(3, change_brightness, (2,)).start() > > # wait for the scheduled changes > p = select.poll() > p.register(uleds.fileno(), select.POLLIN) > p.poll() > print(uleds.read(1)) > p.poll() > print(uleds.read(1)) > > > > drivers/leds/Kconfig | 8 ++ > drivers/leds/Makefile | 3 + > drivers/leds/uleds.c | 224 +++++++++++++++++++++++++++++++++++++++++++++ > include/uapi/linux/Kbuild | 1 + > include/uapi/linux/uleds.h | 23 +++++ > 5 files changed, 259 insertions(+) > create mode 100644 drivers/leds/uleds.c > create mode 100644 include/uapi/linux/uleds.h > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 9dcc9b1..25c6168 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 leds-user. > + > 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..992bca4 > --- /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 Please arrange include directives in alphabetical order. > +#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