Received: by 2002:a05:6a10:8c0a:0:0:0:0 with SMTP id go10csp4434188pxb; Sun, 14 Feb 2021 10:09:08 -0800 (PST) X-Google-Smtp-Source: ABdhPJzlfm2fv8B/KPae1er5VgkEYaHM8IDg7eo6Tc/xY+7AEbJNYqmYbK6Jvrw98XMNODHPdkpI X-Received: by 2002:a17:906:4c85:: with SMTP id q5mr12189435eju.375.1613326147766; Sun, 14 Feb 2021 10:09:07 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1613326147; cv=none; d=google.com; s=arc-20160816; b=mSx6MD+VXEkwt51sY4yS1Wtl/Ab2nc6EWk6+7OtREZX4IXTpk+jsiAT2kKljMU2pYt FBwO3hy46suwVb9uAwlt0Xx54qgj13jl9d4JNSLaGOuSb5cO3JSqioHagu57+iEzWoz8 HGWX5V88p6G0zo1cgLySpTdUZK3xKPX8ydg4bFNF95AkZEtfJ1k2iWjFm52hB/FPKZlD 23+17SZZtx4Erm1FxZqeYsqcM7R6xJODM7/RK9MaHtQJ/pFdOKKiLDw61PqpiIzHaelR aixxNPWBDHq5RQam+I2WPCSe2TXc+AXvKmnl5o5eCKKN+jGH7Dl/Cv0/MCeCpPJJx7ax 94YA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:subject:cc:to:from:date; bh=7fti2tGE0StZvWUTR/UAQ8lXBzZO6w8gawJAdpXSSSU=; b=TAkUsgu8AHTDX7yLRRRDmkHK5SmbOz/YWKH5CxeZ6aeQiot5fVEMg1+PKZiiEAGVbN 362+9slNVOM9rw8lrKpAxD05PlhPW3gGw414x29jwTUK5CGGbG7gnxcDLXu3TEYlyklz xMSAEsUHr5X35MDUBm20QPY36k5jZWVtYo9VQwLGU1b0zftjq3t7S6UoltYxUkwkmi9P 9mBV4oRs4raxpZIcuDY/88ikwZ06n1X+z0G8ui1FOETPBlBo+NmBOs43tJ22eyUSdn5R gqGh8cm2I1fnrWQSjNnpquXhTQo1GSxm8GIueKQC5qGT0CaqNniZBYzWeK0lklBiRfc2 tJcA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id v2si10278335ejy.301.2021.02.14.10.08.44; Sun, 14 Feb 2021 10:09:07 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229837AbhBNSHA (ORCPT + 99 others); Sun, 14 Feb 2021 13:07:00 -0500 Received: from mail.kernel.org ([198.145.29.99]:54772 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229768AbhBNSG7 (ORCPT ); Sun, 14 Feb 2021 13:06:59 -0500 Received: from archlinux (cpc108967-cmbg20-2-0-cust86.5-4.cable.virginm.net [81.101.6.87]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 6BFAC64E4E; Sun, 14 Feb 2021 18:06:15 +0000 (UTC) Date: Sun, 14 Feb 2021 18:06:12 +0000 From: Jonathan Cameron To: William Breathitt Gray Cc: kernel@pengutronix.de, linux-stm32@st-md-mailman.stormreply.com, a.fatoum@pengutronix.de, kamel.bouhara@bootlin.com, gwendal@chromium.org, alexandre.belloni@bootlin.com, david@lechnology.com, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, syednwaris@gmail.com, patrick.havelange@essensium.com, fabrice.gasnier@st.com, mcoquelin.stm32@gmail.com, alexandre.torgue@st.com, o.rempel@pengutronix.de, Dan Carpenter Subject: Re: [PATCH v8 17/22] counter: Add character device interface Message-ID: <20210214180612.03af6f0d@archlinux> In-Reply-To: <720278e3aaf3f249657ec18d158eca3f962baf8e.1613131238.git.vilhelm.gray@gmail.com> References: <720278e3aaf3f249657ec18d158eca3f962baf8e.1613131238.git.vilhelm.gray@gmail.com> X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 12 Feb 2021 21:13:41 +0900 William Breathitt Gray wrote: > 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. > > Cc: David Lechner > Cc: Gwendal Grignou > Cc: Dan Carpenter > Cc: Oleksij Rempel > Signed-off-by: William Breathitt Gray Hi William, A few minor comments. Mostly seems to have come together well and makes sense to me. Jonathan > --- > drivers/counter/Makefile | 2 +- > drivers/counter/counter-chrdev.c | 496 +++++++++++++++++++++++++++++++ > drivers/counter/counter-chrdev.h | 16 + > drivers/counter/counter-core.c | 37 ++- > include/linux/counter.h | 45 +++ > include/uapi/linux/counter.h | 70 +++++ > 6 files changed, 661 insertions(+), 5 deletions(-) > create mode 100644 drivers/counter/counter-chrdev.c > create mode 100644 drivers/counter/counter-chrdev.h > ... > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c > index bcf672e1fc0d..c137fcb97d9c 100644 > --- a/drivers/counter/counter-core.c > +++ b/drivers/counter/counter-core.c > @@ -5,12 +5,16 @@ > */ > #include > #include > +#include > #include > +#include > #include > #include > #include > #include > +#include > > +#include "counter-chrdev.h" > #include "counter-sysfs.h" > > /* Provides a unique ID for each counter device */ > @@ -33,6 +37,8 @@ static struct bus_type counter_bus_type = { > .name = "counter" > }; > > +static dev_t counter_devt; > + > /** > * counter_register - register Counter to the system > * @counter: pointer to Counter to register > @@ -54,7 +60,6 @@ int counter_register(struct counter_device *const counter) > if (counter->id < 0) > return counter->id; > > - /* Configure device structure for Counter */ Not sure why this comment gets removed here. > dev->type = &counter_device_type; > dev->bus = &counter_bus_type; > if (counter->parent) { > @@ -65,18 +70,25 @@ int counter_register(struct counter_device *const counter) > device_initialize(dev); > dev_set_drvdata(dev, counter); > > + /* Add Counter character device */ > + err = counter_chrdev_add(counter, counter_devt); > + if (err < 0) > + goto err_free_id; > + > /* Add Counter sysfs attributes */ > err = counter_sysfs_add(counter); > if (err < 0) > - goto err_free_id; > + goto err_remove_chrdev; > > /* Add device to system */ > err = device_add(dev); > if (err < 0) > - goto err_free_id; > + goto err_remove_chrdev; It might be worth thinking about using cdev_device_add() though will require a slightly different order of adding. > > return 0; > > +err_remove_chrdev: > + counter_chrdev_remove(counter); > err_free_id: > put_device(dev); > return err; > @@ -138,13 +150,30 @@ int devm_counter_register(struct device *dev, > } > EXPORT_SYMBOL_GPL(devm_counter_register); > > +#define COUNTER_DEV_MAX 256 > + > static int __init counter_init(void) > { > - return bus_register(&counter_bus_type); > + int err; > + > + err = bus_register(&counter_bus_type); > + if (err < 0) > + return err; > + > + err = alloc_chrdev_region(&counter_devt, 0, COUNTER_DEV_MAX, "counter"); > + if (err < 0) > + goto err_unregister_bus; > + > + return 0; > + > +err_unregister_bus: > + bus_unregister(&counter_bus_type); > + return err; > } > > static void __exit counter_exit(void) > { > + unregister_chrdev_region(counter_devt, COUNTER_DEV_MAX); > bus_unregister(&counter_bus_type); > } > ... > diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h > index 6113938a6044..3d647a5383b8 100644 > --- a/include/uapi/linux/counter.h > +++ b/include/uapi/linux/counter.h > @@ -6,6 +6,19 @@ > #ifndef _UAPI_COUNTER_H_ > #define _UAPI_COUNTER_H_ > > +#include > +#include > + > +/* Component type definitions */ > +enum counter_component_type { > + COUNTER_COMPONENT_NONE, > + COUNTER_COMPONENT_SIGNAL, > + COUNTER_COMPONENT_COUNT, > + COUNTER_COMPONENT_FUNCTION, > + COUNTER_COMPONENT_SYNAPSE_ACTION, > + COUNTER_COMPONENT_EXTENSION, > +}; > + > /* Component scope definitions */ > enum counter_scope { > COUNTER_SCOPE_DEVICE, > @@ -13,6 +26,63 @@ enum counter_scope { > COUNTER_SCOPE_COUNT, > }; > > +/** > + * struct counter_component - Counter component identification > + * @type: component type (one of enum counter_component_type) > + * @scope: component scope (one of enum counter_scope) > + * @parent: parent component ID (matching the Y/Z suffix of the respective sysfs > + * path as described in Documentation/ABI/testing/sysfs-bus-counter) Probably good to give an example here as well as the cross reference. > + * @id: component ID (matching the Y/Z suffix of the respective sysfs path as > + * described in Documentation/ABI/testing/sysfs-bus-counter) > + */ > +struct counter_component { > + __u8 type; > + __u8 scope; > + __u8 parent; > + __u8 id; > +}; > + > +/* Event type definitions */ > +enum counter_event_type { > + COUNTER_EVENT_OVERFLOW, > + COUNTER_EVENT_UNDERFLOW, > + COUNTER_EVENT_OVERFLOW_UNDERFLOW, > + COUNTER_EVENT_THRESHOLD, > + COUNTER_EVENT_INDEX, > +}; > + > +/** > + * struct counter_watch - Counter component watch configuration > + * @component: component to watch when event triggers > + * @event: event that triggers (one of enum counter_event_type) > + * @channel: event channel (typically 0 unless the device supports concurrent > + * events of the same type) > + */ > +struct counter_watch { > + struct counter_component component; > + __u8 event; > + __u8 channel; > +}; > + > +/* ioctl commands */ > +#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x00, struct counter_watch) > +#define COUNTER_ENABLE_EVENTS_IOCTL _IO(0x3E, 0x01) > +#define COUNTER_DISABLE_EVENTS_IOCTL _IO(0x3E, 0x02) > + > +/** > + * struct counter_event - Counter event data > + * @timestamp: best estimate of time of event occurrence, in nanoseconds > + * @value: component value > + * @watch: component watch configuration > + * @status: return status (system error number) > + */ > +struct counter_event { > + __aligned_u64 timestamp; > + __aligned_u64 value; > + struct counter_watch watch; > + __u8 status; > +}; > + > /* Count direction values */ > enum counter_count_direction { > COUNTER_COUNT_DIRECTION_FORWARD,