Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1270178 > unrolled thread
| Started by | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| First post | 2015-11-16 15:00 +0100 |
| Last post | 2015-11-17 16:40 +0100 |
| Articles | 6 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH 00/27] mtd: nand: refactor the NAND subsystem (part 1) Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-11-16 15:00 +0100
Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip Brian Norris <computersforpeace@gmail.com> - 2015-11-17 04:10 +0100
Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-11-17 09:40 +0100
Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip Julia Lawall <julia.lawall@lip6.fr> - 2015-11-17 10:10 +0100
Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-11-17 15:30 +0100
Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip Julia Lawall <julia.lawall@lip6.fr> - 2015-11-17 16:40 +0100
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2015-11-16 15:00 +0100 |
| Subject | [PATCH 00/27] mtd: nand: refactor the NAND subsystem (part 1) |
| Message-ID | <qvsaK-7Us-5@gated-at.bofh.it> |
Hello,
This huge series aims at clarifying the relationship between the mtd and
nand_chip structures and hiding NAND framework internals to NAND
controller drivers.
The first part of the series provide an mtd_to_nand() helper to hide the
way mtd and nand_chip are linked together.
The second part of the series embeds the mtd structure into the nand_chip
one so that NAND controller drivers don't have to bother allocating the
MTD device and linking it with the NAND chip.
The last part of the series hides accesses to the chip->priv field behind
two helper functions.
This allows removal of some of the boilerplate code done in all NAND
controller drivers, but most importantly, it unifies a bit the way NAND
chip structures are instantiated (even though we still have two different
kinds of drivers: those embedding the nand_chip struct into their private
nand chip representation, and those allocating two different structures
and linking them together with the chip->priv field).
As said in the title, this refactoring is only the first step. I plan to
rework the NAND controller / NAND chip separation for pretty much the same
reasons: clarifying the separation between the two concepts, and getting
rid of more boilerplate code in NAND controller drivers.
Stay tuned ;-).
Best Regards,
Boris
Boris Brezillon (27):
mtd: nand: fix drivers abusing mtd->priv
mtd: nand: add an mtd_to_nand() helper
mtd: nand: update examples in the documentation to use mtd_to_nand()
ARM: nand: make use of mtd_to_nand() where appropriate
blackfin: nand: make use of mtd_to_nand() where appropriate
cris: nand: make use of mtd_to_nand() where appropriate
mips: nand: make use of mtd_to_nand() where appropriate
sh: nand: make use of mtd_to_nand() where appropriate
mtd: nand: make use of mtd_to_nand() in NAND core code
mtd: nand: make use of mtd_to_nand() in NAND drivers
staging: mt29f_spinand: make use of mtd_to_nand()
mtd: nand: embed an mtd_info structure into nand_chip
mtd: nand: add nand_to_mtd() helper
mtd: nand: use the mtd instance embedded in struct nand_chip
mtd: nand: update the documentation to reflect framework changes
staging: mt29f_spinand: use the mtd instance embedded in struct
nand_chip
cris: nand: use the mtd instance embedded in struct nand_chip
mtd: nand: update mtd_to_nand()
mtd: nand: remove useless mtd->priv = chip assignments
cris: nand: remove useless mtd->priv = chip assignments
staging: mt29f_spinand: remove useless mtd->priv = chip assignment
mtd: nand: simplify nand_dt_init() usage
mtd: nand: kill the chip->flash_node field
mtd: nand: add helpers to access ->priv
ARM: make use of nand_set/get_controller_data() helpers
mtd: nand: make use of nand_set/get_controller_data() helpers
staging: mt29f_spinand: make use of nand_set/get_controller_data()
helpers
Documentation/DocBook/mtdnand.tmpl | 35 +++---
arch/arm/mach-ep93xx/snappercl15.c | 4 +-
arch/arm/mach-ep93xx/ts72xx.c | 4 +-
arch/arm/mach-imx/mach-qong.c | 2 +-
arch/arm/mach-ixp4xx/ixdp425-setup.c | 6 +-
arch/arm/mach-omap1/board-nand.c | 2 +-
arch/arm/mach-orion5x/ts78xx-setup.c | 6 +-
arch/arm/mach-pxa/balloon3.c | 2 +-
arch/arm/mach-pxa/em-x270.c | 2 +-
arch/arm/mach-pxa/palmtx.c | 2 +-
arch/blackfin/mach-bf537/boards/stamp.c | 2 +-
arch/blackfin/mach-bf561/boards/acvilon.c | 2 +-
arch/cris/arch-v32/drivers/mach-a3/nandflash.c | 8 +-
arch/cris/arch-v32/drivers/mach-fs/nandflash.c | 8 +-
arch/mips/alchemy/devboards/db1200.c | 2 +-
arch/mips/alchemy/devboards/db1300.c | 2 +-
arch/mips/alchemy/devboards/db1550.c | 2 +-
arch/mips/pnx833x/common/platform.c | 2 +-
arch/mips/rb532/devices.c | 2 +-
arch/sh/boards/mach-migor/setup.c | 2 +-
drivers/mtd/nand/ams-delta.c | 26 ++---
drivers/mtd/nand/atmel_nand.c | 116 ++++++++++---------
drivers/mtd/nand/au1550nd.c | 40 +++----
drivers/mtd/nand/bcm47xxnflash/bcm47xxnflash.h | 1 -
drivers/mtd/nand/bcm47xxnflash/main.c | 9 +-
drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c | 34 +++---
drivers/mtd/nand/bf5xx_nand.c | 27 +++--
drivers/mtd/nand/brcmnand/brcmnand.c | 54 +++++----
drivers/mtd/nand/cafe_nand.c | 41 ++++---
drivers/mtd/nand/cmx270_nand.c | 20 ++--
drivers/mtd/nand/cs553x_nand.c | 30 +++--
drivers/mtd/nand/davinci_nand.c | 33 +++---
drivers/mtd/nand/denali.c | 61 +++++-----
drivers/mtd/nand/denali.h | 1 -
drivers/mtd/nand/diskonchip.c | 148 ++++++++++++-------------
drivers/mtd/nand/docg4.c | 93 ++++++++--------
drivers/mtd/nand/fsl_elbc_nand.c | 58 +++++-----
drivers/mtd/nand/fsl_ifc_nand.c | 67 ++++++-----
drivers/mtd/nand/fsl_upm.c | 30 ++---
drivers/mtd/nand/fsmc_nand.c | 72 ++++++------
drivers/mtd/nand/gpio.c | 16 +--
drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 2 +-
drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 67 ++++++-----
drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 1 -
drivers/mtd/nand/hisi504_nand.c | 48 ++++----
drivers/mtd/nand/jz4740_nand.c | 14 +--
drivers/mtd/nand/lpc32xx_mlc.c | 32 +++---
drivers/mtd/nand/lpc32xx_slc.c | 42 ++++---
drivers/mtd/nand/mpc5121_nfc.c | 52 +++++----
drivers/mtd/nand/mxc_nand.c | 76 +++++++------
drivers/mtd/nand/nand_base.c | 102 +++++++++--------
drivers/mtd/nand/nand_bbt.c | 32 +++---
drivers/mtd/nand/nand_bch.c | 4 +-
drivers/mtd/nand/nand_ecc.c | 4 +-
drivers/mtd/nand/nandsim.c | 34 +++---
drivers/mtd/nand/ndfc.c | 52 ++++-----
drivers/mtd/nand/nuc900_nand.c | 23 ++--
drivers/mtd/nand/omap2.c | 107 ++++++++++--------
drivers/mtd/nand/orion_nand.c | 13 +--
drivers/mtd/nand/pasemi_nand.c | 21 ++--
drivers/mtd/nand/plat_nand.c | 16 +--
drivers/mtd/nand/pxa3xx_nand.c | 66 ++++++-----
drivers/mtd/nand/r852.c | 41 +++----
drivers/mtd/nand/r852.h | 1 -
drivers/mtd/nand/s3c2410.c | 29 ++---
drivers/mtd/nand/sh_flctl.c | 9 +-
drivers/mtd/nand/sharpsl.c | 20 ++--
drivers/mtd/nand/sm_common.c | 2 +-
drivers/mtd/nand/socrates_nand.c | 24 ++--
drivers/mtd/nand/sunxi_nand.c | 40 +++----
drivers/mtd/nand/tmio_nand.c | 10 +-
drivers/mtd/nand/txx9ndfmc.c | 25 ++---
drivers/mtd/nand/vf610_nfc.c | 6 +-
drivers/mtd/nand/xway_nand.c | 4 +-
drivers/staging/mt29f_spinand/mt29f_spinand.c | 19 ++--
include/linux/mtd/nand.h | 29 ++++-
include/linux/mtd/sh_flctl.h | 3 +-
77 files changed, 1067 insertions(+), 1077 deletions(-)
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Brian Norris <computersforpeace@gmail.com> |
|---|---|
| Date | 2015-11-17 04:10 +0100 |
| Subject | Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip |
| Message-ID | <qvEOD-7G2-27@gated-at.bofh.it> |
| In reply to | #1270178 |
Hi Boris,
On Mon, Nov 16, 2015 at 02:37:47PM +0100, Boris Brezillon wrote:
> struct nand_chip now embeds an mtd device. Patch all drivers to make use
> of this mtd instance instead of using the instance embedded in their
> private struct or dynamically allocated.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
> ---
> Most of those changes were generate with this coccinelle script:
> http://code.bulix.org/5vxuih-89429
I appreciate that this patch is mostly autogenerated (a good thing for
preventing errors!), but there are some issues that I don't think play
out very well stylistically. Hopefully the cocci script can be improved
to handle some of this?
I'll try to point out a few snippets below.
Also, in case others are interested in reviewing your cocci script
directly, it might be better to paste it inline than to link to it.
Given the size of the patch, I don't think people would mind a few dozen
extra lines to show how it wsa generated. Or maybe stick some in the
cover letter too, if you end up reusing them in several patches.
> ---
> drivers/mtd/nand/ams-delta.c | 13 ++--
> drivers/mtd/nand/atmel_nand.c | 11 ++-
> drivers/mtd/nand/au1550nd.c | 18 ++---
> drivers/mtd/nand/bcm47xxnflash/bcm47xxnflash.h | 1 -
> drivers/mtd/nand/bcm47xxnflash/main.c | 7 +-
> drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c | 2 +-
> drivers/mtd/nand/bf5xx_nand.c | 14 ++--
> drivers/mtd/nand/brcmnand/brcmnand.c | 11 ++-
> drivers/mtd/nand/cafe_nand.c | 10 +--
> drivers/mtd/nand/cmx270_nand.c | 11 ++-
> drivers/mtd/nand/cs553x_nand.c | 13 ++--
> drivers/mtd/nand/davinci_nand.c | 25 +++----
> drivers/mtd/nand/denali.c | 61 +++++++++--------
> drivers/mtd/nand/denali.h | 1 -
> drivers/mtd/nand/diskonchip.c | 11 ++-
> drivers/mtd/nand/docg4.c | 18 +++--
> drivers/mtd/nand/fsl_elbc_nand.c | 22 +++---
> drivers/mtd/nand/fsl_ifc_nand.c | 23 +++----
> drivers/mtd/nand/fsl_upm.c | 26 +++----
> drivers/mtd/nand/fsmc_nand.c | 59 +++++++++-------
> drivers/mtd/nand/gpio.c | 16 ++---
> drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 2 +-
> drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 20 +++---
> drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 1 -
> drivers/mtd/nand/hisi504_nand.c | 11 ++-
> drivers/mtd/nand/jz4740_nand.c | 9 ++-
> drivers/mtd/nand/lpc32xx_mlc.c | 7 +-
> drivers/mtd/nand/lpc32xx_slc.c | 7 +-
> drivers/mtd/nand/mpc5121_nfc.c | 3 +-
> drivers/mtd/nand/mxc_nand.c | 5 +-
> drivers/mtd/nand/nandsim.c | 12 ++--
> drivers/mtd/nand/ndfc.c | 22 +++---
> drivers/mtd/nand/nuc900_nand.c | 21 +++---
> drivers/mtd/nand/omap2.c | 94 +++++++++++++++-----------
> drivers/mtd/nand/orion_nand.c | 4 +-
> drivers/mtd/nand/pasemi_nand.c | 14 ++--
> drivers/mtd/nand/plat_nand.c | 14 ++--
> drivers/mtd/nand/pxa3xx_nand.c | 33 ++++-----
^^ BTW, this file already has a few conflicts. Sorry :(
I'll try to keep any eye out for things like this once we're close to
being able to apply something like this, so I don't merge unnecessary
churn. But for now, I hope we can review this series, and it won't be
too much work to rebase/resend once the bigger things have been worked
out.
> drivers/mtd/nand/r852.c | 34 ++++------
> drivers/mtd/nand/r852.h | 1 -
> drivers/mtd/nand/s3c2410.c | 19 +++---
> drivers/mtd/nand/sh_flctl.c | 8 +--
> drivers/mtd/nand/sharpsl.c | 18 ++---
> drivers/mtd/nand/socrates_nand.c | 5 +-
> drivers/mtd/nand/sunxi_nand.c | 13 ++--
> drivers/mtd/nand/tmio_nand.c | 7 +-
> drivers/mtd/nand/txx9ndfmc.c | 3 +-
> drivers/mtd/nand/vf610_nfc.c | 5 +-
> include/linux/mtd/sh_flctl.h | 3 +-
> 49 files changed, 383 insertions(+), 385 deletions(-)
>
...
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index f8aac0a..51748b4 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
...
> @@ -318,7 +317,7 @@ static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int bank)
>
> if (bank) {
> /* Only for a 2k-page or lower flash, NFC can handle 2 banks */
> - if (host->mtd.writesize > 2048)
> + if (nand_to_mtd(&host->nand_chip)->writesize > 2048)
(This isn't the worst one, but it just happens to be one of the first.)
There are many cases where the typical style would be to declare a new
variable at the top of the function, where you perform the
macro/function-call to convert from one abstraction to another. Like
static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int bank)
{
struct mtd_info *mtd = nand_to_mtd(&hot->nand_chip);
...
and then use it later. Can that be done very easily?
> return -EINVAL;
> nfc_writel(host->nfc->hsmc_regs, BANK, ATMEL_HSMC_NFC_BANK1);
> } else {
...
> diff --git a/drivers/mtd/nand/au1550nd.c b/drivers/mtd/nand/au1550nd.c
> index 73fceb8..7e2a376 100644
> --- a/drivers/mtd/nand/au1550nd.c
> +++ b/drivers/mtd/nand/au1550nd.c
> @@ -23,7 +23,6 @@
>
>
> struct au1550nd_ctx {
> - struct mtd_info info;
> struct nand_chip chip;
>
> int cs;
> @@ -197,7 +196,8 @@ static void au_read_buf16(struct mtd_info *mtd, u_char *buf, int len)
>
> static void au1550_hwcontrol(struct mtd_info *mtd, int cmd)
> {
> - struct au1550nd_ctx *ctx = container_of(mtd, struct au1550nd_ctx, info);
> + struct au1550nd_ctx *ctx = container_of(mtd_to_nand(mtd),
> + struct au1550nd_ctx, chip);
> struct nand_chip *this = mtd_to_nand(mtd);
This is another good example. It's a little awkward to do this at all
(function call within a macro):
container_of(mtd_to_nand(mtd), ...);
but that's not unforgiveable. It's a bit worse, though, when followed by
assigning the next field to the same thing:
struct nand_chip *this = mtd_to_nand(mtd);
i.e., this would be nicer to see as:
struct nand_chip *this = mtd_to_nand(mtd);
struct au1550nd_ctx *ctx = container_of(this,
struct au1550nd_ctx, chip);
Again, I'm not sure how best to automate this kind of transformation.
>
> switch (cmd) {
> @@ -267,7 +267,8 @@ static void au1550_select_chip(struct mtd_info *mtd, int chip)
> */
> static void au1550_command(struct mtd_info *mtd, unsigned command, int column, int page_addr)
> {
> - struct au1550nd_ctx *ctx = container_of(mtd, struct au1550nd_ctx, info);
> + struct au1550nd_ctx *ctx = container_of(mtd_to_nand(mtd),
> + struct au1550nd_ctx, chip);
> struct nand_chip *this = mtd_to_nand(mtd);
Same here.
> int ce_override = 0, i;
> unsigned long flags = 0;
Snipped the rest, since it's pretty similar comments that apply.
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2015-11-17 09:40 +0100 |
| Subject | Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip |
| Message-ID | <qvJXY-2zA-17@gated-at.bofh.it> |
| In reply to | #1270783 |
Hi Brian,
On Mon, 16 Nov 2015 19:00:19 -0800
Brian Norris <computersforpeace@gmail.com> wrote:
> Hi Boris,
>
> On Mon, Nov 16, 2015 at 02:37:47PM +0100, Boris Brezillon wrote:
> > struct nand_chip now embeds an mtd device. Patch all drivers to make use
> > of this mtd instance instead of using the instance embedded in their
> > private struct or dynamically allocated.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Cc: Julia Lawall <Julia.Lawall@lip6.fr>
> > ---
> > Most of those changes were generate with this coccinelle script:
> > http://code.bulix.org/5vxuih-89429
>
> I appreciate that this patch is mostly autogenerated (a good thing for
> preventing errors!), but there are some issues that I don't think play
> out very well stylistically. Hopefully the cocci script can be improved
> to handle some of this?
>
> I'll try to point out a few snippets below.
>
> Also, in case others are interested in reviewing your cocci script
> directly, it might be better to paste it inline than to link to it.
> Given the size of the patch, I don't think people would mind a few dozen
> extra lines to show how it wsa generated. Or maybe stick some in the
> cover letter too, if you end up reusing them in several patches.
Sure, I'll paste the script directly in the commit message next time.
>
> > ---
> > drivers/mtd/nand/ams-delta.c | 13 ++--
> > drivers/mtd/nand/atmel_nand.c | 11 ++-
> > drivers/mtd/nand/au1550nd.c | 18 ++---
> > drivers/mtd/nand/bcm47xxnflash/bcm47xxnflash.h | 1 -
> > drivers/mtd/nand/bcm47xxnflash/main.c | 7 +-
> > drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c | 2 +-
> > drivers/mtd/nand/bf5xx_nand.c | 14 ++--
> > drivers/mtd/nand/brcmnand/brcmnand.c | 11 ++-
> > drivers/mtd/nand/cafe_nand.c | 10 +--
> > drivers/mtd/nand/cmx270_nand.c | 11 ++-
> > drivers/mtd/nand/cs553x_nand.c | 13 ++--
> > drivers/mtd/nand/davinci_nand.c | 25 +++----
> > drivers/mtd/nand/denali.c | 61 +++++++++--------
> > drivers/mtd/nand/denali.h | 1 -
> > drivers/mtd/nand/diskonchip.c | 11 ++-
> > drivers/mtd/nand/docg4.c | 18 +++--
> > drivers/mtd/nand/fsl_elbc_nand.c | 22 +++---
> > drivers/mtd/nand/fsl_ifc_nand.c | 23 +++----
> > drivers/mtd/nand/fsl_upm.c | 26 +++----
> > drivers/mtd/nand/fsmc_nand.c | 59 +++++++++-------
> > drivers/mtd/nand/gpio.c | 16 ++---
> > drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 2 +-
> > drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 20 +++---
> > drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 1 -
> > drivers/mtd/nand/hisi504_nand.c | 11 ++-
> > drivers/mtd/nand/jz4740_nand.c | 9 ++-
> > drivers/mtd/nand/lpc32xx_mlc.c | 7 +-
> > drivers/mtd/nand/lpc32xx_slc.c | 7 +-
> > drivers/mtd/nand/mpc5121_nfc.c | 3 +-
> > drivers/mtd/nand/mxc_nand.c | 5 +-
> > drivers/mtd/nand/nandsim.c | 12 ++--
> > drivers/mtd/nand/ndfc.c | 22 +++---
> > drivers/mtd/nand/nuc900_nand.c | 21 +++---
> > drivers/mtd/nand/omap2.c | 94 +++++++++++++++-----------
> > drivers/mtd/nand/orion_nand.c | 4 +-
> > drivers/mtd/nand/pasemi_nand.c | 14 ++--
> > drivers/mtd/nand/plat_nand.c | 14 ++--
> > drivers/mtd/nand/pxa3xx_nand.c | 33 ++++-----
>
> ^^ BTW, this file already has a few conflicts. Sorry :(
>
> I'll try to keep any eye out for things like this once we're close to
> being able to apply something like this, so I don't merge unnecessary
> churn. But for now, I hope we can review this series, and it won't be
> too much work to rebase/resend once the bigger things have been worked
> out.
No problem, resolving this conflict was pretty easy.
>
> > drivers/mtd/nand/r852.c | 34 ++++------
> > drivers/mtd/nand/r852.h | 1 -
> > drivers/mtd/nand/s3c2410.c | 19 +++---
> > drivers/mtd/nand/sh_flctl.c | 8 +--
> > drivers/mtd/nand/sharpsl.c | 18 ++---
> > drivers/mtd/nand/socrates_nand.c | 5 +-
> > drivers/mtd/nand/sunxi_nand.c | 13 ++--
> > drivers/mtd/nand/tmio_nand.c | 7 +-
> > drivers/mtd/nand/txx9ndfmc.c | 3 +-
> > drivers/mtd/nand/vf610_nfc.c | 5 +-
> > include/linux/mtd/sh_flctl.h | 3 +-
> > 49 files changed, 383 insertions(+), 385 deletions(-)
> >
>
> ...
>
> > diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> > index f8aac0a..51748b4 100644
> > --- a/drivers/mtd/nand/atmel_nand.c
> > +++ b/drivers/mtd/nand/atmel_nand.c
>
> ...
>
> > @@ -318,7 +317,7 @@ static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int bank)
> >
> > if (bank) {
> > /* Only for a 2k-page or lower flash, NFC can handle 2 banks */
> > - if (host->mtd.writesize > 2048)
> > + if (nand_to_mtd(&host->nand_chip)->writesize > 2048)
>
> (This isn't the worst one, but it just happens to be one of the first.)
> There are many cases where the typical style would be to declare a new
> variable at the top of the function, where you perform the
> macro/function-call to convert from one abstraction to another. Like
>
> static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int bank)
> {
> struct mtd_info *mtd = nand_to_mtd(&hot->nand_chip);
> ...
>
> and then use it later. Can that be done very easily?
>
> > return -EINVAL;
> > nfc_writel(host->nfc->hsmc_regs, BANK, ATMEL_HSMC_NFC_BANK1);
> > } else {
>
> ...
Honestly, I don't know how to do that with a coccinelle script, and it
will probably take me more time to find how to do it than addressing
those problems manually.
Julia, could you give us some hint?
>
> > diff --git a/drivers/mtd/nand/au1550nd.c b/drivers/mtd/nand/au1550nd.c
> > index 73fceb8..7e2a376 100644
> > --- a/drivers/mtd/nand/au1550nd.c
> > +++ b/drivers/mtd/nand/au1550nd.c
> > @@ -23,7 +23,6 @@
> >
> >
> > struct au1550nd_ctx {
> > - struct mtd_info info;
> > struct nand_chip chip;
> >
> > int cs;
> > @@ -197,7 +196,8 @@ static void au_read_buf16(struct mtd_info *mtd, u_char *buf, int len)
> >
> > static void au1550_hwcontrol(struct mtd_info *mtd, int cmd)
> > {
> > - struct au1550nd_ctx *ctx = container_of(mtd, struct au1550nd_ctx, info);
> > + struct au1550nd_ctx *ctx = container_of(mtd_to_nand(mtd),
> > + struct au1550nd_ctx, chip);
> > struct nand_chip *this = mtd_to_nand(mtd);
>
> This is another good example. It's a little awkward to do this at all
> (function call within a macro):
>
> container_of(mtd_to_nand(mtd), ...);
>
> but that's not unforgiveable. It's a bit worse, though, when followed by
> assigning the next field to the same thing:
>
> struct nand_chip *this = mtd_to_nand(mtd);
>
> i.e., this would be nicer to see as:
>
> struct nand_chip *this = mtd_to_nand(mtd);
> struct au1550nd_ctx *ctx = container_of(this,
> struct au1550nd_ctx, chip);
>
> Again, I'm not sure how best to automate this kind of transformation.
Yes, I noticed all those problems too, but as I said, I don't know how
to handle them using a coccinelle script.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2015-11-17 10:10 +0100 |
| Subject | Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip |
| Message-ID | <qvKr0-31p-29@gated-at.bofh.it> |
| In reply to | #1270957 |
> > (This isn't the worst one, but it just happens to be one of the first.)
> > There are many cases where the typical style would be to declare a new
> > variable at the top of the function, where you perform the
> > macro/function-call to convert from one abstraction to another. Like
> >
> > static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int bank)
> > {
> > struct mtd_info *mtd = nand_to_mtd(&hot->nand_chip);
> > ...
> >
> > and then use it later. Can that be done very easily?
> >
> > > return -EINVAL;
> > > nfc_writel(host->nfc->hsmc_regs, BANK, ATMEL_HSMC_NFC_BANK1);
> > > } else {
> >
> > ...
>
> Honestly, I don't know how to do that with a coccinelle script, and it
> will probably take me more time to find how to do it than addressing
> those problems manually.
>
> Julia, could you give us some hint?
Probably something like the following would be easiest. You can just run
it after your other transformations:
@r exists@
identifier f;
expression e;
@@
f(...) { <+... nand_to_mtd(e) ...+> }
@@
identifier r.f;
expression r.e;
@@
f(...) {
+ struct mtd_info *mtd = nand_to_mtd(e);
...
}
This won't work if there is more than one possible value of e. If that is
likely, then I could come up with something more complex. It also assumes
that you want to convert all such calls. If you only want to convert calls
that occur in a particular context, eg a field reference, then you could
enhance the pattern inside the <+... ...+> in the first rule.
julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2015-11-17 15:30 +0100 |
| Subject | Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip |
| Message-ID | <qvPqG-6b2-17@gated-at.bofh.it> |
| In reply to | #1270984 |
Hi Julia,
On Tue, 17 Nov 2015 10:05:03 +0100 (CET)
Julia Lawall <julia.lawall@lip6.fr> wrote:
> > > (This isn't the worst one, but it just happens to be one of the first.)
> > > There are many cases where the typical style would be to declare a new
> > > variable at the top of the function, where you perform the
> > > macro/function-call to convert from one abstraction to another. Like
> > >
> > > static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int bank)
> > > {
> > > struct mtd_info *mtd = nand_to_mtd(&hot->nand_chip);
> > > ...
> > >
> > > and then use it later. Can that be done very easily?
> > >
> > > > return -EINVAL;
> > > > nfc_writel(host->nfc->hsmc_regs, BANK, ATMEL_HSMC_NFC_BANK1);
> > > > } else {
> > >
> > > ...
> >
> > Honestly, I don't know how to do that with a coccinelle script, and it
> > will probably take me more time to find how to do it than addressing
> > those problems manually.
> >
> > Julia, could you give us some hint?
>
> Probably something like the following would be easiest. You can just run
> it after your other transformations:
>
> @r exists@
> identifier f;
> expression e;
> @@
>
> f(...) { <+... nand_to_mtd(e) ...+> }
>
> @@
> identifier r.f;
> expression r.e;
> @@
>
> f(...) {
> + struct mtd_info *mtd = nand_to_mtd(e);
> ...
> }
Thanks for the the suggestion...
>
> This won't work if there is more than one possible value of e. If that is
> likely, then I could come up with something more complex. It also assumes
> that you want to convert all such calls. If you only want to convert calls
> that occur in a particular context, eg a field reference, then you could
> enhance the pattern inside the <+... ...+> in the first rule.
... unfortunately, as you've guessed, it's a bit more complicated.
This mtd local variable is usually extracted from another local
variable (struct nand_chip *chip), so we have to declare
struct mtd_info *mtd = nand_to_mtd(e);
after
struct nand_chip *chip = expression;
But this is not the only particular case. Sometime the chip variable is
not assigned where it's declared (especially when it is dynamically
allocated), and sometime it does not exist at all (we just extract it
from a private struct: &priv->chip).
The mtd variable can also be declared in sub-code blocks (loop or
conditional statements).
And, as you stated, we might want to keep direct calls to nand_to_mtd()
if it's only called once in a function context.
I'm pretty sure we could describe all those cases with specific context
description, but I must admit that it takes less time for me to fix
those specific cases manually than figuring out how to describe them
correctly in a coccinelle script :).
This being said, I'd be happy to see how you would handle all these
different cases.
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2015-11-17 16:40 +0100 |
| Subject | Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip |
| Message-ID | <qvQwq-6RR-39@gated-at.bofh.it> |
| In reply to | #1271280 |
On Tue, 17 Nov 2015, Boris Brezillon wrote:
> Hi Julia,
>
> On Tue, 17 Nov 2015 10:05:03 +0100 (CET)
> Julia Lawall <julia.lawall@lip6.fr> wrote:
>
> > > > (This isn't the worst one, but it just happens to be one of the first.)
> > > > There are many cases where the typical style would be to declare a new
> > > > variable at the top of the function, where you perform the
> > > > macro/function-call to convert from one abstraction to another. Like
> > > >
> > > > static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int bank)
> > > > {
> > > > struct mtd_info *mtd = nand_to_mtd(&hot->nand_chip);
> > > > ...
> > > >
> > > > and then use it later. Can that be done very easily?
> > > >
> > > > > return -EINVAL;
> > > > > nfc_writel(host->nfc->hsmc_regs, BANK, ATMEL_HSMC_NFC_BANK1);
> > > > > } else {
> > > >
> > > > ...
> > >
> > > Honestly, I don't know how to do that with a coccinelle script, and it
> > > will probably take me more time to find how to do it than addressing
> > > those problems manually.
> > >
> > > Julia, could you give us some hint?
> >
> > Probably something like the following would be easiest. You can just run
> > it after your other transformations:
> >
> > @r exists@
> > identifier f;
> > expression e;
> > @@
> >
> > f(...) { <+... nand_to_mtd(e) ...+> }
> >
> > @@
> > identifier r.f;
> > expression r.e;
> > @@
> >
> > f(...) {
> > + struct mtd_info *mtd = nand_to_mtd(e);
> > ...
> > }
>
> Thanks for the the suggestion...
>
> >
> > This won't work if there is more than one possible value of e. If that is
> > likely, then I could come up with something more complex. It also assumes
> > that you want to convert all such calls. If you only want to convert calls
> > that occur in a particular context, eg a field reference, then you could
> > enhance the pattern inside the <+... ...+> in the first rule.
>
> ... unfortunately, as you've guessed, it's a bit more complicated.
> This mtd local variable is usually extracted from another local
> variable (struct nand_chip *chip), so we have to declare
>
> struct mtd_info *mtd = nand_to_mtd(e);
>
> after
>
> struct nand_chip *chip = expression;
>
> But this is not the only particular case. Sometime the chip variable is
> not assigned where it's declared (especially when it is dynamically
> allocated), and sometime it does not exist at all (we just extract it
> from a private struct: &priv->chip).
> The mtd variable can also be declared in sub-code blocks (loop or
> conditional statements).
> And, as you stated, we might want to keep direct calls to nand_to_mtd()
> if it's only called once in a function context.
>
> I'm pretty sure we could describe all those cases with specific context
> description, but I must admit that it takes less time for me to fix
> those specific cases manually than figuring out how to describe them
> correctly in a coccinelle script :).
>
> This being said, I'd be happy to see how you would handle all these
> different cases.
Maybe the following would be useful. It won't handle all the cases, but
maybe it will take care of a good number of the nontrivial ones.
julia
@r@
local idexpression x;
identifier fld,y;
@@
nand_to_mtd(&x->fld)->y
@exists@
type T;
local idexpression r.x;
identifier xx,r.y,r.fld;
@@
T xx;
+ struct mtd_info *mtd = nand_to_mtd(&x->fld);
...
nand_to_mtd(&x@xx->fld)->y
@@
local idexpression r.x;
identifier r.fld;
@@
struct mtd_info *mtd = nand_to_mtd(&x->fld);
<...
- nand_to_mtd(&x->fld)
+ mtd
...>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web