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=-15.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable 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 63475C433DB for ; Sun, 14 Feb 2021 18:07:47 +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 2714C64E29 for ; Sun, 14 Feb 2021 18:07:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2714C64E29 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org 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-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Message-ID: Subject: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=R06cU/O9u7QJprtqLLRW364hnCDcndnTlgg5NhRi308=; b=MSHfanyk3QC6eCSRJmaZfmEde jogBTIajJMmCgrEJtd2cYxkVRM99ajicyJI6ejMZQDYuWYshkPLWn8e/GYBB/oSYknrcfyyxd5iVo Nych33z5ZEHWHBdFFa+FFF83MQAJfJZ5AndNiqFIhLieja5ts5IndfHOf5k6ujQgrImVpFeB12x/B aD0nKNlz8aOmd2j+hIuD/F+yMT83/TSWiUeq4aHRZvC07RYnisYT75GL5JDRaEDMDQe2bH6Gt3cH4 l58KC00AsknrJ2AtSWyu72/+s3vE5iIbLUhYz8dZ+xwjRkkWKb4xXqo67lQx4NfUSWtYWGwygOG1o uCoUEMihw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lBLmn-0006US-Ba; Sun, 14 Feb 2021 18:06:21 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1lBLml-0006U2-0T for linux-arm-kernel@lists.infradead.org; Sun, 14 Feb 2021 18:06:20 +0000 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 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 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210214_130619_208711_3B1E33D6 X-CRM114-Status: GOOD ( 28.66 ) 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, o.rempel@pengutronix.de, 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: 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 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, _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel