Re: [PATCH v7 2/4] pwm: Add support for RZ/V2M PWM driver

[Uwe Kleine-König]

Hello Fabrizio,

your MUA introduces strange line breaks. You could do every reader of
you mails a favour and fix that. I fixed it up for my reply.

On Thu, Feb 29, 2024 at 10:45:01PM +0000, Fabrizio Castro wrote:
> > From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > Sent: Thursday, February 29, 2024 4:42 PM
> > To: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> > Subject: Re: [PATCH v7 2/4] pwm: Add support for RZ/V2M PWM driver
> > 
> > On Mon, Feb 12, 2024 at 09:06:50PM +0000, Fabrizio Castro wrote:
> > > +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_roundup(u64 a, u64 b,
> > u64 c)
> > > +{
> > > +	u64 ab = a * b;
> > 
> > a * b might overflow?!
> 
> In the context of this driver, this cannot overflow.
> The 2 formulas the above is needed for are:
> 1) period = (cyc + 1)*(NSEC_PER_SEC * frequency_divisor)/rate
> 2) duty_cycle = (cyc + 1 - low)*(NSEC_PER_SEC * frequency_divisor)/rate
> 
> With respect to 1), the dividend overflows when period * rate also
> overflows (its product is calculated in rzv2m_pwm_config).
> However, limiting the period to a maximum value of U64_MAX / rate
> prevents the calculations from overflowing (in both directions, from period to cyc, and from cyc to period). v6 introduced max_period for this.
> The situation for 2) is very similar to 1), with duty_cycle<=period,
> therefore limiting period to a max value (and clamping the duty cycle
> accordingly) will ensure that the calculation for duty_cycle won't
> overflow, either.

OK, so it might be right from a technical POV. From a maintainer POV
this is still bad. Authors for other drivers might copy it, or the driver
might be changed and there is no indication that the the function relies
on only be called with certain parameters.

> > > +	u64 d = div64_u64(ab, c);
> > > +	u64 e = d * c;
> > > +
> > > +	return d + ((ab - e) ? 1 : 0);
> > > +}
> > > +
> > > +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_rounddown(u64 a, u64 b,
> > u64 c)
> > > +{
> > > +	return div64_u64(a * b, c);
> > 
> > ditto. This is the same function as mul_u64_u64_div_u64() isn't it?
> 
> Since a * b cannot overflow in the case of this driver, I believe the
> above to be a better option than mul_u64_u64_div_u64.

Same technical POV vs maintainer POV as above. Plus: Even if
mul_u64_u64_div_u64 is a tad slower, reusing it has some benefits
nevertheless.
 
> > > [...]
> > > +	cyc = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> > > +	state->period = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1,
> > > +				NSEC_PER_SEC * frequency_divisor,
> > > +				rzv2m_pwm->rate);
> > > +
> > > +	low = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> > > +	state->duty_cycle = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1 - low,
> > > +				NSEC_PER_SEC * frequency_divisor,
> > > +				rzv2m_pwm->rate);
> > 
> > The register semantic makes me wonder if each period starts with the low
> > part. In that case the hardware called "normal" what is called inverted
> > in the pwm framework?!
> 
> My understanding is that the PWM framework defines "normal" polarity a
> signal that starts high (and stays high) for the duration of the duty cycle,
> and goes low for the remainder of the period. Conversely, a signal with
> "inversed" polarity starts low (and stays low) for the duration of the duty
> cycle and goes high for the remainder of the period.

Ack.

> This IP _does_ start low, but it _doesn't_ stay low for the duration of the
> duty cycle, as it then goes high for the duration of the duty cycle,
> therefore this IP doesn't perfectly fit either ("normal" or "inverted")
> definitions.
> I think you can say that the "normal" signal is _shifted_ in phase for this
> IP, rather than being "inverted".

Alternatively (and a better match): What you describe is an inverted
wave form with duty_cycle = period - duty_cycle.

> > > +	return pm_runtime_put(chip->dev);
> > 
> > If you evaluate the return value of pm_runtime_put() maybe check
> > pm_runtime_get_sync() for symmetry, too?
> 
> Or I could just discard it and return 0?
> I am fine with either, what's your preference?

My preference would be to always check the return value, but given that
many drivers don't care for that, I agree to accept never checking it.
So choose one option and do it consistently please.
 
> > > +	if (pwm_cyc && !FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc - 1))
> > > +		pwm_cyc = RZV2M_PWMCYC_PERIOD + 1;
> > 
> > I don't understand the relevance of FIELD_FIT(RZV2M_PWMCYC_PERIOD,
> > pwm_cyc - 1).
> 
> CYC is only made of 24 bits, therefore this is to make sure we don't
> go beyond a 24-bit representation.

I would have understood:

	if (FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc + 1))
		pwm_cyc = RZV2M_PWMCYC_PERIOD + 1;

Notice there are three changes compared to your variant:
 - drop pwm_cyc != 0 check
 - drop ! from FIELD_FIT
 - pwm_cyc + 1 instead of pwm_cyc - 1

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature