Re: [PATCH v4 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 06 Jan 2025 17:23:02 -0500
Mikael Gonella-Bolduc via B4 Relay <devnull+mgonellabolduc.dimonoff.com@kernel.org> wrote:

> From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> 
> APDS9160 is a combination of ALS and proximity sensors.
> 
> This patch add supports for:
>     - Intensity clear data and illuminance data
>     - Proximity data
>     - Gain control, rate control
>     - Event thresholds
> 
> Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>

Hi Mikael,

A few really minor things inline in addition to potential changes from
the dt-binding review / real units suggestion for the current controls.

Jonathan

> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index f14a3744271242f04fe7849b47308397ac2c9939..4a22043acdbbbed2b696a3225e4024654d5d9339 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
>  obj-$(CONFIG_ADUX1020)		+= adux1020.o
>  obj-$(CONFIG_AL3010)		+= al3010.o
>  obj-$(CONFIG_AL3320A)		+= al3320a.o
> +obj-$(CONFIG_APDS9160)		+= apds9160.o
>  obj-$(CONFIG_APDS9300)		+= apds9300.o
>  obj-$(CONFIG_APDS9306)		+= apds9306.o
>  obj-$(CONFIG_APDS9960)		+= apds9960.o
> diff --git a/drivers/iio/light/apds9160.c b/drivers/iio/light/apds9160.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..4aa02f87ace72056a9c50c70e9f761cb6c52c985
> --- /dev/null
> +++ b/drivers/iio/light/apds9160.c
> @@ -0,0 +1,1586 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * APDS9160 sensor driver.
> + * Chip is combined proximity and ambient light sensor.
> + * Author: 2024 Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/sysfs.h>
> +
> +#include <linux/unaligned.h>
> +
> +#define APDS9160_REGMAP_NAME "apds9160_regmap"
Just put this inline.  No benefit in having the define that I can see.

> +#define APDS9160_REG_CNT 37
Same for this. Define isn't adding anything. 

> +
> +/* Main control register */
> +#define APDS9160_REG_CTRL 0x00
> +#define APDS9160_CTRL_SWRESET BIT(4) /* 1: Activate reset */
> +#define APDS9160_CTRL_MODE_RGB BIT(2) /* 0: ALS & IR, 1: RGB & IR */
> +#define APDS9160_CTRL_EN_ALS BIT(1) /* 1: ALS active */
> +#define APDS9160_CTLR_EN_PS BIT(0) /* 1: PS active */
> +
> +/* Status register  */
> +#define APDS9160_SR_LS_INT BIT(4)
> +#define APDS9160_SR_LS_NEW_DATA BIT(3)
> +#define APDS9160_SR_PS_INT BIT(1)
> +#define APDS9160_SR_PS_NEW_DATA BIT(0)
> +
> +/* Interrupt configuration registers */
> +#define APDS9160_REG_INT_CFG 0x19
> +#define APDS9160_REG_INT_PST 0x1A
> +#define APDS9160_INT_CFG_EN_LS BIT(2) /* LS int enable */
> +#define APDS9160_INT_CFG_EN_PS BIT(0) /* PS int enable */
> +
> +/* Proximity registers */
> +#define APDS9160_REG_PS_LED 0x01
> +#define APDS9160_REG_PS_PULSES 0x02
> +#define APDS9160_REG_PS_MEAS_RATE 0x03
> +#define APDS9160_REG_PS_THRES_HI_LSB 0x1B
> +#define APDS9160_REG_PS_THRES_HI_MSB 0x1C
> +#define APDS9160_REG_PS_THRES_LO_LSB 0x1D
> +#define APDS9160_REG_PS_THRES_LO_MSB 0x1E
> +#define APDS9160_REG_PS_DATA_LSB 0x08
> +#define APDS9160_REG_PS_DATA_MSB 0x09
> +#define APDS9160_REG_PS_CAN_LEVEL_DIG_LSB 0x1F
> +#define APDS9160_REG_PS_CAN_LEVEL_DIG_MSB 0x20
> +#define APDS9160_REG_PS_CAN_LEVEL_ANA_DUR 0x21
> +#define APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT 0x22
> +
> +/* Light sensor registers */
> +#define APDS9160_REG_LS_MEAS_RATE 0x04
> +#define APDS9160_REG_LS_GAIN 0x05
> +#define APDS9160_REG_LS_DATA_CLEAR_LSB 0x0A
> +#define APDS9160_REG_LS_DATA_CLEAR 0x0B
> +#define APDS9160_REG_LS_DATA_CLEAR_MSB 0x0C
> +#define APDS9160_REG_LS_DATA_ALS_LSB 0x0D
> +#define APDS9160_REG_LS_DATA_ALS 0x0E
> +#define APDS9160_REG_LS_DATA_ALS_MSB 0x0F
> +#define APDS9160_REG_LS_THRES_UP_LSB 0x24
> +#define APDS9160_REG_LS_THRES_UP 0x25
> +#define APDS9160_REG_LS_THRES_UP_MSB 0x26
> +#define APDS9160_REG_LS_THRES_LO_LSB 0x27
> +#define APDS9160_REG_LS_THRES_LO 0x28
> +#define APDS9160_REG_LS_THRES_LO_MSB 0x29
> +#define APDS9160_REG_LS_THRES_VAR 0x2A
> +
> +/* Part identification number register */
> +#define APDS9160_REG_ID 0x06
> +
> +/* Status register */
> +#define APDS9160_REG_SR 0x07
> +#define APDS9160_SR_DATA_ALS BIT(3)
> +#define APDS9160_SR_DATA_PS BIT(0)
> +
> +/* Supported ID:s */
> +#define APDS9160_PART_ID_0 0x03
> +
> +#define APDS9160_PS_THRES_MAX 0x7FF
> +#define APDS9160_LS_THRES_MAX 0xFFFFF
> +#define APDS9160_CMD_LS_RESOLUTION_25MS 0x04
> +#define APDS9160_CMD_LS_RESOLUTION_50MS 0x03
> +#define APDS9160_CMD_LS_RESOLUTION_100MS 0x02
> +#define APDS9160_CMD_LS_RESOLUTION_200MS 0x01
> +#define APDS9160_PS_DATA_MASK 0x7FF
> +
> +#define APDS9160_DEFAULT_LS_GAIN 3
> +#define APDS9160_DEFAULT_LS_RATE 100
> +#define APDS9160_DEFAULT_PS_RATE 100
> +#define APDS9160_DEFAULT_PS_CANCELLATION_LEVEL 0
> +#define APDS9160_DEFAULT_PS_ANALOG_CANCELLATION 0
> +#define APDS9160_DEFAULT_PS_GAIN 1
> +#define APDS9160_DEFAULT_PS_CURRENT 100
> +#define APDS9160_DEFAULT_PS_RESOLUTION_11BITS 0x03
> +
> +static const struct reg_default apds9160_reg_defaults[] = {
> +	{ APDS9160_REG_CTRL, 0x00 }, /* Sensors disabled by default  */
> +	{ APDS9160_REG_PS_LED, 0x33 }, /* 60 kHz frequency, 100 mA */

No need to change anything but a parsing comment.
This is one reason I personally don't like the regfield stuff (though
I never stop people using it, I won't encourage it either!)
If we'd had a traditional use of FIELD_PREP() and masks throughout then
we'd have everything needed to allow explicit setting of these default
values as fields as well.  We don't have the defines for that and I
definitely don't want to suggest you add them alongside regfield
equivalents.

> +	{ APDS9160_REG_PS_PULSES, 0x08 }, /* 8 pulses */
> +	{ APDS9160_REG_PS_MEAS_RATE, 0x05 }, /* 100ms */
> +	{ APDS9160_REG_LS_MEAS_RATE, 0x22 }, /* 100ms */
> +	{ APDS9160_REG_LS_GAIN, 0x01 }, /* 3x */
> +	{ APDS9160_REG_INT_CFG, 0x10 }, /* Interrupts disabled */
> +	{ APDS9160_REG_INT_PST, 0x00 },
> +	{ APDS9160_REG_PS_THRES_HI_LSB, 0xFF },
> +	{ APDS9160_REG_PS_THRES_HI_MSB, 0x07 },
> +	{ APDS9160_REG_PS_THRES_LO_LSB, 0x00 },
> +	{ APDS9160_REG_PS_THRES_LO_MSB, 0x00 },
> +	{ APDS9160_REG_PS_CAN_LEVEL_DIG_LSB, 0x00 },
> +	{ APDS9160_REG_PS_CAN_LEVEL_DIG_MSB, 0x00 },
> +	{ APDS9160_REG_PS_CAN_LEVEL_ANA_DUR, 0x00 },
> +	{ APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT, 0x00 },
> +	{ APDS9160_REG_LS_THRES_UP_LSB, 0xFF },
> +	{ APDS9160_REG_LS_THRES_UP, 0xFF },
> +	{ APDS9160_REG_LS_THRES_UP_MSB, 0x0F },
> +	{ APDS9160_REG_LS_THRES_LO_LSB, 0x00 },
> +	{ APDS9160_REG_LS_THRES_LO, 0x00 },
> +	{ APDS9160_REG_LS_THRES_LO_MSB, 0x00 },
> +	{ APDS9160_REG_LS_THRES_VAR, 0x00 },
> +};

> +/*
> + * This parameter determines the cancellation pulse duration
> + * in each of the PWM pulse. The cancellation is applied during the
> + * integration phase of the PS measurement.
> + * Duration is programmed in half clock cycles
> + * A duration value of 0 or 1 will not generate any cancellation pulse
> + */
> +static int apds9160_set_ps_analog_cancellation(struct apds9160_chip *data,
> +					       int val)
> +{
> +	int ret;
> +
> +	if (val < 0 || val > 63)
> +		return -EINVAL;
> +
> +	ret = regmap_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_ANA_DUR,
> +			   val);
> +
> +	return ret;

return regmap_write...

> +}



