Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220615135130.227236-1-shreeya.patel@collabora.com> <20220615135130.227236-3-shreeya.patel@collabora.com>
In-Reply-To: <20220615135130.227236-3-shreeya.patel@collabora.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 15 Jun 2022 18:32:33 +0200
Message-ID: <CAHp75VeU9oSJmMdyT_0BZDuG5WMiuJiCPv5So4DWG05EEDKrhw@mail.gmail.com>
Subject: Re: [PATCH v6 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>,
        kernel test robot <lkp@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jun 15, 2022 at 3:52 PM Shreeya Patel
<shreeya.patel@collabora.com> wrote:
>
> From: Zhigang Shi <Zhigang.Shi@liteon.com>
>
> Add initial support for ltrf216a ambient light sensor.

This doesn't clarify why regmap API for SMBus can't be used.

...

> Datasheet: https://bit.ly/3MRTYwY

These kinds of links tend to disappear, please use the real link.

...

> Reported-by: kernel test robot <lkp@intel.com>

No, the new feature may not be reported.

...

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/iopoll.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>

+ blank line

> +#include <linux/iio/iio.h>

+ blank line

> +#include <asm/unaligned.h>

...

> +/*
> + * Window Factor is needed when the device is under Window glass
> + * with coated tinted ink. This is to compensate the light loss

compensate for the

> + * due to the lower transmission rate of the window glass and helps
> + * in calculating lux.
> + */

...

> +static int ltrf216a_set_power_state(struct ltrf216a_data *data, bool on)
> +{
> +       struct device *dev = &data->client->dev;
> +       int ret, suspended;
> +
> +       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]);

Even if the get_sync() failed?

Also, how do you take care about reference count in the case of failed
get_sync90?

> +       } else {
> +               pm_runtime_mark_last_busy(dev);
> +               ret = pm_runtime_put_autosuspend(dev);
> +       }
> +
> +       return ret;
> +}

...

> +static int ltrf216a_read_raw(struct iio_dev *indio_dev,
> +                            struct iio_chan_spec const *chan, int *val,
> +                            int *val2, long mask)
> +{
> +       struct ltrf216a_data *data = iio_priv(indio_dev);
> +       int ret;
> +
> +       mutex_lock(&data->lock);
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_PROCESSED:
> +               ret = ltrf216a_get_lux(data);
> +               if (ret < 0)
> +                       break;
> +               *val = ret;
> +               ret = IIO_VAL_INT;
> +               break;
> +       case IIO_CHAN_INFO_INT_TIME:
> +               ret = ltrf216a_get_int_time(data, val, val2);
> +               break;
> +       default:
> +               ret = -EINVAL;
> +               break;
> +       }
> +
> +       mutex_unlock(&data->lock);
> +
> +       return ret;
> +}

You can refactor this function to call mutex lock/unlock as many times
as cases you have and return directly.

...

> +       /* reset sensor, chip fails to respond to this, so ignore any errors */
> +       ltrf216a_reset(indio_dev);
> +
> +       ret = pm_runtime_set_active(&client->dev);
> +       if (ret)
> +               goto error_power_down;

Why do you need to power down here?

> +       pm_runtime_enable(&client->dev);
> +       pm_runtime_set_autosuspend_delay(&client->dev, 5000);
> +       pm_runtime_use_autosuspend(&client->dev);
> +
> +       ltrf216a_set_power_state(data, true);

The below code suggests that you are mixing badly devm_ with non-devm_
APIs, don't do this. You have to group devm_ first followed by
non-devm_ calls.

...

> +static int ltrf216a_remove(struct i2c_client *client)
> +{
> +       struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +       iio_device_unregister(indio_dev);
> +       pm_runtime_disable(&client->dev);
> +       pm_runtime_set_suspended(&client->dev);
> +       ltrf216a_disable(indio_dev);
> +
> +       return 0;

I believe the ordering of freeing resources and reverting state is not
in reverse. See above why.

> +}

...

> +static DEFINE_SIMPLE_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_runtime_suspend,
> +                               ltrf216a_runtime_resume);

Are you sure you are using proper macro? SIMPLE is for system sleep,
while the function names suggest that this is about runtime PM.

-- 
With Best Regards,
Andy Shevchenko