Jordan Crouse wrote: > On 27/09/08 16:04 +0200, Hans de Goede wrote: >> Hi Jordan, >> >> This time I've done a more thorough review, here is a quite long list of >> remarks I would like you to take care of in one last revision, then this can go >> upstream (through Jean or Andrew, Jean ?). > > Thanks for your time. > Your welcome, thanks for being persistent we (the lm_sensors community) really need to do a better job of timely reviewing driver submissions. >> 11) set_pwm_ctrl set_pwm_chan need better error checking. They should check the >> value they get passed is not negative, and set_pwm_chan should check for >> unsupported combos. I guess the best would be to do no checking and just >> pass the old chan and the new ctrl or vica versa to hw_set_pwm() >> and then return -EINVAL from hw_set_pwm on not valid combo's (and do >> storing of the values in the data struct also in hw_set_pwm when >> things are ok). > > I did the bounds checking in the individual functions - it just makes > things easier then trying to make hw_set_pwm do bounds checking as well > as calculating the value. But bounds only checking is not enough, as not all combo's of make these temp channels influence this pwm are supported, and since not all combo's are supported, you will need a switch case to check which are valid, and that switch case is already present in hw_set_pwm, really moving the error checking to hw_set_pwm is easy, just mke it return a status code and add default cases to catch invalid values. Regards, Hans
