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

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

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