Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver

[Tomer Maimon <tmaimon77@xxxxxxxxx>]

Hi Linus,

On 30 July 2018 at 00:59, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:

On Thu, Jul 26, 2018 at 2:01 AM Tomer Maimon <tmaimon77@xxxxxxxxx> wrote:

> I initialize bgpio as follow:
>
>                         ret = bgpio_init(&pctrl->gpio_bank[id].gc,
>                                          pctrl->dev, 4,
>                                          pctrl->gpio_bank[id].base +
>                                          NPCM7XX_GP_N_DIN,
>                                          pctrl->gpio_bank[id].base +
>                                          NPCM7XX_GP_N_DOUT,
>                                          NULL,
>                                          NULL,
>                                          pctrl->gpio_bank[id].base +
>                                          NPCM7XX_GP_N_IEM,
>                                          BGPIOF_READ_OUTPUT_REG_SET);

(...)
> The problem occur when reading the GPIO value from bgpio_get_set function,
> because the directions value are inverse it reading the wrong I/O registers
>
> For direction out it reading dat register (instead of set register)
>
> For direction in it calling set register (instead of dat register)

Hm I don't quite get it... sorry. Maybe if you show your fix and what
you expect to happen I can understand better?

 

Of course, in the last patch sent three days a go (V3 - https://patchwork.ozlabs.org/patch/949942/) I did as follow to workaround the issue:

       int (*get)(struct gpio_chip *chip, unsigned offset);
       int (*get_multiple)(struct gpio_chip *chip, unsigned long *mask,
                           unsigned long *bits);

......

static int npcmgpio_get_value(struct gpio_chip *chip, unsigned int offset)
{
       struct npcm7xx_gpio *bank = gpiochip_get_data(chip);
       unsigned long tmp_bgpio_dir = bank->gc.bgpio_dir;
       int val;

       /*
        * sets bgpio_dir parameter value to the opposite value
        * for calling the right registers in bgpio_get_set
        * function
        */
       bank->gc.bgpio_dir = ~bank->gc.bgpio_dir;
       val = bank->get(chip, offset);
       bank->gc.bgpio_dir = tmp_bgpio_dir;

       return val;
}

static int npcmgpio_get_multiple_value(struct gpio_chip *chip,
                                      unsigned long *mask, unsigned long *bits)
{
       struct npcm7xx_gpio *bank = gpiochip_get_data(chip);
       unsigned long tmp_bgpio_dir = bank->gc.bgpio_dir;
       int val;

       /*
        * sets bgpio_dir parameter value to the opposite value
        * for calling the right registers in bgpio_get_set_multiple
        * function
        */
       bank->gc.bgpio_dir = ~bank->gc.bgpio_dir;
       val = bank->get_multiple(chip, mask, bits);
       bank->gc.bgpio_dir = tmp_bgpio_dir;

       return val;
}

 

......

       pctrl->gpio_bank[id].get = pctrl->gpio_bank[id].gc.get;
       pctrl->gpio_bank[id].gc.get = npcmgpio_get_value;
       pctrl->gpio_bank[id].get_multiple = ptrl->gpio_bank[id].gc.get_multiple;
       pctrl->gpio_bank[id].gc.get_multiple = npcmgpio_get_multiple_value;

but it is not that good solution, because the bold commands are not atomic (locked) operations.

 

Do you mean that because you write the inverse value to
IEM this happens, and the BGPIO code assumes that
you always write 1 to set a line as input and 0 to set it
as output?

yes, because of it the bgpio_get_set and bgpio_get_set_multiple setting the opposite data registers .

  

I would say if this causes the problem we should just add
a new BGPIOF_INVERTED_REG_DIR with comment in
include/linux/gpio/driver.h and make the necessary fix to
respect this flag in the gpio-mmio.c core so it works right.

If you do this as a separate patch I would be grateful :)

Sure, I will send a separate patch later on to overcome it.

Yours,
Linus Walleij

Thanks,

Tomer