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 BA795C6FD1F for ; Tue, 14 Mar 2023 15:46:19 +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:In-Reply-To:MIME-Version:References: 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=6oYB4YgLeMiXE0SVSMTClOkMbJoe2sBBvytGpL/eBSQ=; b=gMmZrZaJ+ji0mj SwemRvF57qMbsUX+CwmJh4lzzBJmPMQZFN3VsgdUgKsqVokUHpFBAY38hVu3wuwk13osDWdMoKnHx FLEyvTkbrhFIQIR5mIgjcaJ1c27iZX2IA4V7mLRuVLHx4YjKWKvaREEC3Rr9e1M7a4WNhoC2KDC8q FxUZjAsih/DBvTR8K7QrBmb/myWfdf0QU8PPtH+jtHWNF2kreo5ylhHzgCtX7fGM1McS1rQFVWYBg MqbXw482Vif6VYeu4y4oyoxEPPykSx+DErbcn2lyr/FpA6MWoEv3ziQL8UNMQfEJb2Ps21Yr3vyDy g58NWS7rXWuReeRmelcw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pc6qA-00Afnj-1s; Tue, 14 Mar 2023 15:45:30 +0000 Received: from mail-wr1-x42e.google.com ([2a00:1450:4864:20::42e]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pc6q5-00AfmN-3B for linux-arm-kernel@lists.infradead.org; Tue, 14 Mar 2023 15:45:28 +0000 Received: by mail-wr1-x42e.google.com with SMTP id t15so14773624wrz.7 for ; Tue, 14 Mar 2023 08:45:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20210112.gappssmtp.com; s=20210112; t=1678808724; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=Hlyp7nrREyg/a0hHUvwNjxDQM8a9Yo9MGnyhTjJAXJ4=; b=noeaJDguqKfIh08cy+5zlHvWFslWScBjda+n6hdS13WrqLSjWFT3TFB5MnvHfB3mfn 6aaiXE6S5ZgLmqEXXQqXS/OF4AI9JWdrdUQtzsZtSqHJwSrq4orjPHqnZAoEveGi97H1 iFDVwXz7PzUQ2EI564GoRaReRoTS/DYHJX8T7YNSqfWEps1Q8mc0RGj2kFQlc5uxYvTW hWq4g8jWVdr8YBJdRobqGcbG/1QDdPhq1ujcfJ9Zi329cLV0oYOjpW6Xr+QJf2Dx+ilt 6uI/BLW9kKbXyxN3H8oxjrjhAxVL56BL6hgYq4CV/WBA8ex7ScCLbnjJ2TBqeZpafa8t YL1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678808724; 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 :message-id:reply-to; bh=Hlyp7nrREyg/a0hHUvwNjxDQM8a9Yo9MGnyhTjJAXJ4=; b=tuBgUzN5JzefZADkF9GtxtflNznytANXH6KrTp84fMvdLKV/qHLg7Ucdn0XYCLb9gl iUeVugH5SP/mKByNIcOcQ6iqlHOjPa4h9ZWJbEm3+VWsyXiLoOK2A35KhthF0YpYUGpu pM7jg3QnMCgPRL8YFcL+o3qHn1wttgMj7x4MWviNxCe+cDPMQFqzuQAFqkR34o2s+1qH 6Z6K8c2OeWvC39XxcHFIB+wzCeXS6oiBrxmc68+VdvG3jyipyAVvO1Byyv10HuDV+PbE iQSXzQ/eo8kN5BxMr5QY4KG2n4+gUabl2Eq1r6z/rU7VSDnUdv+qPwrgvzGl6IY2zjb+ gKFQ== X-Gm-Message-State: AO0yUKVn6VT8oy0pcYYhYgXbEHUGtzb2+qF6FLR3sxYwXfsOkQG3TfYl lH9vjQPgSMELea75v1yscSp3lg== X-Google-Smtp-Source: AK7set9kOQSCt1nILgCoCuDKCZW+ZbwUmc5Jc2ebbeLxsp+0MErUKCOweY8716ROMkP894uRtiidSA== X-Received: by 2002:a5d:408e:0:b0:2c9:b9bf:e20c with SMTP id o14-20020a5d408e000000b002c9b9bfe20cmr11747197wrp.2.1678808723716; Tue, 14 Mar 2023 08:45:23 -0700 (PDT) Received: from localhost (host-213-179-129-39.customer.m-online.net. [213.179.129.39]) by smtp.gmail.com with ESMTPSA id m6-20020adff386000000b002c5493a17efsm2345919wro.25.2023.03.14.08.45.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Mar 2023 08:45:23 -0700 (PDT) Date: Tue, 14 Mar 2023 16:45:22 +0100 From: Jiri Pirko To: Vadim Fedorenko Cc: Jakub Kicinski , Arkadiusz Kubalewski , Jonathan Lemon , Paolo Abeni , Vadim Fedorenko , poros@redhat.com, mschmidt@redhat.com, netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, Milena Olech , Michal Michalik Subject: Re: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions Message-ID: References: <20230312022807.278528-1-vadfed@meta.com> <20230312022807.278528-3-vadfed@meta.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230312022807.278528-3-vadfed@meta.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230314_084526_210553_2B57807E X-CRM114-Status: GOOD ( 11.53 ) 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 Sun, Mar 12, 2023 at 03:28:03AM CET, vadfed@meta.com wrote: [...] >diff --git a/MAINTAINERS b/MAINTAINERS >index edd3d562beee..0222b19af545 100644 >--- a/MAINTAINERS >+++ b/MAINTAINERS >@@ -6289,6 +6289,15 @@ F: Documentation/networking/device_drivers/ethernet/freescale/dpaa2/switch-drive > F: drivers/net/ethernet/freescale/dpaa2/dpaa2-switch* > F: drivers/net/ethernet/freescale/dpaa2/dpsw* > >+DPLL CLOCK SUBSYSTEM Why "clock"? You don't mention "clock" anywhere else. [...] >diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c >new file mode 100644 >index 000000000000..3fc151e16751 >--- /dev/null >+++ b/drivers/dpll/dpll_core.c >@@ -0,0 +1,835 @@ >+// SPDX-License-Identifier: GPL-2.0 >+/* >+ * dpll_core.c - Generic DPLL Management class support. Why "class" ? [...] >+static int >+dpll_msg_add_pin_freq(struct sk_buff *msg, const struct dpll_pin *pin, >+ struct netlink_ext_ack *extack, bool dump_any_freq) >+{ >+ enum dpll_pin_freq_supp fs; >+ struct dpll_pin_ref *ref; >+ unsigned long i; >+ u32 freq; >+ >+ xa_for_each((struct xarray *)&pin->dpll_refs, i, ref) { >+ if (ref && ref->ops && ref->dpll) >+ break; >+ } >+ if (!ref || !ref->ops || !ref->dpll) >+ return -ENODEV; >+ if (!ref->ops->frequency_get) >+ return -EOPNOTSUPP; >+ if (ref->ops->frequency_get(pin, ref->dpll, &freq, extack)) >+ return -EFAULT; >+ if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY, freq)) >+ return -EMSGSIZE; >+ if (!dump_any_freq) >+ return 0; >+ for (fs = DPLL_PIN_FREQ_SUPP_UNSPEC + 1; >+ fs <= DPLL_PIN_FREQ_SUPP_MAX; fs++) { >+ if (test_bit(fs, &pin->prop.freq_supported)) { >+ if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED, >+ dpll_pin_freq_value[fs])) This is odd. As I suggested in the yaml patch, better to treat all supported frequencies the same, no matter if it is range or not. The you don't need this weird bitfield. You can have a macro to help driver to assemble array of supported frequencies and ranges. >+ return -EMSGSIZE; >+ } >+ } >+ if (pin->prop.any_freq_min && pin->prop.any_freq_max) { >+ if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MIN, >+ pin->prop.any_freq_min)) >+ return -EMSGSIZE; >+ if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MAX, >+ pin->prop.any_freq_max)) >+ return -EMSGSIZE; >+ } >+ >+ return 0; >+} >+ [...] >+static int >+dpll_cmd_pin_on_dpll_get(struct sk_buff *msg, struct dpll_pin *pin, >+ struct dpll_device *dpll, >+ struct netlink_ext_ack *extack) >+{ >+ struct dpll_pin_ref *ref; >+ int ret; >+ >+ if (nla_put_u32(msg, DPLL_A_PIN_IDX, pin->dev_driver_id)) >+ return -EMSGSIZE; >+ if (nla_put_string(msg, DPLL_A_PIN_DESCRIPTION, pin->prop.description)) >+ return -EMSGSIZE; >+ if (nla_put_u8(msg, DPLL_A_PIN_TYPE, pin->prop.type)) >+ return -EMSGSIZE; >+ if (nla_put_u32(msg, DPLL_A_PIN_DPLL_CAPS, pin->prop.capabilities)) >+ return -EMSGSIZE; >+ ret = dpll_msg_add_pin_direction(msg, pin, extack); >+ if (ret) >+ return ret; >+ ret = dpll_msg_add_pin_freq(msg, pin, extack, true); >+ if (ret && ret != -EOPNOTSUPP) >+ return ret; >+ ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll); >+ if (!ref) How exactly this can happen? Looks to me like only in case of a bug. WARN_ON() perhaps (put directly into dpll_xa_ref_dpll_find()? >+ return -EFAULT; >+ ret = dpll_msg_add_pin_prio(msg, pin, ref, extack); >+ if (ret && ret != -EOPNOTSUPP) >+ return ret; >+ ret = dpll_msg_add_pin_on_dpll_state(msg, pin, ref, extack); >+ if (ret && ret != -EOPNOTSUPP) >+ return ret; >+ ret = dpll_msg_add_pin_parents(msg, pin, extack); >+ if (ret) >+ return ret; >+ if (pin->rclk_dev_name) Use && and single if >+ if (nla_put_string(msg, DPLL_A_PIN_RCLK_DEVICE, >+ pin->rclk_dev_name)) >+ return -EMSGSIZE; >+ >+ return 0; >+} >+ [...] >+static int >+dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a, >+ struct netlink_ext_ack *extack) >+{ >+ u32 freq = nla_get_u32(a); >+ struct dpll_pin_ref *ref; >+ unsigned long i; >+ int ret; >+ >+ if (!dpll_pin_is_freq_supported(pin, freq)) >+ return -EINVAL; >+ >+ xa_for_each(&pin->dpll_refs, i, ref) { >+ ret = ref->ops->frequency_set(pin, ref->dpll, freq, extack); >+ if (ret) >+ return -EFAULT; return what the op returns: ret >+ dpll_pin_notify(ref->dpll, pin, DPLL_A_PIN_FREQUENCY); >+ } >+ >+ return 0; >+} >+ [...] >+static int >+dpll_pin_direction_set(struct dpll_pin *pin, struct nlattr *a, >+ struct netlink_ext_ack *extack) >+{ >+ enum dpll_pin_direction direction = nla_get_u8(a); >+ struct dpll_pin_ref *ref; >+ unsigned long i; >+ >+ if (!(DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE & pin->prop.capabilities)) >+ return -EOPNOTSUPP; >+ >+ xa_for_each(&pin->dpll_refs, i, ref) { >+ if (ref->ops->direction_set(pin, ref->dpll, direction, extack)) ret = .. if (ret) return ret; Please use this pattern in other ops call code as well. >+ return -EFAULT; >+ dpll_pin_notify(ref->dpll, pin, DPLL_A_PIN_DIRECTION); >+ } >+ >+ return 0; [...] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel