On 2/1/26 8:45 PM, David Lechner wrote:
> On 2/1/26 11:03 AM, Erikas Bitovtas wrote:
>> This driver adds the initial support for the Capella cm36686
>> ambient light and proximity sensor and cm36672p proximity sensor.
>>
>> Signed-off-by: Erikas Bitovtas <[email protected]>
>> ---
>> drivers/iio/light/Kconfig | 11 +
>> drivers/iio/light/Makefile | 1 +
>> drivers/iio/light/cm36686.c | 810
>> ++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 822 insertions(+)
>>
>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>> index ac1408d374c9..b1d1943dec33 100644
>> --- a/drivers/iio/light/Kconfig
>> +++ b/drivers/iio/light/Kconfig
>> @@ -220,6 +220,17 @@ config CM36651
>> To compile this driver as a module, choose M here:
>> the module will be called cm36651.
>>
>> +config CM36686
>> + depends on I2C
>> + tristate "CM36686 driver"
>> + help
>> + Say Y here if you use cm36686.
>> + This option enables ambient light & proximity sensor using
>> + Capella cm36686 device driver.
>> +
>> + To compile this driver as a module, choose M here:
>> + the module will be called cm36686.
>> +
>> config IIO_CROS_EC_LIGHT_PROX
>> tristate "ChromeOS EC Light and Proximity Sensors"
>> depends on IIO_CROS_EC_SENSORS_CORE
>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>> index c0048e0d5ca8..806df80f6454 100644
>> --- a/drivers/iio/light/Makefile
>> +++ b/drivers/iio/light/Makefile
>> @@ -23,6 +23,7 @@ obj-$(CONFIG_CM3232) += cm3232.o
>> obj-$(CONFIG_CM3323) += cm3323.o
>> obj-$(CONFIG_CM3605) += cm3605.o
>> obj-$(CONFIG_CM36651) += cm36651.o
>> +obj-$(CONFIG_CM36686) += cm36686.o
>> obj-$(CONFIG_IIO_CROS_EC_LIGHT_PROX) += cros_ec_light_prox.o
>> obj-$(CONFIG_GP2AP002) += gp2ap002.o
>> obj-$(CONFIG_GP2AP020A00F) += gp2ap020a00f.o
>> diff --git a/drivers/iio/light/cm36686.c b/drivers/iio/light/cm36686.c
>> new file mode 100644
>> index 000000000000..eb108af7226d
>> --- /dev/null
>> +++ b/drivers/iio/light/cm36686.c
>> @@ -0,0 +1,810 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>
> I would expect copyright statements to be preserved here since the cover
> letter mentions much of this was derived from existing code.
>
>> +
>> +#include <asm-generic/errno-base.h>
>
> Should not be using generic, it may be different from arch-specific.
>
> Generally we #include <linux/err.h> instead.
>
>> +#include <linux/array_size.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/device/devres.h>
>> +#include <linux/iio/types.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/property.h>
>> +#include <linux/i2c.h>
>
> Alphabetial order please.
>
>> +#include <linux/module.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/events.h>
>> +#include <linux/interrupt.h>
>
> linux/iio/*.h can be grouped seperately.
>
>> +
>> +/* Device registers */
>> +#define CM36686_REG_ALS_CONF 0x00
>> +#define CM36686_REG_PS_CONF1 0x03
>> +#define CM36686_REG_PS_CONF3 0x04
>> +#define CM36686_REG_PS_THDL 0x06
>> +#define CM36686_REG_PS_THDH 0x07
>> +#define CM36686_REG_PS_DATA 0x08
>> +#define CM36686_REG_ALS_DATA 0x09
>> +#define CM36686_REG_INT_FLAG 0x0B
>> +#define CM36686_REG_ID_FLAG 0x0C
>> +
>> +/* ALS_CONF */
>> +#define CM36686_ALS_IT GENMASK(7, 6)
>> +#define CM36686_ALS_GAIN GENMASK(3, 2)
>> +#define CM36686_ALS_INT_EN BIT(1)
>> +#define CM36686_ALS_SD BIT(0)
>> +
>> +/* PS_CONF1 */
>> +#define CM36686_PS_DR GENMASK(7, 6)
>> +#define CM36686_PS_PERS GENMASK(5, 4)
>> +#define CM36686_PS_IT GENMASK(3, 1)
>> +#define CM36686_PS_SD BIT(0)
>> +
>> +#define CM36686_PS_INT_OUT BIT(9)
>> +#define CM36686_PS_INT_IN BIT(8)
>> +
>> +/* PS_CONF3 */
>> +#define CM36686_PS_SMART_PERS_ENABLE BIT(4)
>> +
>> +#define CM36686_LED_I GENMASK(10, 8)
>> +
>> +/* INT_FLAG */
>> +#define CM36686_PS_IF GENMASK(9, 8)
>> +
>> +/* Default values */
>> +#define CM36686_ALS_ENABLE 0x00
>> +#define CM36686_PS_DR_1_320 FIELD_PREP(CM36686_PS_DR, 3)
>> +#define CM36686_PS_PERS_2 FIELD_PREP(CM36686_PS_PERS, 1)
>> +#define CM36686_PS_IT_2_5T FIELD_PREP(CM36686_PS_IT, 3)
>> +#define CM36686_LED_I_100 FIELD_PREP(CM36686_LED_I, 2)
>> +
>> +/* Shifts */
>> +#define CM36686_INT_FLAG_SHIFT 8
>> +
>> +/* Max proximity thresholds */
>> +#define CM36686_MAX_PS_VALUE (BIT(12) - 1)
>> +
>> +#define CM36686_DEVICE_ID 0x86
>> +
>> +enum {
>> + CM36686,
>> + CM36672P,
>> +};
>
> We try to avoid an ID enum in new drivers.
>
>> +
>> +enum cm36686_distance {
>> + CM36686_AWAY = 1,
>> + CM36686_CLOSE,
>> + CM36686_BOTH
>> +};
>> +
>> +enum {
>> + CM36686_PS_CONF1,
>> + CM36686_PS_CONF3,
>> + CM36686_PS_CONF_NUM
>> +};
>> +
>> +enum {
>> + CM36686_SUPPLY_VDD,
>> + CM36686_SUPPLY_VDDIO,
>> + CM36686_SUPPLY_VLED,
>> + CM36686_SUPPLY_NUM,
>> +};
>> +
>> +static const int cm36686_als_it_times[][2] = {
>> + {0, 80000},
>> + {0, 160000},
>> + {0, 320000},
>> + {0, 640000}
>> +};
>
> We try to keep a consistent style of spaces inside of braces and
> trailing common on the last item (unless it is a sentinl value).
>
> { 0, 640000 },
>
>> +
>> +static const int cm36686_ps_it_times[][2] = {
>> + {0, 320},
>> + {0, 480},
>> + {0, 640},
>> + {0, 800},
>> + {0, 960},
>> + {0, 1120},
>> + {0, 1280},
>> + {0, 2560}
>> +};
>> +
>> +static const int cm36686_ps_led_current[] = {
>
> We try to always include the units in the identifier name.
> Is this milliamps or microamps?
Original submission for cm36672p says milliamps. In the device tree I wrote
microamps. My mistake, will fix in v2.
>
>> + 50,
>> + 75,
>> + 100,
>> + 120,
>> + 140,
>> + 160,
>> + 180,
>> + 200
>> +};
>> +
>> +struct cm36686_data {
>> + struct mutex lock;
>
> Locks need to always have a comment explaining what they are protecting.
>
>> + struct i2c_client *client;
>> + struct regulator_bulk_data supplies[CM36686_SUPPLY_NUM];
>> + int als_conf;
>> + int ps_conf[CM36686_PS_CONF_NUM];
>> + int ps_close;
>> + int ps_away;
>> +};
>> +
>> +struct cm36686_chip_info {
>> + const char *name;
>> + const struct iio_chan_spec *channels;
>> + const int num_channels;
>> +};
>> +
>> +static int cm36686_current_to_index(int led_current)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(cm36686_ps_led_current); i++)
>> + if (led_current < cm36686_ps_led_current[i])
>> + break;
>> +
>> + return i > 0 ? i - 1 : -EINVAL;
>
> Not sure I follow the logic here. Shouldn't this just `return i;`
> instead of `break;` and then unconditnionally `return -EINVAL;` at
> the end?
>
> If not, it could use some comments to explain.
>
>> +}
>> +
>> +static ssize_t cm36686_read_near_level(struct iio_dev *indio_dev,
>> + uintptr_t priv,
>> + const struct iio_chan_spec *chan,
>> + char *buf)
>> +{
>> + struct cm36686_data *chip = iio_priv(indio_dev);
>> +
>> + return sysfs_emit(buf, "%u\n", chip->ps_close);
>> +}
>> +
>> +static const struct iio_chan_spec_ext_info cm36686_ext_info[] = {
>> + {
>> + .name = "nearlevel",
>> + .shared = IIO_SEPARATE,
>> + .read = cm36686_read_near_level,
>> + },
>> + {}
>
> IIO style for sentinil values is `{ }` (with space in between).
>
>> +};
>> +
>> +static const struct iio_event_spec cm36686_proximity_event_spec[] = {
>> + {
>> + .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_RISING,
>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
>> + BIT(IIO_EV_INFO_ENABLE),
>> + }
>
> All of these should have trailing comma.
>
>> +};
>> +
>> +static const struct iio_chan_spec cm36686_channels[] = {
>> + {
>> + .type = IIO_LIGHT,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>
> IIO_LIGHT should have IIO_CHAN_INFO_SCALE to convert raw to convert the
> raw value to lux. If we don't have that info due to lack of documentation,
> then we should add a comment to explain that. Or maybe we can make an
> educated guess?
No documentation on this is available, but we know that ASUS in their driver for
cm36686 use 160ms integration time for light and, after applying their custom
calibration functions, divide received raw value by 25 if the device is cm36686,
or by 20 if it's cm36283. Maybe from there we can extrapolate the lux value like
this:
80ms -> raw / 12.5
160ms -> raw / 25
320ms -> raw / 50
640ms -> raw / 100
Not sure how accurate that would be, however.
Xiaomi gets IIO_CHAN_INFO_SCALE by multiplying a value retrieved from a device
tree property called "als_trans_ratio", which is not documented anywhere.
>
>> + BIT(IIO_CHAN_INFO_INT_TIME),
>> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
>> + .address = CM36686_REG_ALS_DATA,
>> + },
>> + {
>> + .type = IIO_PROXIMITY,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_INT_TIME),
>> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
>> + .address = CM36686_REG_PS_DATA,
>> + .event_spec = cm36686_proximity_event_spec,
>> + .num_event_specs = ARRAY_SIZE(cm36686_proximity_event_spec),
>> + .ext_info = cm36686_ext_info
>> + }
>> +};
>> +
>> +static const struct iio_chan_spec cm36672p_channels[] = {
>> + {
>> + .type = IIO_PROXIMITY,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_INT_TIME),
>> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
>> + .address = CM36686_REG_PS_DATA,
>> + .event_spec = cm36686_proximity_event_spec,
>> + .num_event_specs = ARRAY_SIZE(cm36686_proximity_event_spec),
>> + .ext_info = cm36686_ext_info
>> + }
>> +};
>> +
>> +static int cm36686_read_avail(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + const int **vals, int *type, int *length,
>> + long mask)
>> +{
>> + if (mask != IIO_CHAN_INFO_INT_TIME)
>> + return -EINVAL;
>> +
>> + switch (chan->type) {
>> + case IIO_LIGHT:
>> + *vals = (int *)(cm36686_als_it_times);
>> + *length = 2 * ARRAY_SIZE(cm36686_als_it_times);
>> + *type = IIO_VAL_INT_PLUS_MICRO;
>> + return IIO_AVAIL_LIST;
>> + case IIO_PROXIMITY:
>> + *vals = (int *)(cm36686_ps_it_times);
>> + *length = 2 * ARRAY_SIZE(cm36686_ps_it_times);
>> + *type = IIO_VAL_INT_PLUS_MICRO;
>> + return IIO_AVAIL_LIST;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static int cm36686_read_channel(struct cm36686_data *chip,
>> + struct iio_chan_spec const *chan, int *val)
>> +{
>> + struct i2c_client *client = chip->client;
>> + int ret = IIO_VAL_INT;
>> +
>> + int data = i2c_smbus_read_word_data(client, chan->address);
>> +
>> + if (data < 0) {
>> + dev_err(&client->dev, "Failed to read register: %pe",
>> ERR_PTR(data));
>
> The error is passed to userspace, so this error message doesn't
> seem useful (could end up flooding logs if userspace keeps reading).
>
>> + ret = -EIO;
>
> Can just return directly here and avoid the local variable.
>
>> + } else {
>> + *val = data;
>> + }
>> + return ret;
>> +}
>> +
>> +static int cm36686_read_int_time(struct cm36686_data *chip,
>> + struct iio_chan_spec const *chan, int *val,
>> + int *val2)
>> +{
>> + int als_it_index, ps_it_index;
>> +
>> + switch (chan->type) {
>> + case IIO_LIGHT:
>> + als_it_index = FIELD_GET(CM36686_ALS_IT, chip->als_conf);
>> + *val = cm36686_als_it_times[als_it_index][0];
>> + *val2 = cm36686_als_it_times[als_it_index][1];
>> + return IIO_VAL_INT_PLUS_MICRO;
>> + case IIO_PROXIMITY:
>> + ps_it_index = FIELD_GET(CM36686_PS_IT,
>> + chip->ps_conf[CM36686_PS_CONF1]);
>> + *val = cm36686_ps_it_times[ps_it_index][0];
>> + *val2 = cm36686_ps_it_times[ps_it_index][1];
>> + return IIO_VAL_INT_PLUS_MICRO;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static int cm36686_write_light_int_time(struct cm36686_data *chip, int val2)
>> +{
>> + struct i2c_client *client = chip->client;
>> + int index = -1, ret, new_it_time;
>> +
>> + for (int i = 0; i < ARRAY_SIZE(cm36686_als_it_times); i++) {
>> + if (cm36686_als_it_times[i][1] == val2) {
>> + index = i;
>> + break;
>> + }
>> + }
>> +
>> + if (index == -1)
>> + return -EINVAL;
>> +
>> + new_it_time = chip->als_conf & ~CM36686_ALS_IT;
>> + new_it_time |= FIELD_PREP(CM36686_ALS_IT, index);
>
> Can use FIELD_MODIFY()?
>
>> +
>> + ret = i2c_smbus_write_word_data(chip->client, CM36686_REG_ALS_CONF,
>> + new_it_time);
>> + if (ret < 0)
>> + dev_err(&client->dev,
>> + "Failed to set ALS integration time: %pe",
>> ERR_PTR(ret));
>
> This error message can go too.
>
>> + else
>> + chip->als_conf = new_it_time;
>> +
>> + return ret;
>> +}
>> +
>> +static int cm36686_write_prox_int_time(struct cm36686_data *chip, int val2)
>> +{
>> + struct i2c_client *client = chip->client;
>> + int index = -1, ret, new_it_time;
>> +
>> + for (int i = 0; i < ARRAY_SIZE(cm36686_ps_it_times); i++) {
>> + if (cm36686_ps_it_times[i][1] == val2) {
>> + index = i;
>> + break;
>> + }
>> + }
>> +
>> + if (index == -1)
>> + return -EINVAL;
>> +
>> + new_it_time = chip->ps_conf[CM36686_PS_CONF1] & ~CM36686_PS_IT;
>> + new_it_time |= FIELD_PREP(CM36686_PS_IT, index);
>> +
>> + ret = i2c_smbus_write_word_data(chip->client, CM36686_REG_PS_CONF1,
>> + new_it_time);
>> + if (ret < 0)
>> + dev_err(&client->dev, "Failed to set PS integration time: %pe",
>> + ERR_PTR(ret));
>> + else
>> + chip->ps_conf[CM36686_PS_CONF1] = new_it_time;
>> +
>> + return ret;
>> +}
>> +
>> +static int cm36686_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int *val,
>> + int *val2, long mask)
>> +{
>> + struct cm36686_data *chip = iio_priv(indio_dev);
>> + int ret;
>> +
>> + mutex_lock(&chip->lock);
>
> Can use linux/cleanup.h `guard(mutex)(&chip->lock);`. Then we can return
> directly below and get rid of ret.
>
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + ret = cm36686_read_channel(chip, chan, val);
>> + break;
>> + case IIO_CHAN_INFO_INT_TIME:
>> + ret = cm36686_read_int_time(chip, chan, val, val2);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + }
>> +
>> + mutex_unlock(&chip->lock);
>> + return ret;
>> +}
>> +
>> +static int cm36686_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int val,
>> + int val2, long mask)
>> +{
>> + struct cm36686_data *chip = iio_priv(indio_dev);
>> + int ret;
>> +
>> + if (val) /* Integration time more than 1s is not supported */
>> + return -EINVAL;
>> +
>> + if (mask != IIO_CHAN_INFO_INT_TIME)
>> + return -EINVAL;
>> +
>> + mutex_lock(&chip->lock);
>> +
>> + switch (chan->type) {
>> + case IIO_LIGHT:
>> + ret = cm36686_write_light_int_time(chip, val2);
>> + break;
>> + case IIO_PROXIMITY:
>> + ret = cm36686_write_prox_int_time(chip, val2);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + }
>> +
>> + mutex_unlock(&chip->lock);
>> + return ret;
>> +}
>> +
>> +static int cm36686_read_prox_thresh(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)
>> +{
>> + struct cm36686_data *chip = iio_priv(indio_dev);
>> +
>> + if (chan->type != IIO_PROXIMITY)
>> + return -EINVAL;
>> +
>> + switch (dir) {
>> + case IIO_EV_DIR_RISING:
>> + *val = chip->ps_close;
>> + break;
>> + case IIO_EV_DIR_FALLING:
>> + *val = chip->ps_away;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return IIO_VAL_INT;
>> +}
>> +
>> +static int cm36686_write_prox_thresh(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)
>> +{
>> + struct cm36686_data *chip = iio_priv(indio_dev);
>> + struct i2c_client *client = chip->client;
>> + int ret = 0, address, *thresh;
>> +
>> + if (chan->type != IIO_PROXIMITY)
>> + return -EINVAL;
>> +
>> + switch (dir) {
>> + case IIO_EV_DIR_FALLING:
>> + if (val > chip->ps_close || val < 0)
>> + return -EINVAL;
>> +
>> + address = CM36686_REG_PS_THDL;
>> + thresh = &chip->ps_away;
>> + break;
>> + case IIO_EV_DIR_RISING:
>> + if (val < chip->ps_away || val > CM36686_MAX_PS_VALUE)
>> + return -EINVAL;
>> +
>> + address = CM36686_REG_PS_THDH;
>> + thresh = &chip->ps_close;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + mutex_lock(&chip->lock);
>> +
>> + ret = i2c_smbus_write_word_data(client, address, val);
>> + if (ret < 0)
>> + dev_err(&client->dev,
>> + "Failed to set PS threshold value: %pe", ERR_PTR(ret));
>> + else
>> + *thresh = val;
>> +
>> + mutex_unlock(&chip->lock);
>> + return ret;
>
> In case it isn't obvious, previous comments about error messages and locks
> apply here and elsewhere too.
>
>> +}
>> +
>> +static int cm36686_read_prox_event_config(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir)
>> +{
>> + struct cm36686_data *chip = iio_priv(indio_dev);
>> +
>> + if (chan->type != IIO_PROXIMITY)
>> + return -EINVAL;
>> +
>> + switch (dir) {
>> + case IIO_EV_DIR_FALLING:
>> + return FIELD_GET(CM36686_PS_INT_OUT,
>> chip->ps_conf[CM36686_PS_CONF1]);
>> + case IIO_EV_DIR_RISING:
>> + return FIELD_GET(CM36686_PS_INT_IN,
>> chip->ps_conf[CM36686_PS_CONF1]);
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static int cm36686_write_prox_event_config(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir,
>> + bool state)
>> +{
>> + struct cm36686_data *chip = iio_priv(indio_dev);
>> + struct i2c_client *client = chip->client;
>> + int ret = 0, new_ps_conf;
>> +
>> + if (chan->type != IIO_PROXIMITY)
>> + return -EINVAL;
>> +
>> + switch (dir) {
>> + case IIO_EV_DIR_FALLING:
>> + new_ps_conf = chip->ps_conf[CM36686_PS_CONF1] &
>> ~CM36686_PS_INT_OUT;
>> + new_ps_conf |= FIELD_PREP(CM36686_PS_INT_OUT, state);
>> + break;
>> + case IIO_EV_DIR_RISING:
>> + new_ps_conf = chip->ps_conf[CM36686_PS_CONF1] &
>> ~CM36686_PS_INT_IN;
>> + new_ps_conf |= FIELD_PREP(CM36686_PS_INT_IN, state);
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + mutex_lock(&chip->lock);
>> +
>> + ret = i2c_smbus_write_word_data(chip->client, CM36686_REG_PS_CONF1,
>> new_ps_conf);
>> + if (ret < 0)
>> + dev_err(&client->dev,
>> + "Failed to set proximity event interrupt config: %pe",
>> ERR_PTR(ret));
>> + else
>> + chip->ps_conf[CM36686_PS_CONF1] = new_ps_conf;
>> +
>> + mutex_unlock(&chip->lock);
>> +
>> + return ret;
>> +}
>> +
>> +static int cm36686_fallback_read_ps(struct iio_dev *indio_dev)
>> +{
>> + struct cm36686_data *chip = iio_priv(indio_dev);
>> + struct i2c_client *client = chip->client;
>> + int data = i2c_smbus_read_word_data(client, CM36686_REG_PS_DATA);
>> +
>> + if (data < 0)
>> + return data;
>> +
>> + if (data < chip->ps_away)
>> + return IIO_EV_DIR_FALLING;
>> + else if (data > chip->ps_close)
>
> Don't need else after if with unconditinoal return.
>
>> + return IIO_EV_DIR_RISING;
>> + else
>> + return IIO_EV_DIR_EITHER;
>> +}
>> +
>> +static irqreturn_t cm36686_irq_handler(int irq, void *data)
>> +{
>> + struct iio_dev *indio_dev = data;
>> + struct cm36686_data *chip = iio_priv(indio_dev);
>> + struct i2c_client *client = chip->client;
>> + int ev_dir, ret;
>> + u64 ev_code;
>> +
>> + /* Reading the interrupt flag acknowledges the interrupt */
>> + ret = i2c_smbus_read_word_data(client, CM36686_REG_INT_FLAG);
>> + if (ret < 0) {
>> + dev_err(&client->dev,
>> + "Interrupt flag register read failed: %pe",
>> ERR_PTR(ret));
>
> Error messages in interrupt handlers should probably be rate-limited.
>
>> + return IRQ_HANDLED;
>> + }
>> +
>> + ret >>= CM36686_INT_FLAG_SHIFT;
>
> I think a FIELD_GET() would make more sense here.
>
>> + switch (ret) {
>> + case CM36686_CLOSE:
>> + ev_dir = IIO_EV_DIR_RISING;
>> + break;
>> + case CM36686_AWAY:
>> + ev_dir = IIO_EV_DIR_FALLING;
>> + break;
>> + case CM36686_BOTH:
>> + ev_dir = cm36686_fallback_read_ps(indio_dev);
>> + if (ev_dir < 0) {
>> + dev_err(&client->dev, "Failed to settle interrupt
>> state: %pe",
>> + ERR_PTR(ret));
>> + return IRQ_HANDLED;
>> + }
>> + break;
>> + default:
>> + dev_err(&client->dev, "Unknown interrupt state: %x", ret);
>> + return IRQ_HANDLED;
>> + }
>> + ev_code = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, IIO_EV_INFO_VALUE,
>> + IIO_EV_TYPE_THRESH, ev_dir);
>> +
>> + iio_push_event(indio_dev, ev_code, iio_get_time_ns(indio_dev));
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static const struct iio_info cm36686_info = {
>> + .read_avail = cm36686_read_avail,
>> + .read_raw = cm36686_read_raw,
>> + .write_raw = cm36686_write_raw,
>> + .read_event_value = cm36686_read_prox_thresh,
>> + .write_event_value = cm36686_write_prox_thresh,
>> + .read_event_config = cm36686_read_prox_event_config,
>> + .write_event_config = cm36686_write_prox_event_config,
>> +};
>> +
>> +static struct cm36686_chip_info cm36686_chip_info_tbl[] = {
>> + [CM36686] = {
>> + .name = "cm36686",
>> + .channels = cm36686_channels,
>> + .num_channels = ARRAY_SIZE(cm36686_channels),
>> + },
>> + [CM36672P] = {
>> + .name = "cm36672p",
>> + .channels = cm36672p_channels,
>> + .num_channels = ARRAY_SIZE(cm36672p_channels),
>> + },
>> +};
>
> We avoid this array style in new drivers in favor of individual struct
> per chip.
>
>> +
>> +static int cm36686_setup(struct cm36686_data *chip)
>> +{
>> + struct i2c_client *client = chip->client;
>> + int ret, led_current, led_index;
>> +
>> + chip->als_conf = CM36686_ALS_ENABLE;
>> +
>> + ret = i2c_smbus_write_word_data(client, CM36686_REG_ALS_CONF,
>> + chip->als_conf);
>> + if (ret < 0) {
>> + dev_err(&client->dev,
>> + "Failed to enable ambient light sensor: %pe",
>> ERR_PTR(ret));
>> + return ret;
>
> This setup function is only called from probe, so we can use
>
> return dev_err_probe() here and below.
>
>> + }
>> +
>
> Wouldn't mind a comment here explaining why these values were chosen as
> the default.
>
>> + chip->ps_conf[CM36686_PS_CONF1] = CM36686_PS_INT_IN |
>> + CM36686_PS_INT_OUT | CM36686_PS_DR_1_320 |
>> + CM36686_PS_PERS_2 | CM36686_PS_IT_2_5T;
>> +
>> + ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF1,
>> + chip->ps_conf[CM36686_PS_CONF1]);
>> + if (ret < 0) {
>> + dev_err(&client->dev,
>> + "Failed to enable proximity sensor: %pe", ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> + chip->ps_conf[CM36686_PS_CONF3] = CM36686_PS_SMART_PERS_ENABLE;
>> +
>> + ret = device_property_read_u32(&client->dev,
>> + "capella,proximity-led-current", &led_current);
>
> Only -EINVAL means the property was not present. Other errors can be
> propigated. Also, handling of the default value if the property isn't
> present isn't clear.
>
>> + if (!ret) {
>> + led_index = cm36686_current_to_index(led_current);
>> + if (led_index < 0) {
>> + dev_err(&client->dev, "No appropriate current for IR
>> LED found.");
>> + return led_index;
>> + }
>> +
>> + chip->ps_conf[CM36686_PS_CONF3] &= ~CM36686_LED_I;
>> + chip->ps_conf[CM36686_PS_CONF3] |= FIELD_PREP(CM36686_LED_I,
>> led_index);
>> + }
>> +
>> + ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF3,
>> + chip->ps_conf[CM36686_PS_CONF3]);
>> + if (ret < 0) {
>> + dev_err(&client->dev,
>> + "Failed to enable proximity sensor: %pe", ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> + ret = device_property_read_u32(&client->dev, "proximity-near-level",
>> + &chip->ps_close);
>> + if (ret < 0)
>> + chip->ps_close = 0;
>> +
>> + ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_THDH,
>> + chip->ps_close);
>> + if (ret < 0) {
>> + dev_err(&client->dev,
>> + "Failed to set close proximity threshold: %pe",
>> ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> + chip->ps_away = chip->ps_close;
>> +
>> + ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_THDL,
>> + chip->ps_away);
>> + if (ret < 0) {
>> + dev_err(&client->dev,
>> + "Failed to set away proximity threshold: %pe",
>> ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void cm36686_shutdown(void *data)
>> +{
>> + struct cm36686_data *chip = data;
>> + struct i2c_client *client = chip->client;
>> + int ret, als_shutdown, ps_shutdown;
>> +
>> + als_shutdown = chip->als_conf | CM36686_ALS_SD;
>> +
>> + ret = i2c_smbus_write_word_data(client, CM36686_REG_ALS_CONF,
>> + als_shutdown);
>> + if (ret < 0)
>> + dev_err(&client->dev, "Failed to shutdown ALS");
>> +
>> + ps_shutdown = chip->ps_conf[CM36686_PS_CONF1] | CM36686_PS_SD;
>> +
>> + ret = i2c_smbus_write_word_data(client, CM36686_REG_PS_CONF1,
>> + ps_shutdown);
>> + if (ret < 0)
>> + dev_err(&client->dev, "Failed to shutdown PS");
>> +
>> + ret = regulator_bulk_disable(ARRAY_SIZE(chip->supplies),
>> chip->supplies);
>> + if (ret < 0)
>> + dev_err(&client->dev, "Failed to disable regulators");
>
> If we use devm_regulator_bulk_get_enable(), then we don't need to disable
> regulators here and we can drop the chip->supplies field from the struct.
>
>> +}
>> +
>> +static int cm36686_probe(struct i2c_client *client)
>> +{
>> + struct iio_dev *indio_dev;
>> + struct cm36686_data *chip;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&client->dev,
>> + sizeof(struct cm36686_data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + chip = iio_priv(indio_dev);
>> +
>
>> + const struct i2c_device_id *id = i2c_client_get_device_id(client);
>> +
>> + const struct cm36686_chip_info *info =
>> + &cm36686_chip_info_tbl[id->driver_data];
>
> After getting rid of the array, this is replaced with i2c_get_match_data().
>
>> +
>> + ret = i2c_smbus_read_byte_data(client, CM36686_REG_ID_FLAG);
>> + if (ret < 0)
>> + return dev_err_probe(&client->dev, ret, "Failed to read device
>> ID");
>> +
>> + if (ret != CM36686_DEVICE_ID)
>> + return dev_err_probe(&client->dev, -ENODEV, "Device not
>> recognized!");
>
> If anything, this should only be a warning. Too many times, we get a chip that
> doesn't have the "right" ID but is still compatible.
>
>> +
>> + i2c_set_clientdata(client, indio_dev);
>> + chip->client = client;
>> + ret = devm_mutex_init(&client->dev, &chip->lock);
>> + if (ret < 0)
>> + return dev_err_probe(&client->dev, ret,
>> + "Failed to initialize mutex");
>> +
>> + chip->supplies[CM36686_SUPPLY_VDD].supply = "vdd";
>> + chip->supplies[CM36686_SUPPLY_VDDIO].supply = "vddio";
>> + chip->supplies[CM36686_SUPPLY_VLED].supply = "vled";
>> +
>> + ret = devm_regulator_bulk_get(&client->dev,
>
> This needs to be devm_regulator_bulk_get_enable(), or it doesn't actually
> enable the regulators.
>
>> + ARRAY_SIZE(chip->supplies), chip->supplies);
>> + if (ret < 0)
>> + return dev_err_probe(&client->dev, ret,
>> + "Failed to enable regulators");
>> +
>> + ret = devm_add_action_or_reset(&client->dev, cm36686_shutdown, chip);
>> + if (ret)
>> + return dev_err_probe(&client->dev, ret,
>> + "Failed to set shutdown action");
>
> This cleanup action should not be added until after the setup function
> since that is what it is undoing.
>
>> +
>> + ret = cm36686_setup(chip);
>> + if (ret < 0)
>> + return dev_err_probe(&client->dev, ret,
>> + "Failed to set up registers");
>> +
>> + indio_dev->name = info->name;
>> + indio_dev->channels = info->channels;
>> + indio_dev->num_channels = info->num_channels;
>> + indio_dev->info = &cm36686_info;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> + cm36686_irq_handler,
>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> + indio_dev->name, indio_dev);
>> + if (ret)
>> + return dev_err_probe(&client->dev, ret,
>> + "Failed to request irq");
>
> It should be OK to make the interrupt optional since that only affects
> events AFAICT. There would need to be a second cm36686_info struct without
> the events to handle this.
>
>> +
>> + ret = devm_iio_device_register(&client->dev, indio_dev);
>> + if (ret)
>> + return dev_err_probe(&client->dev, ret,
>> + "Failed to register iio device");
>> +
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id cm36686_id[] = {
>> + { "cm36686", CM36686 },
>> + { "cm36672p", CM36672P },
>> + {}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, cm36686_id);
>> +
>> +static const struct of_device_id cm36686_of_match[] = {
>> + { .compatible = "capella,cm36686" },
>> + { .compatible = "capella,cm36672p" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, cm36686_of_match);
>> +
>> +static struct i2c_driver cm36686_driver = {
>> + .driver = {
>> + .name = "cm36686",
>> + .of_match_table = cm36686_of_match,
>> + },
>> + .probe = cm36686_probe,
>> + .id_table = cm36686_id
>
> Trailing comma here.
>
>> +};
>> +
>
> Usually no blank line here.
>
>> +module_i2c_driver(cm36686_driver);
>> +
>> +MODULE_AUTHOR("Erikas Bitovtas <[email protected]>");
>> +MODULE_DESCRIPTION("CM36686 ambient light and proximity sensor driver");
>> +MODULE_LICENSE("GPL");
>>
>
