From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 20171C4332F for ; Mon, 17 Oct 2022 17:08:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=0mYy7DWcVnwgkg3UO1Ym72271Mpp1ADlekjA78ezmB0=; b=w2HTLGDWDKip2K slS05bEs1w6dYloLwWqWNOMmpp0n9fqs3QCcoFA1euBusM2h0B/bL1K/7xC84OblKgjj4CPjVwZjb JvBO5fMWzxMvhhdNyj2ghtp5K8sl21Tn3zCiFRne8zz1mHFcwpthOkKBBcsI68aNeMdinQLeHxYQc UzpaZmAq1/tWEyUQbiBprnOAhcNL7lyEKASw0DWDfU/vEqK0LRvdhe++k3YtoRaH37gM+KS4Z5zkB Ot+cI1AhVtL6a6QfISukhTy6zykbndWWPB9r+IBFf+MS7pSGuIRxE71HhvmoZlR/2yOYturOz1LtQ 3t2xTvsI7Y2kGQWyqnVA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1okTZz-00Dt4g-K7; Mon, 17 Oct 2022 17:07:07 +0000 Received: from relay11.mail.gandi.net ([217.70.178.231]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1okTZS-00DsxY-5Y for linux-arm-kernel@lists.infradead.org; Mon, 17 Oct 2022 17:06:36 +0000 Received: (Authenticated sender: kamel.bouhara@bootlin.com) by mail.gandi.net (Postfix) with ESMTPSA id 14411100004; Mon, 17 Oct 2022 17:06:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1666026388; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=kpNIdAz1588XyIs1IKXrVbeF6UQl3z3oJgLRUJ+9sWk=; b=Cmv7/OsKkBt6pn3P9946UjW4XsrKIoqSk7GiVbcG1iPhkOLFR63Rx40H6XRwOb8o8c0IRb AtB08+gCPuQWqQ4KJf2PYrO/N5KBt6rZY9h0X/6nwG6hRLfAqXAdu6yvsTWPKdNUmfnv7V a8sxLBUGlR/Wtn+z/A8QFw0syzhCnQbrMlZtxzlWmkY0S/C7CwQxbCO95I/d3/a9NXnRNW 2VPfnsMeWVaBTTSgfm83ckqNzzQHWGbqyu4qcOU8Lbi5QIiDWVQgWzrSdSOXjrZ6oHHScx iXbpANL/qHYtB1DhKtMKtzOX5G1ofC25PKQvi6lONsYivoNcW/rhrp+6Y06TVQ== Date: Mon, 17 Oct 2022 19:06:26 +0200 From: Kamel Bouhara To: William Breathitt Gray Cc: linux-arm-kernel@lists.infradead.org, linux-iio@vger.kernel.org Subject: Re: Handling Signal1 in microchip-tcb-capture Message-ID: References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221017_100634_636625_487CEDFE X-CRM114-Status: GOOD ( 35.62 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Oct 17, 2022 at 08:53:18AM -0400, William Breathitt Gray wrote: > On Mon, Oct 17, 2022 at 11:59:37AM +0200, Kamel Bouhara wrote: > > On Sat, Oct 15, 2022 at 09:52:27AM -0400, William Breathitt Gray wrote: > > > I was looking over the microchip-tcb-capture driver recently and noticed > > > that the code doesn't seem to account for Signal1. In particular, it > > > appears that mchp_tc_count_signal_read() and mchp_tc_count_action_read() > > > don't check the Signal id at all and just assume they are handling > > > Signal0. This creates a situation where the information returned for the > > > Signal1 sysfs attributes are just duplicated reports of Signal0. > > > > > > What exactly is the relationship of Signal0 ("Channel A") and Signal1 > > > ("Channel B"); is SignalB only relevant when the counter device is > > > configured for quadrature mode? > > > > Indeed both signals are required when in quadrature mode, where the > > signal0 is representing the speed and signal1 the revolution or number > > of rotation. > > > > We have described all availables modes in details in the following blog post: https://bootlin.com/blog/timer-counters-linux-microchip/ > > > > Regards, > > Kamel > > Thank you for the link, the block diagram helps illustrate how the > signals correlate to the TCB channels. > > Let me check if I understand correctly. In microchip-tcb-capture.c, > mchp_tc_count_signals[0] is TIOA0 while mchp_tc_count_signals[1] is > TIOB0? In quadrature mode, are TIOA and TIOB the two phases of a > quadrature encoder? You mentioned one signal is speed while the other is > the number of rotations; does this mean one signal serves as the > position incrementation from a rotary wheel while the other signal is > the index (z-phase) indicate for each full rotation? > IIRC this is indeed both signal edges (phase A and B) are accumulated on channel 0 and channel 1 stores the revolution or number of rotation of the qdec encoder. > In particular, I'm having trouble understanding > mchp_tc_count_signal_read(). I suspect it is unintentionally always > returning the signal status for TIOA:: > > regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr); > > if (priv->trig_inverted) > sigstatus = (sr & ATMEL_TC_MTIOB); > else > sigstatus = (sr & ATMEL_TC_MTIOA); > > *lvl = sigstatus ? COUNTER_SIGNAL_LEVEL_HIGH : COUNTER_SIGNAL_LEVEL_LOW; > > Here we read the status register for channel 0, select between TIOA and > TIOB based on priv->trig_inverted, and then return the signal level. > > I don't see priv->trig_inverted referenced anywhere else so it appears > that priv->trig_inverted will always be 0, thus resulting in > mchp_tc_count_signal_read() always returning the TIOA status. I think > the intended behavior is to return the status of the selected signal:: IIRC the trig_inverted shall be used when signals are inverted which means we read position on TIOB and revolution on TIOA. > > if (signal->id == 1) > sigstatus = (sr & ATMEL_TC_MTIOB); > else > sigstatus = (sr & ATMEL_TC_MTIOA); > > As for mchp_tc_count_action_read(), we have a similar problem: no > distinction is made for the Synapse requested. The channel mode register > for channel 0 is read and then masked against ATMEL_TC_ETRGEDG to > determine the action mode. It appears that this code is always assuming > the Synapse for TIOA is requested, but the Synapse for TIOB could be > passed. You can determine which corresponding Signal you have by > checking synapse->signal->id before deciding what action mode to return. > That is indeed a good point as both signals are eligible to trigger the TC for both modes (capture/qdec). > To clarify, in COUNTER_FUNCTION_INCREASE mode, does the Count value > increment based on the edge of TIOA and not TIOB? In Yes, currently the driver only support TIOA. > COUNTER_FUNCTION_QUADRATURE_X4 mode, does the Count value increment > based on both edges of TIOA and TIOB serving as quadrature encoding > phase A and B signals? Yes as explained above. > > The fixes for this issue are trivial enough that I can submit a patch > for them later, but I want to make sure I'm understanding the nature of > these signals correctly before I do so. > > Thanks, > > William Breathitt Gray -- Kamel Bouhara, Bootlin Embedded Linux and kernel engineering https://bootlin.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel