Re: [PATCH 1/3] spi: spi-mem: add automatic poll status functions

From: Mark Brown <broonie@kernel.org>
To: Pratyush Yadav <p.yadav@ti.com>
Cc: patrice.chotard@foss.st.com,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Boris Brezillon <boris.brezillon@collabora.com>,
	linux-mtd@lists.infradead.org,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	linux-spi@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, christophe.kerello@foss.st.com
Subject: Re: [PATCH 1/3] spi: spi-mem: add automatic poll status functions
Date: Mon, 26 Apr 2021 17:51:18 +0100	[thread overview]
Message-ID: <20210426165118.GH4590@sirena.org.uk> (raw)
In-Reply-To: <20210426162610.erpt5ubeddx7paeq@ti.com>

[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]

On Mon, Apr 26, 2021 at 09:56:12PM +0530, Pratyush Yadav wrote:
> On 26/04/21 04:39PM, patrice.chotard@foss.st.com wrote:

> > + * spi_mem_poll_status() - Poll memory device status
> > + * @mem: SPI memory device
> > + * @op: the memory operation to execute
> > + * @mask: status bitmask to ckeck
> > + * @match: status expected value

> Technically, (status & mask) expected value. Dunno if that is obvious 
> enough to not spell out explicitly.

Is it possible there's some situation where you're waiting for some bits
to clear as well?

> > +		ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout);

I'm not sure I like this name since it makes me think the driver is
going to poll when really it's offloaded to the hardware, but I can't
think of any better ideas either and it *is* what the hardware is going
to be doing so meh.

> I wonder if it is better to let spi-mem core take care of the timeout 
> part. On one hand it reduces code duplication on the driver side a 
> little bit. Plus it makes sure drivers don't mess anything up with bad 
> (or no) handling of the timeout. But on the other hand the interface 
> becomes a bit awkward since you'd have to pass a struct completion 
> around, and it isn't something particularly hard to get right either. 
> What do you think?

We already have the core handling other timeouts.  We don't pass around
completions but rather have an API function that the driver has to call
when the operation completes, a similar pattern might work here.  Part
of the thing with those APIs which I'm missing here is that this will
just return -EOPNOTSUPP if the driver can't do the delay in hardware, I
think it would be cleaner if this API were similar and the core dealt
with doing the delay/poll on the CPU.  That way the users don't need to
repeat the handling for the offload/non-offload cases.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]