All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mtd: spi-nor: Decouple SPI NOR's device_node from controller device
@ 2015-08-21  9:09 Marek Vasut
  2015-08-21 14:15 ` Jonas Gorski
  2015-09-02  0:38 ` Brian Norris
  0 siblings, 2 replies; 9+ messages in thread
From: Marek Vasut @ 2015-08-21  9:09 UTC (permalink / raw)
  To: linux-mtd; +Cc: Marek Vasut, Brian Norris

The problem this patch is trying to address is such, that SPI NOR flash
devices attached to a dedicated SPI NOR controller cannot read their
properties from the associated struct device_node.

A couple of facts first:
1) Each SPI NOR flash has a struct spi_nor associated with it.
2) Each SPI NOR flash has certain device properties associated
   with it, for example the OF property 'm25p,fast-read' is a
   good pick. These properties are used by the SPI NOR core to
   select which opcodes are sent to such SPI NOR flash. These
   properties are coming from spi_nor .dev->device_node .

The problem is, that for SPI NOR controllers, the struct spi_nor .dev
element points to the struct device of the SPI NOR controller, not the
SPI NOR flash. Therefore, the associated dev->device_node also is the
one of the controller and therefore the SPI NOR core code is trying to
parse the SPI NOR controller's properties, not the properties of the
SPI NOR flash.

Note: The m25p80 driver is not affected, because the controller and
      the flash are the same device, so the associated device_node
      of the controller and the flash are the same.

This patch adjusts the SPI NOR core such that the device_node is not
picked from spi_nor .dev directly, but from a new separate spi_nor .dn
element. This let's the SPI NOR controller drivers set up a different
spi_nor .dn element for each SPI NOR flash.

This patch also fixes the controller drivers to be compatible with
this modification and correctly set the spi_nor .dn element.

This patch is inspired by 5844feeaa4154d1c46d3462c7a4653d22356d8b4
mtd: nand: add common DT init code

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/devices/m25p80.c      | 1 +
 drivers/mtd/spi-nor/fsl-quadspi.c | 1 +
 drivers/mtd/spi-nor/nxp-spifi.c   | 1 +
 include/linux/mtd/spi-nor.h       | 3 +++
 4 files changed, 6 insertions(+)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 9cd3631..725320f 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -202,6 +202,7 @@ static int m25p_probe(struct spi_device *spi)
 
 	nor->dev = &spi->dev;
 	nor->mtd = &flash->mtd;
+	nor->dn = &spi->dev.of_node;
 	nor->priv = flash;
 
 	spi_set_drvdata(spi, flash);
diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index d32b7e0..f918a60 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -1017,6 +1017,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 
 		nor->mtd = mtd;
 		nor->dev = dev;
+		nor->dn = np;
 		nor->priv = q;
 		mtd->priv = nor;
 
