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
