[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-iio
Subject:    Re: [PATCH v2 2/2] counter: Add support for Intel Quadrature Encoder Peripheral
From:       William Breathitt Gray <vilhelm.gray () gmail ! com>
Date:       2021-05-27 8:33:34
Message-ID: YK9ZXlKzfvj6P9f8 () shinobu
[Download RAW message or body]


On Thu, May 27, 2021 at 11:14:06AM +0300, Jarkko Nikula wrote:
> Hi
> 
> On 5/27/21 5:16 AM, William Breathitt Gray wrote:
> >> +What:		/sys/bus/counter/devices/counterX/signalY/invert
> >> +KernelVersion:	5.14
> >> +Contact:	linux-iio@vger.kernel.org
> >> +Description:
> >> +		Whether signal Y inversion is enabled. Valid attribute values
> >> +		are boolean.
> >> +
> >> +		For counter devices that have feature to control inversion of
> >> +		signal Y.
> > 
> > I want to understand this functionality a bit better. So, this device
> > has two quadrature encoder signals coming in (Phase A and Phase B) and
> > this "invert" option allows the user to configure the device to invert
> > these signals in hardware before before they are evaluated by the
> > quadrature encoding counter. Users are able to invert each signal
> > independent of the other; e.g. Phase A can be inverted, but Phase B can
> > be left alone. Is my understanding correct, or is the inversion applied
> > across all signals rather than just one independently?
> > 
> > What is the purpose of this functionality? Is this to allow users to
> > adjust the direction of the count without having to physically reorient
> > the encoding device (e.g. tracking counter-clockwise versus clockwise
> > movement)?
> > 
> Yes, it's independent for each signal. Here Phase A, B and Index.
> 
> According to specification idea is to remove need for board specific 
> inversion logic. Which makes me thinking this kind configuration should 
> come from firmware. As well as inputs swapped function. Which is for 
> correcting possible miswiring of Phase A and B signals on the board.
> 
> I'm now puzzled is there even need to have userspace visibility and 
> control for these signal inversions and input swapping? Of course yes 
> with my hacker hat on but for an ordinary Linux distribution point of 
> view those inversions and input swapping should be set by the kernel 
> automatically IMHO.
> 
> What do you think? Should I keep these sysfs attributes in the next 
> version or remove them? Though I don't have plans to add firmware data 
> this time. It's nice to save room for future contributions if there is a 
> real need for these features :-)

Hi Jarkko,

Right now I'm not sure whether this inversion functionality should be
exposed to userspace as sysfs attributes in this particular way just
yet. We should be careful in introducing new sysfs attributes because
once they're released we're stuck supporting them for the indefinite
future.

Because of these reasons, I think it's best for us to err on the side of
simplicity and postpone these extensions for the future if the need
arises; we can always add them later after this driver is stabilized and
merged. For now, let's remove these sysfs attributes from the next
version and keep it simple.

> >> +static struct counter_signal intel_qep_signals[] = {
> >> +	INTEL_QEP_SIGNAL(0, "Phase A", INTEL_QEPCON_EDGE_A),
> >> +	INTEL_QEP_SIGNAL(1, "Phase B", INTEL_QEPCON_EDGE_A),
> >> +	INTEL_QEP_SIGNAL(2, "Index", INTEL_QEPCON_EDGE_A),
> > 
> > Is INTEL_QEPCON_EDGE_A three times here correct, or did mean to use
> > INTEL_QEPCON_EDGE_B and INTEL_QEPCON_EDGE_INDX as well?
> > 
> What a facepalm... last minute "lets have a nice macro here and blind 
> copy-pasting" just before sending this out.

No worries, I can ensure you I've made the same mistake as well far more
times than I care to admit. ;-)

William Breathitt Gray

["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]