From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C4F32C433F5 for ; Sun, 9 Oct 2022 11:44:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229797AbiJILo1 (ORCPT ); Sun, 9 Oct 2022 07:44:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56398 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229602AbiJILo0 (ORCPT ); Sun, 9 Oct 2022 07:44:26 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5A9742AC66 for ; Sun, 9 Oct 2022 04:44:25 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id EAA5F60BA9 for ; Sun, 9 Oct 2022 11:44:24 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C44B5C433D6; Sun, 9 Oct 2022 11:44:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1665315864; bh=XoBn8JDusgMDII6knjqYs6PwnqkxgJicWV318ZzCTcM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=QKVAJXkVUMgWtJTkovQsQQK4QfJVZqYwc63drlULdhLnU9EQL3CFA7aNdtn4yPhWq /o4d5V+M545Qh2LwK1PrWwCMqnTKQ/GCtwyQt+afjyRiFFJ8tU+1ZGa6lXnES9vj4x P3X2DYn+cWcaKXYmQq0GggcBgdsRS4wTZtn5ZlO72kDpD+XYo3zocAArQtQuz6FtF0 jIVLd0Ucv8Z2aSTGwS+Pm/J8FnGs+pajzEoxa9/l1Mh07IXaJ5rIEkOVOI2k5S/wqQ cSobF8c5beTVLxIaZPUFu1AUp05Jejx1F/c0UpHgWcB+Zf9XG+NpqfrM5gESbP1RbS SF9kU43X0ad+A== Date: Sun, 9 Oct 2022 12:44:40 +0100 From: Jonathan Cameron To: Nuno =?UTF-8?B?U8Oh?= Cc: Andy Shevchenko , Nuno =?UTF-8?B?U8Oh?= , linux-amlogic@lists.infradead.org, linux-imx@nxp.com, linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org, Heiko Stuebner , Martin Blumenstingl , Neil Armstrong , Shawn Guo , Lars-Peter Clausen , Jyoti Bhayana , Hans de Goede , Andriy Tryshnivskyy , Pengutronix Kernel Team , Miquel Raynal , Cixi Geng , Baolin Wang , Ciprian Regus , Fabio Estevam , Sascha Hauer , Alexandru Ardelean , Florian Boor , Michael Hennerich , Orson Zhai , Chen-Yu Tsai , Chunyan Zhang , Vladimir Zapolskiy , Jerome Brunet , Haibo Chen , Kevin Hilman Subject: Re: [PATCH v2 15/16] iio: health: max30102: do not use internal iio_dev lock Message-ID: <20221009124440.71adf505@jic23-huawei> In-Reply-To: <428661431e54744064946ced681cc0351d51d698.camel@gmail.com> References: <20221004134909.1692021-1-nuno.sa@analog.com> <20221004134909.1692021-16-nuno.sa@analog.com> <428661431e54744064946ced681cc0351d51d698.camel@gmail.com> X-Mailer: Claws Mail 4.1.0 (GTK 3.24.34; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-iio@vger.kernel.org On Wed, 05 Oct 2022 10:17:00 +0200 Nuno S=C3=A1 wrote: > On Tue, 2022-10-04 at 17:15 +0300, Andy Shevchenko wrote: > > On Tue, Oct 4, 2022 at 4:49 PM Nuno S=C3=A1 wrote:= =20 > > >=20 > > > The pattern used in this device does not quite fit in the > > > iio_device_claim_direct_mode() typical usage. In this case, we want > > > to > > > know if we are in buffered mode or not to know if the device is > > > powered > > > (buffer mode) or not. And depending on that max30102_get_temp() > > > will > > > power on the device if needed. Hence, in order to keep the same > > > functionality, we try to: > > >=20 > > > 1. Claim Buffered mode; > > > 2: If 1) succeeds call max30102_get_temp() without powering on the > > > =C2=A0=C2=A0 device; > > > 3: Release Buffered mode; > > > 4: If 1) fails, Claim Direct mode; > > > 5: If 4) succeeds call max30102_get_temp() with powering on the > > > device; > > > 6: Release Direct mode; > > > 7: If 4) fails, goto to 1) and try again. > > >=20 > > > This dance between buffered and direct mode is not particularly > > > pretty > > > (as well as the loop introduced by the goto statement) but it does > > > allow > > > us to get rid of the mlock usage while keeping the same behavior. =20 > >=20 > > ... > > =20 > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 if (iio_device_claim_buffer_mode(indio_dev)) { =20 > >=20 > > Why is ret not used here? > > =20 > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * = This one is a *bit* hacky. If we cannot > > > claim buffer > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * = mode, then try direct mode so that we > > > make sure > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * = things cannot concurrently change. And > > > we just keep > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * = trying until we get one of the modes... > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */ > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if > > > (iio_device_claim_direct_mode(indio_dev)) =20 > >=20 > > ...and here? > > =20 > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto any_mode_retry; =20 > > =20 > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 } else { =20 > > =20 > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 } =20 > >=20 > > I.o.w. what error code will be visible to the caller and why. > > =20 >=20 > Note that we do not really care about the error code returned by both > iio_device_claim_buffer_mode() and iio_device_claim_direct_mode(). We > just loop until we get one of the modes (thus ret =3D 0) so that we can > safely call one of the max30102_get_temp() variants. And that will be > the visible error code (if any). That said, you can look at the first > version of the series about this (and the previous patch) and why this > is being done like this (note that I'm also not 100% convinced about > this ping pong between getting one of the IIO modes but it's also not > that bad and if there's no major complains, I'm fine with it). This is a vanishingly rare corner case and not in a remotely high performan= ce path so I'm not keen on introducing a more complex API just to handle this corner. If we added a means to get the lock independent of mode we'd have an interface that is far to likely to get missused. What you have here does the job and looks nasty enough to put people off copying it unless they really need it! Jonathan >=20 > - Nuno S=C3=A1