| Message ID | 1447713292-91525-4-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Commit | 1d158315c13b6989f7ddb1d8d334965d14d958d8 |
| Headers | show |
On Mon, Nov 16, 2015 at 02:34:52PM -0800, Brian Norris wrote: > Pull the supported chip names from drivers/mtd/devices/m25p80.c and stop > pointing readers to Linux code. > > Also (although I see this habit repeated throughout the > Documentation/devicetree/bindings/ tree), stop using the title "driver" > in this file, when we're trying explicitly to describe hardware, not > software. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > Cc: <devicetree@vger.kernel.org> Thanks for the clean-up. Acked-by: Rob Herring <robh@kernel.org> > --- > .../devicetree/bindings/mtd/jedec,spi-nor.txt | 56 ++++++++++++++++++++-- > 1 file changed, 51 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > index 2bee68103b01..2c91c03e7eb0 100644 > --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > @@ -1,15 +1,61 @@ > -* MTD SPI driver for ST M25Pxx (and similar) serial flash chips > +* SPI NOR flash: ST M25Pxx (and similar) serial flash chips > > Required properties: > - #address-cells, #size-cells : Must be present if the device has sub-nodes > representing partitions. > - compatible : May include a device-specific string consisting of the > - manufacturer and name of the chip. Bear in mind the DT binding > - is not Linux-only, but in case of Linux, see the "m25p_ids" > - table in drivers/mtd/devices/m25p80.c for the list of supported > - chips. > + manufacturer and name of the chip. A list of supported chip > + names follows. > Must also include "jedec,spi-nor" for any SPI NOR flash that can > be identified by the JEDEC READ ID opcode (0x9F). > + > + Supported chip names: > + at25df321a > + at25df641 > + at26df081a > + mr25h256 > + mx25l4005a > + mx25l1606e > + mx25l6405d > + mx25l12805d > + mx25l25635e > + n25q064 > + n25q128a11 > + n25q128a13 > + n25q512a > + s25fl256s1 > + s25fl512s > + s25sl12801 > + s25fl008k > + s25fl064k > + sst25vf040b > + m25p40 > + m25p80 > + m25p16 > + m25p32 > + m25p64 > + m25p128 > + w25x80 > + w25x32 > + w25q32 > + w25q32dw > + w25q80bl > + w25q128 > + w25q256 > + > + The following chip names have been used historically to > + designate quirky versions of flash chips that do not support the > + JEDEC READ ID opcode (0x9F): > + m25p05-nonjedec > + m25p10-nonjedec > + m25p20-nonjedec > + m25p40-nonjedec > + m25p80-nonjedec > + m25p16-nonjedec > + m25p32-nonjedec > + m25p64-nonjedec > + m25p128-nonjedec > + > - reg : Chip-Select number > - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at > > -- > 2.6.0.rc2.230.g3dd15c0 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Brian, On 11/16/2015 07:34 PM, Brian Norris wrote: > Pull the supported chip names from drivers/mtd/devices/m25p80.c and stop > pointing readers to Linux code. > > Also (although I see this habit repeated throughout the > Documentation/devicetree/bindings/ tree), stop using the title "driver" Agreed. > in this file, when we're trying explicitly to describe hardware, not > software. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > Cc: <devicetree@vger.kernel.org> > --- > .../devicetree/bindings/mtd/jedec,spi-nor.txt | 56 ++++++++++++++++++++-- > 1 file changed, 51 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > index 2bee68103b01..2c91c03e7eb0 100644 > --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > @@ -1,15 +1,61 @@ > -* MTD SPI driver for ST M25Pxx (and similar) serial flash chips > +* SPI NOR flash: ST M25Pxx (and similar) serial flash chips > > Required properties: > - #address-cells, #size-cells : Must be present if the device has sub-nodes > representing partitions. > - compatible : May include a device-specific string consisting of the > - manufacturer and name of the chip. Bear in mind the DT binding > - is not Linux-only, but in case of Linux, see the "m25p_ids" > - table in drivers/mtd/devices/m25p80.c for the list of supported > - chips. > + manufacturer and name of the chip. A list of supported chip > + names follows. Here says that the compatible string consists of manufacturer and name... > Must also include "jedec,spi-nor" for any SPI NOR flash that can > be identified by the JEDEC READ ID opcode (0x9F). > + > + Supported chip names: > + at25df321a > + at25df641 > + at26df081a > + mr25h256 > + mx25l4005a > + mx25l1606e > + mx25l6405d > + mx25l12805d > + mx25l25635e > + n25q064 > + n25q128a11 > + n25q128a13 > + n25q512a > + s25fl256s1 > + s25fl512s > + s25sl12801 > + s25fl008k > + s25fl064k > + sst25vf040b > + m25p40 > + m25p80 > + m25p16 > + m25p32 > + m25p64 > + m25p128 > + w25x80 > + w25x32 > + w25q32 > + w25q32dw > + w25q80bl > + w25q128 > + w25q256 > + ... but the entries in the list don't have a manufacturer. I know this is due backward compatibility because as we discussed in the thread mentioned in the cover letter, the SPI core didn't use the manufacturer and that implementation detail leaked into the DTBs. But I think that either only the correct list with vendor should be added to the DT binding docs (but make sure that backward compatibility in the driver and SPI core is maintained) or both the wrong and correct list should be documented and the former be marked as deprecated. > + The following chip names have been used historically to > + designate quirky versions of flash chips that do not support the > + JEDEC READ ID opcode (0x9F): > + m25p05-nonjedec > + m25p10-nonjedec > + m25p20-nonjedec > + m25p40-nonjedec > + m25p80-nonjedec > + m25p16-nonjedec > + m25p32-nonjedec > + m25p64-nonjedec > + m25p128-nonjedec > + Same here, I would prefer if the DT binding make it clear that not having a vendor is wrong and is only documented to maintain backward compatibility. > - reg : Chip-Select number > - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at > > Best regards,
Hi, On Tue, Nov 17, 2015 at 11:04:55AM -0300, Javier Martinez Canillas wrote: > On 11/16/2015 07:34 PM, Brian Norris wrote: > > diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > > index 2bee68103b01..2c91c03e7eb0 100644 > > --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > > +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > > @@ -1,15 +1,61 @@ > > -* MTD SPI driver for ST M25Pxx (and similar) serial flash chips > > +* SPI NOR flash: ST M25Pxx (and similar) serial flash chips > > > > Required properties: > > - #address-cells, #size-cells : Must be present if the device has sub-nodes > > representing partitions. > > - compatible : May include a device-specific string consisting of the > > - manufacturer and name of the chip. Bear in mind the DT binding > > - is not Linux-only, but in case of Linux, see the "m25p_ids" > > - table in drivers/mtd/devices/m25p80.c for the list of supported > > - chips. > > + manufacturer and name of the chip. A list of supported chip > > + names follows. > > Here says that the compatible string consists of manufacturer and name... > > > Must also include "jedec,spi-nor" for any SPI NOR flash that can > > be identified by the JEDEC READ ID opcode (0x9F). > > + > > + Supported chip names: > > + at25df321a > > + at25df641 [...] > + > > ... but the entries in the list don't have a manufacturer. I know this is > due backward compatibility because as we discussed in the thread mentioned > in the cover letter, the SPI core didn't use the manufacturer and that > implementation detail leaked into the DTBs. > > But I think that either only the correct list with vendor should be added > to the DT binding docs (but make sure that backward compatibility in the > driver and SPI core is maintained) or both the wrong and correct list should > be documented and the former be marked as deprecated. First, note that the list says "Supported chip *names*" (not "Supported compatible values"). It does not attempt to specify the full compatible value, and that's intentional. Second, I believe it is hard to determine after-the-fact what all the reasonable pairings of vendors might be. For some of these parts, various companies have produced parts under the same chip ID -- usually because one company bought another. For most chips though, this probably isn't a problem, so I could probably pick reasonable vendor pairings. IOW, there isn't just "a wrong" and "a correct" list; there's a (probably?) correct list and an enormous space of "I don't know what people might have put here" list. It's nearly unbounded, but even a reasonable list might get pretty large. So in practice, we'd essentially be sacrificing completeness for...what reason? > > + The following chip names have been used historically to > > + designate quirky versions of flash chips that do not support the > > + JEDEC READ ID opcode (0x9F): > > + m25p05-nonjedec > > + m25p10-nonjedec > > + m25p20-nonjedec > > + m25p40-nonjedec > > + m25p80-nonjedec > > + m25p16-nonjedec > > + m25p32-nonjedec > > + m25p64-nonjedec > > + m25p128-nonjedec > > + > > Same here, I would prefer if the DT binding make it clear that not having > a vendor is wrong and is only documented to maintain backward compatibility. The doc never says anything about not including the vendor. It says "May include a device-specific string consisting of the manufacturer and name of the chip" and it lists the chip names. So if someone is actually following the documentation, they will include a vendor. The vendor names are not listed because they're really not relevant to the implementation. But I could try to include them. > > - reg : Chip-Select number > > - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at > > > > So, what makes sense? I can make a separate list of vendors (my preference), or even try to pair up vendors with chip names (even if it's sometimes an N:1 relationship). But I don't see that really buying us much, since the implementation never has (and probably never will) enforce this. What do you think? Regards, Brian
Hello Brian, On 11/18/2015 04:43 PM, Brian Norris wrote: > Hi, > > On Tue, Nov 17, 2015 at 11:04:55AM -0300, Javier Martinez Canillas wrote: >> On 11/16/2015 07:34 PM, Brian Norris wrote: >>> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt >>> index 2bee68103b01..2c91c03e7eb0 100644 >>> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt >>> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt >>> @@ -1,15 +1,61 @@ >>> -* MTD SPI driver for ST M25Pxx (and similar) serial flash chips >>> +* SPI NOR flash: ST M25Pxx (and similar) serial flash chips >>> >>> Required properties: >>> - #address-cells, #size-cells : Must be present if the device has sub-nodes >>> representing partitions. >>> - compatible : May include a device-specific string consisting of the >>> - manufacturer and name of the chip. Bear in mind the DT binding >>> - is not Linux-only, but in case of Linux, see the "m25p_ids" >>> - table in drivers/mtd/devices/m25p80.c for the list of supported >>> - chips. >>> + manufacturer and name of the chip. A list of supported chip >>> + names follows. >> >> Here says that the compatible string consists of manufacturer and name... >> >>> Must also include "jedec,spi-nor" for any SPI NOR flash that can >>> be identified by the JEDEC READ ID opcode (0x9F). >>> + >>> + Supported chip names: >>> + at25df321a >>> + at25df641 > [...] >> + >> >> ... but the entries in the list don't have a manufacturer. I know this is >> due backward compatibility because as we discussed in the thread mentioned >> in the cover letter, the SPI core didn't use the manufacturer and that >> implementation detail leaked into the DTBs. >> >> But I think that either only the correct list with vendor should be added >> to the DT binding docs (but make sure that backward compatibility in the >> driver and SPI core is maintained) or both the wrong and correct list should >> be documented and the former be marked as deprecated. > > First, note that the list says "Supported chip *names*" (not "Supported > compatible values"). It does not attempt to specify the full compatible > value, and that's intentional. > Right, sorry I missed that subtlety. > Second, I believe it is hard to determine after-the-fact what all the > reasonable pairings of vendors might be. For some of these parts, > various companies have produced parts under the same chip ID -- usually > because one company bought another. For most chips though, this probably > isn't a problem, so I could probably pick reasonable vendor pairings. > > IOW, there isn't just "a wrong" and "a correct" list; there's a > (probably?) correct list and an enormous space of "I don't know what > people might have put here" list. It's nearly unbounded, but even a > reasonable list might get pretty large. So in practice, we'd essentially > be sacrificing completeness for...what reason? > I see. >>> + The following chip names have been used historically to >>> + designate quirky versions of flash chips that do not support the >>> + JEDEC READ ID opcode (0x9F): >>> + m25p05-nonjedec >>> + m25p10-nonjedec >>> + m25p20-nonjedec >>> + m25p40-nonjedec >>> + m25p80-nonjedec >>> + m25p16-nonjedec >>> + m25p32-nonjedec >>> + m25p64-nonjedec >>> + m25p128-nonjedec >>> + >> >> Same here, I would prefer if the DT binding make it clear that not having >> a vendor is wrong and is only documented to maintain backward compatibility. > > The doc never says anything about not including the vendor. It says > > "May include a device-specific string consisting of the manufacturer > and name of the chip" > > and it lists the chip names. So if someone is actually following the > documentation, they will include a vendor. The vendor names are not > listed because they're really not relevant to the implementation. But I > could try to include them. > My goal was to start forcing people to use a complete compatible string to avoid the "garbage,chip-name" that you mentioned in the other thread. The vendor are not relevant in the current implementation because how the SPI core is implemented but I think that shouldn't affect the accuracy on which hardware is described in the DT. >>> - reg : Chip-Select number >>> - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at >>> >>> > > So, what makes sense? I can make a separate list of vendors (my > preference), or even try to pair up vendors with chip names (even if > it's sometimes an N:1 relationship). But I don't see that really buying > us much, since the implementation never has (and probably never will) > enforce this. What do you think? > Yes, you are right that is hard to come with a reasonable list specially since the vendor and chip relation could be N:1 as you said. $SUBJECT is definitely an improvement over the current doc that mentions the "m25p_ids" table in the driver though. So we can update later the DT binding once / if the SPI core is modified to report proper OF modalias so OF-only drivers can get rid of their SPI id table. So for this patch: Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com> Best regards,
diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt index 2bee68103b01..2c91c03e7eb0 100644 --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt @@ -1,15 +1,61 @@ -* MTD SPI driver for ST M25Pxx (and similar) serial flash chips +* SPI NOR flash: ST M25Pxx (and similar) serial flash chips Required properties: - #address-cells, #size-cells : Must be present if the device has sub-nodes representing partitions. - compatible : May include a device-specific string consisting of the - manufacturer and name of the chip. Bear in mind the DT binding - is not Linux-only, but in case of Linux, see the "m25p_ids" - table in drivers/mtd/devices/m25p80.c for the list of supported - chips. + manufacturer and name of the chip. A list of supported chip + names follows. Must also include "jedec,spi-nor" for any SPI NOR flash that can be identified by the JEDEC READ ID opcode (0x9F). + + Supported chip names: + at25df321a + at25df641 + at26df081a + mr25h256 + mx25l4005a + mx25l1606e + mx25l6405d + mx25l12805d + mx25l25635e + n25q064 + n25q128a11 + n25q128a13 + n25q512a + s25fl256s1 + s25fl512s + s25sl12801 + s25fl008k + s25fl064k + sst25vf040b + m25p40 + m25p80 + m25p16 + m25p32 + m25p64 + m25p128 + w25x80 + w25x32 + w25q32 + w25q32dw + w25q80bl + w25q128 + w25q256 + + The following chip names have been used historically to + designate quirky versions of flash chips that do not support the + JEDEC READ ID opcode (0x9F): + m25p05-nonjedec + m25p10-nonjedec + m25p20-nonjedec + m25p40-nonjedec + m25p80-nonjedec + m25p16-nonjedec + m25p32-nonjedec + m25p64-nonjedec + m25p128-nonjedec + - reg : Chip-Select number - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at
Pull the supported chip names from drivers/mtd/devices/m25p80.c and stop pointing readers to Linux code. Also (although I see this habit repeated throughout the Documentation/devicetree/bindings/ tree), stop using the title "driver" in this file, when we're trying explicitly to describe hardware, not software. Signed-off-by: Brian Norris <computersforpeace@gmail.com> Cc: <devicetree@vger.kernel.org> --- .../devicetree/bindings/mtd/jedec,spi-nor.txt | 56 ++++++++++++++++++++-- 1 file changed, 51 insertions(+), 5 deletions(-)