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.
*/
#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/ |
