| Message ID | 1431624773-4165-1-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Superseded, archived |
| 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> -- 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
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 -- 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
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 > -- 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
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 -- 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
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 Fri, May 15, 2015 at 08:55:41PM +0100, Brian Norris wrote: > It really helps if I test patches... > > On Thu, May 14, 2015 at 10:32:53AM -0700, 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. > > > > .../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/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"}, > > So I forgot (again; we hit this before) that the SPI/OF framework strips > everything before the first comma before binding devices. See > of_modalias_node(). So I'll have to squash in the patch below to get a > usable binding. Is it not possible to use the of_match_table on spi_driver::driver? If not, we really should make it so. Mark. -- 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
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. -- 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
+ linux-spi On Mon, May 18, 2015 at 11:45:01AM +0100, Mark Rutland wrote: > On Fri, May 15, 2015 at 08:55:41PM +0100, Brian Norris wrote: > > It really helps if I test patches... > > > > On Thu, May 14, 2015 at 10:32:53AM -0700, Brian Norris wrote: [...] > > > @@ -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"}, > > > > So I forgot (again; we hit this before) that the SPI/OF framework strips > > everything before the first comma before binding devices. See > > of_modalias_node(). So I'll have to squash in the patch below to get a > > usable binding. > > Is it not possible to use the of_match_table on spi_driver::driver? If > not, we really should make it so. Hmm, it does look like spi.c supports multiple matching mechanisms, so I guess m25p80.c could match some with of_match_table and some with modalias/spi_driver.id_table. See: static int spi_match_device(struct device *dev, struct device_driver *drv) { const struct spi_device *spi = to_spi_device(dev); const struct spi_driver *sdrv = to_spi_driver(drv); /* Attempt an OF style match */ if (of_driver_match_device(dev, drv)) return 1; // ^^^^ we aren't yet (but could be) using this /* Then try ACPI */ if (acpi_driver_match_device(dev, drv)) return 1; if (sdrv->id_table) return !!spi_match_id(sdrv->id_table, spi); // ^^^^ we're currently only using this return strcmp(spi->modalias, drv->name) == 0; } I'll see about patching this for 4.2. We have a working solution for 4.1 at least. Brian -- 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
Hi Brian, On Mon, May 18, 2015 at 8:34 PM, Brian Norris <computersforpeace@gmail.com> wrote: > On Mon, May 18, 2015 at 11:45:01AM +0100, Mark Rutland wrote: >> On Fri, May 15, 2015 at 08:55:41PM +0100, Brian Norris wrote: >> > It really helps if I test patches... >> > >> > On Thu, May 14, 2015 at 10:32:53AM -0700, Brian Norris wrote: > [...] >> > > @@ -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"}, >> > >> > So I forgot (again; we hit this before) that the SPI/OF framework strips >> > everything before the first comma before binding devices. See >> > of_modalias_node(). So I'll have to squash in the patch below to get a >> > usable binding. >> >> Is it not possible to use the of_match_table on spi_driver::driver? If >> not, we really should make it so. > > Hmm, it does look like spi.c supports multiple matching mechanisms, so I > guess m25p80.c could match some with of_match_table and some with > modalias/spi_driver.id_table. See: > > static int spi_match_device(struct device *dev, struct device_driver *drv) > { > const struct spi_device *spi = to_spi_device(dev); > const struct spi_driver *sdrv = to_spi_driver(drv); > > /* Attempt an OF style match */ > if (of_driver_match_device(dev, drv)) > return 1; > // ^^^^ we aren't yet (but could be) using this > > /* Then try ACPI */ > if (acpi_driver_match_device(dev, drv)) > return 1; > > if (sdrv->id_table) > return !!spi_match_id(sdrv->id_table, spi); > // ^^^^ we're currently only using this > > return strcmp(spi->modalias, drv->name) == 0; > } > > I'll see about patching this for 4.2. We have a working solution for > 4.1 at least. When using DT: - spi_driver.id_table is used to match with vendor-stripped DT "compatible" entries. - spi_driver.driver.of_match_table is used to match with full DT "compatible" entries. Note that stripping of vendor names from DT "compatible" entries is done for the _first_ "compatible" entry in the device node only! Hence compatible = "myvendor,m25p80"; will match against "m25p80" in m25p_ids[], while compatible = "myvendor,shinynewdevice", "st,m25p80"; will _not_ match against "m25p80" in m25p_ids[], and fail to probe in the absence of a driver entry for the first (real) "compatible" entry. I2c behaves similarly. 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 -- 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
On 19 May 2015 at 03:34, Brian Norris <computersforpeace@gmail.com> wrote: > So how about the following patch? It seems like we'll need to be able to > ignore useless 'modalias' values in cases like this: > > // modalias = "shinynewdevice" > compatible = "myvendor,shinynewdevice", "jedec,spi-nor"; > > and also if somebody leaves off the entire shinynewdevice string: > > // modalias = "spi-nor" > compatible = "jedec,spi-nor"; > > So we rework the spi-nor library to not reject "bad" names, and just > fall back to autodetection, and we add the .of_match_table to properly > catch all "jedec,spi-nor". That's nice but what about platforms using platform data instead of DT? I would like to use some kind of "spi-nor" (with some prefix *maybe*) for them too. -- 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
On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote: > On 19 May 2015 at 03:34, Brian Norris <computersforpeace@gmail.com> wrote: > > So how about the following patch? It seems like we'll need to be able to > > ignore useless 'modalias' values in cases like this: > > > > // modalias = "shinynewdevice" > > compatible = "myvendor,shinynewdevice", "jedec,spi-nor"; > > > > and also if somebody leaves off the entire shinynewdevice string: > > > > // modalias = "spi-nor" > > compatible = "jedec,spi-nor"; > > > > So we rework the spi-nor library to not reject "bad" names, and just > > fall back to autodetection, and we add the .of_match_table to properly > > catch all "jedec,spi-nor". > > That's nice but what about platforms using platform data instead of > DT? I would like to use some kind of "spi-nor" (with some prefix > *maybe*) for them too. For platform devices, you might as well just use the name of the driver, which is 'm25p80'. Isn't that how most platform devices are matched with drivers? Or if you can propose other clean solutions, I'm all ears. I'm likely to merge this patch for 4.2 unless a better solution comes up, though. Brian -- 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
On 20 May 2015 at 23:35, Brian Norris <computersforpeace@gmail.com> wrote: > On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote: >> On 19 May 2015 at 03:34, Brian Norris <computersforpeace@gmail.com> wrote: >> > So how about the following patch? It seems like we'll need to be able to >> > ignore useless 'modalias' values in cases like this: >> > >> > // modalias = "shinynewdevice" >> > compatible = "myvendor,shinynewdevice", "jedec,spi-nor"; >> > >> > and also if somebody leaves off the entire shinynewdevice string: >> > >> > // modalias = "spi-nor" >> > compatible = "jedec,spi-nor"; >> > >> > So we rework the spi-nor library to not reject "bad" names, and just >> > fall back to autodetection, and we add the .of_match_table to properly >> > catch all "jedec,spi-nor". >> >> That's nice but what about platforms using platform data instead of >> DT? I would like to use some kind of "spi-nor" (with some prefix >> *maybe*) for them too. > > For platform devices, you might as well just use the name of the driver, > which is 'm25p80'. Isn't that how most platform devices are matched with > drivers? Yes and I think it's ugly because it keeps causing the warning about read flash model not matching specified one (m25p80). Are you seriously not going to allow platform stuff *clearly* request flash model detection (JEDEC RDID OP)? Just because they don't use DT?
On Mon, May 18, 2015 at 06:34:15PM -0700, Brian Norris wrote: > So how about the following patch? It seems like we'll need to be able to > ignore useless 'modalias' values in cases like this: > > // modalias = "shinynewdevice" > compatible = "myvendor,shinynewdevice", "jedec,spi-nor"; > > and also if somebody leaves off the entire shinynewdevice string: > > // modalias = "spi-nor" > compatible = "jedec,spi-nor"; > > So we rework the spi-nor library to not reject "bad" names, and just > fall back to autodetection, and we add the .of_match_table to properly > catch all "jedec,spi-nor". > > Signed-off-by: Brian Norris <computerfsorpeace@gmail.com> I realized this conversation ended in essentially an ack from Rafal, with no other comments. So I've pushed this (with some extra commentary) to l2-mtd.git. Holler if there are objections. Thanks, Brian > --- > drivers/mtd/devices/m25p80.c | 18 +++++++++++------- > drivers/mtd/spi-nor/spi-nor.c | 8 ++++---- > 2 files changed, 15 insertions(+), 11 deletions(-) > > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index 3af137f49ac9..30d608775f5a 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -223,8 +223,6 @@ static int m25p_probe(struct spi_device *spi) > */ > if (data && data->type) > flash_name = data->type; > - else if (!strcmp(spi->modalias, "spi-nor")) > - flash_name = NULL; /* auto-detect */ > else > flash_name = spi->modalias; > > @@ -301,19 +299,25 @@ static const struct spi_device_id m25p_ids[] = { > {"w25q128"}, {"w25q256"}, {"cat25c11"}, > {"cat25c03"}, {"cat25c09"}, {"cat25c17"}, {"cat25128"}, > > - /* > - * Generic support for SPI NOR that can be identified by the JEDEC READ > - * ID opcode (0x9F). Use this, if possible. > - */ > - {"spi-nor"}, > { }, > }; > MODULE_DEVICE_TABLE(spi, m25p_ids); > > +static const struct of_device_id m25p_of_table[] = { > + /* > + * Generic compatibility for SPI NOR that can be identified by the > + * JEDEC READ ID opcode (0x9F). Use this, if possible. > + */ > + { .compatible = "jedec,spi-nor" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, m25p_of_table); > + > static struct spi_driver m25p80_driver = { > .driver = { > .name = "m25p80", > .owner = THIS_MODULE, > + .of_match_table = m25p_of_table, > }, > .id_table = m25p_ids, > .probe = m25p_probe, > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 14a5d2325dac..390d6fa0a53f 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1003,11 +1003,11 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) > if (ret) > return ret; > > - /* Try to auto-detect if chip name wasn't specified */ > - if (!name) > - id = spi_nor_read_id(nor); > - else > + if (name) > id = spi_nor_match_id(name); > + /* Try to auto-detect if chip name wasn't specified or not found */ > + if (!id) > + id = spi_nor_read_id(nor); > if (IS_ERR_OR_NULL(id)) > return -ENOENT; > > -- > 1.9.1 > -- 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
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%)