lkml.org 
[lkml]   [2016]   [Jan]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2016-01-07 19:01    [W:2.340 / U:0.900 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site