Re: [PATCH v2 1/1] counter: cros_ec: Add synchronization sensor

From: William Breathitt Gray <vilhelm.gray@gmail.com>
To: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
	Fabien Lahoudere <fabien.lahoudere@collabora.com>,
	gwendal@chromium.org, egranata@chromium.org,
	kernel@collabora.com, Jonathan Corbet <corbet@lwn.net>,
	Benson Leung <bleung@chromium.org>,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	Guenter Roeck <groeck@chromium.org>,
	Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	Lee Jones <lee.jones@linaro.org>,
	Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Nicolas Ferre <nicolas.ferre@microchip.com>,
	Nick Vaccaro <nvaccaro@chromium.org>,
	linux-iio@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/1] counter: cros_ec: Add synchronization sensor
Date: Mon, 11 Nov 2019 20:16:18 -0500	[thread overview]
Message-ID: <20191112011618.GA62259@icarus> (raw)
In-Reply-To: <20191111114955.00001031@huawei.com>

On Mon, Nov 11, 2019 at 11:49:55AM +0000, Jonathan Cameron wrote:
> On Sun, 10 Nov 2019 10:14:08 -0500
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > On Tue, Sep 24, 2019 at 04:20:51PM +0200, Fabien Lahoudere wrote:
> > > Hi all,
> > > 
> > > After some discussions and investigation, the timestamp is very
> > > important for that sync driver.
> > > Google team uses that timestamp to compare with gyroscope timestamp.
> > > 
> > > So the important data is timestamp and counter value is useless.
> > > Just the event of counter increment is important to get a timestamp.
> > > 
> > > In that case, my idea was to just use an IIO driver with a single
> > > channel with IIO_TIMESTAMP. We discuss this here and it seems
> > > controversial.
> > > 
> > > So my question to Jonathan is if we have a timestamp coming from the EC
> > > itself, can we consider this timestamp as a good IIO driver?
> > > 
> > > Any other idea is welcome, however Google team would like to manage
> > > only IIO drivers if possible.
> > > 
> > > Thanks  
> > 
> > Jonathan,
> > 
> > Should the the timestamp from the EC be introduced as an IIO driver
> > using IIO_TIMESTAMP?
> 
> It is is a rather odd driver but I suppose it would be fine with lots
> of clear docs on why it is how it is...
> 
> > 
> > Since there is no corresponding EC Counter driver in the baseline right
> > now we don't have a conflict yet. If the EC timestamp is introduced as
> > an IIO driver then we should make any future EC Counter driver mutually
> > exclusive with the IIO driver in order to prevent any memory space
> > conflict. At that point we may deprecate the IIO driver and move the
> > timestamp functionality to the corresponding Counter driver.
> 
> That route does become somewhat of a mess so I suspect we'd have to have
> a single driver supporting both userspace interfaces.  If you are happy
> that we'd be adding a bit of legacy to support for ever then we can go
> that way.

Generally I'd prefer all components of a device to be supported, but
if this is as Fabien suggests that due to the nature of this particular
device the counter value is of no interest, then a Counter driver is of
little practical use here. In this particular case, it seems better to
restrict the driver support to just the timestamp functionality that
will be used, rather than introduce extra code to expose values that
will likely be ignored and risk adding code to the kernel that becomes
unmaintained due to lack of exposure or interest.

William Breathitt Gray

> 
> > 
> > That's assuming someone is interested in the Count component enough to
> > implement an EC Counter driver; otherwise, the IIO driver will serve
> > just fine if timestamp is the only data desired from this device.
> > 
> > William Breathitt Gray
> 
> 