Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]

Hello,

On Fri, Dec 04, 2020 at 08:44:17AM +0000, Sean Young wrote:
> On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> > > You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> > > you are sure that this won't discard relevant bits, please explain
> > > this in a comment for the cursory reader.
> > 
> > What about an extra check then to make sure that the period has not been truncated,
> > e.g:
> > 
> > 	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);
> > 
> > 	/* dont accept a period that is too small or has been truncated */
> > 	if ((value < PERIOD_MIN) ||
> > 	    (value != DIV_ROUND_CLOSEST_ULL(state->period, scaler)))
> > 		return -EINVAL;
> 
> Rather than doing another 64 bit division which is expensive (esp on 32 bit
> kernels), you could assign to u64 and check:
> 
> 	if (value < PERIOD || value > U32_MAX)
> 		return -EINVAL;

Given that value is a u32, value > U32_MAX will never trigger.

Maybe checking period before doing the division is more sensible.

> > > Also note that round_closed is probably wrong, as .apply() is
> > > supposed to round down the period to the next achievable period. (But
> > > fixing this has to do done in a separate patch.)
> > 
> > According to commit 11fc4edc4 rounding to the closest integer has been introduced
> > to improve precision in case that the pwm controller is used by the pwm-ir-tx driver.
> > I dont know how strong the requirement is to round down the period in apply(), but I
> > can imagine that this may be a good reason to deviate from this rule.
> > (CCing Sean Young who introduced DIV_ROUND_CLOSEST)
> 
> There was a problem where the carrier is incorrect for some IR hardware
> which uses a carrier of 455kHz. With periods that small, rounding errors
> do really matter and rounding down might cause problems.
> 
> A policy of rounding down the carrier is not the right thing to do
> for pwm-ir-tx, and such a change will probably break pwm-ir-tx in some
> edge cases.

IMO it's not an option to say: pwm-driver A is used for IR, so A's
.apply uses round-nearest and pwm-driver B is used for $somethingelse,
so B's .apply uses round-down. To be a sensible API pwm_apply_state
should have a fixed behaviour. I consider round-down the sensible
choice (because it is easier to implmement the other options with this)
and for consumers like the IR stuff we need to provide some more
functions to allow it selecting a better suited state. Something like:

	pwm_round_state_nearest(pwm, { .period = 2198, .. }, &state)

which queries the hardwares capabilities and then assigns state.period =
2200 instead of 2100.

Where can I find the affected (consumer) driver?

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]