On Wed, Jun 2, 2021 at 4:45 PM Christoph Fritz <chf.fritz@xxxxxxxxxxxxxx> wrote:
>
> This patch adds support for Intersil isl76683 light sensor.
Any Datasheet link? (If it's one there, add it here as Datasheet: tag)
...
Missed bits.h
> +#include <linux/module.h>
Missed mod_devicetable.h
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/util_macros.h>
+ blank line
Ordered alphabetically?
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
Ditto.
...
> +#define ISL76683_INTPERS_MASK 0x3
GENMASK()
...
> +#define ISL76683_LUXRANGE_MASK (0x3 << ISL76683_LUXRANGE_SHFT)
GENMASK() with direct numbers, please. No need to reuse those shifts here.
...
> +#define ISL76683_RESOLUTION_MASK (0x3 << ISL76683_RESOLUTION_SHFT)
Ditto.
...
> +enum isl76683_dmode {
> + ISL76683_DIODE_0 = 0,
0 is default by C standard.
Ditto for all enums.
> + ISL76683_DIODE_IR,
> + ISL76683_DIODE_DIFF,
> +};
...
> +static const int isl76683_lux_ranges_available[] = {
> + 1000, 4000, 16000, 64000};
Wrong indentation, i.e. '};' part should be on a separate line.
...
> +struct isl76683_chip {
> + enum isl76683_lux_range luxrange;
> + enum isl76683_dmode photodiode;
> + struct i2c_client *client;
> + struct regmap *rmp;
> + struct completion irq_complete;
> + struct iio_trigger *trig;
> + bool trig_enabled;
> + struct mutex lock;
Lock must be explained. What's for?
> + s64 time_ns;
> +};
...
> +static int isl76683_singleshot_conversion(struct isl76683_chip *chip, int *val)
> +{
> + long timeout;
> + int ret;
> + /* wait for measurement to complete */
> + timeout = wait_for_completion_interruptible_timeout(
> + &chip->irq_complete,
> + msecs_to_jiffies(5000));
Magic number. Add a self-explanatory definition with explanation of
the chosen value.
> + if (timeout == 0) {
> + dev_err(&chip->client->dev, "measurement timed out\n");
> + return -ETIMEDOUT;
> + } else if (timeout < 0) {
> + dev_err(&chip->client->dev, "wait for measurement failed\n");
> + return -EINTR;
> + }
> +
> + ret = isl76683_get_sensordata(chip, val);
> + if (ret) {
> + dev_err(&chip->client->dev, "%s: Error %d reading lux\n",
> + __func__, ret);
Drop __func__, it doesn't bring any value.
> + return ret;
> + }
> +
> + return IIO_VAL_INT;
> +}
...
> +static irqreturn_t isl76683_trigger_handler(int irq, void *p)
> +{
> +done:
out_unlock: ?
> + mutex_unlock(&chip->lock);
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
...
> + case IIO_CHAN_INFO_SCALE:
> + *val = isl76683_lux_ranges_available[chip->luxrange];
> + return IIO_VAL_INT;
> + }
> +
> + return -EINVAL;
default case?
...
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + mutex_lock(&chip->lock);
> + chip->luxrange = find_closest(val,
> + isl76683_lux_ranges_available,
> + ARRAY_SIZE(isl76683_lux_ranges_available));
> + ret = isl76683_set_config(chip);
> + mutex_unlock(&chip->lock);
> + return ret;
> + }
> +
> + return -EINVAL;
Ditto.
...
> +static IIO_CONST_ATTR(in_illuminance_scale_available,
> + ISL76683_LUXRANGE_STR);
One line?
...
> +static int isl76683_set_trigger_state(struct iio_trigger *trig, bool enable)
> +{
> + struct isl76683_chip *chip = iio_trigger_get_drvdata(trig);
> + int ret;
> + if (enable) {
> + chip->trig_enabled = true;
> + ret = isl76683_start_measurement(chip);
> + if (ret < 0)
> + return ret;
> + } else
> + chip->trig_enabled = false;
chip->trig_enabled = enable;
if (chip->trig_enabled) {
...
}
> + return 0;
> +}
...
> + chip->rmp = devm_regmap_init_i2c(client, &isl76683_regmap_config);
> + if (IS_ERR(chip->rmp)) {
> + ret = PTR_ERR(chip->rmp);
> + dev_err(dev, "%s: Error %d initializing regmap\n",
> + __func__, ret);
Drop __func__.
> + return ret;
> + }
...
> + chip->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> + indio_dev->name, indio_dev->id);
One line?
> + if (!chip->trig)
> + return -ENOMEM;
...
> + ret = devm_request_irq(dev, client->irq,
> + isl76683_interrupt_handler,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
One short for non-threaded IRQ?! Perhaps you wanted a threaded IRQ handler?
> + "isl76683_event", indio_dev);
> + if (ret) {
> + dev_err(dev, "irq request error\n");
> + return ret;
> + }
...
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret) {
> + dev_err(dev, "%s(): iio registration failed with error %d\n",
> + __func__, ret);
Drop __func__.
> + }
...
> +static int __maybe_unused isl76683_suspend(struct device *dev)
> +{
> + struct isl76683_chip *chip =
> + iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
One line.
> +}
...
> +static int __maybe_unused isl76683_resume(struct device *dev)
> +{
> + struct isl76683_chip *chip =
> + iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
Ditto.
In both cases use dev_get_drvdata() IIUC.
> +}
> +
Useless blank line.
> +static SIMPLE_DEV_PM_OPS(isl76683_pm_ops, isl76683_suspend, isl76683_resume);
--
With Best Regards,
Andy Shevchenko
