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:   Fri, 6 Jul 2018 18:15:37 +0100
From:   Jonathan Cameron <jonathan.cameron@huawei.com>
To:     Linus Walleij <linus.walleij@linaro.org>
CC:     William Breathitt Gray <vilhelm.gray@gmail.com>,
        David Lechner <david@lechnology.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        "Jonathan Cameron" <jic23@kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        <linux-iio@vger.kernel.org>,
        Fabrice Gasnier <fabrice.gasnier@st.com>,
        Benjamin Gaignard <benjamin.gaignard@st.com>,
        Rob Herring <robh+dt@kernel.org>,
        Hartmut Knaack <knaack.h@gmx.de>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald <pmeerw@pmeerw.net>,
        Mark Rutland <mark.rutland@arm.com>
Subject: Re: [v7,03/10] docs: Add Generic Counter interface documentation
Message-ID: <20180706181537.00006916@huawei.com>
In-Reply-To: <CACRpkdbq1ued8gA-zcJYtK2jYJyMbuSkvinCvuoz_VGU=i+1nQ@mail.gmail.com>
References: <7606bdc53c26c332b2bbff0f865380fb0e874b56.1529607879.git.vilhelm.gray@gmail.com>
        <b7c690eb-274e-3377-cf8a-2f9ab836677e@lechnology.com>
        <20180703141620.GB14958@sophia>
        <CACRpkdbq1ued8gA-zcJYtK2jYJyMbuSkvinCvuoz_VGU=i+1nQ@mail.gmail.com>
Organization: Huawei
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 Wed, 4 Jul 2018 19:23:26 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Tue, Jul 3, 2018 at 4:16 PM William Breathitt Gray
> <vilhelm.gray@gmail.com> wrote:
> > On Mon, Jul 02, 2018 at 02:37:53PM -0500, David Lechner wrote:  
> > >On 06/21/2018 04:07 PM, William Breathitt Gray wrote:  
> > >> +Userspace Interface
> > >> +===================
> > >> +
> > >> +Several sysfs attributes are generated by the Generic Counter interface,
> > >> +and reside under the /sys/bus/counter/devices/counterX directory, where
> > >> +counterX refers to the respective counter device. Please see
> > >> +Documentation/ABI/testing/sys-bus-counter-generic-sysfs for detailed
> > >> +information on each Generic Counter interface sysfs attribute.
> > >> +
> > >> +Through these sysfs attributes, programs and scripts may interact with
> > >> +the Generic Counter paradigm Counts, Signals, and Synapses of respective
> > >> +counter devices.
> > >> +  
> > >
> > >Have you considered a character device in addition to the sysfs interface?
> > >
> > >I basically have many of the same concerns that resulted in a char dev for
> > >gpio[1].
> > >
> > >- With sysfs, you *can* technically poll for events, but then you have to
> > >   seek and read or re-open the file.

For this to be relevant we need some type of self clocking sampling of a counter,
so far this hasn't really been true for William's devices - they tend to have
internal monitoring and you just 'ask' them when you want to know the rotation.
Sure if we have 'events' such as soft limit switches in the hardware, then
we'd want some sort of event chrdev (personally I think these should be separate
from the main data flow - but that's a detail).

> > >- File permissions are annoying if you want a non root user to be able to
> > >   use the device.

They aren't a whole lot different for a chrdev.  In both cases you can allow
a non root user write access if you want to.

> > >- A single program can't claim exclusive access to a device.

Agreed.  Though that only matters for control, why do you care if someone
else can read.  In IIO we get around this by 'generally' blocking settings
changes that will a process that is sampling data via the chrdev.
It's not a hard and fast rule though.  I really don't like configuration
via chrdevs as for complex devices you end up with a non self describing
interface with a lot of complexity.

The exceptions are things like the media controller frameworks, but they
are very very heavyweight for an simple devices like counters.

> > >- There is no automatic cleanup if a userspace program accessing the device
> > >    crashes.

For these devices, the question is what sort of cleanup makes sense?

Often they are freerunning so the most you could do is power down if you knew
no one cared, but for an encoder you may well want to continue tracking even
if no one is looking right now.

I can think of reasons you 'might' want to tidy up, but we'd need real
driver code to show the necessity of this one.

> > >
> > >[1]: https://www.elinux.org/images/7/74/Elce2017_new_GPIO_interface.pdf  
> >
> > Those look like good technical reasons for implementing a character
> > device for the Generic Counter interface. I chose to implement the sysfs
> > interface because I was using the IIO code as a reference, but I
> > personally don't have much opposition to a character device in addition.  
> 
> IIO is also using a character device for outputting events and sensor
> data. In IIO sysfs is only used for configuring what events and
> data should come out of the character device.

Yes, with the addition that we typically provide data readback as well.
For some simple devices which are slow and are actually polled to get
a reading, there is not a lot of point in implementing the chrdev route
so in IIO it is optional.
> 
> > I'd like to get Jonathan's opinion on this as well if possible -- I
> > vaguely recall us considering this option briefly last year when the
> > Generic Counter interface was still in its beginnings. I've CC'd Linus
> > Walleij as well for input as the GPIO maintainer.  

I'm not against it, but I do want to see use cases that are not
satisfied by sysfs first.

So far we've no seen them but sounds like you might have one David!

Jonathan

> 
> The GPIO character device was based on the IIO character device.
> 
> We have one TODO item: to merge the timestamping format
> between GPIO and IIO and use the same sysfs file for configuring
> the time base.
> 
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html