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

[Javier Carrasco <javier.carrasco.cruz@gmail.com>]

Hi Mikael,

a few comments inline to add to what Krzysztof pointed out.

On 19/11/2024 21:36, Mikael Gonella-Bolduc via B4 Relay 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>
> ---
>  MAINTAINERS                  |    7 +
>  drivers/iio/light/Kconfig    |   13 +
>  drivers/iio/light/Makefile   |    1 +
>  drivers/iio/light/apds9160.c | 1420 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1441 insertions(+)
> 

...

> +config APDS9160
> +	tristate "APDS9160 combined als and proximity sensors"
> +	select REGMAP_I2C
> +	select IIO_BUFFER
> +	select IIO_KFIFO_BUF
> +	depends on I2C
> +	help
> +	   Say Y here if you want to build a driver for Broadcom APDS9160
> +	   combined ambient light and proximity sensor chip.
> +

You can drop that "If unsure, say N here." as it is not common for such
drivers in IIO. There are a couple of entries in the whole subsystem
(not in this Kconfig, though) with this sentence, and some of them could
be dropped too.

> +	   To compile this driver as a module, choose M here: the
> +	   module will be called apds9160. If unsure, say N here.
> +
>  config APDS9300
>  	tristate "APDS9300 ambient light sensor"
>  	depends on I2C

...


> +
> +static int apds9160_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int val,
> +			      int val2, long mask)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_INTENSITY:
> +			return apds9160_set_als_int_time(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			return apds9160_set_ps_rate(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_INTENSITY:
> +			return apds9160_set_als_gain(data, val);
> +		case IIO_PROXIMITY:
> +			return apds9160_set_ps_gain(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			return apds9160_set_ps_cancellation_level(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			return apds9160_set_ps_analog_cancellation(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_RAW:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_CURRENT:
> +			return apds9160_set_ps_current(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +

If you only have a switch with a return in every path, this return 0
can't be reached.

> +	return 0;
> +}
> +
> +static inline int apds9160_get_thres_reg(const struct iio_chan_spec *chan,
> +					 enum iio_event_direction dir, u8 *reg)
> +{
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			*reg = APDS9160_REG_PS_THRES_HI_LSB;
> +			break;
> +		case IIO_INTENSITY:
> +			*reg = APDS9160_REG_LS_THRES_UP_LSB;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			*reg = APDS9160_REG_PS_THRES_LO_LSB;
> +			break;
> +		case IIO_INTENSITY:
> +			*reg = APDS9160_REG_LS_THRES_LO_LSB;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int apds9160_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)
> +{
> +	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;
> +
> +	if (chan->type == IIO_PROXIMITY) {
> +		__le16 buf;
> +
> +		ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
> +		if (ret < 0)
> +			return ret;
> +		*val = le16_to_cpu(buf);
> +	} else if (chan->type == IIO_INTENSITY) {
> +		__le32 buf = 0;
> +
> +		ret = regmap_bulk_read(data->regmap, reg, &buf, 3);
> +		if (ret < 0)
> +			return ret;
> +		*val = le32_to_cpu(buf);

Missing braces for that else (use them in all arms if you need them in one).

> +	} else
> +		return -EINVAL;
> +
> +	*val2 = 0;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +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;
> +
> +	if (chan->type == IIO_PROXIMITY) {
> +		if (val < 0 || val > APDS9160_PS_THRES_MAX)
> +			return -EINVAL;
> +		__le16 buf;
> +
> +		buf = cpu_to_le16(val);
> +		ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
> +		if (ret < 0)
> +			return ret;
> +	} else if (chan->type == IIO_INTENSITY) {
> +		if (val < 0 || val > APDS9160_LS_THRES_MAX)
> +			return -EINVAL;
> +		__le32 buf = 0;
> +
> +		buf = cpu_to_le32(val);
> +		ret = regmap_bulk_write(data->regmap, reg, &buf, 3);
> +		if (ret < 0)
> +		

Same here.

	return ret;
> +	} else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int apds9160_read_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		return data->ps_int;
> +	case IIO_INTENSITY:
> +		return data->als_int;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +

The 'state' argument is now a bool. To avoid issues, please rebase to
newer branches like linux-next, iio/testing iio/togreg. Otherwise it
will not compile with that modification. data->ps_int should then become
a bool too.

> +static int apds9160_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)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	state = !!state;
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		if (data->ps_int == state)
> +			return -EINVAL;
> +
> +		ret = regmap_field_write(data->reg_int_ps, state);
> +		if (ret)
> +			return ret;
> +		data->ps_int = state;
> +		break;
> +	case IIO_INTENSITY:
> +		if (data->als_int == state)
> +			return -EINVAL;
> +
> +		ret = regmap_field_write(data->reg_int_als, state);
> +		if (ret)
> +			return ret;
> +		data->als_int = state;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}


Best regards,
Javier Carrasco