Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp3349271imm; Sun, 29 Jul 2018 16:49:01 -0700 (PDT) X-Google-Smtp-Source: AAOMgpegg/sFG72VQU1lCfME+Hn4dRQ8tAD/sidvaOZBYTnJReRNxFVYiXJOsPbYIHdANkuHZcth X-Received: by 2002:a62:6547:: with SMTP id z68-v6mr15700698pfb.20.1532908141768; Sun, 29 Jul 2018 16:49:01 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1532908141; cv=none; d=google.com; s=arc-20160816; b=XIwEAEHcqMYgMfyAlXUnbO/ZQ/ubm51bRYTVu+6zB5RB6Tx5Bix1URQnmvrwUvK0Zu /0ulFij+eWNoVXBqW9FnquZZRrAbbIk2ZwV+WBYdiMWkJN+naCNapqG7COBLKUnnp24S SvXGK6Ff0eNPF+jRE4ITfPKXaO7eGUS4By48jUtxB537LMvYRJONJbnN7BpMZ7udXXGC geV/lFW5Iv8OjV0ePBmpMHczIVr2CcAd/pBztx6d/eXSrUqL+GHXzqHueDxYpjOcZyGp EIQaobSzdd3A29Xkfoebpz0R/ntNee8dwIVql3nz3z2+AhnfhAQpIJOMxYE8qhDxwjIB v58Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:dkim-signature :arc-authentication-results; bh=P0pzJqvlyzX3AFpZIMt1qZ3iUBPozHTQyLM6EyiIawg=; b=wvcfeR67KCHbNQ1ppCTKXoUMEwRzrurhPtUTk6WhsbKHAUQ2l1ZxbBiI0xc3PCNdtA tOemh9M0EkMWyGshxn0EEdVqEfZvdbk8UGeB6/xEPrA3P7DZBylvqUZj1kDRvg6HyHPx GMOx4D4VxX8tuGTGLOWmEajVNxRqKoDAZeKEtszBy3KOhX+kDSyYNM5jrL6jnbewRcOl +WanlhYL0dq2NJZVF4euI+egtqDgGcuXBMDc4HhBtvlPAeDRjQcmzeFRyY5TeC6tTXyf 2TCEBI+huivmyrbfY4GjWoS28S0yztldIKvIrdeEXYlQ3zuftDMuCD2FzPPoffphlii6 Pk8A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=eJmPFyyD; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id n1-v6si8393854plp.166.2018.07.29.16.48.47; Sun, 29 Jul 2018 16:49:01 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=eJmPFyyD; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731333AbeG3A4R (ORCPT + 99 others); Sun, 29 Jul 2018 20:56:17 -0400 Received: from mail-it0-f68.google.com ([209.85.214.68]:54200 "EHLO mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728528AbeG3A4Q (ORCPT ); Sun, 29 Jul 2018 20:56:16 -0400 Received: by mail-it0-f68.google.com with SMTP id 72-v6so14858285itw.3; Sun, 29 Jul 2018 16:24:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=P0pzJqvlyzX3AFpZIMt1qZ3iUBPozHTQyLM6EyiIawg=; b=eJmPFyyDyEqYO5qsIrtRZlqmBeLviyaYVXDOTT//oPaWF7twHkwwyIIEKRL5ZJtohc O8mbJIRDr6DIxaAPSQwKC9s+tEx/hth1HNL6EIAFhpwvkQvrxr03/beLK11xOL01zEWB bg3Oh/eAHlsfpLFYiZT8PJJzd9+uuEEhbNgmu8kYg+GsupVnhpXJhnqYhxD8H4n5Gd3w D+Jpu4Akgawft0hX/W41S4spHr9iGChaVSYQuXyytJ/eJbyYy1sN9gWevacIL+yCkzZL VJBReZ2ijoly+ljayeckBurze5NhdX+Qwc++9dHaFEbQ0DAj+FGLm0RAYub4dynwx1DE RHsg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=P0pzJqvlyzX3AFpZIMt1qZ3iUBPozHTQyLM6EyiIawg=; b=eMSviR8mY6STZSrRWf2pFNaGVYeo1o9M7p3CByuaHroEQEf+l702JmADQdUO7F/tQz EGUOHSG2r52779H031/R8vHxdmbNA0D73CcMT/63RRdu4uXj/rrMQ8AP+IOb+CDfS4B0 IADYFNGiatrNi+E7l1VB4st5lF5m418DhCUYeMomE9Ze2YU2rT+3UJvPFJx7THymuGUz A745W/8KMgcXM8RVEJxv1jbkMWBb5i1ovjgypunKaUm7mPz7VOhh9MBgLRigMzKuMfeT QXl0SNFxAPZx/Xk0q/AZ40CFUk485QT2aqy4pqS2Kc/jXi1UL522fi2vtvKI47b3G/nm 5ugw== X-Gm-Message-State: AOUpUlFXqCgHGAKDmauLXhXHV9vANZsZY0LEqT7lFBWf+HplM62wxsig Kqt+iYMwvU8k1mzRNHNlzc1J8Es24eAsFRLFyVc= X-Received: by 2002:a02:9b2a:: with SMTP id o39-v6mr14692788jak.106.1532906641457; Sun, 29 Jul 2018 16:24:01 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a02:8506:0:0:0:0:0 with HTTP; Sun, 29 Jul 2018 16:24:00 -0700 (PDT) In-Reply-To: References: <20180712214203.114844-1-tmaimon77@gmail.com> <20180712214203.114844-3-tmaimon77@gmail.com> From: Tomer Maimon Date: Mon, 30 Jul 2018 02:24:00 +0300 Message-ID: Subject: Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver To: Linus Walleij Cc: Rob Herring , Mark Rutland , Avi Fishman , Nancy Yuen , Brendan Higgins , Patrick Venture , Joel Stanley , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "linux-kernel@vger.kernel.org" , "open list:GPIO SUBSYSTEM" , OpenBMC Maillist Content-Type: multipart/alternative; boundary="000000000000d6878605722ba3e2" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --000000000000d6878605722ba3e2 Content-Type: text/plain; charset="UTF-8" Hi Linus, On 30 July 2018 at 00:59, Linus Walleij wrote: > On Thu, Jul 26, 2018 at 2:01 AM Tomer Maimon 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_mul tiple; 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 --000000000000d6878605722ba3e2 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Linus,

On 30 July 2018 at 00:59, Linus Walleij <linus.walleij@lin= aro.org> wrote:
On Thu, Jul 26, 2018 at 2:01 AM Tomer Maimon <tmaimon77@gmail.com> wr= ote:

> I initialize bgpio as follow:
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0ret =3D bgpio_init(&pctrl->gpio_bank[id].gc= ,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 pctrl->dev, 4,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 pctrl->gpio_bank[id].base +
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 NPCM7XX_GP_N_DIN,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 pctrl->gpio_bank[id].base +
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 NPCM7XX_GP_N_DOUT,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 NULL,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 NULL,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 pctrl->gpio_bank[id].base +
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 NPCM7XX_GP_N_IEM,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 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 regi= sters
>
> 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?
=C2=A0
Of course, in the last patch sent=C2=A0three days a go (V3 - https://patch= work.ozlabs.org/patch/949942/) I did as follow to workaround the issue:=

=C2=A0 =C2=A0 =C2=A0 =C2=A0int (*get)(struct g= pio_chip *chip, unsigned offset);
=C2=A0 =C2=A0 =C2=A0 =C2=A0int (*get_multiple)(struct gpi= o_chip *chip, unsigned long *mask,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long *bits);
=

......

static int npcmgpio_get_value(struct gpio_chip *chip, uns= igned int offset)
<= span style=3D"font-size:12.8px;background-color:rgb(255,255,255);text-decor= ation-style:initial;text-decoration-color:initial;float:none;display:inline= ">{
=C2=A0 =C2= =A0 =C2=A0 =C2=A0struct npcm7xx_gpio *bank =3D gpiochip_get_data(chip);
=C2=A0 =C2=A0 =C2= =A0 =C2=A0unsigned long tmp_bgpio_dir =3D bank->gc.bgpio_dir;
=C2=A0 =C2=A0 =C2=A0 =C2= =A0int val;

=C2=A0 =C2=A0 =C2=A0 =C2=A0/*=
=C2=A0 =C2=A0 = =C2=A0 =C2=A0 * sets bgpio_dir parameter value to the opposite value=
=C2=A0 =C2=A0 =C2=A0 = =C2=A0 * for calling the right registers in bgpio_get_set
=C2=A0 =C2=A0 =C2=A0 =C2=A0 * f= unction
=C2=A0 = =C2=A0 =C2=A0 =C2=A0 */
=C2=A0 =C2=A0 =C2=A0 =C2=A0bank->gc.bgpio_dir =3D ~bank->g= c.bgpio_dir;
=C2= =A0 =C2=A0 =C2=A0 =C2=A0val =3D bank->get(chip, offset);
=C2=A0 =C2=A0 =C2=A0 =C2=A0bank= ->gc.bgpio_dir =3D tmp_bgpio_dir;

= =C2=A0 =C2=A0 =C2=A0 =C2=A0return val;
}

static int npcmg= pio_get_multiple_value(struct gpio_chip *chip,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 unsigned long *mask, unsigned long *bits)
{
=C2=A0 =C2=A0 =C2=A0 =C2=A0stru= ct npcm7xx_gpio *bank =3D gpiochip_get_data(chip);
=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long= tmp_bgpio_dir =3D bank->gc.bgpio_dir;
=C2=A0 =C2=A0 =C2=A0 =C2=A0int val;

=C2=A0 =C2=A0 =C2=A0 =C2=A0/*
=C2=A0 =C2=A0 =C2=A0 =C2=A0 * sets bgpio= _dir parameter value to the opposite value
=C2=A0 =C2=A0 =C2=A0 =C2=A0 * for calling the ri= ght registers in bgpio_get_set_multiple
=C2=A0 =C2=A0 =C2=A0 =C2=A0 * function
=C2=A0 =C2=A0 =C2=A0 =C2=A0 = */
=C2=A0 = =C2=A0 =C2=A0 =C2=A0bank->gc.bgpio_dir =3D ~bank->gc.bgpio_dir;
=C2=A0 =C2=A0 =C2=A0 = =C2=A0val =3D bank->get_multiple(chip, mask, bits);
=C2=A0 =C2=A0 =C2=A0 =C2=A0bank->= gc.bgpio_dir =3D tmp_bgpio_dir;

=C2= =A0 =C2=A0 =C2=A0 =C2=A0return val;
}
=C2=A0
......

=C2= =A0 =C2=A0 =C2=A0 =C2=A0pctrl->gpio_bank[id].get =3D pctrl->gpio_bank= [id].gc.get;
=C2= =A0 =C2=A0 =C2=A0 =C2=A0pctrl->gpio_bank[id].gc.get =3D npcmgpio_get_val= ue;
=C2=A0 =C2= =A0 =C2=A0 =C2=A0pctrl->gpio_bank[id].get_multiple =3D p<= /span>trl->gpio_bank[id].gc.get_multiple;
=C2=A0 =C2=A0 =C2=A0 =C2=A0p= ctrl->gpio_bank[id].gc.get_multiple =3D=C2=A0np= cmgpio_get_multiple_value;


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

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,=20
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 :)
<= div>
Sure, I will send a to overcome it.=

Yours,
Linus Walleij

Thanks,

Tomer
--000000000000d6878605722ba3e2--