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 B063FEB64D9 for ; Thu, 6 Jul 2023 14:10:48 +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:Content-ID:In-Reply-To: References:Message-ID:Date:Subject:CC:To:From:Reply-To:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=hCs6Np8mUKzSgjrQM2agn/S2BZqGs0UR5G5njJxT8yc=; b=uyRQX7v6T9vb/T bR5cvJ8NyvfI7Z4Ih+Z5IBI8WaDdevPGYE0DnZp6j03b+KJeqSUqo1uMl614wjz6cTXVo79JzdZSx XQ4CkNDqEn/xsqU+yA8LJru3E3A2FdPoD5OeyvHXh/TzLQve/7KIu/nEQPSuWx9hSQff3UALoY8fm 7el62eaBmsUAoBxWnkFHad95keuX7F7w0E+5IxC1aycKm79vl2Mx0x4B3kdnX2sI27L8UtzXqSGRY obhJdvcbpwTr/kZvQWhlpJaultZ6ZZEtJ9kOmbuQ573Z+s/Px4Wclkolpp0sOFb5DLzblgvYgV/6e HYb3ln56webryPg4vuEA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qHPgS-001qLS-1G; Thu, 06 Jul 2023 14:10:12 +0000 Received: from mx0b-0039f301.pphosted.com ([148.163.137.242]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qHPgL-001qK8-1Z for linux-arm-kernel@lists.infradead.org; Thu, 06 Jul 2023 14:10:09 +0000 Received: from pps.filterd (m0174682.ppops.net [127.0.0.1]) by mx0b-0039f301.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 366E0Vvu031766; Thu, 6 Jul 2023 14:09:42 GMT Received: from eur05-vi1-obe.outbound.protection.outlook.com (mail-vi1eur05lp2169.outbound.protection.outlook.com [104.47.17.169]) by mx0b-0039f301.pphosted.com (PPS) with ESMTPS id 3rny0er279-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 06 Jul 2023 14:09:42 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=BCe9en5N2lNkd20egNuNgUJtlZGrhfjnu2BNSsobH9bBwR43BF9xKT2SINVW5+QbcdztZiGMFPdMmebGu7XE5vchiBXaDedJyODLnY0+1J+17n8FenZ/dXY5DVkkXYN2FuXpoE2NmSfYV//XbPUKeNIGvURdybQOBcV+cxjrW52a81slnbfuyHPnDmuHOLiMPR0EmrtJjjhL2kIeekQ7obiG4ycH4WyM/s9hjzqKSbJCqLEDenQEBk0ujIYpteAb44L8AhmGEg+WiVbOK/+whc1E8bGWtvWxDzShSfM/DoFK16KdIf6vgPOB1THpBhLHt7l6AXEaikG27pislSljxg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=GY8mImJXfkmcapswyyAMxgzjb9XQChNvbX9RBJj4Npo=; b=nAIIA3qLGC6zb4Jo95NHQjrMY9Q87jKHFyFQlYahqQYyTLuRsIiqRgVN+/w4zOKHPSPCZC8JWZjNty+6ok87RSv29ygVHkoLJP8dkmrffV3mT6EPS3dZ8A6PilbNlcCGHfqtml9+8mUB97isHd3/uZYpy0slw5xUsxSxvNbjmfs6Gi8PIOwaTuFKrFwlSz6g4U9KWtSVn4cGTG2N8OE1q/hEWnEjq/7PqOUKhIeSGYP/w9jiEfGvPp6Xjqu78Q97cQL5YNZ1i8YAxN7R+/oIu+/2EzcsVr+5NEjyl9KAlINRMDuNsgCGfb1M/xLQjWXxJ2RftUYC0AA2X1JJITPtvg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=epam.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=GY8mImJXfkmcapswyyAMxgzjb9XQChNvbX9RBJj4Npo=; b=EoleIk5Yn51MKNtrAu0zo7LgG/vKPK+ry8dIr6yVv0dYhhZjYm1IOwvdqLVuno/doiA2rJKoKgu7D2sWN9jgY1W5Q/pPuyRXBG80cF18chUjajCv419sGQtOqDTo2FppvDPwniI4WyT9rXzRPdILhxcr7Jv+N9+78o3awWBku7TKBLvaL0YgJOtg6ymbEejSVtF9AQ5+ghW0mFP7bvVQtslUfLXK7Ey8BNOwMrKU/yEvDGWwvATwHNCYiTGUMvZRnR2JGy4PoTEW2SZBvNsjoprjhL+gMBetTdMoHl4FrIRk+mLglwTB6k/BpH9fI+6+eCfmZ6X2SMLvgkqh/srutg== Received: from PA4PR03MB7136.eurprd03.prod.outlook.com (2603:10a6:102:ea::23) by DB3PR0302MB9157.eurprd03.prod.outlook.com (2603:10a6:10:43f::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6565.17; Thu, 6 Jul 2023 14:09:38 +0000 Received: from PA4PR03MB7136.eurprd03.prod.outlook.com ([fe80::528d:e0b6:ecc6:25e5]) by PA4PR03MB7136.eurprd03.prod.outlook.com ([fe80::528d:e0b6:ecc6:25e5%4]) with mapi id 15.20.6565.016; Thu, 6 Jul 2023 14:09:38 +0000 From: Oleksii Moisieiev To: Cristian Marussi CC: "sudeep.holla@arm.com" , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Linus Walleij , "linux-arm-kernel@lists.infradead.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-gpio@vger.kernel.org" Subject: Re: [PATCH v3 2/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support Thread-Topic: [PATCH v3 2/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support Thread-Index: AQHZmJMVZdZ9jTlnl0a4bbb/rGp54K+n/cyAgAT4DIA= Date: Thu, 6 Jul 2023 14:09:38 +0000 Message-ID: <20230706140937.GA821831@EPUAKYIW0A6A> References: In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: PA4PR03MB7136:EE_|DB3PR0302MB9157:EE_ x-ms-office365-filtering-correlation-id: 579725a9-c1c6-4112-aded-08db7e2aa291 x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 0Wul8vBpFZRmr2nO3AgnJBohQsL5+30BPgR8xA61jPdkN5ucJTNi5Iaiabi5rgapgFfaN/8BwN3uo92PQQxKV/AYVEIqGXnOGG9C8MC9j0DUoUPCZuJ8GAfVP53zs85d4tCZlsS88dtWHtTkFOitKC+eGTwr7IdaPTdoQqWEZEPGBDWU4Hmfelyz+gK4IMvVv13GChlf6b9IFCjI7MRVms86guIFb4iTGvo8xvIXnCMKbbvPjDPBaUX1/bzLjG8A7STSAMg/HueDLXhL6mJ0j9X18hDXWBrOwirKaqVJWD4fHq7tuzleOEF102HMBwL/uxRa1jljhpyZSpz2962ZFVcBWd75LNdS1NTwRsrFs2XwaQfVMEf/DkbeLuQjklB793uDOWrE6/kbNF2vwDiF5SM+gcwj/pfIMm0wmRLmF/jD0gAgnHqf1P/4PIQt8fSvRejgGNBL18PszySdU07O8GyRadRmrRhgc6wlF3H78sHyEYpcz9PcDslKdOSIrvavEoAgtW4m7dIXz/VexFJpWc7Pckr9x1VoKGtZvWnu6ul8mtB6GgX6RrhN/KbV67aMEJbuhFvgmOY8OxUUSyjNDDlbgdJuTjK+hgjEI7hzisI= x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:PA4PR03MB7136.eurprd03.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230028)(4636009)(7916004)(136003)(396003)(366004)(376002)(39860400002)(346002)(451199021)(66476007)(54906003)(478600001)(6512007)(1076003)(966005)(9686003)(26005)(6506007)(186003)(86362001)(71200400001)(6486002)(316002)(83380400001)(38100700002)(76116006)(4326008)(66556008)(64756008)(122000001)(66446008)(91956017)(6916009)(66946007)(5660300002)(7416002)(8676002)(8936002)(38070700005)(30864003)(41300700001)(2906002)(33716001)(33656002)(559001)(579004);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?3smjrsSqh92qP6ZGcviZIwn+UV3Tc1/Nysin88g2hxcMZx8NVAksgGNQ/ZGI?= =?us-ascii?Q?3tQWGXzmXa6RAJo8a7lvEccdyDrTcVePJW0ZupHFV4b0Jmaq7gG9o4wvBIpA?= =?us-ascii?Q?ZYmUU0xNIK/TBNiGVDkgxdviraLA7dhvWJThazClQTU4DInWI5EzLGWRI3mD?= =?us-ascii?Q?thWHAoJeLfwy0yRQypgEpnaIQ9xl/hceEHH+jlwLrIv4NGDyQ+Lo8oroKhsz?= =?us-ascii?Q?negYChHk7rXsjOQKCkyapiyhihhpGFxuFjIWhxB5H+5//Ndml/kJzgSbc2Dh?= =?us-ascii?Q?1mRWIl1xcN4lTyFgQU1YepM4PDRiB44nooMwYzfNFIf4OUnaJahtpp4HIgda?= =?us-ascii?Q?gNzjginM0egjqNgq7RUQHJI8+1Xhyk8Qvhjuk8CuvvwL6rGBLiTfUO2aC1GO?= =?us-ascii?Q?58A/jHhU1okigTWPydVhZzS+AwhBsrkzJLyfuT9na8NcGs58atcgnWc68IJ1?= =?us-ascii?Q?p+Bo+nXZZcktv3etVrFdn5zWhMJhKQXNYx279Z9vi7zz/zfgAikV0XzysFlo?= =?us-ascii?Q?TqsrTbILE2vp8IXgjm23Df2mRyswyFzgiGmhws4w/up5LejGAvUyIGtDWapN?= =?us-ascii?Q?MVkL9hu2GOC1QKNW6iUWA4FkhsqQTnTIrwQebHw7JRvgX0wfExkgQavYIqzH?= =?us-ascii?Q?Zwaiy8r/eJHDm7M9imcrdOO/ZTQJK3CG06G+rBfo0Zk3j+Fd188UX4/Ve6kw?= =?us-ascii?Q?l5p3RrQSoFVvv50EpuoZmccff5KJS76Rb343hjWk41V4W9DQEW9P1lF6stu5?= =?us-ascii?Q?Oh3TMyX9/QUiA7rXCnilUPsWRdHZrOxZsLl9WFz4TzlDzN/41ZZgP6hyD3XH?= =?us-ascii?Q?s0lFsoWYHB39ocM4kj1apv0NlsFFb83kwVqWr4Mgw8nzkFQJyTu1lAJWMyoD?= =?us-ascii?Q?g0id4yZ4kUYKJtiC+gYgR+fA3rGSSPCdU0vWEWzz9pIfwNCQDTs9wx9QfRNG?= =?us-ascii?Q?RzRS/Kh3DgFZE1ZykbC8WUP2FrwUyfzT/Yqs7PESjqAUxXvbOyGQ9wTqeMrc?= =?us-ascii?Q?jiIOQgQREApkLUAfiwT6vxA4wuNYALhkoyftaNcjpS5LLpM2dWE4ywaxz4RK?= =?us-ascii?Q?f3G9vsTQ/4M3QQEzIyVjLvzsF9yN0vA7rFPA9LCHwE0xeKXXFcerYIFGSqQk?= =?us-ascii?Q?HMvhtxJNIWPHNQyqk9xDi1hJPNtllErf3SJ/a1TSf8zRwWLCv7gcnfDGq8w7?= =?us-ascii?Q?AO7id1qWqR5d7fVx0Fq0hhpcDVZmTLfOs4FPhiPp9ebXFd7BaElFff8GrqY8?= =?us-ascii?Q?WWmPqtKKz3kfSKDe+LecUhuSGnuRxC73jNUwm/BrHYq4dABXRneijf/w42oT?= =?us-ascii?Q?/89CpSvvNbvXgyB0siWgmkllWXGJDorA/5CM7nTHYrIwYA0S+FBetYDPP6Ah?= =?us-ascii?Q?gVZLuDinESgpcCh1ZlrTmW7ptrXp7cZL4ZsEl1UTwT660m/Ez8GoyxyWZQtI?= =?us-ascii?Q?TiDEPEXpkPR4afPqyb+lHZaUaiKHEHKv8j/cDih4S1wLcsvj0dVQXmP8roDt?= =?us-ascii?Q?0iCImaOFBNVAEfX5k4YS2hdI5qYCP+qM5Juc3HA8hAthOpmyWlc+CdBH6oNZ?= =?us-ascii?Q?P9v4y+FnyQtIVz1lKemA2sNuLkCHKWLvXHvOUUMsz9DMERGTe0MBKJqE4589?= =?us-ascii?Q?Ew=3D=3D?= Content-ID: <62E652B3E0739E40B5A7104612CA27BE@eurprd03.prod.outlook.com> MIME-Version: 1.0 X-OriginatorOrg: epam.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: PA4PR03MB7136.eurprd03.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 579725a9-c1c6-4112-aded-08db7e2aa291 X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Jul 2023 14:09:38.3658 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: b41b72d0-4e9f-4c26-8a69-f949f367c91d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: xMA+XhE3uz1rtr+FDSPDDxqROKTrJggwZmDxuh/mL0LROX5YoagYfa3ae0bmRgobRqPWzBp8uz1ON+OcMzjbEgWAxSToM+EdfL7PWgjMUaA= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB3PR0302MB9157 X-Proofpoint-ORIG-GUID: GvPGu0JSzVbhGrB4enj-Va8tWwIRP47J X-Proofpoint-GUID: GvPGu0JSzVbhGrB4enj-Va8tWwIRP47J X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26 definitions=2023-07-06_10,2023-07-06_02,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 suspectscore=0 priorityscore=1501 malwarescore=0 lowpriorityscore=0 mlxlogscore=999 phishscore=0 spamscore=0 impostorscore=0 bulkscore=0 adultscore=0 mlxscore=0 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2305260000 definitions=main-2307060127 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230706_071005_799904_079560BE X-CRM114-Status: GOOD ( 35.38 ) 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 Hi Cristian, Sorry for late answer. Please see below. On Mon, Jul 03, 2023 at 11:16:47AM +0100, Cristian Marussi wrote: > On Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev wrote: > > scmi: Introduce pinctrl SCMI protocol driver > > > > Hi Oleksii, > > spurios line above. Yep thanks, I will remove. > > > Add basic implementation of the SCMI v3.2 pincontrol protocol > > excluding GPIO support. All pinctrl related callbacks and operations > > are exposed in the include/linux/scmi_protocol.h > > > > As Andy said already, you can drop the second sentence here, but I would > ALSO drop the GPIO part in the first sentence, since there is nothing > specific to GPIO in the SCMI spec and this patch is about the SCMI protocol > not the pinctrl driver. > I've added few words about GPIO because in v2 series Michal Simek asked about it: https://lore.kernel.org/linux-arm-kernel/5bf0e975-d314-171f-b6a8-c1c1c7198cd3@amd.com/ So I've decided to mention that there is still no GPIO support in the commit message to avoid this type of questions in future. But I agree that the commit message looks weird and will try to rephrase it. > > Signed-off-by: Oleksii Moisieiev > > --- > > MAINTAINERS | 6 + > > drivers/firmware/arm_scmi/Makefile | 2 +- > > drivers/firmware/arm_scmi/driver.c | 2 + > > drivers/firmware/arm_scmi/pinctrl.c | 836 ++++++++++++++++++++++++++ > > drivers/firmware/arm_scmi/protocols.h | 1 + > > include/linux/scmi_protocol.h | 47 ++ > > 6 files changed, 893 insertions(+), 1 deletion(-) > > create mode 100644 drivers/firmware/arm_scmi/pinctrl.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 0dab9737ec16..297b2512963d 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -20522,6 +20522,12 @@ F: include/linux/sc[mp]i_protocol.h > > F: include/trace/events/scmi.h > > F: include/uapi/linux/virtio_scmi.h > > > > +PINCTRL DRIVER FOR SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE (SCPI/SCMI) > > SCPI is a leftover here I suppose... > Thanks. I'll fix it. > > +M: Oleksii Moisieiev > > +L: linux-arm-kernel@lists.infradead.org > > +S: Maintained > > +F: drivers/firmware/arm_scmi/pinctrl.c > > + > > SYSTEM RESET/SHUTDOWN DRIVERS > > M: Sebastian Reichel > > L: linux-pm@vger.kernel.org > > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile > > index b31d78fa66cc..603430ec0bfe 100644 > > --- a/drivers/firmware/arm_scmi/Makefile > > +++ b/drivers/firmware/arm_scmi/Makefile > > @@ -10,7 +10,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o > > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o > > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o > > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o > > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y) > > > > obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > > index 5be931a07c84..a9fd337b9596 100644 > > --- a/drivers/firmware/arm_scmi/driver.c > > +++ b/drivers/firmware/arm_scmi/driver.c > > @@ -3025,6 +3025,7 @@ static int __init scmi_driver_init(void) > > scmi_voltage_register(); > > scmi_system_register(); > > scmi_powercap_register(); > > + scmi_pinctrl_register(); > > > > return platform_driver_register(&scmi_driver); > > } > > @@ -3042,6 +3043,7 @@ static void __exit scmi_driver_exit(void) > > scmi_voltage_unregister(); > > scmi_system_unregister(); > > scmi_powercap_unregister(); > > + scmi_pinctrl_unregister(); > > > > scmi_transports_exit(); > > > > diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c > > new file mode 100644 > > index 000000000000..fc0fcc26dfb6 > > --- /dev/null > > +++ b/drivers/firmware/arm_scmi/pinctrl.c > > @@ -0,0 +1,836 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * System Control and Management Interface (SCMI) Pinctrl Protocol > > + * > > + * Copyright (C) 2023 EPAM > > + */ > > + > > +#include > > +#include > > +#include > > + > > +#include "protocols.h" > > + > > +#define REG_TYPE_BITS GENMASK(9, 8) > > +#define REG_CONFIG GENMASK(7, 0) > > + > > +#define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16)) > > +#define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0)) > > +#define GET_FUNCTIONS_NR(x) le32_get_bits((x), GENMASK(15, 0)) > > + > > +#define EXT_NAME_FLAG(x) le32_get_bits((x), BIT(31)) > > +#define NUM_ELEMS(x) le32_get_bits((x), GENMASK(15, 0)) > > + > > +#define REMAINING(x) le32_get_bits((x), GENMASK(31, 16)) > > +#define RETURNED(x) le32_get_bits((x), GENMASK(11, 0)) > > + > > +enum scmi_pinctrl_protocol_cmd { > > + PINCTRL_ATTRIBUTES = 0x3, > > + PINCTRL_LIST_ASSOCIATIONS = 0x4, > > + PINCTRL_CONFIG_GET = 0x5, > > + PINCTRL_CONFIG_SET = 0x6, > > + PINCTRL_FUNCTION_SELECT = 0x7, > > + PINCTRL_REQUEST = 0x8, > > + PINCTRL_RELEASE = 0x9, > > + PINCTRL_NAME_GET = 0xa, > > + PINCTRL_SET_PERMISSIONS = 0xb > > +}; > > + > > +struct scmi_msg_conf_set { > > + __le32 identifier; > > + __le32 attributes; > > + __le32 config_value; > > +}; > > + > > +struct scmi_msg_conf_get { > > + __le32 identifier; > > + __le32 attributes; > > +}; > > + > > +struct scmi_msg_pinctrl_protocol_attributes { > > + __le32 attributes_low; > > + __le32 attributes_high; > > +}; > > + > > +struct scmi_msg_pinctrl_attributes { > > + __le32 identifier; > > + __le32 flags; > > +}; > > + > > +struct scmi_resp_pinctrl_attributes { > > + __le32 attributes; > > + u8 name[SCMI_SHORT_NAME_MAX_SIZE]; > > +}; > > + > > +struct scmi_msg_pinctrl_list_assoc { > > + __le32 identifier; > > + __le32 flags; > > + __le32 index; > > +}; > > + > > +struct scmi_resp_pinctrl_list_assoc { > > + __le32 flags; > > + __le16 array[]; > > +}; > > + > > +struct scmi_msg_func_set { > > + __le32 identifier; > > + __le32 function_id; > > + __le32 flags; > > +}; > > + > > +struct scmi_msg_request { > > + __le32 identifier; > > + __le32 flags; > > +}; > > + > > +struct scmi_group_info { > > + bool present; > > + char name[SCMI_MAX_STR_SIZE]; > > + unsigned int *group_pins; > > + unsigned int nr_pins; > > +}; > > + > > +struct scmi_function_info { > > + bool present; > > + char name[SCMI_MAX_STR_SIZE]; > > + unsigned int *groups; > > + unsigned int nr_groups; > > +}; > > + > > A small note related to Andy remarks about directly embedding here pinctrl > subsystem structures (like pingroup / pinfucntion) that I forgot to say > in my reply to him. > > These structs above indeed are very similar to the Pinctrl ones but this is > the protocol layer inside SCMI, I would not mix here stuff from the Pinctrl > subsystem which is, at the end the, one of the possible users of this layer > (via the SCMI pinctrl driver) but not necessarily the only one in the > future; moreover Pinctrl subsystem is not even needed at all if you think > about a testing scenario, so I would not build up a dependency here between > SCMI and Pinctrl by using Pinctrl structures...what if these latter change > in the future ? > > All of this to just say this is fine for me as it is now :D > I agree with you. What we currently have is that scmi pinctrl protocol is not bound to pinctrl-subsystem so in case of some changes in the pinctrl - no need to change the protocol implementation. Also, as I mentioned in v2: I can't use pincfunction it has the following groups definition: const char * const *groups; Which is meant to be constantly allocated. So I when I try to gather list of groups in pinctrl_scmi_get_function_groups I will receive compilation error. Pinctrl subsystem was designed to use statically defined pins/groups/functions so we can't use those structures on lazy allocations. > > +struct scmi_pin_info { > > + bool present; > > + char name[SCMI_MAX_STR_SIZE]; > > +}; > > + > > +struct scmi_pinctrl_info { > > + u32 version; > > + int nr_groups; > > + int nr_functions; > > + int nr_pins; > > + struct scmi_group_info *groups; > > + struct scmi_function_info *functions; > > + struct scmi_pin_info *pins; > > +}; > > + > > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph, > > + struct scmi_pinctrl_info *pi) > > +{ > > + int ret; > > + struct scmi_xfer *t; > > + struct scmi_msg_pinctrl_protocol_attributes *attr; > > + > > + if (!pi) > > + return -EINVAL; > > You can drop this, cannot happen given the code paths. > Ok. thanks. > > + > > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, > > + 0, sizeof(*attr), &t); > > + if (ret) > > + return ret; > > + > > + attr = t->rx.buf; > > + > > + ret = ph->xops->do_xfer(ph, t); > > + if (!ret) { > > + pi->nr_functions = GET_FUNCTIONS_NR(attr->attributes_high); > > + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low); > > + pi->nr_pins = GET_PINS_NR(attr->attributes_low); > > + } > > + > > + ph->xops->xfer_put(ph, t); > > + return ret; > > +} > > + > > +static int scmi_pinctrl_get_count(const struct scmi_protocol_handle *ph, > > + enum scmi_pinctrl_selector_type type) > > +{ > > + struct scmi_pinctrl_info *pi; > > + > > + pi = ph->get_priv(ph); > > + if (!pi) > > + return -ENODEV; > > You dont need to check for NULL here and nowhere else. > You set protocol private data with set_priv at the end of protocol init > which is called as soon as a user tries to use this protocol operations, > so it cannot ever be NULL in any of these following ops. > And what if I call set_priv(ph, NULL) on init stage? As I can see there is no check for NULL in scmi_set_protocol_priv. So theoretically I'm able to set ph->priv = NULL. Or did I missed some check in SCMI driver? Or maybe priv = NULL is expected scenario and I shouldn't return error here? > > + > > + switch (type) { > > + case PIN_TYPE: > > + return pi->nr_pins; > > + case GROUP_TYPE: > > + return pi->nr_groups; > > + case FUNCTION_TYPE: > > + return pi->nr_functions; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph, > > + u32 identifier, > > + enum scmi_pinctrl_selector_type type) > > +{ > > + int value; > > + > > + value = scmi_pinctrl_get_count(ph, type); > > + if (value < 0) > > + return value; > > + > > + if (identifier >= value) > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > +static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph, > > + enum scmi_pinctrl_selector_type type, > > + u32 selector, char *name, > > + unsigned int *n_elems) > > +{ > > + int ret; > > + struct scmi_xfer *t; > > + struct scmi_msg_pinctrl_attributes *tx; > > + struct scmi_resp_pinctrl_attributes *rx; > > + > > + if (!name) > > + return -EINVAL; > > + > > + ret = scmi_pinctrl_validate_id(ph, selector, type); > > + if (ret) > > + return ret; > > + > > + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx), > > + sizeof(*rx), &t); > > + if (ret) > > + return ret; > > + > > + tx = t->tx.buf; > > + rx = t->rx.buf; > > + tx->identifier = cpu_to_le32(selector); > > + tx->flags = cpu_to_le32(type); > > + > > + ret = ph->xops->do_xfer(ph, t); > > + if (!ret) { > > + if (n_elems) > > + *n_elems = NUM_ELEMS(rx->attributes); > > + > > + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE); > > + } > > + > > + ph->xops->xfer_put(ph, t); > > + > > + /* > > + * If supported overwrite short name with the extended one; > > + * on error just carry on and use already provided short name. > > + */ > > + if (!ret && EXT_NAME_FLAG(rx->attributes)) > > + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector, > > + (u32 *)&type, name, > > + SCMI_MAX_STR_SIZE); > > + return ret; > > +} > > + > > [snip] > > > + > > +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph, > > + u32 selector, > > + struct scmi_group_info *group) > > +{ > > + int ret; > > + > > + if (!group) > > + return -EINVAL; > > + > > + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector, > > + group->name, > > + &group->nr_pins); > > + if (ret) > > + return ret; > > + > > + if (!group->nr_pins) { > > + dev_err(ph->dev, "Group %d has 0 elements", selector); > > + return -ENODATA; > > + } > > + > > + group->group_pins = devm_kmalloc_array(ph->dev, group->nr_pins, > > + sizeof(*group->group_pins), > > + GFP_KERNEL); > > + if (!group->group_pins) > > + return -ENOMEM; > > This is a lazy-allocation happening outside of the protocol init path so you > are rigthly tracking it manually here and in protocol_deinit, BUT you > should not use devm_ helpers, it is fuorviating even though harmless; > just plain kmalloc/kcalloc/kfree will do. (As I said oin the reply to > Andy I miss this previously, apologies) > Thank you and Andy for the observations. I will refactor. > > + > > + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE, > > + group->nr_pins, group->group_pins); > > + if (ret) { > > + devm_kfree(ph->dev, group->group_pins); > > kfree > Ok. > > + return ret; > > + } > > + > > + group->present = true; > > + return 0; > > +} > > + > > +static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph, > > + u32 selector, const char **name) > > +{ > > + struct scmi_pinctrl_info *pi; > > + > > + if (!name) > > + return -EINVAL; > > + > > + pi = ph->get_priv(ph); > > + if (!pi) > > Ditto. > > > + return -EINVAL; > > + > > + if (selector > pi->nr_groups) > > + return -EINVAL; > > selector >= ? > Right. Thanks. > > + > > + if (!pi->groups[selector].present) { > > + int ret; > > + > > + ret = scmi_pinctrl_get_group_info(ph, selector, > > + &pi->groups[selector]); > > + if (ret) > > + return ret; > > + } > > + > > + *name = pi->groups[selector].name; > > + > > + return 0; > > +} > > + > > +static int scmi_pinctrl_get_group_pins(const struct scmi_protocol_handle *ph, > > + u32 selector, const unsigned int **pins, > > + unsigned int *nr_pins) > > +{ > > + struct scmi_pinctrl_info *pi; > > + > > + if (!pins || !nr_pins) > > + return -EINVAL; > > + > > + pi = ph->get_priv(ph); > > + if (!pi) > > + return -EINVAL; > > Ditto. > > > + > > + if (selector > pi->nr_groups) > > + return -EINVAL; > > + > > selector >= ? > Yes, thanks. > > + if (!pi->groups[selector].present) { > > + int ret; > > + > > + ret = scmi_pinctrl_get_group_info(ph, selector, > > + &pi->groups[selector]); > > + if (ret) > > + return ret; > > + } > > + > > + *pins = pi->groups[selector].group_pins; > > + *nr_pins = pi->groups[selector].nr_pins; > > + > > + return 0; > > +} > > + > > +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph, > > + u32 selector, > > + struct scmi_function_info *func) > > +{ > > + int ret; > > + > > + if (!func) > > + return -EINVAL; > > + > > + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector, > > + func->name, > > + &func->nr_groups); > > + if (ret) > > + return ret; > > + > > + if (!func->nr_groups) { > > + dev_err(ph->dev, "Function %d has 0 elements", selector); > > + return -ENODATA; > > + } > > + > > + func->groups = devm_kmalloc_array(ph->dev, func->nr_groups, > > + sizeof(*func->groups), > > + GFP_KERNEL); > > + if (!func->groups) > > + return -ENOMEM; > > Same as above...lazy allocation properly tracked BUT do not use devm_ > variants. > Ok. > > + > > + ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE, > > + func->nr_groups, func->groups); > > + if (ret) { > > + devm_kfree(ph->dev, func->groups); > > + return ret; > > kfree > > > + } > > + > > + func->present = true; > > + return 0; > > +} > > + > > +static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph, > > + u32 selector, const char **name) > > +{ > > + struct scmi_pinctrl_info *pi; > > + > > + if (!name) > > + return -EINVAL; > > + > > + pi = ph->get_priv(ph); > > + if (!pi) > > + return -EINVAL; > > Ditto. > > > + > > + if (selector > pi->nr_functions) > > + return -EINVAL; > > selector >= ? > Ok. thanks. > > + > > + if (!pi->functions[selector].present) { > > + int ret; > > + > > + ret = scmi_pinctrl_get_function_info(ph, selector, > > + &pi->functions[selector]); > > + if (ret) > > + return ret; > > + } > > + > > + *name = pi->functions[selector].name; > > + return 0; > > +} > > + > > +static int scmi_pinctrl_get_function_groups(const struct scmi_protocol_handle *ph, > > + u32 selector, > > + unsigned int *nr_groups, > > + const unsigned int **groups) > > +{ > > + struct scmi_pinctrl_info *pi; > > + > > + if (!groups || !nr_groups) > > + return -EINVAL; > > + > > + pi = ph->get_priv(ph); > > + if (!pi) > > + return -EINVAL; > > Ditto. > > > + > > + if (selector > pi->nr_functions) > > + return -EINVAL; > > selector >= ? > Ok. thanks. > > + > > + if (!pi->functions[selector].present) { > > + int ret; > > + > > + ret = scmi_pinctrl_get_function_info(ph, selector, > > + &pi->functions[selector]); > > + if (ret) > > + return ret; > > + } > > + > > + *groups = pi->functions[selector].groups; > > + *nr_groups = pi->functions[selector].nr_groups; > > + > > + return 0; > > +} > > + > > +static int scmi_pinctrl_set_mux(const struct scmi_protocol_handle *ph, > > + u32 selector, u32 group) > > +{ > > + return scmi_pinctrl_function_select(ph, group, GROUP_TYPE, > > + selector); > > +} > > + > > +static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph, > > + u32 selector, struct scmi_pin_info *pin) > > +{ > > + int ret; > > + struct scmi_pinctrl_info *pi; > > + > > + if (!pin) > > + return -EINVAL; > > + > > + pi = ph->get_priv(ph); > > + if (!pi) > > + return -EINVAL; > > Ditto. > > > + > > + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector, > > + pin->name, NULL); > > + if (ret) > > + return ret; > > + > > + pin->present = true; > > + return 0; > > +} > > + > > +static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph, > > + u32 selector, const char **name) > > +{ > > + struct scmi_pinctrl_info *pi; > > + > > + if (!name) > > + return -EINVAL; > > + > > + pi = ph->get_priv(ph); > > + if (!pi) > > + return -EINVAL; > > Ditto. > > > + > > + if (selector > pi->nr_pins) > > + return -EINVAL; > > selector >= ? > Ok. thanks. > > + > > + if (!pi->pins[selector].present) { > > + int ret; > > + > > + ret = scmi_pinctrl_get_pin_info(ph, selector, > > + &pi->pins[selector]); > > + if (ret) > > + return ret; > > + } > > + > > + *name = pi->pins[selector].name; > > + > > + return 0; > > +} > > + > > +static int scmi_pinctrl_get_name(const struct scmi_protocol_handle *ph, > > + u32 selector, > > + enum scmi_pinctrl_selector_type type, > > + const char **name) > > +{ > > + switch (type) { > > + case PIN_TYPE: > > + return scmi_pinctrl_get_pin_name(ph, selector, name); > > + case GROUP_TYPE: > > + return scmi_pinctrl_get_group_name(ph, selector, name); > > + case FUNCTION_TYPE: > > + return scmi_pinctrl_get_function_name(ph, selector, name); > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = { > > + .get_count = scmi_pinctrl_get_count, > > + .get_name = scmi_pinctrl_get_name, > > + .get_group_pins = scmi_pinctrl_get_group_pins, > > + .get_function_groups = scmi_pinctrl_get_function_groups, > > + .set_mux = scmi_pinctrl_set_mux, > > + .get_config = scmi_pinctrl_get_config, > > + .set_config = scmi_pinctrl_set_config, > > + .request_pin = scmi_pinctrl_request_pin, > > + .free_pin = scmi_pinctrl_free_pin > > +}; > > + > > +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph) > > +{ > > + int ret; > > + u32 version; > > + struct scmi_pinctrl_info *pinfo; > > + > > + ret = ph->xops->version_get(ph, &version); > > + if (ret) > > + return ret; > > + > > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n", > > + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); > > + > > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL); > > + if (!pinfo) > > + return -ENOMEM; > > + > > + ret = scmi_pinctrl_attributes_get(ph, pinfo); > > + if (ret) > > + return ret; > > + > > + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins, > > + sizeof(*pinfo->pins), > > + GFP_KERNEL); > > + if (!pinfo->pins) > > + return -ENOMEM; > > + > > + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups, > > + sizeof(*pinfo->groups), > > + GFP_KERNEL); > > + if (!pinfo->groups) > > + return -ENOMEM; > > + > > + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions, > > + sizeof(*pinfo->functions), > > + GFP_KERNEL); > > + if (!pinfo->functions) > > + return -ENOMEM; > > + > > + pinfo->version = version; > > + > > + return ph->set_priv(ph, pinfo); > > +} > > + > > +static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph) > > +{ > > + int i; > > + struct scmi_pinctrl_info *pi; > > + > > + pi = ph->get_priv(ph); > > + if (!pi) > > + return -EINVAL; > > Ditto. You never get even here if protocol init had not succesfully > completed. > > > + > > + for (i = 0; i < pi->nr_groups; i++) > > + if (pi->groups[i].present) { > > + devm_kfree(ph->dev, pi->groups[i].group_pins); > > kfree..you are managing these. > > > + pi->groups[i].present = false; > > + } > > + > > + for (i = 0; i < pi->nr_functions; i++) > > + if (pi->functions[i].present) { > > + devm_kfree(ph->dev, pi->functions[i].groups); > > kfree..you are managing these. > > > + pi->functions[i].present = false; > > + } > > + > > + return 0; > > +} > > + > > +static const struct scmi_protocol scmi_pinctrl = { > > + .id = SCMI_PROTOCOL_PINCTRL, > > + .owner = THIS_MODULE, > > + .instance_init = &scmi_pinctrl_protocol_init, > > + .instance_deinit = &scmi_pinctrl_protocol_deinit, > > + .ops = &pinctrl_proto_ops, > > +}; > > + > > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl) > > diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h > > index b3c6314bb4b8..674f949354f9 100644 > > --- a/drivers/firmware/arm_scmi/protocols.h > > +++ b/drivers/firmware/arm_scmi/protocols.h > > @@ -346,5 +346,6 @@ DECLARE_SCMI_REGISTER_UNREGISTER(sensors); > > DECLARE_SCMI_REGISTER_UNREGISTER(voltage); > > DECLARE_SCMI_REGISTER_UNREGISTER(system); > > DECLARE_SCMI_REGISTER_UNREGISTER(powercap); > > +DECLARE_SCMI_REGISTER_UNREGISTER(pinctrl); > > > > #endif /* _SCMI_PROTOCOLS_H */ > > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h > > index 0ce5746a4470..97631783a5a4 100644 > > --- a/include/linux/scmi_protocol.h > > +++ b/include/linux/scmi_protocol.h > > @@ -735,6 +735,52 @@ struct scmi_notify_ops { > > struct notifier_block *nb); > > }; > > > > +enum scmi_pinctrl_selector_type { > > + PIN_TYPE = 0, > > + GROUP_TYPE, > > + FUNCTION_TYPE > > +}; > > + > > +/** > > + * struct scmi_pinctrl_proto_ops - represents the various operations provided > > + * by SCMI Pinctrl Protocol > > + * > > + * @get_count: returns count of the registered elements in given type > > + * @get_name: returns name by index of given type > > + * @get_group_pins: returns the set of pins, assigned to the specified group > > + * @get_function_groups: returns the set of groups, assigned to the specified > > + * function > > + * @set_mux: set muxing function for groups of pins > > + * @get_config: returns configuration parameter for pin or group > > + * @set_config: sets the configuration parameter for pin or group > > + * @request_pin: aquire pin before selecting mux setting > > + * @free_pin: frees pin, acquired by request_pin call > > + */ > > +struct scmi_pinctrl_proto_ops { > > + int (*get_count)(const struct scmi_protocol_handle *ph, > > + enum scmi_pinctrl_selector_type type); > > + int (*get_name)(const struct scmi_protocol_handle *ph, > > + u32 selector, > > + enum scmi_pinctrl_selector_type type, > > + const char **name); > > + int (*get_group_pins)(const struct scmi_protocol_handle *ph, > > + u32 selector, > > + const unsigned int **pins, unsigned int *nr_pins); > > + int (*get_function_groups)(const struct scmi_protocol_handle *ph, > > + u32 selector, unsigned int *nr_groups, > > + const unsigned int **groups); > > + int (*set_mux)(const struct scmi_protocol_handle *ph, u32 selector, > > + u32 group); > > + int (*get_config)(const struct scmi_protocol_handle *ph, u32 selector, > > + enum scmi_pinctrl_selector_type type, > > + u8 config_type, unsigned long *config_value); > > + int (*set_config)(const struct scmi_protocol_handle *ph, u32 selector, > > + enum scmi_pinctrl_selector_type type, > > + u8 config_type, unsigned long config_value); > > + int (*request_pin)(const struct scmi_protocol_handle *ph, u32 pin); > > + int (*free_pin)(const struct scmi_protocol_handle *ph, u32 pin); > > +}; > > + > > Can you move all of these before .notify_ops where all others protocol > ops lives ? ... and rename all pinctrl_ops to match the "._" > pattern like other ops (i.e. count_get/name_get ... instead get_count/get_name) > I will do this. > > /** > > * struct scmi_handle - Handle returned to ARM SCMI clients for usage. > > * > > @@ -783,6 +829,7 @@ enum scmi_std_protocol { > > SCMI_PROTOCOL_RESET = 0x16, > > SCMI_PROTOCOL_VOLTAGE = 0x17, > > SCMI_PROTOCOL_POWERCAP = 0x18, > > + SCMI_PROTOCOL_PINCTRL = 0x19, > > }; > > > > Thanks, > Cristian Thanks, Oleksii. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel