From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753381AbdJaKiq (ORCPT ); Tue, 31 Oct 2017 06:38:46 -0400 Received: from heliosphere.sirena.org.uk ([172.104.155.198]:60368 "EHLO heliosphere.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753260AbdJaKie (ORCPT ); Tue, 31 Oct 2017 06:38:34 -0400 Date: Tue, 31 Oct 2017 10:38:30 +0000 From: Mark Brown To: Baolin Wang Cc: gregkh@linuxfoundation.org, lee.jones@linaro.org, arnd@arndb.de, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] regmap: Add hardware spinlock support Message-ID: <20171031103830.qdtudo2uwsc2ssp3@sirena.co.uk> References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="bzbmeuy2gxxofavz" Content-Disposition: inline In-Reply-To: X-Cookie: You can't fall off the floor. User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --bzbmeuy2gxxofavz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Oct 31, 2017 at 02:35:55PM +0800, Baolin Wang wrote: This looks mostly good, a few small things: > +static void regmap_lock_hwlock(void *__map) > +{ > + struct regmap *map = __map; > + int ret; > + > + if (map->hwlock_timeout) > + ret = hwspin_lock_timeout(map->hwlock, map->hwlock_timeout); > + else > + ret = hwspin_trylock(map->hwlock); > + > + if (ret) > + dev_err(map->dev, "Failed to get hwlock %d\n", ret); > +} Given that we have no error handling path on the locks should we be supporting timeout mode at all? Otherwise we should probably add a set of error handling paths whenever we take the lock... > + if (config->hwlock_mode == HWLOCK_IRQSTATE) { > + map->lock = regmap_lock_hwlock_irqsave; > + map->unlock = regmap_unlock_hwlock_irqrestore; > + } else if (config->hwlock_mode == HWLOCK_IRQ) { > + map->lock = regmap_lock_hwlock_irq; > + map->unlock = regmap_unlock_hwlock_irq; > + } else { > + map->lock = regmap_lock_hwlock; > + map->unlock = regmap_unlock_hwlock; > + } This should be a switch statement. > --- a/include/linux/regmap.h > +++ b/include/linux/regmap.h > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include We don't actually use hwspinlock.h in the header (the config options are all just unsigned ints) so this could be moved to regmap.c with a forward declaration of the struct in internal.h. That way we don't force a rebuild of every regmap user when hwspinlock changes. --bzbmeuy2gxxofavz Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAln4UqUACgkQJNaLcl1U h9CeUQf/YeoV4A6zqSd7FaqiRzEHJSbxYtRUqXIGzgdpiW/LSLOA+DxMw443n8ud 2bwp49Y6EdWGFCyh1R+S1HnMGo5eqf0uKUlB8tf1MDwwjbtybIQas3mAMNYrRdE0 PAqwXpfEY+2fs9o/xUU2TPMs2oigH6/Cj1XPzqDIhC0rx+eM4zSNIZHf3MuqKADD +JdH3oUFnebHf0OOU1JQcBYXbGnrZnQSouyI/KHv002O411nmfDSjs/lLyDUqbA2 JcNXBAwIubRZDk8N4hKJfHDY6t+mVpU11u6Kdbs7ryj9oDyo0QQigOm+5U5Hn2NB dIYLpTQIyXGCrgXkksxnM35Q+Ylv1A== =iNAz -----END PGP SIGNATURE----- --bzbmeuy2gxxofavz--