Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751717AbdI1FEG (ORCPT ); Thu, 28 Sep 2017 01:04:06 -0400 Received: from mail-wr0-f182.google.com ([209.85.128.182]:50074 "EHLO mail-wr0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750977AbdI1FEB (ORCPT ); Thu, 28 Sep 2017 01:04:01 -0400 X-Google-Smtp-Source: AOwi7QAEK0Fdw7l6OTV6frmooA5OBoghCzXAZakv5Y6PKoGEI2Qnofzy7DIrp/YfGVEuty0SX4VQTkqOUzH0AYfuHEU= MIME-Version: 1.0 In-Reply-To: References: <20170913175400.42744-1-dtwlin@google.com> <20170913202032.GA30844@amd> <9c75c3a9-4123-c7f3-7725-45ba752d672a@gmail.com> <20170914205804.GA24339@amd> <7a611993-ebaa-08bb-b10c-ebe4fb9ca33a@gmail.com> From: David Lin Date: Wed, 27 Sep 2017 22:03:28 -0700 Message-ID: Subject: Re: Vibrations in input vs. LED was Re: [PATCH v2 0/3] led: ledtrig-transient: add support for hrtimer To: Dmitry Torokhov Cc: Jacek Anaszewski , Pavel Machek , "linux-input@vger.kernel.org" , Jonathan Corbet , Richard Purdie , Hans de Goede , Greg Kroah-Hartman , Rob Herring , Rom Lemarchand , "linux-doc@vger.kernel.org" , lkml , "linux-leds@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6293 Lines: 139 Dmitry, On Fri, Sep 15, 2017 at 3:30 PM, Dmitry Torokhov wrote: > On Fri, Sep 15, 2017 at 2:55 PM, Jacek Anaszewski > wrote: >> On 09/15/2017 08:34 PM, Dmitry Torokhov wrote: >>> On Thu, Sep 14, 2017 at 1:58 PM, Pavel Machek wrote: >>>> On Thu 2017-09-14 21:31:31, Jacek Anaszewski wrote: >>>>> 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. >>>> >>>> Well, no. >>>> >>>> Kernel is supposed to provide hardware abstraction, that means it >>>> should hide differences between different devices. >>>> >>>> And we already have devices using input as designed. We don't want to >>>> have situation where "on phones A, D and E, vibrations are handled via >>>> input, while on B, C and F, they are handled via /sys/class/leds". >>>> >>>> If we want to have discussion "how to make vibrations in input >>>> easier to use", well that's fair. But I don't think it is particulary hard. >>>> >>> >>> I would like to know more about why you find the FF interface hard, >> >> led-transient trigger can be activated using only following bash >> commands: >> >> # echo 1 > state >> # echo 1000 > duration >> # while [ 1 ]; do echo 1 > activate; sleep 3; done >> >> Could you share sample sequence of commands to use ff driver? > > Cut what you need from this: > https://github.com/flosse/linuxconsole/blob/master/utils/fftest.c > > If your objection is that FF is not easily engaged from the shell - > yes, but I do not think that actual users who want to do vibration do > that via shell either. On the other hand, can you drop privileges and > still allow a certain process control your vibrator via LED interface? > With FF you can pass an FD to whoever you deem worthy and later revoke > access. > > IOW sysfs interfaces are nice for quick hacks, but when you want to > use them in real frameworks, where you need to think about proper > namespaces, isolation, etc, etc, other kinds of interfaces might suit > better. > >> >>> given that for rumble you need calls - one ioctl to set up rumble >>> parameters, and a write to start the playback. The FF core should take >>> care of handling duration of the effect, ramping it up and decaying, >>> if desired, and we make sure to automatically stop effects when >>> userspace closes the fd (because of ordinary exit or crash or FD being >>> revoked). >>> >>>> If we want to say "lets move all vibrations from input to LED >>>> subsystem"... I don't think that is good idea, but its a valid >>>> discussion. Some good reasons would be needed. >>>> >>>> But having half devices use one interface and half use different one >>>> is just bad... >>> >>> Completely agree here. I just merged PWM vibra driver from Sebastian >>> Reichel, we already had regulator-haptic driver, do we need gpio-based >>> one? Or make regulator-based one working with fixed regulators? >> >> Just to clarify: the background of this discussion is the question >> whether we should remove the following lines from >> Documentation/leds/ledtrig-transient.txt: >> >> -As a specific example of this use-case, let's look at vibrate feature on >> -phones. Vibrate function on phones is implemented using PWM pins on SoC or >> -PMIC. There is a need to activate one shot timer to control the vibrate >> -feature, to prevent user space crashes leaving the phone in vibrate mode >> -permanently causing the battery to drain. >> whether we should remove the following use case example from >> >> In effect Pavel has objections to increasing ledtrig-transient >> interval accuracy by adding hr_timer support to it, because vibrate >> devices, as one of the use cases, can benefit from it. >> >> So there are two issues: >> 1. Addition of hr_timer support to LED trigger. >> 2. Removal of vibrate devices use case from ledtrig-transient doc. >> >> I am in favour of 1. and against 2. since we're not gaining anything >> by hiding information about some kernel functionality when it will >> still be there. It also doesn't define the location of any vibrate >> device drivers, since sheer leds-gpio driver can be used for that >> purpose. > > I would say that while leds-gpio can be used to do whatever (you can > wire PS/2 mouse to a set of gpios and probably manage to drive it > through a set of leds-gpio devices) it does not make it right > interface for everything. We do have a standard interface for haptic > (via rumble FF), at this moment I see no reason why we need another > one. It just confuses userspace as it now needs to implement multiple > interfaces, depending on the system it runs. Android expects that HAL > will take care of hiding all of that from their upper layers and does > not really pay close attention to kernel ABIs, but I think we should. One thing I noticed is that input_ff_create_memless() also does not use high-resolution timer hence it also does not have the stop-time precision that I'm looking for. Another thing is that ff_reply duration value is maxed-out at 32767 ms (u16) while the Android/userspace requires size long for performing long notification alert. David