Re: [PATCH 2/2] counter: Add support for Intel Quadrature Encoder Peripheral

[Jarkko Nikula <jarkko.nikula@linux.intel.com>]

Hi

On 4/2/21 1:30 PM, William Breathitt Gray wrote:
> On Thu, Apr 01, 2021 at 06:32:28PM +0300, Jarkko Nikula wrote:
>> Add support for Intel Quadrature Encoder Peripheral found on Intel
>> Elkhart Lake platform.
>>
>> Initial implementation was done by Felipe Balbi while he was working at
>> Intel with later changes from Raymond Tan and me.
>>
>> Co-developed-by: Felipe Balbi (Intel) <balbi@kernel.org>
>> Signed-off-by: Felipe Balbi (Intel) <balbi@kernel.org>
>> Co-developed-by: Raymond Tan <raymond.tan@intel.com>
>> Signed-off-by: Raymond Tan <raymond.tan@intel.com>
>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> 
> Hello Jarkko,
> 
> Please see the questions and suggestions inline below.
> 
Thanks for review! I'll address them for the next version. Some comments 
and discussion below.

>> +static enum counter_synapse_action intel_qep_synapse_actions[] = {
> 
> This enum can be const too.
> 
>> +	[INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE] =
>> +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
>> +
>> +	[INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE] =
>> +	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
>> +};
> 
> Quadrature x4 mode is expected to evaluate on both edges on both phase
> signals. Shouldn't this array have COUNTER_SYNAPSE_ACTION_BOTH_EDGES?
> 
...
>> +static int intel_qep_action_get(struct counter_device *counter,
>> +				struct counter_count *count,
>> +				struct counter_synapse *synapse,
>> +				size_t *action)
>> +{
>> +	struct intel_qep *qep = counter_to_qep(counter);
>> +	u32 reg;
>> +
>> +	pm_runtime_get_sync(qep->dev);
>> +	reg = intel_qep_readl(qep, INTEL_QEPCON);
>> +	pm_runtime_put(qep->dev);
>> +
>> +	*action = (reg & synapse->signal->id) ?
>> +		INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE :
>> +		INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE;
> 
> I'm a bit confused about this quadrature encoding. Is this counting only
> one edge on each phase signal?
> 
...
> 
> Are you actually able to set the action mode for the phase signals? My
> expectation is that the action mode for the phase signals will always be
> "both edges" because the encoding is quadrature x4. What exactly is
> happening for the device when you write INTEL_QEPCON?
> 
You are right. I've overlooked this. Action is always both edges but 
what HW actually does here is an inversion control for each input signal 
so these should be then extensions I think.

 From the specification "An individually configurable Edge Select block 
allows control over rising or falling edge detection on each input pins, 
removing the need for platform logic inversion.".

>> +static ssize_t enable_write(struct counter_device *counter,
>> +			    struct counter_count *count,
>> +			    void *priv, const char *buf, size_t len)
>> +{
>> +	struct intel_qep *qep = counter_to_qep(counter);
>> +	u32 reg;
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = kstrtou32(buf, 0, &val);
> 
> "enable" is expected to handle boolean values so use kstrtobool here.
> 
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	mutex_lock(&qep->lock);
>> +	if (val && !qep->enabled) {
>> +		pm_runtime_get_sync(qep->dev);
>> +		reg = intel_qep_readl(qep, INTEL_QEPCON);
>> +		reg |= INTEL_QEPCON_EN;
>> +		intel_qep_writel(qep, INTEL_QEPCON, reg);
>> +		qep->enabled = true;
> 
> Are you missing pm_runtime_put() here?
> 
>> +	} else if (!val && qep->enabled) {
> 
> Are you missing pm_runtime_get_sync() here?
> 
Both are intented. Here pm_runtime_ calls are not only for accessing the 
registers like in other functions but keep the HW block on and be able 
to count when enabled. So after enabling the runtime PM usage count 
stays at least 1 and goes to zero only after disabling.

> If you have both val and qep->enabled as bool, you can make this logic
> clearer by evaluating their xor to determine if there's a change. So
> something like this:
> 
> 	bool changed = val ^ qep->enabled;
> 	if (!changed)
> 		return len;

Good point. Will check how does it look.

>> +static const struct counter_device_ext intel_qep_ext[] = {
>> +	INTEL_QEP_COUNTER_EXT_RW(noise),
>> +	INTEL_QEP_COUNTER_EXT_RW(preset_enable)
>> +};
> 
> "noise" is a new attribute so you'll need to provide an entry in
> Documentation/ABI/testing/sysfs-bus-counter explaining it.
> 
Should this noise actually be visible as seconds (ns, µs, ms) instead of 
plain register value? From the spec "It selects the maximum glitch width 
to remove in terms of peripheral clock cycles: PCLK_CYCLES = MAX_COUNT + 2".

I think it should since for a person configuring the system plain 
register value is more or less guesswork.

Jarkko