Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v4 1/2] leds: core: Introduce generic pattern interface
To:     Baolin Wang <baolin.wang@linaro.org>, pavel@ucw.cz
Cc:     bjorn.andersson@linaro.org, broonie@kernel.org,
        linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org
References: <a0b874372f9ccd0491b54c79ac63b41812efef56.1531460834.git.baolin.wang@linaro.org>
From:   Jacek Anaszewski <jacek.anaszewski@gmail.com>
Message-ID: <7263fcd3-4bd0-d3b1-5468-5bd6234fbd53@gmail.com>
Date:   Fri, 13 Jul 2018 22:07:13 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <a0b874372f9ccd0491b54c79ac63b41812efef56.1531460834.git.baolin.wang@linaro.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Baolin,

Thank you for the update.

On 07/13/2018 08:21 AM, Baolin Wang wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Some LED controllers have support for autonomously controlling
> brightness over time, according to some preprogrammed pattern or
> function.
> 
> This adds a new optional operator that LED class drivers can implement
> if they support such functionality as well as a new device attribute to
> configure the pattern for a given LED.
> 
> [Baolin Wang did some improvements.]
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v3:
>   - Move the check in pattern_show() to of_led_classdev_register().
>   - Add more documentation to explain how to set/clear one pattern.
> 
> Changes from v2:
>   - Change kernel version to 4.19.
>   - Force user to return error pointer if failed to issue pattern_get().
>   - Use strstrip() to trim trailing newline.
>   - Other optimization.
> 
> Changes from v1:
>   - Add some comments suggested by Pavel.
>   - Change 'delta_t' can be 0.
> 
> Note: I removed the pattern repeat check and will get the repeat number by adding
> one extra file named 'pattern_repeat' according to previous discussion.
> ---
>   Documentation/ABI/testing/sysfs-class-led |   20 +++++
>   drivers/leds/led-class.c                  |  118 +++++++++++++++++++++++++++++
>   include/linux/leds.h                      |   19 +++++
>   3 files changed, 157 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> index 5f67f7a..f4b73ad 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -61,3 +61,23 @@ Description:
>   		gpio and backlight triggers. In case of the backlight trigger,
>   		it is useful when driving a LED which is intended to indicate
>   		a device in a standby like state.
> +
> +What: /sys/class/leds/<led>/pattern
> +Date:		July 2018
> +KernelVersion:	4.19
> +Description:
> +		Specify a pattern for the LED, for LED hardware that support
> +		altering the brightness as a function of time.
> +
> +		The pattern is given by a series of tuples, of brightness and
> +		duration (ms). The LED is expected to traverse the series and
> +		each brightness value for the specified duration. Duration of
> +		0 means brightness should immediately change to new value.
> +
> +		As LED hardware might have different capabilities and precision
> +		the requested pattern might be slighly adjusted by the driver
> +		and the resulting pattern of such operation should be returned
> +		when this file is read.
> +
> +		Writing non-empty string to this file will active the pattern,

s/active/activate/

