Messages in this thread | | | From | Lino Sanfilippo <> | Subject | Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration | Date | Fri, 4 Dec 2020 00:42:15 +0100 |
| |
Hi Uwe,
First off, thanks for the review!
On 29.11.20 at 19:10, Uwe Kleine-König wrote:
> > Changelog between review rounds go to below the tripple-dash below. >
Right, will do so in the next patch version.
> > 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;
> 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)
Regards, Lino
| |