From: Kalle Valo <kvalo@codeaurora.org>
To: Maya Erez <qca_merez@qca.qualcomm.com>
Cc: linux-wireless@vger.kernel.org, wil6210@qca.qualcomm.com
Subject: Re: [PATCH v2 7/7] wil6210: add support for device led configuration
References: <1461589078-29854-1-git-send-email-qca_merez@qca.qualcomm.com>
	<1461589078-29854-8-git-send-email-qca_merez@qca.qualcomm.com>
Date: Mon, 25 Apr 2016 21:56:39 +0300
In-Reply-To: <1461589078-29854-8-git-send-email-qca_merez@qca.qualcomm.com>
	(Maya Erez's message of "Mon, 25 Apr 2016 15:57:58 +0300")
Message-ID: <87a8kh1pqw.fsf@kamboji.qca.qualcomm.com> (sfid-20160425_205701_427125_1C0494F9)
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Sender: linux-wireless-owner@vger.kernel.org

Maya Erez <qca_merez@qca.qualcomm.com> writes:

> Add the ability to configure the device led to be used for notifying
> the AP activity (60G device supports leds 0-2).
> The host can also configure the blinking frequency of the led in
> three states.

[...]

> +/* led_blink_time, write:
> + * "<blink_on_slow> <blink_off_slow> <blink_on_med> <blink_off_med> <blink_on_fast> <blink_off_fast>
> + */
> +static ssize_t wil_write_led_blink_time(struct file *file,
> +					const char __user *buf,
> +					size_t len, loff_t *ppos)
> +{
> +	int rc;
> +	char *kbuf = kmalloc(len + 1, GFP_KERNEL);
> +
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	rc = simple_write_to_buffer(kbuf, len, ppos, buf, len);
> +	if (rc != len) {
> +		kfree(kbuf);
> +		return rc >= 0 ? -EIO : rc;
> +	}
> +
> +	kbuf[len] = '\0';
> +	rc = sscanf(kbuf, "%d %d %d %d %d %d",
> +		    &led_blink_time[WIL_LED_TIME_SLOW].on_ms,
> +		    &led_blink_time[WIL_LED_TIME_SLOW].off_ms,
> +		    &led_blink_time[WIL_LED_TIME_MED].on_ms,
> +		    &led_blink_time[WIL_LED_TIME_MED].off_ms,
> +		    &led_blink_time[WIL_LED_TIME_FAST].on_ms,
> +		    &led_blink_time[WIL_LED_TIME_FAST].off_ms);
> +	kfree(kbuf);
> +
> +	if (rc < 0)
> +		return rc;
> +	if (rc < 6)
> +		return -EINVAL;
> +
> +	return len;
> +}

Don't we already have a proper framework for leds? At least
include/linux/led.h and drivers/led/ makes me suspect that. I'm not
really fond of the idea reinventing the wheel, unless there's a really
good reason.

-- 
Kalle Valo