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 X-Spam-Level: X-Spam-Status: No, score=-10.7 required=3.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 079FCC433E0 for ; Fri, 12 Feb 2021 06:33:49 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 8469C64DEE for ; Fri, 12 Feb 2021 06:33:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8469C64DEE Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type:Cc: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=9ObtnHaUo2/FYfAjS5a0UR8kjvxees+zBxtlXjzovUs=; b=epDV1QUfHiqLCa2V+jqzQz3Yu hGmzr4KIrOgvB4mVdaUeEEhXeG7aW14iVOaNvghH1Oopxdgx2lXgOPjorpMvjymSdZG2psCP9NeSn ORjGWSiifOAJWfk1Wx0eH9dmzDzioyO7hT4AcqojBq9BbysZOtSdggFiGEnFpiO8IGHxS0z2NZR/k 29yfyUFiw/17LRj4+qz0iLL2meiS7VrOfdTFurAUaOtwS38/0zgx0mSaq+tDNxn63NUHIAcxt4wGi plqncObEQnnWzphUV3Vp0Bx/DHaInuW4YJrB3o3cZ3oU7xOfjhWhuka/yDqQQWtSXUS/NWjQ31amV was6j8sng==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lAS0B-0002vB-E5; Fri, 12 Feb 2021 06:32:27 +0000 Received: from mail-qt1-x833.google.com ([2607:f8b0:4864:20::833]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1lAS09-0002uj-5b for linux-arm-kernel@lists.infradead.org; Fri, 12 Feb 2021 06:32:26 +0000 Received: by mail-qt1-x833.google.com with SMTP id v3so6051184qtw.4 for ; Thu, 11 Feb 2021 22:32:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=YpdyqTTIfUaE/xKGvquExFqY3IIyGF7XvCEZZkDqm0w=; b=PEH3n7H2JHE0OeIyomMC7MNrnsAxMbGOwkycCmXW/v3qNAp3iTb4TfnNqEoN8jyLWr mfJ8wNJQ4B9+ZCZToBFIQdiCoKaxFPboLOXn98qaa9v7HnkLNBMpPitjD2CrgPyStlID sQsB9255hKAk/1JqRwZ3x1c7qQdG42kykUjQydNQrYVByw+IH0ApseMNrUG/7kGyTKsj BfkvbnEJg2GAbj6dOU/2uQk94Fz9HbA+Bzl0VvFlE2NXsuyikuHMkP7harjuy/CT3sxi zpr7WqIFGxCdDj25y73SLgIC/Lt/JJPogNXF4N2K5Ntv+8pxh88/fGwXWX9GJxX+emy/ j6qA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=YpdyqTTIfUaE/xKGvquExFqY3IIyGF7XvCEZZkDqm0w=; b=o0xRPhpKbXEouz6x2tPQlwbKcBZMrnrUfVScCpf5DzPjbFUmo9YKJX5q4ZcIclIsWG bw9QsyqL/9tTdBLNdUzFjV9aQ29FX1arJJ7RBDtPLkPTXXZ8RZDBRlB6febNMxBt7HEo RJ6Le05TdlByR49V9ieR5T0ywT1W4g3bCmKkTorwxPmIERcW4IlU3K7u6ublrjfqPrXE 3xzXrZZDIOSTGaIVq/cKP8RKdWWY1Cpx2kkjJUU1NqBN1RqPo4+ZQP0DAZIH5iTUbTrp myyB35ds3rUQVc/YJArJKxxL0TWZGP/hsYQDeZXW5aTbSeO9F7i20pMH1h+QJDnbyLDY ddQQ== X-Gm-Message-State: AOAM532EK0AmYZIVBsP5BMYLy6Ku1no7bji7lU87BJT6L+mwv9Ew5QE9 6E6gXVB7Uvu8CFrzLGncuJA= X-Google-Smtp-Source: ABdhPJykoum8SlFYEq4qIDiUVmjCNungJ8LPedsQkCkS86BCab+ez01mnwN1qgig72D9vAhu2LiZVQ== X-Received: by 2002:ac8:1494:: with SMTP id l20mr1260768qtj.151.1613111542162; Thu, 11 Feb 2021 22:32:22 -0800 (PST) Received: from shinobu ([193.27.12.132]) by smtp.gmail.com with ESMTPSA id v30sm5016239qte.26.2021.02.11.22.32.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 11 Feb 2021 22:32:21 -0800 (PST) Date: Fri, 12 Feb 2021 15:32:12 +0900 From: William Breathitt Gray To: Jonathan Cameron Subject: Re: [PATCH v7 3/5] counter: Add character device interface Message-ID: References: <57bc509273bf288d74835e6ebdaebf27b4991888.1608935587.git.vilhelm.gray@gmail.com> <20201230150440.0723cab9@archlinux> MIME-Version: 1.0 In-Reply-To: <20201230150440.0723cab9@archlinux> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210212_013225_334266_3EF2B35C X-CRM114-Status: GOOD ( 35.18 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kamel.bouhara@bootlin.com, gwendal@chromium.org, a.fatoum@pengutronix.de, david@lechnology.com, linux-iio@vger.kernel.org, patrick.havelange@essensium.com, alexandre.belloni@bootlin.com, mcoquelin.stm32@gmail.com, linux-kernel@vger.kernel.org, Dan Carpenter , kernel@pengutronix.de, fabrice.gasnier@st.com, syednwaris@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, alexandre.torgue@st.com Content-Type: multipart/mixed; boundary="===============4177399706052516422==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============4177399706052516422== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="nmunDh9PKWC61MXK" Content-Disposition: inline --nmunDh9PKWC61MXK Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Dec 30, 2020 at 03:04:40PM +0000, Jonathan Cameron wrote: > On Fri, 25 Dec 2020 19:15:36 -0500 > William Breathitt Gray wrote: >=20 > > This patch introduces a character device interface for the Counter > > subsystem. Device data is exposed through standard character device read > > operations. Device data is gathered when a Counter event is pushed by > > the respective Counter device driver. Configuration is handled via ioctl > > operations on the respective Counter character device node. > >=20 > > Cc: David Lechner > > Cc: Gwendal Grignou > > Cc: Dan Carpenter > > Signed-off-by: William Breathitt Gray >=20 > There are a few things in here that could profitably be pulled out as pre= cursor > patches. I don't really understand the connection of extension_name to t= he > addition of a chardev for example. Might be needed to provide enough > info to actually use the chardev, but does it have meaning without that? > Either way, definitely feels like it can be done in a separate patch. The extension_name attributes are needed so chrdev users have enough info to identify which extension number corresponds to which extension. I'll move this to change to a separate patch and provide an appropriate explanation there to make things clearer. > > +static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd, > > + unsigned long arg) > > +{ > > + struct counter_device *const counter =3D filp->private_data; > > + unsigned long flags; > > + int err =3D 0; > > + > > + switch (cmd) { > > + case COUNTER_CLEAR_WATCHES_IOCTL: > > + return counter_clear_watches(counter); > > + case COUNTER_ADD_WATCH_IOCTL: > > + return counter_add_watch(counter, arg); > > + case COUNTER_LOAD_WATCHES_IOCTL: > > + raw_spin_lock_irqsave(&counter->events_list_lock, flags); > > + > > + counter_events_list_free(&counter->events_list); > > + list_replace_init(&counter->next_events_list, > > + &counter->events_list); > > + > > + if (counter->ops->events_configure) > > + err =3D counter->ops->events_configure(counter); > > + > > + raw_spin_unlock_irqrestore(&counter->events_list_lock, flags); > > + break; >=20 > return here.=20 Ack. > > +static int counter_get_data(struct counter_device *const counter, > > + const struct counter_comp_node *const comp_node, > > + u64 *const value) > > +{ > > + const struct counter_comp *const comp =3D &comp_node->comp; > > + void *const parent =3D comp_node->parent; > > + int err =3D 0; > > + u8 value_u8 =3D 0; > > + u32 value_u32 =3D 0; > > + > > + if (comp_node->component.type =3D=3D COUNTER_COMPONENT_NONE) > > + return 0; > > + > > + switch (comp->type) { > > + case COUNTER_COMP_U8: > > + case COUNTER_COMP_BOOL: > > + switch (comp_node->component.scope) { > > + case COUNTER_SCOPE_DEVICE: > > + err =3D comp->device_u8_read(counter, &value_u8); > > + break; > > + case COUNTER_SCOPE_SIGNAL: > > + err =3D comp->signal_u8_read(counter, parent, &value_u8); > > + break; > > + case COUNTER_SCOPE_COUNT: > > + err =3D comp->count_u8_read(counter, parent, &value_u8); > > + break; > > + } > > + *value =3D value_u8; > > + break; > > + case COUNTER_COMP_SIGNAL_LEVEL: > > + case COUNTER_COMP_FUNCTION: > > + case COUNTER_COMP_ENUM: > > + case COUNTER_COMP_COUNT_DIRECTION: > > + case COUNTER_COMP_COUNT_MODE: > > + switch (comp_node->component.scope) { > > + case COUNTER_SCOPE_DEVICE: > > + err =3D comp->device_u32_read(counter, &value_u32); > > + break; > > + case COUNTER_SCOPE_SIGNAL: > > + err =3D comp->signal_u32_read(counter, parent, > > + &value_u32); > > + break; > > + case COUNTER_SCOPE_COUNT: > > + err =3D comp->count_u32_read(counter, parent, &value_u32); > > + break; > > + } > > + *value =3D value_u32; >=20 > Seems like a return here would make more sense as no shared stuff to do at > end of the switch. Same in other similar cases. Ack. > > + break; > > + case COUNTER_COMP_U64: > > + switch (comp_node->component.scope) { > > + case COUNTER_SCOPE_DEVICE: > > + return comp->device_u64_read(counter, value); > > + case COUNTER_SCOPE_SIGNAL: > > + return comp->signal_u64_read(counter, parent, value); > > + case COUNTER_SCOPE_COUNT: > > + return comp->count_u64_read(counter, parent, value); > > + } > > + break; > > + case COUNTER_COMP_SYNAPSE_ACTION: > > + err =3D comp->action_read(counter, parent, comp->priv, > > + &value_u32); > > + *value =3D value_u32; > > + break; > > + } > > + > > + return err; > > +} > > + > > +/** > > + * counter_push_event - queue event for userspace reading > > + * @counter: pointer to Counter structure > > + * @event: triggered event > > + * @channel: event channel > > + * > > + * Note: If no one is watching for the respective event, it is silently > > + * discarded. > > + */ > > +void counter_push_event(struct counter_device *const counter, const u8= event, > > + const u8 channel) > > +{ > > + struct counter_event ev =3D {0}; > > + unsigned int copied =3D 0; > > + unsigned long flags; > > + struct counter_event_node *event_node; > > + struct counter_comp_node *comp_node; > > + > > + ev.timestamp =3D ktime_get_ns(); > > + ev.watch.event =3D event; > > + ev.watch.channel =3D channel; > > + > > + raw_spin_lock_irqsave(&counter->events_list_lock, flags); >=20 > For a raw spin lock, we definitely want to see comments on why it > is necessary. Ack. > > @@ -650,7 +670,7 @@ static int counter_count_attrs_create(struct counte= r_device *const counter, > > return err; > > =20 > > /* Create Count name attribute */ > > - err =3D counter_name_attr_create(dev, group, count->name); > > + err =3D counter_name_attr_create(dev, group, "name", count->name); >=20 > This refactoring could also be pulled out to a precusor patch. Ack. This will be part of the extension_name patch. > > @@ -319,12 +315,21 @@ struct counter_device { > > =20 > > int id; > > struct device dev; > > + struct cdev chrdev; > > + struct list_head events_list; > > + raw_spinlock_t events_list_lock; > > + struct list_head next_events_list; > > + DECLARE_KFIFO(events, struct counter_event, 64); >=20 > Why 64? Probably want that to be somewhat dynamic, even if only at build= time. Ack. This will be dynamically configurable via sysfs attribute in v8. > > + wait_queue_head_t events_wait; > > + struct mutex events_lock; > > }; > > =20 > > int counter_register(struct counter_device *const counter); > > void counter_unregister(struct counter_device *const counter); > > int devm_counter_register(struct device *dev, > > struct counter_device *const counter); > > +void counter_push_event(struct counter_device *const counter, const u8= event, > > + const u8 channel); > > =20 > > #define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \ > > { \ > > diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h > > new file mode 100644 > > index 000000000000..7585dc9db19d > > --- /dev/null > > +++ b/include/uapi/linux/counter.h > Small thing but I would have been tempted to do a precursor patch to the > main change simply putting in place the userspace header. >=20 > Classic Nop patch that makes it easier to focus on the real stuff in this > patch by getting that noise out of the way! >=20 > Jonathan Ack. William Breathitt Gray --nmunDh9PKWC61MXK Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEk5I4PDJ2w1cDf/bghvpINdm7VJIFAmAmIOwACgkQhvpINdm7 VJLTlw//cJaxYjnD+n4pnRnze6CezjGL0dEENF8QzlY3ren7gJXOTJgkwHVWzYd5 sq7WaVSZUoxkug07sYTckZQq2mohvTQQE6BhUv51lM+rkkoVhnvMzDWdB3ygyur9 d+rssNdhUMg0RrXdxRi4gH0hh23K1SQAI5/Choecz1YoGPMCSBh+0zTOkJ+UDszN hjaYmdVGnc2hxMuoZLoM0twTkmTJWXyvHRQqCbDgWlc4cUEyziTjldqyklJTSSqK ovJxjA6NXmEh3/zWJeFwKLsnYe+eqsv3S9CHg6B+gVWnIiE7VyDkv6vh2s2wE7cg r67ebRddj7MBqrAq34G6xN4W0wqhWSAQZ4xUYSRzIgzUwnAl1umZb2Qi5Un4bPtk BHNCYB6GnBLETxmIRSUDLMbaUe8XJjQXc7ogLc8qimN4oRmN4fByhuiKM5wirCAv jq6UXRPHv1SI+00SgzVj0yPKPVFv5raZvEjwErekjNFoq7JNhdWkZ59eS0xHwLNZ 82TbRtwZvM78g+fHFCTgcgAkk2l3cJ4Kx/PNJ/J6kb+8ahTMtd89K6S0hZ/w6zCo xCxYYV6q5FXQMr6siKpCxyI4fGORc7vqlPX8xnZJi/aREc4cVhKxGEWPqp8yqRkG zVots+ppRy1D262LDn7OBsAqg52fLigQDw+WX+t3o/GuVaUBzRE= =x9JT -----END PGP SIGNATURE----- --nmunDh9PKWC61MXK-- --===============4177399706052516422== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============4177399706052516422==--