On Wed, 27 Feb 2019 15:19:57 +0100
Miquel Raynal [off-list ref] wrote:

Hi Boris,

Boris Brezillon [off-list ref] wrote on Wed, 27 Feb
2019 15:06:33 +0100:

quoted

On Wed, 27 Feb 2019 14:56:07 +0100
Miquel Raynal [off-list ref] wrote:
  

quoted

Hi Boris,

Boris Brezillon [off-list ref] wrote on Mon, 25 Feb 2019
19:55:43 +0100:
    

quoted

On Thu, 21 Feb 2019 11:01:52 +0100
Miquel Raynal [off-list ref] wrote:
      

quoted

+
+/**
+ * struct nand_ecc_engine_ops - Generic ECC engine operations
+ *
+ * @init_ctx: given a desired user configuration for the pointed NAND device,
+ *            requests the ECC engine driver to setup a configuration with
+ *            values it supports.
+ * @cleanup_ctx: clean the context initialized by @init_ctx.
+ * @prepare_io_req: is called before reading/writing a page to prepare the I/O
+ *                  request to be performed with ECC correction.
+ * @finish_io_req: is called after reading/writing a page to terminate the I/O
+ *                 request and ensure proper ECC correction.
+ */
+struct nand_ecc_engine_ops {        

We might want to add a

	void (*put_engine)(struct nand_ecc_engine *engine);

here if we want the nanddev cleanup path to be generic.
This hook would be implemented by drivers where the ECC engine object is
refcounted (typically the case for HW ECC engines shared by the raw NAND
controller and the SPI controller).

Alternatively, you can just add one nand_put_xxx_ecc_engine() func per
engine class (SW, ondie and HW).      

Can't this be handled in the init/cleanup_ctx() path directly?    

You really have to get the reference before init_ctx() otherwise the
engine might disappear between your get() and init() call, and, to keep
things symmetric, I think it's best to handle the put() outside the
cleanup_ctx() path.
  

quoted

Furthermore if this is just a hook to do reference counting.    

Well, what this put() does depends on the class of engine. For SW and
on-die ECC it can be a NOOP (that's true only if you keep the approach
where you have a single instance shared by everyone for SW-based ECC
engines).
For HW-controller-side ECC engines, you'll have to call device_get() on
the parent device in your nand_get_hw_ecc_engine() function while you
hold the lock protecting the ECC engine list. And device_put() will be
called in nand_put_hw_ecc_engine().  

I see.

Then I prefer keeping the logic in the core, not in the engine driver
and propose a

        void nand_ecc_put_engine(struct nand_ecc_engine *engine)

which will do nothing for on-die/sw engines and drop the reference for
hw engines. I will also rename the "find_ecc_engine" to "get_engine" so
that the call to the "put" helper has more meaning.

Ack for most of it. One thing I'd like to clarify: it's probably better
to have a separate function called nand_ecc_put_hw_engine() which you'll
call from nand_ecc_put_engine() when you're dealing with an
HW ECC engine rather than calling put_device() directly from
nand_ecc_put_engine(). This way you keep the code for HW ECC engine
well isolated.

Same goes for the nand_ecc_get_engine() path, just delegate to
nand_ecc_get_hw_engine() when ->provider == HW_ECC.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel