Patchwork devicetree/bindings: Add binding for micron n25q512a memory

login
register
mail settings
Submitter Priyanka Jain
Date July 3, 2014, 10:12 a.m.
Message ID <1404382376-3115-1-git-send-email-Priyanka.Jain@freescale.com>
Download mbox | patch
Permalink /patch/366818/
State New
Headers show

Comments

Priyanka Jain - July 3, 2014, 10:12 a.m.
-Micron n25q512a memory is supported by m25p80 driver.
 Add compatible field required to support n25q512a in m25p80.txt
-Add micron to the vendor-prefixes.txt file

Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
---
 Documentation/devicetree/bindings/mtd/m25p80.txt   |    1 +
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)
Scott Wood - July 3, 2014, 10:10 p.m.
On Thu, 2014-07-03 at 15:42 +0530, Priyanka Jain wrote:
> -Micron n25q512a memory is supported by m25p80 driver.
>  Add compatible field required to support n25q512a in m25p80.txt
> -Add micron to the vendor-prefixes.txt file
> 
> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
> ---
>  Documentation/devicetree/bindings/mtd/m25p80.txt   |    1 +
>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>  2 files changed, 2 insertions(+), 0 deletions(-)

Why did you send this to the ppc list but not the spi or mtd lists?

I'm having a hard time following the flow of how these SPI devices get
bound -- is the compatible involved at all?  I don't see this string
(with vendor prefix included) in the driver.  I do see a table that
contains what looks like device IDs.  If the device can report its id,
shouldn't we rely on that rather than device tree compatible?

-Scott


> diff --git a/Documentation/devicetree/bindings/mtd/m25p80.txt b/Documentation/devicetree/bindings/mtd/m25p80.txt
> index 4611aa8..ce02e81 100644
> --- a/Documentation/devicetree/bindings/mtd/m25p80.txt
> +++ b/Documentation/devicetree/bindings/mtd/m25p80.txt
> @@ -7,6 +7,7 @@ Required properties:
>                 the DT binding is not Linux-only, but in case of Linux, see the
>                 "spi_nor_ids" table in drivers/mtd/spi-nor/spi-nor.c for the list
>                 of supported chips.
> +               example: "micron,n25q512a"
>  - reg : Chip-Select number
>  - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at
>  
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 4d7f375..a44cfee 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -79,6 +79,7 @@ marvell	Marvell Technology Group Ltd.
>  maxim	Maxim Integrated Products
>  micrel	Micrel Inc.
>  microchip	Microchip Technology Inc.
> +micron	Micron Technology Inc.
>  mosaixtech	Mosaix Technologies, Inc.
>  moxa	Moxa
>  mpl	MPL AG
Priyanka Jain - July 4, 2014, 4:08 a.m.
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Friday, July 04, 2014 3:40 AM

> To: Jain Priyanka-B32167

> Cc: devicetree@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-

> spi@vger.kernel.org; linux-mtd@lists.infradead.org

> Subject: Re: [PATCH] devicetree/bindings: Add binding for micron n25q512a

> memory

> 

> On Thu, 2014-07-03 at 15:42 +0530, Priyanka Jain wrote:

> > -Micron n25q512a memory is supported by m25p80 driver.

> >  Add compatible field required to support n25q512a in m25p80.txt -Add

> > micron to the vendor-prefixes.txt file

> >

> > Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>

> > ---

> >  Documentation/devicetree/bindings/mtd/m25p80.txt   |    1 +

> >  .../devicetree/bindings/vendor-prefixes.txt        |    1 +

> >  2 files changed, 2 insertions(+), 0 deletions(-)

> 

> Why did you send this to the ppc list but not the spi or mtd lists?

> 

[Jain Priyanka-B32167] Sorry, I missed that
> I'm having a hard time following the flow of how these SPI devices get

> bound -- is the compatible involved at all?  I don't see this string

> (with vendor prefix included) in the driver.  I do see a table that

> contains what looks like device IDs.  If the device can report its id,

> shouldn't we rely on that rather than device tree compatible?

> 

> -Scott

[Jain Priyanka-B32167]
Spi driver has a check to device name corresponding to device-id and compare to what is passed in dts string.
New checkpatch script search for exact string match in binding folder. So, I have added this in example.
> 

> 

> > diff --git a/Documentation/devicetree/bindings/mtd/m25p80.txt

> > b/Documentation/devicetree/bindings/mtd/m25p80.txt

> > index 4611aa8..ce02e81 100644

> > --- a/Documentation/devicetree/bindings/mtd/m25p80.txt

> > +++ b/Documentation/devicetree/bindings/mtd/m25p80.txt

> > @@ -7,6 +7,7 @@ Required properties:

> >                 the DT binding is not Linux-only, but in case of Linux,

