From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Machek Subject: Re: [PATCH v7 3/3] gpio: pca953x: fix address calculation for pcal6524 Date: Wed, 6 Jun 2018 09:35:25 +0200 Message-ID: <20180606073525.GA18743@amd> References: <20180523140635.GB27215@amd> <20180605203941.GA28143@amd> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="4Ckj6UjgE2iN1+kY" Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: "H. Nikolaus Schaller" Cc: Andy Shevchenko , Kumar Gala , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Linus Walleij , Alexandre Courbot , devicetree , "open list:GPIO SUBSYSTEM" , Linux Kernel Mailing List , Discussions about the Letux Kernel , kernel@pyra-handheld.com List-Id: devicetree@vger.kernel.org --4Ckj6UjgE2iN1+kY Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed 2018-06-06 07:33:32, H. Nikolaus Schaller wrote: > Hi, >=20 > > Am 05.06.2018 um 22:39 schrieb Pavel Machek : > >=20 > > On Tue 2018-06-05 18:37:21, Andy Shevchenko wrote: > >> On Wed, May 23, 2018 at 5:06 PM, Pavel Machek wrote: > >>> On Thu 2018-05-17 06:59:49, H. Nikolaus Schaller wrote: > >>>> The register constants are so far defined in a way that they fit > >>>> for the pcal9555a when shifted by the number of banks, i.e. are > >>>> multiplied by 2 in the accessor function. > >>>>=20 > >>>> Now, the pcal6524 has 3 banks which means the relative offset > >>>> is multiplied by 4 for the standard registers. > >>>>=20 > >>>> Simply applying the bit shift to the extended registers gives > >>>> a wrong result, since the base offset is already included in > >>>> the offset. > >>>>=20 > >>>> Therefore, we have to add code to the 24 bit accessor functions > >>>> that adjusts the register number for these exended registers. > >>>>=20 > >>>> The formula finally used was developed and proposed by > >>>> Andy Shevchenko . > >>=20 > >>>> int bank_shift =3D fls((chip->gpio_chip.ngpio - 1) / BANK_SZ); > >>>> + int addr =3D (reg & PCAL_GPIO_MASK) << bank_shift; > >>>> + int pinctrl =3D (reg & PCAL_PINCTRL_MASK) << 1; > >>=20 > >>> Is this reasonable to do on each register access? Compiler will not be > >>> able to optimize out fls and shifts, right? > >>=20 > >> On modern CPUs fls() is one assembly command. OTOH, any proposal to do > >> this better? > >>=20 > >> What I can see is that bank_shift is invariant to the function, and > >> maybe cached. > >=20 > > Yes, I thought that caching bank_shift might be good idea. I thought > > it was constant for given chip... >=20 > Yes, it is an f(chip), but the question that comes to my mind is if > optimization is worth any effort. This is an accessor method over It will also be less ugly. Copy&pasted complex exprepsion all over the driver is not nice. Pavel --=20 (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blo= g.html --4Ckj6UjgE2iN1+kY Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iEYEARECAAYFAlsXjr0ACgkQMOfwapXb+vIc1wCggj6eiSaPQ5SgwApWF99NsdKs vqoAoMPTlFh+Gg+YVlfFIoV6TYdftu6l =1hdR -----END PGP SIGNATURE----- --4Ckj6UjgE2iN1+kY--