Re: [PATCH RFC 2/7] pinctrl: sh-pfc: remove incomplete flag "cfg->type"

Re: [PATCH RFC 2/7] pinctrl: sh-pfc: remove incomplete flag "cfg->type"

[Linus Walleij]

On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:

> The old commit c58d9c1b26e3 ("sh-pfc: Implement generic pinconf
> support") broke the cfg->type flag to PINMUX_TYPE_FUNCTION because
> sh_pfc_pinconf_set() didn't call sh_pfc_reconfig_pin().
> Now if we fix the cfg->type condition, it gets worse because:
>  - Some drivers might be deferred so that .set_mux() will be called
>    multiple times.
>  - In such the case, the sh-pfc driver returns -EBUSY even if
>    the group is the same, and then that driver fails to probe.
>
> Since the pinctrl subsystem already has such conditions according
> to @set_mux and @gpio_request_enable, this patch just remove
> the incomplete flag from sh-pfc/pinctrl.c.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

This looks like it should have a Fixes: tag as well.

Geert will decide what to do with this.

Can all the pinctrl patches be applied independently of the other
changes so Geert can apply and send me those patches in his pull
requests?

Yours,
Linus Walleij

RE: [PATCH RFC 2/7] pinctrl: sh-pfc: remove incomplete flag "cfg->type"

[Yoshihiro Shimoda]

Hi Linus, Geert,

> From: Linus Walleij, Sent: Monday, July 29, 2019 8:02 AM
> 
> On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
> > The old commit c58d9c1b26e3 ("sh-pfc: Implement generic pinconf
> > support") broke the cfg->type flag to PINMUX_TYPE_FUNCTION because
> > sh_pfc_pinconf_set() didn't call sh_pfc_reconfig_pin().
> > Now if we fix the cfg->type condition, it gets worse because:
> >  - Some drivers might be deferred so that .set_mux() will be called
> >    multiple times.
> >  - In such the case, the sh-pfc driver returns -EBUSY even if
> >    the group is the same, and then that driver fails to probe.
> >
> > Since the pinctrl subsystem already has such conditions according
> > to @set_mux and @gpio_request_enable, this patch just remove
> > the incomplete flag from sh-pfc/pinctrl.c.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> This looks like it should have a Fixes: tag as well.

I got it. The Fixes tag should be:

Fixes: c58d9c1b26e3 ("sh-pfc: Implement generic pinconf support")

> Geert will decide what to do with this.

I got it.

> Can all the pinctrl patches be applied independently of the other
> changes so Geert can apply and send me those patches in his pull
> requests?

The pinctrl patches (1/7 through 3/7) can be applied on next-20190726
so I think Geert can apply these patches into his repo.

Geert, if I should resend the pinctrl patches, please let me know!

Best regards,
Yoshihiro Shimoda

> Yours,
> Linus Walleij

Re: [PATCH RFC 2/7] pinctrl: sh-pfc: remove incomplete flag "cfg->type"

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Jul 29, 2019 at 7:16 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Linus Walleij, Sent: Monday, July 29, 2019 8:02 AM
> >
> > On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> >
> > > The old commit c58d9c1b26e3 ("sh-pfc: Implement generic pinconf
> > > support") broke the cfg->type flag to PINMUX_TYPE_FUNCTION because
> > > sh_pfc_pinconf_set() didn't call sh_pfc_reconfig_pin().
> > > Now if we fix the cfg->type condition, it gets worse because:
> > >  - Some drivers might be deferred so that .set_mux() will be called
> > >    multiple times.
> > >  - In such the case, the sh-pfc driver returns -EBUSY even if
> > >    the group is the same, and then that driver fails to probe.
> > >
> > > Since the pinctrl subsystem already has such conditions according
> > > to @set_mux and @gpio_request_enable, this patch just remove
> > > the incomplete flag from sh-pfc/pinctrl.c.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >
> > This looks like it should have a Fixes: tag as well.
>
> I got it. The Fixes tag should be:
>
> Fixes: c58d9c1b26e3 ("sh-pfc: Implement generic pinconf support")

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > Geert will decide what to do with this.
>
> I got it.
>
> > Can all the pinctrl patches be applied independently of the other
> > changes so Geert can apply and send me those patches in his pull
> > requests?
>
> The pinctrl patches (1/7 through 3/7) can be applied on next-20190726
> so I think Geert can apply these patches into his repo.

Looks mostly OK to me (I have some comments on 3/7).
I'll apply it to my local tree, so it will receive some testing on all
boards I have.

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

Re: [PATCH RFC 2/7] pinctrl: sh-pfc: remove incomplete flag "cfg->type"

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> The old commit c58d9c1b26e3 ("sh-pfc: Implement generic pinconf
> support") broke the cfg->type flag to PINMUX_TYPE_FUNCTION because
> sh_pfc_pinconf_set() didn't call sh_pfc_reconfig_pin().
> Now if we fix the cfg->type condition, it gets worse because:
>  - Some drivers might be deferred so that .set_mux() will be called
>    multiple times.
>  - In such the case, the sh-pfc driver returns -EBUSY even if
>    the group is the same, and then that driver fails to probe.
>
> Since the pinctrl subsystem already has such conditions according
> to @set_mux and @gpio_request_enable, this patch just remove
> the incomplete flag from sh-pfc/pinctrl.c.

Do we need to set sh_pfc_pinmux_ops.strict = true?

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

RE: [PATCH RFC 2/7] pinctrl: sh-pfc: remove incomplete flag "cfg->type"

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, August 6, 2019 6:24 PM
> 
> Hi Shimoda-san,
> 
> On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > The old commit c58d9c1b26e3 ("sh-pfc: Implement generic pinconf
> > support") broke the cfg->type flag to PINMUX_TYPE_FUNCTION because
> > sh_pfc_pinconf_set() didn't call sh_pfc_reconfig_pin().
> > Now if we fix the cfg->type condition, it gets worse because:
> >  - Some drivers might be deferred so that .set_mux() will be called
> >    multiple times.
> >  - In such the case, the sh-pfc driver returns -EBUSY even if
> >    the group is the same, and then that driver fails to probe.
> >
> > Since the pinctrl subsystem already has such conditions according
> > to @set_mux and @gpio_request_enable, this patch just remove
> > the incomplete flag from sh-pfc/pinctrl.c.
> 
> Do we need to set sh_pfc_pinmux_ops.strict = true?

If the .strict = true, the final pwm patch on this series failed with the following error:

[   11.453716] sh-pfc e6060000.pin-controller: pin GP_2_7 already requested by e6e31000.pwm; cannot claim for e6052000.gpio:459

Best regards,
Yoshihiro Shimoda

> 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

Re: [PATCH RFC 1/7] pinctrl: sh-pfc: add new flags into struct sh_pfc_pin_config

[Sergei Shtylyov]

On 08.07.2019 12:07, Yoshihiro Shimoda wrote:

> To clean/modify the code up later, this patch just adds new flags
> "mux_set" and "gpio_enabled" into the struct sh_pfc_pin_config.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>   drivers/pinctrl/sh-pfc/pinctrl.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c
> index 2824be4..157b257 100644
> --- a/drivers/pinctrl/sh-pfc/pinctrl.c
> +++ b/drivers/pinctrl/sh-pfc/pinctrl.c
> @@ -27,6 +27,8 @@
>   
>   struct sh_pfc_pin_config {
>   	u32 type;
> +	bool mux_set;
> +	bool gpio_enabled;
>   };
>   
>   struct sh_pfc_pinctrl {
> @@ -364,7 +366,15 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector,
>   	for (i = 0; i < grp->nr_pins; ++i) {
>   		ret = sh_pfc_config_mux(pfc, grp->mux[i], PINMUX_TYPE_FUNCTION);
>   		if (ret < 0)
> -			break;
> +			goto done;
> +	}
> +
> +	/* All group pins are configurated, mark the pins as mux_set */

    Configured.

> +	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];
> +
> +		cfg->mux_set = true;
>   	}
>   
>   done:
[...]

MBR, Sergei

Re: [PATCH RFC 1/7] pinctrl: sh-pfc: add new flags into struct sh_pfc_pin_config

[Geert Uytterhoeven]

On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> To clean/modify the code up later, this patch just adds new flags
> "mux_set" and "gpio_enabled" into the struct sh_pfc_pin_config.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

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

[Geert Uytterhoeven]

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.
Probably sh_pfc_pin_group.{pins,mux} should be changed from unsigned int
to u16, but that's out of scope for this patch.

>         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?

> +                */
> +               WARN_ON(cfg->gpio_enabled);

Can this actually happen?
Should this cause a failure instead?

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

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

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

[Yoshihiro Shimoda]

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

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

[Geert Uytterhoeven]

Hi Shimoda-san,

On Tue, Aug 6, 2019 at 1:38 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Tuesday, August 6, 2019 6:03 PM
> > 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.

For now, please don't worry about it. I can make that change later, as it will
affect all drivers.

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

OK.

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

IC. At first I thought they were the internal enum for the whole pin group, but
I was wrong.
They are the mux *_MARK enu, which is unique for each pin/function combo.

> 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

Thanks for checking!

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

Re: [PATCH RFC 4/7] dt-bindings: pwm: rcar: Add specific gpios property to output duty zero

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> The R-Car SoCs PWM Timer cannot output duty zero. So, this patch
> adds a specific gpio property to output it.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thanks for your patch!

> --- a/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
> +++ b/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
> @@ -26,6 +26,9 @@ Required Properties:
>  - pinctrl-0: phandle, referring to a default pin configuration node.
>  - pinctrl-names: Set to "default".
>
> +Optional properties:
> +- renesas,duty-zero-gpios: Specify GPIO for outputting duty zero.
> +
>  Example: R8A7743 (RZ/G1M) PWM Timer node
>
>         pwm0: pwm@e6e30000 {

I'm not so fond of adding a property to specify this explicitly: the PFC
driver already knows the mapping from the PWM output pin to the GPIO
number. However, I agree it is not easy to obtain this in a generic way.

For a PWM block with a single pin, it's easy: the pin you want to switch
between GPIO and pin function is the single pin in the single pin
control group specified in the board DT.

For blocks with multiple pins (e.g. SPI, UART), it is more complex, and
depends on the granularity of the pin control groups.
E.g. for UART, Renesas SoCs typically use 3 pin control groups ("data"
for RXD/TXD, "ctrl" for RTS/CTS, and "clk" for clock), and the pin
control driver (at least for sh-pfc) does not know which pin corresponds
to which GPIO inside each group.  Perhaps this information should be
added, with an API to retrieve it?

Anyone who has a good suggestion?
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

Re: [PATCH RFC 5/7] pwm: rcar: remove a redundant condition in rcar_pwm_apply()

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Since the rcar_pwm_apply() has already check whehter state->enabled
> is not set or not, this patch removes a redundant condition.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

This is completely independent from the rest of the series, and can be applied
immediately, right?

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

RE: [PATCH RFC 5/7] pwm: rcar: remove a redundant condition in rcar_pwm_apply()

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, August 6, 2019 6:06 PM
> 
> Hi Shimoda-san,
> 
> On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > Since the rcar_pwm_apply() has already check whehter state->enabled
> > is not set or not, this patch removes a redundant condition.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thank you for your review!

> This is completely independent from the rest of the series, and can be applied
> immediately, right?

That's right.

Best regards,
Yoshihiro Shimoda

> 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

Re: [PATCH RFC 5/7] pwm: rcar: remove a redundant condition in rcar_pwm_apply()

[Uwe Kleine-König]

Hello,

On Tue, Aug 06, 2019 at 11:05:30AM +0200, Geert Uytterhoeven wrote:
> On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > Since the rcar_pwm_apply() has already check whehter state->enabled
> > is not set or not, this patch removes a redundant condition.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> This is completely independent from the rest of the series, and can be applied
> immediately, right?

The original patch didn't make it into my mailbox. I only see a few
replies. Is it only me?
https://patchwork.ozlabs.org/project/linux-pwm/list/ doesn't seem to
have it either.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH RFC 5/7] pwm: rcar: remove a redundant condition in rcar_pwm_apply()

[Yoshihiro Shimoda]

Hello,

> From: Uwe Kleine-König, Sent: Wednesday, August 7, 2019 1:00 AM
> 
> Hello,
> 
> On Tue, Aug 06, 2019 at 11:05:30AM +0200, Geert Uytterhoeven wrote:
> > On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > Since the rcar_pwm_apply() has already check whehter state->enabled
> > > is not set or not, this patch removes a redundant condition.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > This is completely independent from the rest of the series, and can be applied
> > immediately, right?
> 
> The original patch didn't make it into my mailbox. I only see a few
> replies. Is it only me?
> https://patchwork.ozlabs.org/project/linux-pwm/list/ doesn't seem to
> have it either.

I don't know why but, linux-renesas-soc ML only got the patch series.
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=143129


JFYI, but I submitted another patch yesterday, and it seemed to be archived on all MLs:
https://www.spinics.net/lists/stable/msg322085.html
https://lkml.org/lkml/2019/8/6/274
https://patchwork.kernel.org/patch/11078469/

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH RFC 5/7] pwm: rcar: remove a redundant condition in rcar_pwm_apply()

[Uwe Kleine-König]

Hello,

On Mon, Jul 08, 2019 at 06:07:46PM +0900, Yoshihiro Shimoda wrote:
> Since the rcar_pwm_apply() has already check whehter state->enabled
> is not set or not, this patch removes a redundant condition.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

This patch (and also patch 6 of this series) doesn't seem to have made
it to the pwm list and pwm patchwork.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH RFC 6/7] pwm: rcar: Add gpio support to output duty zero

[Uwe Kleine-König]

Hello,

On Mon, Jul 08, 2019 at 06:07:47PM +0900, Yoshihiro Shimoda wrote:
> The R-Car SoCs PWM Timer cannot output duty zero. So, this patch
> adds gpio support to output it.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pwm/pwm-rcar.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)

I'd like to see a paragraph at the top of the driver describing the
limitations of this driver similar to what pwm-sifive.c does.

Something like:

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index 5b2b8ecc354c..b67ac84db834 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -3,6 +3,9 @@
  * R-Car PWM Timer driver
  *
  * Copyright (C) 2015 Renesas Electronics Corporation
+ *
+ * Limitations:
+ * - The hardware cannot generate a 0% duty cycle.
  */
 
 #include <linux/clk.h>

While at it: If there is a publicly available reference manual adding a line:

	Reference Manual: https://...

would be great, too.

> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index c8cd43f..1c19a8b 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/clk.h>
>  #include <linux/err.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/io.h>
>  #include <linux/log2.h>
>  #include <linux/math64.h>
> @@ -38,6 +39,7 @@ struct rcar_pwm_chip {
>  	struct pwm_chip chip;
>  	void __iomem *base;
>  	struct clk *clk;
> +	struct gpio_desc *gpio;
>  };
>  
>  static inline struct rcar_pwm_chip *to_rcar_pwm_chip(struct pwm_chip *chip)
> @@ -119,8 +121,11 @@ static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns,
>  	ph = tmp & RCAR_PWMCNT_PH0_MASK;
>  
>  	/* Avoid prohibited setting */
> -	if (cyc == 0 || ph == 0)
> +	if (cyc == 0)
>  		return -EINVAL;
> +	/* Try to use GPIO to output duty zero */
> +	if (ph == 0)
> +		return -EAGAIN;

If there is no gpio requesting cyc=0 should still yield an error.

>  	rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
>  
> @@ -157,6 +162,28 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
>  	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
>  }
>  
> +static int rcar_pwm_gpiod_get(struct rcar_pwm_chip *rp)
> +{
> +	if (rp->gpio)
> +		return 0;
> +
> +	rp->gpio = gpiod_get(rp->chip.dev, "renesas,duty-zero", GPIOD_OUT_LOW);
> +	if (!IS_ERR(rp->gpio))
> +		return 0;
> +
> +	rp->gpio = NULL;
> +	return -EINVAL;

Please use gpiod_get_optional() instead of open coding it.

Does getting the gpio automatically switch the pinmuxing?

If yes, this is IMHO a really surprising mis-feature of the gpio
subsystem. I'd prefer to "get" the gpio at probe time and only switch
the pinmuxing in .apply(). This makes .apply() quicker, ensures that all
resources necessary for pwm operation are available, handles
-EPROBE_DEFER (and maybe other errors) correctly.

Note you're introducing a bug here because switching to gpio doesn't
ensure that the currently running period is completed.

> +static void rcar_pwm_gpiod_put(struct rcar_pwm_chip *rp)
> +{
> +	if (!rp->gpio)
> +		return;
> +
> +	gpiod_put(rp->gpio);
> +	rp->gpio = NULL;
> +}
> +
>  static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			  struct pwm_state *state)
>  {
> @@ -171,6 +198,7 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	if (!state->enabled) {
>  		rcar_pwm_disable(rp);
> +		rcar_pwm_gpiod_put(rp);

From the framework's POV disabling a PWM is quite similar to duty cycle
0. Assuming disabling the PWM completes the currently running period[1]
it might be better and easier to disable instead of switching to gpio.
(Further assuming that disable really yields the inactive level which is
should and is another limitation if not.)

>  		return 0;
>  	}
>  
> @@ -187,8 +215,12 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	/* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */
>  	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
>  
> -	if (!ret)
> +	if (!ret) {
>  		ret = rcar_pwm_enable(rp);
> +		rcar_pwm_gpiod_put(rp);
> +	} else if (ret == -EAGAIN) {
> +		ret = rcar_pwm_gpiod_get(rp);
> +	}
>  
>  	return ret;
>  }

