lkml.org 
[lkml]   [2022]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v5 2/2] iio: light: Add support for ltrf216a sensor
On Wed, Jun 8, 2022 at 1:37 PM Shreeya Patel
<shreeya.patel@collabora.com> wrote:
>
> From: Zhigang Shi <Zhigang.Shi@liteon.com>
>
> 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

\
 
 \ /
  Last update: 2022-06-08 18:22    [W:0.082 / U:1.328 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site