[PATCH 3/3] staging: iio: light: isl29018: Use standard sysfs attributes for range and scale

[Roberta Dobrescu]

On 19.02.2015 11:54, Daniel Baluta wrote:
> Some more comments.
>
> <snip>
>
>> -static int isl29018_set_range(struct isl29018_chip *chip, unsigned long range,
>> -               unsigned int *new_range)
>> +static int isl29018_set_range(struct isl29018_chip *chip, unsigned long range)
>>   {
>> -       static const unsigned long supp_ranges[] = {1000, 4000, 16000, 64000};
>> -       int i;
>> +       int i, ret;
>>
>> -       for (i = 0; i < ARRAY_SIZE(supp_ranges); ++i) {
>> -               if (range <= supp_ranges[i]) {
>> -                       *new_range = (unsigned int)supp_ranges[i];
>> +       for (i = 0; i < ARRAY_SIZE(isl29018_ranges); ++i) {
>> +               if (range == isl29018_ranges[i]) {
>> +                       chip->range = i;
>>                          break;
>>                  }
>>          }
> We need to use some local variables to keep track of range and scale.

You're right about this. I'll introduce local variables.

>>
>> -       if (i >= ARRAY_SIZE(supp_ranges))
>> +       if (i >= ARRAY_SIZE(isl29018_ranges))
>>                  return -EINVAL;
>>
>> +       chip->scale = isl29018_scales[chip->range][0];
>> +
>> +       ret = regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
>> +                       COMMANDII_RESOLUTION_MASK, 0);
>> +       if (ret < 0)
>> +               return ret;
>> +
>>          return regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
>>                          COMMANDII_RANGE_MASK, i << COMMANDII_RANGE_SHIFT);
>
> .. after regmap_update_bits returns true then we can update the chip
> state with range and scale.
>
> Otherwise, in case an error we will have an inconsistent view of that
> chips state/scale.

So local variables here too.

> Also, perhaps we could merge these 2 regmap_update_bits calls since they update
> the same register.

Will do.

>>   }
>>
>> -static int isl29018_set_resolution(struct isl29018_chip *chip,
>> -                       unsigned long adcbit, unsigned int *conf_adc_bit)
>> +static int isl29018_set_scale(struct isl29018_chip *chip, int scale, int uscale)
>>   {
>> -       static const unsigned long supp_adcbit[] = {16, 12, 8, 4};
>>          int i;
>>
>> -       for (i = 0; i < ARRAY_SIZE(supp_adcbit); ++i) {
>> -               if (adcbit >= supp_adcbit[i]) {
>> -                       *conf_adc_bit = (unsigned int)supp_adcbit[i];
>> +       for (i = 0; i < ARRAY_SIZE(isl29018_scales[chip->range]); ++i) {
>> +               if (scale == isl29018_scales[chip->range][i].scale &&
>> +                       uscale == isl29018_scales[chip->range][i].uscale) {
>> +                       chip->scale = isl29018_scales[chip->range][i];
> The same problem here. We update state of chip before knowing that
> the hardware operation succeeded.

Thanks for finding this problems. I'll solve them asap.