lkml.org 
[lkml]   [2022]   [Jun]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v6 2/2] iio: light: Add support for ltrf216a sensor
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

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