Tue, May 30, 2023 at 04:24:03PM +0200, Astrid Rost kirjoitti:
> Add read/write attribute for proximity and illuminance period. The
> period is set in the interrupt persistence flags(PS_PERS and ALS_PERS).
> An interrupt will not be asserted if the raw value is not over (or lower)
> than the threshold for the set continued amount of measurements.
> The time in seconds is calculated by the number of continued refreshes
> multiplied with the integration time.
> It will always pick the next lower possible value. The period changes,
> if the integration time is changed.
...
> +static ssize_t vcnl4040_read_als_period(struct vcnl4000_data *data, int *val, int *val2)
> +{
> + int ret, ret_pers, ret_it;
> + int64_t val_c;
> +
> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF);
> + if (ret < 0)
> + return ret;
> + ret_pers = FIELD_GET(VCNL4040_ALS_CONF_PERS, ret);
> +
Redundant blank line.
> + if (ret_pers >= ARRAY_SIZE(vcnl4040_als_persistence))
> + return -EINVAL;
> +
> + ret_it = FIELD_GET(VCNL4040_ALS_CONF_IT, ret);
> +
Ditto.
> + if (ret_it >= data->chip_spec->num_als_it_times)
> + return -EINVAL;
Also name them better, at least by dropping ret prefix.
> + val_c = mul_u32_u32((*data->chip_spec->als_it_times)[ret_it][1],
> + vcnl4040_als_persistence[ret_pers]);
> + *val = div_u64_rem(val_c, MICRO, val2);
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +}
...
> +static ssize_t vcnl4040_write_als_period(struct vcnl4000_data *data, int val, int val2)
> +{
> + unsigned int index;
> + int ret, ret_it;
Name variable better.
> + u16 regval;
> + int64_t val_n = mul_u32_u32(val, MICRO) + val2;
It is inconsistent to use uXX and intXX_t in the same module. Can you explain
the intention?
> +
> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF);
> + if (ret < 0)
> + return ret;
> +
> + ret_it = FIELD_GET(VCNL4040_ALS_CONF_IT, ret);
> +
Redundant blank line.
> + if (ret_it >= data->chip_spec->num_als_it_times)
> + return -EINVAL;
> +
> + for (index = 0; index < ARRAY_SIZE(vcnl4040_als_persistence) - 1; index++)
In one case you use i, here index. Please, be consistent with your code.
> + if (val_n < mul_u32_u32(vcnl4040_als_persistence[index],
> + (*data->chip_spec->als_it_times)[ret_it][1]))
> + break;
It would be better to have
for (...) {
...
}
> + mutex_lock(&data->vcnl4000_lock);
> +
> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF);
> + if (ret < 0)
> + goto out_unlock;
> +
> + regval = (ret & ~VCNL4040_ALS_CONF_PERS) |
> + FIELD_PREP(VCNL4040_ALS_CONF_PERS, index);
> + ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF,
> + regval);
> +
> +out_unlock:
> + mutex_unlock(&data->vcnl4000_lock);
> + return ret;
> +}
...
All the same to the other functions applies.
I stopped here.
--
With Best Regards,
Andy Shevchenko
