On Wed, Jun 8, 2022 at 1:37 PM Shreeya Patel
<shreeya.patel@xxxxxxxxxxxxx> wrote:
>
> From: Zhigang Shi <Zhigang.Shi@xxxxxxxxxx>
>
> Add initial support for ltrf216a ambient light sensor.
>
> Datasheet: gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTRF216A.pdf
https?
...
> +#define LTRF216A_ALS_READ_DATA_DELAY 20000
What units?
...
> +/* Window Factor is needed when device is under Window glass
the device
> + * with coated tinted ink. This is to compensate the light loss
for the?
> + * due to the lower transmission rate of the window glass.
> + */
/*
* Multi-line comments should look
* like this very example. Find the difference.
*/
...
> +static int ltrf216a_init(struct iio_dev *indio_dev)
> +{
> + struct ltrf216a_data *data = iio_priv(indio_dev);
> + int ret = 0;
Useless assignment.
> +
> + /* enable sensor */
> + ret |= FIELD_PREP(LTRF216A_ALS_ENABLE_MASK, 1);
This is bad code. Use another variable with distinguashable name.
> + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, ret);
Can this driver utilize regmap I2C?
> + if (ret < 0)
> + dev_err(&data->client->dev,
> + "Error writing to LTRF216A_MAIN_CTRL while enabling the sensor: %d\n", ret);
> +
> + return ret;
> +}
...
> +static int ltrf216a_disable(struct iio_dev *indio_dev)
> +{
> + struct ltrf216a_data *data = iio_priv(indio_dev);
> + int ret = 0;
Useless assignment.
> + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, 0);
> + if (ret < 0)
> + dev_err(&data->client->dev,
> + "Error writing to LTRF216A_MAIN_CTRL while disabling the sensor: %d\n",
> + ret);
With a temporary variable for the device this may be located on one line.
Same for the similar cases.
> + return ret;
> +}
...
> +#ifdef CONFIG_PM
Why? Can't it be hidden by using pm_sleep_ptr() or alike?
> +static int ltrf216a_set_power_state(struct ltrf216a_data *data, bool on)
> +{
> + struct device *dev = &data->client->dev;
> + int ret = 0, suspended;
Useless assignment. Please, go thru all your code and drop these
potentially dangerous assignments.
> +
> + if (on) {
> + suspended = pm_runtime_suspended(dev);
> + ret = pm_runtime_get_sync(dev);
> +
> + /* Allow one integration cycle before allowing a reading */
> + if (suspended)
> + msleep(ltrf216a_int_time_reg[0][0]);
> + } else {
> + pm_runtime_mark_last_busy(dev);
> + ret = pm_runtime_put_autosuspend(dev);
> + }
> +
> + return ret;
> +}
> +#else
> +static int ltrf216a_set_power_state(struct ltrf216a_data *data, bool on)
> +{
> + return 0;
> +}
> +#endif
> +
> +int ltrf216a_check_for_data(struct i2c_client *client)
> +{
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(client, LTRF216A_MAIN_STATUS);
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to read LTRF216A_MAIN_STATUS register: %d\n", ret);
> + return ret;
Dup.
> + }
> +
> + return ret;
> +}
...
> +#ifdef CONFIG_PM_SLEEP
Oh, please no.
> +#endif
...
> +static const struct dev_pm_ops ltrf216a_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
> + SET_RUNTIME_PM_OPS(ltrf216a_runtime_suspend,
> + ltrf216a_runtime_resume, NULL)
> +};
Use pm_sleep_ptr() and corresponding top-level macros.
--
With Best Regards,
Andy Shevchenko
