Re: [PATCH v4 07/15] mtd: nand: move Samsung specific init/detection logic in nand_samsung.c

[Marek Vasut]

On 01/07/2017 08:49 AM, Boris Brezillon wrote:
> On Sat, 7 Jan 2017 00:53:24 +0100
> Marek Vasut <marek.vasut@xxxxxxxxx> wrote:
> 
>> On 01/04/2017 06:08 PM, Boris Brezillon wrote:
>>> On Wed, 4 Jan 2017 16:14:07 +0100
>>> Marek Vasut <marek.vasut@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>    
>>>>
>>>> [...]
>>>>  
>>>>> 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