Message-ID: <20160107171700.GE109450@google.com>
Date:	Thu, 7 Jan 2016 09:17:00 -0800
From:	Brian Norris <computersforpeace@...il.com>
To:	Boris Brezillon <boris.brezillon@...e-electrons.com>
Cc:	David Woodhouse <dwmw2@...radead.org>,
	linux-mtd@...ts.infradead.org,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	linux-kernel@...r.kernel.org
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@...il.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists