From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751467AbdFGHpX (ORCPT ); Wed, 7 Jun 2017 03:45:23 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:36531 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751020AbdFGHpV (ORCPT ); Wed, 7 Jun 2017 03:45:21 -0400 Date: Wed, 7 Jun 2017 09:45:09 +0200 From: Boris Brezillon To: Masahiro Yamada Cc: Mark Rutland , devicetree@vger.kernel.org, Richard Weinberger , Marek Vasut , Artem Bityutskiy , Cyrille Pitchen , Linux Kernel Mailing List , Dinh Nguyen , Rob Herring , linux-mtd@lists.infradead.org, Masami Hiramatsu , Chuanxiao Dong , Jassi Brar , Brian Norris , Enrico Jorns , David Woodhouse , Graham Moore Subject: Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes Message-ID: <20170607094509.263f155d@bbrezillon> In-Reply-To: References: <1496704922-12261-1-git-send-email-yamada.masahiro@socionext.com> <1496704922-12261-5-git-send-email-yamada.masahiro@socionext.com> <20170607000133.69fbf82b@bbrezillon> <20170607090250.729625c9@bbrezillon> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 7 Jun 2017 16:21:15 +0900 Masahiro Yamada wrote: > Hi Boris, > > > 2017-06-07 16:02 GMT+09:00 Boris Brezillon : > > On Wed, 7 Jun 2017 12:09:31 +0900 > > Masahiro Yamada 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes Date: Wed, 7 Jun 2017 09:45:09 +0200 Message-ID: <20170607094509.263f155d@bbrezillon> References: <1496704922-12261-1-git-send-email-yamada.masahiro@socionext.com> <1496704922-12261-5-git-send-email-yamada.masahiro@socionext.com> <20170607000133.69fbf82b@bbrezillon> <20170607090250.729625c9@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Masahiro Yamada Cc: Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Richard Weinberger , Marek Vasut , Artem Bityutskiy , Cyrille Pitchen , Linux Kernel Mailing List , Dinh Nguyen , Rob Herring , linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Masami Hiramatsu , Chuanxiao Dong , Jassi Brar , Brian Norris , Enrico Jorns , David Woodhouse , Graham Moore List-Id: devicetree@vger.kernel.org On Wed, 7 Jun 2017 16:21:15 +0900 Masahiro Yamada wrote: > Hi Boris, > > > 2017-06-07 16:02 GMT+09:00 Boris Brezillon : > > On Wed, 7 Jun 2017 12:09:31 +0900 > > Masahiro Yamada 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html