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

On 2026-07-23 23:44 +02:00, Joshua Crofts wrote:
> On Thu, 23 Jul 2026 22:58:34 +0530
> Kaustabh Chakraborty <[email protected]> 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.
>>=20
>> 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.
>>=20
>> Link: https://github.com/Exynos7870/android_kernel_samsung_universal7870=
/blob/lineage-16.0/drivers/sensors/gp2ap070s.c [1]
>> Signed-off-by: Kaustabh Chakraborty <[email protected]>
>> ---

[...]

>> =20
>> +config GP2AP070S
>> +	tristate "Sharp GP2AP070S proximity sensor"
>> +	select REGMAP_I2C
>> +	depends on I2C
>
> A very small nit (and probably a personal opinion), but "depends on" shou=
ld
> go before "select"

I happen to agree with this one. However most (but not all) entries
follow select -> depends on though. In any case I'll change it.

>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * IIO driver for Sharp GP2AP070S proximity sensor.
>> + *
>> + * Copyright (C) 2026 Kaustabh Chakraborty <[email protected]>
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/iio/events.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/types.h>
>
> Please add iio/* includes after the generic linux/* headers. Ensure
> that there is a blank line between the two groups.
>
> Additionally, you're also missing <asm/byteorder.h>, array_size.h, err.h,
> types.h and delay.h.

By the way, is there any tooling to satisfactorily point out the
shortfalls with includes? Or is it just intuition and experience?

>> +	ret =3D devm_request_threaded_irq(dev, client->irq, NULL,
>> +					gp2ap070s_irq_handler, IRQF_ONESHOT,
>> +					"gp2ap070s-irq", indio_dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to request IRQ");
>
> Just return ret instead, dev_err_probe() is called automatically on failu=
re.

Are you sure about that? I happen to call dev_err_probe() on all other
places and other drivers (as of late) as well (other than -ENOMEM).