From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758717AbdAKNIR (ORCPT ); Wed, 11 Jan 2017 08:08:17 -0500 Received: from mail-wj0-f193.google.com ([209.85.210.193]:33544 "EHLO mail-wj0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751802AbdAKNIP (ORCPT ); Wed, 11 Jan 2017 08:08:15 -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> <27e81612-a7c4-9265-a608-3d5cf1180973@gmail.com> <20170111085727.314de956@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: Date: Wed, 11 Jan 2017 14:02:36 +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: <20170111085727.314de956@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/11/2017 08:57 AM, Boris Brezillon wrote: > On Tue, 10 Jan 2017 20:00:28 +0100 > Marek Vasut wrote: > >> 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. >> > > If we plan to rework it this way, I'd like to keep the existing > approach (with the extern) to avoid changing the prototype of > nand_manufacturer once again when we rework the nand_manufacturer > registration logic. > > Also note that in v6 I'm keeping a pointer to the nand_manfucturer > object in nand_chip, so that if we ever need to print the manufacturer > name we don't have to search again in the NAND manufacturer table. > After this rework, I no longer store the manufacturer_ops directly in > nand_chip, and have to access them by doing > chip->manufacturer.desc->ops->xxx. > > Which means, with your solution, I'll have to do > > ops = nand_get_manufacturer_ops(chip->manufacturer.desc); > ops->xxx(); > > instead of > > chip->manufacturer.desc->ops->xxx(); > All right, I think we can live with this either way. -- Best regards, Marek Vasut