> see the

> >                 "spi_nor_ids" table in drivers/mtd/spi-nor/spi-nor.c

> for the list

> >                 of supported chips.

> > +               example: "micron,n25q512a"

> >  - reg : Chip-Select number

> >  - spi-max-frequency : Maximum frequency of the SPI bus the chip can

> > operate at

> >

> > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt

> > b/Documentation/devicetree/bindings/vendor-prefixes.txt

> > index 4d7f375..a44cfee 100644

> > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt

> > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt

> > @@ -79,6 +79,7 @@ marvell	Marvell Technology Group Ltd.

> >  maxim	Maxim Integrated Products

> >  micrel	Micrel Inc.

> >  microchip	Microchip Technology Inc.

> > +micron	Micron Technology Inc.

> >  mosaixtech	Mosaix Technologies, Inc.

> >  moxa	Moxa

> >  mpl	MPL AG

>
Scott Wood - July 7, 2014, 9:25 p.m.
On Thu, 2014-07-03 at 23:08 -0500, Jain Priyanka-B32167 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Friday, July 04, 2014 3:40 AM
> > To: Jain Priyanka-B32167
> > Cc: devicetree@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> > spi@vger.kernel.org; linux-mtd@lists.infradead.org
> > Subject: Re: [PATCH] devicetree/bindings: Add binding for micron n25q512a
> > memory
> > 
> > On Thu, 2014-07-03 at 15:42 +0530, Priyanka Jain wrote:
> > > -Micron n25q512a memory is supported by m25p80 driver.
> > >  Add compatible field required to support n25q512a in m25p80.txt -Add
> > > micron to the vendor-prefixes.txt file
> > >
> > > Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
> > > ---
> > >  Documentation/devicetree/bindings/mtd/m25p80.txt   |    1 +
> > >  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
> > >  2 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > Why did you send this to the ppc list but not the spi or mtd lists?
> > 
> [Jain Priyanka-B32167] Sorry, I missed that
> > I'm having a hard time following the flow of how these SPI devices get
> > bound -- is the compatible involved at all?  I don't see this string
> > (with vendor prefix included) in the driver.  I do see a table that
> > contains what looks like device IDs.  If the device can report its id,
> > shouldn't we rely on that rather than device tree compatible?
> > 
> > -Scott
> [Jain Priyanka-B32167]
> Spi driver has a check to device name corresponding to device-id and compare to what is passed in dts string.

Please go into more detail.  I don't see where the string
"micron,n25q512a" appears in the current kernel.  I do see "n25q512a",
but how does that compare successfully with the version of the string
that has a vendor compatible?

Again, is there a device ID that can be read at runtime?  Is there a
compatible string that represents a common programming interface?

> New checkpatch script search for exact string match in binding folder. So, I have added this in example.

That check has a lot of false positives when it comes to compatible
strings for specific chips that don't need to be specifically mentioned
in a more generic binding.

-Scott
Priyanka Jain - July 8, 2014, 4:41 a.m.
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Tuesday, July 08, 2014 2:56 AM

> To: Jain Priyanka-B32167

> Cc: devicetree@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-

> spi@vger.kernel.org; linux-mtd@lists.infradead.org

> Subject: Re: [PATCH] devicetree/bindings: Add binding for micron n25q512a

> memory

> 

> On Thu, 2014-07-03 at 23:08 -0500, Jain Priyanka-B32167 wrote:

> >

> > > -----Original Message-----

> > > From: Wood Scott-B07421

> > > Sent: Friday, July 04, 2014 3:40 AM

> > > To: Jain Priyanka-B32167

> > > Cc: devicetree@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > linux- spi@vger.kernel.org; linux-mtd@lists.infradead.org

> > > Subject: Re: [PATCH] devicetree/bindings: Add binding for micron

> > > n25q512a memory

> > >

> > > On Thu, 2014-07-03 at 15:42 +0530, Priyanka Jain wrote:

> > > > -Micron n25q512a memory is supported by m25p80 driver.

> > > >  Add compatible field required to support n25q512a in m25p80.txt

> > > > -Add micron to the vendor-prefixes.txt file

> > > >

> > > > Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>

> > > > ---

> > > >  Documentation/devicetree/bindings/mtd/m25p80.txt   |    1 +

> > > >  .../devicetree/bindings/vendor-prefixes.txt        |    1 +

> > > >  2 files changed, 2 insertions(+), 0 deletions(-)

> > >

> > > Why did you send this to the ppc list but not the spi or mtd lists?

> > >

> > [Jain Priyanka-B32167] Sorry, I missed that

> > > I'm having a hard time following the flow of how these SPI devices

> > > get bound -- is the compatible involved at all?  I don't see this