> +		and empty string will disable the pattern.
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 3c7e348..0992a0e 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -74,6 +74,119 @@ static ssize_t max_brightness_show(struct device *dev,
>   }
>   static DEVICE_ATTR_RO(max_brightness);
>   
> +static ssize_t pattern_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_pattern *pattern;
> +	size_t offset = 0;
> +	int count, n, i;
> +
> +	pattern = led_cdev->pattern_get(led_cdev, &count);
> +	if (IS_ERR(pattern))
> +		return PTR_ERR(pattern);
> +
> +	for (i = 0; i < count; i++) {
> +		n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d ",
> +			     pattern[i].brightness, pattern[i].delta_t);
> +
> +		if (offset + n >= PAGE_SIZE)
> +			goto err_nospc;
> +
> +		offset += n;
> +	}
> +
> +	buf[offset - 1] = '\n';
> +
> +	kfree(pattern);
> +	return offset;
> +
> +err_nospc:
> +	kfree(pattern);
> +	return -ENOSPC;
> +}
> +
> +static ssize_t pattern_store(struct device *dev,
> +			     struct device_attribute *attr,
> +			     const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_pattern *pattern = NULL;
> +	char *sbegin, *elem, *s;
> +	unsigned long val;
> +	int ret = 0, len = 0;
> +	bool odd = true;
> +
> +	sbegin = kstrndup(buf, size, GFP_KERNEL);
> +	if (!sbegin)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Trim trailing newline, if the remaining string is empty,
> +	 * clear the pattern.
> +	 */
> +	s = strstrip(sbegin);
> +	if (!*s) {
> +		if (led_cdev->pattern_clear)
> +			ret = led_cdev->pattern_clear(led_cdev);

Why you don't require pattern_clear op to be initialized?
Without it there seems to be no other way to disable the pattern.

Please fail in led_classdev_register() if it is NULL, similarly
as when pattern_get is NULL.

> +		goto out;
> +	}
> +
> +	pattern = kcalloc(size, sizeof(*pattern), GFP_KERNEL);
> +	if (!pattern) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* Parse out the brightness & delta_t touples */
> +	while ((elem = strsep(&s, " ")) != NULL) {
> +		ret = kstrtoul(elem, 10, &val);
> +		if (ret)
> +			goto out;
> +
> +		if (odd) {
> +			pattern[len].brightness = val;
> +		} else {
> +			pattern[len].delta_t = val;
> +			len++;
> +		}
> +
> +		odd = !odd;
> +	}
> +
> +	/*
> +	 * Fail if we didn't find any data points or last data point was partial
> +	 */
> +	if (!len || !odd) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ret = led_cdev->pattern_set(led_cdev, pattern, len);
> +
> +out:
> +	kfree(pattern);
> +	kfree(sbegin);
> +	return ret < 0 ? ret : size;
> +}
> +static DEVICE_ATTR_RW(pattern);
> +
> +static umode_t led_class_attrs_mode(struct kobject *kobj,
> +				    struct attribute *attr, int index)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	if (attr == &dev_attr_brightness.attr)
> +		return attr->mode;
> +	if (attr == &dev_attr_max_brightness.attr)
> +		return attr->mode;
> +	if (attr == &dev_attr_pattern.attr && led_cdev->pattern_set)
> +		return attr->mode;
> +
> +	return 0;
> +}
> +
>   #ifdef CONFIG_LEDS_TRIGGERS
>   static DEVICE_ATTR(trigger, 0644, led_trigger_show, led_trigger_store);
>   static struct attribute *led_trigger_attrs[] = {
> @@ -88,11 +201,13 @@ static ssize_t max_brightness_show(struct device *dev,
>   static struct attribute *led_class_attrs[] = {
>   	&dev_attr_brightness.attr,
>   	&dev_attr_max_brightness.attr,
> +	&dev_attr_pattern.attr,
>   	NULL,
>   };
>   
>   static const struct attribute_group led_group = {
>   	.attrs = led_class_attrs,
> +	.is_visible = led_class_attrs_mode,
>   };
>   
>   static const struct attribute_group *led_groups[] = {
> @@ -256,6 +371,9 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
>   	char name[LED_MAX_NAME_SIZE];
>   	int ret;
>   
> +	if (led_cdev->pattern_set && !led_cdev->pattern_get)

if (led_cdev->pattern_set && (!led_cdev->pattern_get ||
                               !led_cdev->pattern_clear)

> +		return -EINVAL;
> +
>   	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
>   	if (ret < 0)
>   		return ret;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index b7e8255..acdbb2f 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -22,6 +22,7 @@
>   #include <linux/workqueue.h>
>   
>   struct device;
> +struct led_pattern;
>   /*
>    * LED Core
>    */
> @@ -88,6 +89,14 @@ struct led_classdev {
>   				     unsigned long *delay_on,
>   				     unsigned long *delay_off);
>   
> +	int (*pattern_set)(struct led_classdev *led_cdev,
> +			   struct led_pattern *pattern, int len);
> +
> +	int (*pattern_clear)(struct led_classdev *led_cdev);
> +
> +	struct led_pattern *(*pattern_get)(struct led_classdev *led_cdev,
> +					   int *len);
> +
>   	struct device		*dev;
>   	const struct attribute_group	**groups;
>   
> @@ -446,4 +455,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
>   	struct led_classdev *led_cdev, enum led_brightness brightness) { }
>   #endif
>   
> +/**
> + * struct led_pattern - brigheness value in a pattern
> + * @delta_t: delay until next entry, in milliseconds
> + * @brightness: brightness at time = 0
> + */
> +struct led_pattern {
> +	int delta_t;
> +	int brightness;
> +};
> +
>   #endif		/* __LINUX_LEDS_H_INCLUDED */
> 

-- 
Best regards,
Jacek Anaszewski