| Message ID | 20241005165119.3549472-3-perdaniel.olsson@axis.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support for Texas Instruments OPT4060 RGBW Color sensor. | expand |
On Sat, 5 Oct 2024 18:51:19 +0200 Per-Daniel Olsson <perdaniel.olsson@axis.com> wrote: > Add support for Texas Instruments OPT4060 RGBW Color sensor. Hi Per-Daniel. Thanks for your driver. Various comments inline. The two bigger things I'd like to understand are: 1) Why we have a threshold trigger? That is unusual so I'd like to fully understand the use case. I see an explanation in your cover letter so I'll comment on that here but I would like one here as well. 2) Why do we need processed and raw? That may make sense but it is also unusual so I'd like a comment in this patch description on your reasoning. I've only raised some style issues in a few places. Please check to see if those apply more broadly. Thanks, Jonathan > > Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.com> > --- > drivers/iio/light/Kconfig | 13 + > drivers/iio/light/Makefile | 1 + > drivers/iio/light/opt4060.c | 1216 +++++++++++++++++++++++++++++++++++ > 3 files changed, 1230 insertions(+) > create mode 100644 drivers/iio/light/opt4060.c > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > index 515ff46b5b82..0e2059512157 100644 > --- a/drivers/iio/light/Kconfig > +++ b/drivers/iio/light/Kconfig > @@ -490,6 +490,19 @@ config OPT4001 > If built as a dynamically linked module, it will be called > opt4001. > > +config OPT4060 > + tristate "Texas Instruments OPT4060 RGBW Color Sensor" > + depends on I2C > + select REGMAP_I2C > + select IIO_BUFFER > + select IIO_TRIGGERED_BUFFER > + help > + If you say Y or M here, you get support for Texas Instruments > + OPT4060 RGBW Color Sensor. > + > + If built as a dynamically linked module, it will be called > + opt4060. > + > config PA12203001 > tristate "TXC PA12203001 light and proximity sensor" > depends on I2C > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile > index 321010fc0b93..55902b21ec0c 100644 > --- a/drivers/iio/light/Makefile > +++ b/drivers/iio/light/Makefile > @@ -42,6 +42,7 @@ obj-$(CONFIG_MAX44009) += max44009.o > obj-$(CONFIG_NOA1305) += noa1305.o > obj-$(CONFIG_OPT3001) += opt3001.o > obj-$(CONFIG_OPT4001) += opt4001.o > +obj-$(CONFIG_OPT4060) += opt4060.o > obj-$(CONFIG_PA12203001) += pa12203001.o > obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o > obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o > diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c > new file mode 100644 > index 000000000000..a917b59cd500 > --- /dev/null > +++ b/drivers/iio/light/opt4060.c > @@ -0,0 +1,1216 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2024 Axis Communications AB > + * > + * Datasheet: https://www.ti.com/lit/gpn/opt4060 > + * > + * Device driver for the Texas Instruments OPT4060 RGBW Color Sensor. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/i2c.h> > +#include <linux/iio/iio.h> > +#include <linux/math64.h> > +#include <linux/units.h> > +#include <linux/limits.h> > +#include <linux/module.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/iio/events.h> > +#include <linux/iio/trigger.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/triggered_buffer.h> > + > +#define OPT_4060_DRV_NAME "opt4060" I'm generally against these DRV_NAME type defines as they tend to imply there is a reason various strings in the driver match when it is an arbitrary choice. I'd rather see the string inline. > + > +struct opt4060_buffer { > + u32 chan[OPT4060_NUM_CHANS]; > + s64 ts; aligned_s64 Which is needed to cover the delights of 32 bit x86 where an s64 is 32bit aligned and this whole structure can end up aligned differently from the obvious. > +}; ... > + > +static u8 opt4060_calculate_crc(u8 exp, u32 mantissa, u8 count) Good to include the formula here. I'm assuming this doesn't correspond to any of the standard crc codes for which we have implementations already. > +{ > + u8 crc; > + > + crc = (hweight32(mantissa) + hweight32(exp) + hweight32(count)) % 2; > + crc |= ((hweight32(mantissa & 0xAAAAA) + hweight32(exp & 0xA) > + + hweight32(count & 0xA)) % 2) << 1; > + crc |= ((hweight32(mantissa & 0x88888) + hweight32(exp & 0x8) > + + hweight32(count & 0x8)) % 2) << 2; > + crc |= (hweight32(mantissa & 0x80808) % 2) << 3; > + > + return crc; > +} > + > +static int opt4060_read_raw_value(struct opt4060_chip *chip, > + unsigned long address, u32 *raw) > +{ > + int ret; > + u32 result; > + u16 msb; > + u16 lsb; > + u8 exp; > + u8 count; > + u8 crc; Combine entrees of same type to save a few lines here > + u8 calc_crc; > + u32 mantissa_raw; > + > + ret = regmap_bulk_read(chip->regmap, address, &result, 2); > + if (ret) > + dev_err(chip->dev, "Reading channel data failed\n"); return ret; and drop the else. Also kernel coding style requires {} for all legs if any need it. > + else { > + count = FIELD_GET(OPT4060_COUNTER_MASK, result >> 16); > + crc = FIELD_GET(OPT4060_CRC_MASK, result >> 16); > + lsb = FIELD_GET(OPT4060_LSB_MASK, result >> 16); > + exp = FIELD_GET(OPT4060_EXPONENT_MASK, result); > + msb = FIELD_GET(OPT4060_MSB_MASK, result); > + mantissa_raw = (msb << 8) + lsb; > + calc_crc = opt4060_calculate_crc(exp, mantissa_raw, count); > + if (calc_crc != crc) > + return -EIO; > + > + *raw = mantissa_raw << exp; > + } > + return ret; > +} > + > +static int opt4060_read_chan_value(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, bool processed) > +{ > + struct opt4060_chip *chip = iio_priv(indio_dev); > + u32 adc_raw; > + int ret; > + /* Set timeout to two times the integration time multiplied by channel count. */ > + unsigned int timeout_us = 2 * OPT4060_NUM_CHANS * > + opt4060_int_time_reg[chip->int_time][0]; > + > + if (chip->irq) { > + reinit_completion(&chip->completion); > + ret = opt4060_trigger_one_shot(chip); > + if (ret) > + return ret; > + if (wait_for_completion_timeout(&chip->completion, > + usecs_to_jiffies(timeout_us)) == 0) > + dev_info(chip->dev, "Completion timed out.\n"); It's an error dev_err() appropriate. > + } > + > + ret = opt4060_read_raw_value(chip, chan->address, &adc_raw); > + if (ret) { > + dev_err(chip->dev, "Reading raw channel data failed\n"); > + return ret; > + } > + if (processed) { Having both processed and raw for a channel needs an explanation. It's not helped by processed being meaningless for color channels as there are not really an well defined units. > + u32 lux_raw; > + u32 rem; > + u32 mul = opt4060_channel_factors[chan->scan_index].mul; > + u32 div = opt4060_channel_factors[chan->scan_index].div; > + > + lux_raw = adc_raw * mul; > + *val = div_u64_rem(lux_raw, div, &rem); > + *val2 = rem * div_u64(div, 10); > + return IIO_VAL_INT_PLUS_NANO; > + } > + *val = adc_raw; > + *val2 = 0; No need to set val2 if IIO_VAL_INT The core code guarantees never to use it. > + return IIO_VAL_INT; > +} > +static int opt4060_power_down(struct opt4060_chip *chip) > +{ > + int ret; > + unsigned int reg; > + > + ret = regmap_read(chip->regmap, OPT4060_CTRL, ®); > + if (ret) { > + dev_err(chip->dev, "Failed to read configuration\n"); > + return ret; > + } > + > + /* MODE_OFF is 0x0 so just set bits to 0 */ > + reg &= ~OPT4060_CTRL_OPER_MODE_MASK; regmap_clear_bits() maybe? > + > + ret = regmap_write(chip->regmap, OPT4060_CTRL, reg); > + if (ret) > + dev_err(chip->dev, "Failed to power down\n"); > + > + return ret; > +} > + > +static void opt4060_chip_off_action(void *data) > +{ > + struct opt4060_chip *chip = data; No need for the local variable. Just rename data to chip is enough. > + > + opt4060_power_down(chip); > +} > + > +#define OPT4060_CHAN(color) \ > +{ \ > + .type = IIO_LIGHT, \ > + .modified = 1, \ > + .channel2 = IIO_MOD_LIGHT_##color, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | \ > + BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), \ > + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME), \ > + .address = OPT4060_##color##_MSB, \ > + .scan_index = OPT4060_##color, \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 32, \ > + .storagebits = 32, \ > + .endianness = IIO_LE, \ > + }, \ > + .event_spec = opt4060_event_spec, \ where you have an optional irq like here, have two sets of chan_spec and pick between them dependent on the availability of that irq. If there is no irq, there should be no event attributes. > + .num_event_specs = ARRAY_SIZE(opt4060_event_spec), \ > +} > + > +static const struct iio_event_spec opt4060_event_spec[] = { > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_RISING, > + .mask_separate = BIT(IIO_EV_INFO_VALUE) | > + BIT(IIO_EV_INFO_ENABLE), > + }, { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_FALLING, > + .mask_separate = BIT(IIO_EV_INFO_VALUE) | > + BIT(IIO_EV_INFO_ENABLE), > + }, { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_EITHER, > + .mask_separate = BIT(IIO_EV_INFO_PERIOD), > + }, > +}; > + > +static bool opt4060_event_active(struct opt4060_chip *chip) > +{ > + return chip->thresh_event_lo_active || chip->thresh_event_hi_active; > +} > + > +static int opt4060_read_available(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + const int **vals, int *type, int *length, > + long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_INT_TIME: > + *length = ARRAY_SIZE(opt4060_int_time_available) * 2; > + *vals = (const int *)opt4060_int_time_available; > + *type = IIO_VAL_INT_PLUS_MICRO; > + return IIO_AVAIL_LIST; > + > + default: > + return -EINVAL; > + } > +} > + One blank line is enough. Check for other instances. > + > +static int opt4060_event_set_state(struct opt4060_chip *chip, bool state) > +{ > + int ret = 0; > + > + if (state) > + ret = opt4060_set_continous_mode(chip, true); return opt... > + else if (!chip->data_trigger_active && chip->irq) > + ret = opt4060_set_continous_mode(chip, false); return opt... > + > + return ret; Not an error? > +} > + > +static int opt4060_trigger_set_state(struct iio_trigger *trig, bool state) > +{ > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > + struct opt4060_chip *chip = iio_priv(indio_dev); > + int ret = 0; > + > + if (indio_dev->trig) { > + iio_trigger_put(indio_dev->trig); > + indio_dev->trig = NULL; > + } > + > + if (state) { > + indio_dev->trig = iio_trigger_get(trig); > + ret = opt4060_set_continous_mode(chip, true); return opt... > + } else if (!opt4060_event_active(chip) && chip->irq) > + ret = opt4060_set_continous_mode(chip, false); return opt > + > + if (trig == chip->data_trig) { > + chip->data_trigger_active = state; > + chip->threshold_trigger_active = !state; > + } else if (trig == chip->threshold_trig) { > + chip->data_trigger_active = !state; > + chip->threshold_trigger_active = state; > + } > + > + return ret; > +} > + > +static int opt4060_get_channel_sel(struct opt4060_chip *chip, int *ch_sel) > +{ > + int ret; > + u32 regval; > + > + ret = regmap_read(chip->regmap, OPT4060_INT_CTRL, ®val); > + if (ret) > + dev_err(chip->dev, "Failed to get channel selection.\n"); return ret; > + else no else needed. > + *ch_sel = FIELD_GET(OPT4060_INT_CTRL_THRESH_SEL, regval); > + return ret; return 0; Check for other cases that look like this. Early returns on error almost always make for more readable code than keeping the return for the end (After doing nothing useful extra) > +} > + > +static int opt4060_set_channel_sel(struct opt4060_chip *chip, int ch_sel) > +{ > + int ret; > + u32 regval; > + > + regval = FIELD_PREP(OPT4060_INT_CTRL_THRESH_SEL, ch_sel); > + ret = regmap_update_bits(chip->regmap, OPT4060_INT_CTRL, > + OPT4060_INT_CTRL_THRESH_SEL, regval); > + if (ret) > + dev_err(chip->dev, "Failed to set channel selection.\n"); > + return ret; > +} > + > +static int opt4060_get_thresholds(struct opt4060_chip *chip, u32 *th_lo, u32 *th_hi) > +{ > + int ret; > + u32 regval; > + > + ret = regmap_read(chip->regmap, OPT4060_THRESHOLD_LOW, ®val); > + if (ret) { > + dev_err(chip->dev, "Failed to read THRESHOLD_LOW.\n"); > + return ret; > + } > + *th_lo = opt4060_calc_val_from_th_reg(regval); > + > + ret = regmap_read(chip->regmap, OPT4060_THRESHOLD_HIGH, ®val); > + if (ret) { > + dev_err(chip->dev, "Failed to read THRESHOLD_LOW.\n"); > + return ret; > + } > + *th_hi = opt4060_calc_val_from_th_reg(regval); > + > + return ret; return 0; > +} > + > +static int opt4060_set_thresholds(struct opt4060_chip *chip, u32 th_lo, u32 th_hi) > +{ > + int ret; > + > + ret = regmap_write(chip->regmap, OPT4060_THRESHOLD_LOW, opt4060_calc_th_reg(th_lo)); > + if (ret) { > + dev_err(chip->dev, "Failed to write THRESHOLD_LOW.\n"); > + return ret; > + } > + > + ret = regmap_write(chip->regmap, OPT4060_THRESHOLD_HIGH, opt4060_calc_th_reg(th_hi)); > + if (ret) > + dev_err(chip->dev, "Failed to write THRESHOLD_HIGH.\n"); > + > + return ret; > +} > + > +static int opt4060_read_event(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, > + int *val, int *val2) > +{ > + int ret = -EINVAL; > + struct opt4060_chip *chip = iio_priv(indio_dev); > + > + switch (info) { > + case IIO_EV_INFO_VALUE: > + if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) { > + u32 th_lo, th_hi; > + > + if (opt4060_get_thresholds(chip, &th_lo, &th_hi)) > + return -EFAULT; > + if (dir == IIO_EV_DIR_FALLING) { > + *val = th_lo; > + ret = IIO_VAL_INT; > + } else if (dir == IIO_EV_DIR_RISING) { > + *val = th_hi; > + ret = IIO_VAL_INT; > + } > + } > + break; Similar refactor to do early returns as suggested for the next function (see below) > + case IIO_EV_INFO_PERIOD: > + return opt4060_read_ev_period(chip, val, val2); > + default: > + return -EINVAL; > + } > + return ret; > +} > + > +static int opt4060_write_event(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, > + int val, int val2) > +{ > + int ret = -EINVAL; > + struct opt4060_chip *chip = iio_priv(indio_dev); > + > + switch (info) { > + case IIO_EV_INFO_VALUE: > + if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) { > + u32 th_lo, th_hi; > + > + if (opt4060_get_thresholds(chip, &th_lo, &th_hi)) > + return -EFAULT; > + if (dir == IIO_EV_DIR_FALLING) > + th_lo = val; > + else if (dir == IIO_EV_DIR_RISING) > + th_hi = val; > + if (opt4060_set_thresholds(chip, th_lo, th_hi)) > + return -EFAULT; > + ret = 0; return 0; > + } return -EINVAL; > + break; > + case IIO_EV_INFO_PERIOD: > + return opt4060_write_ev_period(chip, val, val2); > + default: > + return -EINVAL; > + } > + return ret; Drop this as with above always returned already. > +} > + > +static int opt4060_read_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir) > +{ > + int ret = -EINVAL; > + struct opt4060_chip *chip = iio_priv(indio_dev); > + > + if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) { As below. Consider flipping the condition for an early return Check all similar cases for whether it improves readability. > + int ch_idx = chan->scan_index; > + int ch_sel; > + > + if (opt4060_get_channel_sel(chip, &ch_sel)) > + return -EFAULT; > + > + if (((dir == IIO_EV_DIR_FALLING) && chip->thresh_event_lo_active) || > + ((dir == IIO_EV_DIR_RISING) && chip->thresh_event_hi_active)) > + ret = (ch_sel == ch_idx); > + else > + ret = FALSE; > + } > + return ret; > +} > + > +static int opt4060_write_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, int state) > +{ > + int ret = -EINVAL; > + struct opt4060_chip *chip = iio_priv(indio_dev); > + > + if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) { Maybe flip the condition to reduce effective indent. if (chan->type != IIO_LIGHT) return -EINVAL; if (type != IIO_EV_TYPE_THRESH) reuturn -EINVAL; etc > + int ch_idx = chan->scan_index; > + int ch_sel; > + > + if (opt4060_get_channel_sel(chip, &ch_sel)) > + return -EFAULT; > + > + if (state) { > + /* Only one channel can be active at the same time */ > + if ((chip->thresh_event_lo_active || > + chip->thresh_event_hi_active) && (ch_idx != ch_sel)) > + return -EBUSY; > + if (dir == IIO_EV_DIR_FALLING) > + chip->thresh_event_lo_active = TRUE; > + else if (dir == IIO_EV_DIR_RISING) > + chip->thresh_event_hi_active = TRUE; > + if (opt4060_set_channel_sel(chip, ch_idx)) > + return -EFAULT; > + ret = 0; > + } else { > + if (ch_idx == ch_sel) { > + if (dir == IIO_EV_DIR_FALLING) > + chip->thresh_event_lo_active = FALSE; false etc > + else if (dir == IIO_EV_DIR_RISING) > + chip->thresh_event_hi_active = FALSE; > + } > + ret = 0; > + } > + > + if (opt4060_event_set_state(chip, chip->thresh_event_hi_active | > + chip->thresh_event_lo_active)) > + return -EFAULT; > + } > + return ret; > +} > + > +static bool opt4060_volatile_reg(struct device *dev, unsigned int reg) > +{ > + if ((reg >= OPT4060_RED_MSB && reg <= OPT4060_CLEAR_LSB) || > + reg == OPT4060_RES_CTRL) > + return true; > + return false; As below, just return the condition. > +} > + > +static bool opt4060_writable_reg(struct device *dev, unsigned int reg) > +{ > + if (reg >= OPT4060_THRESHOLD_LOW || reg >= OPT4060_INT_CTRL) > + return true; > + return false; return reg >=... > +} > + > +static bool opt4060_readonly_reg(struct device *dev, unsigned int reg) > +{ > + if (reg == OPT4060_DEVICE_ID) > + return true; > + return false; return reg == .. > +} > + > +static bool opt4060_readable_reg(struct device *dev, unsigned int reg) > +{ > + /* Volatile, writable and read-only registers are readable. */ > + if (opt4060_volatile_reg(dev, reg) || opt4060_writable_reg(dev, reg) || > + opt4060_readonly_reg(dev, reg)) > + return TRUE; true > + return false; return opt.... > +} > + > + > +static irqreturn_t opt4060_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *idev = pf->indio_dev; > + struct opt4060_chip *chip = iio_priv(idev); > + struct opt4060_buffer raw; > + int ret, chan; > + int i = 0; > + > + memset(&raw, 0, sizeof(raw)); > + > + for_each_set_bit(chan, idev->active_scan_mask, idev->masklength) { Try compiling this on latest togreg branch. We have removed direct access to masklength There is a iio_for_each_active_channel() macro that handles this usage more cleanly. > + ret = opt4060_read_raw_value(chip, > + opt4060_channels[chan].address, > + &raw.chan[i++]); > + if (ret) { > + dev_err(chip->dev, "Reading raw channel data failed\n"); > + goto err_read; > + } > + } > + > + iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp); > +err_read: > + iio_trigger_notify_done(idev->trig); > + return IRQ_HANDLED; > +} > + ... > +static int opt4060_setup_triggers(struct opt4060_chip *chip, struct iio_dev *idev) > +{ > + struct iio_trigger *data_trigger; > + struct iio_trigger *threshold_trigger; > + char *name; > + int ret; > + > + ret = devm_iio_triggered_buffer_setup(chip->dev, idev, > + &iio_pollfunc_store_time, > + opt4060_trigger_handler, > + &opt4060_buffer_ops); > + if (ret) > + return dev_err_probe(chip->dev, ret, > + "iio_triggered_buffer_setup_ext FAIL\n"); > + > + data_trigger = devm_iio_trigger_alloc(chip->dev, "%s-data-ready-dev%d", > + idev->name, iio_device_id(idev)); Where it doesn't result in very long lines, align to just after ( That may mean you have more line breaks. If so that is fine. > + if (!data_trigger) > + return -ENOMEM; Slight preference for a blank line after error checks like this. > + chip->data_trig = data_trigger; > + data_trigger->ops = &opt4060_trigger_ops; > + iio_trigger_set_drvdata(data_trigger, idev); > + ret = devm_iio_trigger_register(chip->dev, data_trigger); > + if (ret) > + return dev_err_probe(chip->dev, ret, > + "Data ready trigger registration failed\n"); > + > + threshold_trigger = devm_iio_trigger_alloc(chip->dev, "%s-threshold-dev%d", > + idev->name, iio_device_id(idev)); This needs some explanation. Why a threshold trigger rather than just using events? Superficially looks like you should just use the event interfaces. > + if (!threshold_trigger) > + return -ENOMEM; > + chip->threshold_trig = threshold_trigger; > + threshold_trigger->ops = &opt4060_trigger_ops; > + iio_trigger_set_drvdata(threshold_trigger, idev); > + ret = devm_iio_trigger_register(chip->dev, threshold_trigger); > + if (ret) > + return dev_err_probe(chip->dev, ret, > + "Threshold trigger registration failed\n"); > + One blank line is plenty. > + > + name = devm_kasprintf(chip->dev, GFP_KERNEL, "%s-opt4060", > + dev_name(chip->dev)); Check for failure to allocate. > + > + ret = devm_request_threaded_irq(chip->dev, chip->irq, NULL, opt4060_irq_thread, > + IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | > + IRQF_ONESHOT, name, idev); > + if (ret) > + return dev_err_probe(chip->dev, ret, "Could not request IRQ\n"); > + > + ret = regmap_write_bits(chip->regmap, OPT4060_INT_CTRL, > + OPT4060_INT_CTRL_OUTPUT, > + OPT4060_INT_CTRL_OUTPUT); > + if (ret) > + return dev_err_probe(chip->dev, ret, > + "Failed to set interrupt as output\n"); > + > + return 0; > +} > + > +static int opt4060_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct opt4060_chip *chip; > + struct iio_dev *indio_dev; > + int ret; > + uint dev_id; unsigned int to match the regmap parameter type. > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*chip)); > + if (!indio_dev) > + return -ENOMEM; > + > + chip = iio_priv(indio_dev); > + > + /* Request regulator */ very obvious. Drop the comment. > + ret = devm_regulator_get_enable(dev, "vdd"); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to enable vdd supply\n"); > + > + chip->regmap = devm_regmap_init_i2c(client, &opt4060_regmap_config); > + if (IS_ERR(chip->regmap)) > + return dev_err_probe(dev, PTR_ERR(chip->regmap), > + "regmap initialization failed\n"); > + > + chip->dev = dev; > + chip->irq = client->irq; > + init_completion(&chip->completion); > + > + indio_dev->info = &opt4060_info; > + > + ret = regmap_reinit_cache(chip->regmap, &opt4060_regmap_config); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to reinit regmap cache\n"); > + > + ret = regmap_read(chip->regmap, OPT4060_DEVICE_ID, &dev_id); > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "Failed to read the device ID register\n"); > + > + dev_id = FIELD_GET(OPT4060_DEVICE_ID_MASK, dev_id); > + if (dev_id != OPT4060_DEVICE_ID_VAL) > + dev_warn(dev, "Device ID: %#04x unknown\n", dev_id); Prefer dev_info() for this as it may well be a valid fallback compatible. > + > + indio_dev->channels = opt4060_channels; > + indio_dev->num_channels = ARRAY_SIZE(opt4060_channels); > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->name = OPT_4060_DRV_NAME; > + > + /* Initialize device with default settings */ As below. Comment is (rightly) obvious from the code, so don't have a comment. > + ret = opt4060_load_defaults(chip); > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "Failed to set sensor defaults\n"); > + > + /* If irq is enabled, initialize triggers */ The comment doesn't add anything not obvious from the code. I'd drop it and any similar ones. > + if (chip->irq) { > + ret = opt4060_setup_triggers(chip, indio_dev); > + if (ret) > + return ret; > + } else { > + dev_info(chip->dev, "No IRQ, events and triggers disabled\n"); Too noisy. At most dev_dbg. Probably not useful at all as it is fairly easy to see no irq was registered. > + } > + > + ret = devm_add_action_or_reset(dev, opt4060_chip_off_action, chip); This needs to be alongside whatever turned it on. Or if that is a side effect of later accesses (i.e. it is powered up only channel read) then add a comment to that affect. > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "Failed to setup power off action\n"); > + > + return devm_iio_device_register(dev, indio_dev); > +} > + > +/* > + * The compatible string determines which constants to use depending on > + * opt4060 packaging There is only one. So this comment is currently meaningless. > + */ > +static const struct i2c_device_id opt4060_id[] = { > + { OPT_4060_DRV_NAME, 0 }, Put the string directly in here. There is no reason why this should match the DRV_NAME. Also drop the 0 as it is currently pointless and when you do introduce some data it should be a pointer anyway. > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, opt4060_id); > + > +static const struct of_device_id opt4060_of_match[] = { > + { .compatible = "ti,opt4060" }, > + {} Trivial but I'm trying to get consistency in IIO (slowly) on { } for these terminators (so with the space). I'd prefer to not introduce more instances of {} Thanks, Jonathan > +}; > +MODULE_DEVICE_TABLE(of, opt4060_of_match);
Hi Per-Daniel,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 0c559323bbaabee7346c12e74b497e283aaafef5]
url: https://github.com/intel-lab-lkp/linux/commits/Per-Daniel-Olsson/dt-bindings-iio-light-Document-TI-OPT4060-RGBW-sensor/20241006-005244
base: 0c559323bbaabee7346c12e74b497e283aaafef5
patch link: https://lore.kernel.org/r/20241005165119.3549472-3-perdaniel.olsson%40axis.com
patch subject: [PATCH v2 2/2] iio: light: Add support for TI OPT4060 color sensor
config: um-allmodconfig (https://download.01.org/0day-ci/archive/20241007/202410071407.8Vp5xp85-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project fef3566a25ff0e34fb87339ba5e13eca17cec00f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241007/202410071407.8Vp5xp85-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410071407.8Vp5xp85-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/iio/light/opt4060.c:11:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/iio/light/opt4060.c:11:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/iio/light/opt4060.c:11:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
693 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
701 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
709 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
718 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
727 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
736 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
In file included from drivers/iio/light/opt4060.c:11:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/iio/light/opt4060.c:1026:8: warning: variable 'chan' is uninitialized when used here [-Wuninitialized]
1026 | chan,
| ^~~~
include/linux/iio/events.h:54:51: note: expanded from macro 'IIO_UNMOD_EVENT_CODE'
54 | IIO_EVENT_CODE(chan_type, 0, 0, direction, type, number, 0, 0)
| ^~~~~~
include/linux/iio/events.h:29:9: note: expanded from macro 'IIO_EVENT_CODE'
29 | ((u16)chan))
| ^~~~
drivers/iio/light/opt4060.c:994:15: note: initialize the variable 'chan' to silence this warning
994 | int ret, chan, dummy;
| ^
| = 0
14 warnings generated.
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for MODVERSIONS
Depends on [n]: MODULES [=y] && !COMPILE_TEST [=y]
Selected by [y]:
- RANDSTRUCT_FULL [=y] && (CC_HAS_RANDSTRUCT [=y] || GCC_PLUGINS [=n]) && MODULES [=y]
WARNING: unmet direct dependencies detected for GET_FREE_REGION
Depends on [n]: SPARSEMEM [=n]
Selected by [m]:
- RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]
vim +/chan +1026 drivers/iio/light/opt4060.c
989
990 static irqreturn_t opt4060_irq_thread(int irq, void *private)
991 {
992 struct iio_dev *idev = private;
993 struct opt4060_chip *chip = iio_priv(idev);
994 int ret, chan, dummy;
995 unsigned int int_res;
996
997 ret = regmap_read(chip->regmap, OPT4060_RES_CTRL, &int_res);
998 if (ret < 0) {
999 dev_err(chip->dev, "Failed to read interrupt reasons.\n");
1000 return IRQ_NONE;
1001 }
1002
1003 if (!chip->data_trigger_active) {
1004 /* Only trigger interrupts on thresholds */
1005 ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_THRESHOLD);
1006 if (ret) {
1007 dev_err(chip->dev, "Failed to set interrupt state\n");
1008 return IRQ_NONE;
1009 }
1010 }
1011
1012 /* Read OPT4060_CTRL to clear interrupt */
1013 ret = regmap_read(chip->regmap, OPT4060_CTRL, &dummy);
1014 if (ret < 0) {
1015 dev_err(chip->dev, "Failed to clear interrupt\n");
1016 return IRQ_NONE;
1017 }
1018
1019 /* Handle events */
1020 if (int_res & (OPT4060_RES_CTRL_FLAG_H | OPT4060_RES_CTRL_FLAG_L)) {
1021 u64 code;
1022
1023 /* Check if the interrupt is from the lower threshold */
1024 if (int_res & OPT4060_RES_CTRL_FLAG_L) {
1025 code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> 1026 chan,
1027 IIO_EV_TYPE_THRESH,
1028 IIO_EV_DIR_FALLING);
1029 iio_push_event(idev, code,
1030 iio_get_time_ns(idev));
1031 }
1032 /* Check if the interrupt is from the upper threshold */
1033 if (int_res & OPT4060_RES_CTRL_FLAG_H) {
1034 code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
1035 chan,
1036 IIO_EV_TYPE_THRESH,
1037 IIO_EV_DIR_RISING);
1038 iio_push_event(idev, code,
1039 iio_get_time_ns(idev));
1040 }
1041 /* Handle threshold triggers */
1042 if (chip->threshold_trigger_active && iio_buffer_enabled(idev))
1043 iio_trigger_poll_nested(chip->threshold_trig);
1044 }
1045
1046 /* Handle conversion ready */
1047 if (int_res & OPT4060_RES_CTRL_CONV_READY) {
1048 /* Signal completion for potentially waiting reads */
1049 complete(&chip->completion);
1050
1051 /* Handle data ready triggers */
1052 if (chip->data_trigger_active && iio_buffer_enabled(idev))
1053 iio_trigger_poll_nested(chip->data_trig);
1054 }
1055 return IRQ_HANDLED;
1056 }
1057
Hi Per-Daniel, kernel test robot noticed the following build warnings: [auto build test WARNING on 0c559323bbaabee7346c12e74b497e283aaafef5] url: https://github.com/intel-lab-lkp/linux/commits/Per-Daniel-Olsson/dt-bindings-iio-light-Document-TI-OPT4060-RGBW-sensor/20241006-005244 base: 0c559323bbaabee7346c12e74b497e283aaafef5 patch link: https://lore.kernel.org/r/20241005165119.3549472-3-perdaniel.olsson%40axis.com patch subject: [PATCH v2 2/2] iio: light: Add support for TI OPT4060 color sensor config: csky-randconfig-r122-20241007 (https://download.01.org/0day-ci/archive/20241007/202410072240.s3wGb17S-lkp@intel.com/config) compiler: csky-linux-gcc (GCC) 14.1.0 reproduce: (https://download.01.org/0day-ci/archive/20241007/202410072240.s3wGb17S-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202410072240.s3wGb17S-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/iio/light/opt4060.c:963:9: sparse: sparse: dereference of noderef expression >> drivers/iio/light/opt4060.c:963:9: sparse: sparse: dereference of noderef expression >> drivers/iio/light/opt4060.c:963:9: sparse: sparse: dereference of noderef expression vim +963 drivers/iio/light/opt4060.c 951 952 static irqreturn_t opt4060_trigger_handler(int irq, void *p) 953 { 954 struct iio_poll_func *pf = p; 955 struct iio_dev *idev = pf->indio_dev; 956 struct opt4060_chip *chip = iio_priv(idev); 957 struct opt4060_buffer raw; 958 int ret, chan; 959 int i = 0; 960 961 memset(&raw, 0, sizeof(raw)); 962 > 963 for_each_set_bit(chan, idev->active_scan_mask, idev->masklength) { 964 ret = opt4060_read_raw_value(chip, 965 opt4060_channels[chan].address, 966 &raw.chan[i++]); 967 if (ret) { 968 dev_err(chip->dev, "Reading raw channel data failed\n"); 969 goto err_read; 970 } 971 } 972 973 iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp); 974 err_read: 975 iio_trigger_notify_done(idev->trig); 976 return IRQ_HANDLED; 977 } 978
On 10/6/24 17:18, Jonathan Cameron wrote: > On Sat, 5 Oct 2024 18:51:19 +0200 > Per-Daniel Olsson <perdaniel.olsson@axis.com> wrote: > >> Add support for Texas Instruments OPT4060 RGBW Color sensor. > Hi Per-Daniel. > > Thanks for your driver. Various comments inline. Hi Jonathan. Thank you for your comments. I will address and correct the inline comments in the next version and I will try to answer your two bigger questions here. I replied to your comments in the cover letter earlier but I forgot to answer them here also, as you requested. > The two bigger things I'd like to understand are: > 1) Why we have a threshold trigger? That is unusual so I'd like to fully understand > the use case. I see an explanation in your cover letter so I'll comment on that here > but I would like one here as well. Our userspace application needs the values after getting the threshold event. Before I implemented the threshold trigger and the triggered buffer, the application would read the values from sysfs right after the event. In that case the values will not be the ones that actually triggered the event. When the light condition is close to the threshold, the value from sysfs might even be on the wrong side of the threshold which can be confusing for the state machine in userspace. I would say that this feature is fairly important to us, this is the way our userspace is using the sensor. > > 2) Why do we need processed and raw? That may make sense but it is also unusual > so I'd like a comment in this patch description on your reasoning. > The raw values are the actual ADC values from the sensor. Those values are the ones that are needed when setting thresholds, so the raw values are definitely needed. The processed values are a conversion to something that should be close to lux. This is not something I know much about, I just read the data sheet (https://www.ti.com/lit/gpn/opt4060). According to chapter 8.4.5.2 (page 17), lux can be calculated this way: lux = GREEN_ADC_RAW * 2.15e-3 It is also stated that R=G=B for a D65 standard white light source if red is multiplied by 2.4 and blue is multiplied with 1.3. We currently don't use the processed values in our use of the sensor, but I assume that other users will expect this functionality. > I've only raised some style issues in a few places. Please check to see if those > apply more broadly. I'll try to address the style issues broadly in the next version. Best regards / P-D > > Thanks, > > Jonathan > > >> >> Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.com> >> --- >> drivers/iio/light/Kconfig | 13 + >> drivers/iio/light/Makefile | 1 + >> drivers/iio/light/opt4060.c | 1216 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 1230 insertions(+) >> create mode 100644 drivers/iio/light/opt4060.c >> >> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig >> index 515ff46b5b82..0e2059512157 100644 >> --- a/drivers/iio/light/Kconfig >> +++ b/drivers/iio/light/Kconfig >> @@ -490,6 +490,19 @@ config OPT4001 >> If built as a dynamically linked module, it will be called >> opt4001. >> >> +config OPT4060 >> + tristate "Texas Instruments OPT4060 RGBW Color Sensor" >> + depends on I2C >> + select REGMAP_I2C >> + select IIO_BUFFER >> + select IIO_TRIGGERED_BUFFER >> + help >> + If you say Y or M here, you get support for Texas Instruments >> + OPT4060 RGBW Color Sensor. >> + >> + If built as a dynamically linked module, it will be called >> + opt4060. >> + >> config PA12203001 >> tristate "TXC PA12203001 light and proximity sensor" >> depends on I2C >> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile >> index 321010fc0b93..55902b21ec0c 100644 >> --- a/drivers/iio/light/Makefile >> +++ b/drivers/iio/light/Makefile >> @@ -42,6 +42,7 @@ obj-$(CONFIG_MAX44009) += max44009.o >> obj-$(CONFIG_NOA1305) += noa1305.o >> obj-$(CONFIG_OPT3001) += opt3001.o >> obj-$(CONFIG_OPT4001) += opt4001.o >> +obj-$(CONFIG_OPT4060) += opt4060.o >> obj-$(CONFIG_PA12203001) += pa12203001.o >> obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o >> obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o >> diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c >> new file mode 100644 >> index 000000000000..a917b59cd500 >> --- /dev/null >> +++ b/drivers/iio/light/opt4060.c >> @@ -0,0 +1,1216 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (C) 2024 Axis Communications AB >> + * >> + * Datasheet: https://www.ti.com/lit/gpn/opt4060 >> + * >> + * Device driver for the Texas Instruments OPT4060 RGBW Color Sensor. >> + */ >> + >> +#include <linux/bitfield.h> >> +#include <linux/i2c.h> >> +#include <linux/iio/iio.h> >> +#include <linux/math64.h> >> +#include <linux/units.h> >> +#include <linux/limits.h> >> +#include <linux/module.h> >> +#include <linux/property.h> >> +#include <linux/regmap.h> >> +#include <linux/regulator/consumer.h> >> +#include <linux/iio/events.h> >> +#include <linux/iio/trigger.h> >> +#include <linux/iio/trigger_consumer.h> >> +#include <linux/iio/triggered_buffer.h> >> + >> +#define OPT_4060_DRV_NAME "opt4060" > I'm generally against these DRV_NAME type defines as they tend to imply there > is a reason various strings in the driver match when it is an arbitrary choice. > I'd rather see the string inline. > >> + >> +struct opt4060_buffer { >> + u32 chan[OPT4060_NUM_CHANS]; >> + s64 ts; > aligned_s64 > Which is needed to cover the delights of 32 bit x86 where an s64 is 32bit aligned > and this whole structure can end up aligned differently from the obvious. > > >> +}; > > ... >> + >> +static u8 opt4060_calculate_crc(u8 exp, u32 mantissa, u8 count) > > Good to include the formula here. I'm assuming this doesn't correspond > to any of the standard crc codes for which we have implementations already. > >> +{ >> + u8 crc; >> + >> + crc = (hweight32(mantissa) + hweight32(exp) + hweight32(count)) % 2; >> + crc |= ((hweight32(mantissa & 0xAAAAA) + hweight32(exp & 0xA) >> + + hweight32(count & 0xA)) % 2) << 1; >> + crc |= ((hweight32(mantissa & 0x88888) + hweight32(exp & 0x8) >> + + hweight32(count & 0x8)) % 2) << 2; >> + crc |= (hweight32(mantissa & 0x80808) % 2) << 3; >> + >> + return crc; >> +} > > >> + >> +static int opt4060_read_raw_value(struct opt4060_chip *chip, >> + unsigned long address, u32 *raw) >> +{ >> + int ret; >> + u32 result; >> + u16 msb; >> + u16 lsb; >> + u8 exp; >> + u8 count; >> + u8 crc; > > Combine entrees of same type to save a few lines here > >> + u8 calc_crc; >> + u32 mantissa_raw; >> + >> + ret = regmap_bulk_read(chip->regmap, address, &result, 2); >> + if (ret) >> + dev_err(chip->dev, "Reading channel data failed\n"); > return ret; and drop the else. > > Also kernel coding style requires {} for all legs if any need it. > >> + else { >> + count = FIELD_GET(OPT4060_COUNTER_MASK, result >> 16); >> + crc = FIELD_GET(OPT4060_CRC_MASK, result >> 16); >> + lsb = FIELD_GET(OPT4060_LSB_MASK, result >> 16); >> + exp = FIELD_GET(OPT4060_EXPONENT_MASK, result); >> + msb = FIELD_GET(OPT4060_MSB_MASK, result); >> + mantissa_raw = (msb << 8) + lsb; >> + calc_crc = opt4060_calculate_crc(exp, mantissa_raw, count); >> + if (calc_crc != crc) >> + return -EIO; >> + >> + *raw = mantissa_raw << exp; >> + } >> + return ret; >> +} >> + >> +static int opt4060_read_chan_value(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, >> + int *val, int *val2, bool processed) >> +{ >> + struct opt4060_chip *chip = iio_priv(indio_dev); >> + u32 adc_raw; >> + int ret; >> + /* Set timeout to two times the integration time multiplied by channel count. */ >> + unsigned int timeout_us = 2 * OPT4060_NUM_CHANS * >> + opt4060_int_time_reg[chip->int_time][0]; >> + >> + if (chip->irq) { >> + reinit_completion(&chip->completion); >> + ret = opt4060_trigger_one_shot(chip); >> + if (ret) >> + return ret; >> + if (wait_for_completion_timeout(&chip->completion, >> + usecs_to_jiffies(timeout_us)) == 0) >> + dev_info(chip->dev, "Completion timed out.\n"); > > It's an error dev_err() appropriate. > >> + } >> + >> + ret = opt4060_read_raw_value(chip, chan->address, &adc_raw); >> + if (ret) { >> + dev_err(chip->dev, "Reading raw channel data failed\n"); >> + return ret; >> + } >> + if (processed) { > > Having both processed and raw for a channel needs an explanation. > It's not helped by processed being meaningless for color channels as there > are not really an well defined units. > > >> + u32 lux_raw; >> + u32 rem; >> + u32 mul = opt4060_channel_factors[chan->scan_index].mul; >> + u32 div = opt4060_channel_factors[chan->scan_index].div; >> + >> + lux_raw = adc_raw * mul; >> + *val = div_u64_rem(lux_raw, div, &rem); >> + *val2 = rem * div_u64(div, 10); >> + return IIO_VAL_INT_PLUS_NANO; >> + } >> + *val = adc_raw; >> + *val2 = 0; > > No need to set val2 if IIO_VAL_INT > The core code guarantees never to use it. > >> + return IIO_VAL_INT; >> +} > >> +static int opt4060_power_down(struct opt4060_chip *chip) >> +{ >> + int ret; >> + unsigned int reg; >> + >> + ret = regmap_read(chip->regmap, OPT4060_CTRL, ®); >> + if (ret) { >> + dev_err(chip->dev, "Failed to read configuration\n"); >> + return ret; >> + } >> + >> + /* MODE_OFF is 0x0 so just set bits to 0 */ >> + reg &= ~OPT4060_CTRL_OPER_MODE_MASK; > > regmap_clear_bits() maybe? > >> + >> + ret = regmap_write(chip->regmap, OPT4060_CTRL, reg); >> + if (ret) >> + dev_err(chip->dev, "Failed to power down\n"); >> + >> + return ret; >> +} >> + >> +static void opt4060_chip_off_action(void *data) >> +{ >> + struct opt4060_chip *chip = data; > No need for the local variable. Just rename data to chip is enough. >> + >> + opt4060_power_down(chip); >> +} >> + >> +#define OPT4060_CHAN(color) \ >> +{ \ >> + .type = IIO_LIGHT, \ >> + .modified = 1, \ >> + .channel2 = IIO_MOD_LIGHT_##color, \ >> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | \ >> + BIT(IIO_CHAN_INFO_RAW), \ >> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), \ >> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME), \ >> + .address = OPT4060_##color##_MSB, \ >> + .scan_index = OPT4060_##color, \ >> + .scan_type = { \ >> + .sign = 'u', \ >> + .realbits = 32, \ >> + .storagebits = 32, \ >> + .endianness = IIO_LE, \ >> + }, \ >> + .event_spec = opt4060_event_spec, \ > where you have an optional irq like here, have two sets of chan_spec and pick > between them dependent on the availability of that irq. > > If there is no irq, there should be no event attributes. > >> + .num_event_specs = ARRAY_SIZE(opt4060_event_spec), \ >> +} >> + >> +static const struct iio_event_spec opt4060_event_spec[] = { >> + { >> + .type = IIO_EV_TYPE_THRESH, >> + .dir = IIO_EV_DIR_RISING, >> + .mask_separate = BIT(IIO_EV_INFO_VALUE) | >> + BIT(IIO_EV_INFO_ENABLE), >> + }, { >> + .type = IIO_EV_TYPE_THRESH, >> + .dir = IIO_EV_DIR_FALLING, >> + .mask_separate = BIT(IIO_EV_INFO_VALUE) | >> + BIT(IIO_EV_INFO_ENABLE), >> + }, { >> + .type = IIO_EV_TYPE_THRESH, >> + .dir = IIO_EV_DIR_EITHER, >> + .mask_separate = BIT(IIO_EV_INFO_PERIOD), >> + }, >> +}; > >> + >> +static bool opt4060_event_active(struct opt4060_chip *chip) >> +{ >> + return chip->thresh_event_lo_active || chip->thresh_event_hi_active; >> +} >> + >> +static int opt4060_read_available(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, >> + const int **vals, int *type, int *length, >> + long mask) >> +{ >> + switch (mask) { >> + case IIO_CHAN_INFO_INT_TIME: >> + *length = ARRAY_SIZE(opt4060_int_time_available) * 2; >> + *vals = (const int *)opt4060_int_time_available; >> + *type = IIO_VAL_INT_PLUS_MICRO; >> + return IIO_AVAIL_LIST; >> + >> + default: >> + return -EINVAL; >> + } >> +} >> + > One blank line is enough. Check for other instances. > >> + >> +static int opt4060_event_set_state(struct opt4060_chip *chip, bool state) >> +{ >> + int ret = 0; >> + >> + if (state) >> + ret = opt4060_set_continous_mode(chip, true); > return opt... >> + else if (!chip->data_trigger_active && chip->irq) >> + ret = opt4060_set_continous_mode(chip, false); > return opt... > >> + >> + return ret; > Not an error? > >> +} >> + >> +static int opt4060_trigger_set_state(struct iio_trigger *trig, bool state) >> +{ >> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); >> + struct opt4060_chip *chip = iio_priv(indio_dev); >> + int ret = 0; >> + >> + if (indio_dev->trig) { >> + iio_trigger_put(indio_dev->trig); >> + indio_dev->trig = NULL; >> + } >> + >> + if (state) { >> + indio_dev->trig = iio_trigger_get(trig); >> + ret = opt4060_set_continous_mode(chip, true); > return opt... > >> + } else if (!opt4060_event_active(chip) && chip->irq) >> + ret = opt4060_set_continous_mode(chip, false); > return opt >> + >> + if (trig == chip->data_trig) { >> + chip->data_trigger_active = state; >> + chip->threshold_trigger_active = !state; >> + } else if (trig == chip->threshold_trig) { >> + chip->data_trigger_active = !state; >> + chip->threshold_trigger_active = state; >> + } >> + >> + return ret; >> +} > >> + >> +static int opt4060_get_channel_sel(struct opt4060_chip *chip, int *ch_sel) >> +{ >> + int ret; >> + u32 regval; >> + >> + ret = regmap_read(chip->regmap, OPT4060_INT_CTRL, ®val); >> + if (ret) >> + dev_err(chip->dev, "Failed to get channel selection.\n"); > return ret; >> + else > no else needed. >> + *ch_sel = FIELD_GET(OPT4060_INT_CTRL_THRESH_SEL, regval); >> + return ret; > return 0; > Check for other cases that look like this. Early returns on error almost > always make for more readable code than keeping the return for the end > (After doing nothing useful extra) >> +} >> + >> +static int opt4060_set_channel_sel(struct opt4060_chip *chip, int ch_sel) >> +{ >> + int ret; >> + u32 regval; >> + >> + regval = FIELD_PREP(OPT4060_INT_CTRL_THRESH_SEL, ch_sel); >> + ret = regmap_update_bits(chip->regmap, OPT4060_INT_CTRL, >> + OPT4060_INT_CTRL_THRESH_SEL, regval); >> + if (ret) >> + dev_err(chip->dev, "Failed to set channel selection.\n"); >> + return ret; >> +} >> + >> +static int opt4060_get_thresholds(struct opt4060_chip *chip, u32 *th_lo, u32 *th_hi) >> +{ >> + int ret; >> + u32 regval; >> + >> + ret = regmap_read(chip->regmap, OPT4060_THRESHOLD_LOW, ®val); >> + if (ret) { >> + dev_err(chip->dev, "Failed to read THRESHOLD_LOW.\n"); >> + return ret; >> + } >> + *th_lo = opt4060_calc_val_from_th_reg(regval); >> + >> + ret = regmap_read(chip->regmap, OPT4060_THRESHOLD_HIGH, ®val); >> + if (ret) { >> + dev_err(chip->dev, "Failed to read THRESHOLD_LOW.\n"); >> + return ret; >> + } >> + *th_hi = opt4060_calc_val_from_th_reg(regval); >> + >> + return ret; > return 0; >> +} >> + >> +static int opt4060_set_thresholds(struct opt4060_chip *chip, u32 th_lo, u32 th_hi) >> +{ >> + int ret; >> + >> + ret = regmap_write(chip->regmap, OPT4060_THRESHOLD_LOW, opt4060_calc_th_reg(th_lo)); >> + if (ret) { >> + dev_err(chip->dev, "Failed to write THRESHOLD_LOW.\n"); >> + return ret; >> + } >> + >> + ret = regmap_write(chip->regmap, OPT4060_THRESHOLD_HIGH, opt4060_calc_th_reg(th_hi)); >> + if (ret) >> + dev_err(chip->dev, "Failed to write THRESHOLD_HIGH.\n"); >> + >> + return ret; >> +} >> + >> +static int opt4060_read_event(struct iio_dev *indio_dev, >> + const struct iio_chan_spec *chan, >> + enum iio_event_type type, >> + enum iio_event_direction dir, >> + enum iio_event_info info, >> + int *val, int *val2) >> +{ >> + int ret = -EINVAL; >> + struct opt4060_chip *chip = iio_priv(indio_dev); >> + >> + switch (info) { >> + case IIO_EV_INFO_VALUE: >> + if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) { >> + u32 th_lo, th_hi; >> + >> + if (opt4060_get_thresholds(chip, &th_lo, &th_hi)) >> + return -EFAULT; >> + if (dir == IIO_EV_DIR_FALLING) { >> + *val = th_lo; >> + ret = IIO_VAL_INT; >> + } else if (dir == IIO_EV_DIR_RISING) { >> + *val = th_hi; >> + ret = IIO_VAL_INT; >> + } >> + } >> + break; > Similar refactor to do early returns as suggested for the next function (see below) > >> + case IIO_EV_INFO_PERIOD: >> + return opt4060_read_ev_period(chip, val, val2); >> + default: >> + return -EINVAL; >> + } >> + return ret; >> +} >> + >> +static int opt4060_write_event(struct iio_dev *indio_dev, >> + const struct iio_chan_spec *chan, >> + enum iio_event_type type, >> + enum iio_event_direction dir, >> + enum iio_event_info info, >> + int val, int val2) >> +{ >> + int ret = -EINVAL; >> + struct opt4060_chip *chip = iio_priv(indio_dev); >> + >> + switch (info) { >> + case IIO_EV_INFO_VALUE: >> + if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) { >> + u32 th_lo, th_hi; >> + >> + if (opt4060_get_thresholds(chip, &th_lo, &th_hi)) >> + return -EFAULT; >> + if (dir == IIO_EV_DIR_FALLING) >> + th_lo = val; >> + else if (dir == IIO_EV_DIR_RISING) >> + th_hi = val; >> + if (opt4060_set_thresholds(chip, th_lo, th_hi)) >> + return -EFAULT; >> + ret = 0; > return 0; >> + } > return -EINVAL; >> + break; >> + case IIO_EV_INFO_PERIOD: >> + return opt4060_write_ev_period(chip, val, val2); >> + default: >> + return -EINVAL; >> + } >> + return ret; > Drop this as with above always returned already. > >> +} >> + >> +static int opt4060_read_event_config(struct iio_dev *indio_dev, >> + const struct iio_chan_spec *chan, >> + enum iio_event_type type, >> + enum iio_event_direction dir) >> +{ >> + int ret = -EINVAL; >> + struct opt4060_chip *chip = iio_priv(indio_dev); >> + >> + if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) { > As below. Consider flipping the condition for an early return > Check all similar cases for whether it improves readability. > > >> + int ch_idx = chan->scan_index; >> + int ch_sel; >> + >> + if (opt4060_get_channel_sel(chip, &ch_sel)) >> + return -EFAULT; >> + >> + if (((dir == IIO_EV_DIR_FALLING) && chip->thresh_event_lo_active) || >> + ((dir == IIO_EV_DIR_RISING) && chip->thresh_event_hi_active)) >> + ret = (ch_sel == ch_idx); >> + else >> + ret = FALSE; >> + } >> + return ret; >> +} >> + >> +static int opt4060_write_event_config(struct iio_dev *indio_dev, >> + const struct iio_chan_spec *chan, >> + enum iio_event_type type, >> + enum iio_event_direction dir, int state) >> +{ >> + int ret = -EINVAL; >> + struct opt4060_chip *chip = iio_priv(indio_dev); >> + >> + if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) { > Maybe flip the condition to reduce effective indent. > if (chan->type != IIO_LIGHT) > return -EINVAL; > if (type != IIO_EV_TYPE_THRESH) > reuturn -EINVAL; > > etc >> + int ch_idx = chan->scan_index; >> + int ch_sel; >> + >> + if (opt4060_get_channel_sel(chip, &ch_sel)) >> + return -EFAULT; >> + >> + if (state) { >> + /* Only one channel can be active at the same time */ >> + if ((chip->thresh_event_lo_active || >> + chip->thresh_event_hi_active) && (ch_idx != ch_sel)) >> + return -EBUSY; >> + if (dir == IIO_EV_DIR_FALLING) >> + chip->thresh_event_lo_active = TRUE; >> + else if (dir == IIO_EV_DIR_RISING) >> + chip->thresh_event_hi_active = TRUE; >> + if (opt4060_set_channel_sel(chip, ch_idx)) >> + return -EFAULT; >> + ret = 0; >> + } else { >> + if (ch_idx == ch_sel) { >> + if (dir == IIO_EV_DIR_FALLING) >> + chip->thresh_event_lo_active = FALSE; > false etc > >> + else if (dir == IIO_EV_DIR_RISING) >> + chip->thresh_event_hi_active = FALSE; >> + } >> + ret = 0; >> + } >> + >> + if (opt4060_event_set_state(chip, chip->thresh_event_hi_active | >> + chip->thresh_event_lo_active)) >> + return -EFAULT; >> + } >> + return ret; >> +} > >> + >> +static bool opt4060_volatile_reg(struct device *dev, unsigned int reg) >> +{ >> + if ((reg >= OPT4060_RED_MSB && reg <= OPT4060_CLEAR_LSB) || >> + reg == OPT4060_RES_CTRL) >> + return true; >> + return false; > > As below, just return the condition. > >> +} >> + >> +static bool opt4060_writable_reg(struct device *dev, unsigned int reg) >> +{ >> + if (reg >= OPT4060_THRESHOLD_LOW || reg >= OPT4060_INT_CTRL) >> + return true; >> + return false; > return reg >=... >> +} >> + >> +static bool opt4060_readonly_reg(struct device *dev, unsigned int reg) >> +{ >> + if (reg == OPT4060_DEVICE_ID) >> + return true; >> + return false; > return reg == .. > >> +} >> + >> +static bool opt4060_readable_reg(struct device *dev, unsigned int reg) >> +{ >> + /* Volatile, writable and read-only registers are readable. */ >> + if (opt4060_volatile_reg(dev, reg) || opt4060_writable_reg(dev, reg) || >> + opt4060_readonly_reg(dev, reg)) >> + return TRUE; > true > >> + return false; > > return opt.... > >> +} >> + > >> + >> +static irqreturn_t opt4060_trigger_handler(int irq, void *p) >> +{ >> + struct iio_poll_func *pf = p; >> + struct iio_dev *idev = pf->indio_dev; >> + struct opt4060_chip *chip = iio_priv(idev); >> + struct opt4060_buffer raw; >> + int ret, chan; >> + int i = 0; >> + >> + memset(&raw, 0, sizeof(raw)); >> + >> + for_each_set_bit(chan, idev->active_scan_mask, idev->masklength) { > Try compiling this on latest togreg branch. We have removed direct > access to masklength > > There is a iio_for_each_active_channel() macro that handles this usage > more cleanly. > >> + ret = opt4060_read_raw_value(chip, >> + opt4060_channels[chan].address, >> + &raw.chan[i++]); >> + if (ret) { >> + dev_err(chip->dev, "Reading raw channel data failed\n"); >> + goto err_read; >> + } >> + } >> + >> + iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp); >> +err_read: >> + iio_trigger_notify_done(idev->trig); >> + return IRQ_HANDLED; >> +} >> + > > ... > >> +static int opt4060_setup_triggers(struct opt4060_chip *chip, struct iio_dev *idev) >> +{ >> + struct iio_trigger *data_trigger; >> + struct iio_trigger *threshold_trigger; >> + char *name; >> + int ret; >> + >> + ret = devm_iio_triggered_buffer_setup(chip->dev, idev, >> + &iio_pollfunc_store_time, >> + opt4060_trigger_handler, >> + &opt4060_buffer_ops); >> + if (ret) >> + return dev_err_probe(chip->dev, ret, >> + "iio_triggered_buffer_setup_ext FAIL\n"); >> + >> + data_trigger = devm_iio_trigger_alloc(chip->dev, "%s-data-ready-dev%d", >> + idev->name, iio_device_id(idev)); > > Where it doesn't result in very long lines, align to just after ( > > That may mean you have more line breaks. If so that is fine. > > >> + if (!data_trigger) >> + return -ENOMEM; > Slight preference for a blank line after error checks like this. > >> + chip->data_trig = data_trigger; >> + data_trigger->ops = &opt4060_trigger_ops; >> + iio_trigger_set_drvdata(data_trigger, idev); >> + ret = devm_iio_trigger_register(chip->dev, data_trigger); >> + if (ret) >> + return dev_err_probe(chip->dev, ret, >> + "Data ready trigger registration failed\n"); >> + >> + threshold_trigger = devm_iio_trigger_alloc(chip->dev, "%s-threshold-dev%d", >> + idev->name, iio_device_id(idev)); > > This needs some explanation. Why a threshold trigger rather than just using > events? Superficially looks like you should just use the event interfaces. > >> + if (!threshold_trigger) >> + return -ENOMEM; >> + chip->threshold_trig = threshold_trigger; >> + threshold_trigger->ops = &opt4060_trigger_ops; >> + iio_trigger_set_drvdata(threshold_trigger, idev); >> + ret = devm_iio_trigger_register(chip->dev, threshold_trigger); >> + if (ret) >> + return dev_err_probe(chip->dev, ret, >> + "Threshold trigger registration failed\n"); >> + > One blank line is plenty. >> + >> + name = devm_kasprintf(chip->dev, GFP_KERNEL, "%s-opt4060", >> + dev_name(chip->dev)); > > Check for failure to allocate. > >> + >> + ret = devm_request_threaded_irq(chip->dev, chip->irq, NULL, opt4060_irq_thread, >> + IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | >> + IRQF_ONESHOT, name, idev); >> + if (ret) >> + return dev_err_probe(chip->dev, ret, "Could not request IRQ\n"); >> + >> + ret = regmap_write_bits(chip->regmap, OPT4060_INT_CTRL, >> + OPT4060_INT_CTRL_OUTPUT, >> + OPT4060_INT_CTRL_OUTPUT); >> + if (ret) >> + return dev_err_probe(chip->dev, ret, >> + "Failed to set interrupt as output\n"); >> + >> + return 0; >> +} >> + >> +static int opt4060_probe(struct i2c_client *client) >> +{ >> + struct device *dev = &client->dev; >> + struct opt4060_chip *chip; >> + struct iio_dev *indio_dev; >> + int ret; >> + uint dev_id; > > unsigned int to match the regmap parameter type. > >> + >> + indio_dev = devm_iio_device_alloc(dev, sizeof(*chip)); >> + if (!indio_dev) >> + return -ENOMEM; >> + >> + chip = iio_priv(indio_dev); >> + >> + /* Request regulator */ > > very obvious. Drop the comment. > >> + ret = devm_regulator_get_enable(dev, "vdd"); >> + if (ret) >> + return dev_err_probe(dev, ret, "Failed to enable vdd supply\n"); >> + >> + chip->regmap = devm_regmap_init_i2c(client, &opt4060_regmap_config); >> + if (IS_ERR(chip->regmap)) >> + return dev_err_probe(dev, PTR_ERR(chip->regmap), >> + "regmap initialization failed\n"); >> + >> + chip->dev = dev; >> + chip->irq = client->irq; >> + init_completion(&chip->completion); >> + >> + indio_dev->info = &opt4060_info; >> + >> + ret = regmap_reinit_cache(chip->regmap, &opt4060_regmap_config); >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "failed to reinit regmap cache\n"); >> + >> + ret = regmap_read(chip->regmap, OPT4060_DEVICE_ID, &dev_id); >> + if (ret < 0) >> + return dev_err_probe(dev, ret, >> + "Failed to read the device ID register\n"); >> + >> + dev_id = FIELD_GET(OPT4060_DEVICE_ID_MASK, dev_id); >> + if (dev_id != OPT4060_DEVICE_ID_VAL) >> + dev_warn(dev, "Device ID: %#04x unknown\n", dev_id); > > Prefer dev_info() for this as it may well be a valid fallback > compatible. > >> + >> + indio_dev->channels = opt4060_channels; >> + indio_dev->num_channels = ARRAY_SIZE(opt4060_channels); >> + indio_dev->modes = INDIO_DIRECT_MODE; >> + indio_dev->name = OPT_4060_DRV_NAME; >> + >> + /* Initialize device with default settings */ > > As below. Comment is (rightly) obvious from the code, so don't have > a comment. > >> + ret = opt4060_load_defaults(chip); >> + if (ret < 0) >> + return dev_err_probe(dev, ret, >> + "Failed to set sensor defaults\n"); >> + >> + /* If irq is enabled, initialize triggers */ > The comment doesn't add anything not obvious from the code. I'd drop it > and any similar ones. >> + if (chip->irq) { >> + ret = opt4060_setup_triggers(chip, indio_dev); >> + if (ret) >> + return ret; >> + } else { >> + dev_info(chip->dev, "No IRQ, events and triggers disabled\n"); > > Too noisy. At most dev_dbg. Probably not useful at all as it is fairly easy > to see no irq was registered. > >> + } >> + >> + ret = devm_add_action_or_reset(dev, opt4060_chip_off_action, chip); > > This needs to be alongside whatever turned it on. Or if that is a side effect > of later accesses (i.e. it is powered up only channel read) then add > a comment to that affect. > >> + if (ret < 0) >> + return dev_err_probe(dev, ret, >> + "Failed to setup power off action\n"); >> + >> + return devm_iio_device_register(dev, indio_dev); >> +} >> + >> +/* >> + * The compatible string determines which constants to use depending on >> + * opt4060 packaging > There is only one. So this comment is currently meaningless. > >> + */ >> +static const struct i2c_device_id opt4060_id[] = { >> + { OPT_4060_DRV_NAME, 0 }, > Put the string directly in here. There is no reason why this > should match the DRV_NAME. Also drop the 0 as it is currently > pointless and when you do introduce some data it should be a pointer > anyway. > >> + {} >> +}; >> +MODULE_DEVICE_TABLE(i2c, opt4060_id); >> + >> +static const struct of_device_id opt4060_of_match[] = { >> + { .compatible = "ti,opt4060" }, >> + {} > > Trivial but I'm trying to get consistency in IIO (slowly) on > { } for these terminators (so with the space). > I'd prefer to not introduce more instances of {} > > Thanks, > > Jonathan > > >> +}; >> +MODULE_DEVICE_TABLE(of, opt4060_of_match);
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 515ff46b5b82..0e2059512157 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -490,6 +490,19 @@ config OPT4001 If built as a dynamically linked module, it will be called opt4001. +config OPT4060 + tristate "Texas Instruments OPT4060 RGBW Color Sensor" + depends on I2C + select REGMAP_I2C + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + help + If you say Y or M here, you get support for Texas Instruments + OPT4060 RGBW Color Sensor. + + If built as a dynamically linked module, it will be called + opt4060. + config PA12203001 tristate "TXC PA12203001 light and proximity sensor" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index 321010fc0b93..55902b21ec0c 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -42,6 +42,7 @@ obj-$(CONFIG_MAX44009) += max44009.o obj-$(CONFIG_NOA1305) += noa1305.o obj-$(CONFIG_OPT3001) += opt3001.o obj-$(CONFIG_OPT4001) += opt4001.o +obj-$(CONFIG_OPT4060) += opt4060.o obj-$(CONFIG_PA12203001) += pa12203001.o obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c new file mode 100644 index 000000000000..a917b59cd500 --- /dev/null +++ b/drivers/iio/light/opt4060.c @@ -0,0 +1,1216 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2024 Axis Communications AB + * + * Datasheet: https://www.ti.com/lit/gpn/opt4060 + * + * Device driver for the Texas Instruments OPT4060 RGBW Color Sensor. + */ + +#include <linux/bitfield.h> +#include <linux/i2c.h> +#include <linux/iio/iio.h> +#include <linux/math64.h> +#include <linux/units.h> +#include <linux/limits.h> +#include <linux/module.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/iio/events.h> +#include <linux/iio/trigger.h> +#include <linux/iio/trigger_consumer.h> +#include <linux/iio/triggered_buffer.h> + +#define OPT_4060_DRV_NAME "opt4060" + +/* OPT4060 register set */ +#define OPT4060_RED_MSB 0x00 +#define OPT4060_RED_LSB 0x01 +#define OPT4060_GREEN_MSB 0x02 +#define OPT4060_GREEN_LSB 0x03 +#define OPT4060_BLUE_MSB 0x04 +#define OPT4060_BLUE_LSB 0x05 +#define OPT4060_CLEAR_MSB 0x06 +#define OPT4060_CLEAR_LSB 0x07 +#define OPT4060_THRESHOLD_LOW 0x08 +#define OPT4060_THRESHOLD_HIGH 0x09 +#define OPT4060_CTRL 0x0a +#define OPT4060_INT_CTRL 0x0b +#define OPT4060_RES_CTRL 0x0c +#define OPT4060_DEVICE_ID 0x11 + +/* OPT4060 register mask */ +#define OPT4060_EXPONENT_MASK GENMASK(15, 12) +#define OPT4060_MSB_MASK GENMASK(11, 0) +#define OPT4060_LSB_MASK GENMASK(15, 8) +#define OPT4060_COUNTER_MASK GENMASK(7, 4) +#define OPT4060_CRC_MASK GENMASK(3, 0) + +/* OPT4060 device id mask */ +#define OPT4060_DEVICE_ID_MASK GENMASK(11, 0) + +/* OPT4060 control register masks */ +#define OPT4060_CTRL_QWAKE_MASK BIT(15) +#define OPT4060_CTRL_RANGE_MASK GENMASK(13, 10) +#define OPT4060_CTRL_CONV_TIME_MASK GENMASK(9, 6) +#define OPT4060_CTRL_OPER_MODE_MASK GENMASK(5, 4) +#define OPT4060_CTRL_LATCH_MASK BIT(3) +#define OPT4060_CTRL_INT_POL_MASK BIT(2) +#define OPT4060_CTRL_FAULT_COUNT_MASK GENMASK(1, 0) + +/* OPT4060 interrupt control register masks */ +#define OPT4060_INT_CTRL_THRESH_SEL GENMASK(6, 5) +#define OPT4060_INT_CTRL_OUTPUT BIT(4) +#define OPT4060_INT_CTRL_INT_CFG GENMASK(3, 2) +#define OPT4060_INT_CTRL_THRESHOLD 0x0 +#define OPT4060_INT_CTRL_NEXT_CH 0x1 +#define OPT4060_INT_CTRL_ALL_CH 0x3 + +/* OPT4060 result control register masks */ +#define OPT4060_RES_CTRL_OVERLOAD BIT(3) +#define OPT4060_RES_CTRL_CONV_READY BIT(2) +#define OPT4060_RES_CTRL_FLAG_H BIT(1) +#define OPT4060_RES_CTRL_FLAG_L BIT(0) + +/* OPT4060 constants */ +#define OPT4060_DEVICE_ID_VAL 0x821 + +/* OPT4060 operating modes */ +#define OPT4060_CTRL_OPER_MODE_OFF 0x0 +#define OPT4060_CTRL_OPER_MODE_FORCED 0x1 +#define OPT4060_CTRL_OPER_MODE_ONE_SHOT 0x2 +#define OPT4060_CTRL_OPER_MODE_CONTINUOUS 0x3 + +/* OPT4060 conversion control register definitions */ +#define OPT4060_CTRL_CONVERSION_0_6MS 0x0 +#define OPT4060_CTRL_CONVERSION_1MS 0x1 +#define OPT4060_CTRL_CONVERSION_1_8MS 0x2 +#define OPT4060_CTRL_CONVERSION_3_4MS 0x3 +#define OPT4060_CTRL_CONVERSION_6_5MS 0x4 +#define OPT4060_CTRL_CONVERSION_12_7MS 0x5 +#define OPT4060_CTRL_CONVERSION_25MS 0x6 +#define OPT4060_CTRL_CONVERSION_50MS 0x7 +#define OPT4060_CTRL_CONVERSION_100MS 0x8 +#define OPT4060_CTRL_CONVERSION_200MS 0x9 +#define OPT4060_CTRL_CONVERSION_400MS 0xa +#define OPT4060_CTRL_CONVERSION_800MS 0xb + +/* OPT4060 fault count control register definitions */ +#define OPT4060_CTRL_FAULT_COUNT_1 0x0 +#define OPT4060_CTRL_FAULT_COUNT_2 0x1 +#define OPT4060_CTRL_FAULT_COUNT_4 0x2 +#define OPT4060_CTRL_FAULT_COUNT_8 0x3 + +/* OPT4060 scale light level range definitions */ +#define OPT4060_CTRL_LIGHT_SCALE_AUTO 12 + +/* OPT4060 default values */ +#define OPT4060_DEFAULT_CONVERSION_TIME OPT4060_CTRL_CONVERSION_50MS + +/* + * enum opt4060_chan_type - OPT4060 channel types + * @OPT4060_RED: Red channel. + * @OPT4060_GREEN: Green channel. + * @OPT4060_BLUE: Blue channel. + * @OPT4060_CLEAR: Clear (white) channel. + * @OPT4060_NUM_CHANS: Number of channel types. + */ +enum opt4060_chan_type { + OPT4060_RED, + OPT4060_GREEN, + OPT4060_BLUE, + OPT4060_CLEAR, + OPT4060_NUM_CHANS +}; + +struct opt4060_chip { + struct regmap *regmap; + struct device *dev; + struct iio_trigger *data_trig; + bool data_trigger_active; + struct iio_trigger *threshold_trig; + bool threshold_trigger_active; + u8 int_time; + int irq; + struct completion completion; + bool thresh_event_lo_active; + bool thresh_event_hi_active; +}; + +struct opt4060_channel_factor { + u32 mul; + u32 div; +}; + +struct opt4060_buffer { + u32 chan[OPT4060_NUM_CHANS]; + s64 ts; +}; + +static const struct opt4060_channel_factor opt4060_channel_factors[] = { + { + /* RED 2.4 * 2.15e-3 */ + .mul = 516, + .div = 100000, + }, { + /* GREEN 1.0 * 2.15e-3 */ + .mul = 215, + .div = 100000, + }, { + /* BLUE 1.3 * 2.15e-3 */ + .mul = 258, + .div = 100000, + }, { + /* CLEAR 1.0 * 2.15e-3 */ + .mul = 215, + .div = 100000, + }, +}; + +static const int opt4060_int_time_available[][2] = { + { 0, 600 }, + { 0, 1000 }, + { 0, 1800 }, + { 0, 3400 }, + { 0, 6500 }, + { 0, 12700 }, + { 0, 25000 }, + { 0, 50000 }, + { 0, 100000 }, + { 0, 200000 }, + { 0, 400000 }, + { 0, 800000 }, +}; + +/* + * Conversion time is integration time + time to set register + * this is used as integration time. + */ +static const int opt4060_int_time_reg[][2] = { + { 600, OPT4060_CTRL_CONVERSION_0_6MS }, + { 1000, OPT4060_CTRL_CONVERSION_1MS }, + { 1800, OPT4060_CTRL_CONVERSION_1_8MS }, + { 3400, OPT4060_CTRL_CONVERSION_3_4MS }, + { 6500, OPT4060_CTRL_CONVERSION_6_5MS }, + { 12700, OPT4060_CTRL_CONVERSION_12_7MS }, + { 25000, OPT4060_CTRL_CONVERSION_25MS }, + { 50000, OPT4060_CTRL_CONVERSION_50MS }, + { 100000, OPT4060_CTRL_CONVERSION_100MS }, + { 200000, OPT4060_CTRL_CONVERSION_200MS }, + { 400000, OPT4060_CTRL_CONVERSION_400MS }, + { 800000, OPT4060_CTRL_CONVERSION_800MS }, +}; + +static int opt4060_als_time_to_index(const u32 als_integration_time) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(opt4060_int_time_available); i++) { + if (als_integration_time == opt4060_int_time_available[i][1]) + return i; + } + + return -EINVAL; +} + +static u8 opt4060_calculate_crc(u8 exp, u32 mantissa, u8 count) +{ + u8 crc; + + crc = (hweight32(mantissa) + hweight32(exp) + hweight32(count)) % 2; + crc |= ((hweight32(mantissa & 0xAAAAA) + hweight32(exp & 0xA) + + hweight32(count & 0xA)) % 2) << 1; + crc |= ((hweight32(mantissa & 0x88888) + hweight32(exp & 0x8) + + hweight32(count & 0x8)) % 2) << 2; + crc |= (hweight32(mantissa & 0x80808) % 2) << 3; + + return crc; +} + +static int opt4060_set_int_state(struct opt4060_chip *chip, u32 state) +{ + int ret; + unsigned int regval; + + regval = FIELD_PREP(OPT4060_INT_CTRL_INT_CFG, state); + ret = regmap_update_bits(chip->regmap, OPT4060_INT_CTRL, + OPT4060_INT_CTRL_INT_CFG, regval); + if (ret) + dev_err(chip->dev, "Failed to set interrupt config\n"); + return ret; +} + +static int opt4060_trigger_one_shot(struct opt4060_chip *chip) +{ + int ret; + unsigned int ctrl_reg, oper_mode; + + /* Enable interrupt for conversion ready of all channels */ + ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_ALL_CH); + if (ret) + return ret; + + ret = regmap_read(chip->regmap, OPT4060_CTRL, &ctrl_reg); + if (ret < 0) { + dev_err(chip->dev, "Failed to read ctrl register\n"); + return ret; + } + + oper_mode = FIELD_GET(OPT4060_CTRL_OPER_MODE_MASK, ctrl_reg); + + /* If the device is already in continuous mode, one-shot is not needed. */ + if (oper_mode != OPT4060_CTRL_OPER_MODE_CONTINUOUS) { + ctrl_reg &= ~OPT4060_CTRL_OPER_MODE_MASK; + ctrl_reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK, + OPT4060_CTRL_OPER_MODE_ONE_SHOT); + + /* Trigger a new conversion by writing to CRTL register. */ + ret = regmap_write(chip->regmap, OPT4060_CTRL, ctrl_reg); + if (ret) + dev_err(chip->dev, "Failed to set ctrl register\n"); + } + return ret; +} + +static int opt4060_set_continous_mode(struct opt4060_chip *chip, + bool continuous) +{ + u16 reg; + int ret; + + if (continuous) + reg = FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK, + OPT4060_CTRL_OPER_MODE_CONTINUOUS); + else + reg = FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK, + OPT4060_CTRL_OPER_MODE_ONE_SHOT); + + ret = regmap_update_bits(chip->regmap, OPT4060_CTRL, + OPT4060_CTRL_OPER_MODE_MASK, reg); + if (ret) + dev_err(chip->dev, "Failed to set configuration\n"); + + return ret; +} + +static int opt4060_read_raw_value(struct opt4060_chip *chip, + unsigned long address, u32 *raw) +{ + int ret; + u32 result; + u16 msb; + u16 lsb; + u8 exp; + u8 count; + u8 crc; + u8 calc_crc; + u32 mantissa_raw; + + ret = regmap_bulk_read(chip->regmap, address, &result, 2); + if (ret) + dev_err(chip->dev, "Reading channel data failed\n"); + else { + count = FIELD_GET(OPT4060_COUNTER_MASK, result >> 16); + crc = FIELD_GET(OPT4060_CRC_MASK, result >> 16); + lsb = FIELD_GET(OPT4060_LSB_MASK, result >> 16); + exp = FIELD_GET(OPT4060_EXPONENT_MASK, result); + msb = FIELD_GET(OPT4060_MSB_MASK, result); + mantissa_raw = (msb << 8) + lsb; + calc_crc = opt4060_calculate_crc(exp, mantissa_raw, count); + if (calc_crc != crc) + return -EIO; + + *raw = mantissa_raw << exp; + } + return ret; +} + +static int opt4060_read_chan_value(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, bool processed) +{ + struct opt4060_chip *chip = iio_priv(indio_dev); + u32 adc_raw; + int ret; + /* Set timeout to two times the integration time multiplied by channel count. */ + unsigned int timeout_us = 2 * OPT4060_NUM_CHANS * + opt4060_int_time_reg[chip->int_time][0]; + + if (chip->irq) { + reinit_completion(&chip->completion); + ret = opt4060_trigger_one_shot(chip); + if (ret) + return ret; + if (wait_for_completion_timeout(&chip->completion, + usecs_to_jiffies(timeout_us)) == 0) + dev_info(chip->dev, "Completion timed out.\n"); + } + + ret = opt4060_read_raw_value(chip, chan->address, &adc_raw); + if (ret) { + dev_err(chip->dev, "Reading raw channel data failed\n"); + return ret; + } + if (processed) { + u32 lux_raw; + u32 rem; + u32 mul = opt4060_channel_factors[chan->scan_index].mul; + u32 div = opt4060_channel_factors[chan->scan_index].div; + + lux_raw = adc_raw * mul; + *val = div_u64_rem(lux_raw, div, &rem); + *val2 = rem * div_u64(div, 10); + return IIO_VAL_INT_PLUS_NANO; + } + *val = adc_raw; + *val2 = 0; + return IIO_VAL_INT; +} + +static int opt4060_set_conf(struct opt4060_chip *chip) +{ + u16 reg; + int ret; + + reg = FIELD_PREP(OPT4060_CTRL_RANGE_MASK, + OPT4060_CTRL_LIGHT_SCALE_AUTO); + reg |= FIELD_PREP(OPT4060_CTRL_CONV_TIME_MASK, chip->int_time); + if (chip->irq) + reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK, + OPT4060_CTRL_OPER_MODE_ONE_SHOT); + else + reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK, + OPT4060_CTRL_OPER_MODE_CONTINUOUS); + reg |= OPT4060_CTRL_QWAKE_MASK | OPT4060_CTRL_LATCH_MASK; + + ret = regmap_write(chip->regmap, OPT4060_CTRL, reg); + if (ret) + dev_err(chip->dev, "Failed to set configuration\n"); + + return ret; +} + +static int opt4060_power_down(struct opt4060_chip *chip) +{ + int ret; + unsigned int reg; + + ret = regmap_read(chip->regmap, OPT4060_CTRL, ®); + if (ret) { + dev_err(chip->dev, "Failed to read configuration\n"); + return ret; + } + + /* MODE_OFF is 0x0 so just set bits to 0 */ + reg &= ~OPT4060_CTRL_OPER_MODE_MASK; + + ret = regmap_write(chip->regmap, OPT4060_CTRL, reg); + if (ret) + dev_err(chip->dev, "Failed to power down\n"); + + return ret; +} + +static void opt4060_chip_off_action(void *data) +{ + struct opt4060_chip *chip = data; + + opt4060_power_down(chip); +} + +#define OPT4060_CHAN(color) \ +{ \ + .type = IIO_LIGHT, \ + .modified = 1, \ + .channel2 = IIO_MOD_LIGHT_##color, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | \ + BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), \ + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME), \ + .address = OPT4060_##color##_MSB, \ + .scan_index = OPT4060_##color, \ + .scan_type = { \ + .sign = 'u', \ + .realbits = 32, \ + .storagebits = 32, \ + .endianness = IIO_LE, \ + }, \ + .event_spec = opt4060_event_spec, \ + .num_event_specs = ARRAY_SIZE(opt4060_event_spec), \ +} + +static const struct iio_event_spec opt4060_event_spec[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + .mask_separate = BIT(IIO_EV_INFO_VALUE) | + BIT(IIO_EV_INFO_ENABLE), + }, { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_FALLING, + .mask_separate = BIT(IIO_EV_INFO_VALUE) | + BIT(IIO_EV_INFO_ENABLE), + }, { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_EITHER, + .mask_separate = BIT(IIO_EV_INFO_PERIOD), + }, +}; + +static const struct iio_chan_spec opt4060_channels[] = { + OPT4060_CHAN(RED), + OPT4060_CHAN(GREEN), + OPT4060_CHAN(BLUE), + OPT4060_CHAN(CLEAR), + IIO_CHAN_SOFT_TIMESTAMP(OPT4060_NUM_CHANS), +}; + +static int opt4060_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct opt4060_chip *chip = iio_priv(indio_dev); + + switch (mask) { + case IIO_CHAN_INFO_RAW: + return opt4060_read_chan_value(indio_dev, chan, val, val2, + false); + case IIO_CHAN_INFO_PROCESSED: + return opt4060_read_chan_value(indio_dev, chan, val, val2, + true); + case IIO_CHAN_INFO_INT_TIME: + *val = 0; + *val2 = opt4060_int_time_reg[chip->int_time][0]; + return IIO_VAL_INT_PLUS_MICRO; + default: + return -EINVAL; + } +} + +static int opt4060_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + struct opt4060_chip *chip = iio_priv(indio_dev); + int int_time; + + switch (mask) { + case IIO_CHAN_INFO_INT_TIME: + int_time = opt4060_als_time_to_index(val2); + if (int_time < 0) + return int_time; + chip->int_time = int_time; + return opt4060_set_conf(chip); + default: + return -EINVAL; + } +} + +static int opt4060_write_raw_get_fmt(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_INT_TIME: + return IIO_VAL_INT_PLUS_MICRO; + default: + return -EINVAL; + } +} + +static u32 opt4060_calc_th_reg(u32 adc_val) +{ + u32 th_val, th_exp; + u32 bits; + /* + * The threshold registers take 4 bits of exponent and 12 bits of data + * ADC = TH_VAL << (8 + TH_EXP) + */ + bits = fls(adc_val); + + if (bits > 31) + th_exp = 11; /* Maximum exponent */ + else if (bits > 20) + th_exp = bits - 20; + else + th_exp = 0; + th_val = (adc_val >> (8 + th_exp)) & 0xfff; + + return (th_exp << 12) + th_val; +} + +static u32 opt4060_calc_val_from_th_reg(u32 th_reg) +{ + /* + * The threshold registers take 4 bits of exponent and 12 bits of data + * ADC = TH_VAL << (8 + TH_EXP) + */ + u32 th_val, th_exp; + + th_exp = (th_reg >> 12) & 0xf; + th_val = th_reg & 0xfff; + + return th_val << (8 + th_exp); +} + +static bool opt4060_event_active(struct opt4060_chip *chip) +{ + return chip->thresh_event_lo_active || chip->thresh_event_hi_active; +} + +static int opt4060_read_available(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int **vals, int *type, int *length, + long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_INT_TIME: + *length = ARRAY_SIZE(opt4060_int_time_available) * 2; + *vals = (const int *)opt4060_int_time_available; + *type = IIO_VAL_INT_PLUS_MICRO; + return IIO_AVAIL_LIST; + + default: + return -EINVAL; + } +} + + +static int opt4060_event_set_state(struct opt4060_chip *chip, bool state) +{ + int ret = 0; + + if (state) + ret = opt4060_set_continous_mode(chip, true); + else if (!chip->data_trigger_active && chip->irq) + ret = opt4060_set_continous_mode(chip, false); + + return ret; +} + +static int opt4060_trigger_set_state(struct iio_trigger *trig, bool state) +{ + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); + struct opt4060_chip *chip = iio_priv(indio_dev); + int ret = 0; + + if (indio_dev->trig) { + iio_trigger_put(indio_dev->trig); + indio_dev->trig = NULL; + } + + if (state) { + indio_dev->trig = iio_trigger_get(trig); + ret = opt4060_set_continous_mode(chip, true); + } else if (!opt4060_event_active(chip) && chip->irq) + ret = opt4060_set_continous_mode(chip, false); + + if (trig == chip->data_trig) { + chip->data_trigger_active = state; + chip->threshold_trigger_active = !state; + } else if (trig == chip->threshold_trig) { + chip->data_trigger_active = !state; + chip->threshold_trigger_active = state; + } + + return ret; +} + +static ssize_t opt4060_read_ev_period(struct opt4060_chip *chip, int *val, + int *val2) +{ + int ret, pers, fault_count, int_time; + u64 uval; + + int_time = opt4060_int_time_reg[chip->int_time][0]; + + ret = regmap_read(chip->regmap, OPT4060_CTRL, &fault_count); + if (ret < 0) + return ret; + + fault_count = fault_count & OPT4060_CTRL_FAULT_COUNT_MASK; + switch (fault_count) { + case OPT4060_CTRL_FAULT_COUNT_2: + pers = 2; + break; + case OPT4060_CTRL_FAULT_COUNT_4: + pers = 4; + break; + case OPT4060_CTRL_FAULT_COUNT_8: + pers = 8; + break; + + default: + pers = 1; + } + + uval = mul_u32_u32(int_time, pers); + *val = div_u64_rem(uval, MICRO, val2); + + return IIO_VAL_INT_PLUS_MICRO; +} + +static ssize_t opt4060_write_ev_period(struct opt4060_chip *chip, int val, + int val2) +{ + u64 uval, int_time; + unsigned int regval, fault_count_val; + + uval = mul_u32_u32(val, MICRO) + val2; + int_time = opt4060_int_time_reg[chip->int_time][0]; + + /* Check if the period is closest to 1, 2, 4 or 8 times integration time.*/ + if (uval <= int_time) + fault_count_val = OPT4060_CTRL_FAULT_COUNT_1; + else if (uval <= int_time * 2) + fault_count_val = OPT4060_CTRL_FAULT_COUNT_2; + else if (uval <= int_time * 4) + fault_count_val = OPT4060_CTRL_FAULT_COUNT_4; + else + fault_count_val = OPT4060_CTRL_FAULT_COUNT_8; + + regval = FIELD_PREP(OPT4060_CTRL_FAULT_COUNT_MASK, fault_count_val); + return regmap_update_bits(chip->regmap, OPT4060_CTRL, + OPT4060_CTRL_FAULT_COUNT_MASK, regval); +} + +static int opt4060_get_channel_sel(struct opt4060_chip *chip, int *ch_sel) +{ + int ret; + u32 regval; + + ret = regmap_read(chip->regmap, OPT4060_INT_CTRL, ®val); + if (ret) + dev_err(chip->dev, "Failed to get channel selection.\n"); + else + *ch_sel = FIELD_GET(OPT4060_INT_CTRL_THRESH_SEL, regval); + return ret; +} + +static int opt4060_set_channel_sel(struct opt4060_chip *chip, int ch_sel) +{ + int ret; + u32 regval; + + regval = FIELD_PREP(OPT4060_INT_CTRL_THRESH_SEL, ch_sel); + ret = regmap_update_bits(chip->regmap, OPT4060_INT_CTRL, + OPT4060_INT_CTRL_THRESH_SEL, regval); + if (ret) + dev_err(chip->dev, "Failed to set channel selection.\n"); + return ret; +} + +static int opt4060_get_thresholds(struct opt4060_chip *chip, u32 *th_lo, u32 *th_hi) +{ + int ret; + u32 regval; + + ret = regmap_read(chip->regmap, OPT4060_THRESHOLD_LOW, ®val); + if (ret) { + dev_err(chip->dev, "Failed to read THRESHOLD_LOW.\n"); + return ret; + } + *th_lo = opt4060_calc_val_from_th_reg(regval); + + ret = regmap_read(chip->regmap, OPT4060_THRESHOLD_HIGH, ®val); + if (ret) { + dev_err(chip->dev, "Failed to read THRESHOLD_LOW.\n"); + return ret; + } + *th_hi = opt4060_calc_val_from_th_reg(regval); + + return ret; +} + +static int opt4060_set_thresholds(struct opt4060_chip *chip, u32 th_lo, u32 th_hi) +{ + int ret; + + ret = regmap_write(chip->regmap, OPT4060_THRESHOLD_LOW, opt4060_calc_th_reg(th_lo)); + if (ret) { + dev_err(chip->dev, "Failed to write THRESHOLD_LOW.\n"); + return ret; + } + + ret = regmap_write(chip->regmap, OPT4060_THRESHOLD_HIGH, opt4060_calc_th_reg(th_hi)); + if (ret) + dev_err(chip->dev, "Failed to write THRESHOLD_HIGH.\n"); + + return ret; +} + +static int opt4060_read_event(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, + int *val, int *val2) +{ + int ret = -EINVAL; + struct opt4060_chip *chip = iio_priv(indio_dev); + + switch (info) { + case IIO_EV_INFO_VALUE: + if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) { + u32 th_lo, th_hi; + + if (opt4060_get_thresholds(chip, &th_lo, &th_hi)) + return -EFAULT; + if (dir == IIO_EV_DIR_FALLING) { + *val = th_lo; + ret = IIO_VAL_INT; + } else if (dir == IIO_EV_DIR_RISING) { + *val = th_hi; + ret = IIO_VAL_INT; + } + } + break; + case IIO_EV_INFO_PERIOD: + return opt4060_read_ev_period(chip, val, val2); + default: + return -EINVAL; + } + return ret; +} + +static int opt4060_write_event(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, + int val, int val2) +{ + int ret = -EINVAL; + struct opt4060_chip *chip = iio_priv(indio_dev); + + switch (info) { + case IIO_EV_INFO_VALUE: + if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) { + u32 th_lo, th_hi; + + if (opt4060_get_thresholds(chip, &th_lo, &th_hi)) + return -EFAULT; + if (dir == IIO_EV_DIR_FALLING) + th_lo = val; + else if (dir == IIO_EV_DIR_RISING) + th_hi = val; + if (opt4060_set_thresholds(chip, th_lo, th_hi)) + return -EFAULT; + ret = 0; + } + break; + case IIO_EV_INFO_PERIOD: + return opt4060_write_ev_period(chip, val, val2); + default: + return -EINVAL; + } + return ret; +} + +static int opt4060_read_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir) +{ + int ret = -EINVAL; + struct opt4060_chip *chip = iio_priv(indio_dev); + + if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) { + int ch_idx = chan->scan_index; + int ch_sel; + + if (opt4060_get_channel_sel(chip, &ch_sel)) + return -EFAULT; + + if (((dir == IIO_EV_DIR_FALLING) && chip->thresh_event_lo_active) || + ((dir == IIO_EV_DIR_RISING) && chip->thresh_event_hi_active)) + ret = (ch_sel == ch_idx); + else + ret = FALSE; + } + return ret; +} + +static int opt4060_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, int state) +{ + int ret = -EINVAL; + struct opt4060_chip *chip = iio_priv(indio_dev); + + if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) { + int ch_idx = chan->scan_index; + int ch_sel; + + if (opt4060_get_channel_sel(chip, &ch_sel)) + return -EFAULT; + + if (state) { + /* Only one channel can be active at the same time */ + if ((chip->thresh_event_lo_active || + chip->thresh_event_hi_active) && (ch_idx != ch_sel)) + return -EBUSY; + if (dir == IIO_EV_DIR_FALLING) + chip->thresh_event_lo_active = TRUE; + else if (dir == IIO_EV_DIR_RISING) + chip->thresh_event_hi_active = TRUE; + if (opt4060_set_channel_sel(chip, ch_idx)) + return -EFAULT; + ret = 0; + } else { + if (ch_idx == ch_sel) { + if (dir == IIO_EV_DIR_FALLING) + chip->thresh_event_lo_active = FALSE; + else if (dir == IIO_EV_DIR_RISING) + chip->thresh_event_hi_active = FALSE; + } + ret = 0; + } + + if (opt4060_event_set_state(chip, chip->thresh_event_hi_active | + chip->thresh_event_lo_active)) + return -EFAULT; + } + return ret; +} + +static const struct iio_info opt4060_info = { + .read_raw = opt4060_read_raw, + .write_raw = opt4060_write_raw, + .write_raw_get_fmt = opt4060_write_raw_get_fmt, + .read_avail = opt4060_read_available, + .read_event_value = opt4060_read_event, + .write_event_value = opt4060_write_event, + .read_event_config = opt4060_read_event_config, + .write_event_config = opt4060_write_event_config, +}; + +static int opt4060_load_defaults(struct opt4060_chip *chip) +{ + int ret; + + chip->int_time = OPT4060_DEFAULT_CONVERSION_TIME; + + /* Set initial MIN/MAX thresholds */ + ret = opt4060_set_thresholds(chip, 0, UINT_MAX); + if (ret) + return ret; + + return opt4060_set_conf(chip); +} + +static bool opt4060_volatile_reg(struct device *dev, unsigned int reg) +{ + if ((reg >= OPT4060_RED_MSB && reg <= OPT4060_CLEAR_LSB) || + reg == OPT4060_RES_CTRL) + return true; + return false; +} + +static bool opt4060_writable_reg(struct device *dev, unsigned int reg) +{ + if (reg >= OPT4060_THRESHOLD_LOW || reg >= OPT4060_INT_CTRL) + return true; + return false; +} + +static bool opt4060_readonly_reg(struct device *dev, unsigned int reg) +{ + if (reg == OPT4060_DEVICE_ID) + return true; + return false; +} + +static bool opt4060_readable_reg(struct device *dev, unsigned int reg) +{ + /* Volatile, writable and read-only registers are readable. */ + if (opt4060_volatile_reg(dev, reg) || opt4060_writable_reg(dev, reg) || + opt4060_readonly_reg(dev, reg)) + return TRUE; + return false; +} + +static const struct regmap_config opt4060_regmap_config = { + .name = OPT_4060_DRV_NAME, + .reg_bits = 8, + .val_bits = 16, + .cache_type = REGCACHE_RBTREE, + .max_register = OPT4060_DEVICE_ID, + .readable_reg = opt4060_readable_reg, + .writeable_reg = opt4060_writable_reg, + .volatile_reg = opt4060_volatile_reg, + .val_format_endian = REGMAP_ENDIAN_BIG, +}; + +static const struct iio_trigger_ops opt4060_trigger_ops = { + .validate_device = iio_trigger_validate_own_device, + .set_trigger_state = opt4060_trigger_set_state, +}; + +static irqreturn_t opt4060_trigger_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *idev = pf->indio_dev; + struct opt4060_chip *chip = iio_priv(idev); + struct opt4060_buffer raw; + int ret, chan; + int i = 0; + + memset(&raw, 0, sizeof(raw)); + + for_each_set_bit(chan, idev->active_scan_mask, idev->masklength) { + ret = opt4060_read_raw_value(chip, + opt4060_channels[chan].address, + &raw.chan[i++]); + if (ret) { + dev_err(chip->dev, "Reading raw channel data failed\n"); + goto err_read; + } + } + + iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp); +err_read: + iio_trigger_notify_done(idev->trig); + return IRQ_HANDLED; +} + +static int opt4060_buffer_preenable(struct iio_dev *idev) +{ + struct opt4060_chip *chip = iio_priv(idev); + + return opt4060_trigger_one_shot(chip); +} + +static const struct iio_buffer_setup_ops opt4060_buffer_ops = { + .preenable = opt4060_buffer_preenable, +}; + +static irqreturn_t opt4060_irq_thread(int irq, void *private) +{ + struct iio_dev *idev = private; + struct opt4060_chip *chip = iio_priv(idev); + int ret, chan, dummy; + unsigned int int_res; + + ret = regmap_read(chip->regmap, OPT4060_RES_CTRL, &int_res); + if (ret < 0) { + dev_err(chip->dev, "Failed to read interrupt reasons.\n"); + return IRQ_NONE; + } + + if (!chip->data_trigger_active) { + /* Only trigger interrupts on thresholds */ + ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_THRESHOLD); + if (ret) { + dev_err(chip->dev, "Failed to set interrupt state\n"); + return IRQ_NONE; + } + } + + /* Read OPT4060_CTRL to clear interrupt */ + ret = regmap_read(chip->regmap, OPT4060_CTRL, &dummy); + if (ret < 0) { + dev_err(chip->dev, "Failed to clear interrupt\n"); + return IRQ_NONE; + } + + /* Handle events */ + if (int_res & (OPT4060_RES_CTRL_FLAG_H | OPT4060_RES_CTRL_FLAG_L)) { + u64 code; + + /* Check if the interrupt is from the lower threshold */ + if (int_res & OPT4060_RES_CTRL_FLAG_L) { + code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT, + chan, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_FALLING); + iio_push_event(idev, code, + iio_get_time_ns(idev)); + } + /* Check if the interrupt is from the upper threshold */ + if (int_res & OPT4060_RES_CTRL_FLAG_H) { + code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT, + chan, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_RISING); + iio_push_event(idev, code, + iio_get_time_ns(idev)); + } + /* Handle threshold triggers */ + if (chip->threshold_trigger_active && iio_buffer_enabled(idev)) + iio_trigger_poll_nested(chip->threshold_trig); + } + + /* Handle conversion ready */ + if (int_res & OPT4060_RES_CTRL_CONV_READY) { + /* Signal completion for potentially waiting reads */ + complete(&chip->completion); + + /* Handle data ready triggers */ + if (chip->data_trigger_active && iio_buffer_enabled(idev)) + iio_trigger_poll_nested(chip->data_trig); + } + return IRQ_HANDLED; +} + +static int opt4060_setup_triggers(struct opt4060_chip *chip, struct iio_dev *idev) +{ + struct iio_trigger *data_trigger; + struct iio_trigger *threshold_trigger; + char *name; + int ret; + + ret = devm_iio_triggered_buffer_setup(chip->dev, idev, + &iio_pollfunc_store_time, + opt4060_trigger_handler, + &opt4060_buffer_ops); + if (ret) + return dev_err_probe(chip->dev, ret, + "iio_triggered_buffer_setup_ext FAIL\n"); + + data_trigger = devm_iio_trigger_alloc(chip->dev, "%s-data-ready-dev%d", + idev->name, iio_device_id(idev)); + if (!data_trigger) + return -ENOMEM; + chip->data_trig = data_trigger; + data_trigger->ops = &opt4060_trigger_ops; + iio_trigger_set_drvdata(data_trigger, idev); + ret = devm_iio_trigger_register(chip->dev, data_trigger); + if (ret) + return dev_err_probe(chip->dev, ret, + "Data ready trigger registration failed\n"); + + threshold_trigger = devm_iio_trigger_alloc(chip->dev, "%s-threshold-dev%d", + idev->name, iio_device_id(idev)); + if (!threshold_trigger) + return -ENOMEM; + chip->threshold_trig = threshold_trigger; + threshold_trigger->ops = &opt4060_trigger_ops; + iio_trigger_set_drvdata(threshold_trigger, idev); + ret = devm_iio_trigger_register(chip->dev, threshold_trigger); + if (ret) + return dev_err_probe(chip->dev, ret, + "Threshold trigger registration failed\n"); + + + name = devm_kasprintf(chip->dev, GFP_KERNEL, "%s-opt4060", + dev_name(chip->dev)); + + ret = devm_request_threaded_irq(chip->dev, chip->irq, NULL, opt4060_irq_thread, + IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | + IRQF_ONESHOT, name, idev); + if (ret) + return dev_err_probe(chip->dev, ret, "Could not request IRQ\n"); + + ret = regmap_write_bits(chip->regmap, OPT4060_INT_CTRL, + OPT4060_INT_CTRL_OUTPUT, + OPT4060_INT_CTRL_OUTPUT); + if (ret) + return dev_err_probe(chip->dev, ret, + "Failed to set interrupt as output\n"); + + return 0; +} + +static int opt4060_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct opt4060_chip *chip; + struct iio_dev *indio_dev; + int ret; + uint dev_id; + + indio_dev = devm_iio_device_alloc(dev, sizeof(*chip)); + if (!indio_dev) + return -ENOMEM; + + chip = iio_priv(indio_dev); + + /* Request regulator */ + ret = devm_regulator_get_enable(dev, "vdd"); + if (ret) + return dev_err_probe(dev, ret, "Failed to enable vdd supply\n"); + + chip->regmap = devm_regmap_init_i2c(client, &opt4060_regmap_config); + if (IS_ERR(chip->regmap)) + return dev_err_probe(dev, PTR_ERR(chip->regmap), + "regmap initialization failed\n"); + + chip->dev = dev; + chip->irq = client->irq; + init_completion(&chip->completion); + + indio_dev->info = &opt4060_info; + + ret = regmap_reinit_cache(chip->regmap, &opt4060_regmap_config); + if (ret) + return dev_err_probe(dev, ret, + "failed to reinit regmap cache\n"); + + ret = regmap_read(chip->regmap, OPT4060_DEVICE_ID, &dev_id); + if (ret < 0) + return dev_err_probe(dev, ret, + "Failed to read the device ID register\n"); + + dev_id = FIELD_GET(OPT4060_DEVICE_ID_MASK, dev_id); + if (dev_id != OPT4060_DEVICE_ID_VAL) + dev_warn(dev, "Device ID: %#04x unknown\n", dev_id); + + indio_dev->channels = opt4060_channels; + indio_dev->num_channels = ARRAY_SIZE(opt4060_channels); + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->name = OPT_4060_DRV_NAME; + + /* Initialize device with default settings */ + ret = opt4060_load_defaults(chip); + if (ret < 0) + return dev_err_probe(dev, ret, + "Failed to set sensor defaults\n"); + + /* If irq is enabled, initialize triggers */ + if (chip->irq) { + ret = opt4060_setup_triggers(chip, indio_dev); + if (ret) + return ret; + } else { + dev_info(chip->dev, "No IRQ, events and triggers disabled\n"); + } + + ret = devm_add_action_or_reset(dev, opt4060_chip_off_action, chip); + if (ret < 0) + return dev_err_probe(dev, ret, + "Failed to setup power off action\n"); + + return devm_iio_device_register(dev, indio_dev); +} + +/* + * The compatible string determines which constants to use depending on + * opt4060 packaging + */ +static const struct i2c_device_id opt4060_id[] = { + { OPT_4060_DRV_NAME, 0 }, + {} +}; +MODULE_DEVICE_TABLE(i2c, opt4060_id); + +static const struct of_device_id opt4060_of_match[] = { + { .compatible = "ti,opt4060" }, + {} +}; +MODULE_DEVICE_TABLE(of, opt4060_of_match); + +static struct i2c_driver opt4060_driver = { + .driver = { + .name = OPT_4060_DRV_NAME, + .of_match_table = opt4060_of_match, + }, + .probe = opt4060_probe, + .id_table = opt4060_id, +}; +module_i2c_driver(opt4060_driver); + +MODULE_DESCRIPTION("Texas Instruments opt4060 RGBW color sensor driver"); +MODULE_LICENSE("GPL");
Add support for Texas Instruments OPT4060 RGBW Color sensor. Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.com> --- drivers/iio/light/Kconfig | 13 + drivers/iio/light/Makefile | 1 + drivers/iio/light/opt4060.c | 1216 +++++++++++++++++++++++++++++++++++ 3 files changed, 1230 insertions(+) create mode 100644 drivers/iio/light/opt4060.c