Re: [PATCH v2 4/4] iio: proximity: rfd77402: Add interrupt handling support

[Jonathan Cameron <jic23@kernel.org>]

Hi Shrikant,

I'll try not to replicate too much stuff from Andy's review.

> +static irqreturn_t rfd77402_interrupt_handler(int irq, void *dev_id)

Why dev_id?  Seems unrelated to what it is which is data about the device.

> +{
> +	struct rfd77402_data *data = dev_id;
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, RFD77402_ICSR);
> +	if (ret < 0)
> +		return IRQ_NONE;
> +
> +	/* Check if the interrupt is from our device */
> +	if (!(ret & RFD77402_ICSR_RESULT))
> +		return IRQ_NONE;
> +
> +	/* Signal completion of measurement */
> +	complete(&data->completion);
> +	return IRQ_HANDLED;
> +}

> -static int rfd77402_measure(struct i2c_client *client)
> +static int rfd77402_measure(struct rfd77402_data *data)
>  {
> +	struct i2c_client *client = data->client;
>  	int ret;
> +

Blank line should have been in patch 3.  I just replied to that
having missed it whilst reading that one.


>  	ret = rfd77402_set_state(client, RFD77402_CMD_MCPU_ON,
>  				 RFD77402_STATUS_MCPU_ON);

> @@ -195,8 +247,25 @@ static const struct iio_info rfd77402_info = {
>  	.read_raw = rfd77402_read_raw,
>  };
>  
> +static int rfd77402_config_irq(struct i2c_client *client, u8 csr, u8 ier)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, RFD77402_ICSR, csr);
> +	if (ret < 0)

I would use
	if (ret)
here to make the next change more consistent looking.

> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, RFD77402_IER, ier);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
	return i2c_smbus_write_byte_data();

is fine as it is documented as never returning anything other than negative
or 0.

> +}
> +
>  static int rfd77402_init(struct i2c_client *client)
>  {
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct rfd77402_data *data = iio_priv(indio_dev);
>  	int ret, i;
>  
>  	ret = rfd77402_set_state(client, RFD77402_CMD_STANDBY,
> @@ -204,9 +273,25 @@ static int rfd77402_init(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> -	/* configure INT pad as push-pull, active low */
> -	ret = i2c_smbus_write_byte_data(client, RFD77402_ICSR,
> -					RFD77402_ICSR_INT_MODE);
> +	if (data->irq_en) {
> +	/*
> +	 * Enable interrupt mode:
> +	 * - Configure ICSR for auto-clear on read, push-pull output and falling edge
> +	 * - Enable "result ready" interrupt in IER
> +	 */

Indent should match code.  Will require a bit more wrapping.


> +		ret = rfd77402_config_irq(client,
> +					  RFD77402_ICSR_CLR_CFG |
> +					  RFD77402_ICSR_INT_MODE,
> +					  RFD77402_IER_RESULT);
> +	} else {
> +	/*
> +	 * Disable all interrupts:
> +	 * - Clear ICSR configuration
> +	 * - Disable all interrupt in IER
> +	 */
As above.

> +		ret = rfd77402_config_irq(client, 0, 0);
> +	}
> +
>  	if (ret < 0)
>  		return ret;
>  
> @@ -283,6 +368,31 @@ static int rfd77402_probe(struct i2c_client *client)
>  	data = iio_priv(indio_dev);
>  	data->client = client;
>  	mutex_init(&data->lock);
> +	init_completion(&data->completion);
> +
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	data->irq_en = false;
> +	if (client->irq > 0) {
> +		/* interrupt mode */

Kind of obvious comment. I'd drop it.

> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL, rfd77402_interrupt_handler,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,

Repeating Andy here the direction is a firmware problem.
We have some historical drivers doing this which we now can't
fix as we don't know if boards need it.  But no new cases should
be added.

> +						"rfd77402", data);
> +		if (ret < 0) {
> +			dev_err(&client->dev,
> +				"Failed to request IRQ %d: %d\n",
> +				client->irq, ret);
> +			return ret;
> +		}
> +
> +		data->irq_en = true;
> +		dev_info(&client->dev, "Using interrupt mode\n");

dev_dbg()

> +
> +	} else {
> +		/* polling mode */
> +		dev_info(&client->dev, "No interrupt specified, using polling mode\n");
Easy to tell from userspace with cat /proc/interrupts so no need for such a noisy print.
dev_dbg()
> +	}
>  
>  	indio_dev->info = &rfd77402_info;
>  	indio_dev->channels = rfd77402_channels;