Re: [PATCH v2 2/3] memory: omap-gpmc: add support for wait pin polarity

[Roger Quadros <rogerq@kernel.org>]

Hi,

On 05/09/2022 10:17, B. Niedermayr wrote:
> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> 
> Setting the wait pin polarity from the device tree is currently not
> possible. The device tree property "gpmc,wait-pin-polarity" can be used
> for that. If this property is missing the previous default value
> is used instead, which preserves backwards compatibility.
> 
> The wait pin polarity is then set via the gpiochip
> direction_input callback function.
> 
> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> ---
>  drivers/memory/omap-gpmc.c              | 30 ++++++++++++++++++++-----
>  include/linux/platform_data/gpmc-omap.h |  1 +
>  2 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 579903457415..be3c35ae9619 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -35,6 +35,8 @@
>  
>  #include <linux/platform_data/mtd-nand-omap2.h>
>  
> +#include "../gpio/gpiolib.h"
> +
>  #define	DEVICE_NAME		"omap-gpmc"
>  
>  /* GPMC register offsets */
> @@ -1980,6 +1982,11 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
>  							"gpmc,wait-on-read");
>  		p->wait_on_write = of_property_read_bool(np,
>  							 "gpmc,wait-on-write");
> +		p->wait_pin_polarity = of_property_read_u32(np,
> +								 "gpmc,wait-pin-polarity",
> +								 &p->wait_pin_polarity);

This is wrong. of_property_read_u32() returns 0 or error value. It does not return the properties value.
The properties value is already stored in the pointer passed in the 3rd argument.

> +		if (p->wait_pin_polarity < 0)
> +			p->wait_pin_polarity = GPIO_ACTIVE_HIGH;
>  		if (!p->wait_on_read && !p->wait_on_write)
>  			pr_debug("%s: rd/wr wait monitoring not enabled!\n",
>  				 __func__);
> @@ -2210,10 +2217,11 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  	if (gpmc_s.wait_on_read || gpmc_s.wait_on_write) {
>  		unsigned int wait_pin = gpmc_s.wait_pin;
>  
> -		waitpin_desc = gpiochip_request_own_desc(&gpmc->gpio_chip,
> -							 wait_pin, "WAITPIN",
> -							 GPIO_ACTIVE_HIGH,
> -							 GPIOD_IN);
> +		waitpin_desc =
> +			gpiochip_request_own_desc(&gpmc->gpio_chip,
> +				wait_pin, "WAITPIN",
> +				gpmc_s.wait_pin_polarity ? GPIO_ACTIVE_HIGH : GPIO_ACTIVE_LOW,

#define GPIO_ACTIVE_HIGH 0
#define GPIO_ACTIVE_LOW 1

Why not just retain the same logic for gpmc_s.wait_pin_polarity and the DT property?

> +				GPIOD_IN);

This change to gpiochip_request_own_desc() is not really required as we are merely reserving the GPIO so other users cannot use it. The wait_pin's polarity is actually set in GPMC_CONFIG register. GPMC wait_pin polarity logic is hard-wired and doesn't depend on GPIO subsystem for its polarity.

>  		if (IS_ERR(waitpin_desc)) {
>  			ret = PTR_ERR(waitpin_desc);
>  			if (ret == -EBUSY) {
> @@ -2342,7 +2350,19 @@ static int gpmc_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>  static int gpmc_gpio_direction_input(struct gpio_chip *chip,
>  				     unsigned int offset)
>  {
> -	return 0;	/* we're input only */
> +	u32 reg;
> +	struct gpio_desc *desc = gpiochip_get_desc(chip, offset);
> +
> +	offset += 8;
> +	reg = gpmc_read_reg(GPMC_CONFIG);
> +
> +	if (BIT(FLAG_ACTIVE_LOW) & desc->flags)
> +		reg &= ~BIT(offset);
> +	else
> +		reg |= BIT(offset);
> +
> +	gpmc_write_reg(GPMC_CONFIG, reg);
> +	return 0;

This is the wrong place to put this code.
Wait pin plarity logic has nothing to do with GPIO subsystem.

The GPMC_CONFIG wait_pin polarity setting needs to be set in gpmc_cs_program_settings().
You need to check gpmc_settings->wait_pin_polarity there and set the polarity flag in GPMC_CONFIG register accordingly.

>  }
>  
>  static int gpmc_gpio_direction_output(struct gpio_chip *chip,
> diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h
> index c9cc4e32435d..bf4f2246f31d 100644
> --- a/include/linux/platform_data/gpmc-omap.h
> +++ b/include/linux/platform_data/gpmc-omap.h
> @@ -149,6 +149,7 @@ struct gpmc_settings {
>  	u32 device_width;	/* device bus width (8 or 16 bit) */
>  	u32 mux_add_data;	/* multiplex address & data */
>  	u32 wait_pin;		/* wait-pin to be used */
> +	u32 wait_pin_polarity;	/* wait-pin polarity */
>  };
>  
>  /* Data for each chip select */

cheers,
-roger