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

2017-01-11 Thread Marek Vasut 
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 _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

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

2017-01-11 Thread Marek Vasut 
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 _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

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

2017-01-11 Thread Boris Brezillon 
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 _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();

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

2017-01-11 Thread Boris Brezillon 
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 _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();

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

2017-01-10 Thread Marek Vasut 
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 _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

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

2017-01-10 Thread Marek Vasut 
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 _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

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

2017-01-06 Thread Boris Brezillon 
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 _nand_mafuf_ops;
}

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?

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.

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

2017-01-06 Thread Boris Brezillon 
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 _nand_mafuf_ops;
}

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?

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.

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

2017-01-06 Thread Marek Vasut 
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 ?

-- 
Best regards,
Marek Vasut

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

2017-01-06 Thread Marek Vasut 
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 ?

-- 
Best regards,
Marek Vasut

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

2017-01-04 Thread Marek Vasut 
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 ?

>  struct nand_manufacturers nand_manuf_ids[] = {
>   {NAND_MFR_TOSHIBA, "Toshiba"},
>   {NAND_MFR_ESMT, "ESMT"},
> - {NAND_MFR_SAMSUNG, "Samsung"},
> + {NAND_MFR_SAMSUNG, "Samsung", _nand_manuf_ops},
>   {NAND_MFR_FUJITSU, "Fujitsu"},
>   {NAND_MFR_NATIONAL, "National"},
>   {NAND_MFR_RENESAS, "Renesas"},

[...]


-- 
Best regards,
Marek Vasut

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

2017-01-04 Thread Marek Vasut 
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 ?

>  struct nand_manufacturers nand_manuf_ids[] = {
>   {NAND_MFR_TOSHIBA, "Toshiba"},
>   {NAND_MFR_ESMT, "ESMT"},
> - {NAND_MFR_SAMSUNG, "Samsung"},
> + {NAND_MFR_SAMSUNG, "Samsung", _nand_manuf_ops},
>   {NAND_MFR_FUJITSU, "Fujitsu"},
>   {NAND_MFR_NATIONAL, "National"},
>   {NAND_MFR_RENESAS, "Renesas"},

[...]


-- 
Best regards,
Marek Vasut

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

2017-01-04 Thread Boris Brezillon 
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.

> 
> >  struct nand_manufacturers nand_manuf_ids[] = {
> > {NAND_MFR_TOSHIBA, "Toshiba"},
> > {NAND_MFR_ESMT, "ESMT"},
> > -   {NAND_MFR_SAMSUNG, "Samsung"},
> > +   {NAND_MFR_SAMSUNG, "Samsung", _nand_manuf_ops},
> > {NAND_MFR_FUJITSU, "Fujitsu"},
> > {NAND_MFR_NATIONAL, "National"},
> > {NAND_MFR_RENESAS, "Renesas"},  
> 
> [...]
> 
>

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

2017-01-04 Thread Boris Brezillon 
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.

> 
> >  struct nand_manufacturers nand_manuf_ids[] = {
> > {NAND_MFR_TOSHIBA, "Toshiba"},
> > {NAND_MFR_ESMT, "ESMT"},
> > -   {NAND_MFR_SAMSUNG, "Samsung"},
> > +   {NAND_MFR_SAMSUNG, "Samsung", _nand_manuf_ops},
> > {NAND_MFR_FUJITSU, "Fujitsu"},
> > {NAND_MFR_NATIONAL, "National"},
> > {NAND_MFR_RENESAS, "Renesas"},  
> 
> [...]
> 
>

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

2017-01-03 Thread Boris Brezillon 
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 
---
 drivers/mtd/nand/Makefile   |  1 +
 drivers/mtd/nand/nand_base.c| 52 ++--
 drivers/mtd/nand/nand_ids.c |  6 ++-
 drivers/mtd/nand/nand_samsung.c | 89 +
 4 files changed, 98 insertions(+), 50 deletions(-)
 create mode 100644 drivers/mtd/nand/nand_samsung.c

diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index bfd5d12b9ade..d4b90b0f879e 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -61,3 +61,4 @@ obj-$(CONFIG_MTD_NAND_QCOM)   += qcom_nandc.o
 obj-$(CONFIG_MTD_NAND_MTK) += mtk_nand.o mtk_ecc.o
 
 nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
+nand-objs += nand_samsung.o
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 8f80faa57984..0f0fa13e9448 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3792,48 +3792,13 @@ void nand_decode_ext_id(struct nand_chip *chip)
/*
 * Field definitions are in the following datasheets:
 * Old style (4,5 byte ID): Samsung K9GAG08U0M (p.32)
-* New Samsung (6 byte ID): Samsung K9GAG08U0F (p.44)
 * Hynix MLC   (6 byte ID): Hynix H27UBG8T2B (p.22)
 *
 * Check for ID length, non-zero 6th byte, cell type, and Hynix/Samsung
 * ID to decide what to do.
 */
-   if (id_len == 6 && id_data[0] == NAND_MFR_SAMSUNG &&
-   !nand_is_slc(chip) && id_data[5] != 0x00) {
-   /* Calc pagesize */
-   mtd->writesize = 2048 << (extid & 0x03);
-   extid >>= 2;
-   /* Calc oobsize */
-   switch (((extid >> 2) & 0x04) | (extid & 0x03)) {
-   case 1:
-   mtd->oobsize = 128;
-   break;
-   case 2:
-   mtd->oobsize = 218;
-   break;
-   case 3:
-   mtd->oobsize = 400;
-   break;
-   case 4:
-   mtd->oobsize = 436;
-   break;
-   case 5:
-   mtd->oobsize = 512;
-   break;
-   case 6:
-   mtd->oobsize = 640;
-   break;
-   case 7:
-   default: /* Other cases are "reserved" (unknown) */
-   mtd->oobsize = 1024;
-   break;
-   }
-   extid >>= 2;
-   /* Calc blocksize */
-   mtd->erasesize = (128 * 1024) <<
-   (((extid >> 1) & 0x04) | (extid & 0x03));
-   } else if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
-   !nand_is_slc(chip)) {
+   if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
+   !nand_is_slc(chip)) {
unsigned int tmp;
 
/* Calc pagesize */
@@ -3961,13 +3926,10 @@ static void nand_decode_bbm_options(struct nand_chip 
*chip)
 * Micron devices with 2KiB pages and on SLC Samsung, Hynix, Toshiba,
 * AMD/Spansion, and Macronix.  All others scan only the first page.
 */
-   if (!nand_is_slc(chip) &&
-   (maf_id == NAND_MFR_SAMSUNG ||
-maf_id == NAND_MFR_HYNIX))
+   if (!nand_is_slc(chip) && maf_id == NAND_MFR_HYNIX)
chip->bbt_options |= NAND_BBT_SCANLASTPAGE;
else if ((nand_is_slc(chip) &&
-   (maf_id == NAND_MFR_SAMSUNG ||
-maf_id == NAND_MFR_HYNIX ||
+   (maf_id == NAND_MFR_HYNIX ||
 maf_id == NAND_MFR_TOSHIBA ||
 maf_id == NAND_MFR_AMD ||
 maf_id == NAND_MFR_MACRONIX)) ||
@@ -4149,12 +4111,6 @@ static int nand_detect(struct nand_chip *chip, struct 
nand_flash_dev *type)
/* Get chip options */
chip->options |= type->options;
 
-   /*
-* Check if chip is not a Samsung device. Do not clear the
-* options for chips which do not have an extended id.
-*/
-   if (maf_id != NAND_MFR_SAMSUNG && !type->pagesize)
-   chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
 ident_done:
 
if (chip->options & NAND_BUSWIDTH_AUTO) {
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

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

2017-01-03 Thread Boris Brezillon 
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 
---
 drivers/mtd/nand/Makefile   |  1 +
 drivers/mtd/nand/nand_base.c| 52 ++--
 drivers/mtd/nand/nand_ids.c |  6 ++-
 drivers/mtd/nand/nand_samsung.c | 89 +
 4 files changed, 98 insertions(+), 50 deletions(-)
 create mode 100644 drivers/mtd/nand/nand_samsung.c

diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index bfd5d12b9ade..d4b90b0f879e 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -61,3 +61,4 @@ obj-$(CONFIG_MTD_NAND_QCOM)   += qcom_nandc.o
 obj-$(CONFIG_MTD_NAND_MTK) += mtk_nand.o mtk_ecc.o
 
 nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
+nand-objs += nand_samsung.o
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 8f80faa57984..0f0fa13e9448 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3792,48 +3792,13 @@ void nand_decode_ext_id(struct nand_chip *chip)
/*
 * Field definitions are in the following datasheets:
 * Old style (4,5 byte ID): Samsung K9GAG08U0M (p.32)
-* New Samsung (6 byte ID): Samsung K9GAG08U0F (p.44)
 * Hynix MLC   (6 byte ID): Hynix H27UBG8T2B (p.22)
 *
 * Check for ID length, non-zero 6th byte, cell type, and Hynix/Samsung
 * ID to decide what to do.
 */
-   if (id_len == 6 && id_data[0] == NAND_MFR_SAMSUNG &&
-   !nand_is_slc(chip) && id_data[5] != 0x00) {
-   /* Calc pagesize */
-   mtd->writesize = 2048 << (extid & 0x03);
-   extid >>= 2;
-   /* Calc oobsize */
-   switch (((extid >> 2) & 0x04) | (extid & 0x03)) {
-   case 1:
-   mtd->oobsize = 128;
-   break;
-   case 2:
-   mtd->oobsize = 218;
-   break;
-   case 3:
-   mtd->oobsize = 400;
-   break;
-   case 4:
-   mtd->oobsize = 436;
-   break;
-   case 5:
-   mtd->oobsize = 512;
-   break;
-   case 6:
-   mtd->oobsize = 640;
-   break;
-   case 7:
-   default: /* Other cases are "reserved" (unknown) */
-   mtd->oobsize = 1024;
-   break;
-   }
-   extid >>= 2;
-   /* Calc blocksize */
-   mtd->erasesize = (128 * 1024) <<
-   (((extid >> 1) & 0x04) | (extid & 0x03));
-   } else if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
-   !nand_is_slc(chip)) {
+   if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
+   !nand_is_slc(chip)) {
unsigned int tmp;
 
/* Calc pagesize */
@@ -3961,13 +3926,10 @@ static void nand_decode_bbm_options(struct nand_chip 
*chip)
 * Micron devices with 2KiB pages and on SLC Samsung, Hynix, Toshiba,
 * AMD/Spansion, and Macronix.  All others scan only the first page.
 */
-   if (!nand_is_slc(chip) &&
-   (maf_id == NAND_MFR_SAMSUNG ||
-maf_id == NAND_MFR_HYNIX))
+   if (!nand_is_slc(chip) && maf_id == NAND_MFR_HYNIX)
chip->bbt_options |= NAND_BBT_SCANLASTPAGE;
else if ((nand_is_slc(chip) &&
-   (maf_id == NAND_MFR_SAMSUNG ||
-maf_id == NAND_MFR_HYNIX ||
+   (maf_id == NAND_MFR_HYNIX ||
 maf_id == NAND_MFR_TOSHIBA ||
 maf_id == NAND_MFR_AMD ||
 maf_id == NAND_MFR_MACRONIX)) ||
@@ -4149,12 +4111,6 @@ static int nand_detect(struct nand_chip *chip, struct 
nand_flash_dev *type)
/* Get chip options */
chip->options |= type->options;
 
-   /*
-* Check if chip is not a Samsung device. Do not clear the
-* options for chips which do not have an extended id.
-*/
-   if (maf_id != NAND_MFR_SAMSUNG && !type->pagesize)
-   chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
 ident_done:
 
if (chip->options & NAND_BUSWIDTH_AUTO) {
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