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

[Roberta Dobrescu]

On 20.02.2015 14:32, Daniel Baluta wrote:
>
>
> On 02/19/2015 04:44 PM, Roberta Dobrescu wrote:
>> This patch refactors the isl29018 driver code in order to use standard
>> sysfs attributes for range and scale.
>>
>> ISL29018 light sensor uses four ranges and four ADC's resolutions
>> which influence the calculated lux.
>>
>> This patch eliminates the resolution (adc bits) and introduces scale.
>> Each range (1k, 4k, 16k or 64k) has a corresponding set of 4 scales
>> (for 16, 12, 8 or 4 bits). Both range and scale can be changed by the
>> user according to the corresponding set of values (exposed by the
>> attributes
>> in_illuminance_range_available and in_illuminance_scale_available).
>>
>> When the range is changed, the set of available scales is set
>> accordingly and
>> by default the scale used is the one for 16 adc bits. Scale can be
>> changed
>> anytime with one of the available values.
>>
>
> We should mention here that this patch renames lux_scale to calibscale
> and lux_uscale to ucalibscale.
>
> Perhaps, we could do the renaming in a separate patch preceding this one.

Maybe it's more clear like that. I'll do another patch for the renaming.

>>
>> -static int isl29018_set_range(struct isl29018_chip *chip, unsigned
>> long range,
>> -        unsigned int *new_range)
>> +/* when range is set, resolution is set by default at 16 bits */
>> +static int isl29018_set_range(struct isl29018_chip *chip, unsigned
>> long range)
>>   {
>> -    static const unsigned long supp_ranges[] = {1000, 4000, 16000,
>> 64000};
>> -    int i;
>> -
>> -    for (i = 0; i < ARRAY_SIZE(supp_ranges); ++i) {
>> -        if (range <= supp_ranges[i]) {
>> -            *new_range = (unsigned int)supp_ranges[i];
>> +    int i, ret;
>> +    unsigned long new_range;
>> +    struct isl29018_scale new_scale;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(isl29018_ranges); ++i) {
>> +        if (range == isl29018_ranges[i]) {
>> +            new_range = i;
>> +            new_scale = isl29018_scales[i][0];
>>               break;
>>           }
>>       }
>>
>> -    if (i >= ARRAY_SIZE(supp_ranges))
>> +    if (i >= ARRAY_SIZE(isl29018_ranges))
>>           return -EINVAL;
>>
>> -    return regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
>> -            COMMANDII_RANGE_MASK, i << COMMANDII_RANGE_SHIFT);
>> +    ret = regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
>> +                             COMMANDII_RESOLUTION_MASK |
>> COMMANDII_RANGE_MASK,
>> +                             i << COMMANDII_RANGE_SHIFT);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    chip->range = new_range;
>> +    chip->scale = new_scale;
>> +
>> +    return 0;
>>   }
>>
>> -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;
>> +    int i, ret;
>> +    struct isl29018_scale new_scale;
>>
>> -    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) {
>> +            new_scale = isl29018_scales[chip->range][i];
>>               break;
>>           }
>>       }
>>
>> -    if (i >= ARRAY_SIZE(supp_adcbit))
>> +    if (i >= ARRAY_SIZE(isl29018_scales[chip->range]))
>>           return -EINVAL;
>>
>> -    return regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
>> -            COMMANDII_RESOLUTION_MASK,
>> -            i << COMMANDII_RESOLUTION_SHIFT);
>> +    ret = regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
>> +                             COMMANDII_RESOLUTION_MASK,
>> +                             i << COMMANDII_RESOLUTION_SHIFT);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    chip->scale = new_scale;
>> +
>> +    return 0;
>>   }
>>
>
>>
>> @@ -321,10 +386,23 @@ static int isl29018_read_raw(struct iio_dev
>> *indio_dev,
>>           if (!ret)
>>               ret = IIO_VAL_INT;
>>           break;
>> +    case IIO_CHAN_INFO_RANGE:
>> +        if (chan->type == IIO_LIGHT) {
>> +            *val = 1000 << (chip->range * 2);
> I don't understand this shouldn't this be:
> *val = isl29018_ranges [chip->range] ?

You're right. It forgot to change it.

>> +            ret = IIO_VAL_INT;
>> +        }
>> +        break;
>
> <snip>
>
> 22,8 +509,6 @@ enum {
>>   static int isl29018_chip_init(struct isl29018_chip *chip)
>>   {
>>       int status;
>> -    unsigned int new_adc_bit;
>> -    unsigned int new_range;
>>
>>       if (chip->type == isl29035) {
>>           status = isl29035_detect(chip);
>> @@ -472,15 +557,12 @@ static int isl29018_chip_init(struct
>> isl29018_chip *chip)
>>       usleep_range(1000, 2000);    /* per data sheet, page 10 */
>>
>>       /* set defaults */
>> -    status = isl29018_set_range(chip, chip->range, &new_range);
>> +    status = isl29018_set_range(chip, 1000 << (chip->range * 2));
>
> The same here.
>
>
> thanks,
> Daniel.