Messages in this thread | | | Subject | Re: [PATCH v4 07/15] mtd: nand: move Samsung specific init/detection logic in nand_samsung.c | From | Marek Vasut <> | Date | Wed, 11 Jan 2017 14:02:36 +0100 |
| |
On 01/11/2017 08:57 AM, Boris Brezillon wrote: > On Tue, 10 Jan 2017 20:00:28 +0100 > Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 01/07/2017 08:49 AM, Boris Brezillon wrote: >>> On Sat, 7 Jan 2017 00:53:24 +0100 >>> Marek Vasut <marek.vasut@gmail.com> wrote: >>> >>>> On 01/04/2017 06:08 PM, Boris Brezillon wrote: >>>>> On Wed, 4 Jan 2017 16:14:07 +0100 >>>>> Marek Vasut <marek.vasut@gmail.com> 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 <boris.brezillon@free-electrons.com> >>>>>> >>>>>> [...] >>>>>> >>>>>>> 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 <linux/mtd/nand.h> >>>>>>> #include <linux/sizes.h> >>>>>>> >>>>>>> -#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_<vendor>.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
| |