RE: [PATCH RFC 3/7] pinctrl: sh-pfc: Rollback to mux if requires when the gpio is freed

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>]

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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxx
> 
> 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