Best regards
Uwe

[1] if not, please add "Disabling doesn't complete the currently running
    period" to the list of limitations.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH RFC 6/7] pwm: rcar: Add gpio support to output duty zero

[Yoshihiro Shimoda]

Hi Uwe,

Thank you for your review!

> From: Uwe Kleine-König, Sent: Wednesday, August 7, 2019 4:03 PM
> 
> Hello,
> 
> On Mon, Jul 08, 2019 at 06:07:47PM +0900, Yoshihiro Shimoda wrote:
> > The R-Car SoCs PWM Timer cannot output duty zero. So, this patch
> > adds gpio support to output it.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pwm/pwm-rcar.c | 36 ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> I'd like to see a paragraph at the top of the driver describing the
> limitations of this driver similar to what pwm-sifive.c does.
> 
> Something like:
> 
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index 5b2b8ecc354c..b67ac84db834 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -3,6 +3,9 @@
>   * R-Car PWM Timer driver
>   *
>   * Copyright (C) 2015 Renesas Electronics Corporation
> + *
> + * Limitations:
> + * - The hardware cannot generate a 0% duty cycle.
>   */

I'll add this.

>  #include <linux/clk.h>
> 
> While at it: If there is a publicly available reference manual adding a line:
> 
> 	Reference Manual: https://...
> 
> would be great, too.

