Re: [PATCH 4/4] iio: light: gp2ap020a00f: Add GP2AP030A00F support

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 08 Nov 2026 00:31:36 +0100
Linus Walleij <linusw@kernel.org> wrote:

> The GP2AP030A00F has the same register addresses and interrupt modes
> as the GP2AP020A00F, but does not implement its automatic
> illuminance calculation or selectable ambient-light cancellation.

Hmm. Raw clear and IR signals are normally intensity channels, not
light ones (because there is no standard match to illuminance).
Seems the original driver is not doing this right and we are stuck
with that though.

> 
> Datasheet: https://jp.sharp/products/device/doc/opto/gp2ap030a00f_appl_e.pdf
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
>  drivers/iio/light/Kconfig        |  6 +--
>  drivers/iio/light/gp2ap020a00f.c | 80 ++++++++++++++++++++++++++++++++++++----

Hmm. Fairly small.  Normally I'd ask for this in two patches
1) Introduce the infrastructure to support chip variants, but no
   functional changes.
2) Introduce the stuff for the part being added.

If it is simple enough I'll sometimes be more relaxed about that.
This is slightly too complex for me to be completely happy.

>  2 files changed, 76 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 462fff1ddef2..549f644994df 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -259,15 +259,15 @@ config GP2AP002
>  	  module will be called gp2ap002.
>  
>  config GP2AP020A00F
> -	tristate "Sharp GP2AP020A00F Proximity/ALS sensor"
> +	tristate "Sharp GP2AP020A00F/GP2AP030A00F Proximity/ALS sensors"

As for the description below, I'd just use 'and similar' as this will
become too long on the next part addition. So might as well avoid the
problem now.

>  	depends on I2C
>  	select REGMAP_I2C
>  	select IIO_BUFFER
>  	select IIO_TRIGGERED_BUFFER
>  	select IRQ_WORK
>  	help
> -	  Say Y here if you have a Sharp GP2AP020A00F proximity/ALS combo-chip
> -	  hooked to an I2C bus.
> +	  Say Y here if you have a Sharp GP2AP020A00F or GP2AP030A00F
> +	  proximity/ALS combo-chip hooked to an I2C bus.

Similar on making extension to more parts easy.
Change to a bulleted list of parts now as it will avoid churn when
the next one turns up. Lots of recent examples of this in tree.

>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called gp2ap020a00f.

> +
>  static bool gp2ap020a00f_is_volatile_reg(struct device *dev, unsigned int reg)
>  {
>  	switch (reg) {
> @@ -364,6 +400,7 @@ static int gp2ap020a00f_set_operation_mode(struct gp2ap020a00f_data *data,
>  					enum gp2ap020a00f_opmode op)
>  {
>  	unsigned int op_reg_val;
> +	u8 op_reg = opmode_regs_settings[op].op_reg;

If nothing else effects things, prefer reverse xmas tree. So I'd move
this up one line.

>  	int err;
>  
>  	if (op != GP2AP020A00F_OPMODE_SHUTDOWN) {
> @@ -401,12 +438,16 @@ static int gp2ap020a00f_set_operation_mode(struct gp2ap020a00f_data *data,
>  			return err;
>  	}
>  
> +	/* Bit 0 is reserved on the GP2AP030A00F. */
> +	if (!data->chip_info->has_als_auto)
> +		op_reg &= ~GP2AP020A00F_TYPE_MASK;

Perhaps set it explicitly to MANUAL_CALC given I guess that is
direct equivalent of this?  Effectively chip doesn't support AUTO_CALC.
That should be self explanatory given the has_als_auto check so you
can probably drop the comment after doing that.

> +
>  	/* Set OP_REG and apply operation mode (power on / off) */
>  	err = regmap_update_bits(data->regmap,
>  				 GP2AP020A00F_OP_REG,
>  				 GP2AP020A00F_OP_MASK | GP2AP020A00F_OP2_MASK |
>  				 GP2AP020A00F_OP3_MASK | GP2AP020A00F_TYPE_MASK,
> -				 opmode_regs_settings[op].op_reg);
> +				 op_reg);
>  	if (err < 0)
>  		return err;

...

> @@ -1401,7 +1448,7 @@ static const struct iio_buffer_setup_ops gp2ap020a00f_buffer_setup_ops = {
>  
>  static int gp2ap020a00f_probe(struct i2c_client *client)
>  {
> -	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +	struct iio_chan_spec *channels;
>  	struct device *dev = &client->dev;
>  	struct gp2ap020a00f_data *data;
>  	struct iio_dev *indio_dev;
> @@ -1413,6 +1460,7 @@ static int gp2ap020a00f_probe(struct i2c_client *client)
>  		return -ENOMEM;
>  
>  	data = iio_priv(indio_dev);
> +	data->chip_info = i2c_get_match_data(client);

Check for NULL.  Sadly can happen via various weird probe paths.

>  
>  	err = devm_regulator_get_enable_optional(dev, "vdd");
>  	if (err && err != -ENODEV)
> @@ -1437,9 +1485,12 @@ static int gp2ap020a00f_probe(struct i2c_client *client)
>  		goto error_regulator_disable;
>  	}
>  
> +	if (!data->chip_info->has_als_auto)
> +		usleep_range(1000, 2000);

fsleep() so no need to think if allow that slack makes sense or not.

> +
>  	/* Initialize device registers */
>  	err = regmap_bulk_write(regmap, GP2AP020A00F_OP_REG,
> -			gp2ap020a00f_reg_init_tab,
> +			data->chip_info->reg_init_tab,
>  			ARRAY_SIZE(gp2ap020a00f_reg_init_tab));
>  
>  	if (err < 0) {
> @@ -1456,9 +1507,20 @@ static int gp2ap020a00f_probe(struct i2c_client *client)
>  
>  	mutex_init(&data->lock);
>  	indio_dev->channels = gp2ap020a00f_channels;
> +	if (!data->chip_info->has_als_auto) {
> +		channels = devm_kmemdup(dev, gp2ap020a00f_channels,
> +					sizeof(gp2ap020a00f_channels), GFP_KERNEL);
> +		if (!channels) {
> +			err = -ENOMEM;
> +			goto error_regulator_disable;
> +		}
> +		channels[GP2AP020A00F_SCAN_MODE_LIGHT_CLEAR].scan_type.realbits = 16;
> +		channels[GP2AP020A00F_SCAN_MODE_LIGHT_IR].scan_type.realbits = 16;
> +		indio_dev->channels = channels;

For this just pick between two static arrays.
Whilst that may make things a bit bigger module size wise, it is
easier to read ad puts all this in one place.  Put a pointer
to the channel array + count of it's size in the chip_info structure.


> +	}
>  	indio_dev->num_channels = ARRAY_SIZE(gp2ap020a00f_channels);
>  	indio_dev->info = &gp2ap020a00f_info;
> -	indio_dev->name = id->name;
> +	indio_dev->name = data->chip_info->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	/* Allocate buffer */
> @@ -1534,13 +1596,17 @@ static void gp2ap020a00f_remove(struct i2c_client *client)
>  }

> @@ -1557,5 +1623,5 @@ static struct i2c_driver gp2ap020a00f_driver = {
>  module_i2c_driver(gp2ap020a00f_driver);
>  
>  MODULE_AUTHOR("Jacek Anaszewski <j.anaszewski@samsung.com>");
> -MODULE_DESCRIPTION("Sharp GP2AP020A00F Proximity/ALS sensor driver");
> +MODULE_DESCRIPTION("Sharp GP2AP020A00F/GP2AP030A00F proximity/ALS sensor driver");

These get long fast.  Maybe just jump directly to where we end up
which is the weasel words option of adding 'and similar' 

>  MODULE_LICENSE("GPL v2");
>