Re: [PATCH v5 5/7] iio: light: vcnl4000: Add period for vcnl4040/4200

[andy . shevchenko]

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