Unfortunately, the document is not public...

> > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> > index c8cd43f..1c19a8b 100644
> > --- a/drivers/pwm/pwm-rcar.c
> > +++ b/drivers/pwm/pwm-rcar.c
> > @@ -7,6 +7,7 @@
> >
> >  #include <linux/clk.h>
> >  #include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/io.h>
> >  #include <linux/log2.h>
> >  #include <linux/math64.h>
> > @@ -38,6 +39,7 @@ struct rcar_pwm_chip {
> >  	struct pwm_chip chip;
> >  	void __iomem *base;
> >  	struct clk *clk;
> > +	struct gpio_desc *gpio;
> >  };
> >
> >  static inline struct rcar_pwm_chip *to_rcar_pwm_chip(struct pwm_chip *chip)
> > @@ -119,8 +121,11 @@ static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns,
> >  	ph = tmp & RCAR_PWMCNT_PH0_MASK;
> >
> >  	/* Avoid prohibited setting */
> > -	if (cyc == 0 || ph == 0)
> > +	if (cyc == 0)
> >  		return -EINVAL;
> > +	/* Try to use GPIO to output duty zero */
> > +	if (ph == 0)
> > +		return -EAGAIN;
> 
> If there is no gpio requesting cyc=0 should still yield an error.

I'm sorry, I cannot understand this.

> >  	rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
> >
> > @@ -157,6 +162,28 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
> >  	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
> >  }
> >
> > +static int rcar_pwm_gpiod_get(struct rcar_pwm_chip *rp)
> > +{
> > +	if (rp->gpio)
> > +		return 0;
> > +
> > +	rp->gpio = gpiod_get(rp->chip.dev, "renesas,duty-zero", GPIOD_OUT_LOW);
> > +	if (!IS_ERR(rp->gpio))
> > +		return 0;
> > +
> > +	rp->gpio = NULL;
> > +	return -EINVAL;
> 
> Please use gpiod_get_optional() instead of open coding it.

I got it.

> Does getting the gpio automatically switch the pinmuxing?
> 
> If yes, this is IMHO a really surprising mis-feature of the gpio
> subsystem. I'd prefer to "get" the gpio at probe time and only switch
> the pinmuxing in .apply(). This makes .apply() quicker, ensures that all
> resources necessary for pwm operation are available, handles
> -EPROBE_DEFER (and maybe other errors) correctly.

The current pinctrl subsystem only has .set_mux(). I checked the pinctrl subsystem
history and the commit 2243a87d90b42eb38bc281957df3e57c712b5e56 removed the ".disable()" ops.
So, IIUC, we cannot such a handling.

> Note you're introducing a bug here because switching to gpio doesn't
> ensure that the currently running period is completed.

Umm, the hardware doesn't have such a condition so that the driver cannot manage it.
So, I'll add this into the "Limitations" too.

> > +static void rcar_pwm_gpiod_put(struct rcar_pwm_chip *rp)
> > +{
> > +	if (!rp->gpio)
> > +		return;
> > +
> > +	gpiod_put(rp->gpio);
> > +	rp->gpio = NULL;
> > +}
> > +
> >  static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  			  struct pwm_state *state)
> >  {
> > @@ -171,6 +198,7 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >
> >  	if (!state->enabled) {
> >  		rcar_pwm_disable(rp);
> > +		rcar_pwm_gpiod_put(rp);
> 
> From the framework's POV disabling a PWM is quite similar to duty cycle
> 0. Assuming disabling the PWM completes the currently running period[1]
> it might be better and easier to disable instead of switching to gpio.
> (Further assuming that disable really yields the inactive level which is
> should and is another limitation if not.)

If we disable the hardware, the duty cycle is 100% unfortunately. So,
I think I should describe it as one of "Limitations".

> >  		return 0;
> >  	}
> >
> > @@ -187,8 +215,12 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	/* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */
> >  	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
> >
> > -	if (!ret)
> > +	if (!ret) {
> >  		ret = rcar_pwm_enable(rp);
> > +		rcar_pwm_gpiod_put(rp);
> > +	} else if (ret == -EAGAIN) {
> > +		ret = rcar_pwm_gpiod_get(rp);
> > +	}
> >
> >  	return ret;
> >  }
> 
> Best regards
> Uwe
> 
> [1] if not, please add "Disabling doesn't complete the currently running
>     period" to the list of limitations.