> > > string (with vendor prefix included) in the driver.  I do see a

> > > table that contains what looks like device IDs.  If the device can

> > > report its id, shouldn't we rely on that rather than device tree

> compatible?

> > >

> > > -Scott

> > [Jain Priyanka-B32167]

> > Spi driver has a check to device name corresponding to device-id and

> compare to what is passed in dts string.

> 

> Please go into more detail.  I don't see where the string

> "micron,n25q512a" appears in the current kernel.  I do see "n25q512a",

> but how does that compare successfully with the version of the string

> that has a vendor compatible?

> 

> Again, is there a device ID that can be read at runtime?  Is there a

> compatible string that represents a common programming interface?

[Jain Priyanka-B32167] 
Spi driver (of_register_spi_devices()) search for compatible string in device-tree. It selects the flash device driver based on the compatible string (n25q512a). This compatible string is essential, else device probe wont continue.  Then Device-id of flash is read at runtime in probe and compared with the corresponding device-id of flash (n25q512a). Device-id (maintained as first element in INFO string in spi_nor_ids[] in drivers/mtd/spi-nor/spi-nor.c file)
Only if both device-ids match, the device is taken as successfully detected.
Regarding vendor "micron", you are right currently the vendor information is not maintained as such in device-id, but the convention that is followed for compatible string is "vendor-id, device-id"
> 

> > New checkpatch script search for exact string match in binding folder.

> So, I have added this in example.

> 

> That check has a lot of false positives when it comes to compatible

> strings for specific chips that don't need to be specifically mentioned

> in a more generic binding.

> 

> -Scott

> 

[Jain Priyanka-B32167] Even though "micron,n25q512a" is in sync with binding mentioned in " Documentation/devicetree/bindings/mtd/m25p80.txt   ", I have to add this as an a example, just to avoid checkpatch error. What should be the approach going forward: Should such checkpatch warnings be ignored?
Scott Wood - July 9, 2014, 1:53 a.m.
On Mon, 2014-07-07 at 23:41 -0500, Jain Priyanka-B32167 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, July 08, 2014 2:56 AM
> > To: Jain Priyanka-B32167
> > Cc: devicetree@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> > spi@vger.kernel.org; linux-mtd@lists.infradead.org
> > Subject: Re: [PATCH] devicetree/bindings: Add binding for micron n25q512a
> > memory
> > 
> > On Thu, 2014-07-03 at 23:08 -0500, Jain Priyanka-B32167 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Friday, July 04, 2014 3:40 AM
> > > > To: Jain Priyanka-B32167
> > > > Cc: devicetree@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > linux- spi@vger.kernel.org; linux-mtd@lists.infradead.org
> > > > Subject: Re: [PATCH] devicetree/bindings: Add binding for micron
> > > > n25q512a memory
> > > >
> > > > On Thu, 2014-07-03 at 15:42 +0530, Priyanka Jain wrote:
> > > > > -Micron n25q512a memory is supported by m25p80 driver.
> > > > >  Add compatible field required to support n25q512a in m25p80.txt
> > > > > -Add micron to the vendor-prefixes.txt file
> > > > >
> > > > > Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/mtd/m25p80.txt   |    1 +
> > > > >  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
> > > > >  2 files changed, 2 insertions(+), 0 deletions(-)
> > > >
> > > > Why did you send this to the ppc list but not the spi or mtd lists?
> > > >
> > > [Jain Priyanka-B32167] Sorry, I missed that
> > > > I'm having a hard time following the flow of how these SPI devices
> > > > get bound -- is the compatible involved at all?  I don't see this
> > > > string (with vendor prefix included) in the driver.  I do see a
> > > > table that contains what looks like device IDs.  If the device can
> > > > report its id, shouldn't we rely on that rather than device tree
> > compatible?
> > > >
> > > > -Scott
> > > [Jain Priyanka-B32167]
> > > Spi driver has a check to device name corresponding to device-id and
> > compare to what is passed in dts string.
> > 
> > Please go into more detail.  I don't see where the string
> > "micron,n25q512a" appears in the current kernel.  I do see "n25q512a",
> > but how does that compare successfully with the version of the string
> > that has a vendor compatible?
> > 
> > Again, is there a device ID that can be read at runtime?  Is there a
> > compatible string that represents a common programming interface?
> [Jain Priyanka-B32167] 
>
> Spi driver (of_register_spi_devices()) search for compatible string in
> device-tree. It selects the flash device driver based on the compatible
> string (n25q512a). This compatible string is essential, else device
> probe wont continue.  Then Device-id of flash is read at runtime in
> probe and compared with the corresponding device-id of flash
> (n25q512a). Device-id (maintained as first element in INFO string in
> spi_nor_ids[] in drivers/mtd/spi-nor/spi-nor.c file)

