From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753802AbcB2L3M (ORCPT ); Mon, 29 Feb 2016 06:29:12 -0500 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:45788 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752321AbcB2L3K (ORCPT ); Mon, 29 Feb 2016 06:29:10 -0500 Date: Mon, 29 Feb 2016 20:28:57 +0900 From: Mark Brown To: Maarten ter Huurne Cc: Wenyou Yang , Zubair Lutfullah Kakakhel , Liam Girdwood , linux-kernel@vger.kernel.org Message-ID: <20160229112857.GY18327@sirena.org.uk> References: <1456674809-16422-1-git-send-email-maarten@treewalker.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="8ikq6d094XsNnsVx" Content-Disposition: inline In-Reply-To: <1456674809-16422-1-git-send-email-maarten@treewalker.org> X-Cookie: Adapt. Enjoy. Survive. User-Agent: Mutt/1.5.24 (2015-08-30) X-SA-Exim-Connect-IP: 122.212.32.58 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [PATCH 1/7] regulator: act8865: Expose act8600 registers via debugfs X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: Yes (on mezzanine.sirena.org.uk) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --8ikq6d094XsNnsVx Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sun, Feb 28, 2016 at 04:53:23PM +0100, Maarten ter Huurne wrote: > The read/write/volatile configuration is valid also when debugfs is > not enabled, but it doesn't add any value then. Please write changelogs that accurately describe what your changes do. Any debugfs changes are at best a second order side effect of changes here, what this appears to do is some combination of defining one or more new registers (which don't seem to ever be referred to...) and providing some access maps. > +#ifdef CONFIG_DEBUG_FS > + No, this is broken. The access information for a device is not affected by debugfs and does have an effect on how we work with the device. Having access maps that depend on random build settings like this is a recipie for hard to diagnose bugs. > +static const struct regmap_range act8600_reg_ranges[] = { > + { 0x00, 0x01 }, > + { 0x10, 0x10 }, { 0x12, 0x12 }, > + { 0x20, 0x20 }, { 0x22, 0x22 }, > + { 0x30, 0x30 }, { 0x32, 0x32 }, Your formatting here is just weird and confusing, randomly grouping things on lines with no obvious . > +}; > +static const struct regmap_range act8600_reg_ro_ranges[] = { Missing blank lines between variables. > +static const struct regmap_range act8600_reg_volatile_ranges[] = { > + { 0x00, 0x01 }, For ranges you should explicitly use .start and .end otherwise these look like they're intended to be register default tables. > @@ -421,6 +470,11 @@ static int act8865_pmic_probe(struct i2c_client *client, > struct device_node **of_node; > int i, ret, num_regulators; > struct act8865 *act8865; > + struct regmap_config regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = 0xFF, > + }; Why have you moved this from a global static variable where it normally is to a local variable? This is unusual and confusing... > +#ifdef CONFIG_DEBUG_FS > + regmap_config.wr_table = &act8600_write_ranges_table; > + regmap_config.rd_table = &act8600_read_ranges_table; > + regmap_config.volatile_table = &act8600_volatile_ranges_table; > +#endif ...and the fact that you're doing things like this ought to be a warning that there's a problem with the way you're handling the access maps. --8ikq6d094XsNnsVx Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJW1Ct4AAoJECTWi3JdVIfQfrAH/REXBk9kG9erIFUS5GFjXmqw NqSVStaTV70JB7Kw1YGAOefzGrA+dfGO+RRGXjSBGCSVNI7kFKNnjyMvw4i9a4JW ZbKdn4rRyseC1zX2xz73QNGUAk3OpcfMh8Vb7H5u1sJWHMWVXqWdGDUugFOacKas RgMeEAeXrNC3pagvWnWSUor+lm6E1X8nXbk9mlmUe8SLdsfXGvAEff8w8WVot2oZ LEyxdm8z138YL+LErwVhbSz6dvsPPkImjcplgHJFIQbckze4T07Eme4CaIaJBEUK s/yWJzDJQZjzvMrWuPirEaCJN7T9xOR4R+F0R3hkmjqNdz2q2NEo494wEvteRVg= =fYet -----END PGP SIGNATURE----- --8ikq6d094XsNnsVx--