Re: [RFC PATCH 7/8] mtd: rawnand: ams-delta: Check sanity of data GPIO resource

[Boris Brezillon]

On Thu, 19 Jul 2018 01:57:09 +0200
Janusz Krzysztofik <jmkrzyszt@xxxxxxxxx> wrote:

> The plan is to replace data port readw()/writew() operations with GPIO
> callbacks provided by gpio-omap driver.  For acceptable performance the
> GPIO chip must support get/set_multiple() GPIO callbacks.
> 
> In order to avoid data corruption, we require the array of data GPIO
> descriptors obtained with gpiod_get_array() to meet some strict
> requirements:
> - all pins must belong to the same single GPIO chip,

You shouldn't care. The parallel NAND interface has WE/RE signals to
trigger a write/read on the data bus, that means you can change data
signals independently without risking data corruption as long as RE/WE
stay high (or low, I don't remember the active state on these pins). Of
course it's slower if you have to toggle data pins independently, but
that's not your problem. It's up to the HW designer to route things
correctly.

> - array index of each pin descriptor must match its hardware number,

Again, this is not really a problem. You'll just have to swap bits if
this is not the case. Not a big deal.

> - pin polarity must not be inverted,

Why?

> - pin hardware configuration must not be open drain nor open source.

This should be taken care of when requesting the pins.

> 
> Let's implement the above described sanity checks before replacing the
> readw()/writew() operations witn GPIO callbacks.  If a check fails,
> return -EINVAL to indicate the board provided GPIO setup is invalid.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@xxxxxxxxx>
> ---
>  drivers/mtd/nand/raw/ams-delta.c | 87 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
> index ad62c0245458..bd501f385e78 100644
> --- a/drivers/mtd/nand/raw/ams-delta.c
> +++ b/drivers/mtd/nand/raw/ams-delta.c
> @@ -21,6 +21,7 @@
>  #include <linux/module.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>

Wow! This is a very very bad idea. There's a clear separation between
the GPIO consumer and the GPIO driver API for a good reason, and
you're violating this. linux/gpio/driver.h should only be included by
GPIO controller drivers.

>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/rawnand.h>
>  #include <linux/mtd/partitions.h>
> @@ -190,7 +191,9 @@ static int ams_delta_init(struct platform_device *pdev)
>  	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	void __iomem *io_base;
>  	struct gpio_descs *data_gpiods;
> -	int err = 0;
> +	struct gpio_chip *data_gpioc;
> +	unsigned long mask, bits;
> +	int i, err = 0;
>  
>  	if (!res)
>  		return -ENXIO;
> @@ -298,6 +301,88 @@ static int ams_delta_init(struct platform_device *pdev)
>  		goto out_mtd;
>  	}
>  
> +	/* Use GPIO chip of first data GPIO pin descriptor */
> +	data_gpioc = gpiod_to_chip(data_gpiods->desc[0]);
> +
> +	/*
> +	 * For acceptable performance require the data GPIO
> +	 * chip to support get/set_multiple() callbacks.
> +	 */
> +	if (!data_gpioc->get_multiple || !data_gpioc->set_multiple) {
> +		err = -EINVAL;
> +		dev_err(&pdev->dev,
> +			"data GPIO chip does not support get/set_multiple()\n");
> +		goto out_mtd;
> +	}
> +
> +	/* Verify if get_multiple() returns all pins low as initialized above */
> +	mask = (1 << data_gpiods->ndescs) - 1;
> +	err = data_gpioc->get_multiple(data_gpioc, &mask, &bits);

And this, you shouldn't do. You should instead go through the GPIO
consumer API to get the pin state (gpiod_get_raw_array_value()).

I guess you'd prefer to have the pin values in a bitmap instead of an
array of integers. That's probably something you can discuss with
Linus, see if he would accept to change the prototype of
gpiod_get_raw_array_value().

> +	if (err) {
> +		dev_err(&pdev->dev,
> +			"data GPIO chip get_multiple() failed: %d\n", err);
> +		goto out_mtd;
> +	}
> +	if (bits) {
> +		err = -EINVAL;
> +		dev_err(&pdev->dev,
> +			"mismmatch of data GPIO initial value: %lu\n", bits);
> +		goto out_mtd;
> +	}
> +
> +	/* Verify each data GPIO pin */
> +	for (i = 0; i < data_gpiods->ndescs; i++) {
> +		/* Require all pins belong to the same GPIO chip */
> +		if (gpiod_to_chip(data_gpiods->desc[i]) != data_gpioc) {
> +			err = -EINVAL;
> +			dev_err(&pdev->dev, "GPIO chip mismatch of data bit %d\n",
> +				i);
> +			goto out_mtd;
> +		}
> +
> +		/* Require all pins active high (not inverted) */
> +		if (gpiod_is_active_low(data_gpiods->desc[i])) {
> +			err = -EINVAL;
> +			dev_err(&pdev->dev,
> +				"unsupported polarity of data GPIO bit %d\n",
> +				i);
> +			goto out_mtd;
> +		}
> +
> +		/*
> +		 * Require pin gpiod array index to match hardware pin number.
> +		 * Verified by setting the pin high with gpiod_set_raw_value()
> +		 * then reading it back with gpiochip->get() for comparison.
> +		 */
> +		gpiod_set_raw_value(data_gpiods->desc[i], 1);
> +		err = data_gpioc->get(data_gpioc, i);
> +		if (err < 0) {
> +			dev_err(&pdev->dev,
> +				"data bit %d GPIO chip get() failed: %d\n", i,
> +				err);
> +			goto out_mtd;
> +		}
> +		if (!err) {
> +			err = -EINVAL;
> +			dev_err(&pdev->dev, "mismatch of data GPIO bit %d value\n",
> +				i);
> +			goto out_mtd;
> +		}
> +
> +		/*
> +		 * Check for unsupported pin hardware configuration.  Use
> +		 * just verified gpiod array index as hardware pin number.
> +		 */
> +		if (gpiochip_line_is_open_drain(data_gpioc, i) ||
> +		    gpiochip_line_is_open_source(data_gpioc, i)) {
> +			err = -EINVAL;
> +			dev_err(&pdev->dev,
> +				"unsupported mode of data GPIO bit %d\n",
> +				i);
> +			goto out_mtd;
> +		}
> +	}
> +
>  	/* Scan to find existence of the device */
>  	err = nand_scan(mtd, 1);
>  	if (err)