From: Boris Brezillon <boris.brezillon@free-electrons.com>
To: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>,
linux-mtd@lists.infradead.org,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 55/58] mtd: nand: add helpers to access ->priv
Date: Sat, 19 Dec 2015 04:01:24 +0100 [thread overview]
Message-ID: <20151219040124.7ffe4274@bbrezillon> (raw)
In-Reply-To: <20151218221758.GQ10460@google.com>
Hi Brian,
On Fri, 18 Dec 2015 14:17:58 -0800
Brian Norris <computersforpeace@gmail.com> wrote:
> Hi Boris,
>
> On Thu, Dec 10, 2015 at 09:00:39AM +0100, Boris Brezillon wrote:
> > Add two helpers to access the field reserved for private controller data.
> > This makes it clearer what this field is reserved for and ease future
> > refactoring.
>
> I agree with the refactoring part, but I'm not sure about the name. Is
> it really "controller" data? That sounds like something that has 1
> instance per controller. But the way this is sometimes used, we get 1
> instance per NAND chip, and there may be more than one NAND chip per
> controller.
>
> So at the moment, this is more like opaque "driver data", like
> dev_{get,set}_drvdata(), which doesn't really have a prescribed use --
> it's up to the driver.
>
> Notably, we already have a (sort of) 1-per-controler-instance field:
> struct nand_hw_control (I notice we have both the 'controller' and
> 'hwcontrol' fields in nand_chip; that's pretty ugly too...).
Will be addressed soon ;-).
> Those don't
> have private data fields, but we could of course extend that if we
> really want "controller" data.
Actually the nand_{get,set}_controller_data() helpers are not about
assigning NAND controller private data (as you pointed those can
already be retrieved thanks to the ->controller field using the
container_of() trick), but per-chip private data instantiated by the
NAND controller and attached to a specific chip. For example, some
controllers pre-compute some register values or a clk rate to set when
a specific chip is selected. This is what per-chip controller data is
meant for.
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
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].
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.
[1]http://lxr.free-electrons.com/source/include/linux/spi/spi.h#L189
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
WARNING: multiple messages have this Message-ID (diff)
From: boris.brezillon@free-electrons.com (Boris Brezillon)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v4 55/58] mtd: nand: add helpers to access ->priv
Date: Sat, 19 Dec 2015 04:01:24 +0100 [thread overview]
Message-ID: <20151219040124.7ffe4274@bbrezillon> (raw)
In-Reply-To: <20151218221758.GQ10460@google.com>
Hi Brian,
On Fri, 18 Dec 2015 14:17:58 -0800
Brian Norris <computersforpeace@gmail.com> wrote:
> Hi Boris,
>
> On Thu, Dec 10, 2015 at 09:00:39AM +0100, Boris Brezillon wrote:
> > Add two helpers to access the field reserved for private controller data.
> > This makes it clearer what this field is reserved for and ease future
> > refactoring.
>
> I agree with the refactoring part, but I'm not sure about the name. Is
> it really "controller" data? That sounds like something that has 1
> instance per controller. But the way this is sometimes used, we get 1
> instance per NAND chip, and there may be more than one NAND chip per
> controller.
>
> So at the moment, this is more like opaque "driver data", like
> dev_{get,set}_drvdata(), which doesn't really have a prescribed use --
> it's up to the driver.
>
> Notably, we already have a (sort of) 1-per-controler-instance field:
> struct nand_hw_control (I notice we have both the 'controller' and
> 'hwcontrol' fields in nand_chip; that's pretty ugly too...).
Will be addressed soon ;-).
> Those don't
> have private data fields, but we could of course extend that if we
> really want "controller" data.
Actually the nand_{get,set}_controller_data() helpers are not about
assigning NAND controller private data (as you pointed those can
already be retrieved thanks to the ->controller field using the
container_of() trick), but per-chip private data instantiated by the
NAND controller and attached to a specific chip. For example, some
controllers pre-compute some register values or a clk rate to set when
a specific chip is selected. This is what per-chip controller data is
meant for.
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
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].
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.
[1]http://lxr.free-electrons.com/source/include/linux/spi/spi.h#L189
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
next prev parent reply other threads:[~2015-12-19 3:01 UTC|newest]
Thread overview: 243+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-10 7:59 [PATCH v4 00/58] mtd: nand: refactor the NAND subsystem (part 1) Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove() Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-11 0:40 ` Brian Norris
2015-12-11 0:40 ` Brian Norris
2015-12-11 0:40 ` Brian Norris
2015-12-11 13:53 ` Boris Brezillon
2015-12-11 13:53 ` Boris Brezillon
2015-12-11 13:53 ` Boris Brezillon
2015-12-11 14:39 ` Dan Carpenter
2015-12-11 14:39 ` Dan Carpenter
2015-12-11 14:39 ` Dan Carpenter
2015-12-11 15:15 ` Boris Brezillon
2015-12-11 15:15 ` Boris Brezillon
2015-12-11 15:15 ` Boris Brezillon
2015-12-11 22:03 ` Boris Brezillon
2015-12-11 22:03 ` Boris Brezillon
2015-12-11 22:03 ` Boris Brezillon
2015-12-11 22:11 ` Brian Norris
2015-12-11 22:11 ` Brian Norris
2015-12-11 22:11 ` Brian Norris
2015-12-11 14:02 ` [PATCH v5 " Boris Brezillon
2015-12-11 14:02 ` Boris Brezillon
2015-12-11 14:02 ` Boris Brezillon
2015-12-11 15:10 ` Boris Brezillon
2015-12-11 15:10 ` Boris Brezillon
2015-12-11 15:10 ` Boris Brezillon
2015-12-11 16:50 ` Dinh Nguyen
2015-12-11 16:50 ` Dinh Nguyen
2015-12-11 16:50 ` Dinh Nguyen
2015-12-11 16:50 ` Dinh Nguyen
2015-12-11 17:08 ` Boris Brezillon
2015-12-11 17:08 ` Boris Brezillon
2015-12-11 17:08 ` Boris Brezillon
2015-12-11 17:08 ` Boris Brezillon
2015-12-11 17:21 ` Dinh Nguyen
2015-12-11 17:21 ` Dinh Nguyen
2015-12-11 17:21 ` Dinh Nguyen
2015-12-11 17:21 ` Dinh Nguyen
2015-12-10 7:59 ` [PATCH v4 02/58] mtd: nand: fsmc: create and use mtd_to_fsmc() Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 03/58] mtd: nand: nuc900: create and use mtd_to_nuc900() Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 04/58] mtd: nand: omap2: create and use mtd_to_omap() Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 05/58] mtd: nand: ams-delta: use the mtd instance embedded in struct nand_chip Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 06/58] mtd: nand: atmel: " Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 07/58] mtd: nand: au1550nd: " Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 08/58] mtd: nand: bcm47xx: " Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 09/58] mtd: nand: bf5xx: " Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 10/58] mtd: nand: brcm: " Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 11/58] mtd: nand: cafe: " Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 12/58] mtd: nand: cmx270: " Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 13/58] mtd: nand: cs553x: " Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 14/58] mtd: nand: davinci: " Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` [PATCH v4 15/58] mtd: nand: denali: " Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-10 7:59 ` Boris Brezillon
2015-12-11 14:06 ` [PATCH v5 " Boris Brezillon
2015-12-11 14:06 ` Boris Brezillon
2015-12-11 14:06 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 16/58] mtd: nand: diskonchip: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 17/58] mtd: nand: docg4: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 18/58] mtd: nand: fsl_elbc: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 19/58] mtd: nand: fsl_ifc: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 20/58] mtd: nand: fsl_upm: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 21/58] mtd: nand: fsmc: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 22/58] mtd: nand: gpio: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 23/58] mtd: nand: gpmi: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 24/58] mtd: nand: hisi504: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 25/58] mtd: nand: jz4740: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 26/58] mtd: nand: lpc32xx: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 27/58] mtd: nand: mpc5121: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 28/58] mtd: nand: mxc: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 29/58] mtd: nand: nandsim: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 30/58] mtd: nand: ndfc: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 31/58] mtd: nand: nuc900: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 32/58] mtd: nand: omap2: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 33/58] mtd: nand: orion: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 34/58] mtd: nand: pasemi: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 35/58] mtd: nand: plat: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 36/58] mtd: nand: pxa3xx: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 37/58] mtd: nand: r852: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 38/58] mtd: nand: s3c2410: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-11 2:38 ` Krzysztof Kozlowski
2015-12-11 2:38 ` Krzysztof Kozlowski
2015-12-11 2:38 ` Krzysztof Kozlowski
2015-12-10 8:00 ` [PATCH v4 39/58] mtd: nand: sh_flctl: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 40/58] mtd: nand: sharpsl: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 41/58] mtd: nand: socrates: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-11 14:04 ` [PATCH v5 " Boris Brezillon
2015-12-11 14:04 ` Boris Brezillon
2015-12-11 14:04 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 42/58] mtd: nand: sunxi: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 43/58] mtd: nand: tmio: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 44/58] mtd: nand: txx9ndfmc: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 45/58] mtd: nand: vf610: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 46/58] mtd: nand: update the documentation to reflect framework changes Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 47/58] staging: mt29f_spinand: use the mtd instance embedded in struct nand_chip Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 48/58] cris: nand: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 49/58] mtd: nand: update mtd_to_nand() Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 50/58] mtd: nand: remove useless mtd->priv = chip assignments Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 51/58] cris: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 52/58] staging: mt29f_spinand: remove useless mtd->priv = chip assignment Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 53/58] mtd: nand: simplify nand_dt_init() usage Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 54/58] mtd: nand: kill the chip->flash_node field Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 55/58] mtd: nand: add helpers to access ->priv Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-18 22:17 ` Brian Norris
2015-12-18 22:17 ` Brian Norris
2015-12-18 22:17 ` Brian Norris
2015-12-19 3:01 ` Boris Brezillon [this message]
2015-12-19 3:01 ` Boris Brezillon
2015-12-19 3:01 ` Boris Brezillon
2016-01-06 23:13 ` Brian Norris
2016-01-06 23:13 ` Brian Norris
2016-01-06 23:13 ` Brian Norris
2016-01-07 14:52 ` Boris Brezillon
2016-01-07 14:52 ` Boris Brezillon
2016-01-07 14:52 ` Boris Brezillon
2016-01-07 17:17 ` Brian Norris
2016-01-07 17:17 ` Brian Norris
2016-01-07 17:17 ` Brian Norris
[not found] ` <20160107182140.02111dc1@bbrezillon>
2016-01-07 18:27 ` Brian Norris
2016-01-07 18:27 ` Brian Norris
2015-12-10 8:00 ` [PATCH v4 56/58] ARM: make use of nand_set/get_controller_data() helpers Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 57/58] mtd: nand: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` [PATCH v4 58/58] staging: mt29f_spinand: " Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-10 8:00 ` Boris Brezillon
2015-12-18 22:03 ` [PATCH v4 00/58] mtd: nand: refactor the NAND subsystem (part 1) Brian Norris
2015-12-18 22:03 ` Brian Norris
2015-12-18 22:03 ` Brian Norris
2015-12-18 22:03 ` Brian Norris
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151219040124.7ffe4274@bbrezillon \
--to=boris.brezillon@free-electrons.com \
--cc=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.