| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f73f39e2-81dd-4204-a3be-c5e7f5e54c1b@epam.com>
Date: Fri, 21 Apr 2023 08:40:47 +0000
From: Oleksii Moisieiev <Oleksii_Moisieiev@...m.com>
To: Peng Fan <peng.fan@....nxp.com>,
Cristian Marussi <cristian.marussi@....com>
CC: "sudeep.holla@....com" <sudeep.holla@....com>,
Linus Walleij <linus.walleij@...aro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"michal.simek@....com" <michal.simek@....com>,
"vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
"souvik.chakravarty@....com" <souvik.chakravarty@....com>
Subject: Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
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.
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@...m.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@...m.com>
>>> +L: linux-arm-kernel@...ts.infradead.org
>>> +S: Maintained
>>> +F: drivers/firmware/arm_scmi/pinctrl.c
>>> +
>>> SYSTEM RESET/SHUTDOWN DRIVERS
>>> M: Sebastian Reichel <sre@...nel.org>
>>> L: linux-pm@...r.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@...ts.infradead.org
>> https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!GF_29dbcQIUBPA!y2hR3PEGGxiPjVeXBcgGyV03DPDhzgUKR0uHvsTpiafKgBar8Egc6oOOs-IkFIquhSf-qBzltqEMyzRZrZi7qlk$
>> [lists[.]infradead[.]org]
Powered by blists - more mailing lists