| Message ID | 1431624773-4165-1-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On 05/14/2015 11:32 AM, Brian Norris wrote: > In commit 8ff16cf77ce3 ("Documentation: devicetree: m25p80: add "nor-jedec" > binding"), we added a generic "nor-jedec" binding to catch all > mostly-compatible SPI NOR flash which can be detected via the READ ID > opcode (0x9F). This was discussed and reviewed at the time, however > objections have come up since then as part of this discussion: > > http://lkml.kernel.org/g/20150511224646.GJ32500@ld-irv-0074 > > It seems the parties involved agree that "jedec,spi-nor" does a better > job of capturing the fact that this is SPI-specific, not just any NOR > flash. > > This binding was only merged for v4.1-rc1, so it's still OK to change > the naming. > > At the same time, let's move the documentation to a better name. > > Next up: prune the m25p_ids[] table to the minimal necessary listing, so > we can stop referring to code (drivers/mtd/devices/m25p80.c) from the > documentation. There's no need to change the code to update the documentation. Simply paste the list of valid device IDs into the documentation. The binding documentation needs to be completely standalone anyway. Binding documentation should never refer to Linux driver code as part of their definition. You can never remove the currently-supported device-specific IDs from the driver, since existing DTs need to continue working forever, even with future drivers/kernels. The binding document should also always include a complete list of supported flash devices. Standard practice is that the DT compatible property contains a list of compatible values, starting with the device-specific value, and followed by any generic values. All of those values should be standardized and specified in the DT documentation, even if the DT binding is written in such a way that a compliant driver need only actively match on the generic value. The device-specific values may not be used today, but are present in case some device-specific workaround needs to be retro-actively implemented/enabled, since that needs to happen for existing DTs too. > I'd *really* like to get an 'ack' from a DT maintainer for this, those those > are apparently very hard to come by. And I'd really not like to have to revisit > this again in a few weeks. We have patches getting queued up for 4.2 that are > using the "nor-jedec" binding, and I'd like to nip those in the bud ASAP. I am not a DT maintainer, but the DT documentation part of this change: Acked-by: Stephen Warren <swarren@nvidia.com>
On Thu, May 14, 2015 at 7:46 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 05/14/2015 11:32 AM, Brian Norris wrote: >> In commit 8ff16cf77ce3 ("Documentation: devicetree: m25p80: add >> "nor-jedec" >> binding"), we added a generic "nor-jedec" binding to catch all >> mostly-compatible SPI NOR flash which can be detected via the READ ID >> opcode (0x9F). This was discussed and reviewed at the time, however >> objections have come up since then as part of this discussion: >> >> http://lkml.kernel.org/g/20150511224646.GJ32500@ld-irv-0074 >> >> It seems the parties involved agree that "jedec,spi-nor" does a better >> job of capturing the fact that this is SPI-specific, not just any NOR >> flash. >> >> This binding was only merged for v4.1-rc1, so it's still OK to change >> the naming. >> >> At the same time, let's move the documentation to a better name. >> >> Next up: prune the m25p_ids[] table to the minimal necessary listing, so >> we can stop referring to code (drivers/mtd/devices/m25p80.c) from the >> documentation. > > There's no need to change the code to update the documentation. Simply paste > the list of valid device IDs into the documentation. The binding > documentation needs to be completely standalone anyway. Binding > documentation should never refer to Linux driver code as part of their > definition. > > You can never remove the currently-supported device-specific IDs from the > driver, since existing DTs need to continue working forever, even with > future drivers/kernels. > > The binding document should also always include a complete list of supported > flash devices. Standard practice is that the DT compatible property contains > a list of compatible values, starting with the device-specific value, and > followed by any generic values. All of those values should be standardized > and specified in the DT documentation, even if the DT binding is written in > such a way that a compliant driver need only actively match on the generic > value. The device-specific values may not be used today, but are present in > case some device-specific workaround needs to be retro-actively > implemented/enabled, since that needs to happen for existing DTs too. Indeed, all supported flash devices should be listed in the DT binding documentation, so checkpatch can validate dts changes: $ scripts/checkpatch.pl -f arch/arm/boot/dts/r8a7791-koelsch.dts [...] WARNING: DT compatible string "spansion,s25fl512s" appears un-documented -- check ./Documentation/devicetree/bindings/ #493: FILE: arch/arm/boot/dts/r8a7791-koelsch.dts:493: + compatible = "spansion,s25fl512s", "nor-jedec"; >> I'd *really* like to get an 'ack' from a DT maintainer for this, those >> those >> are apparently very hard to come by. And I'd really not like to have to >> revisit >> this again in a few weeks. We have patches getting queued up for 4.2 that >> are >> using the "nor-jedec" binding, and I'd like to nip those in the bud ASAP. > > > I am not a DT maintainer, but the DT documentation part of this change: > Acked-by: Stephen Warren <swarren@nvidia.com> Likewise, Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, May 14, 2015 at 06:32:53PM +0100, Brian Norris wrote: > In commit 8ff16cf77ce3 ("Documentation: devicetree: m25p80: add "nor-jedec" > binding"), we added a generic "nor-jedec" binding to catch all > mostly-compatible SPI NOR flash which can be detected via the READ ID > opcode (0x9F). This was discussed and reviewed at the time, however > objections have come up since then as part of this discussion: > > http://lkml.kernel.org/g/20150511224646.GJ32500@ld-irv-0074 > > It seems the parties involved agree that "jedec,spi-nor" does a better > job of capturing the fact that this is SPI-specific, not just any NOR > flash. > > This binding was only merged for v4.1-rc1, so it's still OK to change > the naming. > > At the same time, let's move the documentation to a better name. > > Next up: prune the m25p_ids[] table to the minimal necessary listing, so > we can stop referring to code (drivers/mtd/devices/m25p80.c) from the > documentation. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > Cc: Stephen Warren <swarren@wwwdotorg.org> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Marek Vasut <marex@denx.de> > Cc: Rafał Miłecki <zajec5@gmail.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Pawel Moll <pawel.moll@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> > Cc: Kumar Gala <galak@codeaurora.org> > Cc: devicetree@vger.kernel.org > --- > I'd *really* like to get an 'ack' from a DT maintainer for this, those those > are apparently very hard to come by. And I'd really not like to have to revisit > this again in a few weeks. We have patches getting queued up for 4.2 that are > using the "nor-jedec" binding, and I'd like to nip those in the bud ASAP. This looks sane to me, and doesn't seem controversial. So long as you can get this through before v4.1, you can add my ack: Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark. > > .../devicetree/bindings/mtd/{m25p80.txt => jedec,spi-nor.txt} | 6 +++--- > drivers/mtd/devices/m25p80.c | 6 +++--- > 2 files changed, 6 insertions(+), 6 deletions(-) > rename Documentation/devicetree/bindings/mtd/{m25p80.txt => jedec,spi-nor.txt} (85%) > > diff --git a/Documentation/devicetree/bindings/mtd/m25p80.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > similarity index 85% > rename from Documentation/devicetree/bindings/mtd/m25p80.txt > rename to Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > index f20b111b502a..2bee68103b01 100644 > --- a/Documentation/devicetree/bindings/mtd/m25p80.txt > +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > @@ -8,8 +8,8 @@ Required properties: > 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. > - Must also include "nor-jedec" for any SPI NOR flash that can be > - identified by the JEDEC READ ID opcode (0x9F). > + Must also include "jedec,spi-nor" for any SPI NOR flash that can > + be identified by the JEDEC READ ID opcode (0x9F). > - reg : Chip-Select number > - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at > > @@ -25,7 +25,7 @@ Example: > flash: m25p80@0 { > #address-cells = <1>; > #size-cells = <1>; > - compatible = "spansion,m25p80", "nor-jedec"; > + compatible = "spansion,m25p80", "jedec,spi-nor"; > reg = <0>; > spi-max-frequency = <40000000>; > m25p,fast-read; > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index 7c8b1694a134..3d59ebc16b6e 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -223,7 +223,7 @@ static int m25p_probe(struct spi_device *spi) > */ > if (data && data->type) > flash_name = data->type; > - else if (!strcmp(spi->modalias, "nor-jedec")) > + else if (!strcmp(spi->modalias, "jedec,spi-nor")) > flash_name = NULL; /* auto-detect */ > else > flash_name = spi->modalias; > @@ -255,7 +255,7 @@ static int m25p_remove(struct spi_device *spi) > * since most of these flash are compatible to some extent, and their > * differences can often be differentiated by the JEDEC read-ID command, we > * encourage new users to add support to the spi-nor library, and simply bind > - * against a generic string here (e.g., "nor-jedec"). > + * against a generic string here (e.g., "jedec,spi-nor"). > * > * Many flash names are kept here in this list (as well as in spi-nor.c) to > * keep them available as module aliases for existing platforms. > @@ -305,7 +305,7 @@ static const struct spi_device_id m25p_ids[] = { > * Generic support for SPI NOR that can be identified by the JEDEC READ > * ID opcode (0x9F). Use this, if possible. > */ > - {"nor-jedec"}, > + {"jedec,spi-nor"}, > { }, > }; > MODULE_DEVICE_TABLE(spi, m25p_ids); > -- > 1.9.1 >
On Thu, May 14, 2015 at 10:26:41PM +0200, Geert Uytterhoeven wrote: > On Thu, May 14, 2015 at 7:46 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > > On 05/14/2015 11:32 AM, Brian Norris wrote: > >> In commit 8ff16cf77ce3 ("Documentation: devicetree: m25p80: add > >> "nor-jedec" > >> binding"), we added a generic "nor-jedec" binding to catch all > >> mostly-compatible SPI NOR flash which can be detected via the READ ID > >> opcode (0x9F). This was discussed and reviewed at the time, however > >> objections have come up since then as part of this discussion: > >> > >> http://lkml.kernel.org/g/20150511224646.GJ32500@ld-irv-0074 > >> > >> It seems the parties involved agree that "jedec,spi-nor" does a better > >> job of capturing the fact that this is SPI-specific, not just any NOR > >> flash. > >> > >> This binding was only merged for v4.1-rc1, so it's still OK to change > >> the naming. > >> > >> At the same time, let's move the documentation to a better name. > >> > >> Next up: prune the m25p_ids[] table to the minimal necessary listing, so > >> we can stop referring to code (drivers/mtd/devices/m25p80.c) from the > >> documentation. > > > > There's no need to change the code to update the documentation. Simply paste > > the list of valid device IDs into the documentation. The binding > > documentation needs to be completely standalone anyway. Binding > > documentation should never refer to Linux driver code as part of their > > definition. Of course they shouldn't refer to the driver. That's the main point of my comment. But just because the ID made its way into the driver doesn't mean it's always a useful or necessary DT binding. More below. > > You can never remove the currently-supported device-specific IDs from the > > driver, since existing DTs need to continue working forever, even with > > future drivers/kernels. > > > > The binding document should also always include a complete list of supported > > flash devices. Standard practice is that the DT compatible property contains > > a list of compatible values, starting with the device-specific value, and > > followed by any generic values. All of those values should be standardized > > and specified in the DT documentation, even if the DT binding is written in > > such a way that a compliant driver need only actively match on the generic > > value. The device-specific values may not be used today, but are present in > > case some device-specific workaround needs to be retro-actively > > implemented/enabled, since that needs to happen for existing DTs too. > > Indeed, all supported flash devices should be listed in the DT binding > documentation, so checkpatch can validate dts changes: > > $ scripts/checkpatch.pl -f arch/arm/boot/dts/r8a7791-koelsch.dts > > [...] > > WARNING: DT compatible string "spansion,s25fl512s" appears > un-documented -- check ./Documentation/devicetree/bindings/ > #493: FILE: arch/arm/boot/dts/r8a7791-koelsch.dts:493: > + compatible = "spansion,s25fl512s", "nor-jedec"; But not all entries in m25p80.c are actually used as DT bindings. There are platform devices that share the same mechanism; there are devices which are 99.9% (100%?) compatible with others on the list and don't actually require a separate DT binding at all; and there are entries that were added just for the SW support, while any user was still binding against the original "<manufacturer>,m25p80". Anyway, I'll consider your suggestions, and I'll probably include most or all all the ID strings in the binding documentation in the end. > >> I'd *really* like to get an 'ack' from a DT maintainer for this, those > >> those > >> are apparently very hard to come by. And I'd really not like to have to > >> revisit > >> this again in a few weeks. We have patches getting queued up for 4.2 that > >> are > >> using the "nor-jedec" binding, and I'd like to nip those in the bud ASAP. > > > > > > I am not a DT maintainer, but the DT documentation part of this change: > > Acked-by: Stephen Warren <swarren@nvidia.com> > > Likewise, > Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> Thanks all. I see that I've gotten an ack from Mark as well (thanks!), so I'll apply this (plus the fixup I just sent) to linux-mtd.git and probably pullreq it before the weekend is over. I'll assume Rafal and Geert will take care of following up on their DTS submissions that added "nor-jedec", but let me know if you'd rather me take care of it. I'll try to keep an eye open as we get nearer to the next merge window, to make sure the wrong entries don't get through. BTW, in case I misled anyone previously: I believe there are no "nor-jedec" entries in *.dts for Linus' current tip; there are only entries queued up in linux-next for 4.2, presumably. Brian
On 15 May 2015 at 22:02, Brian Norris <computersforpeace@gmail.com> wrote: > Thanks all. I see that I've gotten an ack from Mark as well (thanks!), > so I'll apply this (plus the fixup I just sent) to linux-mtd.git and > probably pullreq it before the weekend is over. I'll assume Rafal and > Geert will take care of following up on their DTS submissions that added > "nor-jedec", but let me know if you'd rather me take care of it. I'll > try to keep an eye open as we get nearer to the next merge window, to > make sure the wrong entries don't get through. Sure, I will (Cc-ing linux-mtd).
On 05/15/2015 02:02 PM, Brian Norris wrote: > On Thu, May 14, 2015 at 10:26:41PM +0200, Geert Uytterhoeven wrote: >> On Thu, May 14, 2015 at 7:46 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 05/14/2015 11:32 AM, Brian Norris wrote: >>>> In commit 8ff16cf77ce3 ("Documentation: devicetree: m25p80: add >>>> "nor-jedec" >>>> binding"), we added a generic "nor-jedec" binding to catch all >>>> mostly-compatible SPI NOR flash which can be detected via the READ ID >>>> opcode (0x9F). This was discussed and reviewed at the time, however >>>> objections have come up since then as part of this discussion: >>>> >>>> http://lkml.kernel.org/g/20150511224646.GJ32500@ld-irv-0074 >>>> >>>> It seems the parties involved agree that "jedec,spi-nor" does a better >>>> job of capturing the fact that this is SPI-specific, not just any NOR >>>> flash. >>>> >>>> This binding was only merged for v4.1-rc1, so it's still OK to change >>>> the naming. >>>> >>>> At the same time, let's move the documentation to a better name. >>>> >>>> Next up: prune the m25p_ids[] table to the minimal necessary listing, so >>>> we can stop referring to code (drivers/mtd/devices/m25p80.c) from the >>>> documentation. >>> >>> There's no need to change the code to update the documentation. Simply paste >>> the list of valid device IDs into the documentation. The binding >>> documentation needs to be completely standalone anyway. Binding >>> documentation should never refer to Linux driver code as part of their >>> definition. > > Of course they shouldn't refer to the driver. That's the main point of > my comment. But just because the ID made its way into the driver doesn't > mean it's always a useful or necessary DT binding. More below. Yes and no. DT ABI requires that any old DT that worked with an old kernel must continue to work with a new kernel. Thus, any compatible value that was used in an old DT must be supported by any new kernel, and be documented in the binding so that any new driver (e.g. for a new OS) knows to support that same compatible values. Since it's quite possible that people have DTs that aren't checked into the kernel, we must use the set of compatible values that any old driver supports as the list of compatible values to keep supporting and documenting. Of course, if the driver had separate lists of supported devices for the SPI-specific and DT-based instantiation methods, the set of supported compatible values could have been quite small. Unfortunately both I2C and SPI (at least) took shortcuts and allowed DT compatible values to be transformed to remove the vendor ID and match against the I2C/SPI device lists. Hence we do in fact have to document and continue to support every single device type this driver supports.
diff --git a/Documentation/devicetree/bindings/mtd/m25p80.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt similarity index 85% rename from Documentation/devicetree/bindings/mtd/m25p80.txt rename to Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt index f20b111b502a..2bee68103b01 100644 --- a/Documentation/devicetree/bindings/mtd/m25p80.txt +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt @@ -8,8 +8,8 @@ Required properties: 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. - Must also include "nor-jedec" for any SPI NOR flash that can be - identified by the JEDEC READ ID opcode (0x9F). + Must also include "jedec,spi-nor" for any SPI NOR flash that can + be identified by the JEDEC READ ID opcode (0x9F). - reg : Chip-Select number - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at @@ -25,7 +25,7 @@ Example: flash: m25p80@0 { #address-cells = <1>; #size-cells = <1>; - compatible = "spansion,m25p80", "nor-jedec"; + compatible = "spansion,m25p80", "jedec,spi-nor"; reg = <0>; spi-max-frequency = <40000000>; m25p,fast-read; diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index 7c8b1694a134..3d59ebc16b6e 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -223,7 +223,7 @@ static int m25p_probe(struct spi_device *spi) */ if (data && data->type) flash_name = data->type; - else if (!strcmp(spi->modalias, "nor-jedec")) + else if (!strcmp(spi->modalias, "jedec,spi-nor")) flash_name = NULL; /* auto-detect */ else flash_name = spi->modalias; @@ -255,7 +255,7 @@ static int m25p_remove(struct spi_device *spi) * since most of these flash are compatible to some extent, and their * differences can often be differentiated by the JEDEC read-ID command, we * encourage new users to add support to the spi-nor library, and simply bind - * against a generic string here (e.g., "nor-jedec"). + * against a generic string here (e.g., "jedec,spi-nor"). * * Many flash names are kept here in this list (as well as in spi-nor.c) to * keep them available as module aliases for existing platforms. @@ -305,7 +305,7 @@ static const struct spi_device_id m25p_ids[] = { * Generic support for SPI NOR that can be identified by the JEDEC READ * ID opcode (0x9F). Use this, if possible. */ - {"nor-jedec"}, + {"jedec,spi-nor"}, { }, }; MODULE_DEVICE_TABLE(spi, m25p_ids);
In commit 8ff16cf77ce3 ("Documentation: devicetree: m25p80: add "nor-jedec" binding"), we added a generic "nor-jedec" binding to catch all mostly-compatible SPI NOR flash which can be detected via the READ ID opcode (0x9F). This was discussed and reviewed at the time, however objections have come up since then as part of this discussion: http://lkml.kernel.org/g/20150511224646.GJ32500@ld-irv-0074 It seems the parties involved agree that "jedec,spi-nor" does a better job of capturing the fact that this is SPI-specific, not just any NOR flash. This binding was only merged for v4.1-rc1, so it's still OK to change the naming. At the same time, let's move the documentation to a better name. Next up: prune the m25p_ids[] table to the minimal necessary listing, so we can stop referring to code (drivers/mtd/devices/m25p80.c) from the documentation. Signed-off-by: Brian Norris <computersforpeace@gmail.com> Cc: Stephen Warren <swarren@wwwdotorg.org> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Marek Vasut <marex@denx.de> Cc: Rafał Miłecki <zajec5@gmail.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Pawel Moll <pawel.moll@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> Cc: Kumar Gala <galak@codeaurora.org> Cc: devicetree@vger.kernel.org --- I'd *really* like to get an 'ack' from a DT maintainer for this, those those are apparently very hard to come by. And I'd really not like to have to revisit this again in a few weeks. We have patches getting queued up for 4.2 that are using the "nor-jedec" binding, and I'd like to nip those in the bud ASAP. .../devicetree/bindings/mtd/{m25p80.txt => jedec,spi-nor.txt} | 6 +++--- drivers/mtd/devices/m25p80.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) rename Documentation/devicetree/bindings/mtd/{m25p80.txt => jedec,spi-nor.txt} (85%)