By "device id" I'm talking about 0x20bb20, not "n25q512a".  Where does
that come from?

> Only if both device-ids match, the device is taken as successfully
> detected. Regarding vendor "micron", you are right currently the vendor
> information is not maintained as such in device-id, but the convention
> that is followed for compatible string is "vendor-id, device-id"

Is there going to be a patch to the driver to accept the string with
vendor id?

> > > New checkpatch script search for exact string match in binding folder.
> > So, I have added this in example.
> > 
> > That check has a lot of false positives when it comes to compatible
> > strings for specific chips that don't need to be specifically mentioned
> > in a more generic binding.
> > 
> > -Scott
> > 
> [Jain Priyanka-B32167] Even though "micron,n25q512a" is in sync with
> binding mentioned in "
> Documentation/devicetree/bindings/mtd/m25p80.txt   ", I have to add
> this as an a example, just to avoid checkpatch error. What should be
> the approach going forward: Should such checkpatch warnings be ignored?

They shouldn't be ignored nor blindly remedied.  You should look at the
message and decide whether a binding is needed and whether it is
present.  There probably should be a binding here, but I don't have
enough information to say whether it should be specific to n25q512a or
if spi nor can be described more generically.

-Scott
Mark Rutland - July 29, 2014, 9:24 a.m.
On Mon, Jul 07, 2014 at 10:25:54PM +0100, Scott Wood wrote:
> On Thu, 2014-07-03 at 23:08 -0500, Jain Priyanka-B32167 wrote:
> > 
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Friday, July 04, 2014 3:40 AM
> > > To: Jain Priyanka-B32167
> > > Cc: devicetree@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> > > spi@vger.kernel.org; linux-mtd@lists.infradead.org
> > > Subject: Re: [PATCH] devicetree/bindings: Add binding for micron n25q512a
> > > memory
> > > 
> > > On Thu, 2014-07-03 at 15:42 +0530, Priyanka Jain wrote:
> > > > -Micron n25q512a memory is supported by m25p80 driver.
> > > >  Add compatible field required to support n25q512a in m25p80.txt -Add
> > > > micron to the vendor-prefixes.txt file
> > > >
> > > > Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/mtd/m25p80.txt   |    1 +
> > > >  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
> > > >  2 files changed, 2 insertions(+), 0 deletions(-)
> > > 
> > > Why did you send this to the ppc list but not the spi or mtd lists?
> > > 
> > [Jain Priyanka-B32167] Sorry, I missed that
> > > I'm having a hard time following the flow of how these SPI devices get
> > > bound -- is the compatible involved at all?  I don't see this string
> > > (with vendor prefix included) in the driver.  I do see a table that
> > > contains what looks like device IDs.  If the device can report its id,
> > > shouldn't we rely on that rather than device tree compatible?
> > > 
> > > -Scott
> > [Jain Priyanka-B32167]
> > Spi driver has a check to device name corresponding to device-id and compare to what is passed in dts string.
> 
> Please go into more detail.  I don't see where the string
> "micron,n25q512a" appears in the current kernel.  I do see "n25q512a",
> but how does that compare successfully with the version of the string
> that has a vendor compatible?

The spi code has a hack to not require an of_device_id_table:
of_register_spi_devices calls of_modalias_node, which for a given
"foo,bar" compatible string will set spi->modalias to "bar", by cutting
off everything up to and including the comma. Later, spi_match_id can
match a device with an spi_device_id->name of "bar".

Thus "micron,n25q512a" can match one of the entries already in
drivers/mtd/spi-nor/spi-nor.c.

This is a real pain when searching around.

Thanks,
Mark.

Patch

diff --git a/Documentation/devicetree/bindings/mtd/m25p80.txt b/Documentation/devicetree/bindings/mtd/m25p80.txt
index 4611aa8..ce02e81 100644
--- a/Documentation/devicetree/bindings/mtd/m25p80.txt
+++ b/Documentation/devicetree/bindings/mtd/m25p80.txt
@@ -7,6 +7,7 @@  Required properties:
                the DT binding is not Linux-only, but in case of Linux, see the
                "spi_nor_ids" table in drivers/mtd/spi-nor/spi-nor.c for the list
                of supported chips.
+               example: "micron,n25q512a"
 - reg : Chip-Select number
 - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at
 
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 4d7f375..a44cfee 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -79,6 +79,7 @@  marvell	Marvell Technology Group Ltd.
 maxim	Maxim Integrated Products
 micrel	Micrel Inc.
 microchip	Microchip Technology Inc.
+micron	Micron Technology Inc.
 mosaixtech	Mosaix Technologies, Inc.
 moxa	Moxa
 mpl	MPL AG