[PATCH 3/3] staging: iio: light: isl29018: Use standard sysfs attributes for range and scale
Roberta Dobrescu
roberta.dobrescu at gmail.com
Thu Feb 19 12:22:41 EET 2015
On 19.02.2015 11:46, Daniel Baluta wrote:
> On Thu, Feb 19, 2015 at 2:40 AM, Roberta Dobrescu
> <roberta.dobrescu at gmail.com> 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.
>>
>> Signed-off-by: Roberta Dobrescu <roberta.dobrescu at gmail.com>
>> ---
>> After these changes, there are some small problems with the function
>> isl29018_read_lux. Since resolution is gone and scale is now used
>> there is no unshifted value and there seem to be some accuracy issues.
>> Suggestions for fixing this are welcome.
>>
>> drivers/staging/iio/light/isl29018.c | 136 ++++++++++++++++++++++++++---------
>> 1 file changed, 103 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
>> index d3d0611..cfbe7b2 100644
>> --- a/drivers/staging/iio/light/isl29018.c
>> +++ b/drivers/staging/iio/light/isl29018.c
>> @@ -66,6 +66,26 @@
>> #define ISL29035_BOUT_SHIFT 0x07
>> #define ISL29035_BOUT_MASK (0x01 << ISL29035_BOUT_SHIFT)
>>
>> +static const unsigned long isl29018_ranges[] = {1000, 4000, 16000, 64000};
>> +
>> +enum isl29018_range {
>> + ISL29018_RANGE_1,
>> + ISL29018_RANGE_2,
>> + ISL29018_RANGE_3,
>> + ISL29018_RANGE_4,
>> +};
>
> Let's introduce range indexes before the actual values. So move
> this enum above isl29018_ranges array.
Right. I'll do that.
>> +
>> +
>> +static const struct isl29018_scale {
>> + unsigned int scale;
>> + unsigned int uscale;
>> +} isl29018_scales[4][4] = {
>> + { {0, 15000}, {0, 244000}, {3, 906000}, {62, 500000} },
>> + { {0, 60000}, {0, 976000}, {15, 624000}, {250, 0} },
>> + { {0, 240000}, {3, 904000}, {62, 496000}, {1000, 0} },
>> + { {0, 960000}, {15, 616000}, {249, 984000}, {4000, 0} }
>> + };
>
> For RANGE1 and 16 bit of resolution we have:
>
>>>> 1000.0 / pow(2, 16)
> 0.0152587890625
>
> so is29018_scales[0] should be {0, 15258}.
>
> For RANGE2 and 12 bit of resolution we have:
>
>>>> 1000.0 / pow(2, 12)
> 0.244140625
>
> this means that is29108_scales[1] should be {0, 244140}.
>
> Please redo the calculation :).
I know that. I wasn't sure about these values since datasheet has some
reference about them and for 16 bits they are 0.015, 0.06, 0.24. 0.96.
But indeed, I'll redo the calculation to be more accurate.
>> +
>> struct isl29018_chip {
>> struct device *dev;
>> struct regmap *regmap;
>> @@ -74,45 +94,49 @@ struct isl29018_chip {
>> unsigned int lux_scale;
>> unsigned int lux_uscale;
>> unsigned int range;
>> - unsigned int adc_bit;
>> + struct isl29018_scale scale;
>> int prox_scheme;
>> bool suspended;
>> };
>>
>> -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;
>> }
>> }
>>
>> - 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);
>> }
>>
>> -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];
>> 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,
>> @@ -168,10 +192,11 @@ static int isl29018_read_lux(struct isl29018_chip *chip, int *lux)
>> * lux_uscale ranges from 0-999999, so about 20 bits. Split
>> * the /1,000,000 in two to reduce the risk of over/underflow.
>> */
>> - data_x_range = lux_data * chip->range;
>> + data_x_range = lux_data * chip->scale.scale;
>> + data_x_range += lux_data / 1000 * chip->scale.uscale / 1000;
>> lux_unshifted = data_x_range * chip->lux_scale;
>> lux_unshifted += data_x_range / 1000 * chip->lux_uscale / 1000;
>> - *lux = lux_unshifted >> chip->adc_bit;
>> + *lux = data_x_range;
>
> Shouldn't this be *lux = lux_unshifted?
>
> Anyhow, since chip->scale was already calculated from bit resolution
> we don't need the lux_unshifted variable.
> Use data_x_range instead.
>
> Also, would be nice to rename lux_scale to calibscale.
Sorry for that. I was testing and I forgot to change it.
Maybe if we rename lux_scale to calibscale it would had more meaning.
>>
>> return 0;
>> }
>> @@ -229,7 +254,23 @@ static int isl29018_read_proximity_ir(struct isl29018_chip *chip, int scheme,
>> return 0;
>> }
>>
>> -/* Sysfs interface */
>> +static ssize_t show_scale_available(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> + struct isl29018_chip *chip = iio_priv(indio_dev);
>> + int i, len = 0;
>> +
>> + for (i = 0; i < ARRAY_SIZE(isl29018_scales[chip->range]); ++i)
>> + len += sprintf(buf + len, "%d.%06d ",
>> + isl29018_scales[chip->range][i].scale,
>> + isl29018_scales[chip->range][i].uscale);
>> +
>> + buf[len - 1] = '\n';
>> +
>> + return len;
>> +}
>> +
>> /* proximity scheme */
>> static ssize_t show_prox_infrared_suppression(struct device *dev,
>> struct device_attribute *attr, char *buf)
>> @@ -276,11 +317,24 @@ static int isl29018_write_raw(struct iio_dev *indio_dev,
>> int ret = -EINVAL;
>>
>> mutex_lock(&chip->lock);
>> - if (mask == IIO_CHAN_INFO_CALIBSCALE && chan->type == IIO_LIGHT) {
>> - chip->lux_scale = val;
>> - /* With no write_raw_get_fmt(), val2 is a MICRO fraction. */
>> - chip->lux_uscale = val2;
>> - ret = 0;
>> + switch (mask) {
>> + case IIO_CHAN_INFO_CALIBSCALE:
>> + if (chan->type == IIO_LIGHT) {
>> + chip->lux_scale = val;
>> + chip->lux_uscale = val2;
>> + ret = 0;
>> + }
>> + break;
>> + case IIO_CHAN_INFO_RANGE:
>> + if (chan->type == IIO_LIGHT)
>> + ret = isl29018_set_range(chip, val);
>> + break;
>> + case IIO_CHAN_INFO_SCALE:
>> + if (chan->type == IIO_LIGHT)
>> + ret = isl29018_set_scale(chip, val, val2);
>> + break;
>> + default:
>> + break;
>> }
>> mutex_unlock(&chip->lock);
>>
>> @@ -321,6 +375,19 @@ 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);
>> + ret = IIO_VAL_INT;
>> + }
>> + break;
>> + case IIO_CHAN_INFO_SCALE:
>> + if (chan->type == IIO_LIGHT) {
>> + *val = chip->scale.scale;
>> + *val2 = chip->scale.uscale;
>> + ret = IIO_VAL_INT_PLUS_MICRO;
>> + }
>> + break;
>> case IIO_CHAN_INFO_CALIBSCALE:
>> if (chan->type == IIO_LIGHT) {
>> *val = chip->lux_scale;
>> @@ -340,7 +407,9 @@ static int isl29018_read_raw(struct iio_dev *indio_dev,
>> .indexed = 1, \
>> .channel = 0, \
>> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | \
>> - BIT(IIO_CHAN_INFO_CALIBSCALE), \
>> + BIT(IIO_CHAN_INFO_CALIBSCALE) | \
>> + BIT(IIO_CHAN_INFO_SCALE) | \
>> + BIT(IIO_CHAN_INFO_RANGE), \
>> }
>>
>> #define ISL29018_IR_CHANNEL { \
>> @@ -366,6 +435,9 @@ static const struct iio_chan_spec isl29023_channels[] = {
>> ISL29018_IR_CHANNEL,
>> };
>>
>> +static IIO_DEVICE_ATTR(in_illuminance_scale_available, S_IRUGO | S_IWUSR,
>> + show_scale_available, NULL, 0);
>> +static IIO_CONST_ATTR(in_illuminance_range_available, "1000 4000 16000 64000");
>> static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
>> S_IRUGO | S_IWUSR,
>> show_prox_infrared_suppression,
>> @@ -374,11 +446,15 @@ static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
>> #define ISL29018_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
>> #define ISL29018_CONST_ATTR(name) (&iio_const_attr_##name.dev_attr.attr)
>> static struct attribute *isl29018_attributes[] = {
>> + ISL29018_DEV_ATTR(in_illuminance_scale_available),
>> + ISL29018_CONST_ATTR(in_illuminance_range_available),
>> ISL29018_DEV_ATTR(proximity_on_chip_ambient_infrared_suppression),
>> NULL
>> };
>>
>> static struct attribute *isl29023_attributes[] = {
>> + ISL29018_DEV_ATTR(in_illuminance_scale_available),
>> + ISL29018_CONST_ATTR(in_illuminance_range_available),
>> NULL
>> };
>>
>> @@ -422,8 +498,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 +546,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));
>> if (status < 0) {
>> dev_err(chip->dev, "Init of isl29018 fails\n");
>> return status;
>> }
>>
> Since you remove this, add a comment on top of set_range saying that default
> resolution is by default set at 16 bits.
Ok.
>> - status = isl29018_set_resolution(chip, chip->adc_bit,
>> - &new_adc_bit);
>> -
>> return 0;
>> }
>>
>> @@ -609,8 +680,7 @@ static int isl29018_probe(struct i2c_client *client,
>> chip->type = dev_id;
>> chip->lux_scale = 1;
>> chip->lux_uscale = 0;
>> - chip->range = 1000;
>> - chip->adc_bit = 16;
>> + chip->range = ISL29018_RANGE_1;
> Should we also initialize chip->scale here?
If we set range, scale is also set with the value corresponding to 16
bits, so scale would be initialized like that.
>> chip->suspended = false;
>>
>> chip->regmap = devm_regmap_init_i2c(client,
>> --
>> 1.9.1
>>
>>
More information about the firefly
mailing list