Yeah, the hardware will complete the currently running period, but as I said,
the hardware doesn't have such a condition, so that the driver's .apply()
returns immediately without the completion. So, I'll add it as a Limitation.

Best regards,
Yoshihiro Shimoda

> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH RFC 6/7] pwm: rcar: Add gpio support to output duty zero

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, Aug 8, 2019 at 5:53 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Uwe Kleine-König, Sent: Wednesday, August 7, 2019 4:03 PM
> > While at it: If there is a publicly available reference manual adding a line:
> >
> >       Reference Manual: https://...
> >
> > would be great, too.
>
> Unfortunately, the document is not public...

RZ/G1 has the same hardware block, right?
Its Hardware User's Manual is publicly available, e.g. for RZ/G1M:
https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rzg/rzg1m.html#documents

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

RE: [PATCH RFC 6/7] pwm: rcar: Add gpio support to output duty zero

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Thursday, August 8, 2019 3:49 PM
> 
> Hi Shimoda-san,
> 
> On Thu, Aug 8, 2019 at 5:53 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Uwe Kleine-König, Sent: Wednesday, August 7, 2019 4:03 PM
> > > While at it: If there is a publicly available reference manual adding a line:
> > >
> > >       Reference Manual: https://...
> > >
> > > would be great, too.
> >
> > Unfortunately, the document is not public...
> 
> RZ/G1 has the same hardware block, right?
> Its Hardware User's Manual is publicly available, e.g. for RZ/G1M:
> https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rzg/rzg1m.html#documents

You're right.
# Since I could get RZ/G2 series hardware user's manual from public website yesterday,
# I was thinking any manuals are not public...

Best regards,
Yoshihiro Shimoda


> 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

Re: [PATCH RFC 6/7] pwm: rcar: Add gpio support to output duty zero

[Uwe Kleine-König]

Hello,

On Thu, Aug 08, 2019 at 03:52:52AM +0000, Yoshihiro Shimoda wrote:
> > From: Uwe Kleine-König, Sent: Wednesday, August 7, 2019 4:03 PM
> > On Mon, Jul 08, 2019 at 06:07:47PM +0900, Yoshihiro Shimoda wrote:
> > > @@ -119,8 +121,11 @@ static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns,
> > >  	ph = tmp & RCAR_PWMCNT_PH0_MASK;
> > >
> > >  	/* Avoid prohibited setting */
> > > -	if (cyc == 0 || ph == 0)
> > > +	if (cyc == 0)
> > >  		return -EINVAL;
> > > +	/* Try to use GPIO to output duty zero */
> > > +	if (ph == 0)
> > > +		return -EAGAIN;
> > 
> > If there is no gpio requesting cyc=0 should still yield an error.
> 
> I'm sorry, I cannot understand this.

I meant that if getting the gpio failed but it's needed to yield the
right level this should result in an error. I thought I removed that
comment because this is taken care of further below.

