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.
> + 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
