Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f6d0c919-d6da-4361-79ed-1f7fca4d2121@oss.nxp.com>
Date:   Tue, 25 Apr 2023 10:13:56 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
To:     Oleksii Moisieiev <Oleksii_Moisieiev@epam.com>,
        Cristian Marussi <cristian.marussi@arm.com>
Cc:     "sudeep.holla@arm.com" <sudeep.holla@arm.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "michal.simek@amd.com" <michal.simek@amd.com>,
        "vincent.guittot@linaro.org" <vincent.guittot@linaro.org>,
        "souvik.chakravarty@arm.com" <souvik.chakravarty@arm.com>
References: <cover.1680793130.git.oleksii_moisieiev@epam.com>
 <54119b2cb43e29f69c5858a5320d3a58f23fed21.1680793130.git.oleksii_moisieiev@epam.com>
 <ZDcqx9JVMvqr2WYu@e120937-lin>
 <6dc456ff-7fc6-3b73-3727-dd048e9a9629@oss.nxp.com>
 <f73f39e2-81dd-4204-a3be-c5e7f5e54c1b@epam.com>
 <b9eca9a4-f4b6-b98c-2861-eb14380d2d5f@oss.nxp.com>
 <3e696235-a772-f988-da3d-2166f827267b@epam.com>
From:   Peng Fan <peng.fan@oss.nxp.com>
In-Reply-To: <3e696235-a772-f988-da3d-2166f827267b@epam.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: DU0PR04MB9417:EE_|DB9PR04MB8265:EE_
X-MS-Office365-Filtering-Correlation-Id: 38006719-a934-4697-2bf4-08db4532c348
X-MS-Exchange-SharedMailbox-RoutingAgent-Processed: True
X-MS-Exchange-SenderADCheck: 1
X-MS-Exchange-AntiSpam-Relay: 0
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: 3iSI2eVucUlFhFxQ1XJYaXmGc8+rSWyi6WievKVQZZnAt9YM9XDtoL9aktS/jb5KS91HlA+aArCcLuggILvRxamA+eklkfx7+pZlllmfzUyIPk7T9wvYg+rG+Qr3223YTF7jznvL8pax1gfLHen8O+wEwfze3jvMTdRJXbWZ7J1njF+0qbwp+KN59JFxGVBOoZkiio5D59bvEOky1DnAsHuLHW7y7nU73AohI1FUV9Mp4sRNl32k6LbMRFuuo6iCKbo/gp2sg0U7L/X1Er4ta265ieYP/TmuvC5r4ATRNOUcxIKTNyprydGCJ246VS0ae39b3MMtF98qpYinr0MHUtaJrhMhtRcstEqR1Lwean3IeuWOfV3aVuYM/WgXhU7kH6t5c47///uRRM5X7mzRORFWHc2F39ybJhR+KuD1RLOgKOHC17dctAK5rfftNMfDZv5knNB5HRjDugNifaYX++VTXRJxcxdnBZlqPSh1EmDPRrdsiHbPNZp4uuoI5TkQJTNsggSdcdeou4LCJZWEgd5w0LZAotCy5+HXdFEBrzPlpiGg7zhz3S6L4R2QJ539pYmiCq/w6mTIjRcJ4HylWoG82DBo293BELF0RZ6l/i8LjxazPzCf10w5b366/BZxJRzcGMPFvpKIGcPoDQeE+/fjKrK5YCFco+tACDvAnQU=
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DU0PR04MB9417.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230028)(4636009)(39860400002)(366004)(136003)(396003)(376002)(346002)(451199021)(30864003)(2906002)(66476007)(66556008)(66946007)(316002)(4326008)(44832011)(7416002)(5660300002)(8936002)(8676002)(41300700001)(31696002)(86362001)(186003)(26005)(6512007)(53546011)(38100700002)(478600001)(6666004)(966005)(6486002)(83380400001)(31686004)(2616005)(6506007)(110136005)(54906003)(45980500001)(43740500002)(559001)(579004);DIR:OUT;SFP:1101;
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RG55Q0ZwdHdldlYxcE1hT3Z1SWVTYUdYK2V5YlZqUEloNzA0WFlHNUNFMTRl?=
 =?utf-8?B?SkRjZXgwTGxUaVl0dnMzcGc5cnRibnBXZTMwWnV1bHYyWDQ1WG5lV0hLSWND?=
 =?utf-8?B?S2NoV1pvY0lDNW1ZM1Rwdk1qb2ZHSnpEUFc4bzR3Q2QxbWx4OTBnOUYvRWVV?=
 =?utf-8?B?ZjVJN3BPZnpZRVMwcUFYY09waDFKVTZqRHRuQ1hIZExBU252eWR3c3NodVAx?=
 =?utf-8?B?NmR5cHV4c29vQkFkdmt1V2pKbkEvR3I4T0JOV1UvNU96dnNKTnNPWGZDU0tn?=
 =?utf-8?B?VTE1U216TjZoS3BtSndlRjNpYk5RZElleEhnbkVZREV1RjgwY3NBT2ZCODhN?=
 =?utf-8?B?SUZVTWdtMFU3cDZodDlSNDRJTDBhakZyUUtkZWc1Z1dOMHQwbXF5V0RnaTFa?=
 =?utf-8?B?TXJhTTVZbi9BYUwyV2FzRVU5UG9KdGdyK1ErYkZFSEZSemllMDM1TUdCM3Y0?=
 =?utf-8?B?ei9CN2Y5MXpLMW9MeWpvL0N5KzQybmt0Slg4S2ZnNmthS3BLOHZUK0lUS1Iw?=
 =?utf-8?B?VjVUREZTNnFwdlhBUjZLbnBQZ2lZN3psSzRZNW43TUh1Unh2S05YUndPa0Jm?=
 =?utf-8?B?V3VUVUdteFBSOHQ0aUVPeFRNUVlIZm1EYWEvZmt3eUdiazg2T2xqSXVKeUtX?=
 =?utf-8?B?b211YjlZWTRHaHFSekdKeVhXZUpLbjAxeDZONGRFY0dQbVVsS2RYWXpvZ3VC?=
 =?utf-8?B?ZEIwYmxuVXo1bXZ0ZE85d2ZqUTZEUGRYV1czejlIVnIrR01vSmkwWFVJYVhs?=
 =?utf-8?B?dGJVdGtrQ3U1N1YzdkIvSllXSllWNERhN3AxSThGaCt4NmgrTEZLRk5LblFV?=
 =?utf-8?B?MzFIVjlzTDNvOXF4MGN5YTAyTllONmlCSndybitDS0kwMWJaZ2NJcTJJNFMy?=
 =?utf-8?B?d2ZiOGlBV2tUTXJJeFAvUXJvcmhPV0xKMlNVcmZhVTdsSFB2ZFJtTHJqZldu?=
 =?utf-8?B?T3d1bStXM2FyNUNMWHMycW5sWFF4dWJRNFBiblVZVG8vUTJlWkZPWkl2RTB2?=
 =?utf-8?B?TElReVNPUjg5UGJrcmlHVjF3US9ZMUJCWkdSNWtyMTZWY3AvYTE4dU1PQlVx?=
 =?utf-8?B?RCtpeGNhc2ZQYjFzOFJPS2QyazZCaGYybk9JaEdybHowWlBCNHp0Z3J2UGlr?=
 =?utf-8?B?S0pCNklZQ0hHZU96aUlrcXREZ3FEekhsbHNGZUQrRnFka3FPWUxiMGRBY2x1?=
 =?utf-8?B?R3FvelBZTzlBaG92OFNCWTBacFliWHNqZ3VFL2ZyeEJ0aUQ3NGxMVmRwcS9G?=
 =?utf-8?B?dkRnN1lyNFdhWjNIaWZwMEtZYzZ2a3hzZ2h3andhOFRHaTBjdExFTHpEWjZn?=
 =?utf-8?B?NHduTncyNDFHaVRkVmRmN0I3NTArcUtQeFZTVU9CaFJiL0JrMXNjaFU5SExo?=
 =?utf-8?B?SW1Xbjk2RkIrZWlQNm03MEgzSWtBeVY5NktlaW83Y20rN1FVR0FiOUk4ZjdP?=
 =?utf-8?B?YlhESFFVNGNVTnE1OXVhVzl2MWo4UVFWMFlnL3k1bjVGRGtLemlBOW1sNEd4?=
 =?utf-8?B?NzFjUEg1MkYzSVVRVzZYS09sVnZueTZlOTN0TG13Nm9CeTloZ2RYYU5Xazg2?=
 =?utf-8?B?NHVHZGlDVHRBakJPaHk3TnpGdXhCOUZCMWh0QURVNTROVlJRQk5mYjl5UTlw?=
 =?utf-8?B?ZUUxdk9PTnZ6SWE3QW1kTVZQUnVVYUhtQ3hPTmJQay9pL0JTMStIV21BK3Y5?=
 =?utf-8?B?WjM4WlYyWlc4dFJkVzRTL2hGVFdGYmNFSGJIWmZQQlM4cXFOaDlZYUc4S21t?=
 =?utf-8?B?elFSQmVhV2JTaDNtTVNFNkVIbWJhN1NnKzlaRXBrM3pteVRMNFpudmF4OXNI?=
 =?utf-8?B?S0RXdDdWVStWWHN4R1MrUk1aMVNZV2I2cmVEemE5UDBsc0lWQUhEcUxUeVRp?=
 =?utf-8?B?U1RoVThOSVVTQktoa3FWVWR6cHNUK2NsVTNHd1FHWjFHYlo5eUszSVdNT1po?=
 =?utf-8?B?alp5VTJ4ZCsvcTRyNFNpaCt3TmN1c1ljNXBPTWZ4UFBjeW9NSnhSVHpCQXRM?=
 =?utf-8?B?UUFNY0FseEdjTEpiUGZIQ01GUzVXOGx6UjMzZC9TRlR0ZlVad3VXdTNHelBz?=
 =?utf-8?B?VUJiY3RjQ1BrcERuLzhUVlJEMXZHaHIyZjhraDlURkNXNXV4MzNQNU40VnM4?=
 =?utf-8?Q?jGlFkfmKgBbxZh9O1RQr3yljk?=
X-OriginatorOrg: oss.nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 38006719-a934-4697-2bf4-08db4532c348
X-MS-Exchange-CrossTenant-AuthSource: DU0PR04MB9417.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Apr 2023 02:14:13.6305
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: KTu1YrI/2rG+1hDvpxfwnfAru4t433u0xRcq3faJHXOmkX9uv2wFNlDxrehihzoiHgP/EFALpSqH2p1xaHakcg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR04MB8265
X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,
        SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 4/24/2023 6:33 PM, Oleksii Moisieiev wrote:
> 
> On 24.04.23 04:52, Peng Fan wrote:
>>
>>
>> On 4/21/2023 4:40 PM, Oleksii Moisieiev wrote:
>>> Hi Peng Fan,
>>>
>>> On 17.04.23 05:55, Peng Fan wrote:
>>>>
>>>>
>>>> On 4/13/2023 6:04 AM, Cristian Marussi wrote:
>>>>> On Fri, Apr 07, 2023 at 10:18:27AM +0000, Oleksii Moisieiev wrote:
>>>>>> Implementation of the SCMI client driver, which implements
>>>>>> PINCTRL_PROTOCOL. This protocol has ID 19 and is described
>>>>>> in the latest DEN0056 document.
>>>>>
>>>>> Hi,
>>>>>
>>>>>> This protocol is part of the feature that was designed to
>>>>>> separate the pinctrl subsystem from the SCP firmware.
>>>>>> The idea is to separate communication of the pin control
>>>>>> subsystem with the hardware to SCP firmware
>>>>>> (or a similar system, such as ATF), which provides an interface
>>>>>> to give the OS ability to control the hardware through SCMI protocol.
>>>>>> This is a generic driver that implements SCMI protocol,
>>>>>> independent of the platform type.
>>>>>>
>>>>>> DEN0056 document:
>>>>>> https://urldefense.com/v3/__https://developer.arm.com/documentation/den0056/latest__;!!GF_29dbcQIUBPA!y2hR3PEGGxiPjVeXBcgGyV03DPDhzgUKR0uHvsTpiafKgBar8Egc6oOOs-IkFIquhSf-qBzltqEMyzRZHq8eC4g$
>>>>>>
>>>>>> [developer[.]arm[.]com]
>>>>>>
>>>>>
>>>>> No need to specify all of this in the commit message, just a note that
>>>>> you are adding a new SCMIv3.2 Pincontrol protocol, highlighting
>>>>> anything
>>>>> that has been left out in this patch (if any) will be enough.
>>>>
>>>> Is it possible to extend the spec to support multilple uint32_t for PIN
>>>> CONFIG SET?
>>>>
>>>> With only one uint32_t could not satisfy i.MX requirement.
>>>>
>>>> Thanks,
>>>> Peng.
>>>>
>>> IIUC you are expecting to have an ability to set some kind of array of
>>> uint32_t config values to some specific ConfigType?
>>>
>>> I'm not sure if it's supported by pintctrl subsystem right now. I was
>>> unable to find an example in the existing device-tree pinctrl bindings.
>>> This makes me think that this kind of binding is OEM specific.
>>
>> Look at arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts
>>
>> Take lpuart for example:
>> MX93_PAD_UART1_RXD__LPUART1_RX                  0x31e
>>
>> The macro MX93_PAD_UART1_RXD__LPUART1_RX is an array.
>>
>> i.MX pinctrl driver use
>> mux reg, mux value, select reg, select value, pad reg, pad value
>>
>> pinctrl driver will be handled by SCMI firmware for new i.MX SoC.
>>
>> With current spec, only one uint32 is low speed. So if the spec
>> could extended to support more uint32 with pin config set,
>> we could use one SCMI call to support i.MX pinctrl.
>>
>> Thanks
>> Peng.
>>
> Hi Peng Fan,
> 
> Thank you for the example.
> 
> According to the comment for the function imx_pinctrl_parse_pin_mmio:
> 
> /*
>    * Each pin represented in fsl,pins consists of a number of u32 PIN_FUNC_ID
>    * and 1 u32 CONFIG, the total size is PIN_FUNC_ID + CONFIG for each pin.
>    * For generic_pinconf case, there's no extra u32 CONFIG.
>    *
>    * PIN_FUNC_ID format:
>    * Default:
>    *     <mux_reg conf_reg input_reg mux_mode input_val>
>    * SHARE_MUX_CONF_REG:
>    *     <mux_conf_reg input_reg mux_mode input_val>
>    * IMX_USE_SCU:
>    *      <pin_id mux_mode>
>    */
> 
> IIUC this says that in scu format it is expected to use <pin_id
> mux_mode>. This fit's protocol expectations.

The comments are misleading, it should be pin_id mux_mode config_setting
See arch/arm64/boot/dts/freescale/imx8qm-mek.dts

> 
> The idea was to make all platform specific implementation, such as
> <mux_reg conf_reg input_reg mux_mode input_val>
> 
> on the SCP Firmware side and hide it under Pin ID and Config Type so
> Client side can apply generic config in the device tree.
> 
> Can't you use scu format here? Or execute
> imx_pinconf_decode_generic_config on SCMI server side?

Technically with one config value is doable, but that means
SCMI needs expose many pin ids, such as pin id SDHC_CD,
it could be muxed as SDHC_CD, GPIO1_5, SAI_RX. Then
scmi needs export:
SDHC_CD_SDHC_CD
SDHC_CD_GPIO1_5
SDHC_CD_SAI_RX


If with multiple uint32_value support by spec,
scmi just needs exports SDHC_CD, with other values passed
through uint32 value.

Regards,
Peng.


> 
> Best regards,
> 
> Oleksii.
> 
>>>
>>> Maybe it can be implemented by adding new IDs to OEM specific range
>>> (192-255) which is reserved for OEM specific units (See Table 23 of
>>> DEN0056E).
>>>
>>> Best regards,
>>>
>>> Oleksii
>>>
>>>
>>>>> You can look at the very first commit logs of existing protos as an
>>>>> example like: drivers/firmware/arm_scmi/powercap.c
>>>>>
>>>>> Some more comments down below, I'll mostly skip anything related to
>>>>> the
>>>>> SCMI API change I mentioned before...
>>>>>
>>>>> I'll also wont comment on more trivial stuff related to style, BUT
>>>>> there
>>>>> are lots of them: you should run
>>>>>
>>>>> ./scripts/checkpatch.pl --strict <your-git-format-patch-file>
>>>>>
>>>>> for each patch in the series. (and fix accordingly..spacing,
>>>>> brackets...etc)
>>>>>
>>>>>> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
>>>>>> ---
>>>>>>     MAINTAINERS                         |   6 +
>>>>>>     drivers/firmware/arm_scmi/Makefile  |   2 +-
>>>>>>     drivers/firmware/arm_scmi/common.h  |   1 +
>>>>>>     drivers/firmware/arm_scmi/driver.c  |   3 +
>>>>>>     drivers/firmware/arm_scmi/pinctrl.c | 905
>>>>>> ++++++++++++++++++++++++++++
>>>>>>     drivers/pinctrl/Kconfig             |   9 +
>>>>>>     include/linux/scmi_protocol.h       |  58 +-
>>>>>>     7 files changed, 982 insertions(+), 2 deletions(-)
>>>>>>     create mode 100644 drivers/firmware/arm_scmi/pinctrl.c
>>>>>>
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>> index 281de213ef47..abc543fd7544 100644
>>>>>> --- a/MAINTAINERS
>>>>>> +++ b/MAINTAINERS
>>>>>> @@ -16961,6 +16961,12 @@ F:    drivers/reset/reset-scmi.c
>>>>>>     F:    include/linux/sc[mp]i_protocol.h
>>>>>>     F:    include/trace/events/scmi.h
>>>>>>     +PINCTRL DRIVER FOR SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE
>>>>>> (SCPI/SCMI)
>>>>>> +M:    Oleksii Moisieiev <oleksii_moisieiev@epam.com>
>>>>>> +L:    linux-arm-kernel@lists.infradead.org
>>>>>> +S:    Maintained
>>>>>> +F:    drivers/firmware/arm_scmi/pinctrl.c
>>>>>> +
>>>>>>     SYSTEM RESET/SHUTDOWN DRIVERS
>>>>>>     M:    Sebastian Reichel <sre@kernel.org>
>>>>>>     L:    linux-pm@vger.kernel.org
>>>>>> diff --git a/drivers/firmware/arm_scmi/Makefile
>>>>>> b/drivers/firmware/arm_scmi/Makefile
>>>>>> index bc0d54f8e861..5cec357fbfa8 100644
>>>>>> --- a/drivers/firmware/arm_scmi/Makefile
>>>>>> +++ b/drivers/firmware/arm_scmi/Makefile
>>>>>> @@ -4,7 +4,7 @@ scmi-driver-y = driver.o notify.o
>>>>>>     scmi-transport-y = shmem.o
>>>>>>     scmi-transport-$(CONFIG_MAILBOX) += mailbox.o
>>>>>>     scmi-transport-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smc.o
>>>>>> -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
>>>>>> system.o
>>>>>> +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
>>>>>> system.o pinctrl.o
>>>>>>     scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y)
>>>>>> $(scmi-protocols-y) \
>>>>>>                 $(scmi-transport-y)
>>>>>>     obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
>>>>>> diff --git a/drivers/firmware/arm_scmi/common.h
>>>>>> b/drivers/firmware/arm_scmi/common.h
>>>>>> index 65063fa948d4..8bbb404abe8d 100644
>>>>>> --- a/drivers/firmware/arm_scmi/common.h
>>>>>> +++ b/drivers/firmware/arm_scmi/common.h
>>>>>> @@ -170,6 +170,7 @@ DECLARE_SCMI_REGISTER_UNREGISTER(power);
>>>>>>     DECLARE_SCMI_REGISTER_UNREGISTER(reset);
>>>>>>     DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
>>>>>>     DECLARE_SCMI_REGISTER_UNREGISTER(system);
>>>>>> +DECLARE_SCMI_REGISTER_UNREGISTER(pinctrl);
>>>>>>       #define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(id, name) \
>>>>>>     int __init scmi_##name##_register(void) \
>>>>>> diff --git a/drivers/firmware/arm_scmi/driver.c
>>>>>> b/drivers/firmware/arm_scmi/driver.c
>>>>>> index 3dfd8b6a0ebf..fb9525fb3c24 100644
>>>>>> --- a/drivers/firmware/arm_scmi/driver.c
>>>>>> +++ b/drivers/firmware/arm_scmi/driver.c
>>>>>> @@ -743,6 +743,7 @@ static struct scmi_prot_devnames devnames[] = {
>>>>>>         { SCMI_PROTOCOL_CLOCK,  { "clocks" },},
>>>>>>         { SCMI_PROTOCOL_SENSOR, { "hwmon" },},
>>>>>>         { SCMI_PROTOCOL_RESET,  { "reset" },},
>>>>>> +    { SCMI_PROTOCOL_PINCTRL,  { "pinctrl" },},
>>>>>>     };
>>>>>>       static inline void
>>>>>> @@ -947,6 +948,7 @@ static int __init scmi_driver_init(void)
>>>>>>         scmi_reset_register();
>>>>>>         scmi_sensors_register();
>>>>>>         scmi_system_register();
>>>>>> +    scmi_pinctrl_register();
>>>>>>           return platform_driver_register(&scmi_driver);
>>>>>>     }
>>>>>> @@ -962,6 +964,7 @@ static void __exit scmi_driver_exit(void)
>>>>>>         scmi_reset_unregister();
>>>>>>         scmi_sensors_unregister();
>>>>>>         scmi_system_unregister();
>>>>>> +    scmi_pinctrl_unregister();
>>>>>>           platform_driver_unregister(&scmi_driver);
>>>>>>     }
>>>>>> diff --git a/drivers/firmware/arm_scmi/pinctrl.c
>>>>>> b/drivers/firmware/arm_scmi/pinctrl.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..037270d7f39b
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/firmware/arm_scmi/pinctrl.c
>>>>>> @@ -0,0 +1,905 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +/*
>>>>>> + * System Control and Management Interface (SCMI) Pinctrl Protocol
>>>>>> + *
>>>>>> + * Copyright (C) 2021 EPAM.
>>>>>
>>>>> nitpick: update (C) years
>>>>>
>>>>>> + */
>>>>>> +
>>>>>> +#define pr_fmt(fmt) "SCMI Notifications PINCTRL - " fmt
>>>>>> +
>>>>>
>>>>> This is not needed, no notifs in this proto.
>>>>>
>>>>>> +#include <linux/scmi_protocol.h>
>>>>>> +#include <linux/slab.h>
>>>>>> +
>>>>>> +#include "common.h"
>>>>>> +#include "notify.h"
>>>>>
>>>>> Notifs not needed, and in the new API world you'll just need a:
>>>>>
>>>>>     #include "protocols.h"
>>>>>
>>>>>> +
>>>>>> +#define SET_TYPE(x) ((x) & 0x3)
>>>>>> +
>>>>>
>>>>> Even if trivial better to use std bitfield.h macros like
>>>>> FIELD_GET() BIT() ... etc
>>>>>
>>>>>> +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
>>>>>> +};
>>>>>> +
>>>>>> +enum scmi_pinctrl_selector_type {
>>>>>> +    PIN_TYPE = 0,
>>>>>> +    GROUP_TYPE,
>>>>>> +    FUNCTION_TYPE
>>>>>> +};
>>>>>> +
>>>>>> +struct scmi_group_info {
>>>>>> +    bool present;
>>>>>> +    char *name;
>>>>>> +    unsigned int *group_pins;
>>>>>> +    unsigned int nr_pins;
>>>>>> +};
>>>>>> +
>>>>>> +struct scmi_function_info {
>>>>>> +    bool present;
>>>>>> +    char *name;
>>>>>> +    unsigned int *groups;
>>>>>> +    unsigned int nr_groups;
>>>>>> +};
>>>>>> +
>>>>>> +struct scmi_pin_info {
>>>>>> +    bool present;
>>>>>> +    char *name;
>>>>>> +};
>>>>>> +
>>>>>> +struct scmi_pinctrl_info {
>>>>>> +    u32 version;
>>>>>> +    u16 nr_groups;
>>>>>> +    u16 nr_functions;
>>>>>> +    u16 nr_pins;
>>>>>
>>>>> Since these vars are not related to stricly spaced message fields
>>>>> (even though
>>>>> derived from such messages) do not use sized types, you can just
>>>>> stick with
>>>>> unsigned int. (it is also better not to mix sized and unsized types
>>>>> in the same
>>>>> struct). This also could come handy if these will be exposed to the
>>>>> user
>>>>> in scmi_protocol.h in the future (more on this down below)
>>>>>
>>>>>> +    struct scmi_group_info *groups;
>>>>>> +    struct scmi_function_info *functions;
>>>>>> +    struct scmi_pin_info *pins;
>>>>>> +};
>>>>>> +
>>>>>> +struct scmi_conf_tx {
>>>>>> +    __le32 identifier;
>>>>>> +#define SET_TYPE_BITS(attr, x) (((attr) & 0xFFFFFCFF) | (x & 0x3)
>>>>>> << 8)
>>>>>> +#define SET_CONFIG(attr, x) (((attr) & 0xFF) | (x & 0xFF))
>>>>>
>>>>> Use bitfield.h like FIELD_SET / GENMASK etc
>>>>>
>>>>>> +    __le32 attributes;
>>>>>> +};
>>>>>> +
>>>>>> +static int scmi_pinctrl_attributes_get(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                       struct scmi_pinctrl_info *pi)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +    struct scmi_xfer *t;
>>>>>> +    struct scmi_msg_pinctrl_protocol_attributes {
>>>>>> +#define GROUPS_NR(x) ((x) >> 16)
>>>>>> +#define PINS_NR(x) ((x) & 0xffff)
>>>>>> +        __le32 attributes_low;
>>>>>> +#define FUNCTIONS_NR(x) ((x) & 0xffff)
>>>>>> +        __le32 attributes_high;
>>>>>> +    } *attr;
>>>>>
>>>>> For consistency with the rest of the stack (mostly :D), please move
>>>>> this struct
>>>>> definition and related macros outside in the global scope right after
>>>>> command
>>>>> enum. (and use bitfield macros ...)
>>>>>
>>>>>> +
>>>>>> +    if (!pi)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    ret = scmi_xfer_get_init(handle, PROTOCOL_ATTRIBUTES,
>>>>>> +                 SCMI_PROTOCOL_PINCTRL, 0, sizeof(*attr), &t);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    attr = t->rx.buf;
>>>>>> +
>>>>>> +    ret = scmi_do_xfer(handle, t);
>>>>>> +    if (!ret) {
>>>>>> +        pi->nr_functions =
>>>>>> + le16_to_cpu(FUNCTIONS_NR(attr->attributes_high));
>>>>>> +        pi->nr_groups =
>>>>>> le16_to_cpu(GROUPS_NR(attr->attributes_low));
>>>>>> +        pi->nr_pins = le16_to_cpu(PINS_NR(attr->attributes_low));
>>>>>> +    }
>>>>>> +
>>>>>> +    scmi_xfer_put(handle, t);
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_get_groups_count(const struct scmi_handle
>>>>>> *handle)
>>>>>> +{
>>>>>> +    struct scmi_pinctrl_info *pi;
>>>>>> +
>>>>>> +    if (!handle || !handle->pinctrl_priv)
>>>>>> +        return -ENODEV;
>>>>>> +
>>>>>> +    pi = handle->pinctrl_priv;
>>>>>> +
>>>>>> +    return pi->nr_groups;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_get_pins_count(const struct scmi_handle
>>>>>> *handle)
>>>>>> +{
>>>>>> +    struct scmi_pinctrl_info *pi;
>>>>>> +
>>>>>> +    if (!handle || !handle->pinctrl_priv)
>>>>>> +        return -ENODEV;
>>>>>> +
>>>>>> +    pi = handle->pinctrl_priv;
>>>>>> +
>>>>>> +    return pi->nr_pins;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_get_functions_count(const struct
>>>>>> scmi_handle *handle)
>>>>>> +{
>>>>>> +    struct scmi_pinctrl_info *pi;
>>>>>> +
>>>>>> +    if (!handle || !handle->pinctrl_priv)
>>>>>> +        return -ENODEV;
>>>>>> +
>>>>>> +    pi = handle->pinctrl_priv;
>>>>>> +
>>>>>> +    return pi->nr_functions;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_validate_id(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                    u32 identifier,
>>>>>> +                    enum scmi_pinctrl_selector_type type)
>>>>>> +{
>>>>>> +    struct scmi_pinctrl_info *pi;
>>>>>> +
>>>>>> +    if (!handle || !handle->pinctrl_priv)
>>>>>> +        return -ENODEV;
>>>>>> +
>>>>>> +    switch (type) {
>>>>>> +    case PIN_TYPE:
>>>>>> +        pi = handle->pinctrl_priv;
>>>>>> +
>>>>>> +        return (identifier < pi->nr_pins) ? 0 : -EINVAL;
>>>>>> +    case GROUP_TYPE:
>>>>>> +        return (identifier <
>>>>>> +            scmi_pinctrl_get_groups_count(handle)) ?
>>>>>> +            0 : -EINVAL;
>>>>>> +    case FUNCTION_TYPE:
>>>>>> +        return (identifier <
>>>>>> +            scmi_pinctrl_get_functions_count(handle)) ?
>>>>>> +            0 : -EINVAL;
>>>>>> +    default:
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>
>>>>> Here I would just pick the right value to compare, break and then
>>>>> compare and exit, something aroundf the lines of:
>>>>>
>>>>>       case PIN_TYPE:
>>>>>           ...
>>>>>               val = pi->nr_pins;
>>>>>           break;
>>>>>           ...
>>>>>       case GROUP_TYPE:
>>>>>           val = scmi_pinctrl_get_groups_count());
>>>>>           break;
>>>>>
>>>>>       ....
>>>>>       ....
>>>>>           default:
>>>>>           return -EINVAL;
>>>>>       }
>>>>>
>>>>>       if (identifier >= val)
>>>>>           return -EINVAL;
>>>>>
>>>>>       return 0;
>>>>> }
>>>>>
>>>>> ... it's easier to read. What do you think ?
>>>>>
>>>>>
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_get_name(const struct scmi_handle *handle,
>>>>>> +                 u32 identifier,
>>>>>> +                 enum scmi_pinctrl_selector_type type,
>>>>>> +                 char **name)
>>>>>> +{
>>>>>
>>>>> As said, there is common helper for this, but it will need some small
>>>>> adaptation in the SCMI core to work here so keep it as it is, and
>>>>> I'll take
>>>>> care of this later, if it sounds fine for you.
>>>>>
>>>>>> +    struct scmi_xfer *t;
>>>>>> +    int ret = 0;
>>>>>> +    struct scmi_name_tx {
>>>>>> +        __le32 identifier;
>>>>>> +        __le32 flags;
>>>>>> +    } *tx;
>>>>>> +    struct scmi_name_rx {
>>>>>> +        __le32 flags;
>>>>>> +        u8 name[64];
>>>>>> +    } *rx;
>>>>>> +
>>>>>> +    if (!handle || !name)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_validate_id(handle, identifier, type);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    ret = scmi_xfer_get_init(handle, PINCTRL_NAME_GET,
>>>>>> +                 SCMI_PROTOCOL_PINCTRL,
>>>>>> +                 sizeof(*tx), sizeof(*rx), &t);
>>>>>> +
>>>>>> +    tx = t->tx.buf;
>>>>>> +    rx = t->rx.buf;
>>>>>> +    tx->identifier = identifier;
>>>>>> +    tx->flags = SET_TYPE(cpu_to_le32(type));
>>>>>> +
>>>>>> +    ret = scmi_do_xfer(handle, t);
>>>>>> +    if (ret)
>>>>>> +        goto out;
>>>>>> +
>>>>>> +    if (rx->flags) {
>>>>>> +        ret = -EINVAL;
>>>>>> +        goto out;
>>>>>> +    }
>>>>>> +
>>>>>> +    *name = kasprintf(GFP_KERNEL, "%s", rx->name);
>>>>>> +    if (!*name)
>>>>>> +        ret = -ENOMEM;
>>>>>> + out:
>>>>>> +    scmi_xfer_put(handle, t);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_attributes(const struct scmi_handle *handle,
>>>>>> +                   enum scmi_pinctrl_selector_type type,
>>>>>> +                   u32 selector, char **name,
>>>>>> +                   unsigned int *n_elems)
>>>>>> +{
>>>>>> +    int ret = 0;
>>>>>> +    struct scmi_xfer *t;
>>>>>> +    struct scmi_pinctrl_attributes_tx {
>>>>>> +        __le32 identifier;
>>>>>> +        __le32 flags;
>>>>>> +    } *tx;
>>>>>> +    struct scmi_pinctrl_attributes_rx {
>>>>>> +#define EXT_NAME_FLAG(x) ((x) & BIT(31))
>>>>>> +#define NUM_ELEMS(x) ((x) & 0xffff)
>>>>>> +        __le32 attributes;
>>>>>> +        u8 name[16];
>>>>>> +    } *rx;
>>>>>
>>>>> Ditto. Move these defs outside, bitfield.h for macros and try to
>>>>> use the
>>>>> same naming style for message structs as in other protos, i.e.
>>>>>
>>>>>       for commands:     struct scmi_msg_pinctrl_attributes
>>>>>       for replies:     struct scmi_resp_pinctrl_attributes
>>>>>
>>>>> (or some variations around that...
>>>>>       scmi_msg_cmd_*  scmi_msg_resp_*
>>>>>
>>>>>      we have not been fully consistent really, so I dont want to be
>>>>>      pedantic here, but we never used tx/rx in message context since
>>>>> it is
>>>>>      already (mis)-used in SCMI channel context...)
>>>>>
>>>>>> +
>>>>>> +    if (!handle || !name)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_validate_id(handle, selector, type);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    ret = scmi_xfer_get_init(handle, PINCTRL_ATTRIBUTES,
>>>>>> +             SCMI_PROTOCOL_PINCTRL, sizeof(*tx), sizeof(*rx), &t);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    tx = t->tx.buf;
>>>>>> +    rx = t->rx.buf;
>>>>>> +    tx->identifier = selector;
>>>>>> +    tx->flags = SET_TYPE(cpu_to_le32(type));
>>>>>> +
>>>>>> +    ret = scmi_do_xfer(handle, t);
>>>>>> +    if (ret)
>>>>>> +        goto out;
>>>>>> +
>>>>>> +    *n_elems = NUM_ELEMS(rx->attributes);
>>>>>> +
>>>>>> +    if (!EXT_NAME_FLAG(rx->attributes)) {
>>>>>> +        *name = kasprintf(GFP_KERNEL, "%s", rx->name);
>>>>>> +        if (!*name)
>>>>>> +            ret = -ENOMEM;
>>>>>> +    } else
>>>>>> +        ret = scmi_pinctrl_get_name(handle, selector, type, name);
>>>>>> + out:
>>>>>> +    scmi_xfer_put(handle, t);
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_list_associations(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                      u32 selector,
>>>>>> +                      enum scmi_pinctrl_selector_type type,
>>>>>> +                      uint16_t size, unsigned int *array)
>>>>>> +{
>>>>>
>>>>> This is the other functionalities you could implement straight away
>>>>> using
>>>>> ph->hops helpers (iterators) but just leave it this way, and I'll
>>>>> port it later
>>>>> (once we retested all of this as working with the new API but without
>>>>> any
>>>>> ph->hops usage..I think it is safer to change one bit at time... :P)
>>>>>
>>>>>> +    struct scmi_xfer *t;
>>>>>> +    struct scmi_pinctrl_list_assoc_tx {
>>>>>> +        __le32 identifier;
>>>>>> +        __le32 flags;
>>>>>> +        __le32 index;
>>>>>> +    } *tx;
>>>>>> +    struct scmi_pinctrl_list_assoc_rx {
>>>>>> +#define RETURNED(x) ((x) & 0xFFF)
>>>>>> +#define REMAINING(x) ((x) >> 16)
>>>>>> +        __le32 flags;
>>>>>> +        __le16 array[];
>>>>>> +    } *rx;
>>>>>
>>>>> Ditto, about struct naming and macros.
>>>>>
>>>>>> +    u16 tot_num_ret = 0, loop_num_ret;
>>>>>> +    u16 remaining_num_ret;
>>>>>> +    int ret, loop;
>>>>>> +
>>>>>> +    if (!handle || !array || !size)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    if (type == PIN_TYPE)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_validate_id(handle, selector, type);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    ret = scmi_xfer_get_init(handle, PINCTRL_LIST_ASSOCIATIONS,
>>>>>> +                 SCMI_PROTOCOL_PINCTRL, sizeof(*tx),
>>>>>> +                 0, &t);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    tx = t->tx.buf;
>>>>>> +    rx = t->rx.buf;
>>>>>> +
>>>>>> +    do {
>>>>>> +        tx->identifier = cpu_to_le32(selector);
>>>>>> +        tx->flags = SET_TYPE(cpu_to_le32(type));
>>>>>> +        tx->index = cpu_to_le32(tot_num_ret);
>>>>>> +
>>>>>> +        ret = scmi_do_xfer(handle, t);
>>>>>> +        if (ret)
>>>>>> +            break;
>>>>>> +
>>>>>> +        loop_num_ret = le32_to_cpu(RETURNED(rx->flags));
>>>>>> +        remaining_num_ret = le32_to_cpu(REMAINING(rx->flags));
>>>>>> +
>>>>>> +        for (loop = 0; loop < loop_num_ret; loop++) {
>>>>>> +            if (tot_num_ret + loop >= size) {
>>>>>> +                ret = -EMSGSIZE;
>>>>>> +                goto out;
>>>>>> +            }
>>>>>> +
>>>>>> +            array[tot_num_ret + loop] =
>>>>>> +                le16_to_cpu(rx->array[loop]);
>>>>>> +        }
>>>>>> +
>>>>>> +        tot_num_ret += loop_num_ret;
>>>>>> +
>>>>>> +        scmi_reset_rx_to_maxsz(handle, t);
>>>>>> +    } while (remaining_num_ret > 0);
>>>>>> +out:
>>>>>> +    scmi_xfer_put(handle, t);
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_request_config(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                       u32 selector,
>>>>>> +                       enum scmi_pinctrl_selector_type type,
>>>>>> +                       u32 *config)
>>>>>> +{
>>>>>> +    struct scmi_xfer *t;
>>>>>> +    struct scmi_conf_tx *tx;
>>>>>> +    __le32 *packed_config;
>>>>>> +    u32 attributes = 0;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (!handle || !config || type == FUNCTION_TYPE)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_validate_id(handle, selector, type);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    ret = scmi_xfer_get_init(handle, PINCTRL_CONFIG_GET,
>>>>>> +                 SCMI_PROTOCOL_PINCTRL,
>>>>>> +                 sizeof(*tx), sizeof(*packed_config), &t);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    tx = t->tx.buf;
>>>>>> +    packed_config = t->rx.buf;
>>>>>> +    tx->identifier = cpu_to_le32(selector);
>>>>>> +    attributes = SET_TYPE_BITS(attributes, type);
>>>>>> +    attributes = SET_CONFIG(attributes, *config);
>>>>>> +
>>>>>> +    tx->attributes = cpu_to_le32(attributes);
>>>>>> +
>>>>>> +    ret = scmi_do_xfer(handle, t);
>>>>>> +
>>>>>> +    if (!ret)
>>>>>> +        *config = le32_to_cpu(*packed_config);
>>>>>> +
>>>>>> +    scmi_xfer_put(handle, t);
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_get_config(const struct scmi_handle
>>>>>> *handle, u32 pin,
>>>>>> +                   u32 *config)
>>>>>> +{
>>>>>> +    return scmi_pinctrl_request_config(handle, pin, PIN_TYPE,
>>>>>> config);
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_apply_config(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                     u32 selector,
>>>>>> +                     enum scmi_pinctrl_selector_type type,
>>>>>> +                     u32 config)
>>>>>> +{
>>>>>> +    struct scmi_xfer *t;
>>>>>> +    struct scmi_conf_tx *tx;
>>>>>> +    u32 attributes = 0;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (!handle || type == FUNCTION_TYPE)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_validate_id(handle, selector, type);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    ret = scmi_xfer_get_init(handle, PINCTRL_CONFIG_SET,
>>>>>> +                 SCMI_PROTOCOL_PINCTRL,
>>>>>> +                 sizeof(*tx), 0, &t);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    tx = t->tx.buf;
>>>>>> +    tx->identifier = cpu_to_le32(selector);
>>>>>> +    attributes = SET_TYPE_BITS(attributes, type);
>>>>>> +    attributes = SET_CONFIG(attributes, config);
>>>>>> +    tx->attributes = cpu_to_le32(attributes);
>>>>>> +
>>>>>> +    ret = scmi_do_xfer(handle, t);
>>>>>> +
>>>>>> +    scmi_xfer_put(handle, t);
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_set_config(const struct scmi_handle
>>>>>> *handle, u32 pin,
>>>>>> +                   u32 config)
>>>>>> +{
>>>>>> +    return scmi_pinctrl_apply_config(handle, pin, PIN_TYPE, config);
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_get_config_group(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                     u32 group, u32 *config)
>>>>>> +{
>>>>>> +    return scmi_pinctrl_request_config(handle, group, GROUP_TYPE,
>>>>>> config);
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_set_config_group(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                     u32 group, u32 config)
>>>>>> +{
>>>>>> +    return scmi_pinctrl_apply_config(handle, group, GROUP_TYPE,
>>>>>> config);
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_function_select(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                    u32 identifier,
>>>>>> +                    enum scmi_pinctrl_selector_type type,
>>>>>> +                    u32 function_id)
>>>>>> +{
>>>>>> +    struct scmi_xfer *t;
>>>>>> +    struct scmi_func_set_tx {
>>>>>> +        __le32 identifier;
>>>>>> +        __le32 function_id;
>>>>>> +        __le32 flags;
>>>>>> +    } *tx;
>>>>>
>>>>> Ditto.
>>>>>
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (!handle || type == FUNCTION_TYPE)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_validate_id(handle, identifier, type);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    ret = scmi_xfer_get_init(handle, PINCTRL_FUNCTION_SELECT,
>>>>>> +                 SCMI_PROTOCOL_PINCTRL,
>>>>>> +                 sizeof(*tx), 0, &t);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    tx = t->tx.buf;
>>>>>> +    tx->identifier = cpu_to_le32(identifier);
>>>>>> +    tx->function_id = cpu_to_le32(function_id);
>>>>>> +    tx->flags = SET_TYPE(cpu_to_le32(type));
>>>>>> +
>>>>>> +    ret = scmi_do_xfer(handle, t);
>>>>>> +    scmi_xfer_put(handle, t);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_request(const struct scmi_handle *handle,
>>>>>> +                u32 identifier,
>>>>>> +                enum scmi_pinctrl_selector_type type)
>>>>>> +{
>>>>>> +    struct scmi_xfer *t;
>>>>>> +    int ret;
>>>>>> +    struct scmi_request_tx {
>>>>>> +        __le32 identifier;
>>>>>> +        __le32 flags;
>>>>>> +    } *tx;
>>>>>> +
>>>>>
>>>>> Ditto.
>>>>>
>>>>>> +    if (!handle || type == FUNCTION_TYPE)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_validate_id(handle, identifier, type);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    ret = scmi_xfer_get_init(handle, PINCTRL_REQUEST,
>>>>>> SCMI_PROTOCOL_PINCTRL,
>>>>>> +                 sizeof(*tx), 0, &t);
>>>>>> +
>>>>>> +    tx = t->tx.buf;
>>>>>> +    tx->identifier = identifier;
>>>>>> +    tx->flags = SET_TYPE(cpu_to_le32(type));
>>>>>> +
>>>>>> +    ret = scmi_do_xfer(handle, t);
>>>>>> +    scmi_xfer_put(handle, t);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_request_pin(const struct scmi_handle
>>>>>> *handle, u32 pin)
>>>>>> +{
>>>>>> +    return scmi_pinctrl_request(handle, pin, PIN_TYPE);
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_free(const struct scmi_handle *handle, u32
>>>>>> identifier,
>>>>>> +                 enum scmi_pinctrl_selector_type type)
>>>>>> +{
>>>>>> +    struct scmi_xfer *t;
>>>>>> +    int ret;
>>>>>> +    struct scmi_request_tx {
>>>>>> +        __le32 identifier;
>>>>>> +        __le32 flags;
>>>>>> +    } *tx;
>>>>>> +
>>>>> Ditto.
>>>>>
>>>>>> +    if (!handle || type == FUNCTION_TYPE)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_validate_id(handle, identifier, type);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    ret = scmi_xfer_get_init(handle, PINCTRL_RELEASE,
>>>>>> SCMI_PROTOCOL_PINCTRL,
>>>>>> +                 sizeof(*tx), 0, &t);
>>>>>> +
>>>>>> +    tx = t->tx.buf;
>>>>>> +    tx->identifier = identifier;
>>>>>> +    tx->flags = SET_TYPE(cpu_to_le32(type));
>>>>>> +
>>>>>> +    ret = scmi_do_xfer(handle, t);
>>>>>> +    scmi_xfer_put(handle, t);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_free_pin(const struct scmi_handle *handle,
>>>>>> u32 pin)
>>>>>> +{
>>>>>> +    return scmi_pinctrl_free(handle, pin, PIN_TYPE);
>>>>>> +}
>>>>>> +
>>>>>> +
>>>>>> +static int scmi_pinctrl_get_group_info(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                       u32 selector,
>>>>>> +                       struct scmi_group_info *group)
>>>>>> +{
>>>>>> +    int ret = 0;
>>>>>> +    struct scmi_pinctrl_info *pi;
>>>>>> +
>>>>>> +    if (!handle || !handle->pinctrl_priv || !group)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    pi = handle->pinctrl_priv;
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_attributes(handle, GROUP_TYPE, selector,
>>>>>> +                      &group->name,
>>>>>> +                      &group->nr_pins);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    if (!group->nr_pins) {
>>>>>> +        dev_err(handle->dev, "Group %d has 0 elements", selector);
>>>>>> +        return -ENODATA;
>>>>>> +    }
>>>>>> +
>>>>>> +    group->group_pins = devm_kmalloc_array(handle->dev,
>>>>>> group->nr_pins,
>>>>>> +                           sizeof(*group->group_pins),
>>>>>> +                           GFP_KERNEL);
>>>>>
>>>>> I think you can just use for the array allocation
>>>>>
>>>>>       devm_kcalloc(dev, n, size, flags)
>>>>>
>>>>> and it will add also __GFP_ZERO internally to clear it.
>>>>> (indeed it calls in turn devm_kmalloc_array)
>>>>>
>>>>> ...BUT I think there is a further tricky issue here related to memory
>>>>> allocation...
>>>>>
>>>>> You call this and others function of this kind from some
>>>>> scmi_pinctrl_ops,
>>>>> like in scmi_pinctrl_get_group_pins
>>>>> (scmi_pinctrl_ops->get_group_pins),
>>>>> and then this is in turn called by the SCMI Pinctrl driver via
>>>>> pinctrl_ops->get_group_pins AND you set a present flag so that you
>>>>> issue a
>>>>> PINCTRL_LIST_ASSOCIATIONS and allocate here a new group_pins array
>>>>> just
>>>>> the first time: but these are never released anywhere, since, even
>>>>> though
>>>>> lazily dynamically allocated when asked for, these are static data
>>>>> that
>>>>> you pass to the caller/user of this protocol and so you cannot release
>>>>> them anytime soon, indeed.
>>>>>
>>>>> The core SCMI stack usually takes care to track and release all the
>>>>> devm_
>>>>> resources allocated by the protocol ONLY if they were allocated with
>>>>> devres
>>>>> while inside scmi_pinctrl_protocol_init() function.
>>>>> (see
>>>>> drivers/firmware/arm-scmi/driver.c:scmi_alloc_init_protocol_instance()
>>>>>     and scmi_protocol_release)
>>>>>
>>>>> BUT you do not allocate these arrays inside the protocol-init
>>>>> function,
>>>>> you allocate them the first time these ops are called at runtime.
>>>>>
>>>>> If you unbind/unload all the drivers using this protocol and then
>>>>> reload
>>>>> them, all the devm_ allocations in protocol_init will be freed and
>>>>> reallocated BUT these arrays will never be freed (they are boudn to
>>>>> handle->dev)
>>>>> and instead they will be reallocated multiple times (present flag
>>>>> will be cleared
>>>>> on unload), remained unused and freed finally only when the whole
>>>>> SCMI stack is
>>>>> unbind/unloaded.
>>>>>
>>>>> You use a present flag to avoid reissuing the same query and
>>>>> reallocating all the arrays each time a driver calls these
>>>>> protocol_ops one, but really all these data is available early on at
>>>>> protocol init time and they are not supposed to change at runtime,
>>>>> dont they ?
>>>>>
>>>>> Even in a virtualized environment, you boot an agent and the SCMI
>>>>> platform server provides to the agent the list of associations when
>>>>> queried but then this does not change until the next reboot right ?
>>>>> (indeed you do not query more than once...)
>>>>>
>>>>> The agent can only change the PIN status with CONFIG_SET or
>>>>> FUNCTION_SELECT or REQUEST the exclusive use of a pin/group, but it is
>>>>> not that the platform can change the pin/groups associaion for the
>>>>> same
>>>>> agent at run time, this are static data for the whole life of the
>>>>> agent.
>>>>>
>>>>> Am I right ?
>>>>>
>>>>> IOW I think there is some potential memory leak on unbind/bind and it
>>>>> would
>>>>> be better to query and allocate all of these resources at init time
>>>>> and keep
>>>>> them ready to be retrieved by subsequent operations, since the
>>>>> lifetime
>>>>> of these resources is pretty long and they are basically representing
>>>>> static data that does not change after the init/probe phases.
>>>>>
>>>>> Indeed, all the other protocols usually allocate all the needed
>>>>> resources and query all the available SCMI resources once for all
>>>>> during
>>>>> the protocol_init, storing all the retrieved info in some struct
>>>>> *_info
>>>>> exposed in scmi_protocol.h and then provide some related
>>>>> protocol_ops to
>>>>> get the number of resources and to retrieve specific domain info
>>>>> descriptors.
>>>>> (voltage.c is an example and more on this down below...)
>>>>>
>>>>> This way, any dynamic allocation is done during protocol_init, so
>>>>> it can be automatically freed by the SCMI core once there are no more
>>>>> users of that protocol, and all of this static info data is queried
>>>>> and retrieved once for all at protocol initialization time, avoiding
>>>>> unneeded message exchanges to retrieve always the same data.
>>>>> (which you avoid anyway with the present flag)
>>>>>
>>>>> If you have a good reason to instead perform this sort of lazy
>>>>> allocation/query performed only at the last minute when someone ask
>>>>> for
>>>>> that specific resource, you will  have to provide also a
>>>>> .instance_deinit
>>>>> function to clean anything you allocated out of the .instance_init
>>>>> routine; but this would seem strange to me since any resource that is
>>>>> discovered at init will be eventually immediately queried by a driver
>>>>> which uses this protocol...am I missing something ?
>>>>>
>>>>>> +    if (!group->group_pins) {
>>>>>> +        ret = -ENOMEM;
>>>>>> +        goto err;
>>>>>> +    }
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_list_associations(handle, selector,
>>>>>> GROUP_TYPE,
>>>>>> +                         group->nr_pins, group->group_pins);
>>>>>> +    if (ret)
>>>>>> +        goto err_groups;
>>>>>> +
>>>>>> +    group->present = true;
>>>>>> +    return 0;
>>>>>> +
>>>>>> + err_groups:
>>>>>> +    kfree(group->group_pins);
>>>>>> + err:
>>>>>> +    kfree(group->name);
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_get_group_name(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                       u32 selector, const char **name)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +    struct scmi_pinctrl_info *pi;
>>>>>> +
>>>>>> +    if (!handle || !handle->pinctrl_priv || !name)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    pi = handle->pinctrl_priv;
>>>>>> +
>>>>>> +    if (selector > pi->nr_groups)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    if (!pi->groups[selector].present) {
>>>>>> +        ret = scmi_pinctrl_get_group_info(handle, 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_handle
>>>>>> *handle,
>>>>>> +                       u32 selector, const unsigned int **pins,
>>>>>> +                       unsigned int *nr_pins)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +    struct scmi_pinctrl_info *pi;
>>>>>> +
>>>>>> +    if (!handle || !handle->pinctrl_priv || !pins || !nr_pins)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    pi = handle->pinctrl_priv;
>>>>>> +
>>>>>> +    if (selector > pi->nr_groups)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    if (!pi->groups[selector].present) {
>>>>>> +        ret = scmi_pinctrl_get_group_info(handle, selector,
>>>>>> +                          &pi->groups[selector]);
>>>>>> +        if (ret)
>>>>>> +            return ret;
>>>>>> +    }
>>>>>> +
>>>>>> +    *pins = pi->groups[selector].group_pins;
>>>>>> +    *nr_pins = pi->groups[selector].nr_pins;
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_get_function_info(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                      u32 selector,
>>>>>> +                      struct scmi_function_info *func)
>>>>>> +{
>>>>>> +    int ret = 0;
>>>>>> +    struct scmi_pinctrl_info *pi;
>>>>>> +
>>>>>> +    if (!handle || !handle->pinctrl_priv || !func)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    pi = handle->pinctrl_priv;
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_attributes(handle, FUNCTION_TYPE, selector,
>>>>>> +                      &func->name,
>>>>>> +                      &func->nr_groups);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    if (!func->nr_groups) {
>>>>>> +        dev_err(handle->dev, "Function %d has 0 elements",
>>>>>> selector);
>>>>>> +        return -ENODATA;
>>>>>> +    }
>>>>>> +
>>>>>> +    func->groups = devm_kmalloc_array(handle->dev, func->nr_groups,
>>>>>> +                      sizeof(*func->groups),
>>>>>> +                      GFP_KERNEL);
>>>>>> +    if (!func->groups) {
>>>>>> +        ret = -ENOMEM;
>>>>>> +        goto err;
>>>>>> +    }
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_list_associations(handle, selector,
>>>>>> FUNCTION_TYPE,
>>>>>> +                         func->nr_groups, func->groups);
>>>>>> +    if (ret)
>>>>>> +        goto err_funcs;
>>>>>> +
>>>>>> +    func->present = true;
>>>>>> +    return 0;
>>>>>> +
>>>>>> + err_funcs:
>>>>>> +    kfree(func->groups);
>>>>>> + err:
>>>>>> +    kfree(func->name);
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_get_function_name(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                      u32 selector, const char **name)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +    struct scmi_pinctrl_info *pi;
>>>>>> +
>>>>>> +    if (!handle || !handle->pinctrl_priv || !name)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    pi = handle->pinctrl_priv;
>>>>>> +
>>>>>> +    if (selector > pi->nr_functions)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    if (!pi->functions[selector].present) {
>>>>>> +        ret = scmi_pinctrl_get_function_info(handle, 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_handle *handle,
>>>>>> +                        u32 selector,
>>>>>> +                        unsigned int *nr_groups,
>>>>>> +                        const unsigned int **groups)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +    struct scmi_pinctrl_info *pi;
>>>>>> +
>>>>>> +    if (!handle || !handle->pinctrl_priv || !groups || !nr_groups)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    pi = handle->pinctrl_priv;
>>>>>> +
>>>>>> +    if (selector > pi->nr_functions)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    if (!pi->functions[selector].present) {
>>>>>> +        ret = scmi_pinctrl_get_function_info(handle, selector,
>>>>>> + &pi->functions[selector]);
>>>>>> +        if (ret)
>>>>>> +            return ret;
>>>>>> +    }
>>>>>> +
>>>>>> +    *groups = pi->functions[selector].groups;
>>>>>> +    *nr_groups = pi->functions[selector].nr_groups;
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_set_mux(const struct scmi_handle *handle,
>>>>>> u32 selector,
>>>>>> +                u32 group)
>>>>>> +{
>>>>>> +    return scmi_pinctrl_function_select(handle, group, GROUP_TYPE,
>>>>>> +                        selector);
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_get_pin_info(const struct scmi_handle
>>>>>> *handle,
>>>>>> +                     u32 selector, struct scmi_pin_info *pin)
>>>>>> +{
>>>>>> +    int ret = 0;
>>>>>> +    struct scmi_pinctrl_info *pi;
>>>>>> +    unsigned int n_elems;
>>>>>> +
>>>>>> +    if (!handle || !handle->pinctrl_priv || !pin)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    pi = handle->pinctrl_priv;
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_attributes(handle, PIN_TYPE, selector,
>>>>>> +                      &pin->name,
>>>>>> +                      &n_elems);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    if (n_elems != pi->nr_pins) {
>>>>>> +        dev_err(handle->dev, "Wrong pin count expected %d has %d",
>>>>>> +            pi->nr_pins, n_elems);
>>>>>> +        return -ENODATA;
>>>>>> +    }
>>>>>> +
>>>>>> +    if (*(pin->name) == 0) {
>>>>>> +        dev_err(handle->dev, "Pin name is empty");
>>>>>> +        goto err;
>>>>>> +    }
>>>>>> +
>>>>>> +    pin->present = true;
>>>>>> +    return 0;
>>>>>> +
>>>>>> + err:
>>>>>> +    kfree(pin->name);
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int scmi_pinctrl_get_pin_name(const struct scmi_handle
>>>>>> *handle, u32 selector,
>>>>>> +                     const char **name)
>>>>>> +{
>>>>>> +
>>>>>> +    int ret;
>>>>>> +    struct scmi_pinctrl_info *pi;
>>>>>> +
>>>>>> +    if (!handle || !handle->pinctrl_priv || !name)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    pi = handle->pinctrl_priv;
>>>>>> +
>>>>>> +    if (selector > pi->nr_pins)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    if (!pi->pins[selector].present) {
>>>>>> +        ret = scmi_pinctrl_get_pin_info(handle, selector,
>>>>>> +                        &pi->pins[selector]);
>>>>>> +        if (ret)
>>>>>> +            return ret;
>>>>>> +    }
>>>>>> +
>>>>>> +    *name = pi->pins[selector].name;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +
>>>>>> +static const struct scmi_pinctrl_ops pinctrl_ops = {
>>>>>> +    .get_groups_count = scmi_pinctrl_get_groups_count,
>>>>>> +    .get_group_name = scmi_pinctrl_get_group_name,
>>>>>> +    .get_group_pins = scmi_pinctrl_get_group_pins,
>>>>>> +    .get_functions_count = scmi_pinctrl_get_functions_count,
>>>>>> +    .get_function_name = scmi_pinctrl_get_function_name,
>>>>>> +    .get_function_groups = scmi_pinctrl_get_function_groups,
>>>>>> +    .set_mux = scmi_pinctrl_set_mux,
>>>>>> +    .get_pin_name = scmi_pinctrl_get_pin_name,
>>>>>> +    .get_pins_count = scmi_pinctrl_get_pins_count,
>>>>>> +    .get_config = scmi_pinctrl_get_config,
>>>>>> +    .set_config = scmi_pinctrl_set_config,
>>>>>> +    .get_config_group = scmi_pinctrl_get_config_group,
>>>>>> +    .set_config_group = scmi_pinctrl_set_config_group,
>>>>>> +    .request_pin = scmi_pinctrl_request_pin,
>>>>>> +    .free_pin = scmi_pinctrl_free_pin
>>>>>> +};
>>>>>> +
>>>>>> +static int scmi_pinctrl_protocol_init(struct scmi_handle *handle)
>>>>>> +{
>>>>>> +    u32 version;
>>>>>> +    struct scmi_pinctrl_info *pinfo;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (!handle)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    scmi_version_get(handle, SCMI_PROTOCOL_PINCTRL, &version);
>>>>>> +
>>>>>> +    dev_dbg(handle->dev, "Pinctrl Version %d.%d\n",
>>>>>> +        PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
>>>>>> +
>>>>>> +    pinfo = devm_kzalloc(handle->dev, sizeof(*pinfo), GFP_KERNEL);
>>>>>> +    if (!pinfo)
>>>>>> +        return -ENOMEM;
>>>>>> +
>>>>>> +    ret = scmi_pinctrl_attributes_get(handle, pinfo);
>>>>>> +    if (ret)
>>>>>> +        goto free;
>>>>>> +
>>>>>> +    pinfo->pins = devm_kmalloc_array(handle->dev, pinfo->nr_pins,
>>>>>> +                     sizeof(*pinfo->pins),
>>>>>> +                     GFP_KERNEL | __GFP_ZERO);
>>>>>
>>>>>     devm_kcalloc() zeroes on its own
>>>>>
>>>>>> +    if (!pinfo->pins) {
>>>>>> +        ret = -ENOMEM;
>>>>>> +        goto free;
>>>>>> +    }
>>>>>> +
>>>>>> +    pinfo->groups = devm_kmalloc_array(handle->dev,
>>>>>> pinfo->nr_groups,
>>>>>> +                       sizeof(*pinfo->groups),
>>>>>> +                       GFP_KERNEL | __GFP_ZERO);
>>>>>
>>>>> Ditto.
>>>>>> +    if (!pinfo->groups) {
>>>>>> +        ret = -ENOMEM;
>>>>>> +        goto free;
>>>>>> +    }
>>>>>> +
>>>>>> +    pinfo->functions = devm_kmalloc_array(handle->dev,
>>>>>> pinfo->nr_functions,
>>>>>> +                          sizeof(*pinfo->functions),
>>>>>> +                          GFP_KERNEL | __GFP_ZERO);
>>>>>> +    if (!pinfo->functions) {
>>>>>> +        ret = -ENOMEM;
>>>>>> +        goto free;
>>>>>> +    }
>>>>>> +
>>>>>> +    pinfo->version = version;
>>>>>> +    handle->pinctrl_ops = &pinctrl_ops;
>>>>>> +    handle->pinctrl_priv = pinfo;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +free:
>>>>>> +    if (pinfo) {
>>>>>> +        devm_kfree(handle->dev, pinfo->pins);
>>>>>> +        devm_kfree(handle->dev, pinfo->functions);
>>>>>> +        devm_kfree(handle->dev, pinfo->groups);
>>>>>> +    }
>>>>>
>>>>> These frees are really not needed...if this function return failure
>>>>> any
>>>>> devres allocated in it is freed by the SCMI core. (as said
>>>>> above...in a
>>>>> recent kernel with the new API of course)
>>>>>
>>>>>> +
>>>>>> +    devm_kfree(handle->dev, pinfo);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(SCMI_PROTOCOL_PINCTRL,
>>>>>> pinctrl)
>>>>>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>>>>>> index 815095326e2d..68add4d06e8c 100644
>>>>>> --- a/drivers/pinctrl/Kconfig
>>>>>> +++ b/drivers/pinctrl/Kconfig
>>>>>> @@ -431,4 +431,13 @@ config PINCTRL_EQUILIBRIUM
>>>>>>           pin functions, configure GPIO attributes for LGM SoC pins.
>>>>>> Pinmux and
>>>>>>           pinconf settings are retrieved from device tree.
>>>>>>     +config PINCTRL_SCMI
>>>>>> +    bool "Pinctrl driver controlled via SCMI interface"
>>>>>> +    depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
>>>>>> +    help
>>>>>> +      This driver provides support for pinctrl which is controlled
>>>>>> +      by firmware that implements the SCMI interface.
>>>>>> +      It uses SCMI Message Protocol to interact with the
>>>>>> +      firmware providing all the pinctrl controls.
>>>>>> +
>>>>>
>>>>> This does NOT belong to this patch, but to the next right ?
>>>>>
>>>>>>     endif
>>>>>> diff --git a/include/linux/scmi_protocol.h
>>>>>> b/include/linux/scmi_protocol.h
>>>>>> index 9cd312a1ff92..6a909ef3bf51 100644
>>>>>> --- a/include/linux/scmi_protocol.h
>>>>>> +++ b/include/linux/scmi_protocol.h
>>>>>> @@ -12,7 +12,8 @@
>>>>>>     #include <linux/notifier.h>
>>>>>>     #include <linux/types.h>
>>>>>>     -#define SCMI_MAX_STR_SIZE    16
>>>>>> +#define SCMI_MAX_STR_SIZE 16
>>>>>> +#define SCMI_MAX_STR_EXT_SIZE 64
>>>>>
>>>>> This is handled as part of how the extended names are handled with
>>>>> ph->hops
>>>>> in a common way, as I was saying, so please move this if you need
>>>>> it in
>>>>> the protocol code, then I'll port to the ph->hops interface and clean
>>>>> up.
>>>>>
>>>>>>     #define SCMI_MAX_NUM_RATES    16
>>>>>>       /**
>>>>>> @@ -252,6 +253,55 @@ struct scmi_notify_ops {
>>>>>>                          struct notifier_block *nb);
>>>>>>     };
>>>>>>     +/**
>>>>>> + * struct scmi_pinctrl_ops - represents the various operations
>>>>>> provided
>>>>>> + * by SCMI Pinctrl Protocol
>>>>>> + *
>>>>>> + * @get_groups_count: returns count of the registered groups
>>>>>> + * @get_group_name: returns group name by index
>>>>>> + * @get_group_pins: returns the set of pins, assigned to the
>>>>>> specified group
>>>>>> + * @get_functions_count: returns count of the registered fucntions
>>>>>> + * @get_function_name: returns function name by indes
>>>>>> + * @get_function_groups: returns the set of groups, assigned to the
>>>>>> specified
>>>>>> + *    function
>>>>>> + * @set_mux: set muxing function for groups of pins
>>>>>> + * @get_pins: returns the set of pins, registered in driver
>>>>>> + * @get_config: returns configuration parameter for pin
>>>>>> + * @set_config: sets the configuration parameter for pin
>>>>>> + * @get_config_group: returns the configuration parameter for a
>>>>>> group of pins
>>>>>> + * @set_config_group: sets the configuration parameter for a groups
>>>>>> of pins
>>>>>> + * @request_pin: aquire pin before selecting mux setting
>>>>>> + * @free_pin: frees pin, acquired by request_pin call
>>>>>> + */
>>>>>> +struct scmi_pinctrl_ops {
>>>>>> +    int (*get_groups_count)(const struct scmi_handle *handle);
>>>>>> +    int (*get_group_name)(const struct scmi_handle *handles, u32
>>>>>> selector,
>>>>>> +                  const char **name);
>>>>>> +    int (*get_group_pins)(const struct scmi_handle *handle, u32
>>>>>> selector,
>>>>>> +                  const unsigned int **pins, unsigned int *nr_pins);
>>>>>> +    int (*get_functions_count)(const struct scmi_handle *handle);
>>>>>> +    int (*get_function_name)(const struct scmi_handle *handle, u32
>>>>>> selector,
>>>>>> +                 const char **name);
>>>>>> +    int (*get_function_groups)(const struct scmi_handle *handle,
>>>>>> +                   u32 selector, unsigned int *nr_groups,
>>>>>> +                   const unsigned int **groups);
>>>>>> +    int (*set_mux)(const struct scmi_handle *handle, u32 selector,
>>>>>> +               u32 group);
>>>>>> +    int (*get_pin_name)(const struct scmi_handle *handle, u32
>>>>>> selector,
>>>>>> +                const char **name);
>>>>>> +    int (*get_pins_count)(const struct scmi_handle *handle);
>>>>>> +    int (*get_config)(const struct scmi_handle *handle, u32 pin,
>>>>>> +              u32 *config);
>>>>>> +    int (*set_config)(const struct scmi_handle *handle, u32 pin,
>>>>>> +              u32 config);
>>>>>> +    int (*get_config_group)(const struct scmi_handle *handle, u32
>>>>>> pin,
>>>>>> +              u32 *config);
>>>>>> +    int (*set_config_group)(const struct scmi_handle *handle, u32
>>>>>> pin,
>>>>>> +              u32 config);
>>>>>> +    int (*request_pin)(const struct scmi_handle *handle, u32 pin);
>>>>>> +    int (*free_pin)(const struct scmi_handle *handle, u32 pin);
>>>>>> +};
>>>>>> +
>>>>>
>>>>> As mentioned above, here you could drop a lot of this
>>>>> get_X_count/name/pins
>>>>> and instead expose a few of the internal proocol struct scmi__X_info
>>>>> and then
>>>>> provide just a mean to query how many resource are there and then get
>>>>> the info
>>>>> descriptor you want for the specific domain_id, i.e.:
>>>>>
>>>>>        int (*num_domains_get)(ph, type)
>>>>>        void *(*info_get)(ph, type, domain_id);
>>>>>
>>>>> Thanks,
>>>>> Cristian
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> linux-arm-kernel mailing list
>>>>> linux-arm-kernel@lists.infradead.org
>>>>> https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!GF_29dbcQIUBPA!y2hR3PEGGxiPjVeXBcgGyV03DPDhzgUKR0uHvsTpiafKgBar8Egc6oOOs-IkFIquhSf-qBzltqEMyzRZrZi7qlk$
>>>>>
>>>>> [lists[.]infradead[.]org]