Date:   Tue, 7 Mar 2023 21:29:34 -0500
From:   William Breathitt Gray <william.gray@linaro.org>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     linus.walleij@linaro.org, brgl@bgdev.pl,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        broonie@kernel.org, Arnaud de Turckheim <quarium@gmail.com>,
        John Hentges <jhentges@accesio.com>,
        Jay Dolan <jay.dolan@accesio.com>
Subject: Re: [PATCH v4 2/3] gpio: pcie-idio-24: Migrate to the regmap API
Message-ID: <ZAfzDig+UzgH3QqO@fedora>
References: <cover.1678106722.git.william.gray@linaro.org>
 <b96429c66e7caca05d9fb93805e11650fdbad312.1678106722.git.william.gray@linaro.org>
 <ZAX3ntGgO4PjIkxx@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="quWG8+aBeWgbNXre"
Content-Disposition: inline
In-Reply-To: <ZAX3ntGgO4PjIkxx@smile.fi.intel.com>
Precedence: bulk


--quWG8+aBeWgbNXre
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Mar 06, 2023 at 04:24:30PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 06, 2023 at 07:59:52AM -0500, William Breathitt Gray wrote:
> > The regmap API supports IO port accessors so we can take advantage of
> > regmap abstractions rather than handling access to the device registers
> > directly in the driver.
> >=20
> > For the PCIe-IDIO-24 series of devices, the following BARs are
> > available:
> >=20
> >     BAR[0]: memory mapped PEX8311
> >     BAR[1]: I/O mapped PEX8311
> >     BAR[2]: I/O mapped card registers
> >=20
> > There are 24 FET Output lines, 24 Isolated Input lines, and 8 TTL/CMOS
> > lines (which may be configured for either output or input). The GPIO
> > lines are exposed by the following card registers:
> >=20
> >     Base +0x0-0x2 (Read/Write): FET Outputs
> >     Base +0xB (Read/Write): TTL/CMOS
> >     Base +0x4-0x6 (Read): Isolated Inputs
> >     Base +0x7 (Read): TTL/CMOS
> >=20
> > In order for the device to support interrupts, the PLX PEX8311 internal
> > PCI wire interrupt and local interrupt input must first be enabled.
> >=20
> > The following card registers for Change-Of-State may be used:
> >=20
> >     Base +0x8-0xA (Read): COS Status Inputs
> >     Base +0x8-0xA (Write): COS Clear Inputs
> >     Base +0xB (Read): COS Status TTL/CMOS
> >     Base +0xB (Write): COS Clear TTL/CMOS
> >     Base +0xE (Read/Write): COS Enable
> >=20
> > The COS Enable register is used to enable/disable interrupts and
> > configure the interrupt levels; each bit maps to a group of eight inputs
> > as described below:
> >=20
> >     Bit 0: IRQ EN Rising Edge IN0-7
> >     Bit 1: IRQ EN Rising Edge IN8-15
> >     Bit 2: IRQ EN Rising Edge IN16-23
> >     Bit 3: IRQ EN Rising Edge TTL0-7
> >     Bit 4: IRQ EN Falling Edge IN0-7
> >     Bit 5: IRQ EN Falling Edge IN8-15
> >     Bit 6: IRQ EN Falling Edge IN16-23
> >     Bit 7: IRQ EN Falling Edge TTL0-7
> >=20
> > An interrupt is asserted when a change-of-state matching the interrupt
> > level configuration respective for a particular group of eight inputs
> > with enabled COS is detected.
> >=20
> > The COS Status registers may be read to determine which inputs have
> > changed; if interrupts were enabled, an IRQ will be generated for the
> > set bits in these registers. Writing the value read from the COS Status
> > register back to the respective COS Clear register will clear just those
> > interrupts.
>=20
> ...
>=20
> >  - Add mutex to prevent clobbering the COS_ENABLE register when masking
> >    IRQ and setting their type configuration
>=20
> But this doesn't explain killing the raw spin lock.
>=20
> I don't understand how is it suppose to work then.
>=20
> What is this lock for and how IRQ related registers can be updated
> with RT type of kernel?
>=20
> If mutex is okay, doesn't mean we have to add 'can_sleep =3D true'?

I admit that RT locking is an area I often have trouble understanding,
so perhaps you can help me make sure whether this lock type change makes
sense here.

My reasoning for why we need a lock is that the COS Enable register is
modified in both the idio_24_set_type_config() function and the
idio_24_handle_mask_sync() function. These respectively are called by
regmap_irq_set_type() and regmap_irq_sync_unlock(), which are callbacks
for a struct irq_chip irq_set_type() and irq_bus_sync_unlock() members.

I think these callbacks could be executed in parrallel, but are allowed
to sleep, so that is why I selected mutex as the lock type here (I
probably do need to add 'can_sleep =3D true'). Is this reasoning correct,
or have I made a mistake here?

William Breathitt Gray

--quWG8+aBeWgbNXre
Content-Type: application/pgp-signature; name="signature.asc"

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

iHUEARYKAB0WIQSNN83d4NIlKPjon7a1SFbKvhIjKwUCZAfzDgAKCRC1SFbKvhIj
K6X6AQCb8V8G87zN4J8+wxzOx7owtXfV4gShJQXpdJzhhHNcNAD9ECtLyove5EKa
NApOWn2NjlkbJxgx/t7eGKvFk5ViEgo=
=vvBh
-----END PGP SIGNATURE-----

--quWG8+aBeWgbNXre--