On Wed, 7 Jun 2017 16:21:15 +0900 Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote: > Hi Boris, > > > 2017-06-07 16:02 GMT+09:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>: > > On Wed, 7 Jun 2017 12:09:31 +0900 > > Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote: > > > >> >> + > >> >> +static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip, > >> >> + struct denali_nand_info *denali) > >> >> +{ > >> >> + struct nand_ecc_caps caps; > >> >> + int ret; > >> >> + > >> >> + caps.stepinfos = denali->stepinfo; > >> >> + caps.nstepinfos = 1; > >> >> + caps.calc_ecc_bytes = denali_calc_ecc_bytes; > >> >> + caps.oob_reserve_bytes = denali->bbtskipbytes; > >> > > >> > If you get rid of this oob_reserve_bytes field, you can define caps as > >> > a static const and even directly store ecc_caps in denali_nand_info. > >> > >> To make caps static const, denali_calc_ecc_bytes must be exported > >> to be referenced from denali_dt/denali_pci. > >> I am reluctant to do it. > > > > You already duplicate other information in denali_dt.c and > > denali_pci.c, > > The ECC step-size and strength are tightly associated to each IP variant. > I see duplication between denali_dt and denali_pci, but it is just because > Intel and Altera happened to have the same parameters. It's still duplication. > > On the other hand, denali_calc_ecc_bytes() is common to all variants > because ECC algorithm is not customizable. Yes, I agree. > > > > so what prevents you from duplicating this one-line > > function? > > > > Also, denali core already exports 2 functions, > > They are entries for probe/remove. > > > I don't see the problem > > in exporting the common nand_ecc_caps object. Why are you reluctant to > > that? > > denali_calc_ecc_bytes() is independent of DT, PCI, or whatever. > I see less reason to expose it. I don't get that one. The fact that it's a generic implementation makes it a good match for something you want to have in the core and expose to DT/PCI implems. > > caps is only used on probing, so I used a local variable. > I do not think it is a big problem. > It is to me, because you'll be the only user of the API at first, and people tend to copy&paste code from other drivers. nand_ecc_caps is really something that should be const and attached to a specific IP revision. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html