diff --git a/drivers/mtd/spi-nor/nxp-spifi.c b/drivers/mtd/spi-nor/nxp-spifi.c
index 9ad1dd0..3ed4564 100644
--- a/drivers/mtd/spi-nor/nxp-spifi.c
+++ b/drivers/mtd/spi-nor/nxp-spifi.c
@@ -334,6 +334,7 @@ static int nxp_spifi_setup_flash(struct nxp_spifi *spifi,
 	spifi->mtd.priv  = &spifi->nor;
 	spifi->nor.mtd   = &spifi->mtd;
 	spifi->nor.dev   = spifi->dev;
+	spifi->nor.dn    = np;
 	spifi->nor.priv  = spifi;
 	spifi->nor.read  = nxp_spifi_read;
 	spifi->nor.write = nxp_spifi_write;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 1bf6f11..333999d 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -169,6 +169,7 @@ enum spi_nor_option_flags {
  * @lock:		[FLASH-SPECIFIC] lock a region of the SPI NOR
  * @unlock:		[FLASH-SPECIFIC] unlock a region of the SPI NOR
  * @priv:		the private data
+ * @dn:			[BOARD-SPECIFIC] device node describing this instance
  */
 struct spi_nor {
 	struct mtd_info		*mtd;
@@ -208,6 +209,8 @@ struct spi_nor {
 	int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 
 	void *priv;
+
+	struct device_node *dn;
 };
 
 /**
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] mtd: spi-nor: Decouple SPI NOR's device_node from controller device
  2015-08-21  9:09 [PATCH] mtd: spi-nor: Decouple SPI NOR's device_node from controller device Marek Vasut
@ 2015-08-21 14:15 ` Jonas Gorski
  2015-08-21 18:34   ` Marek Vasut
  2015-09-02  0:38 ` Brian Norris
  1 sibling, 1 reply; 9+ messages in thread
From: Jonas Gorski @ 2015-08-21 14:15 UTC (permalink / raw)
  To: Marek Vasut; +Cc: MTD Maling List, Brian Norris

Hi,

On Fri, Aug 21, 2015 at 11:09 AM, Marek Vasut <marex@denx.de> wrote:
> The problem this patch is trying to address is such, that SPI NOR flash
> devices attached to a dedicated SPI NOR controller cannot read their
> properties from the associated struct device_node.
>
> A couple of facts first:
> 1) Each SPI NOR flash has a struct spi_nor associated with it.
> 2) Each SPI NOR flash has certain device properties associated
>    with it, for example the OF property 'm25p,fast-read' is a
>    good pick. These properties are used by the SPI NOR core to
>    select which opcodes are sent to such SPI NOR flash. These
>    properties are coming from spi_nor .dev->device_node .

dev->of_node

>
> The problem is, that for SPI NOR controllers, the struct spi_nor .dev
> element points to the struct device of the SPI NOR controller, not the
> SPI NOR flash. Therefore, the associated dev->device_node also is the

dev->of_node

> one of the controller and therefore the SPI NOR core code is trying to
> parse the SPI NOR controller's properties, not the properties of the
> SPI NOR flash.
>
> Note: The m25p80 driver is not affected, because the controller and
>       the flash are the same device, so the associated device_node
>       of the controller and the flash are the same.
>
> This patch adjusts the SPI NOR core such that the device_node is not
> picked from spi_nor .dev directly, but from a new separate spi_nor .dn
> element. This let's the SPI NOR controller drivers set up a different
> spi_nor .dn element for each SPI NOR flash.
>
> This patch also fixes the controller drivers to be compatible with
> this modification and correctly set the spi_nor .dn element.
>
> This patch is inspired by 5844feeaa4154d1c46d3462c7a4653d22356d8b4
> mtd: nand: add common DT init code

I know that this commit named it dn for nand, but IMHO "dn" isn't a
very readable member name, so I would suggest using something with
"node" in it (just using of_node as well seems to be common). I see no
place where the name length might become an issue.

> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c      | 1 +
>  drivers/mtd/spi-nor/fsl-quadspi.c | 1 +
>  drivers/mtd/spi-nor/nxp-spifi.c   | 1 +
>  include/linux/mtd/spi-nor.h       | 3 +++
>  4 files changed, 6 insertions(+)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 9cd3631..725320f 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -202,6 +202,7 @@ static int m25p_probe(struct spi_device *spi)
>
>         nor->dev = &spi->dev;
>         nor->mtd = &flash->mtd;
> +       nor->dn = &spi->dev.of_node;

drivers/mtd/devices/m25p80.c: In function 'm25p_probe':
drivers/mtd/devices/m25p80.c:221:5: warning: assignment from
incompatible pointer type [enabled by default]

this needs to be ... = spi->dev.of_node;

>         nor->priv = flash;
>
>         spi_set_drvdata(spi, flash);
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index d32b7e0..f918a60 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -1017,6 +1017,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>
>                 nor->mtd = mtd;
>                 nor->dev = dev;
> +               nor->dn = np;
>                 nor->priv = q;
>                 mtd->priv = nor;
>
> diff --git a/drivers/mtd/spi-nor/nxp-spifi.c b/drivers/mtd/spi-nor/nxp-spifi.c
> index 9ad1dd0..3ed4564 100644
> --- a/drivers/mtd/spi-nor/nxp-spifi.c
> +++ b/drivers/mtd/spi-nor/nxp-spifi.c
> @@ -334,6 +334,7 @@ static int nxp_spifi_setup_flash(struct nxp_spifi *spifi,
>         spifi->mtd.priv  = &spifi->nor;
>         spifi->nor.mtd   = &spifi->mtd;
>         spifi->nor.dev   = spifi->dev;
> +       spifi->nor.dn    = np;
>         spifi->nor.priv  = spifi;
>         spifi->nor.read  = nxp_spifi_read;
>         spifi->nor.write = nxp_spifi_write;
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 1bf6f11..333999d 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -169,6 +169,7 @@ enum spi_nor_option_flags {
>   * @lock:              [FLASH-SPECIFIC] lock a region of the SPI NOR
>   * @unlock:            [FLASH-SPECIFIC] unlock a region of the SPI NOR
>   * @priv:              the private data
> + * @dn:                        [BOARD-SPECIFIC] device node describing this instance
>   */
>  struct spi_nor {
>         struct mtd_info         *mtd;
> @@ -208,6 +209,8 @@ struct spi_nor {
>         int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>
>         void *priv;
> +
> +       struct device_node *dn;
>  };


Regards
Jonas

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mtd: spi-nor: Decouple SPI NOR's device_node from controller device
  2015-08-21 14:15 ` Jonas Gorski
@ 2015-08-21 18:34   ` Marek Vasut
  2015-08-21 20:49     ` Jonas Gorski
  0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2015-08-21 18:34 UTC (permalink / raw)
  To: Jonas Gorski; +Cc: MTD Maling List, Brian Norris

On Friday, August 21, 2015 at 04:15:11 PM, Jonas Gorski wrote:
> Hi,

Hi,

> On Fri, Aug 21, 2015 at 11:09 AM, Marek Vasut <marex@denx.de> wrote:
> > The problem this patch is trying to address is such, that SPI NOR flash
> > devices attached to a dedicated SPI NOR controller cannot read their
> > properties from the associated struct device_node.
> > 
> > A couple of facts first:
> > 1) Each SPI NOR flash has a struct spi_nor associated with it.
> > 2) Each SPI NOR flash has certain device properties associated
> > 
> >    with it, for example the OF property 'm25p,fast-read' is a
> >    good pick. These properties are used by the SPI NOR core to
> >    select which opcodes are sent to such SPI NOR flash. These
> >    properties are coming from spi_nor .dev->device_node .
> 
> dev->of_node

Guh, right, thanks :)

> > The problem is, that for SPI NOR controllers, the struct spi_nor .dev
> > element points to the struct device of the SPI NOR controller, not the
> > SPI NOR flash. Therefore, the associated dev->device_node also is the
> 
> dev->of_node

Yep, thanks :)

> > one of the controller and therefore the SPI NOR core code is trying to
> > parse the SPI NOR controller's properties, not the properties of the
> > SPI NOR flash.
> > 
> > Note: The m25p80 driver is not affected, because the controller and
> > 
> >       the flash are the same device, so the associated device_node
> >       of the controller and the flash are the same.
> > 
> > This patch adjusts the SPI NOR core such that the device_node is not
> > picked from spi_nor .dev directly, but from a new separate spi_nor .dn
> > element. This let's the SPI NOR controller drivers set up a different
> > spi_nor .dn element for each SPI NOR flash.
> > 
> > This patch also fixes the controller drivers to be compatible with
> > this modification and correctly set the spi_nor .dn element.
> > 
> > This patch is inspired by 5844feeaa4154d1c46d3462c7a4653d22356d8b4
> > mtd: nand: add common DT init code
> 
> I know that this commit named it dn for nand, but IMHO "dn" isn't a
> very readable member name, so I would suggest using something with
> "node" in it (just using of_node as well seems to be common). I see no
> place where the name length might become an issue.

I thought .dn was supposed to be abbrev for device_node ;-)

> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Brian Norris <computersforpeace@gmail.com>
> > ---
> > 
> >  drivers/mtd/devices/m25p80.c      | 1 +
> >  drivers/mtd/spi-nor/fsl-quadspi.c | 1 +
> >  drivers/mtd/spi-nor/nxp-spifi.c   | 1 +
> >  include/linux/mtd/spi-nor.h       | 3 +++
> >  4 files changed, 6 insertions(+)
> > 
> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > index 9cd3631..725320f 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -202,6 +202,7 @@ static int m25p_probe(struct spi_device *spi)
> > 
> >         nor->dev = &spi->dev;
> >         nor->mtd = &flash->mtd;
> > 
> > +       nor->dn = &spi->dev.of_node;
> 
> drivers/mtd/devices/m25p80.c: In function 'm25p_probe':
> drivers/mtd/devices/m25p80.c:221:5: warning: assignment from
> incompatible pointer type [enabled by default]
> 
> this needs to be ... = spi->dev.of_node;

Right, thanks!

I'll wait for a bit to figure out if this approach is sensible and then
repost with the fixes.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mtd: spi-nor: Decouple SPI NOR's device_node from controller device
  2015-08-21 18:34   ` Marek Vasut
@ 2015-08-21 20:49     ` Jonas Gorski
  2015-08-21 21:16       ` Marek Vasut
  0 siblings, 1 reply; 9+ messages in thread
From: Jonas Gorski @ 2015-08-21 20:49 UTC (permalink / raw)
  To: Marek Vasut; +Cc: Brian Norris, MTD Maling List

On Fri, Aug 21, 2015 at 8:34 PM, Marek Vasut <marex@denx.de> wrote:
> On Friday, August 21, 2015 at 04:15:11 PM, Jonas Gorski wrote:
>> > This patch is inspired by 5844feeaa4154d1c46d3462c7a4653d22356d8b4
>> > mtd: nand: add common DT init code
>>
>> I know that this commit named it dn for nand, but IMHO "dn" isn't a
>> very readable member name, so I would suggest using something with
>> "node" in it (just using of_node as well seems to be common). I see no
>> place where the name length might become an issue.
>
> I thought .dn was supposed to be abbrev for device_node ;-)

Sure, if you know what it is supposed to stand for it is obvious ;-).
And from a "stylistic" point of view, struct spi_nor has members
called page_size, flash_read or cmd_buf and not ps, fr, or cb so using
dn instead of e.g. dev_node seems a bit odd.


Jonas

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mtd: spi-nor: Decouple SPI NOR's device_node from controller device
  2015-08-21 20:49     ` Jonas Gorski
@ 2015-08-21 21:16       ` Marek Vasut
  2015-09-02  0:30         ` Brian Norris
  0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2015-08-21 21:16 UTC (permalink / raw)
  To: Jonas Gorski; +Cc: Brian Norris, MTD Maling List

On Friday, August 21, 2015 at 10:49:46 PM, Jonas Gorski wrote:
> On Fri, Aug 21, 2015 at 8:34 PM, Marek Vasut <marex@denx.de> wrote:
> > On Friday, August 21, 2015 at 04:15:11 PM, Jonas Gorski wrote:
> >> > This patch is inspired by 5844feeaa4154d1c46d3462c7a4653d22356d8b4
> >> > mtd: nand: add common DT init code
> >> 
> >> I know that this commit named it dn for nand, but IMHO "dn" isn't a
> >> very readable member name, so I would suggest using something with
> >> "node" in it (just using of_node as well seems to be common). I see no
> >> place where the name length might become an issue.
> > 
> > I thought .dn was supposed to be abbrev for device_node ;-)
> 
> Sure, if you know what it is supposed to stand for it is obvious ;-).
> And from a "stylistic" point of view, struct spi_nor has members
> called page_size, flash_read or cmd_buf and not ps, fr, or cb so using
> dn instead of e.g. dev_node seems a bit odd.

On the other hand, the .dn is consistent across the MTD subsystem.
I don't have a strong prefference though.

Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mtd: spi-nor: Decouple SPI NOR's device_node from controller device
  2015-08-21 21:16       ` Marek Vasut
@ 2015-09-02  0:30         ` Brian Norris
  2015-09-03 16:20           ` Marek Vasut
  0 siblings, 1 reply; 9+ messages in thread
From: Brian Norris @ 2015-09-02  0:30 UTC (permalink / raw)
  To: Marek Vasut; +Cc: Jonas Gorski, MTD Maling List

On Fri, Aug 21, 2015 at 11:16:21PM +0200, Marek Vasut wrote:
> On Friday, August 21, 2015 at 10:49:46 PM, Jonas Gorski wrote:
> > On Fri, Aug 21, 2015 at 8:34 PM, Marek Vasut <marex@denx.de> wrote:
> > > On Friday, August 21, 2015 at 04:15:11 PM, Jonas Gorski wrote:
> > >> > This patch is inspired by 5844feeaa4154d1c46d3462c7a4653d22356d8b4
> > >> > mtd: nand: add common DT init code
> > >> 
> > >> I know that this commit named it dn for nand, but IMHO "dn" isn't a
> > >> very readable member name, so I would suggest using something with
> > >> "node" in it (just using of_node as well seems to be common). I see no
> > >> place where the name length might become an issue.
> > > 
> > > I thought .dn was supposed to be abbrev for device_node ;-)
> > 
> > Sure, if you know what it is supposed to stand for it is obvious ;-).
> > And from a "stylistic" point of view, struct spi_nor has members
> > called page_size, flash_read or cmd_buf and not ps, fr, or cb so using
> > dn instead of e.g. dev_node seems a bit odd.
> 
> On the other hand, the .dn is consistent across the MTD subsystem.
> I don't have a strong prefference though.

