On Sat, 2018-12-08 at 16:12 +0000, Jonathan Cameron wrote:
> On Sat, 8 Dec 2018 20:46:37 +0530
> Shreeya Patel <shreeya.patel23...@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.patel23...@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.
>
As I was introducing a new function named "adt7316_setup_irq" so I
thought patch 1 should come first because we are setting up the
irq_type there.
But yes, this made the code complex to review.
I didn't merge both patches because both the patches were having
different changes. If I would have done that then there was a
possibility where someone would have said to split the patches.
> 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.
>
It's ok, I'll merge both patches and send as a v4 to you. I'll send it
after 3-4 days so we can get other reviews by that time if there are
any to come.
My vacation has started so I'll work faster now.
> 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_hand
> > ler,
> > - 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);
>
>
