[PATCH v4 55/58] mtd: nand: add helpers to access ->priv

Hi Boris,

On Thu, Jan 07, 2016 at 03:52:37PM +0100, Boris Brezillon wrote:

On Wed, 6 Jan 2016 15:13:23 -0800
Brian Norris [off-list ref] wrote:

quoted

On Sat, Dec 19, 2015 at 04:01:24AM +0100, Boris Brezillon wrote:

quoted

quoted

Now, the reason I explicitly specified the data usage instead of using
a generic name like nand_{get,set}_data() is because I plan to define

I never suggested just "_data"; I said "_drvdata".

Not sure it clarifies the per-chip aspect ;), and driver is, IMHO, too
generic: I also consider manufacturer specific code as drivers (but in
this case they are chip drivers, not controller drivers).

How about nand_{get,set}_ctrldrvdata()?

That's just more confusing :)

quoted

quoted

other helpers to allow NAND manufacturer code to manipulate its own
private data. This is required if we want to support read-retry on some
chips who are requiring a read OTP area step to retrieve some register
values which will later be used to change from one read-retry mode to
another.
The plan was to define the nand_{set,get}_manufacturer_data() helpers,
and create or reuse an existing priv field (mtd->priv?) to store this
private data.

[...]

quoted

quoted

Also note that the spi framework provides the same kind of helpers [1].

Hmm, OK. FWIW, they have both "driver data" and "controller state". It's
not perfectly clear to me why both exist.

Well, "driver data" in this case is the data used by the i2c device
driver (the driver communicating with the device on the i2c bus), while
the "controller state" is the per-device controller specific data.
If we do the analogy with the NAND framework, I'd consider the
"SPI controller state" as what I call here the "NAND controller per-chip
data", and the "SPI driver data" as the "manufacturer data".

I know the names are not necessarily better in the SPI framework, but I
think we should find names that describe as much as possible which
data is used by which part of the code. 

Re-reviewing the SPI framework helpers suggests to me that their usage
scheme is actually not too bad. And it's actually pretty similar to your
current naming scheme.

quoted

quoted

This being said, I'm perfectly fine changing the function names, but
I'd like to replace it by something explicitly telling the user that
this field should only be set by NAND controller drivers. 

Sure. I though a "driver data"-based name did this. But I'll leave it to
you. I could even be OK with "controller data", if you still think this
fits your overall controller refactoring plan, and communicates its
purpose best.

If you're OK with that I'd like to keep a name containing the
'controller' or 'ctrl' word in it:
1/ nand_{get,set}_controller_data() (the names originally proposed)
2/ nand_{get,set}_ctrldrv_data() or nand_{get,set}_ctrldrvdata()

What do you prefer?

I guess (1) is not that bad in the end. I at least prefer it to (2).
Sorry for the bikeshedding!

So are these last few patches still applicable as-is, or should I await
a new submission?

Thanks,
Brian