| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 2 Sep 2022 18:27:46 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Crt Mori <cmo@...exis.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
linux-iio <linux-iio@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] iio: temperature: mlx90632 Add runtime
powermanagement modes
On Fri, Sep 2, 2022 at 4:13 PM <cmo@...exis.com> wrote:
>
> From: Crt Mori <cmo@...exis.com>
>
> Sensor can operate in lower power modes and even make measurements when
The sensor
...or..
Sensors
> in those lower powered modes. The decision was taken that if measurement
> is not requested within 2 seconds the sensor will remain in SLEEP_STEP
> power mode, where measurements are triggered on request with setting the
> start of measurement bit (SOB). In this mode the measurements are taking
> a bit longer because we need to start it and complete it. Currently, in
> continuous mode we read ready data and this mode is activated if sensor
> measurement is requested within 2 seconds. The suspend timeout is
> increased to 6 seconds (instead of 3 before), because that enables more
> measurements in lower power mode (SLEEP_STEP), with the lowest refresh
> rate (2 seconds).
...
> #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/
>
> +#define MLX90632_EE_RR(ee_val) (ee_val & GENMASK(10, 8)) /* Only Refresh Rate bits*/
Missed space. Seems like a copy'n'paste from previous comments that
lacks the space as well.
...
> + unsigned long interraction_timestamp; /* in jiffies */
_ts for timestamp is a fine abbreviation. Also move comment to the kernel doc.
...
> +static int mlx90632_wakeup(struct mlx90632_data *data);
Can we avoid forward declaration? (I don't even see how it is used
among dozens of lines of below code in the patch)
> static s32 mlx90632_pwr_set_sleep_step(struct regmap *regmap)
> {
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(regmap_get_device(regmap)));
Why ping-ponging here and not using dev_get_drvdata()? Ditto for similar cases.
> + struct mlx90632_data *data = iio_priv(indio_dev);
> + s32 ret = 0;
Assignment is not needed, use 'return 0;' directly. Ditto for all
cases like this.
> + if (data->powerstatus != MLX90632_PWR_STATUS_SLEEP_STEP) {
> + ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
> + MLX90632_CFG_PWR_MASK,
> + MLX90632_PWR_STATUS_SLEEP_STEP);
> + if (ret < 0)
> + return ret;
> +
> + data->powerstatus = MLX90632_PWR_STATUS_SLEEP_STEP;
> + }
> + return ret;
> }
...
> + reg = MLX90632_EE_RR(reg) >> 8;
This makes it harder to understand the semantics of reg, can we simply
unite this line with the below?
> + return MLX90632_MEAS_MAX_TIME >> reg;
...
> + refresh_time = refresh_time + ret;
+= ?
...
> + refresh_time = refresh_time + ret;
+=
...
> + refresh_time = refresh_time + ret;
Ditto.
...
> + unsigned int reg_status;
> int ret;
Keep the reversed xmas tree order (like here!) elsewhere for the sake
of consistency.
...
> + ret = regmap_read_poll_timeout(data->regmap, MLX90632_REG_STATUS,
> + reg_status,
> + ((reg_status & MLX90632_STAT_BUSY) == 0),
Too many parentheses
> + 10000, 100 * 10000);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "data not ready");
> + return -ETIMEDOUT;
> + }
...
> + int current_powerstatus = data->powerstatus;
Please, split the assignment and move it closer to the first user.
...
> + data->powerstatus = MLX90632_PWR_STATUS_HALT;
> +
> + if (current_powerstatus == MLX90632_PWR_STATUS_SLEEP_STEP)
> + return mlx90632_pwr_set_sleep_step(data->regmap);
> + else
Redundant.
> + return mlx90632_pwr_continuous(data->regmap);
...
> + ret = read_poll_timeout(mlx90632_perform_measurement, meas, meas == 19,
> + 50000, 800000, false, data);
> + if (ret != 0)
Drop this ' != 0' part. It's useless.
> + goto read_unlock;
> +
Seems redundant blank line.
...
> + }
> +
>
Ditto.
...
> + int ret = 0;
Redundant assignment. Use return 0; directly.
...
> + if (time_in_range(now, data->interraction_timestamp,
> + data->interraction_timestamp +
> + msecs_to_jiffies(MLX90632_MEAS_MAX_TIME + 100))) {
With a local variable you will have better to read code.
> + }
...
> struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
Maybe a separate patch to drop these here-there dereferences...
...
> +static int __maybe_unused mlx90632_pm_runtime_suspend(struct device *dev)
No __maybe_unused, use pm_ptr() / pm_sleep_ptr() below.
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct mlx90632_data *data = iio_priv(indio_dev);
> +
> + return mlx90632_pwr_set_sleep_step(data->regmap);
> +}
> +
> +const struct dev_pm_ops mlx90632_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(mlx90632_pm_suspend, mlx90632_pm_resume)
> + SET_RUNTIME_PM_OPS(mlx90632_pm_runtime_suspend,
> + NULL, NULL)
Please, use new macros from pm.h / runtime_pm.h
> +};
> +EXPORT_SYMBOL_GPL(mlx90632_pm_ops);
Can we use special EXPORT macro from pm.h
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists