Re: [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code

[Jonathan Cameron <jic23@kernel.org>]

On Sat,  8 Dec 2018 20:46:37 +0530
Shreeya Patel <shreeya.patel23498@gmail.com> wrote:

> There is a function adt7316_irq_setup() where irq_type is being
> set. It would be good to move devm_request_threaded_irq() function
> and assignment of chip->config1 in adt7316_irq_setup() to unclutter
> the code in probe function.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
As commented below, this didn't end up as tidy as it might have been.
It would I think have been simpler before patch 1 or just merged with it.

Anyhow, I might combine the two whilst applying. However before I do
that I'd like to leave this on list for a few days to let Alex
or others have time for another look before I apply it.

All heading in the right direction!

Thanks,

Jonathan

> ---
>  drivers/staging/iio/addac/adt7316.c | 34 ++++++++++++++---------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index 86b2c3d53588..97dd48153293 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -1807,11 +1807,12 @@ static irqreturn_t adt7316_event_handler(int irq, void *private)
>  	return IRQ_HANDLED;
>  }
>  
> -static int adt7316_setup_irq(struct device *dev, int irq)
> +static int adt7316_setup_irq(struct iio_dev *indio_dev)

Hmm. This has ended up a lot more complex than ideal due
to the effective two layers of rework.  

I would either have done patches 1 and 2 as one patch or
reordered them so the rework preceded the change
to DT.  It's not that important but it would have lead
to code that was easier to review.


>  {
> -	int irq_type;
> +	struct adt7316_chip_info *chip = iio_priv(indio_dev);
> +	int irq_type, ret;
>  
> -	irq_type = irqd_get_trigger_type(irq_get_irq_data(irq));
> +	irq_type = irqd_get_trigger_type(irq_get_irq_data(chip->bus.irq));
>  
>  	switch (irq_type) {
>  	case IRQF_TRIGGER_HIGH:
> @@ -1821,13 +1822,23 @@ static int adt7316_setup_irq(struct device *dev, int irq)
>  	case IRQF_TRIGGER_FALLING:
>  		break;
>  	default:
> -		dev_info(dev, "mode %d unsupported, using IRQF_TRIGGER_LOW\n",
> +		dev_info(&indio_dev->dev, "mode %d unsupported, using IRQF_TRIGGER_LOW\n",
>  			 irq_type);
>  		irq_type = IRQF_TRIGGER_LOW;
>  		break;
>  	}
>  
> -	return irq_type;
> +	ret = devm_request_threaded_irq(&indio_dev->dev, chip->bus.irq,
> +					NULL, adt7316_event_handler,
> +					irq_type | IRQF_ONESHOT,
> +					indio_dev->name, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	if (irq_type & IRQF_TRIGGER_HIGH)
> +		chip->config1 |= ADT7316_INT_POLARITY;
> +
> +	return ret;
>  }
>  
>  /*
> @@ -2124,7 +2135,6 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus,
>  {
>  	struct adt7316_chip_info *chip;
>  	struct iio_dev *indio_dev;
> -	int irq_type;
>  	int ret = 0;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
> @@ -2168,19 +2178,9 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	if (chip->bus.irq > 0) {
> -		irq_type = adt7316_setup_irq(dev, chip->bus.irq);
> -
> -		ret = devm_request_threaded_irq(dev, chip->bus.irq,
> -						NULL,
> -						adt7316_event_handler,
> -						irq_type | IRQF_ONESHOT,
> -						indio_dev->name,
> -						indio_dev);
> +		ret = adt7316_setup_irq(indio_dev);
>  		if (ret)
>  			return ret;
> -
> -		if (irq_type & IRQF_TRIGGER_HIGH)
> -			chip->config1 |= ADT7316_INT_POLARITY;
>  	}
>  
>  	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, chip->config1);