> >> +static int sca3300_set_frequency(struct sca3300_data *data, int val)
> >> +{
> >> + const struct sca3300_chip_info *chip = data->chip;
> >> + unsigned int index;
> >> + unsigned int i;
> >> +
> >> + if (sca3300_get_op_mode(data, &index))
> >> + return -EINVAL;
> >> +
> >> + for (i = 0; i < chip->num_avail_modes; i++) {
> >> + if ((val == chip->freq_table[chip->freq_map[i]]) &&
> >
> > The conditions being checked here are far from obvious, so I think this would benefit
> > from an explanatory comment.
> >
> > Something along the lines of,
> > "Find a mode in which the requested sampling frequency is available
> > and the scaling currently set is retained".
>
>
> In addition to a comment, how about small restructure of loop and giving
> local variables that tell the purpose, something like
>
>
> ...
>
> unsigned int opmode_scale, new_scale;
>
> opmode_scale = chip->accel_scale[chip->accel_scale_map[index]];
>
> /*
> * Find a mode in which the requested sampling frequency is available
> * and the scaling currently set is retained
> */
> for (i = 0; i < chip->num_avail_modes; i++) {
> if (val == chip->freq_table[chip->freq_map[i]]) {
> new_scale = chip->accel_scale[chip->accel_scale_map[i]]);
> if (opmode_scale == new_scale)
> break;
> }
> }
>
>
> That way it's IMHO more clear what we are comparing.
LGTM.
Thanks,
Jonathan
>
> Thanks,
> Tomas
