On Wed, Jun 15, 2022 at 3:52 PM Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx> wrote: > > From: Zhigang Shi <Zhigang.Shi@xxxxxxxxxx> > > 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@xxxxxxxxx> 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
