Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 15 Sep 2022 11:46:13 -0400
From:   William Breathitt Gray <william.gray@linaro.org>
To:     Andy Shevchenko <andriy.shevchenko@intel.com>
Cc:     brgl@bgdev.pl, linus.walleij@linaro.org,
        linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
Subject: Re: [PATCH 1/3] gpio: idio-16: Introduce the ACCES IDIO-16 GPIO
 library module
Message-ID: <YyNIxWS0sneUKas0@fedora>
References: <cover.1662927941.git.william.gray@linaro.org>
 <6b28fb497c35def57c1920362c82402bed4bd23f.1662927941.git.william.gray@linaro.org>
 <YyCs1/HgOG31MX1N@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="Nzu/QuXFHOIMvVdg"
Content-Disposition: inline
In-Reply-To: <YyCs1/HgOG31MX1N@smile.fi.intel.com>
Precedence: bulk


--Nzu/QuXFHOIMvVdg
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Sep 13, 2022 at 07:16:23PM +0300, Andy Shevchenko wrote:
> On Sun, Sep 11, 2022 at 04:34:38PM -0400, William Breathitt Gray wrote:
> > +	if (*mask & GENMASK(7, 0))
> > +		bitmap_set_value8(bits, ioread8(&reg->out0_7), 0);
> > +	if (*mask & GENMASK(15, 8))
> > +		bitmap_set_value8(bits, ioread8(&reg->out8_15), 8);
> > +	if (*mask & GENMASK(23, 16))
> > +		bitmap_set_value8(bits, ioread8(&reg->in0_7), 16);
> > +	if (*mask & GENMASK(31, 24))
> > +		bitmap_set_value8(bits, ioread8(&reg->in8_15), 24);
>=20
> So, the addresses of the ports are not expected to be continuous?

No, unfortunately the IDIO-16 devices allocate the FET outputs to byte
offsets 0 and 4 while the isolated inputs are allocated to byte offsets
1 and 5. I don't know the design reason for the split but that's the
reason I'm reading these addresses by byte rather than by word.

> > +		return;
> > +
> > +	spin_lock_irqsave(&state->lock, flags);
>=20
> > +	if (value)
> > +		set_bit(offset, state->out_state);
> > +	else
> > +		clear_bit(offset, state->out_state);
>=20
> assign_bit()
>=20
> But I'm wondering why do you need the atomic bitops under the lock?

I don't think atomic bitops are necessary in this case because of the
lock as you pointedly out, but I felt using these made the intention of
the code clearer. Is there a non-atomic version of assign_bit(), or do
you recommend I use bitwise operations directly here instead?

> > +static inline int idio_16_get_direction(const unsigned long offset)
> > +{
> > +	return (offset < IDIO_16_NOUT) ? 0 : 1;
>=20
> 	return (offset >=3D IDIO_16_NOUT) ? 1 : 0;
>=20
> ?

I have no particular preference in this case, so I can switch this to
the >=3D version for consistency with the rest of the code.

Thanks,

William Breathitt Gray

--Nzu/QuXFHOIMvVdg
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQSNN83d4NIlKPjon7a1SFbKvhIjKwUCYyNIxQAKCRC1SFbKvhIj
K3YnAP95t6WB1f2/suNqh2Of/obYvWHLqiVLCQDkfzLB3jOgRwEA4eU+HB/vod5c
mxTR52mBZou143ZlB9l/Uw0zfY6nHQk=
=fJeF
-----END PGP SIGNATURE-----

--Nzu/QuXFHOIMvVdg--