From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Linus Walleij <linus.walleij@linaro.org>,
Geert Uytterhoeven <geert+renesas@glider.be>,
Thierry Reding <thierry.reding@gmail.com>,
Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
Linux PWM List <linux-pwm@vger.kernel.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>,
Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH RFC 3/7] pinctrl: sh-pfc: Rollback to mux if requires when the gpio is freed
Date: Tue, 6 Aug 2019 11:38:25 +0000 [thread overview]
Message-ID: <OSBPR01MB4536870EEEE634B06199722ED8D50@OSBPR01MB4536.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdUAVGbvn0D=UkqhY6RpO70MR-4GBC8i931a+fV9f6+njg@mail.gmail.com>
Hi Geert-san,
Thank you for your review!
> From: Geert Uytterhoeven, Sent: Tuesday, August 6, 2019 6:03 PM
>
> Hi Shimoda-san,
>
> On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > R-Car PWM controller requires the gpio to output zero duty,
> > this patch allows to roll it back from gpio to mux when the gpio
> > is freed.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>
> Thanks for your patch!
>
> > --- a/drivers/pinctrl/sh-pfc/pinctrl.c
> > +++ b/drivers/pinctrl/sh-pfc/pinctrl.c
> > @@ -26,6 +26,7 @@
> > #include "../pinconf.h"
> >
> > struct sh_pfc_pin_config {
> > + unsigned int mux_mark;
>
> Due to padding, adding this field will increase memory consumption by
> 6 bytes per pin.
I see.
> Probably sh_pfc_pin_group.{pins,mux} should be changed from unsigned int
> to u16, but that's out of scope for this patch.
I got it.
> > bool mux_set;
> > bool gpio_enabled;
> > };
> > @@ -353,6 +354,15 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector,
> > spin_lock_irqsave(&pfc->lock, flags);
> >
> > for (i = 0; i < grp->nr_pins; ++i) {
> > + int idx = sh_pfc_get_pin_index(pfc, grp->pins[i]);
> > + struct sh_pfc_pin_config *cfg = &pmx->configs[idx];
> > +
> > + /*
> > + * This doesn't assume the order which gpios are enabled
> > + * and then mux is set.
>
> I'm sorry, I don't understand what you mean?
> Can you please reword or elaborate?
I was also difficult to remember what I meant...
Anyway, this meant,
1) if a device has the default pinctrl-0 property, the set_mux() ops is called
before the device driver's probe() function is called by pinctrl_bind_pins() first,
2) so that any device drivers cannot call gpiod_get() before the 1).
However, this comments don't cover an imbalance pinctrl/gpio handling.
For example (as pseudo):
- SCIF driver uses SCIF2 pinctrl,
- but, IOMMU driver gets the SCIF2 pins before SCIF driver is probed.
So, I'd like to revise the comments as following. What do you think?
--
This driver cannot manage both gpio and mux when the gpio pin
is already enabled. So, this function failed.
--
> > + */
> > + WARN_ON(cfg->gpio_enabled);
>
> Can this actually happen?
This cannot happen actually.
> Should this cause a failure instead?
I think so.
> > +
> > ret = sh_pfc_config_mux(pfc, grp->mux[i], PINMUX_TYPE_FUNCTION);
> > if (ret < 0)
> > goto done;
> > @@ -364,6 +374,7 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector,
> > struct sh_pfc_pin_config *cfg = &pmx->configs[idx];
> >
> > cfg->mux_set = true;
> > + cfg->mux_mark = grp->mux[i];
> > }
> >
> > done:
> > @@ -417,6 +428,9 @@ static void sh_pfc_gpio_disable_free(struct pinctrl_dev *pctldev,
> >
> > spin_lock_irqsave(&pfc->lock, flags);
> > cfg->gpio_enabled = false;
> > + /* If mux is already set, this configure it here */
>
> configures
Oops! I'll fix it.
> > + if (cfg->mux_set)
> > + sh_pfc_config_mux(pfc, cfg->mux_mark, PINMUX_TYPE_FUNCTION);
>
> Have you considered the case where more than one pin of a pinmux group
> was used as a GPIO? In that case sh_pfc_gpio_disable_free() will be called
> multiple times, possibly with the same mux_mark.
I haven't considered the case. But, about the mux_mark, I checked the values and then
they are not the same.
For example (debug printk patch):
diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c
index bc29066..fdac71b 100644
--- a/drivers/pinctrl/sh-pfc/pinctrl.c
+++ b/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -349,7 +349,7 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector,
unsigned int i;
int ret = 0;
- dev_dbg(pctldev->dev, "Configuring pin group %s\n", grp->name);
+ dev_info(pctldev->dev, "Configuring pin group %s\n", grp->name);
spin_lock_irqsave(&pfc->lock, flags);
@@ -375,6 +375,7 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector,
cfg->mux_set = true;
cfg->mux_mark = grp->mux[i];
+ dev_info(pctldev->dev, "%d: %x\n", i, cfg->mux_mark);
}
done:
--
2.7.4
For example (log):
[ 0.497647] sh-pfc e6060000.pin-controller: Configuring pin group scif2_data_a
[ 0.497711] sh-pfc e6060000.pin-controller: 0: 77b
[ 0.497715] sh-pfc e6060000.pin-controller: 1: 760
Best regards,
Yoshihiro Shimoda
> I don't think this will cause issues, though.
>
> > spin_unlock_irqrestore(&pfc->lock, flags);
> > }
>
> Thanks!
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
next prev parent reply other threads:[~2019-08-06 11:38 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-08 9:07 [PATCH RFC 0/7] treewide: modify sh-pfc and add support pwm duty zero Yoshihiro Shimoda
2019-07-08 9:07 ` [PATCH RFC 1/7] pinctrl: sh-pfc: add new flags into struct sh_pfc_pin_config Yoshihiro Shimoda
2019-07-08 15:51 ` Sergei Shtylyov
2019-08-06 8:46 ` Geert Uytterhoeven
2019-07-08 9:07 ` [PATCH RFC 2/7] pinctrl: sh-pfc: remove incomplete flag "cfg->type" Yoshihiro Shimoda
2019-07-28 23:02 ` Linus Walleij
2019-07-29 5:16 ` Yoshihiro Shimoda
2019-08-06 8:49 ` Geert Uytterhoeven
2019-08-06 9:23 ` Geert Uytterhoeven
2019-08-06 11:48 ` Yoshihiro Shimoda
2019-07-08 9:07 ` [PATCH RFC 3/7] pinctrl: sh-pfc: Rollback to mux if requires when the gpio is freed Yoshihiro Shimoda
2019-08-06 9:02 ` Geert Uytterhoeven
2019-08-06 11:38 ` Yoshihiro Shimoda [this message]
2019-08-06 12:01 ` Geert Uytterhoeven
2019-07-08 9:07 ` [PATCH RFC 4/7] dt-bindings: pwm: rcar: Add specific gpios property to output duty zero Yoshihiro Shimoda
2019-08-06 9:21 ` Geert Uytterhoeven
2019-07-08 9:07 ` [PATCH RFC 5/7] pwm: rcar: remove a redundant condition in rcar_pwm_apply() Yoshihiro Shimoda
2019-08-06 9:05 ` Geert Uytterhoeven
2019-08-06 11:39 ` Yoshihiro Shimoda
2019-08-06 16:00 ` Uwe Kleine-König
2019-08-07 2:56 ` Yoshihiro Shimoda
2019-08-07 6:33 ` Uwe Kleine-König
2019-07-08 9:07 ` [PATCH RFC 6/7] pwm: rcar: Add gpio support to output duty zero Yoshihiro Shimoda
2019-08-07 7:03 ` Uwe Kleine-König
2019-08-08 3:52 ` Yoshihiro Shimoda
2019-08-08 6:49 ` Geert Uytterhoeven
2019-08-08 7:02 ` Yoshihiro Shimoda
2019-08-08 7:31 ` Uwe Kleine-König
2019-07-08 9:07 ` [PATCH RFC 7/7] arm64: dts: renesas: salvator-common: add gpio property into pwm1 Yoshihiro Shimoda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=OSBPR01MB4536870EEEE634B06199722ED8D50@OSBPR01MB4536.jpnprd01.prod.outlook.com \
--to=yoshihiro.shimoda.uh@renesas.com \
--cc=devicetree@vger.kernel.org \
--cc=geert+renesas@glider.be \
--cc=geert@linux-m68k.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=robh+dt@kernel.org \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).