lkml.org 
[lkml]   [2020]   [Dec]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
    Date

    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

    \
     
     \ /
      Last update: 2020-12-04 00:45    [W:3.927 / U:0.144 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site