> +static int apds9160_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)
> +{
> +	u8 reg;
> +	int ret = 0;
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	ret = apds9160_get_thres_reg(chan, dir, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY: {
> +		__le16 buf = cpu_to_le16(val);

Trivial: A little odd to set this before the range checks.
> +
> +		if (val < 0 || val > APDS9160_PS_THRES_MAX)
> +			return -EINVAL;
> +
> +		return regmap_bulk_write(data->regmap, reg, &buf, 2);
> +	}
> +	case IIO_LIGHT: {
> +		u8 buf[3];
> +
> +		if (val < 0 || val > APDS9160_LS_THRES_MAX)
> +			return -EINVAL;
> +
> +		put_unaligned_le24(val, buf);
> +		return regmap_bulk_write(data->regmap, reg, &buf, 3);
> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static int apds9160_detect(struct apds9160_chip *chip)
> +{
> +	struct i2c_client *client = chip->client;
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_read(chip->regmap, APDS9160_REG_ID, &val);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "ID read failed\n");
> +		return ret;
> +	}
> +
> +	if (val != APDS9160_PART_ID_0)
> +		dev_info(&client->dev, "Unsupported part id %u\n", val);

"Unknown part" or "Unrecognised part"
we are trying at least to support it in fallback case!

> +
> +	return 0;
> +}