Messages in this thread | | | Date | Thu, 7 Jan 2016 09:17:00 -0800 | From | Brian Norris <> | Subject | Re: [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 <computersforpeace@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
| |