Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 8 Jun 2022 18:16:17 +0200 | Subject | Re: [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
| |