On Thu, 2 Aug 2018 22:34:14 +0300
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Thu, Aug 2, 2018 at 9:27 PM, Parthiban Nallathambi <pn@xxxxxxx> wrote:
> > Add support for VCNL4035, which is capable of Ambient light
> > sensing (ALS) and proximity function. This patch adds support
> > only for ALS function
>
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/bitops.h>
> > +#include <linux/regmap.h>
> > +#include <linux/pm_runtime.h>
>
> Sort?
>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
>
> Ditto.
>
> > + if (reg & (VCNL4035_INT_ALS_IF_H_MASK | VCNL4035_INT_ALS_IF_L_MASK))
> > + return true;
> > + else
> > + return false;
>
> return !!(reg & ...); ?
>
> > +static irqreturn_t vcnl4035_drdy_irq_thread(int irq, void *private)
> > +{
> > + struct iio_dev *indio_dev = private;
> > + struct vcnl4035_data *data = iio_priv(indio_dev);
> > +
>
> > + if (vcnl4035_is_triggered(data)) {
>
> Here is negative conditional suits.
>
> > +#ifndef CONFIG_PM
>
> Why?
>
> > + ret = vcnl4035_set_als_power_state(data, VCNL4035_MODE_ALS_ENABLE);
> > + if (ret < 0)
> > + return ret;
> > +#endif
>
> > + pr_err("regmap_write default THDH returned %d\n", ret);
>
> dev_err()
>
> > + pr_err("regmap_write default THDL returned %d\n", ret);
>
> Ditto.
>
>
> > + ret = pm_runtime_set_active(&client->dev);
> > + if (ret < 0)
> > + goto fail_poweroff;
> > +
> > + pm_runtime_enable(&client->dev);
> > + pm_runtime_set_autosuspend_delay(&client->dev, VCNL4035_SLEEP_DELAY_MS);
> > + pm_runtime_use_autosuspend(&client->dev);
>
> Usually we do this after we probed the device.
I'm not sure we should. The docs are fairly clear on this:
"In order to use autosuspend, subsystems or drivers must call
pm_runtime_use_autosuspend() (preferably before registering the device), and
thereafter they should use the various *_autosuspend() helper functions instead
of the non-autosuspend counterparts:" (Documentation/runtime_pm.txt)
It also makes more logical sense to have removed the userspace interfaces before
unwinding this as then we don't have annoying races around the inevitable
set_suspended.
>
> > + if (client->irq) {
>
> First of all, it can be negative.
> Second, care to factor out this rather long branch to a separate function?
>
> > + if (ret < 0) {
> > + dev_err(&client->dev, "request irq %d for trigger0 failed\n",
> > + client->irq);
> > + goto fail_buffer_clean;
> > + }
>
> Indentation.
>
> > + }
>
> So, do I understand correctly that IRQ is optional for this device?
>
>
>
> > +#ifdef CONFIG_PM
> > +static int vcnl4035_runtime_suspend(struct device *dev)
>
> Perhaps __maybe_unused
>
> > +static int vcnl4035_runtime_resume(struct device *dev)
>
> Ditto.
>
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > + struct vcnl4035_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + regcache_sync(data->regmap);
> > + ret = vcnl4035_set_als_power_state(data, VCNL4035_MODE_ALS_ENABLE);
> > + if (ret < 0)
> > + return ret;
>
> + blank line
>
> > + /* wait for 1 ALS integration cycle */
> > + msleep(data->als_it_val * 100);
> > +
> > + return 0;
> > +}
> > +#endif
>
> > +static const struct of_device_id vcnl4035_of_match[] = {
> > + { .compatible = "vishay,vcnl4035", },
> > + { },
>
> Better w/o comma.
>
> > +};
> > +MODULE_DEVICE_TABLE(of, vcnl4035_of_match);
>
> > +
> > +static const struct i2c_device_id vcnl4035_id[] = {
> > + { "vcnl4035", 0 },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, vcnl4035_id);
>
> Are you expecting legacy code to use this? I would rather switch to
> ->probe_new() and also remove this.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
