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