Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 13 May 2023 19:57:58 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Astrid Rost <astrid.rost@axis.com>
Cc:     Lars-Peter Clausen <lars@metafoo.de>, <linux-iio@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <kernel@axis.com>
Subject: Re: [PATCH v2 1/7] iio: light: vcnl4000: Add proximity irq for
 vcnl4200
Message-ID: <20230513195758.0d18825f@jic23-huawei>
In-Reply-To: <20230509140153.3279288-2-astrid.rost@axis.com>
References: <20230509140153.3279288-1-astrid.rost@axis.com>
        <20230509140153.3279288-2-astrid.rost@axis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 9 May 2023 16:01:47 +0200
Astrid Rost <astrid.rost@axis.com> wrote:

> Add proximity interrupt support for vcnl4200 (similar to vcnl4040).
> Add support to configure proximity sensor interrupts and threshold
> limits. If an interrupt is detected an event will be pushed to the
> event interface.
> 
> Signed-off-by: Astrid Rost <astrid.rost@axis.com>
Where possible (and I think it is here) please avoid putting data as code.
That is, move the necessary register address into the
*_chip_spec structure.  That way, if another version of the chip is supported
in future we can simply add the value to the right instance of that structure.
That tends to end up more flexible than if / else / switch statements.

Thanks,

Jonathan

> ---
>  drivers/iio/light/vcnl4000.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 56d3963d3d66..13568454baff 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -65,6 +65,7 @@
>  #define VCNL4200_PS_DATA	0x08 /* Proximity data */
>  #define VCNL4200_AL_DATA	0x09 /* Ambient light data */
>  #define VCNL4040_INT_FLAGS	0x0b /* Interrupt register */
> +#define VCNL4200_INT_FLAGS	0x0d /* Interrupt register */
>  #define VCNL4200_DEV_ID		0x0e /* Device ID, slave address and version */
>  
>  #define VCNL4040_DEV_ID		0x0c /* Device ID and version */
> @@ -1004,8 +1005,14 @@ static irqreturn_t vcnl4040_irq_thread(int irq, void *p)
>  	struct iio_dev *indio_dev = p;
>  	struct vcnl4000_data *data = iio_priv(indio_dev);
>  	int ret;
> +	int reg;
>  
> -	ret = i2c_smbus_read_word_data(data->client, VCNL4040_INT_FLAGS);
> +	if (data->id == VCNL4200)
> +		reg = VCNL4200_INT_FLAGS;
> +	else
> +		reg = VCNL4040_INT_FLAGS;

Prefer this as
	data->chip_spec->int_reg

> +
> +	ret = i2c_smbus_read_word_data(data->client, reg);
>  	if (ret < 0)
>  		return IRQ_HANDLED;
>  
> @@ -1321,9 +1328,10 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  		.measure_light = vcnl4200_measure_light,
>  		.measure_proximity = vcnl4200_measure_proximity,
>  		.set_power_state = vcnl4200_set_power_state,
> -		.channels = vcnl4000_channels,
> +		.channels = vcnl4040_channels,
>  		.num_channels = ARRAY_SIZE(vcnl4000_channels),
> -		.info = &vcnl4000_info,
> +		.info = &vcnl4040_info,
> +		.irq_thread = vcnl4040_irq_thread,
>  	},
>  };
>