I just stuck in nand_chip::dn since we needed something, and because
I've seen it used as a function parameter name and a local variable name
all over the place, enough that it just seemed natural. But a field name
is probably a bit more important. I'd be OK with making the "standard"
a bit more verbose, and maybe even changing the one in struct nand_chip.

How about:

  flash_node <-- this one leaves room for a controller node, if we
                 eventually need it
  device_node
  dev_node
  of_node <-- this one is commonly used, but mostly out of legacy
              reasons. We're not really dealing with Open Firmware

?

Pick one and run with it.

Brian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mtd: spi-nor: Decouple SPI NOR's device_node from controller device
  2015-08-21  9:09 [PATCH] mtd: spi-nor: Decouple SPI NOR's device_node from controller device Marek Vasut
  2015-08-21 14:15 ` Jonas Gorski
@ 2015-09-02  0:38 ` Brian Norris
  2015-09-03 16:21   ` Marek Vasut
  1 sibling, 1 reply; 9+ messages in thread
From: Brian Norris @ 2015-09-02  0:38 UTC (permalink / raw)
  To: Marek Vasut; +Cc: linux-mtd

On Fri, Aug 21, 2015 at 11:09:20AM +0200, Marek Vasut wrote:
> The problem this patch is trying to address is such, that SPI NOR flash
> devices attached to a dedicated SPI NOR controller cannot read their
> properties from the associated struct device_node.
> 
> A couple of facts first:
> 1) Each SPI NOR flash has a struct spi_nor associated with it.
> 2) Each SPI NOR flash has certain device properties associated
>    with it, for example the OF property 'm25p,fast-read' is a
>    good pick. These properties are used by the SPI NOR core to
>    select which opcodes are sent to such SPI NOR flash. These
>    properties are coming from spi_nor .dev->device_node .
> 
> The problem is, that for SPI NOR controllers, the struct spi_nor .dev
> element points to the struct device of the SPI NOR controller, not the
> SPI NOR flash. Therefore, the associated dev->device_node also is the
> one of the controller and therefore the SPI NOR core code is trying to
> parse the SPI NOR controller's properties, not the properties of the
> SPI NOR flash.
> 
> Note: The m25p80 driver is not affected, because the controller and
>       the flash are the same device, so the associated device_node
>       of the controller and the flash are the same.
> 
> This patch adjusts the SPI NOR core such that the device_node is not
> picked from spi_nor .dev directly, but from a new separate spi_nor .dn
> element.

Does it really?

[...]
> ---
>  drivers/mtd/devices/m25p80.c      | 1 +
>  drivers/mtd/spi-nor/fsl-quadspi.c | 1 +
>  drivers/mtd/spi-nor/nxp-spifi.c   | 1 +
>  include/linux/mtd/spi-nor.h       | 3 +++
>  4 files changed, 6 insertions(+)

^^ I would expect to see spi-nor.c changes in the diffstat. Did you miss
something?

[...]

> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 1bf6f11..333999d 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -169,6 +169,7 @@ enum spi_nor_option_flags {
>   * @lock:		[FLASH-SPECIFIC] lock a region of the SPI NOR
>   * @unlock:		[FLASH-SPECIFIC] unlock a region of the SPI NOR
>   * @priv:		the private data
> + * @dn:			[BOARD-SPECIFIC] device node describing this instance

I might like to see this up near the @dev entry, for organization
purposes.

>   */
>  struct spi_nor {
>  	struct mtd_info		*mtd;
> @@ -208,6 +209,8 @@ struct spi_nor {
>  	int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>  
>  	void *priv;
> +
> +	struct device_node *dn;

And this would get moved up as well.

>  };
>  
>  /**

Brian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mtd: spi-nor: Decouple SPI NOR's device_node from controller device
  2015-09-02  0:30         ` Brian Norris
@ 2015-09-03 16:20           ` Marek Vasut
  0 siblings, 0 replies; 9+ messages in thread
From: Marek Vasut @ 2015-09-03 16:20 UTC (permalink / raw)
  To: Brian Norris; +Cc: Jonas Gorski, MTD Maling List

On Wednesday, September 02, 2015 at 02:30:17 AM, Brian Norris wrote:
> On Fri, Aug 21, 2015 at 11:16:21PM +0200, Marek Vasut wrote:
> > On Friday, August 21, 2015 at 10:49:46 PM, Jonas Gorski wrote:
> > > On Fri, Aug 21, 2015 at 8:34 PM, Marek Vasut <marex@denx.de> wrote:
> > > > On Friday, August 21, 2015 at 04:15:11 PM, Jonas Gorski wrote:
> > > >> > This patch is inspired by 5844feeaa4154d1c46d3462c7a4653d22356d8b4
> > > >> > mtd: nand: add common DT init code
> > > >> 
> > > >> I know that this commit named it dn for nand, but IMHO "dn" isn't a
> > > >> very readable member name, so I would suggest using something with
> > > >> "node" in it (just using of_node as well seems to be common). I see
> > > >> no place where the name length might become an issue.
> > > > 
> > > > I thought .dn was supposed to be abbrev for device_node ;-)
> > > 
> > > Sure, if you know what it is supposed to stand for it is obvious ;-).
> > > And from a "stylistic" point of view, struct spi_nor has members
> > > called page_size, flash_read or cmd_buf and not ps, fr, or cb so using
> > > dn instead of e.g. dev_node seems a bit odd.
> > 
> > On the other hand, the .dn is consistent across the MTD subsystem.
> > I don't have a strong prefference though.
> 
> I just stuck in nand_chip::dn since we needed something, and because
> I've seen it used as a function parameter name and a local variable name
> all over the place, enough that it just seemed natural. But a field name
> is probably a bit more important. I'd be OK with making the "standard"
> a bit more verbose, and maybe even changing the one in struct nand_chip.
> 
> How about:
> 
>   flash_node <-- this one leaves room for a controller node, if we
>                  eventually need it

I like this one the most, exactly because of your reasoning.

>   device_node
>   dev_node
>   of_node <-- this one is commonly used, but mostly out of legacy
>               reasons. We're not really dealing with Open Firmware
> 
> ?
> 
> Pick one and run with it.
> 
> Brian

Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mtd: spi-nor: Decouple SPI NOR's device_node from controller device
  2015-09-02  0:38 ` Brian Norris
@ 2015-09-03 16:21   ` Marek Vasut
  0 siblings, 0 replies; 9+ messages in thread
From: Marek Vasut @ 2015-09-03 16:21 UTC (permalink / raw)
  To: Brian Norris; +Cc: linux-mtd

On Wednesday, September 02, 2015 at 02:38:14 AM, Brian Norris wrote:
> On Fri, Aug 21, 2015 at 11:09:20AM +0200, Marek Vasut wrote:
> > The problem this patch is trying to address is such, that SPI NOR flash
> > devices attached to a dedicated SPI NOR controller cannot read their
> > properties from the associated struct device_node.
> > 
> > A couple of facts first:
> > 1) Each SPI NOR flash has a struct spi_nor associated with it.
> > 2) Each SPI NOR flash has certain device properties associated
> > 
> >    with it, for example the OF property 'm25p,fast-read' is a
> >    good pick. These properties are used by the SPI NOR core to
> >    select which opcodes are sent to such SPI NOR flash. These
> >    properties are coming from spi_nor .dev->device_node .
> > 
> > The problem is, that for SPI NOR controllers, the struct spi_nor .dev
> > element points to the struct device of the SPI NOR controller, not the
> > SPI NOR flash. Therefore, the associated dev->device_node also is the
> > one of the controller and therefore the SPI NOR core code is trying to
> > parse the SPI NOR controller's properties, not the properties of the
> > SPI NOR flash.
> > 
> > Note: The m25p80 driver is not affected, because the controller and
> > 
> >       the flash are the same device, so the associated device_node
> >       of the controller and the flash are the same.
> > 
> > This patch adjusts the SPI NOR core such that the device_node is not
> > picked from spi_nor .dev directly, but from a new separate spi_nor .dn
> > element.
> 
> Does it really?

Yes, it does, but I missed a bit from this patch, sorry.

> [...]
> 
> > ---
> > 
> >  drivers/mtd/devices/m25p80.c      | 1 +
> >  drivers/mtd/spi-nor/fsl-quadspi.c | 1 +
> >  drivers/mtd/spi-nor/nxp-spifi.c   | 1 +
> >  include/linux/mtd/spi-nor.h       | 3 +++
> >  4 files changed, 6 insertions(+)
> 
> ^^ I would expect to see spi-nor.c changes in the diffstat. Did you miss
> something?

Yes, sorry.

> [...]
> 
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index 1bf6f11..333999d 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -169,6 +169,7 @@ enum spi_nor_option_flags {
> > 
> >   * @lock:		[FLASH-SPECIFIC] lock a region of the SPI NOR
> >   * @unlock:		[FLASH-SPECIFIC] unlock a region of the SPI NOR
> >   * @priv:		the private data
> > 
> > + * @dn:			[BOARD-SPECIFIC] device node describing this 
instance
> 
> I might like to see this up near the @dev entry, for organization
> purposes.

Right, will do in V2.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-09-03 16:31 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-21  9:09 [PATCH] mtd: spi-nor: Decouple SPI NOR's device_node from controller device Marek Vasut
2015-08-21 14:15 ` Jonas Gorski
2015-08-21 18:34   ` Marek Vasut
2015-08-21 20:49     ` Jonas Gorski
2015-08-21 21:16       ` Marek Vasut
2015-09-02  0:30         ` Brian Norris
2015-09-03 16:20           ` Marek Vasut
2015-09-02  0:38 ` Brian Norris
2015-09-03 16:21   ` Marek Vasut

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.