On 01/07/2017 08:49 AM, Boris Brezillon wrote:
> On Sat, 7 Jan 2017 00:53:24 +0100
> Marek Vasut <marek.va...@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.va...@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.brezil...@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.
--
Best regards,
Marek Vasut
