From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Andersson Subject: Re: [PATCH v6 1/1] input: add driver for Bosch Sensortec's BMA150 accelerometer Date: Sun, 7 Aug 2011 18:23:25 +0200 Message-ID: <20110807162325.GA7280@skinner.xfiles.lan> References: <1311800027-25982-1-git-send-email-eric.andersson@unixphere.com> <1311800027-25982-2-git-send-email-eric.andersson@unixphere.com> <20110730191726.GG5686@core.coreip.homeip.net> <20110730224917.GA8519@skinner.xfiles.lan> <20110731070844.GA23383@core.coreip.homeip.net> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from csmtp2.one.com ([91.198.169.22]:38365 "EHLO csmtp2.one.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752605Ab1HGQS5 (ORCPT ); Sun, 7 Aug 2011 12:18:57 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: dmitry.torokhov@gmail.com Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, zhengguang.guo@bosch-sensortec.com, stefan.nilsson@unixphere.com, alan@lxorguk.ukuu.org.uk, jic23@cam.ac.uk, xu.zhang@bosch-sensortec.com > On Sun, Jul 31, 2011 at 12:49:17AM +0200, Dmitry Torokhov wrote: > > > > +static int bma150_open(struct bma150_data *bma150) > > > > +{ > > > > + int ret; > > > > + > > > > + ret =3D pm_runtime_set_active(&bma150->client->dev); > > > > + if (ret < 0) > > > > + =A0 =A0 =A0 =A0 return ret; > > > > + > > > > + pm_runtime_enable(&bma150->client->dev); > > > > > > I am pretty sure you want to call pm_runtime_enable() in bma150_p= robe() > > > so that parent controller can be suspended until somebody calls > > > bma150_open() and we mark the device as active (which, in turn, s= hould > > > wake up its parent). > > > > If I call pm_runtime_enable() I cannot use pm_runtime_set_active() = later > > according to the comment in __pm_runtime_set_status (runtime.c): > > "If runtime PM of the device is disabled or its power.runtime_error > > field is different from zero, the status may be changed either to > > RPM_ACTIVE, or to RPM_SUSPENDED..." > > > > If the PM of the device is enabled it will return -EAGAIN. Of cours= e, we > > could enable() in probe, then disable(); set_active(); enable(); in > > open, but that seems a bit confused, right? >=20 > Hmm, indeed. I do not like the idea about disable/set_active/enable s= o I > guess we'll have to keep track of the current mode themselves and cal= l > bma150_set_mode() ourselves if, after calling pm_runtime_get/put_sync= () > we find that our mode is different from what we expect it to be. >=20 > I also noticed that you did not properly free IRQ on device removal a= nd > also polled devices need to be freed always (unlike regular input > devices that should be only unregistered). The default_cfg can't be > __initdata because it is used by __devinit functions. And my version = of > GCC can't figure out that ipoll_dev never used uninitialized and give= s > false warning. >=20 > I tried correcting these issues in the patch below, along with renami= ng > 'ret' to 'error' which I prefer when we dealing with error handling. > Could you please git it a try and if everything still works I'll fold= it > and commit. Thanks Dmitry! I tried your patch and it worked like a charm! Do you take it from here or would you like me to send an updated version that includes your patch? --=20 Best regards, Eric http://www.unixphere.com -- To unsubscribe from this list: send the line "unsubscribe linux-input" = in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html