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 <[email protected]>
Cc: Stephen Warren <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Rafał Miłecki <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Pawel Moll <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Ian Campbell <[email protected]>
Cc: Kumar Gala <[email protected]>
Cc: [email protected]
---
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/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 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 <[email protected]>
On Thu, May 14, 2015 at 7:46 PM, Stephen Warren <[email protected]> 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 <[email protected]>
Likewise,
Acked-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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 <[email protected]>
> Cc: Stephen Warren <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Marek Vasut <[email protected]>
> Cc: Rafał Miłecki <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Pawel Moll <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Ian Campbell <[email protected]>
> Cc: Kumar Gala <[email protected]>
> Cc: [email protected]
> ---
> 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 <[email protected]>
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
>
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 <[email protected]>
> Cc: Stephen Warren <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Marek Vasut <[email protected]>
> Cc: Rafał Miłecki <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Pawel Moll <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Ian Campbell <[email protected]>
> Cc: Kumar Gala <[email protected]>
> Cc: [email protected]
> ---
> 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.
> { },
> };
> MODULE_DEVICE_TABLE(spi, m25p_ids);
This patch will make the code bind against anything like
"<foo>,spi-nor", but we'll leave the comment to recommend
using "jedec,spi-nor".
Signed-off-by: Brian Norris <[email protected]>
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 3d59ebc16b6e..3af137f49ac9 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, "jedec,spi-nor"))
+ else if (!strcmp(spi->modalias, "spi-nor"))
flash_name = NULL; /* auto-detect */
else
flash_name = spi->modalias;
@@ -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.
*/
- {"jedec,spi-nor"},
+ {"spi-nor"},
{ },
};
MODULE_DEVICE_TABLE(spi, m25p_ids);
On Thu, May 14, 2015 at 10:26:41PM +0200, Geert Uytterhoeven wrote:
> On Thu, May 14, 2015 at 7:46 PM, Stephen Warren <[email protected]> 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 <[email protected]>
>
> Likewise,
> Acked-by: Geert Uytterhoeven <[email protected]>
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 <[email protected]> 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).
--
Rafał
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 <[email protected]>
> > Cc: Stephen Warren <[email protected]>
> > Cc: Geert Uytterhoeven <[email protected]>
> > Cc: Marek Vasut <[email protected]>
> > Cc: Rafał Miłecki <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Pawel Moll <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > Cc: Ian Campbell <[email protected]>
> > Cc: Kumar Gala <[email protected]>
> > Cc: [email protected]
> > ---
> > 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.
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 <[email protected]> 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.
+ 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
Hi Brian,
On Mon, May 18, 2015 at 8:34 PM, Brian Norris
<[email protected]> 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 -- [email protected]
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
Hi Geert,
On Mon, May 18, 2015 at 08:51:46PM +0200, Geert Uytterhoeven wrote:
> On Mon, May 18, 2015 at 8:34 PM, Brian Norris
> <[email protected]> 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.
This last part is a little odd, but understandable I guess.
> I2c behaves similarly.
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 <[email protected]>
---
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
On 19 May 2015 at 03:34, Brian Norris <[email protected]> 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.
On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote:
> On 19 May 2015 at 03:34, Brian Norris <[email protected]> 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
On 20 May 2015 at 23:35, Brian Norris <[email protected]> wrote:
> On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote:
>> On 19 May 2015 at 03:34, Brian Norris <[email protected]> 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?
--
Rafał
(trim CC a bit, as this is no longer a DT binding question)
On Thu, May 21, 2015 at 09:12:25AM +0200, Rafał Miłecki wrote:
> On 20 May 2015 at 23:35, Brian Norris <[email protected]> wrote:
> > On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote:
> >> On 19 May 2015 at 03:34, Brian Norris <[email protected]> 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).
Sure, I agree.
> Are you
> seriously not going to allow platform stuff *clearly* request flash
> model detection (JEDEC RDID OP)? Just because they don't use DT?
No, this isn't about "allowing" anything. It's just that my primary
concern was to get the DT binding straightened out properly. Linus'
current tree now has the proper binding, but the m25p80.c code doesn't
quite bind properly. It will work if "jedec,spi-nor" is the first
entry in the compatible property (and so it becomes the 'modalias', but
not second, third, etc. So my patch fixes that properly.
Now, the secondary concern is that you want platform devices to specify
something generic, and that doesn't yield a "found X, expected Y"
message. I'm perfectly fine with fixing that too, if you have a patch
for it. What do you propose?
Brian
On 21 May 2015 at 09:25, Brian Norris <[email protected]> wrote:
> (trim CC a bit, as this is no longer a DT binding question)
>
> On Thu, May 21, 2015 at 09:12:25AM +0200, Rafał Miłecki wrote:
>> On 20 May 2015 at 23:35, Brian Norris <[email protected]> wrote:
>> > On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote:
>> >> On 19 May 2015 at 03:34, Brian Norris <[email protected]> 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).
>
> Sure, I agree.
>
>> Are you
>> seriously not going to allow platform stuff *clearly* request flash
>> model detection (JEDEC RDID OP)? Just because they don't use DT?
>
> No, this isn't about "allowing" anything. It's just that my primary
> concern was to get the DT binding straightened out properly. Linus'
> current tree now has the proper binding, but the m25p80.c code doesn't
> quite bind properly. It will work if "jedec,spi-nor" is the first
> entry in the compatible property (and so it becomes the 'modalias', but
> not second, third, etc. So my patch fixes that properly.
>
> Now, the secondary concern is that you want platform devices to specify
> something generic, and that doesn't yield a "found X, expected Y"
> message. I'm perfectly fine with fixing that too, if you have a patch
> for it. What do you propose?
Maybe I wasn't clear enough. I was going to start using struct
flash_platform_data with
.type = "spi-nor",
but your proposed patch removes support for such name.
While I like matching DT *clearly* against the whole "jedec,spi-nor"
string (really, I'm all for it), I'm confused what I should use for
platform stuff now. I don't have any proposal as my initial plan was
exactly to use this "spi-nor".
I guess I don't want to re-add support for "spi-nor" (as you just
proposed to remove it), so I think I have to bounce the question: what
alternative do you propose?
--
Rafał
On Thu, May 21, 2015 at 10:01:05AM +0200, Rafał Miłecki wrote:
> On 21 May 2015 at 09:25, Brian Norris <[email protected]> wrote:
> > (trim CC a bit, as this is no longer a DT binding question)
> >
> > On Thu, May 21, 2015 at 09:12:25AM +0200, Rafał Miłecki wrote:
> >> On 20 May 2015 at 23:35, Brian Norris <[email protected]> wrote:
> >> > On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote:
> >> >> On 19 May 2015 at 03:34, Brian Norris <[email protected]> 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).
> >
> > Sure, I agree.
> >
> >> Are you
> >> seriously not going to allow platform stuff *clearly* request flash
> >> model detection (JEDEC RDID OP)? Just because they don't use DT?
> >
> > No, this isn't about "allowing" anything. It's just that my primary
> > concern was to get the DT binding straightened out properly. Linus'
> > current tree now has the proper binding, but the m25p80.c code doesn't
> > quite bind properly. It will work if "jedec,spi-nor" is the first
> > entry in the compatible property (and so it becomes the 'modalias', but
> > not second, third, etc. So my patch fixes that properly.
> >
> > Now, the secondary concern is that you want platform devices to specify
> > something generic, and that doesn't yield a "found X, expected Y"
> > message. I'm perfectly fine with fixing that too, if you have a patch
> > for it. What do you propose?
>
> Maybe I wasn't clear enough. I was going to start using struct
> flash_platform_data with
> .type = "spi-nor",
> but your proposed patch removes support for such name.
Ah, OK. So that's the part I was overlooking.
> While I like matching DT *clearly* against the whole "jedec,spi-nor"
> string (really, I'm all for it), I'm confused what I should use for
> platform stuff now. I don't have any proposal as my initial plan was
> exactly to use this "spi-nor".
> I guess I don't want to re-add support for "spi-nor" (as you just
> proposed to remove it),
I wasn't really trying to remove "spi-nor", that was mostly a side
effect.
> so I think I have to bounce the question: what
> alternative do you propose?
I think your comments suggest that I shouldn't be removing "spi-nor"
from m25p_ids[] nor from this block:
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;
So it stays in both m25p_ids[] and .of_match_table.
I suppose that can work. It then allows people to do weird stuff like:
compatible = "idontknowwhatimdoing,spi-nor";
in their device tree. But other than that, there's not much downside I don't
think.
Brian
On 21 May 2015 at 10:15, Brian Norris <[email protected]> wrote:
> On Thu, May 21, 2015 at 10:01:05AM +0200, Rafał Miłecki wrote:
>> On 21 May 2015 at 09:25, Brian Norris <[email protected]> wrote:
>> > (trim CC a bit, as this is no longer a DT binding question)
>> >
>> > On Thu, May 21, 2015 at 09:12:25AM +0200, Rafał Miłecki wrote:
>> >> On 20 May 2015 at 23:35, Brian Norris <[email protected]> wrote:
>> >> > On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote:
>> >> >> On 19 May 2015 at 03:34, Brian Norris <[email protected]> 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).
>> >
>> > Sure, I agree.
>> >
>> >> Are you
>> >> seriously not going to allow platform stuff *clearly* request flash
>> >> model detection (JEDEC RDID OP)? Just because they don't use DT?
>> >
>> > No, this isn't about "allowing" anything. It's just that my primary
>> > concern was to get the DT binding straightened out properly. Linus'
>> > current tree now has the proper binding, but the m25p80.c code doesn't
>> > quite bind properly. It will work if "jedec,spi-nor" is the first
>> > entry in the compatible property (and so it becomes the 'modalias', but
>> > not second, third, etc. So my patch fixes that properly.
>> >
>> > Now, the secondary concern is that you want platform devices to specify
>> > something generic, and that doesn't yield a "found X, expected Y"
>> > message. I'm perfectly fine with fixing that too, if you have a patch
>> > for it. What do you propose?
>>
>> Maybe I wasn't clear enough. I was going to start using struct
>> flash_platform_data with
>> .type = "spi-nor",
>> but your proposed patch removes support for such name.
>
> Ah, OK. So that's the part I was overlooking.
>
>> While I like matching DT *clearly* against the whole "jedec,spi-nor"
>> string (really, I'm all for it), I'm confused what I should use for
>> platform stuff now. I don't have any proposal as my initial plan was
>> exactly to use this "spi-nor".
>> I guess I don't want to re-add support for "spi-nor" (as you just
>> proposed to remove it),
>
> I wasn't really trying to remove "spi-nor", that was mostly a side
> effect.
OK, I think we understand each other now :)
>> so I think I have to bounce the question: what
>> alternative do you propose?
>
> I think your comments suggest that I shouldn't be removing "spi-nor"
> from m25p_ids[] nor from this block:
>
> 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;
>
> So it stays in both m25p_ids[] and .of_match_table.
>
> I suppose that can work. It then allows people to do weird stuff like:
>
> compatible = "idontknowwhatimdoing,spi-nor";
>
> in their device tree. But other than that, there's not much downside I don't
> think.
It sounds like a reasonable solution. I guess there isn't a perfect
one. Even if we decide to go for sth like "jedec-spi-nor", there
always will be a chance of someone using
compatible = "idontknowwhatimdoing,jedec-spi-nor";
So if you rework your patch to leave "spi-nor" support in m25p_ids and
conditions block, it should be OK.
--
Rafał
Hi Rafal, Brian,
On Thu, May 21, 2015 at 10:25 AM, Rafał Miłecki <[email protected]> wrote:
> On 21 May 2015 at 10:15, Brian Norris <[email protected]> wrote:
>> On Thu, May 21, 2015 at 10:01:05AM +0200, Rafał Miłecki wrote:
>>> On 21 May 2015 at 09:25, Brian Norris <[email protected]> wrote:
>>> >> > 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).
>>> >
>>> > Sure, I agree.
>>> >
>>> >> Are you
>>> >> seriously not going to allow platform stuff *clearly* request flash
>>> >> model detection (JEDEC RDID OP)? Just because they don't use DT?
>>> >
>>> > No, this isn't about "allowing" anything. It's just that my primary
>>> > concern was to get the DT binding straightened out properly. Linus'
>>> > current tree now has the proper binding, but the m25p80.c code doesn't
>>> > quite bind properly. It will work if "jedec,spi-nor" is the first
>>> > entry in the compatible property (and so it becomes the 'modalias', but
>>> > not second, third, etc. So my patch fixes that properly.
>>> >
>>> > Now, the secondary concern is that you want platform devices to specify
>>> > something generic, and that doesn't yield a "found X, expected Y"
>>> > message. I'm perfectly fine with fixing that too, if you have a patch
>>> > for it. What do you propose?
>>>
>>> Maybe I wasn't clear enough. I was going to start using struct
>>> flash_platform_data with
>>> .type = "spi-nor",
>>> but your proposed patch removes support for such name.
>>
>> Ah, OK. So that's the part I was overlooking.
>>
>>> While I like matching DT *clearly* against the whole "jedec,spi-nor"
>>> string (really, I'm all for it), I'm confused what I should use for
>>> platform stuff now. I don't have any proposal as my initial plan was
>>> exactly to use this "spi-nor".
>>> I guess I don't want to re-add support for "spi-nor" (as you just
>>> proposed to remove it),
>>
>> I wasn't really trying to remove "spi-nor", that was mostly a side
>> effect.
>
> OK, I think we understand each other now :)
>
>>> so I think I have to bounce the question: what
>>> alternative do you propose?
>>
>> I think your comments suggest that I shouldn't be removing "spi-nor"
>> from m25p_ids[] nor from this block:
>>
>> 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;
>>
>> So it stays in both m25p_ids[] and .of_match_table.
>>
>> I suppose that can work. It then allows people to do weird stuff like:
>>
>> compatible = "idontknowwhatimdoing,spi-nor";
>>
>> in their device tree. But other than that, there's not much downside I don't
>> think.
>
> It sounds like a reasonable solution. I guess there isn't a perfect
> one. Even if we decide to go for sth like "jedec-spi-nor", there
> always will be a chance of someone using
> compatible = "idontknowwhatimdoing,jedec-spi-nor";
> So if you rework your patch to leave "spi-nor" support in m25p_ids and
> conditions block, it should be OK.
Typically platform devices just use the driver's name. Hence IMHO there's
no need to add a shiny new spi-nor device name.
So what's wrong with using "m25p80", and treating that as auto-detect iff
!spi->dev.of_node?
Non-autodetect platform_devices use flash_platform_data.type anyway,
and thus fall under the first "if" clause above, don't they?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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 21 May 2015 at 10:39, Geert Uytterhoeven <[email protected]> wrote:
> Hi Rafal, Brian,
>
> On Thu, May 21, 2015 at 10:25 AM, Rafał Miłecki <[email protected]> wrote:
>> On 21 May 2015 at 10:15, Brian Norris <[email protected]> wrote:
>>> On Thu, May 21, 2015 at 10:01:05AM +0200, Rafał Miłecki wrote:
>>>> On 21 May 2015 at 09:25, Brian Norris <[email protected]> wrote:
>>>> >> > 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).
>>>> >
>>>> > Sure, I agree.
>>>> >
>>>> >> Are you
>>>> >> seriously not going to allow platform stuff *clearly* request flash
>>>> >> model detection (JEDEC RDID OP)? Just because they don't use DT?
>>>> >
>>>> > No, this isn't about "allowing" anything. It's just that my primary
>>>> > concern was to get the DT binding straightened out properly. Linus'
>>>> > current tree now has the proper binding, but the m25p80.c code doesn't
>>>> > quite bind properly. It will work if "jedec,spi-nor" is the first
>>>> > entry in the compatible property (and so it becomes the 'modalias', but
>>>> > not second, third, etc. So my patch fixes that properly.
>>>> >
>>>> > Now, the secondary concern is that you want platform devices to specify
>>>> > something generic, and that doesn't yield a "found X, expected Y"
>>>> > message. I'm perfectly fine with fixing that too, if you have a patch
>>>> > for it. What do you propose?
>>>>
>>>> Maybe I wasn't clear enough. I was going to start using struct
>>>> flash_platform_data with
>>>> .type = "spi-nor",
>>>> but your proposed patch removes support for such name.
>>>
>>> Ah, OK. So that's the part I was overlooking.
>>>
>>>> While I like matching DT *clearly* against the whole "jedec,spi-nor"
>>>> string (really, I'm all for it), I'm confused what I should use for
>>>> platform stuff now. I don't have any proposal as my initial plan was
>>>> exactly to use this "spi-nor".
>>>> I guess I don't want to re-add support for "spi-nor" (as you just
>>>> proposed to remove it),
>>>
>>> I wasn't really trying to remove "spi-nor", that was mostly a side
>>> effect.
>>
>> OK, I think we understand each other now :)
>>
>>>> so I think I have to bounce the question: what
>>>> alternative do you propose?
>>>
>>> I think your comments suggest that I shouldn't be removing "spi-nor"
>>> from m25p_ids[] nor from this block:
>>>
>>> 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;
>>>
>>> So it stays in both m25p_ids[] and .of_match_table.
>>>
>>> I suppose that can work. It then allows people to do weird stuff like:
>>>
>>> compatible = "idontknowwhatimdoing,spi-nor";
>>>
>>> in their device tree. But other than that, there's not much downside I don't
>>> think.
>>
>> It sounds like a reasonable solution. I guess there isn't a perfect
>> one. Even if we decide to go for sth like "jedec-spi-nor", there
>> always will be a chance of someone using
>> compatible = "idontknowwhatimdoing,jedec-spi-nor";
>> So if you rework your patch to leave "spi-nor" support in m25p_ids and
>> conditions block, it should be OK.
>
> Typically platform devices just use the driver's name. Hence IMHO there's
> no need to add a shiny new spi-nor device name.
>
> So what's wrong with using "m25p80", and treating that as auto-detect iff
> !spi->dev.of_node?
Treating "m25p80" as auto-detect triggering string won't allow
platform to *force* "m25p80" flash type if there ever appears to be
needed. Maybe it's unlikely, but it still sounds like a bit bad design
for me.
> Non-autodetect platform_devices use flash_platform_data.type anyway,
> and thus fall under the first "if" clause above, don't they?
They do, but I don't see the point.
--
Rafał
Hi Rafal,
On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <[email protected]> wrote:
>>>> I think your comments suggest that I shouldn't be removing "spi-nor"
>>>> from m25p_ids[] nor from this block:
>>>>
>>>> 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;
>>>>
>>>> So it stays in both m25p_ids[] and .of_match_table.
>>>>
>>>> I suppose that can work. It then allows people to do weird stuff like:
>>>>
>>>> compatible = "idontknowwhatimdoing,spi-nor";
>>>>
>>>> in their device tree. But other than that, there's not much downside I don't
>>>> think.
>>>
>>> It sounds like a reasonable solution. I guess there isn't a perfect
>>> one. Even if we decide to go for sth like "jedec-spi-nor", there
>>> always will be a chance of someone using
>>> compatible = "idontknowwhatimdoing,jedec-spi-nor";
>>> So if you rework your patch to leave "spi-nor" support in m25p_ids and
>>> conditions block, it should be OK.
>>
>> Typically platform devices just use the driver's name. Hence IMHO there's
>> no need to add a shiny new spi-nor device name.
>>
>> So what's wrong with using "m25p80", and treating that as auto-detect iff
>> !spi->dev.of_node?
>
> Treating "m25p80" as auto-detect triggering string won't allow
> platform to *force* "m25p80" flash type if there ever appears to be
> needed. Maybe it's unlikely, but it still sounds like a bit bad design
> for me.
To force m25p80 flash, you set flash_platform_data.type to "m25p80"?
>> Non-autodetect platform_devices use flash_platform_data.type anyway,
>> and thus fall under the first "if" clause above, don't they?
>
> They do, but I don't see the point.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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 21 May 2015 at 10:58, Geert Uytterhoeven <[email protected]> wrote:
> On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <[email protected]> wrote:
>>>>> I think your comments suggest that I shouldn't be removing "spi-nor"
>>>>> from m25p_ids[] nor from this block:
>>>>>
>>>>> 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;
>>>>>
>>>>> So it stays in both m25p_ids[] and .of_match_table.
>>>>>
>>>>> I suppose that can work. It then allows people to do weird stuff like:
>>>>>
>>>>> compatible = "idontknowwhatimdoing,spi-nor";
>>>>>
>>>>> in their device tree. But other than that, there's not much downside I don't
>>>>> think.
>>>>
>>>> It sounds like a reasonable solution. I guess there isn't a perfect
>>>> one. Even if we decide to go for sth like "jedec-spi-nor", there
>>>> always will be a chance of someone using
>>>> compatible = "idontknowwhatimdoing,jedec-spi-nor";
>>>> So if you rework your patch to leave "spi-nor" support in m25p_ids and
>>>> conditions block, it should be OK.
>>>
>>> Typically platform devices just use the driver's name. Hence IMHO there's
>>> no need to add a shiny new spi-nor device name.
>>>
>>> So what's wrong with using "m25p80", and treating that as auto-detect iff
>>> !spi->dev.of_node?
>>
>> Treating "m25p80" as auto-detect triggering string won't allow
>> platform to *force* "m25p80" flash type if there ever appears to be
>> needed. Maybe it's unlikely, but it still sounds like a bit bad design
>> for me.
>
> To force m25p80 flash, you set flash_platform_data.type to "m25p80"?
Oh, I think I got lost in the way m25p80 is probed. Is it handled by
spi_board_info and its "modalias"?
Could I leave flash_platform_data.type set to NULL and still have m25p80 probed?
--
Rafał
Hi Rafal,
On Thu, May 21, 2015 at 11:31 AM, Rafał Miłecki <[email protected]> wrote:
> On 21 May 2015 at 10:58, Geert Uytterhoeven <[email protected]> wrote:
>> On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <[email protected]> wrote:
>>>>>> I think your comments suggest that I shouldn't be removing "spi-nor"
>>>>>> from m25p_ids[] nor from this block:
>>>>>>
>>>>>> 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;
>>>>>>
>>>>>> So it stays in both m25p_ids[] and .of_match_table.
>>>>>>
>>>>>> I suppose that can work. It then allows people to do weird stuff like:
>>>>>>
>>>>>> compatible = "idontknowwhatimdoing,spi-nor";
>>>>>>
>>>>>> in their device tree. But other than that, there's not much downside I don't
>>>>>> think.
>>>>>
>>>>> It sounds like a reasonable solution. I guess there isn't a perfect
>>>>> one. Even if we decide to go for sth like "jedec-spi-nor", there
>>>>> always will be a chance of someone using
>>>>> compatible = "idontknowwhatimdoing,jedec-spi-nor";
>>>>> So if you rework your patch to leave "spi-nor" support in m25p_ids and
>>>>> conditions block, it should be OK.
>>>>
>>>> Typically platform devices just use the driver's name. Hence IMHO there's
>>>> no need to add a shiny new spi-nor device name.
>>>>
>>>> So what's wrong with using "m25p80", and treating that as auto-detect iff
>>>> !spi->dev.of_node?
>>>
>>> Treating "m25p80" as auto-detect triggering string won't allow
>>> platform to *force* "m25p80" flash type if there ever appears to be
>>> needed. Maybe it's unlikely, but it still sounds like a bit bad design
>>> for me.
>>
>> To force m25p80 flash, you set flash_platform_data.type to "m25p80"?
>
> Oh, I think I got lost in the way m25p80 is probed. Is it handled by
> spi_board_info and its "modalias"?
Indeed.
> Could I leave flash_platform_data.type set to NULL and still have m25p80 probed?
Yes, that case is the final "else" branch above.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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 21 May 2015 at 11:39, Geert Uytterhoeven <[email protected]> wrote:
> On Thu, May 21, 2015 at 11:31 AM, Rafał Miłecki <[email protected]> wrote:
>> On 21 May 2015 at 10:58, Geert Uytterhoeven <[email protected]> wrote:
>>> On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <[email protected]> wrote:
>>>>>>> I think your comments suggest that I shouldn't be removing "spi-nor"
>>>>>>> from m25p_ids[] nor from this block:
>>>>>>>
>>>>>>> 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;
>>>>>>>
>>>>>>> So it stays in both m25p_ids[] and .of_match_table.
>>>>>>>
>>>>>>> I suppose that can work. It then allows people to do weird stuff like:
>>>>>>>
>>>>>>> compatible = "idontknowwhatimdoing,spi-nor";
>>>>>>>
>>>>>>> in their device tree. But other than that, there's not much downside I don't
>>>>>>> think.
>>>>>>
>>>>>> It sounds like a reasonable solution. I guess there isn't a perfect
>>>>>> one. Even if we decide to go for sth like "jedec-spi-nor", there
>>>>>> always will be a chance of someone using
>>>>>> compatible = "idontknowwhatimdoing,jedec-spi-nor";
>>>>>> So if you rework your patch to leave "spi-nor" support in m25p_ids and
>>>>>> conditions block, it should be OK.
>>>>>
>>>>> Typically platform devices just use the driver's name. Hence IMHO there's
>>>>> no need to add a shiny new spi-nor device name.
>>>>>
>>>>> So what's wrong with using "m25p80", and treating that as auto-detect iff
>>>>> !spi->dev.of_node?
>>>>
>>>> Treating "m25p80" as auto-detect triggering string won't allow
>>>> platform to *force* "m25p80" flash type if there ever appears to be
>>>> needed. Maybe it's unlikely, but it still sounds like a bit bad design
>>>> for me.
>>>
>>> To force m25p80 flash, you set flash_platform_data.type to "m25p80"?
>>
>> Oh, I think I got lost in the way m25p80 is probed. Is it handled by
>> spi_board_info and its "modalias"?
>
> Indeed.
>
>> Could I leave flash_platform_data.type set to NULL and still have m25p80 probed?
>
> Yes, that case is the final "else" branch above.
Oh, I feel silly now. So I guess Brian's original patch was correct :|
--
Rafał
On 21 May 2015 at 11:39, Geert Uytterhoeven <[email protected]> wrote:
> On Thu, May 21, 2015 at 11:31 AM, Rafał Miłecki <[email protected]> wrote:
>> On 21 May 2015 at 10:58, Geert Uytterhoeven <[email protected]> wrote:
>>> On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <[email protected]> wrote:
>>>>>>> I think your comments suggest that I shouldn't be removing "spi-nor"
>>>>>>> from m25p_ids[] nor from this block:
>>>>>>>
>>>>>>> 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;
>>>>>>>
>>>>>>> So it stays in both m25p_ids[] and .of_match_table.
>>>>>>>
>>>>>>> I suppose that can work. It then allows people to do weird stuff like:
>>>>>>>
>>>>>>> compatible = "idontknowwhatimdoing,spi-nor";
>>>>>>>
>>>>>>> in their device tree. But other than that, there's not much downside I don't
>>>>>>> think.
>>>>>>
>>>>>> It sounds like a reasonable solution. I guess there isn't a perfect
>>>>>> one. Even if we decide to go for sth like "jedec-spi-nor", there
>>>>>> always will be a chance of someone using
>>>>>> compatible = "idontknowwhatimdoing,jedec-spi-nor";
>>>>>> So if you rework your patch to leave "spi-nor" support in m25p_ids and
>>>>>> conditions block, it should be OK.
>>>>>
>>>>> Typically platform devices just use the driver's name. Hence IMHO there's
>>>>> no need to add a shiny new spi-nor device name.
>>>>>
>>>>> So what's wrong with using "m25p80", and treating that as auto-detect iff
>>>>> !spi->dev.of_node?
>>>>
>>>> Treating "m25p80" as auto-detect triggering string won't allow
>>>> platform to *force* "m25p80" flash type if there ever appears to be
>>>> needed. Maybe it's unlikely, but it still sounds like a bit bad design
>>>> for me.
>>>
>>> To force m25p80 flash, you set flash_platform_data.type to "m25p80"?
>>
>> Oh, I think I got lost in the way m25p80 is probed. Is it handled by
>> spi_board_info and its "modalias"?
>
> Indeed.
>
>> Could I leave flash_platform_data.type set to NULL and still have m25p80 probed?
>
> Yes, that case is the final "else" branch above.
Or wait a second, I see the problem. So the last "else" branch does:
flash_name = spi->modalias;
which will result in flash_name getting "m25p80" which will result in
passing "m25p80" model to spi-nor.
So some sort of solution is to replace
if (data && data->type)
flash_name = data->type;
with
if (data)
flash_name = data->type;
which will allow platform stuff to *not* specify type and trigger auto
(JEDEC RD ID) detection.
The only minor problem is we will still need specifying
spi_board_info.platform_data, but 99.9% of code does as it's requires
for setting partitions anyway.
Brian: so I think your initial patch was OK, sorry for all this noise :|
--
Rafał
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 <[email protected]>
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
>