From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1765873AbdAJTBH (ORCPT ); Tue, 10 Jan 2017 14:01:07 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:35941 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752433AbdAJTAc (ORCPT ); Tue, 10 Jan 2017 14:00:32 -0500 Subject: Re: [PATCH v4 07/15] mtd: nand: move Samsung specific init/detection logic in nand_samsung.c To: Boris Brezillon References: <1483448495-31607-1-git-send-email-boris.brezillon@free-electrons.com> <1483448495-31607-8-git-send-email-boris.brezillon@free-electrons.com> <09da7377-1323-0794-fe61-34fd06e35404@gmail.com> <20170104180813.700823eb@bbrezillon> <4982cf88-ff7e-20c9-a158-e1e3826c0762@gmail.com> <20170107084948.1d5d699c@bbrezillon> Cc: Richard Weinberger , linux-mtd@lists.infradead.org, David Woodhouse , Brian Norris , Cyrille Pitchen , Icenowy Zheng , Valdis.Kletnieks@vt.edu, linux-kernel@vger.kernel.org From: Marek Vasut Message-ID: <27e81612-a7c4-9265-a608-3d5cf1180973@gmail.com> Date: Tue, 10 Jan 2017 20:00:28 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170107084948.1d5d699c@bbrezillon> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/07/2017 08:49 AM, Boris Brezillon wrote: > On Sat, 7 Jan 2017 00:53:24 +0100 > Marek Vasut wrote: > >> On 01/04/2017 06:08 PM, Boris Brezillon wrote: >>> On Wed, 4 Jan 2017 16:14:07 +0100 >>> Marek Vasut wrote: >>> >>>> On 01/03/2017 02:01 PM, Boris Brezillon wrote: >>>>> Move Samsung specific initialization and detection logic into >>>>> nand_samsung.c. This is part of the "separate vendor specific code from >>>>> core" cleanup process. >>>>> >>>>> Signed-off-by: Boris Brezillon >>>> >>>> [...] >>>> >>>>> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c >>>>> index b3a332f37e14..05e9366696c9 100644 >>>>> --- a/drivers/mtd/nand/nand_ids.c >>>>> +++ b/drivers/mtd/nand/nand_ids.c >>>>> @@ -10,7 +10,7 @@ >>>>> #include >>>>> #include >>>>> >>>>> -#define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS >>>>> +#define LP_OPTIONS 0 >>>>> #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16) >>>>> >>>>> #define SP_OPTIONS NAND_NEED_READRDY >>>>> @@ -169,10 +169,12 @@ struct nand_flash_dev nand_flash_ids[] = { >>>>> }; >>>>> >>>>> /* Manufacturer IDs */ >>>>> +extern const struct nand_manufacturer_ops samsung_nand_manuf_ops; >>>> >>>> Is the extern needed ? >>> >>> Yes, unless you have another solution. If you remove the extern keyword >>> you just redeclare samsung_nand_manuf_ops here, which is not what we >>> want. >> >> Maybe some accessor function can help ? >> > > You mean, in nand_ids.c > > const struct nand_manufacturer_ops *get_samsung_nand_mafuf_ops(); > > struct nand_manufacturers nand_manuf_ids[] = { > ... > {NAND_MFR_SAMSUNG, "Samsung", get_samsung_nand_mafuf_ops}, > ... > }; > > and then, in nand_samsung.c > > const struct nand_manufacturer_ops *get_samsung_nand_mafuf_ops() > { > return &samsung_nand_mafuf_ops; > } Yeah, something like that. > What's the point of this extra indirection? I mean, in both cases you > use a symbol that is not part of the same source file, so you'll have > to define this symbol (using a function prototype or an extern object > definition). > Is this all about fixing checkpatch warnings, or do you have a problem > with objects shared between different source files? The later, separating this with an accessor function feels a bit cleaner to me than using extern foo. > Now, I agree that the current approach is not ideal. A real improvement > would be to let the NAND manufacturer drivers (nand_.c) register > themselves to the core. Something similar to CLK_OF_DECLARE() or > IRQCHIP_DECLARE() for example. But that means creating a dedicated > section for the nand_manufs_id table, and it's a lot more invasive than > the current approach. Well this would be awesome, but this can also be done later. I presume you'll get to it eventually anyway, as soon as you'll be annoyed enough with the current ugly-ish implementation. -- Best regards, Marek Vasut