Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Brian Norris]

+ 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

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Geert Uytterhoeven]

Hi Brian,

On Mon, May 18, 2015 at 8:34 PM, Brian Norris
<computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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 linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Brian Norris]

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
> <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.

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 <computerfsorpeace@gmail.com>
---
 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

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Rafał Miłecki]

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.

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Brian Norris]

On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote:
> On 19 May 2015 at 03:34, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Rafał Miłecki]

On 20 May 2015 at 23:35, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Brian Norris]

(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 <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).

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

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Rafał Miłecki]

On 21 May 2015 at 09:25, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8@public.gmane.orgm> 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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Brian Norris]

On Thu, May 21, 2015 at 10:01:05AM +0200, Rafał Miłecki wrote:
> On 21 May 2015 at 09:25, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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).
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Rafał Miłecki]

On 21 May 2015 at 10:15, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, May 21, 2015 at 10:01:05AM +0200, Rafał Miłecki wrote:
>> On 21 May 2015 at 09:25, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 <computersforpeace-Re5JQEeQqe8@public.gmane.orgm> 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).
>> >
>> > 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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Geert Uytterhoeven]

Hi Rafal, Brian,

On Thu, May 21, 2015 at 10:25 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 21 May 2015 at 10:15, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Thu, May 21, 2015 at 10:01:05AM +0200, Rafał Miłecki wrote:
>>> On 21 May 2015 at 09:25, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 -- 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 linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Rafał Miłecki]

On 21 May 2015 at 10:39, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> Hi Rafal, Brian,
>
> On Thu, May 21, 2015 at 10:25 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On 21 May 2015 at 10:15, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On Thu, May 21, 2015 at 10:01:05AM +0200, Rafał Miłecki wrote:
>>>> On 21 May 2015 at 09:25, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Geert Uytterhoeven]

Hi Rafal,

On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <zajec5@gmail.com> 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 -- 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 linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Rafał Miłecki]

On 21 May 2015 at 10:58, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <zajec5@gmail.com> 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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Geert Uytterhoeven]

Hi Rafal,

On Thu, May 21, 2015 at 11:31 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 21 May 2015 at 10:58, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <zajec5@gmail.com> 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 -- 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

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Rafał Miłecki]

On 21 May 2015 at 11:39, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> On Thu, May 21, 2015 at 11:31 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On 21 May 2015 at 10:58, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>>> On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <zajec5@gmail.com> 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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Rafał Miłecki]

On 21 May 2015 at 11:39, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> On Thu, May 21, 2015 at 11:31 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On 21 May 2015 at 10:58, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>>> On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <zajec5@gmail.com> 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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Brian Norris]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html