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 A3ACFC4321E for ; Fri, 2 Dec 2022 16:21:53 +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=4S3x7T2t35vx65Qvj31GsHEE/YfYspmU5VA0L5Sn7N0=; b=IH5JzW5KXB3HWB 4gMiJW8MtsxMFYXizKrX4glWsury4i1PQfaA78Bmf8S7LAvpBjsGOPmwMB0pA2eiHZNHnfLQavZsF WGsPtuqQcQXus1eVBpsEp0eGhScIXT210by98Ztn5MrKJ2WchMn0KOn84cVFcUTncecgnHUjbP5iq 2AKCWcBqbNDRiU47Is/bHFaUfvkxSy8RYR7gEnLcO4gwuwNBlvlD1inTCWpqK+WXmmKa6dua7T90Q bSq9RQ/hX12Cl18ncMTywyUFUuyEwRDgK36dMhyI4iuV+zDp4WEMcwb4Ky6koHphRiaMAI3Tt6u1N RXFPVCg2nOZ/RP2PaYqA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p18mJ-00HSI4-UY; Fri, 02 Dec 2022 16:20:44 +0000 Received: from mail-ej1-x633.google.com ([2a00:1450:4864:20::633]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1p18mG-00HSEJ-M3 for linux-arm-kernel@lists.infradead.org; Fri, 02 Dec 2022 16:20:42 +0000 Received: by mail-ej1-x633.google.com with SMTP id ha10so12728381ejb.3 for ; Fri, 02 Dec 2022 08:20:37 -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=Y8LFScKqDqzJLrtmOOWlBwZ43NPyTwr4I3sDAVqBnGc=; b=7s8mLbD6VDNc/lOLdjSMH1X/KN4UtySwF8Rioh+6fuONHVoJ3Rs6jsAMwB1lRaLbEa wmLEK33GR1k9uCEthryrvIxDzAme+CRxhr13biSKLAwQj176m5zgHrs6S8U6NZMdKzv3 5FpUeQD5l85J2CCMg1YktcyfTGWGhlVFEdnhZwxsz6BHg9UizNUcNO6EFJccL5vH8+8N 2rvIgmfrIcvUYtBxFkyCwQJWoVWoM2nxOa3sLHd1vnSCJiE0sC5kKLrhCbG4Vanhxrr4 4rbcbzE8+5SG5AHPk9txwGTCRq7BVvhWSOtpuUfcqtBTn+1+lsA06JSlhbe1FHvh9hbk tm7Q== 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=Y8LFScKqDqzJLrtmOOWlBwZ43NPyTwr4I3sDAVqBnGc=; b=Bt3VKwKkQx9pZveqPmnhw0r3pQTbagaMCRIyYWB3fDFkNZL/8vINTfAE6xo/9Ahsw2 DsETslML99Fp9zpGSjohdE8qJKZdHUjQnGBlUteZ0iCtUcTQs2sx6maTgZO+UamEO3Ju KZ0JAJFy+262UzHegVWA06P2eWV6+Zp7DRY8xPzeMOpJWOwAbu17m8PrWSRxsVxU1kvj TEMkRA46pm7nFT1faa9fdqnSGBKAUUsH0dkZrjmD6D7IpNryNBi3tejsHWyP4wobc/IO 32ud0q9mni8D93nwpn6O9H7A25griEwavwM7rQrLQcCErgZEe4v3o/4D5kHybybsK+T2 ZDRg== X-Gm-Message-State: ANoB5pmWbAuenzJrB7pxuwQ70ZbpW1NGWUwg/HjLe1nQGi5ARus77XUP x9PGbAEWZ9r8Qs2diPrYzugXLg== X-Google-Smtp-Source: AA0mqf5i6upWvhvjXMNyRH65grYqmXIUmKbfuBXzercxFOECmsfQ/bU491DNLSWw5XLYfNioxXgZNw== X-Received: by 2002:a17:906:99c8:b0:7b5:ff35:6715 with SMTP id s8-20020a17090699c800b007b5ff356715mr41992388ejn.255.1669998036795; Fri, 02 Dec 2022 08:20:36 -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 fg7-20020a056402548700b0046b847d6a1csm3052642edb.21.2022.12.02.08.20.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Dec 2022 08:20:35 -0800 (PST) Date: Fri, 2 Dec 2022 17:20:34 +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_082040_757365_46B7F4F7 X-CRM114-Status: GOOD ( 33.84 ) 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 03:39:17PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko >>Sent: Friday, December 2, 2022 1:49 PM >> >>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. > >Thanks! > >> >> >>> >>>> >>>> >>>>>+ >>>>>+ 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. >> > >Our device is not capable of doing this type of switch, but why to assume >that some other HW would not? As I understand generic dpll subsystem must not >be tied to any HW, and you proposal makes it exactly tied to our approaches. >As Vadim requested to have possibility to change pin between source/output >"states" this seems also possible that some HW might have multiple types >possible. How? How do you physically change from EXT connector to SyncE port? That does not make sense. Topology is given. Let's go back to Earth here. >I don't get why "all of the sudden", DPLLA_PIN_TYPE_SUPPORTED can have multiple >values, which means that the user can pick one of those with set command. >Then if HW supports it could redirect signals/setup things accordingly. We have to stritly distinguis between things that are given, wired-up, static and things that could be configured. > >> >> >>>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. >> > >Actually this comment relates to the other commit, could we keep comments >in the threads they belong to please, this would be much easier to track. >But yeah sure, if there is no strong opinion on that we could change it. Ok. > >> >>> >>>> >>>> >>>>>+ >>>>>+ 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. > >Tried to explain on the cover-letter thread, let's discuss there please. Ok. > >> >> >>>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. > >Similar to above, no strong opinion here from me, for shared pin it is >certainly useful. > >> >> >>> >>>> >>>> >>>>>+ } >>>>>+ >>>>> 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. >> > >Well, makes sense, but still need to take a closer look on that. >I could do that on ice-driver part, don't feel strong enough yet to introduce Sure Ice should be ready. >Changes here in ptp_ocp. I think that Vadim said he is going to look at that during the call. My commit introducing this to mlxsw is a nice and simple example how this could be done in ptp_ocp. > >Thank you, >Arkadiusz > >> >> >>> >>>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 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 A21C0C47088 for ; Fri, 2 Dec 2022 16:24:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233540AbiLBQYY (ORCPT ); Fri, 2 Dec 2022 11:24:24 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37238 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234048AbiLBQYJ (ORCPT ); Fri, 2 Dec 2022 11:24:09 -0500 Received: from mail-ej1-x634.google.com (mail-ej1-x634.google.com [IPv6:2a00:1450:4864:20::634]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 49D1C37F82 for ; Fri, 2 Dec 2022 08:20:38 -0800 (PST) Received: by mail-ej1-x634.google.com with SMTP id ud5so12688366ejc.4 for ; Fri, 02 Dec 2022 08:20:38 -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=Y8LFScKqDqzJLrtmOOWlBwZ43NPyTwr4I3sDAVqBnGc=; b=7s8mLbD6VDNc/lOLdjSMH1X/KN4UtySwF8Rioh+6fuONHVoJ3Rs6jsAMwB1lRaLbEa wmLEK33GR1k9uCEthryrvIxDzAme+CRxhr13biSKLAwQj176m5zgHrs6S8U6NZMdKzv3 5FpUeQD5l85J2CCMg1YktcyfTGWGhlVFEdnhZwxsz6BHg9UizNUcNO6EFJccL5vH8+8N 2rvIgmfrIcvUYtBxFkyCwQJWoVWoM2nxOa3sLHd1vnSCJiE0sC5kKLrhCbG4Vanhxrr4 4rbcbzE8+5SG5AHPk9txwGTCRq7BVvhWSOtpuUfcqtBTn+1+lsA06JSlhbe1FHvh9hbk tm7Q== 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=Y8LFScKqDqzJLrtmOOWlBwZ43NPyTwr4I3sDAVqBnGc=; b=L3WK/6XE1nF9vU8pyNW1MW5+3bZjKUJ1kpC9YgxERwxHtDUeKk7H/4XAYH3LBi9s4c RWNsjNrJj6w6S7Uq7+F/gl8MOMs7gmADU+Us6THoPbhRsOeDgsyGC3UcCHzSfCv4nykd t/aU+g20bqJkuxoB51EwX1hC61cRgOkNSkG6P4LE0hmIEGZpKNPjA/DiZUQqmlnlFpeC ZLyT/j7mRGgDV5b/zDbexJtokjuSXZWJW5ZuQDxdUuakbKAMaEAuuzaiFs4JdDeKxLhX EyI5AbUwfo69qnd3VohEmilLsHPedAXSk9v4PPpDVHnHqqgmsjvYJwDIiAHg1Hn/UN8P Bnfg== X-Gm-Message-State: ANoB5pkc2eNFO2YGh3xoUBs5JdZQ+282STphg2/kHHCKEsx9ciLfInjp LpRfD7ONv63bIQ7ElAZjy7cJvg== X-Google-Smtp-Source: AA0mqf5i6upWvhvjXMNyRH65grYqmXIUmKbfuBXzercxFOECmsfQ/bU491DNLSWw5XLYfNioxXgZNw== X-Received: by 2002:a17:906:99c8:b0:7b5:ff35:6715 with SMTP id s8-20020a17090699c800b007b5ff356715mr41992388ejn.255.1669998036795; Fri, 02 Dec 2022 08:20:36 -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 fg7-20020a056402548700b0046b847d6a1csm3052642edb.21.2022.12.02.08.20.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Dec 2022 08:20:35 -0800 (PST) Date: Fri, 2 Dec 2022 17:20:34 +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-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Fri, Dec 02, 2022 at 03:39:17PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko >>Sent: Friday, December 2, 2022 1:49 PM >> >>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. > >Thanks! > >> >> >>> >>>> >>>> >>>>>+ >>>>>+ 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. >> > >Our device is not capable of doing this type of switch, but why to assume >that some other HW would not? As I understand generic dpll subsystem must not >be tied to any HW, and you proposal makes it exactly tied to our approaches. >As Vadim requested to have possibility to change pin between source/output >"states" this seems also possible that some HW might have multiple types >possible. How? How do you physically change from EXT connector to SyncE port? That does not make sense. Topology is given. Let's go back to Earth here. >I don't get why "all of the sudden", DPLLA_PIN_TYPE_SUPPORTED can have multiple >values, which means that the user can pick one of those with set command. >Then if HW supports it could redirect signals/setup things accordingly. We have to stritly distinguis between things that are given, wired-up, static and things that could be configured. > >> >> >>>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. >> > >Actually this comment relates to the other commit, could we keep comments >in the threads they belong to please, this would be much easier to track. >But yeah sure, if there is no strong opinion on that we could change it. Ok. > >> >>> >>>> >>>> >>>>>+ >>>>>+ 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. > >Tried to explain on the cover-letter thread, let's discuss there please. Ok. > >> >> >>>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. > >Similar to above, no strong opinion here from me, for shared pin it is >certainly useful. > >> >> >>> >>>> >>>> >>>>>+ } >>>>>+ >>>>> 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. >> > >Well, makes sense, but still need to take a closer look on that. >I could do that on ice-driver part, don't feel strong enough yet to introduce Sure Ice should be ready. >Changes here in ptp_ocp. I think that Vadim said he is going to look at that during the call. My commit introducing this to mlxsw is a nice and simple example how this could be done in ptp_ocp. > >Thank you, >Arkadiusz > >> >> >>> >>>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 >>>>>