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

[Roger Quadros <rogerq@kernel.org>]

Hi,

On 15/09/2022 12:13, B. Niedermayr wrote:
> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> 
> The waitpin polarity can be configured via the WAITPIN<X>POLARITY bits
> in the GPMC_CONFIG register. This is currently not supported by the
> driver. This patch adds support for setting the required register bits
> with the "gpmc,wait-pin-polarity" dt-property.
> 
> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> ---
>  drivers/memory/omap-gpmc.c              | 22 ++++++++++++++++++++++
>  include/linux/platform_data/gpmc-omap.h |  6 ++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index e3674a15b934..66dd7dd80653 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -132,6 +132,7 @@
>  #define GPMC_CONFIG_DEV_SIZE	0x00000002
>  #define GPMC_CONFIG_DEV_TYPE	0x00000003
>  
> +#define GPMC_CONFIG_WAITPINPOLARITY(pin)	(BIT(pin) << 8)
>  #define GPMC_CONFIG1_WRAPBURST_SUPP     (1 << 31)
>  #define GPMC_CONFIG1_READMULTIPLE_SUPP  (1 << 30)
>  #define GPMC_CONFIG1_READTYPE_ASYNC     (0 << 29)
> @@ -1881,6 +1882,21 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p)
>  
>  	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1);
>  
> +	if (p->wait_on_read || p->wait_on_write) {
> +		config1 = gpmc_read_reg(GPMC_CONFIG);
> +
> +		if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_LOW)
> +			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> +		else if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_HIGH)
> +			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> +		else if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT)
> +			pr_err("%s: invalid wait-pin-polarity (pin: %d, polarity: %d)\n",
> +				__func__, p->wait_pin, p->wait_pin_polarity);

We could avoid the print here. Instead the check for correct polarity
value and printing an error message on invalid values needs to be done
when we read the DT property in gpmc_read_settings_dt()

> +
> +		gpmc_write_reg(GPMC_CONFIG, config1);

How about avoiding the read/write altogether if
p->wait_pin_polarity == WAITPINPOLARITY_DEFAULT?

> +	}
> +
> +
>  	return 0;
>  }
>  
> @@ -1981,6 +1997,12 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
>  	}
>  
>  	if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) {
> +
New line not required.

> +		p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;

This should be moved before this 'if' block. Else we will have cases
where WAITPINPOLARITY_DEFAULT is not set when there was no "gpmc,wait-pin"
property in the DT node.

> +		of_property_read_u32(np,
> +			"gpmc,wait-pin-polarity",
> +			&p->wait_pin_polarity);
> +

Please sanity check p->wait_pin_polarity here.
If invalid value, print error and set to WAITPINPOLARITY_DEFAULT.

>  		p->wait_on_read = of_property_read_bool(np,
>  							"gpmc,wait-on-read");
>  		p->wait_on_write = of_property_read_bool(np,
> diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h
> index c9cc4e32435d..c46c28069c31 100644
> --- a/include/linux/platform_data/gpmc-omap.h
> +++ b/include/linux/platform_data/gpmc-omap.h
> @@ -136,6 +136,11 @@ struct gpmc_device_timings {
>  #define GPMC_MUX_AAD			1	/* Addr-Addr-Data multiplex */
>  #define GPMC_MUX_AD			2	/* Addr-Data multiplex */
>  
> +/* Wait pin polarity values */
> +#define WAITPINPOLARITY_DEFAULT -1
> +#define WAITPINPOLARITY_ACTIVE_LOW 0
> +#define WAITPINPOLARITY_ACTIVE_HIGH 1
> +
>  struct gpmc_settings {
>  	bool burst_wrap;	/* enables wrap bursting */
>  	bool burst_read;	/* enables read page/burst mode */
> @@ -149,6 +154,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