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 smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (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 AB486C001B0 for ; Tue, 15 Aug 2023 03:17:17 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 3B51A60B68; Tue, 15 Aug 2023 03:17:17 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 3B51A60B68 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=osuosl.org; s=default; t=1692069437; bh=wDuQlj57eH7Jh4CmEnaZ1V291/G/qb0J3GA4defHxTM=; h=Date:From:To:In-Reply-To:References:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: Cc:From; b=v//72hAFU8GYZBaDVpefDjW7cYvCBLHGGNW4k1p0Ukb89qCEi6oS7+JpXdaZeq1Ih 4hIcmh3M9Pw0+NKpUp2dAMoaNeAp+ww0KHYUuxcYUKWT0fAl7KsInzI6HI0m2bRtmO apnbnpSPxXKHOgGo+0Q5CBHE2xE1jpY3pis/qZ7Vb5Mzgq1y+tWejtdeNkBZKxrdvg ZH6eEBH11bv2l/UCX7ywVl3lLycrdwhl7T8lQ+Xt0MbJhN0uhpHMRlnm8pXjKaLWxH XH62KToq6ZKtrhWBdAfKxnpYcC7xiIxgo8PrZCuOGCswmbtLM6fvdBwfSXr3KHCb56 h16Rj8JcQB1Cg== X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id r0-PqC8GMF-T; Tue, 15 Aug 2023 03:17:15 +0000 (UTC) Received: from ash.osuosl.org (ash.osuosl.org [140.211.166.34]) by smtp3.osuosl.org (Postfix) with ESMTP id 9237D60EF7; Tue, 15 Aug 2023 03:17:15 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 9237D60EF7 Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by ash.osuosl.org (Postfix) with ESMTP id 239F41BF380 for ; Tue, 15 Aug 2023 03:17:14 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id EE539415C7 for ; Tue, 15 Aug 2023 03:17:13 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org EE539415C7 X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id UQa2-Uz_hpFl for ; Tue, 15 Aug 2023 03:17:12 +0000 (UTC) Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by smtp4.osuosl.org (Postfix) with ESMTPS id 74508408BB for ; Tue, 15 Aug 2023 03:17:12 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 74508408BB Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 8FFD862B08; Tue, 15 Aug 2023 03:17:11 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4EFD7C433C7; Tue, 15 Aug 2023 03:17:10 +0000 (UTC) Date: Mon, 14 Aug 2023 20:17:09 -0700 From: Jakub Kicinski To: Vadim Fedorenko Message-ID: <20230814201709.655a24e2@kernel.org> In-Reply-To: <20230811200340.577359-4-vadim.fedorenko@linux.dev> References: <20230811200340.577359-1-vadim.fedorenko@linux.dev> <20230811200340.577359-4-vadim.fedorenko@linux.dev> MIME-Version: 1.0 X-Mailman-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1692069431; bh=22kBBBnNdB/LlZZ6DoBpY7Dn8SwEgCysRQ5z6dwITWs=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=WLtA43ezjU4SB7ClVMO5p37HipKcIU4LFlDrvBoxm1cUVs35jEK7zYOQBWac9AUTY FEuGKsyVyKrnIS6Rlv2n+nZYwQGU8V68TWujfXNaRNWcPpToTeN/7L7BUIEbZCB8c2 o2W2iNCMn311ApQCWT8Li6c5RGSiZVcXFvEJqAbw6qvivFGDR+T+eBGAYitLs0uXJN kUo9EjHiQsde80sESI71HGhiW/FRcrTsMFfwiGlVJeJUfJ2oau392lBl0jA1gikIGj Y9I2dGDsN7L6hJ7R58MSsmRRvC2nIkRazYrtkIj4majYv1KWp2Dw6Oor8mrQKRJ55A iiUK4ilPSXKWA== X-Mailman-Original-Authentication-Results: smtp4.osuosl.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20201202 header.b=WLtA43ez Subject: Re: [Intel-wired-lan] [PATCH net-next v4 3/9] dpll: core: Add DPLL framework base functions X-BeenThere: intel-wired-lan@osuosl.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Wired Ethernet Linux Kernel Driver Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Jiri Pirko , Bart Van Assche , netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org, linux-arm-kernel@lists.infradead.org, Jiri Pirko , Jonathan Lemon , Paolo Abeni , linux-clk@vger.kernel.org, Milena Olech Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-wired-lan-bounces@osuosl.org Sender: "Intel-wired-lan" On Fri, 11 Aug 2023 21:03:34 +0100 Vadim Fedorenko wrote: > DPLL framework is used to represent and configure DPLL devices > in systems. Each device that has DPLL and can configure inputs > and outputs can use this framework. > > Implement core framework functions for further interactions > with device drivers implementing dpll subsystem, as well as for > interactions of DPLL netlink framework part with the subsystem > itself. > +static struct dpll_device * > +dpll_device_alloc(const u64 clock_id, u32 device_idx, struct module *module) > +{ > + struct dpll_device *dpll; > + int ret; > + > + dpll = kzalloc(sizeof(*dpll), GFP_KERNEL); > + if (!dpll) > + return ERR_PTR(-ENOMEM); > + refcount_set(&dpll->refcount, 1); > + INIT_LIST_HEAD(&dpll->registration_list); > + dpll->device_idx = device_idx; > + dpll->clock_id = clock_id; > + dpll->module = module; > + ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, > + GFP_KERNEL); Why only 16b and why not _cyclic? > +/** > + * dpll_device_register - register the dpll device in the subsystem > + * @dpll: pointer to a dpll > + * @type: type of a dpll > + * @ops: ops for a dpll device > + * @priv: pointer to private information of owner > + * > + * Make dpll device available for user space. > + * > + * Context: Acquires a lock (dpll_lock) > + * Return: > + * * 0 on success > + * * negative - error value > + */ > +int dpll_device_register(struct dpll_device *dpll, enum dpll_type type, > + const struct dpll_device_ops *ops, void *priv) > +{ > + struct dpll_device_registration *reg; > + bool first_registration = false; > + > + if (WARN_ON(!ops)) > + return -EINVAL; > + if (WARN_ON(!ops->mode_get)) > + return -EINVAL; > + if (WARN_ON(!ops->lock_status_get)) > + return -EINVAL; > + if (WARN_ON(type < DPLL_TYPE_PPS || type > DPLL_TYPE_MAX)) > + return -EINVAL; > + > + mutex_lock(&dpll_lock); > + reg = dpll_device_registration_find(dpll, ops, priv); > + if (reg) { > + mutex_unlock(&dpll_lock); > + return -EEXIST; > + } > + > + reg = kzalloc(sizeof(*reg), GFP_KERNEL); > + if (!reg) { > + mutex_unlock(&dpll_lock); > + return -ENOMEM; > + } > + reg->ops = ops; > + reg->priv = priv; > + dpll->type = type; > + first_registration = list_empty(&dpll->registration_list); > + list_add_tail(®->list, &dpll->registration_list); > + if (!first_registration) { > + mutex_unlock(&dpll_lock); > + return 0; > + } > + > + xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); > + mutex_unlock(&dpll_lock); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(dpll_device_register); Is the registration flow documented? It's a bit atypical so we should write some pseudocode somewhere. > +/** > + * dpll_device_unregister - unregister dpll device > + * @dpll: registered dpll pointer > + * @ops: ops for a dpll device > + * @priv: pointer to private information of owner > + * > + * Unregister device, make it unavailable for userspace. > + * Note: It does not free the memory > + * Context: Acquires a lock (dpll_lock) > + */ > +void dpll_device_unregister(struct dpll_device *dpll, > + const struct dpll_device_ops *ops, void *priv) > +{ > + struct dpll_device_registration *reg; > + > + mutex_lock(&dpll_lock); > + ASSERT_DPLL_REGISTERED(dpll); > + reg = dpll_device_registration_find(dpll, ops, priv); > + if (WARN_ON(!reg)) { > + mutex_unlock(&dpll_lock); > + return; > + } > + list_del(®->list); > + kfree(reg); > + > + if (!list_empty(&dpll->registration_list)) { > + mutex_unlock(&dpll_lock); > + return; > + } > + xa_clear_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); > + mutex_unlock(&dpll_lock); > +} > +EXPORT_SYMBOL_GPL(dpll_device_unregister); > +/** > + * struct dpll_pin - structure for a dpll pin > + * @id: unique id number for pin given by dpll subsystem > + * @pin_idx: index of a pin given by dev driver > + * @clock_id: clock_id of creator > + * @module: module of creator > + * @dpll_refs: hold referencees to dplls pin was registered with > + * @parent_refs: hold references to parent pins pin was registered with > + * @prop: pointer to pin properties given by registerer > + * @rclk_dev_name: holds name of device when pin can recover clock from it > + * @refcount: refcount > + **/ > +struct dpll_pin { > + u32 id; > + u32 pin_idx; > + u64 clock_id; > + struct module *module; > + struct xarray dpll_refs; > + struct xarray parent_refs; > + const struct dpll_pin_properties *prop; > + char *rclk_dev_name; Where is rclk_dev_name filled in? > +struct dpll_pin_ops { > + int (*frequency_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + const u64 frequency, > + struct netlink_ext_ack *extack); > + int (*frequency_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + u64 *frequency, struct netlink_ext_ack *extack); > + int (*direction_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + const enum dpll_pin_direction direction, > + struct netlink_ext_ack *extack); > + int (*direction_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + enum dpll_pin_direction *direction, > + struct netlink_ext_ack *extack); > + int (*state_on_pin_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_pin *parent_pin, > + void *parent_pin_priv, > + enum dpll_pin_state *state, > + struct netlink_ext_ack *extack); > + int (*state_on_dpll_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, > + void *dpll_priv, enum dpll_pin_state *state, > + struct netlink_ext_ack *extack); > + int (*state_on_pin_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_pin *parent_pin, > + void *parent_pin_priv, > + const enum dpll_pin_state state, > + struct netlink_ext_ack *extack); > + int (*state_on_dpll_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, > + void *dpll_priv, > + const enum dpll_pin_state state, > + struct netlink_ext_ack *extack); > + int (*prio_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + u32 *prio, struct netlink_ext_ack *extack); > + int (*prio_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + const u32 prio, struct netlink_ext_ack *extack); > +}; The ops need a kdoc > +struct dpll_device > +*dpll_device_get(u64 clock_id, u32 dev_driver_id, struct module *module); nit: * is part of the type, it goes on the previous line _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan 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 7369AEB64DD for ; Tue, 15 Aug 2023 03:36:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234414AbjHODgB (ORCPT ); Mon, 14 Aug 2023 23:36:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53432 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234542AbjHODd3 (ORCPT ); Mon, 14 Aug 2023 23:33:29 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 26B1D35A7 for ; Mon, 14 Aug 2023 20:17:37 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 9B22D635E1 for ; Tue, 15 Aug 2023 03:17:11 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4EFD7C433C7; Tue, 15 Aug 2023 03:17:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1692069431; bh=22kBBBnNdB/LlZZ6DoBpY7Dn8SwEgCysRQ5z6dwITWs=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=WLtA43ezjU4SB7ClVMO5p37HipKcIU4LFlDrvBoxm1cUVs35jEK7zYOQBWac9AUTY FEuGKsyVyKrnIS6Rlv2n+nZYwQGU8V68TWujfXNaRNWcPpToTeN/7L7BUIEbZCB8c2 o2W2iNCMn311ApQCWT8Li6c5RGSiZVcXFvEJqAbw6qvivFGDR+T+eBGAYitLs0uXJN kUo9EjHiQsde80sESI71HGhiW/FRcrTsMFfwiGlVJeJUfJ2oau392lBl0jA1gikIGj Y9I2dGDsN7L6hJ7R58MSsmRRvC2nIkRazYrtkIj4majYv1KWp2Dw6Oor8mrQKRJ55A iiUK4ilPSXKWA== Date: Mon, 14 Aug 2023 20:17:09 -0700 From: Jakub Kicinski To: Vadim Fedorenko Cc: Jiri Pirko , Arkadiusz Kubalewski , Jonathan Lemon , Paolo Abeni , Milena Olech , Michal Michalik , linux-arm-kernel@lists.infradead.org, poros@redhat.com, mschmidt@redhat.com, netdev@vger.kernel.org, linux-clk@vger.kernel.org, Bart Van Assche , intel-wired-lan@lists.osuosl.org, Jiri Pirko Subject: Re: [PATCH net-next v4 3/9] dpll: core: Add DPLL framework base functions Message-ID: <20230814201709.655a24e2@kernel.org> In-Reply-To: <20230811200340.577359-4-vadim.fedorenko@linux.dev> References: <20230811200340.577359-1-vadim.fedorenko@linux.dev> <20230811200340.577359-4-vadim.fedorenko@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-clk@vger.kernel.org On Fri, 11 Aug 2023 21:03:34 +0100 Vadim Fedorenko wrote: > DPLL framework is used to represent and configure DPLL devices > in systems. Each device that has DPLL and can configure inputs > and outputs can use this framework. > > Implement core framework functions for further interactions > with device drivers implementing dpll subsystem, as well as for > interactions of DPLL netlink framework part with the subsystem > itself. > +static struct dpll_device * > +dpll_device_alloc(const u64 clock_id, u32 device_idx, struct module *module) > +{ > + struct dpll_device *dpll; > + int ret; > + > + dpll = kzalloc(sizeof(*dpll), GFP_KERNEL); > + if (!dpll) > + return ERR_PTR(-ENOMEM); > + refcount_set(&dpll->refcount, 1); > + INIT_LIST_HEAD(&dpll->registration_list); > + dpll->device_idx = device_idx; > + dpll->clock_id = clock_id; > + dpll->module = module; > + ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, > + GFP_KERNEL); Why only 16b and why not _cyclic? > +/** > + * dpll_device_register - register the dpll device in the subsystem > + * @dpll: pointer to a dpll > + * @type: type of a dpll > + * @ops: ops for a dpll device > + * @priv: pointer to private information of owner > + * > + * Make dpll device available for user space. > + * > + * Context: Acquires a lock (dpll_lock) > + * Return: > + * * 0 on success > + * * negative - error value > + */ > +int dpll_device_register(struct dpll_device *dpll, enum dpll_type type, > + const struct dpll_device_ops *ops, void *priv) > +{ > + struct dpll_device_registration *reg; > + bool first_registration = false; > + > + if (WARN_ON(!ops)) > + return -EINVAL; > + if (WARN_ON(!ops->mode_get)) > + return -EINVAL; > + if (WARN_ON(!ops->lock_status_get)) > + return -EINVAL; > + if (WARN_ON(type < DPLL_TYPE_PPS || type > DPLL_TYPE_MAX)) > + return -EINVAL; > + > + mutex_lock(&dpll_lock); > + reg = dpll_device_registration_find(dpll, ops, priv); > + if (reg) { > + mutex_unlock(&dpll_lock); > + return -EEXIST; > + } > + > + reg = kzalloc(sizeof(*reg), GFP_KERNEL); > + if (!reg) { > + mutex_unlock(&dpll_lock); > + return -ENOMEM; > + } > + reg->ops = ops; > + reg->priv = priv; > + dpll->type = type; > + first_registration = list_empty(&dpll->registration_list); > + list_add_tail(®->list, &dpll->registration_list); > + if (!first_registration) { > + mutex_unlock(&dpll_lock); > + return 0; > + } > + > + xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); > + mutex_unlock(&dpll_lock); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(dpll_device_register); Is the registration flow documented? It's a bit atypical so we should write some pseudocode somewhere. > +/** > + * dpll_device_unregister - unregister dpll device > + * @dpll: registered dpll pointer > + * @ops: ops for a dpll device > + * @priv: pointer to private information of owner > + * > + * Unregister device, make it unavailable for userspace. > + * Note: It does not free the memory > + * Context: Acquires a lock (dpll_lock) > + */ > +void dpll_device_unregister(struct dpll_device *dpll, > + const struct dpll_device_ops *ops, void *priv) > +{ > + struct dpll_device_registration *reg; > + > + mutex_lock(&dpll_lock); > + ASSERT_DPLL_REGISTERED(dpll); > + reg = dpll_device_registration_find(dpll, ops, priv); > + if (WARN_ON(!reg)) { > + mutex_unlock(&dpll_lock); > + return; > + } > + list_del(®->list); > + kfree(reg); > + > + if (!list_empty(&dpll->registration_list)) { > + mutex_unlock(&dpll_lock); > + return; > + } > + xa_clear_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); > + mutex_unlock(&dpll_lock); > +} > +EXPORT_SYMBOL_GPL(dpll_device_unregister); > +/** > + * struct dpll_pin - structure for a dpll pin > + * @id: unique id number for pin given by dpll subsystem > + * @pin_idx: index of a pin given by dev driver > + * @clock_id: clock_id of creator > + * @module: module of creator > + * @dpll_refs: hold referencees to dplls pin was registered with > + * @parent_refs: hold references to parent pins pin was registered with > + * @prop: pointer to pin properties given by registerer > + * @rclk_dev_name: holds name of device when pin can recover clock from it > + * @refcount: refcount > + **/ > +struct dpll_pin { > + u32 id; > + u32 pin_idx; > + u64 clock_id; > + struct module *module; > + struct xarray dpll_refs; > + struct xarray parent_refs; > + const struct dpll_pin_properties *prop; > + char *rclk_dev_name; Where is rclk_dev_name filled in? > +struct dpll_pin_ops { > + int (*frequency_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + const u64 frequency, > + struct netlink_ext_ack *extack); > + int (*frequency_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + u64 *frequency, struct netlink_ext_ack *extack); > + int (*direction_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + const enum dpll_pin_direction direction, > + struct netlink_ext_ack *extack); > + int (*direction_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + enum dpll_pin_direction *direction, > + struct netlink_ext_ack *extack); > + int (*state_on_pin_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_pin *parent_pin, > + void *parent_pin_priv, > + enum dpll_pin_state *state, > + struct netlink_ext_ack *extack); > + int (*state_on_dpll_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, > + void *dpll_priv, enum dpll_pin_state *state, > + struct netlink_ext_ack *extack); > + int (*state_on_pin_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_pin *parent_pin, > + void *parent_pin_priv, > + const enum dpll_pin_state state, > + struct netlink_ext_ack *extack); > + int (*state_on_dpll_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, > + void *dpll_priv, > + const enum dpll_pin_state state, > + struct netlink_ext_ack *extack); > + int (*prio_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + u32 *prio, struct netlink_ext_ack *extack); > + int (*prio_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + const u32 prio, struct netlink_ext_ack *extack); > +}; The ops need a kdoc > +struct dpll_device > +*dpll_device_get(u64 clock_id, u32 dev_driver_id, struct module *module); nit: * is part of the type, it goes on the previous line 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 77A43C001B0 for ; Tue, 15 Aug 2023 03:17:51 +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:MIME-Version:References:In-Reply-To: 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=J84pWviXES/nCMs3XrxZIykvhaR3v5zzGmQcohocfjw=; b=29iZP8vwo95QXq OQxd5oNlueK67/ISYIEy+fXI1PW5CcfW3JU6OQenC2x7bUxjeLEnuTK/f9/qo1a1YPT4XV1CI6zCs nFW7tL3EQt4Bj4qnNwmYxeWKASxAqjPqehEjocHVsIhaJqXtIXAruqUmSnMfT6R9RjE2QcUCW/Hay 5/nweiGRb9E+/AZXP8BHM/OURAx5g1rfHj3B0Xrag9BFgrASOezVDSHLPVMVgUp2DgEAdjXtQHPNb RHYbSjKoXySv8MrOqTvxdIboBMcrRaz3B2bvVqjua931v3nQdPPwX7VSXtBCV9iiYBg2TVGsq0fiw Cy7/PnV0Kf63pcXeq0OQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qVkYi-000n7H-11; Tue, 15 Aug 2023 03:17:28 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qVkYc-000n6K-0A for linux-arm-kernel@lists.infradead.org; Tue, 15 Aug 2023 03:17:27 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 8FFD862B08; Tue, 15 Aug 2023 03:17:11 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4EFD7C433C7; Tue, 15 Aug 2023 03:17:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1692069431; bh=22kBBBnNdB/LlZZ6DoBpY7Dn8SwEgCysRQ5z6dwITWs=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=WLtA43ezjU4SB7ClVMO5p37HipKcIU4LFlDrvBoxm1cUVs35jEK7zYOQBWac9AUTY FEuGKsyVyKrnIS6Rlv2n+nZYwQGU8V68TWujfXNaRNWcPpToTeN/7L7BUIEbZCB8c2 o2W2iNCMn311ApQCWT8Li6c5RGSiZVcXFvEJqAbw6qvivFGDR+T+eBGAYitLs0uXJN kUo9EjHiQsde80sESI71HGhiW/FRcrTsMFfwiGlVJeJUfJ2oau392lBl0jA1gikIGj Y9I2dGDsN7L6hJ7R58MSsmRRvC2nIkRazYrtkIj4majYv1KWp2Dw6Oor8mrQKRJ55A iiUK4ilPSXKWA== Date: Mon, 14 Aug 2023 20:17:09 -0700 From: Jakub Kicinski To: Vadim Fedorenko Cc: Jiri Pirko , Arkadiusz Kubalewski , Jonathan Lemon , Paolo Abeni , Milena Olech , Michal Michalik , linux-arm-kernel@lists.infradead.org, poros@redhat.com, mschmidt@redhat.com, netdev@vger.kernel.org, linux-clk@vger.kernel.org, Bart Van Assche , intel-wired-lan@lists.osuosl.org, Jiri Pirko Subject: Re: [PATCH net-next v4 3/9] dpll: core: Add DPLL framework base functions Message-ID: <20230814201709.655a24e2@kernel.org> In-Reply-To: <20230811200340.577359-4-vadim.fedorenko@linux.dev> References: <20230811200340.577359-1-vadim.fedorenko@linux.dev> <20230811200340.577359-4-vadim.fedorenko@linux.dev> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230814_201724_680187_7DEE5BF4 X-CRM114-Status: GOOD ( 23.26 ) 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 Fri, 11 Aug 2023 21:03:34 +0100 Vadim Fedorenko wrote: > DPLL framework is used to represent and configure DPLL devices > in systems. Each device that has DPLL and can configure inputs > and outputs can use this framework. > > Implement core framework functions for further interactions > with device drivers implementing dpll subsystem, as well as for > interactions of DPLL netlink framework part with the subsystem > itself. > +static struct dpll_device * > +dpll_device_alloc(const u64 clock_id, u32 device_idx, struct module *module) > +{ > + struct dpll_device *dpll; > + int ret; > + > + dpll = kzalloc(sizeof(*dpll), GFP_KERNEL); > + if (!dpll) > + return ERR_PTR(-ENOMEM); > + refcount_set(&dpll->refcount, 1); > + INIT_LIST_HEAD(&dpll->registration_list); > + dpll->device_idx = device_idx; > + dpll->clock_id = clock_id; > + dpll->module = module; > + ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, > + GFP_KERNEL); Why only 16b and why not _cyclic? > +/** > + * dpll_device_register - register the dpll device in the subsystem > + * @dpll: pointer to a dpll > + * @type: type of a dpll > + * @ops: ops for a dpll device > + * @priv: pointer to private information of owner > + * > + * Make dpll device available for user space. > + * > + * Context: Acquires a lock (dpll_lock) > + * Return: > + * * 0 on success > + * * negative - error value > + */ > +int dpll_device_register(struct dpll_device *dpll, enum dpll_type type, > + const struct dpll_device_ops *ops, void *priv) > +{ > + struct dpll_device_registration *reg; > + bool first_registration = false; > + > + if (WARN_ON(!ops)) > + return -EINVAL; > + if (WARN_ON(!ops->mode_get)) > + return -EINVAL; > + if (WARN_ON(!ops->lock_status_get)) > + return -EINVAL; > + if (WARN_ON(type < DPLL_TYPE_PPS || type > DPLL_TYPE_MAX)) > + return -EINVAL; > + > + mutex_lock(&dpll_lock); > + reg = dpll_device_registration_find(dpll, ops, priv); > + if (reg) { > + mutex_unlock(&dpll_lock); > + return -EEXIST; > + } > + > + reg = kzalloc(sizeof(*reg), GFP_KERNEL); > + if (!reg) { > + mutex_unlock(&dpll_lock); > + return -ENOMEM; > + } > + reg->ops = ops; > + reg->priv = priv; > + dpll->type = type; > + first_registration = list_empty(&dpll->registration_list); > + list_add_tail(®->list, &dpll->registration_list); > + if (!first_registration) { > + mutex_unlock(&dpll_lock); > + return 0; > + } > + > + xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); > + mutex_unlock(&dpll_lock); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(dpll_device_register); Is the registration flow documented? It's a bit atypical so we should write some pseudocode somewhere. > +/** > + * dpll_device_unregister - unregister dpll device > + * @dpll: registered dpll pointer > + * @ops: ops for a dpll device > + * @priv: pointer to private information of owner > + * > + * Unregister device, make it unavailable for userspace. > + * Note: It does not free the memory > + * Context: Acquires a lock (dpll_lock) > + */ > +void dpll_device_unregister(struct dpll_device *dpll, > + const struct dpll_device_ops *ops, void *priv) > +{ > + struct dpll_device_registration *reg; > + > + mutex_lock(&dpll_lock); > + ASSERT_DPLL_REGISTERED(dpll); > + reg = dpll_device_registration_find(dpll, ops, priv); > + if (WARN_ON(!reg)) { > + mutex_unlock(&dpll_lock); > + return; > + } > + list_del(®->list); > + kfree(reg); > + > + if (!list_empty(&dpll->registration_list)) { > + mutex_unlock(&dpll_lock); > + return; > + } > + xa_clear_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); > + mutex_unlock(&dpll_lock); > +} > +EXPORT_SYMBOL_GPL(dpll_device_unregister); > +/** > + * struct dpll_pin - structure for a dpll pin > + * @id: unique id number for pin given by dpll subsystem > + * @pin_idx: index of a pin given by dev driver > + * @clock_id: clock_id of creator > + * @module: module of creator > + * @dpll_refs: hold referencees to dplls pin was registered with > + * @parent_refs: hold references to parent pins pin was registered with > + * @prop: pointer to pin properties given by registerer > + * @rclk_dev_name: holds name of device when pin can recover clock from it > + * @refcount: refcount > + **/ > +struct dpll_pin { > + u32 id; > + u32 pin_idx; > + u64 clock_id; > + struct module *module; > + struct xarray dpll_refs; > + struct xarray parent_refs; > + const struct dpll_pin_properties *prop; > + char *rclk_dev_name; Where is rclk_dev_name filled in? > +struct dpll_pin_ops { > + int (*frequency_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + const u64 frequency, > + struct netlink_ext_ack *extack); > + int (*frequency_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + u64 *frequency, struct netlink_ext_ack *extack); > + int (*direction_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + const enum dpll_pin_direction direction, > + struct netlink_ext_ack *extack); > + int (*direction_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + enum dpll_pin_direction *direction, > + struct netlink_ext_ack *extack); > + int (*state_on_pin_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_pin *parent_pin, > + void *parent_pin_priv, > + enum dpll_pin_state *state, > + struct netlink_ext_ack *extack); > + int (*state_on_dpll_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, > + void *dpll_priv, enum dpll_pin_state *state, > + struct netlink_ext_ack *extack); > + int (*state_on_pin_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_pin *parent_pin, > + void *parent_pin_priv, > + const enum dpll_pin_state state, > + struct netlink_ext_ack *extack); > + int (*state_on_dpll_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, > + void *dpll_priv, > + const enum dpll_pin_state state, > + struct netlink_ext_ack *extack); > + int (*prio_get)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + u32 *prio, struct netlink_ext_ack *extack); > + int (*prio_set)(const struct dpll_pin *pin, void *pin_priv, > + const struct dpll_device *dpll, void *dpll_priv, > + const u32 prio, struct netlink_ext_ack *extack); > +}; The ops need a kdoc > +struct dpll_device > +*dpll_device_get(u64 clock_id, u32 dev_driver_id, struct module *module); nit: * is part of the type, it goes on the previous line _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel