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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A751ECAAD8 for ; Fri, 23 Sep 2022 01:13:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231754AbiIWBNs (ORCPT ); Thu, 22 Sep 2022 21:13:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36954 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232303AbiIWBNp (ORCPT ); Thu, 22 Sep 2022 21:13:45 -0400 Received: from mail-oa1-x2f.google.com (mail-oa1-x2f.google.com [IPv6:2001:4860:4864:20::2f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AA82E1166DC for ; Thu, 22 Sep 2022 18:13:33 -0700 (PDT) Received: by mail-oa1-x2f.google.com with SMTP id 586e51a60fabf-1225219ee46so16389484fac.2 for ; Thu, 22 Sep 2022 18:13:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date; bh=2BBAQBVpQ8fXKmes7SQw9/PDpetGobnws+Vor3/B+A0=; b=V1yvgkxoyU/dThLL9TTrsz8DBaGhI/bZIBapBaw5bStsYTbBrSlbORj/zYoEKopoz8 cvRGXpv9D3hFZzdHPeKXQX+wUNOtINR1v7flrGgfyWcjXUkAi9vguzc6dybjHcXxASH2 qPi/DVTY2Sdp9+cnzaXsYASKeDi/vtfO5dSCCDPXlqjeedSXuCw0Q3ult302E/KOmZ4r rQB8SPV8YM3NkRSM3R3YkUnnH88FrX5DhnHYxrzkNOIv/pjEKIBVlf1arCOTiyVETyN3 mu9CdaK13/N1cyb3D+GYjxGPj5s9/0M4sfkbfAT+tXiNxywf9Xc8UsuAB5fucbAbHQxL PxgA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date; bh=2BBAQBVpQ8fXKmes7SQw9/PDpetGobnws+Vor3/B+A0=; b=bd2JU9ywCdm3iUoLeAU2gSFIVseMsUa6Zv3sNtRZu3SFw4PmMZ0qeencHKmS+CAaT5 KajPjMFSPPcgRAgaMLqPcDY/yVEaqWXHzyM5VFmCY1rTRLp3fSHwyFtYgbu8LcRYLuq9 uGYLCIq/H0YKmipKtvk6A8ElODTaeKVOtW5xIT3NO46yXhlIwdCaTj9k57uJs00alIMl PAFjGbLdsN5WhUTXtAL+IhRDIs3kchPDobe7eQMdw78S86SmnKn8J/rKsWL2P2x3xvS2 Ucfs4ek7NdJtCy+eKlxpRAc6dnwNaqOx6nynjLqbbcFGE4H9lqX7XdHth5Sj5vH760aK ZPxQ== X-Gm-Message-State: ACrzQf3klWQj0g3OF92iirsrRJmeJZ9gffdaWQrl4tNwTR51rcsVI+wH nYbnvt0GpaWUgQFwZUfETpi/Ng== X-Google-Smtp-Source: AMsMyM4y72P7l45Eoh9rVYWaDDqTIPviU+AVVtECUCPHTW7UrA53sCd5HY2mhu2iysRQXSICYV7aOQ== X-Received: by 2002:a05:6870:639e:b0:127:bbc2:223e with SMTP id t30-20020a056870639e00b00127bbc2223emr3665823oap.167.1663895612843; Thu, 22 Sep 2022 18:13:32 -0700 (PDT) Received: from fedora (69-109-179-158.lightspeed.dybhfl.sbcglobal.net. [69.109.179.158]) by smtp.gmail.com with ESMTPSA id i21-20020a056820013500b00432ac97ad09sm2780618ood.26.2022.09.22.18.13.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 22 Sep 2022 18:13:32 -0700 (PDT) Date: Thu, 22 Sep 2022 21:08:05 -0400 From: William Breathitt Gray To: Julien Panis Cc: robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, mranostay@ti.com Subject: Re: [PATCH v8 3/4] counter: ti-ecap-capture: capture driver support for ECAP Message-ID: References: <20220922170402.403683-1-jpanis@baylibre.com> <20220922170402.403683-4-jpanis@baylibre.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="8oIR1Oj1QlTXqeTr" Content-Disposition: inline In-Reply-To: <20220922170402.403683-4-jpanis@baylibre.com> Precedence: bulk List-ID: X-Mailing-List: linux-iio@vger.kernel.org --8oIR1Oj1QlTXqeTr Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Sep 22, 2022 at 07:04:01PM +0200, Julien Panis wrote: > ECAP hardware on TI AM62x SoC supports capture feature. It can be used > to timestamp events (falling/rising edges) detected on input signal. >=20 > This commit adds capture driver support for ECAP hardware on AM62x SoC. >=20 > In the ECAP hardware, capture pin can also be configured to be in > PWM mode. Current implementation only supports capture operating mode. > Hardware also supports timebase sync between multiple instances, but > this driver supports simple independent capture functionality. >=20 > Signed-off-by: Julien Panis Hello Julien, Comments follow inline below. > +/** > + * struct ecap_cnt_dev - device private data structure > + * @enabled: device state > + * @clk: device clock > + * @regmap: device register map > + * @nb_ovf: number of overflows since capture start > + * @pm_ctx: device context for PM operations > + */ > +struct ecap_cnt_dev { > + bool enabled; > + struct clk *clk; > + struct regmap *regmap; > + atomic_t nb_ovf; > + struct { > + u8 ev_mode; > + u32 time_cntr; > + } pm_ctx; > +}; Provide documentation for the ev_mode and time_cntr members. You probably need a lock as well to protect access to this structure or you'll end up with race problems. > +static void ecap_cnt_capture_enable(struct counter_device *counter) > +{ > + struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter); > + > + pm_runtime_get_sync(counter->parent); > + > + /* Enable interrupts on events */ > + regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, > + ECAP_EVT_EN_MASK, ECAP_EVT_EN_MASK); > + > + /* Run counter */ > + regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_CFG_MAS= K, > + ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK); > +} > + > +static void ecap_cnt_capture_disable(struct counter_device *counter) > +{ > + struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter); > + > + /* Disable interrupts on events */ > + regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, ECAP_EVT_EN= _MASK, 0); > + > + /* Stop counter */ > + regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_EN_MASK= , 0); Shouldn't the counter be stopped before stopping the interrupts? > +static int ecap_cnt_count_get_val(struct counter_device *counter, unsign= ed int reg, u32 *val) > +{ > + struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter); > + unsigned int regval; > + > + pm_runtime_get_sync(counter->parent); > + regmap_read(ecap_dev->regmap, reg, ®val); > + pm_runtime_put_sync(counter->parent); > + > + *val =3D regval; > + > + return 0; > +} > + > +static int ecap_cnt_count_set_val(struct counter_device *counter, unsign= ed int reg, u32 val) > +{ > + struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter); > + > + pm_runtime_get_sync(counter->parent); > + regmap_write(ecap_dev->regmap, reg, val); > + pm_runtime_put_sync(counter->parent); > + > + return 0; > +} The ecap_cnt_count_get_val() and ecap_cnt_count_set_val() functions only ever return 0. Redefine them as void functions and eliminate the unnecessary returns. > +static int ecap_cnt_count_write(struct counter_device *counter, > + struct counter_count *count, u64 val) > +{ > + struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter); > + > + if (ecap_dev->enabled) > + return -EBUSY; You should return -EBUSY when the requested operation cannot be completed because the device currently performing a task -- i.e. the requested operation would stall or otherwise fail if forced. In this case, the count value actually can be set while the device is enabled, if I'm not mistaken; the count just continues increasing from the new written value (i.e. no stall/failure). Therefore, there's not need to return -EBUSY here and this check can be eliminated. > +static int ecap_cnt_pol_write(struct counter_device *counter, > + struct counter_signal *signal, > + size_t idx, enum counter_signal_polarity pol) > +{ > + struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter); > + > + if (ecap_dev->enabled) > + return -EBUSY; I suspect this check can go away for the same reason as above. > +static int ecap_cnt_cap_write(struct counter_device *counter, > + struct counter_count *count, > + size_t idx, u64 cap) > +{ > + struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter); > + > + if (ecap_dev->enabled) > + return -EBUSY; Same comment as above. > +static int ecap_cnt_nb_ovf_write(struct counter_device *counter, > + struct counter_count *count, u64 val) > +{ > + struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter); > + > + if (ecap_dev->enabled) > + return -EBUSY; Same comment as above. > +static struct counter_count ecap_cnt_counts[] =3D { > + { > + .id =3D 0, The id member is for differentiating between multiple Counts. You only have one Count in this driver so you don't need to set it because you never use it. William Breathitt Gray --8oIR1Oj1QlTXqeTr Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQSNN83d4NIlKPjon7a1SFbKvhIjKwUCYy0G9QAKCRC1SFbKvhIj KxouAQCf2abb4CAphtgGkhQHIiAbAAbJAjCL1/3/eLHnBeBuwgEAl3EJhEc2Xmrp jmOjd5jNtDVQnvB+NsoZnflvSxpU+gQ= =XTZc -----END PGP SIGNATURE----- --8oIR1Oj1QlTXqeTr--