diff mbox series

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

Message ID 20220905071717.1500568-3-benedikt.niedermayr@siemens.com (mailing list archive)
State New, archived
Headers show
Series omap-gpmc wait pin additions | expand

Commit Message

B. Niedermayr Sept. 5, 2022, 7:17 a.m. UTC 
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(-)

Comments

Roger Quadros Sept. 5, 2022, 9:19 a.m. UTC | #1 
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
B. Niedermayr Sept. 5, 2022, 9:47 a.m. UTC | #2 
On Mon, 2022-09-05 at 12:19 +0300, Roger Quadros wrote:
> 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.
> 

Ah. Yes that's a mistake. Thanks for that hint!
A bool property will make things a bit easier at this point.


> > +		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_HI
> > GH,
> > -							 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?

Also makes sense!
> 
> > +				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.
> 
Ok than I will put this into gpmc_cs_programm_settings function.
This way, the "#include ../gpio/gpiolib.h" is also not required
anymore.

It wasn't very sure about why the waitpins where allocated via a
gpiochip. But I can image it was because of ressource locking of those
pins so they don't get allocated from somewhere else?


> >  }
> >  
> >  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

I will submit my changes during the day.
Thanks for the feedback!

cheers,
benedikt
diff mbox series

Patch

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);
+		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,
+				GPIOD_IN);
 		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;
 }
 
 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 */