Subject: Re: [PATCH v2 0/3] led: ledtrig-transient: add support for hrtimer
To: Pavel Machek <pavel@ucw.cz>, David Lin <dtwlin@google.com>
References: <20170913175400.42744-1-dtwlin@google.com>
 <20170913202032.GA30844@amd>
Cc: corbet@lwn.net, rpurdie@rpsys.net, hdegoede@redhat.com,
        gregkh@linuxfoundation.org, robh@kernel.org, romlem@google.com,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-leds@vger.kernel.org
From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Message-ID: <9c75c3a9-4123-c7f3-7725-45ba752d672a@gmail.com>
Date: Thu, 14 Sep 2017 21:31:31 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170913202032.GA30844@amd>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1435
Lines: 36

Hi David and Pavel,

On 09/13/2017 10:20 PM, Pavel Machek wrote:
> Hi!
> 
>> These patch series add the LED_BRIGHTNESS_FAST flag support for
>> ledtrig-transient to use hrtimer so that platforms with high-resolution timer
>> support can have better accuracy in the trigger duration timing. The need for
>> this support is driven by the fact that Android has removed the timed_ouput [1]
>> and is now using led-trigger for handling vibrator control which requires the
>> timer to be accurate up to a millisecond. However, this flag support would also
>> allow hrtimer to co-exist with the ktimer without causing warning to the
>> existing drivers [2].
> 
> NAK.
> 
> LEDs do not need extra overhead, and vibrator control should not go
> through LED subsystem.
> 
> Input subsystem includes support for vibrations and force
> feedback. Please use that instead.

I think that most vital criterion here is the usability of the
interface. If it can be harnessed for doing the work seemingly
unrelated to the primary subsystem's purpose, that's fine.
Moreover, it is extremely easy to use in comparison to the force
feedback one.

I would change one more thing in this patch, though. The hr_timer engine
should be made optional and not used by default for fast LEDs.
It could be made configurable by exposing additional sysfs file from
ledtrig-transient.c, e.g. hr_timer_support, accepting boolean value.

-- 
Best regards,
Jacek Anaszewski