From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752006AbbEDTyP (ORCPT ); Mon, 4 May 2015 15:54:15 -0400 Received: from smtp-out-180.synserver.de ([212.40.185.180]:1058 "EHLO smtp-out-180.synserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751613AbbEDTyM (ORCPT ); Mon, 4 May 2015 15:54:12 -0400 X-SynServer-TrustedSrc: 1 X-SynServer-AuthUser: lars@metafoo.de X-SynServer-PPID: 22541 Message-ID: <5547CE60.5000300@metafoo.de> Date: Mon, 04 May 2015 21:54:08 +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: Daniel Baluta , jic23@kernel.org CC: jlbec@evilplan.org, knaack.h@gmx.de, linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, octavian.purdila@intel.com, pebolle@tiscali.nl, patrick.porlan@intel.com, adriana.reus@intel.com, constantin.musca@intel.com, marten@intuitiveaerial.com Subject: Re: [PATCH v5 3/4] iio: trigger: Introduce IIO hrtimer based trigger References: <1430736604-22119-1-git-send-email-daniel.baluta@intel.com> <1430736604-22119-4-git-send-email-daniel.baluta@intel.com> In-Reply-To: <1430736604-22119-4-git-send-email-daniel.baluta@intel.com> Content-Type: text/plain; charset=windows-1252; 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/04/2015 12:50 PM, Daniel Baluta wrote: [...] > +IIO_HRTIMER_INFO_ATTR(sampling_frequency, S_IRUGO | S_IWUSR, > + iio_hrtimer_info_show_sampling_frequency, > + iio_hrtimer_info_store_sampling_frequency); I wonder if the sampling frequency should be configurable the regular IIO API, just like any other IIO device. But things like min/max sampling frequency should be configured in configfs. [...] > +#endif /* CONFIGFS_FS */ > + [...] > +static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char *name) > +{ [...] > +#ifdef CONFIG_CONFIGFS_FS > + config_group_init_type_name(&trig_info->swt.group, name, > + &iio_hrtimer_type); > +#endif This should probably have a helper function in the sw trigger core, that gets stubbed out when CONFIG_FS is disabled. Otherwise we'll see the same #ifdef in every software trigger driver. [...] > +} > + > +static int iio_trig_hrtimer_remove(struct iio_sw_trigger *swt) > +{ > + struct iio_hrtimer_info *trig_info; > + > + trig_info = iio_trigger_get_drvdata(swt->trigger); > + > + hrtimer_cancel(&trig_info->timer); > + > + iio_trigger_unregister(swt->trigger); > + iio_trigger_free(swt->trigger); There is a bit of a race condition here. hrtimer_cancel() should be called between unregister and free, otherwise it might be re-armed before it is unregistered. > + kfree(trig_info); > + > + return 0; > +} > + > +struct iio_sw_trigger_ops iio_trig_hrtimer_ops = { const > + .probe = iio_trig_hrtimer_probe, > + .remove = iio_trig_hrtimer_remove, > +}; [...]