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 0115DC4321E for ; Fri, 2 Dec 2022 12:49:35 +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=ej96FkxSEdCm7c49gX5TITG18orB7bMWBgzaf4oCwBo=; b=1p+ggc84jmnOW2 lC+JDC2JBkfu3Tb172MWaKnP7uwpGWSojnxkOAdGtmwLrfU//1jxlP6uXN8JhtR2TQmaCSV3U9ocS obNQYK3kcoiSrxDuA+nJMT5/lk9jt413TAm5sToglZnCR+njVUTFnxGkPymVIys35RSt5o4YdRmFO WjPC2/DtFMPtmFXCECQyOsCKFlIAclCeLBSSgZjZT2txR7XaakrKvyLw/3DnJBdZsG2vo0dfBcKIT eQNUxvTFPoEIy86hHSIBVpE+6fNJ1ON7ge37kf8BSvSwrjEzTzCxQV3UNiFgqfmzuQ5kcZ0qQRhs+ +yA2HROUGUU27Qg2vMrA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p15T6-00GSLG-SY; Fri, 02 Dec 2022 12:48:40 +0000 Received: from mail-ej1-x62e.google.com ([2a00:1450:4864:20::62e]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1p15T3-00GRqb-7L for linux-arm-kernel@lists.infradead.org; Fri, 02 Dec 2022 12:48:39 +0000 Received: by mail-ej1-x62e.google.com with SMTP id ud5so11309231ejc.4 for ; Fri, 02 Dec 2022 04:48:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20210112.gappssmtp.com; s=20210112; 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=AT0KbpDr/o1zMwkliKr6eks9jc+exErBv7eT8T8pj78=; b=skePfRc3JK5U9CKJh6ersO7UP25S5G4SHdxxFzsJKpzlVQ6hGv2q5VOYiRx7WmnXpn BNQauBNM0e46G/mWjMEC4+iipBqfouArZ9uQn3UrTbUkmPlwP435V8kiNz/mwxgycyxN +m4zeDDo3DJcdntArFveir7heK1/rPbaGLWbLqD6rebzjagzRvUbNM7JEZ4ym/6EywKF KMksm8dbbhmDqTTSz5ACVsUV2c3gk9dgmrRyhicQFUDqUqAzTrWl4V1t56gSOz6EDaar g+RUMMS89wnVhdrX3R4LkkYLWAm4Kq2Z1ocbt9kO4n7i0CeeEV6iwCb3VKeguH1SbKbT e9lA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=AT0KbpDr/o1zMwkliKr6eks9jc+exErBv7eT8T8pj78=; b=G6B8ILYxPY9cSRHEmDsrAsLzEfK6M/AYupPVsa9gFv1a/3TblYCOPW/ni9AmOArOWL BSPkEWdXEf686yuAsZjdndGB9DR2rBQlHW5N/fQ+CDdj4m9tLSEs4xcyH+wSlP6Uni+P UHihLKmiF9yjhR0rFKw5PCt+lg8JHfbxBvyB+WcaX+Epbdfy7ppyb8imIZ7xVkv7uCEM jzphZUnitznSxsyiQfa5cChl8eYsj1D4GQsMNsz98ZqALUOMVE7HFEfCd5zBNDBZ7HpC Jy7hrnXctHothbpcEmY+z9WdtooMiTJG9joUln4Uy5EbHBC+jyqbr+DBUQ6UvrEcVAl+ tkSw== X-Gm-Message-State: ANoB5plldJiRGOSHo0BPxb3UApjRFBgxdU4PNMuFZ17olUwsEo4LzHcl dOo1TqzB7vxfKj+ca5I/zDrkGQ== X-Google-Smtp-Source: AA0mqf7jWlSapASGV7QpFArGzZKGyztzyAmsT17+Oylni66HWxNUNyk9jUexCfpd8FjM1Nw6MdfrUg== X-Received: by 2002:a17:906:1ecf:b0:7ad:902c:d1d6 with SMTP id m15-20020a1709061ecf00b007ad902cd1d6mr47737298ejj.143.1669985313640; Fri, 02 Dec 2022 04:48:33 -0800 (PST) Received: from localhost (host-213-179-129-39.customer.m-online.net. [213.179.129.39]) by smtp.gmail.com with ESMTPSA id ky1-20020a170907778100b0072a881b21d8sm2959163ejc.119.2022.12.02.04.48.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Dec 2022 04:48:32 -0800 (PST) Date: Fri, 2 Dec 2022 13:48:31 +0100 From: Jiri Pirko To: "Kubalewski, Arkadiusz" Cc: Vadim Fedorenko , Jakub Kicinski , Jonathan Lemon , Paolo Abeni , "netdev@vger.kernel.org" , Vadim Fedorenko , "linux-arm-kernel@lists.infradead.org" , "linux-clk@vger.kernel.org" Subject: Re: [RFC PATCH v4 4/4] ptp_ocp: implement DPLL ops Message-ID: References: <20221129213724.10119-1-vfedorenko@novek.ru> <20221129213724.10119-5-vfedorenko@novek.ru> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221202_044837_293248_F3C3143F X-CRM114-Status: GOOD ( 28.74 ) 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 Fri, Dec 02, 2022 at 12:27:32PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko >>Sent: Wednesday, November 30, 2022 1:41 PM >> >>Tue, Nov 29, 2022 at 10:37:24PM CET, vfedorenko@novek.ru wrote: >>>From: Vadim Fedorenko [...] >>>+static int ptp_ocp_dpll_get_attr(struct dpll_device *dpll, struct >>dpll_attr *attr) >>>+{ >>>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >>>+ int sync; >>>+ >>>+ sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; >>>+ dpll_attr_lock_status_set(attr, sync ? DPLL_LOCK_STATUS_LOCKED : >>DPLL_LOCK_STATUS_UNLOCKED); >> >>get,set,confuse. This attr thing sucks, sorry :/ > >Once again, I feel obligated to add some explanations :) > >getter is ops called by dpll subsystem, it requires data, so here value shall >be set for the caller, right? >Also have explained the reason why this attr struct and functions are done this >way in the response to cover letter concerns. Okay, I will react there. > >> >> >>>+ >>>+ return 0; >>>+} >>>+ >>>+static int ptp_ocp_dpll_pin_get_attr(struct dpll_device *dpll, struct >>dpll_pin *pin, >>>+ struct dpll_pin_attr *attr) >>>+{ >>>+ dpll_pin_attr_type_set(attr, DPLL_PIN_TYPE_EXT); >> >>This is exactly what I was talking about in the cover letter. This is >>const, should be put into static struct and passed to >>dpll_device_alloc(). > >Actually this type or some other parameters might change in the run-time, No. This should not change. If the pin is SyncE port, it's that for all lifetime of pin. It cannot turn to be a EXT/SMA connector all of the sudden. This should be definitelly fixed, it's a device topology. Can you explain the exact scenario when the change of personality of pin can happen? Perhaps I'm missing something. >depends on the device, it is up to the driver how it will handle any getter, >if driver knows it won't change it could also have some static member and copy >the data with: dpll_pin_attr_copy(...); > >> >> >>>+ return 0; >>>+} >>>+ >>>+static struct dpll_device_ops dpll_ops = { >>>+ .get = ptp_ocp_dpll_get_attr, >>>+}; >>>+ >>>+static struct dpll_pin_ops dpll_pin_ops = { >>>+ .get = ptp_ocp_dpll_pin_get_attr, >>>+}; >>>+ >>> static int >>> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>> { >>>+ const u8 dpll_cookie[DPLL_COOKIE_LEN] = { "OCP" }; >>>+ char pin_desc[PIN_DESC_LEN]; >>> struct devlink *devlink; >>>+ struct dpll_pin *pin; >>> struct ptp_ocp *bp; >>>- int err; >>>+ int err, i; >>> >>> devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev- >>>dev); >>> if (!devlink) { >>>@@ -4230,6 +4263,20 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct >>pci_device_id *id) >>> >>> ptp_ocp_info(bp); >>> devlink_register(devlink); >>>+ >>>+ bp->dpll = dpll_device_alloc(&dpll_ops, DPLL_TYPE_PPS, dpll_cookie, >>pdev->bus->number, bp, &pdev->dev); >>>+ if (!bp->dpll) { >>>+ dev_err(&pdev->dev, "dpll_device_alloc failed\n"); >>>+ goto out; >>>+ } >>>+ dpll_device_register(bp->dpll); >> >>You still have the 2 step init process. I believe it would be better to >>just have dpll_device_create/destroy() to do it in one shot. > >For me either is ok, but due to pins alloc/register as explained below I would >leave it as it is. Please don't, it has no value. Just adds unnecesary code. Have it nice and simple. > >> >> >>>+ >>>+ for (i = 0; i < 4; i++) { >>>+ snprintf(pin_desc, PIN_DESC_LEN, "sma%d", i + 1); >>>+ pin = dpll_pin_alloc(pin_desc, PIN_DESC_LEN); >>>+ dpll_pin_register(bp->dpll, pin, &dpll_pin_ops, bp); >> >>Same here, no point of having 2 step init. > >The alloc of a pin is not required if the pin already exist and would be just >registered with another dpll. Please don't. Have a pin created on a single DPLL. Why you make things compitated here? I don't follow. >Once we decide to entirely drop shared pins idea this could be probably done, >although other kernel code usually use this twostep approach? No, it does not. It's is used whatever fits on the individual usecase. > >> >> >>>+ } >>>+ >>> return 0; >> >> >>Btw, did you consider having dpll instance here as and auxdev? It would >>be suitable I believe. It is quite simple to do it. See following patch >>as an example: > >I haven't think about it, definetly gonna take a look to see if there any >benefits in ice. Please do. The proper separation and bus/device modelling is at least one of the benefits. The other one is that all dpll drivers would happily live in drivers/dpll/ side by side. > >Thanks, >Arkadiusz > >> >>commit bd02fd76d1909637c95e8ef13e7fd1e748af910d >>Author: Jiri Pirko >>Date: Mon Jul 25 10:29:17 2022 +0200 >> >> mlxsw: core_linecards: Introduce per line card auxiliary device >> >> >> >> >>> >>> out: >>>@@ -4247,6 +4294,8 @@ ptp_ocp_remove(struct pci_dev *pdev) >>> struct ptp_ocp *bp = pci_get_drvdata(pdev); >>> struct devlink *devlink = priv_to_devlink(bp); >>> >>>+ dpll_device_unregister(bp->dpll); >>>+ dpll_device_free(bp->dpll); >>> devlink_unregister(devlink); >>> ptp_ocp_detach(bp); >>> pci_disable_device(pdev); >>>-- >>>2.27.0 >>> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel