Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 25 Apr 2020 16:57:09 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Mathieu Othacehe <m.othacehe@gmail.com>,
        Hartmut Knaack <knaack.h@gmx.de>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald <pmeerw@pmeerw.net>,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>
Subject: Re: [PATCH v4 4/4] iio: vcnl4000: Add buffer support for
 VCNL4010/20.
Message-ID: <20200425165709.27c63f05@archlinux>
In-Reply-To: <CAHp75VfXBgQad1oCBe+oqcC_oRa-3q8OBYcAOV8WfCo7n1wXWw@mail.gmail.com>
References: <20200421075532.19192-1-m.othacehe@gmail.com>
        <20200421075532.19192-5-m.othacehe@gmail.com>
        <CAHp75VfXBgQad1oCBe+oqcC_oRa-3q8OBYcAOV8WfCo7n1wXWw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 21 Apr 2020 15:27:14 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Apr 21, 2020 at 10:59 AM Mathieu Othacehe <m.othacehe@gmail.com> wrote:
> >
> > The VCNL4010 and VCNL4020 chips are able to raise interrupts on data ready.
> > Use it to provide triggered buffer support for proximity data.
> >
> > Those two chips also provide ambient light data. However, they are sampled
> > at different rate than proximity data. As this is not handled by the IIO
> > framework for now, and the sample frequencies of ambient light data are
> > very low, do add buffer support for them.  
> 
> ...
> 
> > +static irqreturn_t vcnl4010_trigger_handler(int irq, void *p)
> > +{
> > +       struct iio_poll_func *pf = p;
> > +       struct iio_dev *indio_dev = pf->indio_dev;
> > +       struct vcnl4000_data *data = iio_priv(indio_dev);
> > +       const unsigned long *active_scan_mask = indio_dev->active_scan_mask;
> > +       u16 buffer[8] = {0}; /* 1x16-bit + ts */
> > +       bool data_read = false;
> > +       unsigned long isr;
> > +       int val = 0;
> > +       int ret;
> > +
> > +       ret = i2c_smbus_read_byte_data(data->client, VCNL4010_ISR);
> > +       if (ret < 0)
> > +               goto end;
> > +
> > +       isr = ret;
> > +
> > +       if (test_bit(0, active_scan_mask)) {
> > +               if (test_bit(VCNL4010_INT_PROXIMITY, &isr)) {
> > +                       ret = vcnl4000_read_data(data,
> > +                                                VCNL4000_PS_RESULT_HI,
> > +                                                &val);
> > +                       if (ret < 0)
> > +                               goto end;
> > +
> > +                       buffer[0] = val;
> > +                       data_read = true;
> > +               }
> > +       }
> > +
> > +       ret = i2c_smbus_write_byte_data(data->client, VCNL4010_ISR,
> > +                                       isr & VCNL4010_INT_DRDY);  
> 
> > +       if (ret < 0 || !data_read)  
> 
> I would split them, because they are logically different checks.
> 
> > +               goto end;
> > +
> > +       iio_push_to_buffers_with_timestamp(indio_dev, buffer,
> > +                                          iio_get_time_ns(indio_dev));
> > +
> >  end:
> > +       iio_trigger_notify_done(indio_dev->trig);
> >         return IRQ_HANDLED;
> >  }  
> 
> ...
> 
> > +static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > +       struct vcnl4000_data *data = iio_priv(indio_dev);
> > +       int ret, ret_disable;
> > +
> > +       ret = i2c_smbus_write_byte_data(data->client, VCNL4010_INT_CTRL, 0);
> > +       if (ret < 0)
> > +               goto end;
> > +
> > +       ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, 0);
> > +
> > +end:  
> 
> > +       ret_disable = iio_triggered_buffer_predisable(indio_dev);
> > +       if (ret == 0)
> > +               ret = ret_disable;  
> 
> What is this?
> 
> Can't you rather call IIO API first, and then try to handle the rest?

There is an additional complexity here. Alex is in the middle of trying
to refactor all drivers to handle this in the same order so as to
ultimately remote the need to explicitly make that call at all.

So until that is sorted, I think that needs to be the last call made
even if there isn't a driver related reason for the ordering.

It's a big job with lots of complex corner cases so will be a little
while yet I guess before we can do the core rework.

Jonathan

> 
> > +       return ret;
> > +}  
>