Patchwork [v4,1/4] mtd: nand: gpio: Add DT property to automatically determine bus width

login
register
mail settings
Submitter Alexander Shiyan
Date Aug. 16, 2013, 7:19 a.m.
Message ID <1376637583-12485-1-git-send-email-shc_work@mail.ru>
Download mbox | patch
Permalink /patch/267556/
State New
Headers show

Comments

Alexander Shiyan - Aug. 16, 2013, 7:19 a.m.
This patch adds a property to automatically determine the NAND
bus width. This property works if the bus width is not specified
explicitly.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 .../devicetree/bindings/mtd/gpio-control-nand.txt        |  2 ++
 drivers/mtd/nand/gpio.c                                  | 16 ++++++++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)
Brian Norris - Aug. 17, 2013, 9:14 p.m.
On Fri, Aug 16, 2013 at 11:19:42AM +0400, Alexander Shiyan wrote:
> This patch adds a property to automatically determine the NAND
> bus width. This property works if the bus width is not specified
> explicitly.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  .../devicetree/bindings/mtd/gpio-control-nand.txt        |  2 ++
>  drivers/mtd/nand/gpio.c                                  | 16 ++++++++++++----
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> index 36ef07d..91070d0 100644
> --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> @@ -19,6 +19,8 @@ Optional properties:
>    defaults to 1 byte.
>  - chip-delay : chip dependent delay for transferring data from array to
>    read registers (tR).  If not present then a default of 20us is used.
> +- gpio-control-nand,bank-width-auto : Device bus width is determined
> +  automatically if "bank-width" is omitted (Boolean).
>  - gpio-control-nand,io-sync-reg : A 64-bit physical address for a read
>    location used to guard against bus reordering with regards to accesses to
>    the GPIO's and the NAND flash data bus.  If present, then after changing
> diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
> index e826f89..8eea181 100644
> --- a/drivers/mtd/nand/gpio.c
> +++ b/drivers/mtd/nand/gpio.c
> @@ -116,6 +116,10 @@ static int gpio_nand_get_config_of(const struct device *dev,
>  			dev_err(dev, "invalid bank-width %u\n", val);
>  			return -EINVAL;
>  		}
> +	} else {

Combine with the 'if' and make it 'else if'.

> +		if (of_get_property(dev->of_node,
> +				    "gpio-control-nand,bank-width-auto", NULL))

of_property_read_bool()

> +			plat->options |= NAND_BUSWIDTH_AUTO;
>  	}
>  
>  	plat->gpio_rdy = of_get_gpio(dev->of_node, 0);
> @@ -223,6 +227,14 @@ static int gpio_nand_probe(struct platform_device *pdev)
>  	if (IS_ERR(chip->IO_ADDR_R))
>  		return PTR_ERR(chip->IO_ADDR_R);
>  
> +	ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat);
> +	if (ret)
> +		return ret;
> +
> +	if (resource_size(res) < 2)
> +		gpiomtd->plat.options &= ~(NAND_BUSWIDTH_16 |
> +					   NAND_BUSWIDTH_AUTO);

I can't quite figure out what the significance of resource_size(res) < 2
is. It doesn't quite match my reading of the device-tree documentation.
Maybe I'm being dense. Is the resource provided as either an 8-bit IO
address (length=1) or 16-bit IO address (length=2)?

> +
>  	res = gpio_nand_get_io_sync(pdev);
>  	if (res) {
>  		gpiomtd->io_sync = devm_ioremap_resource(&pdev->dev, res);
> @@ -230,10 +242,6 @@ static int gpio_nand_probe(struct platform_device *pdev)
>  			return PTR_ERR(gpiomtd->io_sync);
>  	}
>  
> -	ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat);
> -	if (ret)
> -		return ret;
> -
>  	ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE");
>  	if (ret)
>  		return ret;

I can fixup the first 2 without a resend. Depending on your response to
the third comment, I'll apply this.

Thanks,
Brian
Alexander Shiyan - Aug. 17, 2013, 9:27 p.m.
> On Fri, Aug 16, 2013 at 11:19:42AM +0400, Alexander Shiyan wrote:
> > This patch adds a property to automatically determine the NAND
> > bus width. This property works if the bus width is not specified
> > explicitly.
> > 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
[...]
> > +	if (resource_size(res) < 2)
> > +		gpiomtd->plat.options &= ~(NAND_BUSWIDTH_16 |
> > +					   NAND_BUSWIDTH_AUTO);
> 
> I can't quite figure out what the significance of resource_size(res) < 2
> is. It doesn't quite match my reading of the device-tree documentation.
> Maybe I'm being dense. Is the resource provided as either an 8-bit IO
> address (length=1) or 16-bit IO address (length=2)?

Size in bytes, so this is just a fault protect for length==1 && (busw==16 || busw==auto).
Do you think it's not necessary?

[...]
> I can fixup the first 2 without a resend. Depending on your response to
> the third comment, I'll apply this.

I'll prefer to resend all in next few days.
Thanks.

---
Stephen Warren - Aug. 19, 2013, 9:09 p.m.
On 08/16/2013 01:19 AM, Alexander Shiyan wrote:
> This patch adds a property to automatically determine the NAND
> bus width. This property works if the bus width is not specified
> explicitly.

> diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt

> +- gpio-control-nand,bank-width-auto : Device bus width is determined
> +  automatically if "bank-width" is omitted (Boolean).

How would a driver determine the bank width automatically; is there some
obvious way a driver should do this based on the/a NAND specification?
Is there more than one way this auto-detection could occur, and if so,
which of those methods is this property indicating will work?

I'd rather expect the property description include details of the
auto-detection mechanism that the DT states will work.

Patch

diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
index 36ef07d..91070d0 100644
--- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
@@ -19,6 +19,8 @@  Optional properties:
   defaults to 1 byte.
 - chip-delay : chip dependent delay for transferring data from array to
   read registers (tR).  If not present then a default of 20us is used.
+- gpio-control-nand,bank-width-auto : Device bus width is determined
+  automatically if "bank-width" is omitted (Boolean).
 - gpio-control-nand,io-sync-reg : A 64-bit physical address for a read
   location used to guard against bus reordering with regards to accesses to
   the GPIO's and the NAND flash data bus.  If present, then after changing
diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
index e826f89..8eea181 100644
--- a/drivers/mtd/nand/gpio.c
+++ b/drivers/mtd/nand/gpio.c
@@ -116,6 +116,10 @@  static int gpio_nand_get_config_of(const struct device *dev,
 			dev_err(dev, "invalid bank-width %u\n", val);
 			return -EINVAL;
 		}
+	} else {
+		if (of_get_property(dev->of_node,
+				    "gpio-control-nand,bank-width-auto", NULL))
+			plat->options |= NAND_BUSWIDTH_AUTO;
 	}
 
 	plat->gpio_rdy = of_get_gpio(dev->of_node, 0);
@@ -223,6 +227,14 @@  static int gpio_nand_probe(struct platform_device *pdev)
 	if (IS_ERR(chip->IO_ADDR_R))
 		return PTR_ERR(chip->IO_ADDR_R);
 
+	ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat);
+	if (ret)
+		return ret;
+
+	if (resource_size(res) < 2)
+		gpiomtd->plat.options &= ~(NAND_BUSWIDTH_16 |
+					   NAND_BUSWIDTH_AUTO);
+
 	res = gpio_nand_get_io_sync(pdev);
 	if (res) {
 		gpiomtd->io_sync = devm_ioremap_resource(&pdev->dev, res);
@@ -230,10 +242,6 @@  static int gpio_nand_probe(struct platform_device *pdev)
 			return PTR_ERR(gpiomtd->io_sync);
 	}
 
-	ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat);
-	if (ret)
-		return ret;
-
 	ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE");
 	if (ret)
 		return ret;