Re: [PATCH v4 1/3] iio: chemical: atlas-sensor: allow probe without interrupt line

[Jonathan Cameron <jic23@kernel.org>]

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);  
> >