Re: [PATCH v5 2/2] iio: light: Add support for ltrf216a sensor

[Shreeya Patel <shreeya.patel@collabora.com>]

On 08/06/22 21:46, Andy Shevchenko wrote:
> 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?

Thanks for all your comments and yes we can use the regmap I2C
but the plan is to get the basic version merged and then I'll be sending
patches for any enhancements that we'd like to do.



Thanks,
Shreeya Patel

>
>> +       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.
>