| Message ID | 20181208151638.8759-3-shreeya.patel23498@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove platform data and introduce DT bindings | expand |
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);
On Sat, 2018-12-08 at 16:12 +0000, Jonathan Cameron wrote: > 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. > 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); > >
On Sat, Dec 08, 2018 at 08:46:37PM +0530, Shreeya Patel wrote: > - 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; return 0; here. > } > > /* regards, dan carpenter
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) { - 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);
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> --- drivers/staging/iio/addac/adt7316.c | 34 ++++++++++++++--------------- 1 file changed, 17 insertions(+), 17 deletions(-)