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;
MIME-Version: 1.0
References: <20200413195514.192868-1-gwendal@chromium.org> <20200414204814.GH7347@icarus>
In-Reply-To: <20200414204814.GH7347@icarus>
From:   Gwendal Grignou <gwendal@chromium.org>
Date:   Mon, 20 Apr 2020 11:54:16 -0700
Message-ID: <CAPUE2utas86PQdQem7bPsNL+xnHreepG8wbvbt2Vk5rtjoyn-A@mail.gmail.com>
Subject: Re: [PATCH v2] drivers: counter: Add Cros EC Sync counter
To:     William Breathitt Gray <vilhelm.gray@gmail.com>
Cc:     Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        Jonathan Cameron <jic23@kernel.org>,
        Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Apr 14, 2020 at 1:48 PM William Breathitt Gray
<vilhelm.gray@gmail.com> wrote:
>
> On Mon, Apr 13, 2020 at 12:55:14PM -0700, Gwendal Grignou wrote:
> > When the camera vsync pin is connected to the embedded controller (EC) of
> > a chromebook, the EC reports a sensor with a counter that increases
> > at each GPIO rising edge.
> >
> > The sensor is presented using the counter subsystem.
> > In addition, it is also presented via the IIO subsystem with a timestamp,
> > allowing synchronisation with sensors connected to the same EC, for
> > image stabilisation or augmented reality applications.
>
> Hi Gwendal,
>
> Sorry for the delay. I have some changes requested below.
>
> > To enable the counter:
> > via counter ABI:
> > echo "rising edge" > counterX/count0/signal_action
> > via iio ABI
> > echo 1 > iio:deviceY/en
> >
> > To disable the counter:
> > via counter ABI:
> > echo "none" > counterX/count0/signal_action
> > via iio ABI
> > echo 0 > iio:deviceY/en
>
> Although in theory a user could manually disable the actions for a
> Signal, this is a very roundabout way of actually disabling the Count.
> It's better to expose an "enable" attribute to allow the users to
> perform this functionality; for example:
>
> echo 0 > counterX/count0/enable
> echo 1 > counterX/count0/enable
>
> >
> > To read the current counter value:
> > via counter ABI:
> > cat counterX/count0/count
> > via iio ABI
> > cat iio:deviceY/in_count_raw
>
> I know we discussed this in the last review but it's still the same as
> before: IIO_COUNT interface is deprecated so new drivers won't be
> allowed to use it. You'll have to remove the IIO_COUNT code in this
> driver and replace it with Counter subsystem equivalents.
I understand the need of a clean separation between counter and IIO subsystems.
I will wait for counter to offer a way to gather timestamp'ed counts.
Do you have a plan/proposed ABI you can share?

Thanks,

Gwendal.