[PATCH 3/3] staging: iio: light: isl29018: Use standard sysfs attributes for range and scale
Daniel Baluta
daniel.baluta at intel.com
Thu Feb 19 11:46:47 EET 2015
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.
> +
> +
> +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 :).
> +
> 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.
>
> 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.
> - 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?
> chip->suspended = false;
>
> chip->regmap = devm_regmap_init_i2c(client,
> --
> 1.9.1
>
>
More information about the firefly
mailing list