From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754686AbbEMSJ3 (ORCPT ); Wed, 13 May 2015 14:09:29 -0400 Received: from smtp-out-163.synserver.de ([212.40.185.163]:1045 "EHLO smtp-out-163.synserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752369AbbEMSJZ (ORCPT ); Wed, 13 May 2015 14:09:25 -0400 X-SynServer-TrustedSrc: 1 X-SynServer-AuthUser: lars@metafoo.de X-SynServer-PPID: 8307 Message-ID: <55539353.4050702@metafoo.de> Date: Wed, 13 May 2015 20:09:23 +0200 From: Lars-Peter Clausen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.6.0 MIME-Version: 1.0 To: Robert Dolca , Jonathan Cameron CC: Robert Dolca , "linux-iio@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Hartmut Knaack , Peter Meerwald , Denis CIOCCA Subject: Re: [PATCH RFC 1/3] iio: Add symlink to triggers in the device's trigger folder References: <1429174868-11953-1-git-send-email-robert.dolca@intel.com> <1429174868-11953-2-git-send-email-robert.dolca@intel.com> <554CD236.4000404@kernel.org> <555230B0.3050700@metafoo.de> <55524F4C.90604@kernel.org> <5552FD16.9050306@metafoo.de> <55538D1A.80500@kernel.org> <5553927F.2090804@metafoo.de> In-Reply-To: <5553927F.2090804@metafoo.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/13/2015 08:05 PM, Lars-Peter Clausen wrote: > On 05/13/2015 08:03 PM, Robert Dolca wrote: >> On Wed, May 13, 2015 at 8:42 PM, Jonathan Cameron wrote: >>> >>> On 13/05/15 08:28, Lars-Peter Clausen wrote: >>>> On 05/12/2015 09:06 PM, Jonathan Cameron wrote: >>>>> On 12/05/15 17:56, Lars-Peter Clausen wrote: >>>>>> On 05/08/2015 05:11 PM, Jonathan Cameron wrote: >>>>>>> On 16/04/15 05:01, Robert Dolca wrote: >>>>>>>> This patch adds a new function called iio_trigger_register_with_dev >>>>>>>> which is a wrapper for iio_trigger_register. Besides the iio_trigger >>>>>>>> struct this function requires iio_dev struct. It adds the trigger in >>>>>>>> the device's trigger list and saves a reference to the device in the >>>>>>>> trigger's struct. >>>>>>>> >>>>>>>> When the device is registered, in the trigger folder of the device >>>>>>>> (where current_trigger file resides) a symlink is being created for >>>>>>>> each trigger that was registered width iio_trigger_register_with_dev. >>>>>>>> >>>>>>>> # ls -l /sys/bus/iio/devices/iio:device0/trigger/ >>>>>>>> total 0 >>>>>>>> -rw-r--r-- 1 root root 4096 Apr 16 08:33 >>>>>>>> current_trigger >>>>>>>> lrwxrwxrwx 1 root root 0 Apr 16 08:33 trigger0 -> >>>>>>>> ../../trigg >>>>>>>> er0 >>>>>>>> >>>>>>>> This should be used for device specific triggers. Doing this the >>>>>>>> user space >>>>>>>> applications can figure out what if the trigger registered by a >>>>>>>> specific device >>>>>>>> and what should they write in the current_trigger file. Currently some >>>>>>>> applications rely on the trigger name and this does not always work. >>>>>>>> >>>>>>>> This implementation assumes that the trigger is registered before >>>>>>>> the device is >>>>>>>> registered. If the order is not this the symlink will not be created >>>>>>>> but >>>>>>>> everything else will work as before. >>>>>>>> >>>>>>>> Signed-off-by: Robert Dolca >>>>>>> I was rather hoping we'd get a few more comments on this. >>>>>>> In principle I like the idea, but it's new ABI and does make life >>>>>>> a tiny bit more complex, so what do people think? >>>>>>> >>>>>>> Few trivial code comments inline. >>>>>> >>>>>> I don't think it adds more information. Both the trigger and the >>>>>> device get registered for the same parent device, so you can already >>>>>> easily find the trigger for a device by going to the parent device >>>>>> and taking a look at the triggers registered by the parent device. >>>>> I had the same thought. The question is whether the slightly gain in >>>>> simplicity for userspace is worth it... I'm undecided at the moment. >>>> >>>> As you may have guessed by now I'm always quite conservative when it >>>> comes to introducing new ABI. Simply because we have to maintain it >>>> forever, the less stuff to maintain forever the better. >>>> >>>> Hence I think all new ABI needs a compelling reason, e.g. like a >>>> improvement in performance. And of course this patch slightly >>>> simplifies things, but in my opinion not enough to justify a ABI >>>> extension. We can always find ways to simplify the interface, but the >>>> metric for ABI should be whether the simplification actually matters. >>>> In this case I don't think it does, finding the trigger for a device >>>> is not really hot-path. The amount of time saved will be disappear in >>>> the noise. >>>> >>>> And in my opinion applications shouldn't directly use the low-level >>>> ABI but rather use middle-ware libraries/frameworks, like e.g. >>>> libiio, and that's where you'd hide the complexities of a operation. >>>> >>>> - Lars >>> I'll go with Lars response on this one. Not worth the hassle. >>> That's the nature of an RFC of course! >>> >>> Jonathan >> >> Would it be acceptable to add the symlinks without adding a new API? >> When a trigger is registered you could use the common parent to get a >> pointer to the iio_dev and then create the symlink. This is a little >> bit complicated but I think it can be done. > > The concerns are with the symlink and with he symlink only. Adding new API > inside the kernel is generally not as much of a problem as external ABI. Sorry, I should have added you can easily find out which triggers and which devices belong together by basically doing a dirname `readlink /sys/bus/iio/devices/X`. Those that are at the same place in the global hierarchy belong to the same device.