| Message ID | 20200206061332.20427-2-matt.ranostay@konsulko.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: chemical: atlas-sensor: add DO support | expand |
On Wed, Feb 05, 2020 at 10:13:30PM -0800, Matt Ranostay wrote: > Sensors don't actually need a interrupt line to give valid readings, > and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove > the required check for interrupt, and continue along in the probe > function. > > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > --- > drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- > 1 file changed, 12 insertions(+), 15 deletions(-) > > diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c > index 2f0a6fed2589..2e34c82cb65d 100644 > --- a/drivers/iio/chemical/atlas-sensor.c > +++ b/drivers/iio/chemical/atlas-sensor.c > @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, > if (ret) > return ret; > > - if (client->irq <= 0) { > - dev_err(&client->dev, "no valid irq defined\n"); > - return -EINVAL; > - } > - > ret = chip->calibration(data); > if (ret) > return ret; > @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, > > init_irq_work(&data->work, atlas_work_handler); > > - /* interrupt pin toggles on new conversion */ > - ret = devm_request_threaded_irq(&client->dev, client->irq, > - NULL, atlas_interrupt_handler, > - IRQF_TRIGGER_RISING | > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > - "atlas_irq", > - indio_dev); > - if (ret) { > - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); > - goto unregister_buffer; > + if (client->irq <= 0) { Should this be > 0 ? Jeremy > + /* interrupt pin toggles on new conversion */ > + ret = devm_request_threaded_irq(&client->dev, client->irq, > + NULL, atlas_interrupt_handler, > + IRQF_TRIGGER_RISING | > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + "atlas_irq", > + indio_dev); > + > + if (ret) > + dev_warn(&client->dev, > + "request irq (%d) failed\n", client->irq); > } > > ret = atlas_set_powermode(data, 1); > -- > 2.20.1 >
On Thu, Feb 6, 2020 at 4:53 PM Jeremy Fertic <jeremyfertic@gmail.com> wrote: > > On Wed, Feb 05, 2020 at 10:13:30PM -0800, Matt Ranostay wrote: > > Sensors don't actually need a interrupt line to give valid readings, > > and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove > > the required check for interrupt, and continue along in the probe > > function. > > > > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > > --- > > drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- > > 1 file changed, 12 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c > > index 2f0a6fed2589..2e34c82cb65d 100644 > > --- a/drivers/iio/chemical/atlas-sensor.c > > +++ b/drivers/iio/chemical/atlas-sensor.c > > @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, > > if (ret) > > return ret; > > > > - if (client->irq <= 0) { > > - dev_err(&client->dev, "no valid irq defined\n"); > > - return -EINVAL; > > - } > > - > > ret = chip->calibration(data); > > if (ret) > > return ret; > > @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, > > > > init_irq_work(&data->work, atlas_work_handler); > > > > - /* interrupt pin toggles on new conversion */ > > - ret = devm_request_threaded_irq(&client->dev, client->irq, > > - NULL, atlas_interrupt_handler, > > - IRQF_TRIGGER_RISING | > > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > - "atlas_irq", > > - indio_dev); > > - if (ret) { > > - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); > > - goto unregister_buffer; > > + if (client->irq <= 0) { > > Should this be > 0 ? > Ah yes good catch :/ - Matt > Jeremy > > > + /* interrupt pin toggles on new conversion */ > > + ret = devm_request_threaded_irq(&client->dev, client->irq, > > + NULL, atlas_interrupt_handler, > > + IRQF_TRIGGER_RISING | > > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > + "atlas_irq", > > + indio_dev); > > + > > + if (ret) > > + dev_warn(&client->dev, > > + "request irq (%d) failed\n", client->irq); > > } > > > > ret = atlas_set_powermode(data, 1); > > -- > > 2.20.1 > >
On Wed, 5 Feb 2020 22:13:30 -0800 Matt Ranostay <matt.ranostay@konsulko.com> wrote: > Sensors don't actually need a interrupt line to give valid readings, > and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove > the required check for interrupt, and continue along in the probe > function. Basic aim is good, but if you don't have an interrupt doesn't make sense to register the trigger either. The interrupt enable / disable is also tied up with the buffer whereas it should probably be done via the trigger enable callback or am I missing something? Jonathan > > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > --- > drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- > 1 file changed, 12 insertions(+), 15 deletions(-) > > diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c > index 2f0a6fed2589..2e34c82cb65d 100644 > --- a/drivers/iio/chemical/atlas-sensor.c > +++ b/drivers/iio/chemical/atlas-sensor.c > @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, > if (ret) > return ret; > > - if (client->irq <= 0) { > - dev_err(&client->dev, "no valid irq defined\n"); > - return -EINVAL; > - } > - > ret = chip->calibration(data); > if (ret) > return ret; > @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, > > init_irq_work(&data->work, atlas_work_handler); > > - /* interrupt pin toggles on new conversion */ > - ret = devm_request_threaded_irq(&client->dev, client->irq, > - NULL, atlas_interrupt_handler, > - IRQF_TRIGGER_RISING | > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > - "atlas_irq", > - indio_dev); > - if (ret) { > - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); > - goto unregister_buffer; > + if (client->irq <= 0) { > + /* interrupt pin toggles on new conversion */ > + ret = devm_request_threaded_irq(&client->dev, client->irq, > + NULL, atlas_interrupt_handler, > + IRQF_TRIGGER_RISING | > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + "atlas_irq", > + indio_dev); > + > + if (ret) > + dev_warn(&client->dev, > + "request irq (%d) failed\n", client->irq); > } > > ret = atlas_set_powermode(data, 1);
> On Feb 8, 2020, at 08:39, Jonathan Cameron <jic23@kernel.org> wrote: > > On Wed, 5 Feb 2020 22:13:30 -0800 > Matt Ranostay <matt.ranostay@konsulko.com> wrote: > >> Sensors don't actually need a interrupt line to give valid readings, >> and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove >> the required check for interrupt, and continue along in the probe >> function. > > Basic aim is good, but if you don't have an interrupt doesn't make > sense to register the trigger either. > > The interrupt enable / disable is also tied up with the buffer whereas > it should probably be done via the trigger enable callback or am I > missing something? It was for allowing sysfs and hrtimer triggers. But just remembered most these sensors have a low refresh rate. I can go either way on having it or not. Thoughts? - Matt > > Jonathan > >> >> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> >> --- >> drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- >> 1 file changed, 12 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c >> index 2f0a6fed2589..2e34c82cb65d 100644 >> --- a/drivers/iio/chemical/atlas-sensor.c >> +++ b/drivers/iio/chemical/atlas-sensor.c >> @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, >> if (ret) >> return ret; >> >> - if (client->irq <= 0) { >> - dev_err(&client->dev, "no valid irq defined\n"); >> - return -EINVAL; >> - } >> - >> ret = chip->calibration(data); >> if (ret) >> return ret; >> @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, >> >> init_irq_work(&data->work, atlas_work_handler); >> >> - /* interrupt pin toggles on new conversion */ >> - ret = devm_request_threaded_irq(&client->dev, client->irq, >> - NULL, atlas_interrupt_handler, >> - IRQF_TRIGGER_RISING | >> - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >> - "atlas_irq", >> - indio_dev); >> - if (ret) { >> - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); >> - goto unregister_buffer; >> + if (client->irq <= 0) { >> + /* interrupt pin toggles on new conversion */ >> + ret = devm_request_threaded_irq(&client->dev, client->irq, >> + NULL, atlas_interrupt_handler, >> + IRQF_TRIGGER_RISING | >> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >> + "atlas_irq", >> + indio_dev); >> + >> + if (ret) >> + dev_warn(&client->dev, >> + "request irq (%d) failed\n", client->irq); >> } >> >> ret = atlas_set_powermode(data, 1); >
> On Feb 8, 2020, at 18:12, Matt Ranostay <matt.ranostay@konsulko.com> wrote: > > > >>> On Feb 8, 2020, at 08:39, Jonathan Cameron <jic23@kernel.org> wrote: >>> >>> On Wed, 5 Feb 2020 22:13:30 -0800 >>> Matt Ranostay <matt.ranostay@konsulko.com> wrote: >>> >>> Sensors don't actually need a interrupt line to give valid readings, >>> and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove >>> the required check for interrupt, and continue along in the probe >>> function. >> >> Basic aim is good, but if you don't have an interrupt doesn't make >> sense to register the trigger either. >> >> The interrupt enable / disable is also tied up with the buffer whereas >> it should probably be done via the trigger enable callback or am I >> missing something? > > It was for allowing sysfs and hrtimer triggers. But just remembered most these sensors have a low refresh rate. I can go either way on having it or not. Thoughts? > > - Matt > Also issue one issue was fixed in v5 related to this. - Matt > >> >> Jonathan >> >>> >>> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> >>> --- >>> drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- >>> 1 file changed, 12 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c >>> index 2f0a6fed2589..2e34c82cb65d 100644 >>> --- a/drivers/iio/chemical/atlas-sensor.c >>> +++ b/drivers/iio/chemical/atlas-sensor.c >>> @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, >>> if (ret) >>> return ret; >>> >>> - if (client->irq <= 0) { >>> - dev_err(&client->dev, "no valid irq defined\n"); >>> - return -EINVAL; >>> - } >>> - >>> ret = chip->calibration(data); >>> if (ret) >>> return ret; >>> @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, >>> >>> init_irq_work(&data->work, atlas_work_handler); >>> >>> - /* interrupt pin toggles on new conversion */ >>> - ret = devm_request_threaded_irq(&client->dev, client->irq, >>> - NULL, atlas_interrupt_handler, >>> - IRQF_TRIGGER_RISING | >>> - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >>> - "atlas_irq", >>> - indio_dev); >>> - if (ret) { >>> - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); >>> - goto unregister_buffer; >>> + if (client->irq <= 0) { >>> + /* interrupt pin toggles on new conversion */ >>> + ret = devm_request_threaded_irq(&client->dev, client->irq, >>> + NULL, atlas_interrupt_handler, >>> + IRQF_TRIGGER_RISING | >>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >>> + "atlas_irq", >>> + indio_dev); >>> + >>> + if (ret) >>> + dev_warn(&client->dev, >>> + "request irq (%d) failed\n", client->irq); >>> } >>> >>> ret = atlas_set_powermode(data, 1); >>
On Sat, 8 Feb 2020 18:12:03 -0800 Matt Ranostay <matt.ranostay@konsulko.com> wrote: > > On Feb 8, 2020, at 08:39, Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Wed, 5 Feb 2020 22:13:30 -0800 > > Matt Ranostay <matt.ranostay@konsulko.com> wrote: > > > >> Sensors don't actually need a interrupt line to give valid readings, > >> and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove > >> the required check for interrupt, and continue along in the probe > >> function. > > > > Basic aim is good, but if you don't have an interrupt doesn't make > > sense to register the trigger either. > > > > The interrupt enable / disable is also tied up with the buffer whereas > > it should probably be done via the trigger enable callback or am I > > missing something? > > It was for allowing sysfs and hrtimer triggers. But just remembered most these sensors have a low refresh rate. I can go either way on having it or not. Thoughts? I'm a bit confused. With sysfs and hrtimer triggers why would we need the interrupt enabled? As things stand it will be as it's done in the buffer setup. I was suggesting moving it to the trigger setup so we only deal with the interrupt if we are actually using the data ready trigger. So move it the atlas_set_interrupt call from pre / post enable to the trigger state callback. Jonathan > > - Matt > > > > > > Jonathan > > > >> > >> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > >> --- > >> drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- > >> 1 file changed, 12 insertions(+), 15 deletions(-) > >> > >> diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c > >> index 2f0a6fed2589..2e34c82cb65d 100644 > >> --- a/drivers/iio/chemical/atlas-sensor.c > >> +++ b/drivers/iio/chemical/atlas-sensor.c > >> @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, > >> if (ret) > >> return ret; > >> > >> - if (client->irq <= 0) { > >> - dev_err(&client->dev, "no valid irq defined\n"); > >> - return -EINVAL; > >> - } > >> - > >> ret = chip->calibration(data); > >> if (ret) > >> return ret; > >> @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, > >> > >> init_irq_work(&data->work, atlas_work_handler); > >> > >> - /* interrupt pin toggles on new conversion */ > >> - ret = devm_request_threaded_irq(&client->dev, client->irq, > >> - NULL, atlas_interrupt_handler, > >> - IRQF_TRIGGER_RISING | > >> - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > >> - "atlas_irq", > >> - indio_dev); > >> - if (ret) { > >> - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); > >> - goto unregister_buffer; > >> + if (client->irq <= 0) { > >> + /* interrupt pin toggles on new conversion */ > >> + ret = devm_request_threaded_irq(&client->dev, client->irq, > >> + NULL, atlas_interrupt_handler, > >> + IRQF_TRIGGER_RISING | > >> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > >> + "atlas_irq", > >> + indio_dev); > >> + > >> + if (ret) > >> + dev_warn(&client->dev, > >> + "request irq (%d) failed\n", client->irq); > >> } > >> > >> ret = atlas_set_powermode(data, 1); > >
diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c index 2f0a6fed2589..2e34c82cb65d 100644 --- a/drivers/iio/chemical/atlas-sensor.c +++ b/drivers/iio/chemical/atlas-sensor.c @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, if (ret) return ret; - if (client->irq <= 0) { - dev_err(&client->dev, "no valid irq defined\n"); - return -EINVAL; - } - ret = chip->calibration(data); if (ret) return ret; @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, init_irq_work(&data->work, atlas_work_handler); - /* interrupt pin toggles on new conversion */ - ret = devm_request_threaded_irq(&client->dev, client->irq, - NULL, atlas_interrupt_handler, - IRQF_TRIGGER_RISING | - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, - "atlas_irq", - indio_dev); - if (ret) { - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); - goto unregister_buffer; + if (client->irq <= 0) { + /* interrupt pin toggles on new conversion */ + ret = devm_request_threaded_irq(&client->dev, client->irq, + NULL, atlas_interrupt_handler, + IRQF_TRIGGER_RISING | + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + "atlas_irq", + indio_dev); + + if (ret) + dev_warn(&client->dev, + "request irq (%d) failed\n", client->irq); } ret = atlas_set_powermode(data, 1);
Sensors don't actually need a interrupt line to give valid readings, and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove the required check for interrupt, and continue along in the probe function. Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> --- drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)