[PATCH v4 55/58] mtd: nand: add helpers to access ->priv
Brian Norris
computersforpeace at gmail.com
Thu Jan 7 09:17:00 PST 2016
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 <computersforpeace at gmail.com> wrote:
> > On Sat, Dec 19, 2015 at 04:01:24AM +0100, Boris Brezillon wrote:
> > > 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 :)
> >
> > > 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.
[...]
> > > 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.
> > > 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
More information about the linux-mtd
mailing list