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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] Double execution of hardware reset on probe error paths.
- [Medium] Meaningless regcache operations due to missing .cache_type, and incorrect volatile register definition.
- [Medium] Silent probe failure on IRQ request error.
- [Medium] write_event_value callback incorrectly returns IIO_VAL_INT on success.
- [Medium] Unconditional enablement of hardware events at probe.
- [Medium] Missing PM suspend/resume operations leaves the sensor active during system sleep.
--

--- Patch [2]: [PATCH v3 2/3] iio: proximity: add driver for Sharp GP2AP070S proximity sensor ---
commit 804bae46d93961c6260dcd9dc62d6687c6acc1bf
Author: Kaustabh Chakraborty <kauschluss@disroot.org>

    iio: proximity: add driver for Sharp GP2AP070S proximity sensor

This commit introduces a new IIO proximity driver for the Sharp GP2AP070S sensor.
It sets up regmap-based I2C communication and configures IIO events and channels
for reading proximity data.

Link: https://github.com/Exynos7870/android_kernel_samsung_universal7870/blob/lineage-16.0/drivers/sensors/gp2ap070s.c [1]

> diff --git a/drivers/iio/proximity/gp2ap070s.c b/drivers/iio/proximity/gp2ap070s.c
> new file mode 100644
> index 0000000000000..98fc12e5ccd58
> --- /dev/null
> +++ b/drivers/iio/proximity/gp2ap070s.c
[ ... ]
> +static bool gp2ap070s_regmap_volatile(struct device *dev, unsigned int reg)
> +{
> +	return reg == GP2AP070S_REG_D0_LE16;
> +}
> +
> +static const struct regmap_config gp2ap070s_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.volatile_reg = gp2ap070s_regmap_volatile,
> +};

[Severity: Medium]
Does this regmap configuration intend to use caching? 

The driver calls regcache_mark_dirty() during gp2ap070s_reset(), but
gp2ap070s_regmap_config lacks a .cache_type assignment, so the regmap core
will bypass caching entirely. 

If caching is enabled in the future, will the current volatile_reg definition
cause data corruption? gp2ap070s_regmap_volatile() only marks 0x90 as volatile,
but a 16-bit value is read spanning 0x90 and 0x91. The upper byte (0x91) would
incorrectly be served from the cache, corrupting the proximity reading.

[ ... ]
> +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)
> +{
> +	struct gp2ap070s_drvdata *drvdata = iio_priv(indio_dev);
> +	__le16 value;
> +	u16 threshold_other;
> +	int ret;
[ ... ]
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
[ ... ]
> +		value = cpu_to_le16((u16)val);
> +		ret = regmap_bulk_write(drvdata->regmap,
> +					GP2AP070S_REG_PS_THD_HI_LE16, &value,
> +					sizeof(value));
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;

[Severity: Medium]
Should this return 0 instead of IIO_VAL_INT?

The IIO ABI requires write_event_value callbacks to return 0 on success.
IIO_VAL_INT is only meant for read_event_value to describe the returned data
type. Returning it here may break IIO consumers.

> +	case IIO_EV_DIR_FALLING:
[ ... ]
> +		value = cpu_to_le16((u16)val);
> +		ret = regmap_bulk_write(drvdata->regmap,
> +					GP2AP070S_REG_PS_THD_LO_LE16, &value,
> +					sizeof(value));
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;

[Severity: Medium]
This also appears to incorrectly return IIO_VAL_INT instead of 0 on success.

[ ... ]
> +static int gp2ap070s_probe_hw_register(struct gp2ap070s_drvdata *drvdata)
> +{
> +	struct device *dev = regmap_get_device(drvdata->regmap);
> +	int ret;
> +
> +	ret = gp2ap070s_reset(drvdata);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to reset sensor\n");
> +
> +	ret = regmap_write(drvdata->regmap, GP2AP070S_REG_COM2, 0);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable interrupts\n");
> +
> +	ret = regmap_set_bits(drvdata->regmap, GP2AP070S_REG_COM3,
> +			      GP2AP070S_COM3_INT_PULSE);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to unmask interrupts\n");

[Severity: Medium]
Does this unconditionally enable hardware interrupts at probe?

The IIO ABI specifies that events should be disabled by default and only
enabled when requested via sysfs by userspace. Enabling them unconditionally at
probe might cause unexpected system wakeups or interrupt load.

[ ... ]
> +static int gp2ap070s_probe(struct i2c_client *client)
> +{
[ ... ]
> +	ret = gp2ap070s_probe_hw_register(drvdata);
> +	if (ret)
> +		goto hw_reset;
> +
> +	ret = devm_add_action_or_reset(dev, gp2ap070s_reset_action, drvdata);
> +	if (ret) {
> +		ret = dev_err_probe(dev, ret, "Failed to schedule reset on driver removal\n");
> +		goto hw_reset;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +					gp2ap070s_irq_handler, IRQF_ONESHOT,
> +					"gp2ap070s-irq", indio_dev);
> +	if (ret)
> +		goto hw_reset;

[Severity: Medium]
Is it intentional that devm_request_threaded_irq() failure is silent?

If the requested interrupt is missing in the Device Tree or fails to be
requested, the driver fails to load without leaving any diagnostic messages in
the kernel log. This should likely use dev_err_probe().

> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		goto hw_reset;
> +
> +	return 0;
> +
> +hw_reset:
> +	gp2ap070s_reset(drvdata);

[Severity: Medium]
Will this cause a double execution of the hardware reset on probe error paths?

If devm_add_action_or_reset() succeeds, but a subsequent step like
devm_request_threaded_irq() fails, the code manually jumps to hw_reset and
calls gp2ap070s_reset(). 

When the probe function then returns the error code, the devres framework will
unwind and automatically invoke gp2ap070s_reset_action(), which calls
gp2ap070s_reset() a second time.

> +
> +	return ret;
> +}
[ ... ]
> +static struct i2c_driver gp2ap070s_i2c_driver = {
> +	.driver = {
> +		.name = "gp2ap070s",
> +		.of_match_table = gp2ap070s_of_device_id,
> +	},
> +	.probe = gp2ap070s_probe,
> +	.id_table = gp2ap070s_i2c_device_id,
> +};
> +module_i2c_driver(gp2ap070s_i2c_driver);

[Severity: Medium]
Is this driver missing power management suspend and resume operations?

Without a .pm structure to handle suspend/resume, the kernel cannot disable
the sensor or its regulators when the system enters suspend, which could
lead to power drain or spurious interrupts during system sleep.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-gp2ap070s-v3-0-d1f5cecf9fe7@disroot.org?part=2