Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
From: Oleksii Moisieiev
Date: Tue Apr 25 2023 - 03:22:40 EST
On 25.04.23 05:13, Peng Fan wrote:
>
>
> 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.
>
>
Hi Peng,
I think SCMI protocol can be modified to accept array of uint32_t of
config_values on PINCTRL_CONFIG_SET
and return array for config_values on PINCTRL_CONFIG_GET. Let's see what
others think about that.
Thanks,
Oleksii.
>>
>> 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@xxxxxxxx>
>>>>>>> ---
>>>>>>> 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@xxxxxxxx>
>>>>>>> +L: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>>>>>>> +S: Maintained
>>>>>>> +F: drivers/firmware/arm_scmi/pinctrl.c
>>>>>>> +
>>>>>>> SYSTEM RESET/SHUTDOWN DRIVERS
>>>>>>> M: Sebastian Reichel <sre@xxxxxxxxxx>
>>>>>>> L: linux-pm@xxxxxxxxxxxxxxx
>>>>>>> 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@xxxxxxxxxxxxxxxxxxx
>>>>>> https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!GF_29dbcQIUBPA!y2hR3PEGGxiPjVeXBcgGyV03DPDhzgUKR0uHvsTpiafKgBar8Egc6oOOs-IkFIquhSf-qBzltqEMyzRZrZi7qlk$
>>>>>>
>>>>>>
>>>>>> [lists[.]infradead[.]org]