Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 12 Feb 2019 18:17:53 -0800
From:   Robert Eshleman <bobbyeshleman@gmail.com>
To:     Sven Van Asbroeck <thesven73@gmail.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Hartmut Knaack <knaack.h@gmx.de>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-iio@vger.kernel.org
Subject: Re: [PATCH 1/3] iio: light: Add driver for ap3216c
Message-ID: <20190213021753.GA19621@bobby.localdomain>
References: <89716a4433cd83aea5f4200359b184b0ee2cc8bd.1549828313.git.bobbyeshleman@gmail.com>
 <CAGngYiXPsj-TAZxC43UtAZYTkbztFtDB-z4jW29CthmaoeNy_w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAGngYiXPsj-TAZxC43UtAZYTkbztFtDB-z4jW29CthmaoeNy_w@mail.gmail.com>
User-Agent: Mutt/1.11.2 (2019-01-07)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Feb 11, 2019 at 02:29:58PM -0500, Sven Van Asbroeck wrote:
> On Sun, Feb 10, 2019 at 3:39 PM Robert Eshleman <bobbyeshleman@gmail.com> wrote:
> >
> > This patch adds support for the ap3216c ambient light and proximity
> > sensor.
> 
> PS
> 
> Why not use the chip in the mode where the interrupt is automatically cleared by
> reading the data? This could work if you read the data in the
> interrupt routine, store
> it in a buffer, then send the event to iio. Then when userspace wants
> to read out the
> value, don't actually touch the hardware, but return the buffered value.
> 
> I don't think you then need any synchronization primitives to
> accomplish this, such as
> mutexes, spin locks, etc. Because the interrupt routine is single-threaded.
> 
> You don't even need a memory barrier for the buffered value,
> because the iio core uses a waitqueue internally, which automatically issues
> an mb(). As far as I know.

Hey Sven,

First, thank you for the feedback.

I had initially went with a similar design, but there is
the case in which the interrupt fires and then before the status
register is read by the handler a user process reads the data and
clears the interrupt.  When the handler continues execution it will
read a zero status and return IRQ_NONE.  My understanding of how
Linux handles IRQ_NONE is pretty poor, but I felt that this behavior
is incorrect even if inconsequential.  This could be avoided by
doing a status register read with every data read, and buffering
that as well, but then we lose the benefit altogether by increasing
I2C reads.

In the approach you describe here, it seems like that would
work if this driver wasn't supporting shared interrupts.  In the
case that a user-space read happens to clear the interrupt before
the handler reads the status register, I think we would end up
falsely returning IRQ_NONE.

Is my understanding of this correct?  It's very possible I'm
misunderstanding IRQ_NONE and shared interrupts.

Again, thank you for the feedback.

-Bobby