Re: [PATCH v4 2/3] iio: proximity: add driver for Sharp GP2AP070S proximity sensor

On Fri, Aug 07, 2026 at 01:18:42AM +0530, Kaustabh Chakraborty wrote:
> The GP2AP070S is a proximity sensor designed and manufactured by Sharp
> Corporation. This sensor is used in mobile devices, including, but not
> limited to - the Samsung Galaxy J6.
> 
> The driver has been adopted from Samsung's downstream kernel
> implementation [1]. Due to the lack of public documentation about the
> schematics of this device. The downstream driver acts as the secondary
> source of information. Driver clarity has also been improved with the
> help of the GP2AP* drivers in iio/light.

...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>

+ bits.h // BIT() et alia

> +#include <linux/delay.h>

+ dev_printk.h
+ device/devres.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>

+ sysfs.h // sysfs_emit()

> +#include <linux/types.h>
> +#include <linux/units.h>

+ asm/byteorder.h // le16_to_cpu()

> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>

> +#include <linux/iio/types.h>

I believe there is an agreement that iio.h implies types.h in iio/.

...

> +#define GP2AP070S_REG_PS_THD_LO_LE16	0x88
> +#define GP2AP070S_REG_PS_THD_HI_LE16	0x8a
> +#define GP2AP070S_REG_D0_LE16		0x90

All those _LE16 do not add any value, we see that from the use of them.

> +#define GP2AP070S_REG_MAX		(GP2AP070S_REG_D0_LE16 + 1)

...

> +struct gp2ap070s_drvdata {
> +	struct regmap *regmap;
> +	struct mutex mutex;

Lock should have a comment explaining the data it protects.

> +	u32 near_level;
> +};

...

> +static const struct regmap_config gp2ap070s_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.volatile_reg = gp2ap070s_regmap_volatile,
> +	.max_register = GP2AP070S_REG_MAX,
> +	.cache_type = REGCACHE_FLAT,

Oh, registers are from 0x80 to 0x91, why FLAT and not MAPLE?

> +};

...

> +static int gp2ap070s_iio_read_raw(struct iio_dev *indio_dev,
> +				  struct iio_chan_spec const *chan, int *val,
> +				  int *val2, long mask)

Split logically here and everywhere else.

static int gp2ap070s_iio_read_raw(struct iio_dev *indio_dev,
				  struct iio_chan_spec const *chan,
				  int *val, int *val2, long mask)

...

> +static int gp2ap070s_iio_read_event_value(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)

Ditto.

> +{
> +	struct gp2ap070s_drvdata *drvdata = iio_priv(indio_dev);
> +	__le16 value;
> +	int ret;
> +
> +	if (type != IIO_EV_TYPE_THRESH || info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	guard(mutex)(&drvdata->mutex);
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		ret = regmap_bulk_read(drvdata->regmap,
> +				       GP2AP070S_REG_PS_THD_HI_LE16, &value,
> +				       sizeof(value));

Ditto.

(With above comment about LE16 this will become

		ret = regmap_bulk_read(drvdata->regmap, GP2AP070S_REG_PS_THD_HI,
				       &value, sizeof(value));

which is one line shorter.)

> +		if (ret)
> +			return ret;
> +
> +		*val = le16_to_cpu(value);
> +		return IIO_VAL_INT;
> +	case IIO_EV_DIR_FALLING:
> +		ret = regmap_bulk_read(drvdata->regmap,
> +				       GP2AP070S_REG_PS_THD_LO_LE16, &value,
> +				       sizeof(value));

Ditto.

> +		if (ret)
> +			return ret;
> +
> +		*val = le16_to_cpu(value);
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static int gp2ap070s_iio_write_event_value(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)

Same as per above function.

...

> +		/* Ensure hi_threshold > lo_threshold */
> +		threshold_other = le16_to_cpu(value);
> +		if (threshold_other <= val)
> +			return -EINVAL;
> +
> +		value = cpu_to_le16(val);
> +		ret = regmap_bulk_write(drvdata->regmap,
> +					GP2AP070S_REG_PS_THD_LO_LE16, &value,
> +					sizeof(value));
> +		if (ret)
> +			return ret;
> +
> +		return 0;

		return regmap_bulk_write(...);

...

> +	ret = regmap_write_bits(drvdata->regmap, GP2AP070S_REG_PS2,
> +				GP2AP070S_PS2_IOUT | GP2AP070S_PS2_SUM32,
> +				FIELD_PREP_CONST(GP2AP070S_PS2_IOUT,
> +						 GP2AP070S_PS2_IOUT_89mA) |

> +				GP2AP070S_PS2_SUM32);

What's the point to have dup constant to be ORed?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set current output\n");

...

> +	fsleep(10 * (MICRO / MILLI));

What's is this multiplier for? I think you wanted USEC_PER_MSEC from time.h.
Also add a comment with the reference to the datasheet (or other means) of why
this value has been chosen.

...

> +	if (device_property_present(dev, "proximity-near-level")) {
> +		ret = device_property_read_u32(dev, "proximity-near-level",
> +					       &drvdata->near_level);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to get value for proximity-near-level\n");
> +	}

Any default to be applied?

-- 
With Best Regards,
Andy Shevchenko