Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220608113553.32083-1-shreeya.patel@collabora.com> <20220608113553.32083-3-shreeya.patel@collabora.com>
In-Reply-To: <20220608113553.32083-3-shreeya.patel@collabora.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 8 Jun 2022 18:16:17 +0200
Message-ID: <CAHp75VcpHO-_Dghdc0VFjT=us-95h1b03Jmg32odJuuJZRy8aA@mail.gmail.com>
Subject: Re: [PATCH v5 2/2] iio: light: Add support for ltrf216a sensor
To:     Shreeya Patel <shreeya.patel@collabora.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>, Zhigang.Shi@liteon.com,
        krisman@collabora.com, linux-iio <linux-iio@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Collabora Kernel ML <kernel@collabora.com>,
        alvaro.soliverez@collabora.com, Dmitry Osipenko <digetx@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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