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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxx
> > >
> > ---
> >  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





[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux