* [PATCH v3 0/3] Remove platform data and introduce DT bindings @ 2018-12-08 15:16 Shreeya Patel 2018-12-08 15:16 ` [PATCH v3 1/3] Staging: iio: adt7316: Use device tree data to assign irq_type Shreeya Patel ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Shreeya Patel @ 2018-12-08 15:16 UTC (permalink / raw) To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, shreeya.patel23498 This patchset introduces the use of device tree bindings for setting up the irq_type and removes the usage of platform data. Also, code related to interrupt is moved to the new function adt7316_setup_irq to unclutter the code in adt7316_probe(). A dev_err() message is added to give more details about the error. Shreeya Patel (3): Staging: iio: adt7316: Use device tree data to assign irq_type Staging: iio: adt7316: Move interrupt related code Staging: iio: adt7316: Add a dev_err() message drivers/staging/iio/addac/adt7316.c | 52 +++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 14 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 1/3] Staging: iio: adt7316: Use device tree data to assign irq_type 2018-12-08 15:16 [PATCH v3 0/3] Remove platform data and introduce DT bindings Shreeya Patel @ 2018-12-08 15:16 ` Shreeya Patel 2018-12-08 15:16 ` [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code Shreeya Patel 2018-12-08 15:16 ` [PATCH v3 3/3] Staging: iio: adt7316: Add a dev_err() message Shreeya Patel 2 siblings, 0 replies; 7+ messages in thread From: Shreeya Patel @ 2018-12-08 15:16 UTC (permalink / raw) To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, shreeya.patel23498 ADT7316 driver no more uses platform data and hence use device tree data instead of platform data for assigning irq_type field. Switch case figures out the type of irq and if it's the default case then assign the default value to the irq_type i.e. irq_type = IRQF_TRIGGER_LOW All this is implemented in a new function called adt7316_setup_irq. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com> --- drivers/staging/iio/addac/adt7316.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c index 9c72538baf9e..86b2c3d53588 100644 --- a/drivers/staging/iio/addac/adt7316.c +++ b/drivers/staging/iio/addac/adt7316.c @@ -1807,6 +1807,29 @@ static irqreturn_t adt7316_event_handler(int irq, void *private) return IRQ_HANDLED; } +static int adt7316_setup_irq(struct device *dev, int irq) +{ + int irq_type; + + irq_type = irqd_get_trigger_type(irq_get_irq_data(irq)); + + switch (irq_type) { + case IRQF_TRIGGER_HIGH: + case IRQF_TRIGGER_RISING: + break; + case IRQF_TRIGGER_LOW: + case IRQF_TRIGGER_FALLING: + break; + default: + dev_info(dev, "mode %d unsupported, using IRQF_TRIGGER_LOW\n", + irq_type); + irq_type = IRQF_TRIGGER_LOW; + break; + } + + return irq_type; +} + /* * Show mask of enabled interrupts in Hex. */ @@ -2101,8 +2124,7 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus, { struct adt7316_chip_info *chip; struct iio_dev *indio_dev; - unsigned short *adt7316_platform_data = dev->platform_data; - int irq_type = IRQF_TRIGGER_LOW; + int irq_type; int ret = 0; indio_dev = devm_iio_device_alloc(dev, sizeof(*chip)); @@ -2146,8 +2168,7 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus, indio_dev->modes = INDIO_DIRECT_MODE; if (chip->bus.irq > 0) { - if (adt7316_platform_data[0]) - irq_type = adt7316_platform_data[0]; + irq_type = adt7316_setup_irq(dev, chip->bus.irq); ret = devm_request_threaded_irq(dev, chip->bus.irq, NULL, -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code 2018-12-08 15:16 [PATCH v3 0/3] Remove platform data and introduce DT bindings Shreeya Patel 2018-12-08 15:16 ` [PATCH v3 1/3] Staging: iio: adt7316: Use device tree data to assign irq_type Shreeya Patel @ 2018-12-08 15:16 ` Shreeya Patel 2018-12-08 16:12 ` Jonathan Cameron 2018-12-11 9:35 ` Dan Carpenter 2018-12-08 15:16 ` [PATCH v3 3/3] Staging: iio: adt7316: Add a dev_err() message Shreeya Patel 2 siblings, 2 replies; 7+ messages in thread From: Shreeya Patel @ 2018-12-08 15:16 UTC (permalink / raw) To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, shreeya.patel23498 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(-) 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); -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code 2018-12-08 15:16 ` [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code Shreeya Patel @ 2018-12-08 16:12 ` Jonathan Cameron 2018-12-08 16:44 ` Shreeya Patel 2018-12-11 9:35 ` Dan Carpenter 1 sibling, 1 reply; 7+ messages in thread From: Jonathan Cameron @ 2018-12-08 16:12 UTC (permalink / raw) To: Shreeya Patel Cc: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel 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); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code 2018-12-08 16:12 ` Jonathan Cameron @ 2018-12-08 16:44 ` Shreeya Patel 0 siblings, 0 replies; 7+ messages in thread From: Shreeya Patel @ 2018-12-08 16:44 UTC (permalink / raw) To: Jonathan Cameron Cc: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel 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); > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code 2018-12-08 15:16 ` [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code Shreeya Patel 2018-12-08 16:12 ` Jonathan Cameron @ 2018-12-11 9:35 ` Dan Carpenter 1 sibling, 0 replies; 7+ messages in thread From: Dan Carpenter @ 2018-12-11 9:35 UTC (permalink / raw) To: Shreeya Patel Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 3/3] Staging: iio: adt7316: Add a dev_err() message 2018-12-08 15:16 [PATCH v3 0/3] Remove platform data and introduce DT bindings Shreeya Patel 2018-12-08 15:16 ` [PATCH v3 1/3] Staging: iio: adt7316: Use device tree data to assign irq_type Shreeya Patel 2018-12-08 15:16 ` [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code Shreeya Patel @ 2018-12-08 15:16 ` Shreeya Patel 2 siblings, 0 replies; 7+ messages in thread From: Shreeya Patel @ 2018-12-08 15:16 UTC (permalink / raw) To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, shreeya.patel23498 Add a dev_err() message "failed to request irq" for describing what went wrong when an error contition is statisfied. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com> --- drivers/staging/iio/addac/adt7316.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c index 97dd48153293..e3eb8ad06403 100644 --- a/drivers/staging/iio/addac/adt7316.c +++ b/drivers/staging/iio/addac/adt7316.c @@ -1832,8 +1832,11 @@ static int adt7316_setup_irq(struct iio_dev *indio_dev) NULL, adt7316_event_handler, irq_type | IRQF_ONESHOT, indio_dev->name, indio_dev); - if (ret) + if (ret) { + dev_err(&indio_dev->dev, "failed to request irq %d\n", + chip->bus.irq); return ret; + } if (irq_type & IRQF_TRIGGER_HIGH) chip->config1 |= ADT7316_INT_POLARITY; -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-12-11 9:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-08 15:16 [PATCH v3 0/3] Remove platform data and introduce DT bindings Shreeya Patel 2018-12-08 15:16 ` [PATCH v3 1/3] Staging: iio: adt7316: Use device tree data to assign irq_type Shreeya Patel 2018-12-08 15:16 ` [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code Shreeya Patel 2018-12-08 16:12 ` Jonathan Cameron 2018-12-08 16:44 ` Shreeya Patel 2018-12-11 9:35 ` Dan Carpenter 2018-12-08 15:16 ` [PATCH v3 3/3] Staging: iio: adt7316: Add a dev_err() message Shreeya Patel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).