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

[Daniel Baluta]

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.

>
> -       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.

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

>  }
>
> -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.