On Wed, 6 Dec 2023 21:49:33 +0100
Crt Mori <cmo@xxxxxxxxxxx> wrote:
> MLX90635 is an Infra Red contactless temperature sensor most suitable
> for consumer applications where measured object temperature is in range
> between -20 to 100 degrees Celsius. It has improved accuracy for
> measurements within temperature range of human body and can operate in
> ambient temperature range between -20 to 85 degrees Celsius.
>
> Driver provides simple power management possibility as it returns to
> lowest possible power mode (Step sleep mode) in which temperature
> measurements can still be performed, yet for continuous measuring it
> switches to Continuous power mode where measurements constantly change
> without triggering.
>
> Signed-off-by: Crt Mori<cmo@xxxxxxxxxxx>
Nice work
I've made a few comments inline for future possible improvements, but
given the cleanup.h stuff is fairly new I'll not block this merging based
on that.
I made some tiny tweak for readability whilst applying. Please sanity check
that. diff was all about return values that must be 0 as they come out of regmap and
we've already checked for error cases.
diff --git a/drivers/iio/temperature/mlx90635.c b/drivers/iio/temperature/mlx90635.c
index 1b3bde36f18a..1f5c962c1818 100644
--- a/drivers/iio/temperature/mlx90635.c
+++ b/drivers/iio/temperature/mlx90635.c
@@ -306,7 +306,7 @@ static int mlx90635_read_ee_ambient(struct regmap *regmap, s16 *PG, s16 *PO, s16
return ret;
*Gb = (u16)read_tmp;
- return ret;
+ return 0;
}
static int mlx90635_read_ee_object(struct regmap *regmap, u32 *Ea, u32 *Eb, u32 *Fa, s16 *Fb,
@@ -357,7 +357,7 @@ static int mlx90635_read_ee_object(struct regmap *regmap, u32 *Ea, u32 *Eb, u32
return ret;
*Fa_scale = (u16)read_tmp;
- return ret;
+ return 0;
}
static int mlx90635_calculate_dataset_ready_time(struct mlx90635_data *data, int *refresh_time)
@@ -405,7 +405,7 @@ static int mlx90635_perform_measurement_burst(struct mlx90635_data *data)
return -ETIMEDOUT;
}
- return ret;
+ return 0;
}
static int mlx90635_read_ambient_raw(struct regmap *regmap,
@@ -424,7 +424,7 @@ static int mlx90635_read_ambient_raw(struct regmap *regmap,
return ret;
*ambient_old_raw = (s16)read_tmp;
- return ret;
+ return 0;
}
static int mlx90635_read_object_raw(struct regmap *regmap, s16 *object_raw)
@@ -444,7 +444,7 @@ static int mlx90635_read_object_raw(struct regmap *regmap, s16 *object_raw)
return ret;
*object_raw = (read - (s16)read_tmp) / 2;
- return ret;
+ return 0;
}
Applied to the togreg branch of iio.git and pushed out as testing for all the normal
reasons.
> diff --git a/drivers/iio/temperature/mlx90635.c b/drivers/iio/temperature/mlx90635.c
> new file mode 100644
> index 000000000000..1b3bde36f18a
> --- /dev/null
> +++ b/drivers/iio/temperature/mlx90635.c
> +static int mlx90635_perform_measurement_burst(struct mlx90635_data *data)
> +{
> + unsigned int reg_status;
> + int refresh_time;
> + int ret;
> +
> + ret = regmap_write_bits(data->regmap, MLX90635_REG_STATUS,
> + MLX90635_STAT_END_CONV, MLX90635_STAT_END_CONV);
> + if (ret < 0)
> + return ret;
> +
> + ret = mlx90635_calculate_dataset_ready_time(data, &refresh_time);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_write_bits(data->regmap, MLX90635_REG_CTRL2,
> + FIELD_PREP(MLX90635_CTRL2_SOB_MASK, 1),
> + FIELD_PREP(MLX90635_CTRL2_SOB_MASK, 1));
> + if (ret < 0)
> + return ret;
> +
> + msleep(refresh_time); /* Wait minimum time for dataset to be ready */
> +
> + ret = regmap_read_poll_timeout(data->regmap, MLX90635_REG_STATUS, reg_status,
> + (!(reg_status & MLX90635_STAT_END_CONV)) == 0,
> + MLX90635_TIMING_POLLING, MLX90635_READ_RETRIES * 10000);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "data not ready");
> + return -ETIMEDOUT;
> + }
> +
> + return ret;
I'll tweak this one to return 0 to make it explicit that no postive values
are returned.
> +}
> +static int mlx90635_read_all_channel(struct mlx90635_data *data,
> + s16 *ambient_new_raw, s16 *ambient_old_raw,
> + s16 *object_raw)
> +{
> + int ret;
> +
> + mutex_lock(&data->lock);
> + if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) {
> + /* Trigger measurement in Sleep Step mode */
> + ret = mlx90635_perform_measurement_burst(data);
> + if (ret < 0)
> + goto read_unlock;
> + }
> +
> + ret = mlx90635_read_ambient_raw(data->regmap, ambient_new_raw,
> + ambient_old_raw);
> + if (ret < 0)
> + goto read_unlock;
> +
> + ret = mlx90635_read_object_raw(data->regmap, object_raw);
> +read_unlock:
> + mutex_unlock(&data->lock);
A good function to use guard(mutex)(&data->lock)
> + return ret;
> +}
> +
> +static int mlx90635_calc_ambient(struct mlx90635_data *data, int *val)
> +{
> + s16 ambient_new_raw, ambient_old_raw;
> + s16 PG, PO, Gb;
> + int ret;
> +
> + ret = mlx90635_read_ee_ambient(data->regmap_ee, &PG, &PO, &Gb);
> + if (ret < 0)
> + return ret;
> +
> + mutex_lock(&data->lock);
I don't mind this not changing for now, but this is exactly the
sort of place for scoped_guard()
Something like:
scoped_guard(mutex, &data->lock) {
if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) {
ret = mlx90635_perform_measurement_burst(data);
if (ret < 0)
return ret;
}
ret = mlx90635_read_ambient_raw(data->regmap, &ambient_new_raw,
&ambient_old_raw);
if (ret < 0)
return ret;
}
> + if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) {
> + ret = mlx90635_perform_measurement_burst(data);
> + if (ret < 0)
> + goto read_ambient_unlock;
> + }
> +
> + ret = mlx90635_read_ambient_raw(data->regmap, &ambient_new_raw,
> + &ambient_old_raw);
> +read_ambient_unlock:
> + mutex_unlock(&data->lock);
> + if (ret < 0)
> + return ret;
> +
> + *val = mlx90635_calc_temp_ambient(ambient_new_raw, ambient_old_raw,
> + PG, PO, Gb);
> + return ret;
> +}
