RE: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hi Thierry, Uwe,

> From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> 
> On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> > This PWM Timer cannot output low because setting 0x000 is prohibited
> > on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > the prohibited, this patch adds a workaround function to change
> > the value from 0 to 1 as pseudo low level.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pwm/pwm-rcar.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> > index e479b6a..888cb37 100644
> > --- a/drivers/pwm/pwm-rcar.c
> > +++ b/drivers/pwm/pwm-rcar.c
> > @@ -166,6 +166,20 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
> >  	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
> >  }
> >
> > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > +{
> > +	/*
> > +	 * This PWM Timer cannot output low because setting 0x000 is
> > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > +	 * the prohibited, this function changes the value from 0 to 1 as
> > +	 * pseudo low level.
> > +	 *
> > +	 * TODO: Add GPIO handling to output low level.
> > +	 */
> > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > +		rp->pwmcnt |= 1;
> 
> In my eyes this is too broken to do. Not sure I have the complete
> picture, but given a small period (say 2) this 1 cycle might result in
> 50 % duty cycle. Depending on how the hardware behaves if you disable
> it, better do this instead.

My colleague suggests that this workaround code also changes the period
as maximum (1023) to avoid 50 % duty cycle for such a case.

What do you think that this idea is acceptable for upstream? Or, should
I add gpio handling that Uwe suggested?

Best regards,
Yoshihiro Shimoda

> Are you aware of the series adding such gpio support to the imx driver?
> 
> @Thierry: So there are three drivers now that could benefit for a
> generic approach.