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