> > >  	rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
> > >
> > > @@ -157,6 +162,28 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
> > >  	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
> > >  }
> > >
> > > +static int rcar_pwm_gpiod_get(struct rcar_pwm_chip *rp)
> > > +{
> > > +	if (rp->gpio)
> > > +		return 0;
> > > +
> > > +	rp->gpio = gpiod_get(rp->chip.dev, "renesas,duty-zero", GPIOD_OUT_LOW);
> > > +	if (!IS_ERR(rp->gpio))
> > > +		return 0;
> > > +
> > > +	rp->gpio = NULL;
> > > +	return -EINVAL;
> > 
> > Does getting the gpio automatically switch the pinmuxing?
> > 
> > If yes, this is IMHO a really surprising mis-feature of the gpio
> > subsystem. I'd prefer to "get" the gpio at probe time and only switch
> > the pinmuxing in .apply(). This makes .apply() quicker, ensures that all
> > resources necessary for pwm operation are available, handles
> > -EPROBE_DEFER (and maybe other errors) correctly.
> 
> The current pinctrl subsystem only has .set_mux(). I checked the pinctrl subsystem
> history and the commit 2243a87d90b42eb38bc281957df3e57c712b5e56 removed the ".disable()" ops.
> So, IIUC, we cannot such a handling.

You'd need to reactivate the pwm setting when changing from 0% to
something bigger of course.

But as I understand it "getting" the gpio already implies that it is
muxed which is bad here. So it would be great if we could opt-out.
Linus?

The pwm-imx driver has a similar problem[1] where we already considered
using a gpio. There auto-muxing would be in the way, too. (Maybe it
isn't because it isn't implmented on i.MX?)

> > Note you're introducing a bug here because switching to gpio doesn't
> > ensure that the currently running period is completed.
> 
> Umm, the hardware doesn't have such a condition so that the driver cannot manage it.
> So, I'll add this into the "Limitations" too.

yes please.

> > > +static void rcar_pwm_gpiod_put(struct rcar_pwm_chip *rp)
> > > +{
> > > +	if (!rp->gpio)
> > > +		return;
> > > +
> > > +	gpiod_put(rp->gpio);
> > > +	rp->gpio = NULL;
> > > +}
> > > +
> > >  static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  			  struct pwm_state *state)
> > >  {
> > > @@ -171,6 +198,7 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >
> > >  	if (!state->enabled) {
> > >  		rcar_pwm_disable(rp);
> > > +		rcar_pwm_gpiod_put(rp);
> > 
> > From the framework's POV disabling a PWM is quite similar to duty cycle
> > 0. Assuming disabling the PWM completes the currently running period[1]
> > it might be better and easier to disable instead of switching to gpio.
> > (Further assuming that disable really yields the inactive level which is
> > should and is another limitation if not.)
> 
> If we disable the hardware, the duty cycle is 100% unfortunately. So,
> I think I should describe it as one of "Limitations".

It seems you got the idea .. :-)

Best regards
Uwe

[1] it goes to 0 on disable even if the polarity is inverted

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |