[RFC PATCH v2] mtd: rawnand: ams-delta: Use GPIO API for data I/O

[RFC PATCH v2] mtd: rawnand: ams-delta: Use GPIO API for data I/O

[Janusz Krzysztofik]

Implement the idea suggested by Artem Bityutskiy and Tony Lindgren,
described in commit b027274d2e3a ("mtd: ams-delta: fix
request_mem_region() failure"). Use pure GPIO API as suggested by
Boris Brezillon.


Janusz Krzysztofik (12):
      mtd: rawnand: ams-delta: Assign mtd->dev.parent, not mtd->owner
      mtd: rawnand: ams-delta: Use private structure
      ARM: OMAP1: ams-delta: Provide GPIO lookup table for NAND data port
      mtd: rawnand: ams-delta: request data port GPIO resource
      mtd: rawnand: ams-delta: use GPIO API for data read/write
      ARM: OMAP1: ams-delta: drop obsolete NAND resources
      mtd: rawnand: ams-delta: Set port direction once per transfer
      mtd: rawnand: ams-delta: Simplify pointer resolution on read/write
      gpiolib: Identify GPIO descriptor arrays with direct mapping
      gpiolib: Introduce bitmap get/set array API extension
      mtd: rawnand: ams-delta: Use GPIO API bitmap extension
      gpiolib: Add fast processing path to bitmap API functions


Changelog:
v2:
[RFC PATCH v2 00/12] mtd: rawnand: ams-delta: Use GPIO API for data I/O
- renamed from former [RFC PATCH 0/8] mtd: rawnand: ams-delta: Use
  gpio-omap accessors for data I/O
[RFC PATCH v2 01/12] mtd: rawnand: ams-delta: Assign mtd->dev.parent,
			not mtd->owner
- split out from former [RFC PATCH 1/8] on Boris request, thanks.
[RFC PATCH v2 02/12] mtd: rawnand: ams-delta: Use private structure
- remaining part of the former [RFC PATCH 1/8].
[RFC PATCH v2 03/12] ARM: OMAP1: ams-delta: Provide GPIO lookup table
			for NAND data port
- split out from former [RFC PATCH 5/8] on Boris requesst, thanks,
[RFC PATCH v2 04/12] mtd: rawnand: ams-delta: request data port GPIO resource
- remaining part of the former [RFC PATCH 5/8],
[RFC PATCH v2 05/12] mtd: rawnand: ams-delta: use GPIO API for data read/write
- reworked from former [RFC PATCH 8/8] on Boris requesst to use pure
  GPIO API, thanks,
- moved up in front of former [RFC PATCH 3/8] on Boris request, thanks.
[RFC PATCH v2 06/12] ARM: OMAP1: ams-delta: drop obsolete NAND resources
- split out from former [RFC PATCH 8/8].
[RFC PATCH v2 07/12] mtd: rawnand: ams-delta: Set port direction once per
			transfer
- reworked from former [RFC PATCH 3/8] on top of [RFC PATCH v2 05/12].
[RFC PATCH v2 08/12] mtd: rawnand: ams-delta: Simplify pointer resolution
			on read/write
- reworked from former [RFC PATCH 4/8] on top of [RFC PATCH v2 08/12],
- renamed from 'Optimize' to 'Simplify' on Boris request, thanks.
[RFC PATCH v2 09/12] gpiolib: Identify GPIO descriptor arrays with direct
			mapping
- new patch.
[RFC PATCH v2 10/12] gpiolib: Introduce bitmap get/set array API extension
- new patch.
[RFC PATCH v2 11/12] mtd: rawnand: ams-delta: Use GPIO API bitmap extension
- new patch.
[RFC PATCH v2 12/12] gpiolib: Add fast processing path to bitmap API functions
- new patch.
Removed from the series:
[RFC PATCH 2/8] mtd: rawnand: ams-delta: Write protect device during probe
- postponed on Boris request, thanks.
[RFC PATCH 6/8] gpio: omap: Add get/set_multiple() callbacks
- already applied by Linux in linux-gpio tree, thanks.
[RFC PATCH 7/8] mtd: rawnand: ams-delta: Check sanity of data GPIO resource
- most controversial one, no longer needed after switching to GPIO API.


diffstat:
 Documentation/driver-api/gpio/consumer.rst |   36 ++
 arch/arm/mach-omap1/board-ams-delta.c      |   22 -
 drivers/gpio/gpiolib.c                     |  237 +++++++++++++++++++
 drivers/mtd/nand/raw/ams-delta.c           |  350 +++++++++++++++--------------
 include/linux/gpio/consumer.h              |   15 +
 5 files changed, 485 insertions(+), 175 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v2 01/12] mtd: rawnand: ams-delta: Assign mtd->dev.parent, not mtd->owner

[Janusz Krzysztofik]

Fix missing mtd->dev.parent assignment and drop useless mtd->owner.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/mtd/nand/raw/ams-delta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index 2a8872ebd14a..af313c620264 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -162,7 +162,7 @@ static int ams_delta_init(struct platform_device *pdev)
 	}
 
 	ams_delta_mtd = nand_to_mtd(this);
-	ams_delta_mtd->owner = THIS_MODULE;
+	ams_delta_mtd->dev.parent = &pdev->dev;
 
 	/*
 	 * Don't try to request the memory region from here,
-- 
2.16.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 01/12] mtd: rawnand: ams-delta: Assign mtd->dev.parent, not mtd->owner

[Marek Vasut]

On 08/07/2018 12:29 AM, Janusz Krzysztofik wrote:
> Fix missing mtd->dev.parent assignment and drop useless mtd->owner.

You fail to explain why this fix is required.

> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
>  drivers/mtd/nand/raw/ams-delta.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
> index 2a8872ebd14a..af313c620264 100644
> --- a/drivers/mtd/nand/raw/ams-delta.c
> +++ b/drivers/mtd/nand/raw/ams-delta.c
> @@ -162,7 +162,7 @@ static int ams_delta_init(struct platform_device *pdev)
>  	}
>  
>  	ams_delta_mtd = nand_to_mtd(this);
> -	ams_delta_mtd->owner = THIS_MODULE;
> +	ams_delta_mtd->dev.parent = &pdev->dev;
>  
>  	/*
>  	 * Don't try to request the memory region from here,
> 


-- 
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 01/12] mtd: rawnand: ams-delta: Assign mtd->dev.parent, not mtd->owner

[Janusz Krzysztofik]

Hi Marek,

On Tuesday, August 7, 2018 1:54:10 AM CEST Marek Vasut wrote:
> On 08/07/2018 12:29 AM, Janusz Krzysztofik wrote:
> > Fix missing mtd->dev.parent assignment and drop useless mtd->owner.
> 
> You fail to explain why this fix is required.

OK, I'll have a look at similar patches from the past and add an explanation.

Thanks,
Janusz


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 01/12] mtd: rawnand: ams-delta: Assign mtd->dev.parent, not mtd->owner

[Boris Brezillon]

On Tue,  7 Aug 2018 00:29:07 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Fix missing mtd->dev.parent assignment and drop useless mtd->owner.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
>  drivers/mtd/nand/raw/ams-delta.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
> index 2a8872ebd14a..af313c620264 100644
> --- a/drivers/mtd/nand/raw/ams-delta.c
> +++ b/drivers/mtd/nand/raw/ams-delta.c
> @@ -162,7 +162,7 @@ static int ams_delta_init(struct platform_device *pdev)
>  	}
>  
>  	ams_delta_mtd = nand_to_mtd(this);
> -	ams_delta_mtd->owner = THIS_MODULE;
> +	ams_delta_mtd->dev.parent = &pdev->dev;
>  
>  	/*
>  	 * Don't try to request the memory region from here,

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v2 02/12] mtd: rawnand: ams-delta: Use private structure

[Janusz Krzysztofik]

Introduce a driver private structure and allocate it on device probe.
Use it for storing nand_chip structure, GPIO descriptors prevoiusly
stored in static variables as well as io_base pointer previously passed
as nand controller data or platform driver data.  Subsequent patches
may populate the structure with more members as needed.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/mtd/nand/raw/ams-delta.c | 126 +++++++++++++++++++++------------------
 1 file changed, 69 insertions(+), 57 deletions(-)

diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index af313c620264..48233d638d2a 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -34,14 +34,18 @@
 /*
  * MTD structure for E3 (Delta)
  */
-static struct mtd_info *ams_delta_mtd = NULL;
-static struct gpio_desc *gpiod_rdy;
-static struct gpio_desc *gpiod_nce;
-static struct gpio_desc *gpiod_nre;
-static struct gpio_desc *gpiod_nwp;
-static struct gpio_desc *gpiod_nwe;
-static struct gpio_desc *gpiod_ale;
-static struct gpio_desc *gpiod_cle;
+
+struct ams_delta_nand {
+	struct nand_chip	nand_chip;
+	struct gpio_desc	*gpiod_rdy;
+	struct gpio_desc	*gpiod_nce;
+	struct gpio_desc	*gpiod_nre;
+	struct gpio_desc	*gpiod_nwp;
+	struct gpio_desc	*gpiod_nwe;
+	struct gpio_desc	*gpiod_ale;
+	struct gpio_desc	*gpiod_cle;
+	void __iomem		*io_base;
+};
 
 /*
  * Define partitions for flash devices
@@ -71,26 +75,28 @@ static const struct mtd_partition partition_info[] = {
 static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
 {
 	struct nand_chip *this = mtd_to_nand(mtd);
-	void __iomem *io_base = (void __iomem *)nand_get_controller_data(this);
+	struct ams_delta_nand *priv = nand_get_controller_data(this);
+	void __iomem *io_base = priv->io_base;
 
 	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
 	writew(byte, this->IO_ADDR_W);
-	gpiod_set_value(gpiod_nwe, 0);
+	gpiod_set_value(priv->gpiod_nwe, 0);
 	ndelay(40);
-	gpiod_set_value(gpiod_nwe, 1);
+	gpiod_set_value(priv->gpiod_nwe, 1);
 }
 
 static u_char ams_delta_read_byte(struct mtd_info *mtd)
 {
 	u_char res;
 	struct nand_chip *this = mtd_to_nand(mtd);
-	void __iomem *io_base = (void __iomem *)nand_get_controller_data(this);
+	struct ams_delta_nand *priv = nand_get_controller_data(this);
+	void __iomem *io_base = priv->io_base;
 
-	gpiod_set_value(gpiod_nre, 0);
+	gpiod_set_value(priv->gpiod_nre, 0);
 	ndelay(40);
 	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
 	res = readw(this->IO_ADDR_R);
-	gpiod_set_value(gpiod_nre, 1);
+	gpiod_set_value(priv->gpiod_nre, 1);
 
 	return res;
 }
@@ -123,11 +129,13 @@ static void ams_delta_read_buf(struct mtd_info *mtd, u_char *buf, int len)
 static void ams_delta_hwcontrol(struct mtd_info *mtd, int cmd,
 				unsigned int ctrl)
 {
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct ams_delta_nand *priv = nand_get_controller_data(this);
 
 	if (ctrl & NAND_CTRL_CHANGE) {
-		gpiod_set_value(gpiod_nce, !(ctrl & NAND_NCE));
-		gpiod_set_value(gpiod_cle, !!(ctrl & NAND_CLE));
-		gpiod_set_value(gpiod_ale, !!(ctrl & NAND_ALE));
+		gpiod_set_value(priv->gpiod_nce, !(ctrl & NAND_NCE));
+		gpiod_set_value(priv->gpiod_cle, !!(ctrl & NAND_CLE));
+		gpiod_set_value(priv->gpiod_ale, !!(ctrl & NAND_ALE));
 	}
 
 	if (cmd != NAND_CMD_NONE)
@@ -136,7 +144,10 @@ static void ams_delta_hwcontrol(struct mtd_info *mtd, int cmd,
 
 static int ams_delta_nand_ready(struct mtd_info *mtd)
 {
-	return gpiod_get_value(gpiod_rdy);
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct ams_delta_nand *priv = nand_get_controller_data(this);
+
+	return gpiod_get_value(priv->gpiod_rdy);
 }
 
 
@@ -145,7 +156,9 @@ static int ams_delta_nand_ready(struct mtd_info *mtd)
  */
 static int ams_delta_init(struct platform_device *pdev)
 {
+	struct ams_delta_nand *priv;
 	struct nand_chip *this;
+	struct mtd_info *mtd;
 	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	void __iomem *io_base;
 	int err = 0;
@@ -154,15 +167,16 @@ static int ams_delta_init(struct platform_device *pdev)
 		return -ENXIO;
 
 	/* Allocate memory for MTD device structure and private data */
-	this = kzalloc(sizeof(struct nand_chip), GFP_KERNEL);
-	if (!this) {
+	priv = devm_kzalloc(&pdev->dev, sizeof(struct ams_delta_nand),
+			    GFP_KERNEL);
+	if (!priv) {
 		pr_warn("Unable to allocate E3 NAND MTD device structure.\n");
-		err = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
+	this = &priv->nand_chip;
 
-	ams_delta_mtd = nand_to_mtd(this);
-	ams_delta_mtd->dev.parent = &pdev->dev;
+	mtd = nand_to_mtd(this);
+	mtd->dev.parent = &pdev->dev;
 
 	/*
 	 * Don't try to request the memory region from here,
@@ -177,7 +191,8 @@ static int ams_delta_init(struct platform_device *pdev)
 		goto out_free;
 	}
 
-	nand_set_controller_data(this, (void *)io_base);
+	priv->io_base = io_base;
+	nand_set_controller_data(this, priv);
 
 	/* Set address of NAND IO lines */
 	this->IO_ADDR_R = io_base + OMAP_MPUIO_INPUT_LATCH;
@@ -187,14 +202,14 @@ static int ams_delta_init(struct platform_device *pdev)
 	this->read_buf = ams_delta_read_buf;
 	this->cmd_ctrl = ams_delta_hwcontrol;
 
-	gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN);
-	if (IS_ERR(gpiod_rdy)) {
-		err = PTR_ERR(gpiod_rdy);
+	priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN);
+	if (IS_ERR(priv->gpiod_rdy)) {
+		err = PTR_ERR(priv->gpiod_rdy);
 		dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err);
 		goto out_mtd;
 	}
 
-	if (gpiod_rdy)
+	if (priv->gpiod_rdy)
 		this->dev_ready = ams_delta_nand_ready;
 
 	/* 25 us command delay time */
@@ -202,66 +217,64 @@ static int ams_delta_init(struct platform_device *pdev)
 	this->ecc.mode = NAND_ECC_SOFT;
 	this->ecc.algo = NAND_ECC_HAMMING;
 
-	platform_set_drvdata(pdev, io_base);
+	platform_set_drvdata(pdev, priv);
 
 	/* Set chip enabled, but  */
-	gpiod_nwp = devm_gpiod_get(&pdev->dev, "nwp", GPIOD_OUT_HIGH);
-	if (IS_ERR(gpiod_nwp)) {
-		err = PTR_ERR(gpiod_nwp);
+	priv->gpiod_nwp = devm_gpiod_get(&pdev->dev, "nwp", GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->gpiod_nwp)) {
+		err = PTR_ERR(priv->gpiod_nwp);
 		dev_err(&pdev->dev, "NWP GPIO request failed (%d)\n", err);
 		goto out_mtd;
 	}
 
-	gpiod_nce = devm_gpiod_get(&pdev->dev, "nce", GPIOD_OUT_HIGH);
-	if (IS_ERR(gpiod_nce)) {
-		err = PTR_ERR(gpiod_nce);
+	priv->gpiod_nce = devm_gpiod_get(&pdev->dev, "nce", GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->gpiod_nce)) {
+		err = PTR_ERR(priv->gpiod_nce);
 		dev_err(&pdev->dev, "NCE GPIO request failed (%d)\n", err);
 		goto out_mtd;
 	}
 
-	gpiod_nre = devm_gpiod_get(&pdev->dev, "nre", GPIOD_OUT_HIGH);
-	if (IS_ERR(gpiod_nre)) {
-		err = PTR_ERR(gpiod_nre);
+	priv->gpiod_nre = devm_gpiod_get(&pdev->dev, "nre", GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->gpiod_nre)) {
+		err = PTR_ERR(priv->gpiod_nre);
 		dev_err(&pdev->dev, "NRE GPIO request failed (%d)\n", err);
 		goto out_mtd;
 	}
 
-	gpiod_nwe = devm_gpiod_get(&pdev->dev, "nwe", GPIOD_OUT_HIGH);
-	if (IS_ERR(gpiod_nwe)) {
-		err = PTR_ERR(gpiod_nwe);
+	priv->gpiod_nwe = devm_gpiod_get(&pdev->dev, "nwe", GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->gpiod_nwe)) {
+		err = PTR_ERR(priv->gpiod_nwe);
 		dev_err(&pdev->dev, "NWE GPIO request failed (%d)\n", err);
 		goto out_mtd;
 	}
 
-	gpiod_ale = devm_gpiod_get(&pdev->dev, "ale", GPIOD_OUT_LOW);
-	if (IS_ERR(gpiod_ale)) {
-		err = PTR_ERR(gpiod_ale);
+	priv->gpiod_ale = devm_gpiod_get(&pdev->dev, "ale", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->gpiod_ale)) {
+		err = PTR_ERR(priv->gpiod_ale);
 		dev_err(&pdev->dev, "ALE GPIO request failed (%d)\n", err);
 		goto out_mtd;
 	}
 
-	gpiod_cle = devm_gpiod_get(&pdev->dev, "cle", GPIOD_OUT_LOW);
-	if (IS_ERR(gpiod_cle)) {
-		err = PTR_ERR(gpiod_cle);
+	priv->gpiod_cle = devm_gpiod_get(&pdev->dev, "cle", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->gpiod_cle)) {
+		err = PTR_ERR(priv->gpiod_cle);
 		dev_err(&pdev->dev, "CLE GPIO request failed (%d)\n", err);
 		goto out_mtd;
 	}
 
 	/* Scan to find existence of the device */
-	err = nand_scan(ams_delta_mtd, 1);
+	err = nand_scan(mtd, 1);
 	if (err)
 		goto out_mtd;
 
 	/* Register the partitions */
-	mtd_device_register(ams_delta_mtd, partition_info,
-			    ARRAY_SIZE(partition_info));
+	mtd_device_register(mtd, partition_info, ARRAY_SIZE(partition_info));
 
 	goto out;
 
  out_mtd:
 	iounmap(io_base);
 out_free:
-	kfree(this);
  out:
 	return err;
 }
@@ -271,16 +284,15 @@ static int ams_delta_init(struct platform_device *pdev)
  */
 static int ams_delta_cleanup(struct platform_device *pdev)
 {
-	void __iomem *io_base = platform_get_drvdata(pdev);
+	struct ams_delta_nand *priv = platform_get_drvdata(pdev);
+	struct mtd_info *mtd = nand_to_mtd(&priv->nand_chip);
+	void __iomem *io_base = priv->io_base;
 
 	/* Release resources, unregister device */
-	nand_release(ams_delta_mtd);
+	nand_release(mtd);
 
 	iounmap(io_base);
 
-	/* Free the MTD device structure */
-	kfree(mtd_to_nand(ams_delta_mtd));
-
 	return 0;
 }
 
-- 
2.16.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 02/12] mtd: rawnand: ams-delta: Use private structure

[Boris Brezillon]

On Tue,  7 Aug 2018 00:29:08 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Introduce a driver private structure and allocate it on device probe.
> Use it for storing nand_chip structure, GPIO descriptors prevoiusly
> stored in static variables as well as io_base pointer previously passed
> as nand controller data or platform driver data.  Subsequent patches
> may populate the structure with more members as needed.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
>  drivers/mtd/nand/raw/ams-delta.c | 126 +++++++++++++++++++++------------------
>  1 file changed, 69 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
> index af313c620264..48233d638d2a 100644
> --- a/drivers/mtd/nand/raw/ams-delta.c
> +++ b/drivers/mtd/nand/raw/ams-delta.c
> @@ -34,14 +34,18 @@
>  /*
>   * MTD structure for E3 (Delta)
>   */
> -static struct mtd_info *ams_delta_mtd = NULL;
> -static struct gpio_desc *gpiod_rdy;
> -static struct gpio_desc *gpiod_nce;
> -static struct gpio_desc *gpiod_nre;
> -static struct gpio_desc *gpiod_nwp;
> -static struct gpio_desc *gpiod_nwe;
> -static struct gpio_desc *gpiod_ale;
> -static struct gpio_desc *gpiod_cle;
> +
> +struct ams_delta_nand {
> +	struct nand_chip	nand_chip;
> +	struct gpio_desc	*gpiod_rdy;
> +	struct gpio_desc	*gpiod_nce;
> +	struct gpio_desc	*gpiod_nre;
> +	struct gpio_desc	*gpiod_nwp;
> +	struct gpio_desc	*gpiod_nwe;
> +	struct gpio_desc	*gpiod_ale;
> +	struct gpio_desc	*gpiod_cle;
> +	void __iomem		*io_base;
> +};
>  
>  /*
>   * Define partitions for flash devices
> @@ -71,26 +75,28 @@ static const struct mtd_partition partition_info[] = {
>  static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
>  {
>  	struct nand_chip *this = mtd_to_nand(mtd);
> -	void __iomem *io_base = (void __iomem *)nand_get_controller_data(this);
> +	struct ams_delta_nand *priv = nand_get_controller_data(this);
> +	void __iomem *io_base = priv->io_base;
>  
>  	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
>  	writew(byte, this->IO_ADDR_W);
> -	gpiod_set_value(gpiod_nwe, 0);
> +	gpiod_set_value(priv->gpiod_nwe, 0);
>  	ndelay(40);
> -	gpiod_set_value(gpiod_nwe, 1);
> +	gpiod_set_value(priv->gpiod_nwe, 1);
>  }
>  
>  static u_char ams_delta_read_byte(struct mtd_info *mtd)
>  {
>  	u_char res;
>  	struct nand_chip *this = mtd_to_nand(mtd);
> -	void __iomem *io_base = (void __iomem *)nand_get_controller_data(this);
> +	struct ams_delta_nand *priv = nand_get_controller_data(this);
> +	void __iomem *io_base = priv->io_base;
>  
> -	gpiod_set_value(gpiod_nre, 0);
> +	gpiod_set_value(priv->gpiod_nre, 0);
>  	ndelay(40);
>  	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
>  	res = readw(this->IO_ADDR_R);
> -	gpiod_set_value(gpiod_nre, 1);
> +	gpiod_set_value(priv->gpiod_nre, 1);
>  
>  	return res;
>  }
> @@ -123,11 +129,13 @@ static void ams_delta_read_buf(struct mtd_info *mtd, u_char *buf, int len)
>  static void ams_delta_hwcontrol(struct mtd_info *mtd, int cmd,
>  				unsigned int ctrl)
>  {
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct ams_delta_nand *priv = nand_get_controller_data(this);
>  
>  	if (ctrl & NAND_CTRL_CHANGE) {
> -		gpiod_set_value(gpiod_nce, !(ctrl & NAND_NCE));
> -		gpiod_set_value(gpiod_cle, !!(ctrl & NAND_CLE));
> -		gpiod_set_value(gpiod_ale, !!(ctrl & NAND_ALE));
> +		gpiod_set_value(priv->gpiod_nce, !(ctrl & NAND_NCE));
> +		gpiod_set_value(priv->gpiod_cle, !!(ctrl & NAND_CLE));
> +		gpiod_set_value(priv->gpiod_ale, !!(ctrl & NAND_ALE));
>  	}
>  
>  	if (cmd != NAND_CMD_NONE)
> @@ -136,7 +144,10 @@ static void ams_delta_hwcontrol(struct mtd_info *mtd, int cmd,
>  
>  static int ams_delta_nand_ready(struct mtd_info *mtd)
>  {
> -	return gpiod_get_value(gpiod_rdy);
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct ams_delta_nand *priv = nand_get_controller_data(this);
> +
> +	return gpiod_get_value(priv->gpiod_rdy);
>  }
>  
>  
> @@ -145,7 +156,9 @@ static int ams_delta_nand_ready(struct mtd_info *mtd)
>   */
>  static int ams_delta_init(struct platform_device *pdev)
>  {
> +	struct ams_delta_nand *priv;
>  	struct nand_chip *this;
> +	struct mtd_info *mtd;
>  	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	void __iomem *io_base;
>  	int err = 0;
> @@ -154,15 +167,16 @@ static int ams_delta_init(struct platform_device *pdev)
>  		return -ENXIO;
>  
>  	/* Allocate memory for MTD device structure and private data */
> -	this = kzalloc(sizeof(struct nand_chip), GFP_KERNEL);
> -	if (!this) {
> +	priv = devm_kzalloc(&pdev->dev, sizeof(struct ams_delta_nand),
> +			    GFP_KERNEL);
> +	if (!priv) {
>  		pr_warn("Unable to allocate E3 NAND MTD device structure.\n");
> -		err = -ENOMEM;
> -		goto out;
> +		return -ENOMEM;
>  	}
> +	this = &priv->nand_chip;
>  
> -	ams_delta_mtd = nand_to_mtd(this);
> -	ams_delta_mtd->dev.parent = &pdev->dev;
> +	mtd = nand_to_mtd(this);
> +	mtd->dev.parent = &pdev->dev;
>  
>  	/*
>  	 * Don't try to request the memory region from here,
> @@ -177,7 +191,8 @@ static int ams_delta_init(struct platform_device *pdev)
>  		goto out_free;
>  	}
>  
> -	nand_set_controller_data(this, (void *)io_base);
> +	priv->io_base = io_base;
> +	nand_set_controller_data(this, priv);
>  
>  	/* Set address of NAND IO lines */
>  	this->IO_ADDR_R = io_base + OMAP_MPUIO_INPUT_LATCH;
> @@ -187,14 +202,14 @@ static int ams_delta_init(struct platform_device *pdev)
>  	this->read_buf = ams_delta_read_buf;
>  	this->cmd_ctrl = ams_delta_hwcontrol;
>  
> -	gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN);
> -	if (IS_ERR(gpiod_rdy)) {
> -		err = PTR_ERR(gpiod_rdy);
> +	priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN);
> +	if (IS_ERR(priv->gpiod_rdy)) {
> +		err = PTR_ERR(priv->gpiod_rdy);
>  		dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err);
>  		goto out_mtd;
>  	}
>  
> -	if (gpiod_rdy)
> +	if (priv->gpiod_rdy)
>  		this->dev_ready = ams_delta_nand_ready;
>  
>  	/* 25 us command delay time */
> @@ -202,66 +217,64 @@ static int ams_delta_init(struct platform_device *pdev)
>  	this->ecc.mode = NAND_ECC_SOFT;
>  	this->ecc.algo = NAND_ECC_HAMMING;
>  
> -	platform_set_drvdata(pdev, io_base);
> +	platform_set_drvdata(pdev, priv);
>  
>  	/* Set chip enabled, but  */
> -	gpiod_nwp = devm_gpiod_get(&pdev->dev, "nwp", GPIOD_OUT_HIGH);
> -	if (IS_ERR(gpiod_nwp)) {
> -		err = PTR_ERR(gpiod_nwp);
> +	priv->gpiod_nwp = devm_gpiod_get(&pdev->dev, "nwp", GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->gpiod_nwp)) {
> +		err = PTR_ERR(priv->gpiod_nwp);
>  		dev_err(&pdev->dev, "NWP GPIO request failed (%d)\n", err);
>  		goto out_mtd;
>  	}
>  
> -	gpiod_nce = devm_gpiod_get(&pdev->dev, "nce", GPIOD_OUT_HIGH);
> -	if (IS_ERR(gpiod_nce)) {
> -		err = PTR_ERR(gpiod_nce);
> +	priv->gpiod_nce = devm_gpiod_get(&pdev->dev, "nce", GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->gpiod_nce)) {
> +		err = PTR_ERR(priv->gpiod_nce);
>  		dev_err(&pdev->dev, "NCE GPIO request failed (%d)\n", err);
>  		goto out_mtd;
>  	}
>  
> -	gpiod_nre = devm_gpiod_get(&pdev->dev, "nre", GPIOD_OUT_HIGH);
> -	if (IS_ERR(gpiod_nre)) {
> -		err = PTR_ERR(gpiod_nre);
> +	priv->gpiod_nre = devm_gpiod_get(&pdev->dev, "nre", GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->gpiod_nre)) {
> +		err = PTR_ERR(priv->gpiod_nre);
>  		dev_err(&pdev->dev, "NRE GPIO request failed (%d)\n", err);
>  		goto out_mtd;
>  	}
>  
> -	gpiod_nwe = devm_gpiod_get(&pdev->dev, "nwe", GPIOD_OUT_HIGH);
> -	if (IS_ERR(gpiod_nwe)) {
> -		err = PTR_ERR(gpiod_nwe);
> +	priv->gpiod_nwe = devm_gpiod_get(&pdev->dev, "nwe", GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->gpiod_nwe)) {
> +		err = PTR_ERR(priv->gpiod_nwe);
>  		dev_err(&pdev->dev, "NWE GPIO request failed (%d)\n", err);
>  		goto out_mtd;
>  	}
>  
> -	gpiod_ale = devm_gpiod_get(&pdev->dev, "ale", GPIOD_OUT_LOW);
> -	if (IS_ERR(gpiod_ale)) {
> -		err = PTR_ERR(gpiod_ale);
> +	priv->gpiod_ale = devm_gpiod_get(&pdev->dev, "ale", GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->gpiod_ale)) {
> +		err = PTR_ERR(priv->gpiod_ale);
>  		dev_err(&pdev->dev, "ALE GPIO request failed (%d)\n", err);
>  		goto out_mtd;
>  	}
>  
> -	gpiod_cle = devm_gpiod_get(&pdev->dev, "cle", GPIOD_OUT_LOW);
> -	if (IS_ERR(gpiod_cle)) {
> -		err = PTR_ERR(gpiod_cle);
> +	priv->gpiod_cle = devm_gpiod_get(&pdev->dev, "cle", GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->gpiod_cle)) {
> +		err = PTR_ERR(priv->gpiod_cle);
>  		dev_err(&pdev->dev, "CLE GPIO request failed (%d)\n", err);
>  		goto out_mtd;
>  	}
>  
>  	/* Scan to find existence of the device */
> -	err = nand_scan(ams_delta_mtd, 1);
> +	err = nand_scan(mtd, 1);
>  	if (err)
>  		goto out_mtd;
>  
>  	/* Register the partitions */
> -	mtd_device_register(ams_delta_mtd, partition_info,
> -			    ARRAY_SIZE(partition_info));
> +	mtd_device_register(mtd, partition_info, ARRAY_SIZE(partition_info));
>  
>  	goto out;
>  
>   out_mtd:
>  	iounmap(io_base);
>  out_free:
> -	kfree(this);
>   out:
>  	return err;
>  }
> @@ -271,16 +284,15 @@ static int ams_delta_init(struct platform_device *pdev)
>   */
>  static int ams_delta_cleanup(struct platform_device *pdev)
>  {
> -	void __iomem *io_base = platform_get_drvdata(pdev);
> +	struct ams_delta_nand *priv = platform_get_drvdata(pdev);
> +	struct mtd_info *mtd = nand_to_mtd(&priv->nand_chip);
> +	void __iomem *io_base = priv->io_base;
>  
>  	/* Release resources, unregister device */
> -	nand_release(ams_delta_mtd);
> +	nand_release(mtd);
>  
>  	iounmap(io_base);
>  
> -	/* Free the MTD device structure */
> -	kfree(mtd_to_nand(ams_delta_mtd));
> -
>  	return 0;
>  }
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v2 03/12] ARM: OMAP1: ams-delta: Provide GPIO lookup table for NAND data port

[Janusz Krzysztofik]

Data port used by Amstrad Delta NAND driver is actually an OMAP MPUIO
device, already under control of gpio-omap driver.  The NAND driver
gets access to the port by ioremapping it and performs read/write
operations.  That is done without any proteciton from other users
legally manipulating the port pins over GPIO API.

The plan is to convert the driver to access the port over GPIO consumer
API.  Before that is implemented, the driver can already obtain
exclusive access to the port by requesting an array of its GPIO
descriptors.

Add respective entries to the NAND GPIO lookup table.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 arch/arm/mach-omap1/board-ams-delta.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c
index eedacdfe9725..16f7bbe47607 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -337,7 +337,8 @@ static struct platform_device ams_delta_nand_device = {
 	.resource	= ams_delta_nand_resources,
 };
 
-#define OMAP_GPIO_LABEL	"gpio-0-15"
+#define OMAP_GPIO_LABEL		"gpio-0-15"
+#define OMAP_MPUIO_LABEL	"mpuio"
 
 static struct gpiod_lookup_table ams_delta_nand_gpio_table = {
 	.table = {
@@ -349,6 +350,14 @@ static struct gpiod_lookup_table ams_delta_nand_gpio_table = {
 		GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_NWE, "nwe", 0),
 		GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_ALE, "ale", 0),
 		GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_CLE, "cle", 0),
+		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 0, "data", 0, 0),
+		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 1, "data", 1, 0),
+		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 2, "data", 2, 0),
+		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 3, "data", 3, 0),
+		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 4, "data", 4, 0),
+		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 5, "data", 5, 0),
+		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 6, "data", 6, 0),
+		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 7, "data", 7, 0),
 		{ },
 	},
 };
-- 
2.16.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 03/12] ARM: OMAP1: ams-delta: Provide GPIO lookup table for NAND data port

[Boris Brezillon]

On Tue,  7 Aug 2018 00:29:09 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Data port used by Amstrad Delta NAND driver is actually an OMAP MPUIO
> device, already under control of gpio-omap driver.  The NAND driver
> gets access to the port by ioremapping it and performs read/write
> operations.  That is done without any proteciton from other users
> legally manipulating the port pins over GPIO API.
> 
> The plan is to convert the driver to access the port over GPIO consumer
> API.  Before that is implemented, the driver can already obtain
> exclusive access to the port by requesting an array of its GPIO
> descriptors.
> 
> Add respective entries to the NAND GPIO lookup table.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
>  arch/arm/mach-omap1/board-ams-delta.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c
> index eedacdfe9725..16f7bbe47607 100644
> --- a/arch/arm/mach-omap1/board-ams-delta.c
> +++ b/arch/arm/mach-omap1/board-ams-delta.c
> @@ -337,7 +337,8 @@ static struct platform_device ams_delta_nand_device = {
>  	.resource	= ams_delta_nand_resources,
>  };
>  
> -#define OMAP_GPIO_LABEL	"gpio-0-15"
> +#define OMAP_GPIO_LABEL		"gpio-0-15"
> +#define OMAP_MPUIO_LABEL	"mpuio"
>  
>  static struct gpiod_lookup_table ams_delta_nand_gpio_table = {
>  	.table = {
> @@ -349,6 +350,14 @@ static struct gpiod_lookup_table ams_delta_nand_gpio_table = {
>  		GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_NWE, "nwe", 0),
>  		GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_ALE, "ale", 0),
>  		GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_CLE, "cle", 0),
> +		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 0, "data", 0, 0),
> +		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 1, "data", 1, 0),
> +		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 2, "data", 2, 0),
> +		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 3, "data", 3, 0),
> +		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 4, "data", 4, 0),
> +		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 5, "data", 5, 0),
> +		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 6, "data", 6, 0),
> +		GPIO_LOOKUP_IDX(OMAP_MPUIO_LABEL, 7, "data", 7, 0),
>  		{ },
>  	},
>  };

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 03/12] ARM: OMAP1: ams-delta: Provide GPIO lookup table for NAND data port

[Linus Walleij]

On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Data port used by Amstrad Delta NAND driver is actually an OMAP MPUIO
> device, already under control of gpio-omap driver.  The NAND driver
> gets access to the port by ioremapping it and performs read/write
> operations.  That is done without any proteciton from other users
> legally manipulating the port pins over GPIO API.
>
> The plan is to convert the driver to access the port over GPIO consumer
> API.  Before that is implemented, the driver can already obtain
> exclusive access to the port by requesting an array of its GPIO
> descriptors.
>
> Add respective entries to the NAND GPIO lookup table.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[RFC PATCH v2 04/12] mtd: rawnand: ams-delta: request data port GPIO resource

[Janusz Krzysztofik]

Data port used by the driver is actually an OMAP MPUIO device, already
under control of gpio-omap driver.  For that reason we used to not
request the memory region of the port as that would fail because the
region is already busy.  Despite that, we are still accessing the port
by just ioremapping it and performing read/write operations.  Moreover,
we are doing that without any proteciton from other users legally
manipulating the port pins over GPIO API.

The plan is to convert the driver to access the port over functions
exposed by the gpio-omap driver.  Before that happens, already prevent
from other users accessing the port pins by requesting an array of its
GPIO descriptors.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/mtd/nand/raw/ams-delta.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index 48233d638d2a..09d6901fc94d 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -161,6 +161,7 @@ static int ams_delta_init(struct platform_device *pdev)
 	struct mtd_info *mtd;
 	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	void __iomem *io_base;
+	struct gpio_descs *data_gpiods;
 	int err = 0;
 
 	if (!res)
@@ -261,6 +262,13 @@ static int ams_delta_init(struct platform_device *pdev)
 		dev_err(&pdev->dev, "CLE GPIO request failed (%d)\n", err);
 		goto out_mtd;
 	}
+	/* Request array of data pins, initialize them as input */
+	data_gpiods = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_IN);
+	if (IS_ERR(data_gpiods)) {
+		err = PTR_ERR(data_gpiods);
+		dev_err(&pdev->dev, "data GPIO request failed: %d\n", err);
+		goto out_mtd;
+	}
 
 	/* Scan to find existence of the device */
 	err = nand_scan(mtd, 1);
-- 
2.16.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 04/12] mtd: rawnand: ams-delta: request data port GPIO resource

[Boris Brezillon]

On Tue,  7 Aug 2018 00:29:10 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Data port used by the driver is actually an OMAP MPUIO device, already
> under control of gpio-omap driver.  For that reason we used to not
> request the memory region of the port as that would fail because the
> region is already busy.  Despite that, we are still accessing the port
> by just ioremapping it and performing read/write operations.  Moreover,
> we are doing that without any proteciton from other users legally
> manipulating the port pins over GPIO API.
> 
> The plan is to convert the driver to access the port over functions
> exposed by the gpio-omap driver.  Before that happens, already prevent
> from other users accessing the port pins by requesting an array of its
> GPIO descriptors.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
>  drivers/mtd/nand/raw/ams-delta.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
> index 48233d638d2a..09d6901fc94d 100644
> --- a/drivers/mtd/nand/raw/ams-delta.c
> +++ b/drivers/mtd/nand/raw/ams-delta.c
> @@ -161,6 +161,7 @@ static int ams_delta_init(struct platform_device *pdev)
>  	struct mtd_info *mtd;
>  	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	void __iomem *io_base;
> +	struct gpio_descs *data_gpiods;
>  	int err = 0;
>  
>  	if (!res)
> @@ -261,6 +262,13 @@ static int ams_delta_init(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "CLE GPIO request failed (%d)\n", err);
>  		goto out_mtd;
>  	}
> +	/* Request array of data pins, initialize them as input */
> +	data_gpiods = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_IN);
> +	if (IS_ERR(data_gpiods)) {
> +		err = PTR_ERR(data_gpiods);
> +		dev_err(&pdev->dev, "data GPIO request failed: %d\n", err);
> +		goto out_mtd;
> +	}
>  
>  	/* Scan to find existence of the device */
>  	err = nand_scan(mtd, 1);

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 04/12] mtd: rawnand: ams-delta: request data port GPIO resource

[Linus Walleij]

On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Data port used by the driver is actually an OMAP MPUIO device, already
> under control of gpio-omap driver.  For that reason we used to not
> request the memory region of the port as that would fail because the
> region is already busy.  Despite that, we are still accessing the port
> by just ioremapping it and performing read/write operations.  Moreover,
> we are doing that without any proteciton from other users legally
> manipulating the port pins over GPIO API.
>
> The plan is to convert the driver to access the port over functions
> exposed by the gpio-omap driver.  Before that happens, already prevent
> from other users accessing the port pins by requesting an array of its
> GPIO descriptors.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[RFC PATCH v2 05/12] mtd: rawnand: ams-delta: use GPIO API for data read/write

[Janusz Krzysztofik]

Don't readw()/writew() data directly from/to GPIO port which is under
control of gpio-omap driver, use GPIO API instead.

Degrade of performance on Amstrad Delta is completely not acceptable.

The driver should work with any 8+-bit bidirectional GPIO port, not
only OMAP.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/mtd/nand/raw/ams-delta.c | 97 ++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 59 deletions(-)

diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index 09d6901fc94d..78996ddf82e0 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -24,13 +24,10 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/rawnand.h>
 #include <linux/mtd/partitions.h>
-#include <linux/platform_data/gpio-omap.h>
+#include <linux/platform_device.h>
 
-#include <asm/io.h>
 #include <asm/sizes.h>
 
-#include <mach/hardware.h>
-
 /*
  * MTD structure for E3 (Delta)
  */
@@ -44,7 +41,7 @@ struct ams_delta_nand {
 	struct gpio_desc	*gpiod_nwe;
 	struct gpio_desc	*gpiod_ale;
 	struct gpio_desc	*gpiod_cle;
-	void __iomem		*io_base;
+	struct gpio_descs	*data_gpiods;
 };
 
 /*
@@ -76,10 +73,14 @@ static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
 {
 	struct nand_chip *this = mtd_to_nand(mtd);
 	struct ams_delta_nand *priv = nand_get_controller_data(this);
-	void __iomem *io_base = priv->io_base;
+	struct gpio_descs *data_gpiods = priv->data_gpiods;
+	unsigned long bits = byte;
+	int i;
+
+	for (i = 0; i < data_gpiods->ndescs; i++)
+		gpiod_direction_output_raw(data_gpiods->desc[i],
+					   test_bit(i, &bits));
 
-	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
-	writew(byte, this->IO_ADDR_W);
 	gpiod_set_value(priv->gpiod_nwe, 0);
 	ndelay(40);
 	gpiod_set_value(priv->gpiod_nwe, 1);
@@ -87,18 +88,28 @@ static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
 
 static u_char ams_delta_read_byte(struct mtd_info *mtd)
 {
-	u_char res;
 	struct nand_chip *this = mtd_to_nand(mtd);
 	struct ams_delta_nand *priv = nand_get_controller_data(this);
-	void __iomem *io_base = priv->io_base;
+	struct gpio_descs *data_gpiods = priv->data_gpiods;
+	unsigned long bits = 0;
+	int i, value_array[data_gpiods->ndescs];
+
+	for (i = 0; i < data_gpiods->ndescs; i++)
+		gpiod_direction_input(data_gpiods->desc[i]);
 
 	gpiod_set_value(priv->gpiod_nre, 0);
 	ndelay(40);
-	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
-	res = readw(this->IO_ADDR_R);
+
+	gpiod_get_raw_array_value(data_gpiods->ndescs, data_gpiods->desc,
+				  value_array);
+
 	gpiod_set_value(priv->gpiod_nre, 1);
 
-	return res;
+	for (i = 0; i < data_gpiods->ndescs; i++)
+		if (value_array[i])
+			__set_bit(i, &bits);
+
+	return bits;
 }
 
 static void ams_delta_write_buf(struct mtd_info *mtd, const u_char *buf,
@@ -159,14 +170,8 @@ static int ams_delta_init(struct platform_device *pdev)
 	struct ams_delta_nand *priv;
 	struct nand_chip *this;
 	struct mtd_info *mtd;
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	void __iomem *io_base;
-	struct gpio_descs *data_gpiods;
 	int err = 0;
 
-	if (!res)
-		return -ENXIO;
-
 	/* Allocate memory for MTD device structure and private data */
 	priv = devm_kzalloc(&pdev->dev, sizeof(struct ams_delta_nand),
 			    GFP_KERNEL);
@@ -179,25 +184,8 @@ static int ams_delta_init(struct platform_device *pdev)
 	mtd = nand_to_mtd(this);
 	mtd->dev.parent = &pdev->dev;
 
-	/*
-	 * Don't try to request the memory region from here,
-	 * it should have been already requested from the
-	 * gpio-omap driver and requesting it again would fail.
-	 */
-
-	io_base = ioremap(res->start, resource_size(res));
-	if (io_base == NULL) {
-		dev_err(&pdev->dev, "ioremap failed\n");
-		err = -EIO;
-		goto out_free;
-	}
-
-	priv->io_base = io_base;
 	nand_set_controller_data(this, priv);
 
-	/* Set address of NAND IO lines */
-	this->IO_ADDR_R = io_base + OMAP_MPUIO_INPUT_LATCH;
-	this->IO_ADDR_W = io_base + OMAP_MPUIO_OUTPUT;
 	this->read_byte = ams_delta_read_byte;
 	this->write_buf = ams_delta_write_buf;
 	this->read_buf = ams_delta_read_buf;
@@ -207,7 +195,7 @@ static int ams_delta_init(struct platform_device *pdev)
 	if (IS_ERR(priv->gpiod_rdy)) {
 		err = PTR_ERR(priv->gpiod_rdy);
 		dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	if (priv->gpiod_rdy)
@@ -225,66 +213,60 @@ static int ams_delta_init(struct platform_device *pdev)
 	if (IS_ERR(priv->gpiod_nwp)) {
 		err = PTR_ERR(priv->gpiod_nwp);
 		dev_err(&pdev->dev, "NWP GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	priv->gpiod_nce = devm_gpiod_get(&pdev->dev, "nce", GPIOD_OUT_HIGH);
 	if (IS_ERR(priv->gpiod_nce)) {
 		err = PTR_ERR(priv->gpiod_nce);
 		dev_err(&pdev->dev, "NCE GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	priv->gpiod_nre = devm_gpiod_get(&pdev->dev, "nre", GPIOD_OUT_HIGH);
 	if (IS_ERR(priv->gpiod_nre)) {
 		err = PTR_ERR(priv->gpiod_nre);
 		dev_err(&pdev->dev, "NRE GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	priv->gpiod_nwe = devm_gpiod_get(&pdev->dev, "nwe", GPIOD_OUT_HIGH);
 	if (IS_ERR(priv->gpiod_nwe)) {
 		err = PTR_ERR(priv->gpiod_nwe);
 		dev_err(&pdev->dev, "NWE GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	priv->gpiod_ale = devm_gpiod_get(&pdev->dev, "ale", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->gpiod_ale)) {
 		err = PTR_ERR(priv->gpiod_ale);
 		dev_err(&pdev->dev, "ALE GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	priv->gpiod_cle = devm_gpiod_get(&pdev->dev, "cle", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->gpiod_cle)) {
 		err = PTR_ERR(priv->gpiod_cle);
 		dev_err(&pdev->dev, "CLE GPIO request failed (%d)\n", err);
-		goto out_mtd;
+		return err;
 	}
 	/* Request array of data pins, initialize them as input */
-	data_gpiods = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_IN);
-	if (IS_ERR(data_gpiods)) {
-		err = PTR_ERR(data_gpiods);
+	priv->data_gpiods = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_IN);
+	if (IS_ERR(priv->data_gpiods)) {
+		err = PTR_ERR(priv->data_gpiods);
 		dev_err(&pdev->dev, "data GPIO request failed: %d\n", err);
-		goto out_mtd;
+		return err;
 	}
 
 	/* Scan to find existence of the device */
 	err = nand_scan(mtd, 1);
 	if (err)
-		goto out_mtd;
+		return err;
 
 	/* Register the partitions */
 	mtd_device_register(mtd, partition_info, ARRAY_SIZE(partition_info));
 
-	goto out;
-
- out_mtd:
-	iounmap(io_base);
-out_free:
- out:
-	return err;
+	return 0;
 }
 
 /*
@@ -294,13 +276,10 @@ static int ams_delta_cleanup(struct platform_device *pdev)
 {
 	struct ams_delta_nand *priv = platform_get_drvdata(pdev);
 	struct mtd_info *mtd = nand_to_mtd(&priv->nand_chip);
-	void __iomem *io_base = priv->io_base;
 
-	/* Release resources, unregister device */
+	/* Unregister device */
 	nand_release(mtd);
 
-	iounmap(io_base);
-
 	return 0;
 }
 
-- 
2.16.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 05/12] mtd: rawnand: ams-delta: use GPIO API for data read/write

[Marek Vasut]

On 08/07/2018 12:29 AM, Janusz Krzysztofik wrote:
> Don't readw()/writew() data directly from/to GPIO port which is under
> control of gpio-omap driver, use GPIO API instead.
> 
> Degrade of performance on Amstrad Delta is completely not acceptable.

I'd expect that changing from direct PIO to access through GPIO API
would degrade the performance. Maybe I misunderstood something ?

> The driver should work with any 8+-bit bidirectional GPIO port, not
> only OMAP.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
>  drivers/mtd/nand/raw/ams-delta.c | 97 ++++++++++++++++------------------------
>  1 file changed, 38 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
> index 09d6901fc94d..78996ddf82e0 100644
> --- a/drivers/mtd/nand/raw/ams-delta.c
> +++ b/drivers/mtd/nand/raw/ams-delta.c
> @@ -24,13 +24,10 @@
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/rawnand.h>
>  #include <linux/mtd/partitions.h>
> -#include <linux/platform_data/gpio-omap.h>
> +#include <linux/platform_device.h>
>  
> -#include <asm/io.h>
>  #include <asm/sizes.h>
>  
> -#include <mach/hardware.h>
> -
>  /*
>   * MTD structure for E3 (Delta)
>   */
> @@ -44,7 +41,7 @@ struct ams_delta_nand {
>  	struct gpio_desc	*gpiod_nwe;
>  	struct gpio_desc	*gpiod_ale;
>  	struct gpio_desc	*gpiod_cle;
> -	void __iomem		*io_base;
> +	struct gpio_descs	*data_gpiods;
>  };
>  
>  /*
> @@ -76,10 +73,14 @@ static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
>  {
>  	struct nand_chip *this = mtd_to_nand(mtd);
>  	struct ams_delta_nand *priv = nand_get_controller_data(this);
> -	void __iomem *io_base = priv->io_base;
> +	struct gpio_descs *data_gpiods = priv->data_gpiods;
> +	unsigned long bits = byte;
> +	int i;
> +
> +	for (i = 0; i < data_gpiods->ndescs; i++)
> +		gpiod_direction_output_raw(data_gpiods->desc[i],
> +					   test_bit(i, &bits));
>  
> -	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
> -	writew(byte, this->IO_ADDR_W);
>  	gpiod_set_value(priv->gpiod_nwe, 0);
>  	ndelay(40);
>  	gpiod_set_value(priv->gpiod_nwe, 1);
> @@ -87,18 +88,28 @@ static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
>  
>  static u_char ams_delta_read_byte(struct mtd_info *mtd)
>  {
> -	u_char res;
>  	struct nand_chip *this = mtd_to_nand(mtd);
>  	struct ams_delta_nand *priv = nand_get_controller_data(this);
> -	void __iomem *io_base = priv->io_base;
> +	struct gpio_descs *data_gpiods = priv->data_gpiods;
> +	unsigned long bits = 0;
> +	int i, value_array[data_gpiods->ndescs];
> +
> +	for (i = 0; i < data_gpiods->ndescs; i++)
> +		gpiod_direction_input(data_gpiods->desc[i]);
>  
>  	gpiod_set_value(priv->gpiod_nre, 0);
>  	ndelay(40);
> -	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
> -	res = readw(this->IO_ADDR_R);
> +
> +	gpiod_get_raw_array_value(data_gpiods->ndescs, data_gpiods->desc,
> +				  value_array);
> +
>  	gpiod_set_value(priv->gpiod_nre, 1);
>  
> -	return res;
> +	for (i = 0; i < data_gpiods->ndescs; i++)
> +		if (value_array[i])
> +			__set_bit(i, &bits);
> +
> +	return bits;
>  }
>  
>  static void ams_delta_write_buf(struct mtd_info *mtd, const u_char *buf,
> @@ -159,14 +170,8 @@ static int ams_delta_init(struct platform_device *pdev)
>  	struct ams_delta_nand *priv;
>  	struct nand_chip *this;
>  	struct mtd_info *mtd;
> -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	void __iomem *io_base;
> -	struct gpio_descs *data_gpiods;
>  	int err = 0;
>  
> -	if (!res)
> -		return -ENXIO;
> -
>  	/* Allocate memory for MTD device structure and private data */
>  	priv = devm_kzalloc(&pdev->dev, sizeof(struct ams_delta_nand),
>  			    GFP_KERNEL);
> @@ -179,25 +184,8 @@ static int ams_delta_init(struct platform_device *pdev)
>  	mtd = nand_to_mtd(this);
>  	mtd->dev.parent = &pdev->dev;
>  
> -	/*
> -	 * Don't try to request the memory region from here,
> -	 * it should have been already requested from the
> -	 * gpio-omap driver and requesting it again would fail.
> -	 */
> -
> -	io_base = ioremap(res->start, resource_size(res));
> -	if (io_base == NULL) {
> -		dev_err(&pdev->dev, "ioremap failed\n");
> -		err = -EIO;
> -		goto out_free;
> -	}
> -
> -	priv->io_base = io_base;
>  	nand_set_controller_data(this, priv);
>  
> -	/* Set address of NAND IO lines */
> -	this->IO_ADDR_R = io_base + OMAP_MPUIO_INPUT_LATCH;
> -	this->IO_ADDR_W = io_base + OMAP_MPUIO_OUTPUT;
>  	this->read_byte = ams_delta_read_byte;
>  	this->write_buf = ams_delta_write_buf;
>  	this->read_buf = ams_delta_read_buf;
> @@ -207,7 +195,7 @@ static int ams_delta_init(struct platform_device *pdev)
>  	if (IS_ERR(priv->gpiod_rdy)) {
>  		err = PTR_ERR(priv->gpiod_rdy);
>  		dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err);
> -		goto out_mtd;
> +		return err;
>  	}
>  
>  	if (priv->gpiod_rdy)
> @@ -225,66 +213,60 @@ static int ams_delta_init(struct platform_device *pdev)
>  	if (IS_ERR(priv->gpiod_nwp)) {
>  		err = PTR_ERR(priv->gpiod_nwp);
>  		dev_err(&pdev->dev, "NWP GPIO request failed (%d)\n", err);
> -		goto out_mtd;
> +		return err;
>  	}
>  
>  	priv->gpiod_nce = devm_gpiod_get(&pdev->dev, "nce", GPIOD_OUT_HIGH);
>  	if (IS_ERR(priv->gpiod_nce)) {
>  		err = PTR_ERR(priv->gpiod_nce);
>  		dev_err(&pdev->dev, "NCE GPIO request failed (%d)\n", err);
> -		goto out_mtd;
> +		return err;
>  	}
>  
>  	priv->gpiod_nre = devm_gpiod_get(&pdev->dev, "nre", GPIOD_OUT_HIGH);
>  	if (IS_ERR(priv->gpiod_nre)) {
>  		err = PTR_ERR(priv->gpiod_nre);
>  		dev_err(&pdev->dev, "NRE GPIO request failed (%d)\n", err);
> -		goto out_mtd;
> +		return err;
>  	}
>  
>  	priv->gpiod_nwe = devm_gpiod_get(&pdev->dev, "nwe", GPIOD_OUT_HIGH);
>  	if (IS_ERR(priv->gpiod_nwe)) {
>  		err = PTR_ERR(priv->gpiod_nwe);
>  		dev_err(&pdev->dev, "NWE GPIO request failed (%d)\n", err);
> -		goto out_mtd;
> +		return err;
>  	}
>  
>  	priv->gpiod_ale = devm_gpiod_get(&pdev->dev, "ale", GPIOD_OUT_LOW);
>  	if (IS_ERR(priv->gpiod_ale)) {
>  		err = PTR_ERR(priv->gpiod_ale);
>  		dev_err(&pdev->dev, "ALE GPIO request failed (%d)\n", err);
> -		goto out_mtd;
> +		return err;
>  	}
>  
>  	priv->gpiod_cle = devm_gpiod_get(&pdev->dev, "cle", GPIOD_OUT_LOW);
>  	if (IS_ERR(priv->gpiod_cle)) {
>  		err = PTR_ERR(priv->gpiod_cle);
>  		dev_err(&pdev->dev, "CLE GPIO request failed (%d)\n", err);
> -		goto out_mtd;
> +		return err;
>  	}
>  	/* Request array of data pins, initialize them as input */
> -	data_gpiods = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_IN);
> -	if (IS_ERR(data_gpiods)) {
> -		err = PTR_ERR(data_gpiods);
> +	priv->data_gpiods = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_IN);
> +	if (IS_ERR(priv->data_gpiods)) {
> +		err = PTR_ERR(priv->data_gpiods);
>  		dev_err(&pdev->dev, "data GPIO request failed: %d\n", err);
> -		goto out_mtd;
> +		return err;
>  	}
>  
>  	/* Scan to find existence of the device */
>  	err = nand_scan(mtd, 1);
>  	if (err)
> -		goto out_mtd;
> +		return err;
>  
>  	/* Register the partitions */
>  	mtd_device_register(mtd, partition_info, ARRAY_SIZE(partition_info));
>  
> -	goto out;
> -
> - out_mtd:
> -	iounmap(io_base);
> -out_free:
> - out:
> -	return err;
> +	return 0;
>  }
>  
>  /*
> @@ -294,13 +276,10 @@ static int ams_delta_cleanup(struct platform_device *pdev)
>  {
>  	struct ams_delta_nand *priv = platform_get_drvdata(pdev);
>  	struct mtd_info *mtd = nand_to_mtd(&priv->nand_chip);
> -	void __iomem *io_base = priv->io_base;
>  
> -	/* Release resources, unregister device */
> +	/* Unregister device */
>  	nand_release(mtd);
>  
> -	iounmap(io_base);
> -
>  	return 0;
>  }
>  
> 


-- 
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 05/12] mtd: rawnand: ams-delta: use GPIO API for data read/write

[Boris Brezillon]

On Tue,  7 Aug 2018 00:29:11 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Don't readw()/writew() data directly from/to GPIO port which is under
> control of gpio-omap driver, use GPIO API instead.
> 
> Degrade of performance on Amstrad Delta is completely not acceptable.

Can we have numbers along with information about where the overhead is
when using gpiod_{get,set}_raw_array_value()?

> 
> The driver should work with any 8+-bit bidirectional GPIO port, not
> only OMAP.

That's cool!
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 05/12] mtd: rawnand: ams-delta: use GPIO API for data read/write

[Janusz Krzysztofik]

Hi Boris,

On Tuesday, August 7, 2018 7:06:27 PM CEST Boris Brezillon wrote:
> On Tue,  7 Aug 2018 00:29:11 +0200
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> 
> > Don't readw()/writew() data directly from/to GPIO port which is under
> > control of gpio-omap driver, use GPIO API instead.
> > 
> > Degrade of performance on Amstrad Delta is completely not acceptable.
> 
> Can we have numbers along with information about where the overhead is
> when using gpiod_{get,set}_raw_array_value()?

Yes, as soon as I get physical access to the device, probably this or next 
weekend.

Thanks,
Janusz


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 05/12] mtd: rawnand: ams-delta: use GPIO API for data read/write

[Linus Walleij]

On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Don't readw()/writew() data directly from/to GPIO port which is under
> control of gpio-omap driver, use GPIO API instead.
>
> Degrade of performance on Amstrad Delta is completely not acceptable.
>
> The driver should work with any 8+-bit bidirectional GPIO port, not
> only OMAP.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[RFC PATCH v2 06/12] ARM: OMAP1: ams-delta: drop obsolete NAND resources

[Janusz Krzysztofik]

Amstrad Delta NAND device now uses GPIO API for data I/O so there is no
need to assign memory I/O resource to the device any longer.  Drop it.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 arch/arm/mach-omap1/board-ams-delta.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c
index 16f7bbe47607..08e732bc1cd2 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -321,20 +321,9 @@ struct modem_private_data {
 
 static struct modem_private_data modem_priv;
 
-static struct resource ams_delta_nand_resources[] = {
-	[0] = {
-		.start	= OMAP1_MPUIO_BASE,
-		.end	= OMAP1_MPUIO_BASE +
-				OMAP_MPUIO_IO_CNTL + sizeof(u32) - 1,
-		.flags	= IORESOURCE_MEM,
-	},
-};
-
 static struct platform_device ams_delta_nand_device = {
 	.name	= "ams-delta-nand",
 	.id	= -1,
-	.num_resources	= ARRAY_SIZE(ams_delta_nand_resources),
-	.resource	= ams_delta_nand_resources,
 };
 
 #define OMAP_GPIO_LABEL		"gpio-0-15"
-- 
2.16.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v2 07/12] mtd: rawnand: ams-delta: Set port direction once per transfer

[Janusz Krzysztofik]

In its current shape, the driver sets data port direction before each
byte read/write operation, even during multi-byte transfers.  Since
performance of the driver is completely not acceptable on Amstrad Delta
after it has been converted to GPIO bitbang, try to improve things a
bit by setting the port direction only on first byte of each transfer.

Resulting performance on Amstrad Delta is still far from acceptable.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/mtd/nand/raw/ams-delta.c | 58 ++++++++++++++++++++++++++++++++--------
 1 file changed, 47 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index 78996ddf82e0..d02c48c013e8 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -69,6 +69,30 @@ static const struct mtd_partition partition_info[] = {
 	  .size		=  3 * SZ_256K },
 };
 
+static void ams_delta_write_commit(struct ams_delta_nand *priv)
+{
+	gpiod_set_value(priv->gpiod_nwe, 0);
+	ndelay(40);
+	gpiod_set_value(priv->gpiod_nwe, 1);
+}
+
+static void ams_delta_write_next_byte(struct mtd_info *mtd, u_char byte)
+{
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct ams_delta_nand *priv = nand_get_controller_data(this);
+	struct gpio_descs *data_gpiods = priv->data_gpiods;
+	unsigned long bits = byte;
+	int i, value_array[data_gpiods->ndescs];
+
+	for (i = 0; i < data_gpiods->ndescs; i++)
+		value_array[i] = test_bit(i, &bits);
+
+	gpiod_set_raw_array_value(data_gpiods->ndescs, data_gpiods->desc,
+				  value_array);
+
+	ams_delta_write_commit(priv);
+}
+
 static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
 {
 	struct nand_chip *this = mtd_to_nand(mtd);
@@ -81,12 +105,10 @@ static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
 		gpiod_direction_output_raw(data_gpiods->desc[i],
 					   test_bit(i, &bits));
 
-	gpiod_set_value(priv->gpiod_nwe, 0);
-	ndelay(40);
-	gpiod_set_value(priv->gpiod_nwe, 1);
+	ams_delta_write_commit(priv);
 }
 
-static u_char ams_delta_read_byte(struct mtd_info *mtd)
+static u_char ams_delta_read_next_byte(struct mtd_info *mtd)
 {
 	struct nand_chip *this = mtd_to_nand(mtd);
 	struct ams_delta_nand *priv = nand_get_controller_data(this);
@@ -94,9 +116,6 @@ static u_char ams_delta_read_byte(struct mtd_info *mtd)
 	unsigned long bits = 0;
 	int i, value_array[data_gpiods->ndescs];
 
-	for (i = 0; i < data_gpiods->ndescs; i++)
-		gpiod_direction_input(data_gpiods->desc[i]);
-
 	gpiod_set_value(priv->gpiod_nre, 0);
 	ndelay(40);
 
@@ -112,21 +131,38 @@ static u_char ams_delta_read_byte(struct mtd_info *mtd)
 	return bits;
 }
 
+static u_char ams_delta_read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct ams_delta_nand *priv = nand_get_controller_data(this);
+	struct gpio_descs *data_gpiods = priv->data_gpiods;
+	int i;
+
+	for (i = 0; i < data_gpiods->ndescs; i++)
+		gpiod_direction_input(data_gpiods->desc[i]);
+
+	return ams_delta_read_next_byte(mtd);
+}
+
 static void ams_delta_write_buf(struct mtd_info *mtd, const u_char *buf,
 				int len)
 {
 	int i;
 
-	for (i=0; i<len; i++)
-		ams_delta_write_byte(mtd, buf[i]);
+	if (len > 0)
+		ams_delta_write_byte(mtd, buf[0]);
+	for (i = 1; i < len; i++)
+		ams_delta_write_next_byte(mtd, buf[i]);
 }
 
 static void ams_delta_read_buf(struct mtd_info *mtd, u_char *buf, int len)
 {
 	int i;
 
-	for (i=0; i<len; i++)
-		buf[i] = ams_delta_read_byte(mtd);
+	if (len > 0)
+		buf[0] = ams_delta_read_byte(mtd);
+	for (i = 1; i < len; i++)
+		buf[i] = ams_delta_read_next_byte(mtd);
 }
 
 /*
-- 
2.16.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 07/12] mtd: rawnand: ams-delta: Set port direction once per transfer

[Boris Brezillon]

On Tue,  7 Aug 2018 00:29:13 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> In its current shape, the driver sets data port direction before each
> byte read/write operation, even during multi-byte transfers.  Since
> performance of the driver is completely not acceptable on Amstrad Delta
> after it has been converted to GPIO bitbang, try to improve things a
> bit by setting the port direction only on first byte of each transfer.
> 
> Resulting performance on Amstrad Delta is still far from acceptable.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
>  drivers/mtd/nand/raw/ams-delta.c | 58 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
> index 78996ddf82e0..d02c48c013e8 100644
> --- a/drivers/mtd/nand/raw/ams-delta.c
> +++ b/drivers/mtd/nand/raw/ams-delta.c
> @@ -69,6 +69,30 @@ static const struct mtd_partition partition_info[] = {
>  	  .size		=  3 * SZ_256K },
>  };
>  
> +static void ams_delta_write_commit(struct ams_delta_nand *priv)
> +{
> +	gpiod_set_value(priv->gpiod_nwe, 0);
> +	ndelay(40);
> +	gpiod_set_value(priv->gpiod_nwe, 1);
> +}
> +
> +static void ams_delta_write_next_byte(struct mtd_info *mtd, u_char byte)
> +{
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct ams_delta_nand *priv = nand_get_controller_data(this);
> +	struct gpio_descs *data_gpiods = priv->data_gpiods;
> +	unsigned long bits = byte;
> +	int i, value_array[data_gpiods->ndescs];
> +
> +	for (i = 0; i < data_gpiods->ndescs; i++)
> +		value_array[i] = test_bit(i, &bits);
> +
> +	gpiod_set_raw_array_value(data_gpiods->ndescs, data_gpiods->desc,
> +				  value_array);
> +
> +	ams_delta_write_commit(priv);
> +}
> +
>  static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
>  {
>  	struct nand_chip *this = mtd_to_nand(mtd);
> @@ -81,12 +105,10 @@ static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
>  		gpiod_direction_output_raw(data_gpiods->desc[i],
>  					   test_bit(i, &bits));
>  
> -	gpiod_set_value(priv->gpiod_nwe, 0);
> -	ndelay(40);
> -	gpiod_set_value(priv->gpiod_nwe, 1);
> +	ams_delta_write_commit(priv);
>  }
>  
> -static u_char ams_delta_read_byte(struct mtd_info *mtd)
> +static u_char ams_delta_read_next_byte(struct mtd_info *mtd)
>  {
>  	struct nand_chip *this = mtd_to_nand(mtd);
>  	struct ams_delta_nand *priv = nand_get_controller_data(this);
> @@ -94,9 +116,6 @@ static u_char ams_delta_read_byte(struct mtd_info *mtd)
>  	unsigned long bits = 0;
>  	int i, value_array[data_gpiods->ndescs];
>  
> -	for (i = 0; i < data_gpiods->ndescs; i++)
> -		gpiod_direction_input(data_gpiods->desc[i]);
> -
>  	gpiod_set_value(priv->gpiod_nre, 0);
>  	ndelay(40);
>  
> @@ -112,21 +131,38 @@ static u_char ams_delta_read_byte(struct mtd_info *mtd)
>  	return bits;
>  }
>  
> +static u_char ams_delta_read_byte(struct mtd_info *mtd)
> +{
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct ams_delta_nand *priv = nand_get_controller_data(this);
> +	struct gpio_descs *data_gpiods = priv->data_gpiods;
> +	int i;
> +
> +	for (i = 0; i < data_gpiods->ndescs; i++)
> +		gpiod_direction_input(data_gpiods->desc[i]);
> +
> +	return ams_delta_read_next_byte(mtd);
> +}
> +
>  static void ams_delta_write_buf(struct mtd_info *mtd, const u_char *buf,
>  				int len)
>  {
>  	int i;
>  
> -	for (i=0; i<len; i++)
> -		ams_delta_write_byte(mtd, buf[i]);
> +	if (len > 0)
> +		ams_delta_write_byte(mtd, buf[0]);
> +	for (i = 1; i < len; i++)
> +		ams_delta_write_next_byte(mtd, buf[i]);
>  }
>  
>  static void ams_delta_read_buf(struct mtd_info *mtd, u_char *buf, int len)
>  {
>  	int i;
>  
> -	for (i=0; i<len; i++)
> -		buf[i] = ams_delta_read_byte(mtd);
> +	if (len > 0)
> +		buf[0] = ams_delta_read_byte(mtd);
> +	for (i = 1; i < len; i++)
> +		buf[i] = ams_delta_read_next_byte(mtd);
>  }

I'd suggest a slightly different approach where the data pins
direction state is stored in the the priv struct and only changed when
required. This way you just have to add a test in
ams_delta_read/write_byte().
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 07/12] mtd: rawnand: ams-delta: Set port direction once per transfer

[Janusz Krzysztofik]

Hi Boris,

On Tuesday, August 7, 2018 8:57:52 PM CEST Boris Brezillon wrote:
> On Tue,  7 Aug 2018 00:29:13 +0200
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> 
> > In its current shape, the driver sets data port direction before each
> > byte read/write operation, even during multi-byte transfers.  Since
> > performance of the driver is completely not acceptable on Amstrad Delta
> > after it has been converted to GPIO bitbang, try to improve things a
> > bit by setting the port direction only on first byte of each transfer.
> ...
> I'd suggest a slightly different approach where the data pins
> direction state is stored in the the priv struct and only changed when
> required. This way you just have to add a test in
> ams_delta_read/write_byte().

Good idea, I'm going to use it, thanks.

Once done, may I also move that one earlier in the series so that it can be 
applied while our discussion on GPIO bitmap changes still continues?

Thanks,
Janusz




--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 07/12] mtd: rawnand: ams-delta: Set port direction once per transfer

[Miquel Raynal]

Hi Janusz,

Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote on Wed, 08 Aug 2018
18:55:35 +0200:

> Hi Boris,
> 
> On Tuesday, August 7, 2018 8:57:52 PM CEST Boris Brezillon wrote:
> > On Tue,  7 Aug 2018 00:29:13 +0200
> > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> >   
> > > In its current shape, the driver sets data port direction before each
> > > byte read/write operation, even during multi-byte transfers.  Since
> > > performance of the driver is completely not acceptable on Amstrad Delta
> > > after it has been converted to GPIO bitbang, try to improve things a
> > > bit by setting the port direction only on first byte of each transfer.  
> > ...
> > I'd suggest a slightly different approach where the data pins
> > direction state is stored in the the priv struct and only changed when
> > required. This way you just have to add a test in
> > ams_delta_read/write_byte().  
> 
> Good idea, I'm going to use it, thanks.
> 
> Once done, may I also move that one earlier in the series so that it can be 
> applied while our discussion on GPIO bitmap changes still continues?

I think I may answer on his behalf: yes! You can move the GPIO bitmap
changes at the end of the series, checking that you never break the
bisectability. Then I could apply the major changes and let us iterate
on the GPIO bitmap stuff only.

Thanks,
Miquèl
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v2 08/12] mtd: rawnand: ams-delta: Simplify pointer resolution on read/write

[Janusz Krzysztofik]

Simplify data read/write sub-functions by changing their APIs so they
accept driver private structure pointer instead of mtd_info.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/mtd/nand/raw/ams-delta.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index d02c48c013e8..30c461138195 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -76,10 +76,8 @@ static void ams_delta_write_commit(struct ams_delta_nand *priv)
 	gpiod_set_value(priv->gpiod_nwe, 1);
 }
 
-static void ams_delta_write_next_byte(struct mtd_info *mtd, u_char byte)
+static void ams_delta_write_next_byte(struct ams_delta_nand *priv, u_char byte)
 {
-	struct nand_chip *this = mtd_to_nand(mtd);
-	struct ams_delta_nand *priv = nand_get_controller_data(this);
 	struct gpio_descs *data_gpiods = priv->data_gpiods;
 	unsigned long bits = byte;
 	int i, value_array[data_gpiods->ndescs];
@@ -93,10 +91,8 @@ static void ams_delta_write_next_byte(struct mtd_info *mtd, u_char byte)
 	ams_delta_write_commit(priv);
 }
 
-static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
+static void ams_delta_write_first_byte(struct ams_delta_nand *priv, u_char byte)
 {
-	struct nand_chip *this = mtd_to_nand(mtd);
-	struct ams_delta_nand *priv = nand_get_controller_data(this);
 	struct gpio_descs *data_gpiods = priv->data_gpiods;
 	unsigned long bits = byte;
 	int i;
@@ -108,10 +104,8 @@ static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
 	ams_delta_write_commit(priv);
 }
 
-static u_char ams_delta_read_next_byte(struct mtd_info *mtd)
+static u_char ams_delta_read_next_byte(struct ams_delta_nand *priv)
 {
-	struct nand_chip *this = mtd_to_nand(mtd);
-	struct ams_delta_nand *priv = nand_get_controller_data(this);
 	struct gpio_descs *data_gpiods = priv->data_gpiods;
 	unsigned long bits = 0;
 	int i, value_array[data_gpiods->ndescs];
@@ -131,38 +125,48 @@ static u_char ams_delta_read_next_byte(struct mtd_info *mtd)
 	return bits;
 }
 
-static u_char ams_delta_read_byte(struct mtd_info *mtd)
+static u_char ams_delta_read_first_byte(struct ams_delta_nand *priv)
 {
-	struct nand_chip *this = mtd_to_nand(mtd);
-	struct ams_delta_nand *priv = nand_get_controller_data(this);
 	struct gpio_descs *data_gpiods = priv->data_gpiods;
 	int i;
 
 	for (i = 0; i < data_gpiods->ndescs; i++)
 		gpiod_direction_input(data_gpiods->desc[i]);
 
-	return ams_delta_read_next_byte(mtd);
+	return ams_delta_read_next_byte(priv);
+}
+
+static u_char ams_delta_read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct ams_delta_nand *priv = nand_get_controller_data(this);
+
+	return ams_delta_read_first_byte(priv);
 }
 
 static void ams_delta_write_buf(struct mtd_info *mtd, const u_char *buf,
 				int len)
 {
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct ams_delta_nand *priv = nand_get_controller_data(this);
 	int i;
 
 	if (len > 0)
-		ams_delta_write_byte(mtd, buf[0]);
+		ams_delta_write_first_byte(priv, buf[0]);
 	for (i = 1; i < len; i++)
-		ams_delta_write_next_byte(mtd, buf[i]);
+		ams_delta_write_next_byte(priv, buf[i]);
 }
 
 static void ams_delta_read_buf(struct mtd_info *mtd, u_char *buf, int len)
 {
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct ams_delta_nand *priv = nand_get_controller_data(this);
 	int i;
 
 	if (len > 0)
-		buf[0] = ams_delta_read_byte(mtd);
+		buf[0] = ams_delta_read_first_byte(priv);
 	for (i = 1; i < len; i++)
-		buf[i] = ams_delta_read_next_byte(mtd);
+		buf[i] = ams_delta_read_next_byte(priv);
 }
 
 /*
@@ -186,7 +190,7 @@ static void ams_delta_hwcontrol(struct mtd_info *mtd, int cmd,
 	}
 
 	if (cmd != NAND_CMD_NONE)
-		ams_delta_write_byte(mtd, cmd);
+		ams_delta_write_first_byte(priv, cmd);
 }
 
 static int ams_delta_nand_ready(struct mtd_info *mtd)
-- 
2.16.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 08/12] mtd: rawnand: ams-delta: Simplify pointer resolution on read/write

[Boris Brezillon]

On Tue,  7 Aug 2018 00:29:14 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Simplify data read/write sub-functions by changing their APIs so they
> accept driver private structure pointer instead of mtd_info.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

Can you move that one earlier in the series so that it can be applied
even if we're still discussing the GPIO bitmap changes?

> ---
>  drivers/mtd/nand/raw/ams-delta.c | 40 ++++++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
> index d02c48c013e8..30c461138195 100644
> --- a/drivers/mtd/nand/raw/ams-delta.c
> +++ b/drivers/mtd/nand/raw/ams-delta.c
> @@ -76,10 +76,8 @@ static void ams_delta_write_commit(struct ams_delta_nand *priv)
>  	gpiod_set_value(priv->gpiod_nwe, 1);
>  }
>  
> -static void ams_delta_write_next_byte(struct mtd_info *mtd, u_char byte)
> +static void ams_delta_write_next_byte(struct ams_delta_nand *priv, u_char byte)
>  {
> -	struct nand_chip *this = mtd_to_nand(mtd);
> -	struct ams_delta_nand *priv = nand_get_controller_data(this);
>  	struct gpio_descs *data_gpiods = priv->data_gpiods;
>  	unsigned long bits = byte;
>  	int i, value_array[data_gpiods->ndescs];
> @@ -93,10 +91,8 @@ static void ams_delta_write_next_byte(struct mtd_info *mtd, u_char byte)
>  	ams_delta_write_commit(priv);
>  }
>  
> -static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
> +static void ams_delta_write_first_byte(struct ams_delta_nand *priv, u_char byte)
>  {
> -	struct nand_chip *this = mtd_to_nand(mtd);
> -	struct ams_delta_nand *priv = nand_get_controller_data(this);
>  	struct gpio_descs *data_gpiods = priv->data_gpiods;
>  	unsigned long bits = byte;
>  	int i;
> @@ -108,10 +104,8 @@ static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
>  	ams_delta_write_commit(priv);
>  }
>  
> -static u_char ams_delta_read_next_byte(struct mtd_info *mtd)
> +static u_char ams_delta_read_next_byte(struct ams_delta_nand *priv)
>  {
> -	struct nand_chip *this = mtd_to_nand(mtd);
> -	struct ams_delta_nand *priv = nand_get_controller_data(this);
>  	struct gpio_descs *data_gpiods = priv->data_gpiods;
>  	unsigned long bits = 0;
>  	int i, value_array[data_gpiods->ndescs];
> @@ -131,38 +125,48 @@ static u_char ams_delta_read_next_byte(struct mtd_info *mtd)
>  	return bits;
>  }
>  
> -static u_char ams_delta_read_byte(struct mtd_info *mtd)
> +static u_char ams_delta_read_first_byte(struct ams_delta_nand *priv)
>  {
> -	struct nand_chip *this = mtd_to_nand(mtd);
> -	struct ams_delta_nand *priv = nand_get_controller_data(this);
>  	struct gpio_descs *data_gpiods = priv->data_gpiods;
>  	int i;
>  
>  	for (i = 0; i < data_gpiods->ndescs; i++)
>  		gpiod_direction_input(data_gpiods->desc[i]);
>  
> -	return ams_delta_read_next_byte(mtd);
> +	return ams_delta_read_next_byte(priv);
> +}
> +
> +static u_char ams_delta_read_byte(struct mtd_info *mtd)
> +{
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct ams_delta_nand *priv = nand_get_controller_data(this);
> +
> +	return ams_delta_read_first_byte(priv);
>  }
>  
>  static void ams_delta_write_buf(struct mtd_info *mtd, const u_char *buf,
>  				int len)
>  {
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct ams_delta_nand *priv = nand_get_controller_data(this);
>  	int i;
>  
>  	if (len > 0)
> -		ams_delta_write_byte(mtd, buf[0]);
> +		ams_delta_write_first_byte(priv, buf[0]);
>  	for (i = 1; i < len; i++)
> -		ams_delta_write_next_byte(mtd, buf[i]);
> +		ams_delta_write_next_byte(priv, buf[i]);
>  }
>  
>  static void ams_delta_read_buf(struct mtd_info *mtd, u_char *buf, int len)
>  {
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct ams_delta_nand *priv = nand_get_controller_data(this);
>  	int i;
>  
>  	if (len > 0)
> -		buf[0] = ams_delta_read_byte(mtd);
> +		buf[0] = ams_delta_read_first_byte(priv);
>  	for (i = 1; i < len; i++)
> -		buf[i] = ams_delta_read_next_byte(mtd);
> +		buf[i] = ams_delta_read_next_byte(priv);
>  }
>  
>  /*
> @@ -186,7 +190,7 @@ static void ams_delta_hwcontrol(struct mtd_info *mtd, int cmd,
>  	}
>  
>  	if (cmd != NAND_CMD_NONE)
> -		ams_delta_write_byte(mtd, cmd);
> +		ams_delta_write_first_byte(priv, cmd);
>  }
>  
>  static int ams_delta_nand_ready(struct mtd_info *mtd)

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 08/12] mtd: rawnand: ams-delta: Simplify pointer resolution on read/write

[Janusz Krzysztofik]

On Tuesday, August 7, 2018 7:02:56 PM CEST Boris Brezillon wrote:
> On Tue,  7 Aug 2018 00:29:14 +0200
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> 
> > Simplify data read/write sub-functions by changing their APIs so they
> > accept driver private structure pointer instead of mtd_info.
> > 
> > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> 
> Can you move that one earlier in the series so that it can be applied
> even if we're still discussing the GPIO bitmap changes?

Sure, I will, and I would be still more happy if you agreed on me doing the 
same with [RFC PATCH v2 07/12] mtd: rawnand: ams-delta: Set port direction 
once per transfer.

Thanks,
Janusz


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v2 09/12] gpiolib: Identify GPIO descriptor arrays with direct mapping

[Janusz Krzysztofik]

Certain GPIO array lookup results may map directly to GPIO pins of a
single GPIO chip in hardware order.  If that condition is recognized
and handled efficiently, significant performance gain of get/set array
functions may be possible.

While processing a request for an array of GPIO descriptors, verify if
the descriptors just collected represent consecutive pins of a single
GPIO chip.  Pass that information with the array to the caller so it
can benefit from enhanced performance as soon as bitmap based get/set
array functions which can make efficient use of that are available.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 Documentation/driver-api/gpio/consumer.rst |  4 +++-
 drivers/gpio/gpiolib.c                     | 14 ++++++++++++++
 include/linux/gpio/consumer.h              |  1 +
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/driver-api/gpio/consumer.rst b/Documentation/driver-api/gpio/consumer.rst
index aa03f389d41d..38a990b5f3b6 100644
--- a/Documentation/driver-api/gpio/consumer.rst
+++ b/Documentation/driver-api/gpio/consumer.rst
@@ -109,11 +109,13 @@ For a function using multiple GPIOs all of those can be obtained with one call::
 					   enum gpiod_flags flags)
 
 This function returns a struct gpio_descs which contains an array of
-descriptors::
+descriptors.  It may also contain a valid descriptor of a single GPIO chip in
+case the array strictly matches pin hardware layout of the chip::
 
 	struct gpio_descs {
 		unsigned int ndescs;
 		struct gpio_desc *desc[];
+		struct gpio_chip *chip;
 	}
 
 The following function returns NULL instead of -ENOENT if no GPIOs have been
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bdbfc95793e7..c50bcec6e2d7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4161,6 +4161,7 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
 {
 	struct gpio_desc *desc;
 	struct gpio_descs *descs;
+	struct gpio_chip *chip;
 	int count;
 
 	count = gpiod_count(dev, con_id);
@@ -4177,6 +4178,19 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
 			gpiod_put_array(descs);
 			return ERR_CAST(desc);
 		}
+
+		/*
+		 * Verify if the array qualifies for fast bitmap operations
+		 * (single chip, pins in hardware order starting from 0)
+		 * and mark the array with the chip descriptor if true.
+		 */
+		chip = gpiod_to_chip(desc);
+		if (descs->chip == NULL)
+			descs->chip = chip;
+		if (!IS_ERR(descs->chip) && (chip != descs->chip ||
+		    gpio_chip_hwgpio(desc) != descs->ndescs))
+			descs->chip = ERR_PTR(-EINVAL);
+
 		descs->desc[descs->ndescs] = desc;
 		descs->ndescs++;
 	}
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 21ddbe440030..862ee027a02f 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -22,6 +22,7 @@ struct gpio_desc;
  * gpiod_get_array().
  */
 struct gpio_descs {
+	struct gpio_chip *chip;
 	unsigned int ndescs;
 	struct gpio_desc *desc[];
 };
-- 
2.16.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 09/12] gpiolib: Identify GPIO descriptor arrays with direct mapping

[Linus Walleij]

Hi Janusz!

On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Certain GPIO array lookup results may map directly to GPIO pins of a
> single GPIO chip in hardware order.  If that condition is recognized
> and handled efficiently, significant performance gain of get/set array
> functions may be possible.
>
> While processing a request for an array of GPIO descriptors, verify if
> the descriptors just collected represent consecutive pins of a single
> GPIO chip.  Pass that information with the array to the caller so it
> can benefit from enhanced performance as soon as bitmap based get/set
> array functions which can make efficient use of that are available.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
(...)
>  This function returns a struct gpio_descs which contains an array of
> -descriptors::
> +descriptors.  It may also contain a valid descriptor of a single GPIO chip in
> +case the array strictly matches pin hardware layout of the chip::
>
>         struct gpio_descs {
>                 unsigned int ndescs;
>                 struct gpio_desc *desc[];
> +               struct gpio_chip *chip;

This must be motivated: if the only purpose is to indicate to the consumer that
all GPIOs are on the same chip, why not just have a

bool all_on_same_chip;

That you set to true if these are all on the same chip?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 09/12] gpiolib: Identify GPIO descriptor arrays with direct mapping

[Janusz Krzysztofik]

Hi Linus,

On Tuesday, August 7, 2018 1:29:43 AM CEST Linus Walleij wrote:
> Hi Janusz!
> 
> On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> 
wrote:
> 
> > Certain GPIO array lookup results may map directly to GPIO pins of a
> > single GPIO chip in hardware order.  If that condition is recognized
> > and handled efficiently, significant performance gain of get/set array
> > functions may be possible.
> >
> > While processing a request for an array of GPIO descriptors, verify if
> > the descriptors just collected represent consecutive pins of a single
> > GPIO chip.  Pass that information with the array to the caller so it
> > can benefit from enhanced performance as soon as bitmap based get/set
> > array functions which can make efficient use of that are available.
> >
> > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> (...)
> >  This function returns a struct gpio_descs which contains an array of
> > -descriptors::
> > +descriptors.  It may also contain a valid descriptor of a single GPIO 
chip in
> > +case the array strictly matches pin hardware layout of the chip::
> >
> >         struct gpio_descs {
> >                 unsigned int ndescs;
> >                 struct gpio_desc *desc[];
> > +               struct gpio_chip *chip;
> 
> This must be motivated: if the only purpose is to indicate to the consumer 
that
> all GPIOs are on the same chip, why not just have a
> 
> bool all_on_same_chip;
> 
> That you set to true if these are all on the same chip?

My approach would probably save one or two instructions per get/set call, but 
I'm not stuck to it and will be happy to find a better solution.

How about folding the chip descriptor inside an additional structure, private 
to drivers, with internals not revealed to consumers?

Thanks,
Janusz


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 09/12] gpiolib: Identify GPIO descriptor arrays with direct mapping

[Boris Brezillon]

On Tue, 07 Aug 2018 18:50:22 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Hi Linus,
> 
> On Tuesday, August 7, 2018 1:29:43 AM CEST Linus Walleij wrote:
> > Hi Janusz!
> > 
> > On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com>   
> wrote:
> >   
> > > Certain GPIO array lookup results may map directly to GPIO pins of a
> > > single GPIO chip in hardware order.  If that condition is recognized
> > > and handled efficiently, significant performance gain of get/set array
> > > functions may be possible.
> > >
> > > While processing a request for an array of GPIO descriptors, verify if
> > > the descriptors just collected represent consecutive pins of a single
> > > GPIO chip.  Pass that information with the array to the caller so it
> > > can benefit from enhanced performance as soon as bitmap based get/set
> > > array functions which can make efficient use of that are available.
> > >
> > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>  
> > (...)  
> > >  This function returns a struct gpio_descs which contains an array of
> > > -descriptors::
> > > +descriptors.  It may also contain a valid descriptor of a single GPIO   
> chip in
> > > +case the array strictly matches pin hardware layout of the chip::
> > >
> > >         struct gpio_descs {
> > >                 unsigned int ndescs;
> > >                 struct gpio_desc *desc[];
> > > +               struct gpio_chip *chip;  
> > 
> > This must be motivated: if the only purpose is to indicate to the consumer   
> that
> > all GPIOs are on the same chip, why not just have a
> > 
> > bool all_on_same_chip;
> > 
> > That you set to true if these are all on the same chip?  
> 
> My approach would probably save one or two instructions per get/set call, but 
> I'm not stuck to it and will be happy to find a better solution.
> 
> How about folding the chip descriptor inside an additional structure, private 
> to drivers, with internals not revealed to consumers?

Or just get the chip from gpio_descs->desc[0]->gdev->chip when
->all_on_same_chip is true...

That adds 2 dereferencing though.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 09/12] gpiolib: Identify GPIO descriptor arrays with direct mapping

[Boris Brezillon]

On Tue,  7 Aug 2018 00:29:15 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Certain GPIO array lookup results may map directly to GPIO pins of a
> single GPIO chip in hardware order.  If that condition is recognized
> and handled efficiently, significant performance gain of get/set array
> functions may be possible.
> 
> While processing a request for an array of GPIO descriptors, verify if
> the descriptors just collected represent consecutive pins of a single
> GPIO chip.  Pass that information with the array to the caller so it
> can benefit from enhanced performance as soon as bitmap based get/set
> array functions which can make efficient use of that are available.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
>  Documentation/driver-api/gpio/consumer.rst |  4 +++-
>  drivers/gpio/gpiolib.c                     | 14 ++++++++++++++
>  include/linux/gpio/consumer.h              |  1 +
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/driver-api/gpio/consumer.rst b/Documentation/driver-api/gpio/consumer.rst
> index aa03f389d41d..38a990b5f3b6 100644
> --- a/Documentation/driver-api/gpio/consumer.rst
> +++ b/Documentation/driver-api/gpio/consumer.rst
> @@ -109,11 +109,13 @@ For a function using multiple GPIOs all of those can be obtained with one call::
>  					   enum gpiod_flags flags)
>  
>  This function returns a struct gpio_descs which contains an array of
> -descriptors::
> +descriptors.  It may also contain a valid descriptor of a single GPIO chip in
> +case the array strictly matches pin hardware layout of the chip::
>  
>  	struct gpio_descs {
>  		unsigned int ndescs;
>  		struct gpio_desc *desc[];
> +		struct gpio_chip *chip;

chip is placed at the beginning of the struct in the real code, which
is expected since putting it at the end won't work because of the
desc[] declaration.

...

> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index 21ddbe440030..862ee027a02f 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -22,6 +22,7 @@ struct gpio_desc;
>   * gpiod_get_array().
>   */
>  struct gpio_descs {
> +	struct gpio_chip *chip;
>  	unsigned int ndescs;
>  	struct gpio_desc *desc[];
>  };

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 09/12] gpiolib: Identify GPIO descriptor arrays with direct mapping

[Janusz Krzysztofik]

On Tuesday, August 7, 2018 7:14:20 PM CEST Boris Brezillon wrote:
> On Tue,  7 Aug 2018 00:29:15 +0200
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> 
> > Certain GPIO array lookup results may map directly to GPIO pins of a
> > single GPIO chip in hardware order.  If that condition is recognized
> > and handled efficiently, significant performance gain of get/set array
> > functions may be possible.
> > 
> > While processing a request for an array of GPIO descriptors, verify if
> > the descriptors just collected represent consecutive pins of a single
> > GPIO chip.  Pass that information with the array to the caller so it
> > can benefit from enhanced performance as soon as bitmap based get/set
> > array functions which can make efficient use of that are available.
> > 
> > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > ---
> >  Documentation/driver-api/gpio/consumer.rst |  4 +++-
> >  drivers/gpio/gpiolib.c                     | 14 ++++++++++++++
> >  include/linux/gpio/consumer.h              |  1 +
> >  3 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/driver-api/gpio/consumer.rst b/Documentation/driver-api/gpio/consumer.rst
> > index aa03f389d41d..38a990b5f3b6 100644
> > --- a/Documentation/driver-api/gpio/consumer.rst
> > +++ b/Documentation/driver-api/gpio/consumer.rst
> > @@ -109,11 +109,13 @@ For a function using multiple GPIOs all of those can be obtained with one call::
> >  					   enum gpiod_flags flags)
> >  
> >  This function returns a struct gpio_descs which contains an array of
> > -descriptors::
> > +descriptors.  It may also contain a valid descriptor of a single GPIO chip in
> > +case the array strictly matches pin hardware layout of the chip::
> >  
> >  	struct gpio_descs {
> >  		unsigned int ndescs;
> >  		struct gpio_desc *desc[];
> > +		struct gpio_chip *chip;
> 
> chip is placed at the beginning of the struct in the real code, which
> is expected since putting it at the end won't work because of the
> desc[] declaration.

Yes, I've already noticed that and will fix on next iteration, thanks.

Janusz


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v2 10/12] gpiolib: Introduce bitmap get/set array API extension

[Janusz Krzysztofik]

Certain GPIO array lookups may return arrays marked as applicable for
fast get/set array operations.  In order to make use of that
information, a new API extension which allows passing it to get/set
functions is needed.

Create a set of frontends to get/set array functions which accept
strict descriptor array structures returned by gpiod_get_array()
instead of arbitrary descriptor arrays.

Since the intended purpose of the new API extension is to speed up
get/set array operations, also replace array of integer values argument
with their bitmap representation, ready for being passed directly to
chip callback functions, without iterating them.

Applicability of the new API is limited to arrays not exceeding bit
length of type unsigned long (32 pins).

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 Documentation/driver-api/gpio/consumer.rst |  26 ++++
 drivers/gpio/gpiolib.c                     | 209 +++++++++++++++++++++++++++++
 include/linux/gpio/consumer.h              |  14 ++
 3 files changed, 249 insertions(+)

diff --git a/Documentation/driver-api/gpio/consumer.rst b/Documentation/driver-api/gpio/consumer.rst
index 38a990b5f3b6..bec4eab3b87c 100644
--- a/Documentation/driver-api/gpio/consumer.rst
+++ b/Documentation/driver-api/gpio/consumer.rst
@@ -383,6 +383,32 @@ or negative on error. Note the difference to gpiod_get_value(), which returns
 0 or 1 on success to convey the GPIO value. With the array functions, the GPIO
 values are stored in value_array rather than passed back as return value.
 
+Additionally, the following variants of the above functions exist which operate
+on bitmaps of values instead of arrays of values::
+
+	int gpiod_get_array_bitmap(struct gpio_descs *desc_array,
+				   unsigned long *bits);
+	int gpiod_get_raw_array_bitmap(struct gpio_descs *desc_array,
+				       unsigned long *bits);
+	int gpiod_get_array_bitmap_cansleep(struct gpio_desc *desc_array,
+					    unsigned long *bits);
+	int gpiod_get_raw_array_bitmap_cansleep(struct gpio_desc **desc_array,
+						unsigned long *bits)
+
+	void gpiod_set_array_bitmap(struct gpio_descs *desc_array,
+				    unsigned long *bits)
+	void gpiod_set_raw_array_bitmap(struct gpio_descs *desc_array,
+				        unsigned long *bitmap)
+	void gpiod_set_array_bitmap_cansleep(struct gpio_descs *desc_array,
+					     unsigned long *bits)
+	void gpiod_set_raw_array_bitmap_cansleep(struct gpio_descs *desc_array,
+						 unsigned long *bits)
+
+Unlike their counterparts, these functions don't accept arbitrary GPIO
+descriptor arrays, only those of type struct gpio_descs returned by
+gpiod_get_array() and its variants.  Supported array size is limited to the size
+of the bitmap, i.e., sizeof(unsigned long).
+
 
 GPIOs mapped to IRQs
 --------------------
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c50bcec6e2d7..5b541364dee0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2836,6 +2836,27 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 	return 0;
 }
 
+int gpiod_get_array_bitmap_complex(bool raw, bool can_sleep,
+				   struct gpio_descs *array,
+				   unsigned long *bits)
+{
+	int value_array[sizeof(*bits)];
+	int i;
+
+	if (array->ndescs > sizeof(*bits))
+		return -EINVAL;
+
+	i = gpiod_get_array_value_complex(raw, can_sleep, array->ndescs,
+					  array->desc, value_array);
+	if (i)
+		return i;
+
+	for (i = 0; i < array->ndescs; i++)
+		__assign_bit(i, bits, value_array[i]);
+
+	return 0;
+}
+
 /**
  * gpiod_get_raw_value() - return a gpio's raw value
  * @desc: gpio whose value will be returned
@@ -2929,6 +2950,50 @@ int gpiod_get_array_value(unsigned int array_size,
 }
 EXPORT_SYMBOL_GPL(gpiod_get_array_value);
 
+/**
+ * gpiod_get_raw_array_bitmap() - read raw values from an array of GPIOs
+ * @desc_array: array of GPIO descriptors whose values will be read, obtained
+ *		with gpiod_get_array(),
+ * @bits: bitmap to store the read values
+ *
+ * Read the raw values of the GPIOs, i.e. the values of the physical lines
+ * without regard for their ACTIVE_LOW status.  Return 0 in case of success,
+ * else an error code.
+ *
+ * This function should be called from contexts where we cannot sleep,
+ * and it will complain if the GPIO chip functions potentially sleep.
+ */
+int gpiod_get_raw_array_bitmap(struct gpio_descs *desc_array,
+			       unsigned long *bits)
+{
+	if (!desc_array)
+		return -EINVAL;
+	return gpiod_get_array_bitmap_complex(true, false, desc_array, bits);
+}
+EXPORT_SYMBOL_GPL(gpiod_get_raw_array_bitmap);
+
+/**
+ * gpiod_get_array_bitmap() - read values from an array of GPIOs
+ * @desc_array: array of GPIO descriptors whose values will be read, obtained
+ *		with gpiod_get_array(),
+ * @bits: bitmap to store the read values
+ *
+ * Read the logical values of the GPIOs, i.e. taking their ACTIVE_LOW status
+ * into account.  Return 0 in case of success, else an error code.
+ *
+ * This function should be called from contexts where we cannot sleep,
+ * and it will complain if the GPIO chip functions potentially sleep.
+ */
+
+int gpiod_get_array_bitmap(struct gpio_descs *desc_array,
+			   unsigned long *bits)
+{
+	if (!desc_array)
+		return -EINVAL;
+	return gpiod_get_array_bitmap_complex(false, false, desc_array, bits);
+}
+EXPORT_SYMBOL_GPL(gpiod_get_raw_array_bitmap);
+
 /*
  *  gpio_set_open_drain_value_commit() - Set the open drain gpio's value.
  * @desc: gpio descriptor whose state need to be set.
@@ -3081,6 +3146,23 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 	return 0;
 }
 
+int gpiod_set_array_bitmap_complex(bool raw, bool can_sleep,
+				   struct gpio_descs *array,
+				   unsigned long *bits)
+{
+	int value_array[sizeof(*bits)];
+	int i;
+
+	if (array->ndescs > sizeof(*bits))
+		return -EINVAL;
+
+	for (i = 0; i < array->ndescs; i++)
+		value_array[i] = test_bit(i, bits);
+
+	return gpiod_set_array_value_complex(raw, can_sleep, array->ndescs,
+					     array->desc, value_array);
+}
+
 /**
  * gpiod_set_raw_value() - assign a gpio's raw value
  * @desc: gpio whose value will be assigned
@@ -3185,6 +3267,48 @@ void gpiod_set_array_value(unsigned int array_size,
 }
 EXPORT_SYMBOL_GPL(gpiod_set_array_value);
 
+/**
+ * gpiod_set_raw_array_bitmap() - assign values to an array of GPIOs
+ * @desc_array: array of GPIO descriptors whose values will be assigned,
+ *		obtained with gpiod_get_array(),
+ * @bits: bitmap of values to assign
+ *
+ * Set the raw values of the GPIOs, i.e. the values of the physical lines
+ * without regard for their ACTIVE_LOW status.
+ *
+ * This function should be called from contexts where we cannot sleep, and will
+ * complain if the GPIO chip functions potentially sleep.
+ */
+int gpiod_set_raw_array_bitmap(struct gpio_descs *desc_array,
+			       unsigned long *bits)
+{
+	if (!desc_array)
+		return -EINVAL;
+	return gpiod_set_array_bitmap_complex(true, false, desc_array, bits);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_raw_array_bitmap);
+
+/**
+ * gpiod_set_array_bitmap() - assign values to an array of GPIOs
+ * @array_size: number of elements in the descriptor / value arrays
+ * @desc_array: array of GPIO descriptors whose values will be assigned
+ * @bits: bitmap of values to assign
+ *
+ * Set the logical values of the GPIOs, i.e. taking their ACTIVE_LOW status
+ * into account.
+ *
+ * This function should be called from contexts where we cannot sleep, and will
+ * complain if the GPIO chip functions potentially sleep.
+ */
+void gpiod_set_array_bitmap(struct gpio_descs *desc_array,
+			    unsigned long *bits)
+{
+	if (!desc_array)
+		return;
+	gpiod_set_array_bitmap_complex(false, false, desc_array, bits);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_array_bitmap);
+
 /**
  * gpiod_cansleep() - report whether gpio value access may sleep
  * @desc: gpio to check
@@ -3446,6 +3570,49 @@ int gpiod_get_array_value_cansleep(unsigned int array_size,
 }
 EXPORT_SYMBOL_GPL(gpiod_get_array_value_cansleep);
 
+/**
+ * gpiod_get_raw_array_bitmap_cansleep() - read raw values from array of GPIOs
+ * @desc_array: array of GPIO descriptors whose values will be read, obtained
+ *		with gpiod_get_array(),
+ * @bits: bitmap to store the read values
+ *
+ * Read the raw values of the GPIOs, i.e. the values of the physical lines
+ * without regard for their ACTIVE_LOW status.  Return 0 in case of success,
+ * else an error code.
+ *
+ * This function is to be called from contexts that can sleep.
+ */
+int gpiod_get_raw_array_bitmap_cansleep(struct gpio_descs *desc_array,
+					unsigned long *bits)
+{
+	might_sleep_if(extra_checks);
+	if (!desc_array)
+		return -EINVAL;
+	return gpiod_get_array_bitmap_complex(true, true, desc_array, bits);
+}
+EXPORT_SYMBOL_GPL(gpiod_get_raw_array_bitmap_cansleep);
+
+/**
+ * gpiod_get_array_bitmap_cansleep() - read values from an array of GPIOs
+ * @desc_array: array of GPIO descriptors whose values will be read, obtained
+ *		with gpiod_get_array(),
+ * @bits: bitmap to store the read values
+ *
+ * Read the logical values of the GPIOs, i.e. taking their ACTIVE_LOW status
+ * into account.  Return 0 in case of success, else an error code.
+ *
+ * This function is to be called from contexts that can sleep.
+ */
+int gpiod_get_array_bitmap_cansleep(struct gpio_descs *desc_array,
+				    unsigned long *bits)
+{
+	might_sleep_if(extra_checks);
+	if (!desc_array)
+		return -EINVAL;
+	return gpiod_get_array_bitmap_complex(false, true, desc_array, bits);
+}
+EXPORT_SYMBOL_GPL(gpiod_get_array_bitmap_cansleep);
+
 /**
  * gpiod_set_raw_value_cansleep() - assign a gpio's raw value
  * @desc: gpio whose value will be assigned
@@ -3545,6 +3712,48 @@ void gpiod_set_array_value_cansleep(unsigned int array_size,
 }
 EXPORT_SYMBOL_GPL(gpiod_set_array_value_cansleep);
 
+/**
+ * gpiod_set_raw_array_bitmap_cansleep() - assign values to an array of GPIOs
+ * @desc_array: array of GPIO descriptors whose values will be assigned,
+ *		obtained with gpiod_get_array(),
+ * @bits: bitmap of values to assign
+ *
+ * Set the raw values of the GPIOs, i.e. the values of the physical lines
+ * without regard for their ACTIVE_LOW status.
+ *
+ * This function is to be called from contexts that can sleep.
+ */
+int gpiod_set_raw_array_bitmap_cansleep(struct gpio_descs *desc_array,
+					unsigned long *bits)
+{
+	might_sleep_if(extra_checks);
+	if (!desc_array)
+		return -EINVAL;
+	return gpiod_set_array_bitmap_complex(true, true, desc_array, bits);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_raw_array_bitmap_cansleep);
+
+/**
+ * gpiod_set_array_bitmap_cansleep() - assign values to an array of GPIOs
+ * @desc_array: array of GPIO descriptors whose values will be assigned,
+ *		obtained with gpiod_get_array(),
+ * @bits: bitmap of values to assign
+ *
+ * Set the logical values of the GPIOs, i.e. taking their ACTIVE_LOW status
+ * into account.
+ *
+ * This function is to be called from contexts that can sleep.
+ */
+void gpiod_set_array_bitmap_cansleep(struct gpio_descs *desc_array,
+				     unsigned long *bits)
+{
+	might_sleep_if(extra_checks);
+	if (!desc_array)
+		return;
+	gpiod_set_array_bitmap_complex(false, true, desc_array, bits);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_array_bitmap_cansleep);
+
 /**
  * gpiod_add_lookup_table() - register GPIO device consumers
  * @table: table of consumers to register
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 862ee027a02f..1eabce4fc6c5 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -106,35 +106,49 @@ int gpiod_direction_output_raw(struct gpio_desc *desc, int value);
 int gpiod_get_value(const struct gpio_desc *desc);
 int gpiod_get_array_value(unsigned int array_size,
 			  struct gpio_desc **desc_array, int *value_array);
+int gpiod_get_array_bitmap(struct gpio_descs *desc_array, unsigned long *bits);
 void gpiod_set_value(struct gpio_desc *desc, int value);
 void gpiod_set_array_value(unsigned int array_size,
 			   struct gpio_desc **desc_array, int *value_array);
+void gpiod_set_array_bitmap(struct gpio_descs *desc_array, unsigned long *bits);
 int gpiod_get_raw_value(const struct gpio_desc *desc);
 int gpiod_get_raw_array_value(unsigned int array_size,
 			      struct gpio_desc **desc_array,
 			      int *value_array);
+int gpiod_get_raw_array_bitmap(struct gpio_descs *desc_array,
+			       unsigned long *bits);
 void gpiod_set_raw_value(struct gpio_desc *desc, int value);
 int gpiod_set_raw_array_value(unsigned int array_size,
 			       struct gpio_desc **desc_array,
 			       int *value_array);
+int gpiod_set_raw_array_bitmap(struct gpio_descs *desc_array,
+			       unsigned long *bits);
 
 /* Value get/set from sleeping context */
 int gpiod_get_value_cansleep(const struct gpio_desc *desc);
 int gpiod_get_array_value_cansleep(unsigned int array_size,
 				   struct gpio_desc **desc_array,
 				   int *value_array);
+int gpiod_get_array_bitmap_cansleep(struct gpio_descs *desc_array,
+				    unsigned long *bits);
 void gpiod_set_value_cansleep(struct gpio_desc *desc, int value);
 void gpiod_set_array_value_cansleep(unsigned int array_size,
 				    struct gpio_desc **desc_array,
 				    int *value_array);
+void gpiod_set_array_bitmap_cansleep(struct gpio_descs *desc_array,
+				     unsigned long *bits);
 int gpiod_get_raw_value_cansleep(const struct gpio_desc *desc);
 int gpiod_get_raw_array_value_cansleep(unsigned int array_size,
 				       struct gpio_desc **desc_array,
 				       int *value_array);
+int gpiod_get_raw_array_bitmap_cansleep(struct gpio_descs *desc_array,
+					unsigned long *bits);
 void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value);
 int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
 					struct gpio_desc **desc_array,
 					int *value_array);
+int gpiod_set_raw_array_bitmap_cansleep(struct gpio_descs *desc_array,
+					unsigned long *bits);
 
 int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
 int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
-- 
2.16.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v2 11/12] mtd: rawnand: ams-delta: Use GPIO API bitmap extension

[Janusz Krzysztofik]

Try to address the driver performance issues by replacing traditional
get/set array function calls with their bitmap based equivalents.

As long as fast bitmap processing path is not implemented in the new
API extension, performance of the driver remains unchanged.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/mtd/nand/raw/ams-delta.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index 30c461138195..7b08b2c441d3 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -78,15 +78,9 @@ static void ams_delta_write_commit(struct ams_delta_nand *priv)
 
 static void ams_delta_write_next_byte(struct ams_delta_nand *priv, u_char byte)
 {
-	struct gpio_descs *data_gpiods = priv->data_gpiods;
 	unsigned long bits = byte;
-	int i, value_array[data_gpiods->ndescs];
-
-	for (i = 0; i < data_gpiods->ndescs; i++)
-		value_array[i] = test_bit(i, &bits);
 
-	gpiod_set_raw_array_value(data_gpiods->ndescs, data_gpiods->desc,
-				  value_array);
+	gpiod_set_raw_array_bitmap(priv->data_gpiods, &bits);
 
 	ams_delta_write_commit(priv);
 }
@@ -106,22 +100,15 @@ static void ams_delta_write_first_byte(struct ams_delta_nand *priv, u_char byte)
 
 static u_char ams_delta_read_next_byte(struct ams_delta_nand *priv)
 {
-	struct gpio_descs *data_gpiods = priv->data_gpiods;
-	unsigned long bits = 0;
-	int i, value_array[data_gpiods->ndescs];
+	unsigned long bits;
 
 	gpiod_set_value(priv->gpiod_nre, 0);
 	ndelay(40);
 
-	gpiod_get_raw_array_value(data_gpiods->ndescs, data_gpiods->desc,
-				  value_array);
+	gpiod_get_raw_array_bitmap(priv->data_gpiods, &bits);
 
 	gpiod_set_value(priv->gpiod_nre, 1);
 
-	for (i = 0; i < data_gpiods->ndescs; i++)
-		if (value_array[i])
-			__set_bit(i, &bits);
-
 	return bits;
 }
 
-- 
2.16.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v2 12/12] gpiolib: Add fast processing path to bitmap API functions

[Janusz Krzysztofik]

Certain GPIO descriptor arrays returned by gpio_get_array() may contain
information on a single GPIO chip driving array member pins in hardware
order.  In such cases, bitmaps of values can be passed directly to the
chip callback functions without wasting time on iterations.

Add respective code to gpiod_get/set_array_bitmap_complex() functions.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 Documentation/driver-api/gpio/consumer.rst |  6 ++++++
 drivers/gpio/gpiolib.c                     | 14 ++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/Documentation/driver-api/gpio/consumer.rst b/Documentation/driver-api/gpio/consumer.rst
index bec4eab3b87c..b82f134dc352 100644
--- a/Documentation/driver-api/gpio/consumer.rst
+++ b/Documentation/driver-api/gpio/consumer.rst
@@ -409,6 +409,12 @@ descriptor arrays, only those of type struct gpio_descs returned by
 gpiod_get_array() and its variants.  Supported array size is limited to the size
 of the bitmap, i.e., sizeof(unsigned long).
 
+If the .chip member of the array structure, filled in by gpiod_get_array() in
+certain circumstances, contains a valid GPIO chip descriptor, the raw variants
+of the functions can take fast processing paths, passing bitmap arguments
+directly to the chip callback functions.  That allows for utilization of GPIO
+banks as data I/O ports without much loss of performance.
+
 
 GPIOs mapped to IRQs
 --------------------
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5b541364dee0..bf95f2964bc5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2846,6 +2846,12 @@ int gpiod_get_array_bitmap_complex(bool raw, bool can_sleep,
 	if (array->ndescs > sizeof(*bits))
 		return -EINVAL;
 
+	if (raw && !IS_ERR_OR_NULL(array->chip)) {
+		unsigned long mask = (1ULL << array->ndescs) - 1;
+
+		return gpio_chip_get_multiple(array->chip, &mask, bits);
+	}
+
 	i = gpiod_get_array_value_complex(raw, can_sleep, array->ndescs,
 					  array->desc, value_array);
 	if (i)
@@ -3156,6 +3162,14 @@ int gpiod_set_array_bitmap_complex(bool raw, bool can_sleep,
 	if (array->ndescs > sizeof(*bits))
 		return -EINVAL;
 
+	if (raw && !IS_ERR_OR_NULL(array->chip)) {
+		unsigned long mask = (1ULL << array->ndescs) - 1;
+
+		gpio_chip_set_multiple(array->chip, &mask, bits);
+
+		return 0;
+	}
+
 	for (i = 0; i < array->ndescs; i++)
 		value_array[i] = test_bit(i, bits);
 
-- 
2.16.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 12/12] gpiolib: Add fast processing path to bitmap API functions

[Linus Walleij]

On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

Hi Janusz!

> Certain GPIO descriptor arrays returned by gpio_get_array() may contain
> information on a single GPIO chip driving array member pins in hardware
> order.  In such cases, bitmaps of values can be passed directly to the
> chip callback functions without wasting time on iterations.
>
> Add respective code to gpiod_get/set_array_bitmap_complex() functions.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

I think it would be disappointing to leave all the users of the old
array API without the performance improvement. I think we need to
deal with this in a way such that everyone can benefit from it.

Also it is kludgy if users (consumers) would need to handle the case
where all lines are on the same chip separately, through the bitmap
API.

What we need is an API that:

- All drivers handling arrays can use (including current users).

- Enables speed-up if the lines are all on the same chip/register.

- Doesn't require consumers to know if they are all on the same
  chip or not.

This means a deep API with a small surface.

How do we achieve this the best way?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 12/12] gpiolib: Add fast processing path to bitmap API functions

[Janusz Krzysztofik]

On Tuesday, August 7, 2018 1:43:56 AM CEST Linus Walleij wrote:
> On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> 
wrote:
> 
> Hi Janusz!
> 
> > Certain GPIO descriptor arrays returned by gpio_get_array() may contain
> > information on a single GPIO chip driving array member pins in hardware
> > order.  In such cases, bitmaps of values can be passed directly to the
> > chip callback functions without wasting time on iterations.
> >
> > Add respective code to gpiod_get/set_array_bitmap_complex() functions.
> >
> > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> 
> I think it would be disappointing to leave all the users of the old
> array API without the performance improvement. I think we need to
> deal with this in a way such that everyone can benefit from it.

There are a few issues to be resolved:

1) array size limited by bitmap size:
    - are we ready to limit array size to a single bitmap for all users?
    - if not, how can we pass a bitmap of an arbitrary size?
    - if as an array of bitmaps, is that still clear enough and easy to use?
    - other ideas?

2) arbitrary array support:
     - are we ready to drop that?
     - if not, do we agree to require all users to pack their arbitrary arrays 
        inside the gpio_descs structure?

Maybe more.

> Also it is kludgy if users (consumers) would need to handle the case
> where all lines are on the same chip separately, through the bitmap
> API.

Not true as long as array size fits (arbitrary arrays can be packed by users), 
but I see your point.

> What we need is an API that:
> 
> - All drivers handling arrays can use (including current users).
> 
> - Enables speed-up if the lines are all on the same chip/register.
> 
> - Doesn't require consumers to know if they are all on the same
>   chip or not.
> 
> This means a deep API with a small surface.
> 
> How do we achieve this the best way?

I think widely accepted solutions to those two issues I've mentioned above can 
give the answer.

Thanks,
Janusz


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 12/12] gpiolib: Add fast processing path to bitmap API functions

[Boris Brezillon]

Hi Janusz,

On Tue, 07 Aug 2018 19:29:53 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> On Tuesday, August 7, 2018 1:43:56 AM CEST Linus Walleij wrote:
> > On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com>   
> wrote:
> > 
> > Hi Janusz!
> >   
> > > Certain GPIO descriptor arrays returned by gpio_get_array() may contain
> > > information on a single GPIO chip driving array member pins in hardware
> > > order.  In such cases, bitmaps of values can be passed directly to the
> > > chip callback functions without wasting time on iterations.
> > >
> > > Add respective code to gpiod_get/set_array_bitmap_complex() functions.
> > >
> > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>  
> > 
> > I think it would be disappointing to leave all the users of the old
> > array API without the performance improvement. I think we need to
> > deal with this in a way such that everyone can benefit from it.  

I agree with Linus on that one. When I initially proposed the gpio
bitbanging API I had something more advanced in mind where the GPIO
framework would be responsible for toggling the GPIOs on its own when
it's given an array of bytes to transmit (this way you avoid going
back and forth between the GPIO user and the GPIO framework). But this
approach would clearly be more invasive than what you propose
here (turning the int array into a bitmap and optimizing). So, if we go
for the "int array -> bitmap" approach I think all users should be
converted so that we end up with a single API.

> 
> There are a few issues to be resolved:
> 
> 1) array size limited by bitmap size:
>     - are we ready to limit array size to a single bitmap for all users?
>     - if not, how can we pass a bitmap of an arbitrary size?
>     - if as an array of bitmaps, is that still clear enough and easy to use?
>     - other ideas?

What we call a bitmap is an array of unsigned longs each entry
containing NBITS_PER_LONG bits, so yes, it's an arbitrary size (see the
bitmap API here [1]).

> 
> 2) arbitrary array support:
>      - are we ready to drop that?
>      - if not, do we agree to require all users to pack their arbitrary arrays 
>         inside the gpio_descs structure?

I could only find one user, and it's the core itself (for the ioctl),
so that shouldn't be too hard to convert all users. Did you find more.

> 
> Maybe more.
> 
> > Also it is kludgy if users (consumers) would need to handle the case
> > where all lines are on the same chip separately, through the bitmap
> > API.  
> 
> Not true as long as array size fits (arbitrary arrays can be packed by users), 
> but I see your point.

I think the API should be the same and the framework should decide to
take the fast path if all gpios belong to the same chip (which AFAICT
is already the case, except it's putting the result in an int array
instead of a bitmap)

> 
> > What we need is an API that:
> > 
> > - All drivers handling arrays can use (including current users).
> > 
> > - Enables speed-up if the lines are all on the same chip/register.
> > 
> > - Doesn't require consumers to know if they are all on the same
> >   chip or not.
> > 
> > This means a deep API with a small surface.
> > 
> > How do we achieve this the best way?  
> 
> I think widely accepted solutions to those two issues I've mentioned above can 
> give the answer.

I'd still like to see how far we are from the initial perfs (the one
poking the GPIO regs directly) with this approach, and what's the
improvement compared to the int array solution we already have in place.

Regards,

Boris

[1]https://elixir.bootlin.com/linux/v4.18-rc8/source/include/linux/bitmap.h
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 12/12] gpiolib: Add fast processing path to bitmap API functions

[Linus Walleij]

On Tue, Aug 7, 2018 at 7:47 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
>
> > On Tuesday, August 7, 2018 1:43:56 AM CEST Linus Walleij wrote:
> > > On Tue, Aug 7, 2018 at 12:29 AM Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > wrote:
> > >
> > > Hi Janusz!
> > >
> > > > Certain GPIO descriptor arrays returned by gpio_get_array() may contain
> > > > information on a single GPIO chip driving array member pins in hardware
> > > > order.  In such cases, bitmaps of values can be passed directly to the
> > > > chip callback functions without wasting time on iterations.
> > > >
> > > > Add respective code to gpiod_get/set_array_bitmap_complex() functions.
> > > >
> > > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > >
> > > I think it would be disappointing to leave all the users of the old
> > > array API without the performance improvement. I think we need to
> > > deal with this in a way such that everyone can benefit from it.
>
> I agree with Linus on that one. When I initially proposed the gpio
> bitbanging API I had something more advanced in mind where the GPIO
> framework would be responsible for toggling the GPIOs on its own when
> it's given an array of bytes to transmit (this way you avoid going
> back and forth between the GPIO user and the GPIO framework). But this
> approach would clearly be more invasive than what you propose
> here (turning the int array into a bitmap and optimizing). So, if we go
> for the "int array -> bitmap" approach I think all users should be
> converted so that we end up with a single API.

I thought about this the recent days and something must have gone
wrong in the development process of the array API because this
was the (mine atleast) intention all the time.

If we look at the GPIOchip driver API it looks like this:

        int                     (*get_multiple)(struct gpio_chip *chip,
                                                unsigned long *mask,
                                                unsigned long *bits);
        void                    (*set_multiple)(struct gpio_chip *chip,
                                                unsigned long *mask,
                                                unsigned long *bits);

So there is nothing hindering the drivers from optimizing a call
here into a single register write, which is what e.g. the gpio-mmio.c
driver does: if the hardware has a dedicated register for clearing
and setting lines, it will simply just write the register with
whatever is passed in, also cache the current value so it doesn't
need to read back the register every time.

When an array comes down to gpiod_set_array_value_complex()
it loops over the descriptors in order to handle e.g. open drain
settings separately. Then the remainder (lines that should just
be set 1/0) is pushed to the .set_multiple() callback if they
are on the same chip.

This is assuming:

1. The CPU is not the bottleneck so we can do a bit
  of complex looping over structs etc in each write.

2. We want to perform as much in a single register write
  as possible to avoid I/O and glitches as all lines (e.g.
  clock and data) get written at the same time, if possible.
  (No skew.)

It seems Janusz has problems with assumption (1) and therefore
is trying to optimize the read/write path. This can be done if all
descs are on the same chip and none of them is using open drain
or open source.

To keep track of "quick path" the array needs to have a state.
So a magic "cookie" or something like that needs to be passed
to the array API.

I would suggest that struct gpiod_descs contain a magic cookie
returned from [devm_]gpiod_get_array[_optional]()  that can be
passed along to get/set array operations or left as NULL to just
fall back to the default:

void gpiod_set_array_value(unsigned int array_size,
                           struct gpio_desc **desc_array, int *value_array,
                           struct gpiod_array_cookie *cookie);

Cookie is just a dummy name, I don't know what makes most
sense. It reflects a state for the entire array.

If this cookie exist in some struct gpio_chip state variable, it
informs gpiolib that this array:

- Has all descriptors in the same gpiochip
- Has no open drain or open source-flagged lines

It can thenbypass the complex check and just write the values
directly by calling down into .set_multiple().

Maybe this cookie could just be a bool that is true when the above
is fulfilled. But it's best if that is hidden from the consumers
I guess, they shouldn't try to half-guess if the criteria is true,
gpiolib should do that.

The current users would have to be augmented to store the
cookie and pass it along when getting/setting arrays, but it would
be pretty simple and straight-forward compared to adding a new
API.

Yours,
Linus Walleij