Hi Cristian, > Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol > protocol basic support > > On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@xxxxxxx> > > > > Add basic implementation of the SCMI v3.2 pincontrol protocol. > > > > Hi, > > a few more comments down below... > > > Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx> > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx> > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx> > > --- > > drivers/firmware/arm_scmi/Makefile | 1 + > > drivers/firmware/arm_scmi/driver.c | 2 + > > drivers/firmware/arm_scmi/pinctrl.c | 921 > ++++++++++++++++++++++++++++++++++ > > drivers/firmware/arm_scmi/protocols.h | 1 + > > include/linux/scmi_protocol.h | 75 +++ > > 5 files changed, 1000 insertions(+) > > > > diff --git a/drivers/firmware/arm_scmi/Makefile > > b/drivers/firmware/arm_scmi/Makefile > > index a7bc4796519c..8e3874ff1544 100644 > > --- a/drivers/firmware/arm_scmi/Makefile > > +++ b/drivers/firmware/arm_scmi/Makefile > > @@ -11,6 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += > msg.o > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o > > scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o > > system.o voltage.o powercap.o > > +scmi-protocols-y += pinctrl.o > > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) > > $(scmi-transport-y) > > > > obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o diff --git > > a/drivers/firmware/arm_scmi/driver.c > > b/drivers/firmware/arm_scmi/driver.c > > index 415e6f510057..ac2d4b19727c 100644 > > --- a/drivers/firmware/arm_scmi/driver.c > > +++ b/drivers/firmware/arm_scmi/driver.c > > @@ -3142,6 +3142,7 @@ static int __init scmi_driver_init(void) > > scmi_voltage_register(); > > scmi_system_register(); > > scmi_powercap_register(); > > + scmi_pinctrl_register(); > > > > return platform_driver_register(&scmi_driver); > > } > > @@ -3159,6 +3160,7 @@ static void __exit scmi_driver_exit(void) > > scmi_voltage_unregister(); > > scmi_system_unregister(); > > scmi_powercap_unregister(); > > + scmi_pinctrl_unregister(); > > > > scmi_transports_exit(); > > > > diff --git a/drivers/firmware/arm_scmi/pinctrl.c > > b/drivers/firmware/arm_scmi/pinctrl.c > > new file mode 100644 > > index 000000000000..87d9b89cab13 > > --- /dev/null > > +++ b/drivers/firmware/arm_scmi/pinctrl.c > > @@ -0,0 +1,921 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * System Control and Management Interface (SCMI) Pinctrl Protocol > > + * > > + * Copyright (C) 2024 EPAM > > + * Copyright 2024 NXP > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/scmi_protocol.h> > > +#include <linux/slab.h> > > + > > +#include "common.h" > > +#include "protocols.h" > > + > > +/* Updated only after ALL the mandatory features for that version are > merged */ > > +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x0 > > + > > AFAICS, the only missing things are PINCTRL_SET_PERMISSIONS (optional > command) I not see users as of now, could we add it until we need it? and the multiple-configs on SETTINGS_GET, but this latter is > something really that we have to ask for in the request AND we did not as of > now since we dont need it...so I would say to bump the version to 0x10000 ok. > just to avoid needless warning as soon as a server supporting Pinctrl is met. > > > +#define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16)) > > +#define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0)) > > +#define GET_FUNCTIONS_NR(x) le32_get_bits((x), GENMASK(15, 0)) > > + > > +#define EXT_NAME_FLAG(x) le32_get_bits((x), BIT(31)) > > +#define NUM_ELEMS(x) le32_get_bits((x), GENMASK(15, 0)) > > + > > +#define REMAINING(x) le32_get_bits((x), GENMASK(31, > 16)) > > +#define RETURNED(x) le32_get_bits((x), GENMASK(11, 0)) > > + > > +#define CONFIG_FLAG_MASK GENMASK(19, 18) > > +#define SELECTOR_MASK GENMASK(17, 16) > > +#define SKIP_CONFIGS_MASK GENMASK(15, 8) > > +#define CONFIG_TYPE_MASK GENMASK(7, 0) > > + > > +enum scmi_pinctrl_protocol_cmd { > > + PINCTRL_ATTRIBUTES = 0x3, > > + PINCTRL_LIST_ASSOCIATIONS = 0x4, > > + PINCTRL_SETTINGS_GET = 0x5, > > + PINCTRL_SETTINGS_CONFIGURE = 0x6, > > + PINCTRL_REQUEST = 0x7, > > + PINCTRL_RELEASE = 0x8, > > + PINCTRL_NAME_GET = 0x9, > > + PINCTRL_SET_PERMISSIONS = 0xa > > +}; > > + > > +struct scmi_msg_settings_conf { > > + __le32 identifier; > > + __le32 function_id; > > + __le32 attributes; > > + __le32 configs[]; > > +}; > > + > > +struct scmi_msg_settings_get { > > + __le32 identifier; > > + __le32 attributes; > > +}; > > + > > +struct scmi_resp_settings_get { > > + __le32 function_selected; > > + __le32 num_configs; > > + __le32 configs[]; > > +}; > > + > > +struct scmi_msg_pinctrl_protocol_attributes { > > + __le32 attributes_low; > > + __le32 attributes_high; > > +}; > > + > > +struct scmi_msg_pinctrl_attributes { > > + __le32 identifier; > > + __le32 flags; > > +}; > > + > > +struct scmi_resp_pinctrl_attributes { > > + __le32 attributes; > > + u8 name[SCMI_SHORT_NAME_MAX_SIZE]; > > +}; > > + > > +struct scmi_msg_pinctrl_list_assoc { > > + __le32 identifier; > > + __le32 flags; > > + __le32 index; > > +}; > > + > > +struct scmi_resp_pinctrl_list_assoc { > > + __le32 flags; > > + __le16 array[]; > > +}; > > + > > +struct scmi_msg_func_set { > > + __le32 identifier; > > + __le32 function_id; > > + __le32 flags; > > +}; > > + > > As said by Dan...drop this. > > > +struct scmi_msg_request { > > + __le32 identifier; > > + __le32 flags; > > +}; > > + > > +struct scmi_group_info { > > + char name[SCMI_MAX_STR_SIZE]; > > + bool present; > > + u32 *group_pins; > > + u32 nr_pins; > > +}; > > + > > +struct scmi_function_info { > > + char name[SCMI_MAX_STR_SIZE]; > > + bool present; > > + u32 *groups; > > + u32 nr_groups; > > +}; > > + > > +struct scmi_pin_info { > > + char name[SCMI_MAX_STR_SIZE]; > > + bool present; > > +}; > > + > > +struct scmi_pinctrl_info { > > + u32 version; > > + int nr_groups; > > + int nr_functions; > > + int nr_pins; > > + struct scmi_group_info *groups; > > + struct scmi_function_info *functions; > > + struct scmi_pin_info *pins; > > +}; > > + > > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle > *ph, > > + struct scmi_pinctrl_info *pi) { > > + int ret; > > + struct scmi_xfer *t; > > + struct scmi_msg_pinctrl_protocol_attributes *attr; > > + > > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, > sizeof(*attr), > > + &t); > > + if (ret) > > + return ret; > > + > > + attr = t->rx.buf; > > + > > + ret = ph->xops->do_xfer(ph, t); > > + if (!ret) { > > + pi->nr_functions = GET_FUNCTIONS_NR(attr- > >attributes_high); > > + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low); > > + pi->nr_pins = GET_PINS_NR(attr->attributes_low); > > I was thinking, does make sense to allow a nr_pins == 0 setup to probe > successfully ? Becasuse is legit for the platform to return zero groups or zero > functions BUT zero pins is just useless (spec does not say > anything) > > Maybe you could just put a dev_warn() here on if (nr_pins == 0) and bail out > with -EINVAL... ok, fix in v7. > > On the other side looking at the zero groups/function case, that is plausible > and handled properly by the driver since a 0-bytes devm_kcalloc will return > ZERO_SIZE_PTR (not NULL) and all the remaining references to pinfo->groups > and pinfo->functions are guarded by a check on selector >= nr_groups (or >= > nr_functions), and by scmi_pinctrl_validate_id() so the zero grouyps/fuctions > scenarios should be safely handled. > > > + } > > + > > + ph->xops->xfer_put(ph, t); > > + return ret; > > +} > > + > > +static int scmi_pinctrl_count_get(const struct scmi_protocol_handle *ph, > > + enum scmi_pinctrl_selector_type type) { > > + struct scmi_pinctrl_info *pi = ph->get_priv(ph); > > + > > + switch (type) { > > + case PIN_TYPE: > > + return pi->nr_pins; > > + case GROUP_TYPE: > > + return pi->nr_groups; > > + case FUNCTION_TYPE: > > + return pi->nr_functions; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph, > > + u32 identifier, > > + enum scmi_pinctrl_selector_type type) { > > + int value; > > + > > + value = scmi_pinctrl_count_get(ph, type); > > + if (value < 0) > > + return value; > > + > > + if (identifier >= value) > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > +static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph, > > + enum scmi_pinctrl_selector_type type, > > + u32 selector, char *name, > > + u32 *n_elems) > > +{ > > + int ret; > > + struct scmi_xfer *t; > > + struct scmi_msg_pinctrl_attributes *tx; > > + struct scmi_resp_pinctrl_attributes *rx; > > + u32 ext_name_flag; > > what about a bool > > > + > > + if (!name) > > + return -EINVAL; > > + > > + ret = scmi_pinctrl_validate_id(ph, selector, type); > > + if (ret) > > + return ret; > > + > > + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx), > > + sizeof(*rx), &t); > > + if (ret) > > + return ret; > > + > > + tx = t->tx.buf; > > + rx = t->rx.buf; > > + tx->identifier = cpu_to_le32(selector); > > + tx->flags = cpu_to_le32(type); > > + > > + ret = ph->xops->do_xfer(ph, t); > > + if (!ret) { > > + if (n_elems) > > + *n_elems = NUM_ELEMS(rx->attributes); > > + > > + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE); > > + > > + ext_name_flag = EXT_NAME_FLAG(rx->attributes); > > + } else > > + ext_name_flag = 0; > > and you dont need this else branch to set ext_name_flag to false, since down > below you will check ext_flag ONLY if !ret, so it will have surely been set if the > do_xfer did not fail. > > > + > > + ph->xops->xfer_put(ph, t); > > + > > + /* > > + * If supported overwrite short name with the extended one; > > + * on error just carry on and use already provided short name. > > + */ > > + if (!ret && ext_name_flag) > > + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, > selector, > > + (u32 *)&type, name, > > + SCMI_MAX_STR_SIZE); > > + return ret; > > +} > > + > > +struct scmi_pinctrl_ipriv { > > + u32 selector; > > + enum scmi_pinctrl_selector_type type; > > + u32 *array; > > +}; > > + > > +static void iter_pinctrl_assoc_prepare_message(void *message, > > + u32 desc_index, > > + const void *priv) > > +{ > > + struct scmi_msg_pinctrl_list_assoc *msg = message; > > + const struct scmi_pinctrl_ipriv *p = priv; > > + > > + msg->identifier = cpu_to_le32(p->selector); > > + msg->flags = cpu_to_le32(p->type); > > + /* Set the number of OPPs to be skipped/already read */ > > OPP ? .. maybe drop this comment that was cut/pasted from somewhere > else :D > > > + msg->index = cpu_to_le32(desc_index); } > > + > > +static int iter_pinctrl_assoc_update_state(struct scmi_iterator_state *st, > > + const void *response, void *priv) > { > > + const struct scmi_resp_pinctrl_list_assoc *r = response; > > + > > + st->num_returned = RETURNED(r->flags); > > + st->num_remaining = REMAINING(r->flags); > > + > > + return 0; > > +} > > + > > +static int > > +iter_pinctrl_assoc_process_response(const struct scmi_protocol_handle > *ph, > > + const void *response, > > + struct scmi_iterator_state *st, void *priv) > { > > + const struct scmi_resp_pinctrl_list_assoc *r = response; > > + struct scmi_pinctrl_ipriv *p = priv; > > + > > + p->array[st->desc_index + st->loop_idx] = > > + le16_to_cpu(r->array[st->loop_idx]); > > + > > + return 0; > > +} > > + > > +static int scmi_pinctrl_list_associations(const struct scmi_protocol_handle > *ph, > > + u32 selector, > > + enum scmi_pinctrl_selector_type > type, > > + u16 size, u32 *array) > > +{ > > + int ret; > > + void *iter; > > + struct scmi_iterator_ops ops = { > > + .prepare_message = iter_pinctrl_assoc_prepare_message, > > + .update_state = iter_pinctrl_assoc_update_state, > > + .process_response = iter_pinctrl_assoc_process_response, > > + }; > > + struct scmi_pinctrl_ipriv ipriv = { > > + .selector = selector, > > + .type = type, > > + .array = array, > > + }; > > + > > + if (!array || !size || type == PIN_TYPE) > > + return -EINVAL; > > + > > + ret = scmi_pinctrl_validate_id(ph, selector, type); > > + if (ret) > > + return ret; > > + > > + iter = ph->hops->iter_response_init(ph, &ops, size, > > + PINCTRL_LIST_ASSOCIATIONS, > > + sizeof(struct > scmi_msg_pinctrl_list_assoc), > > + &ipriv); > > + > > + if (IS_ERR(iter)) > > + return PTR_ERR(iter); > > + > > + return ph->hops->iter_response_run(iter); > > +} > > + > > +struct scmi_settings_get_ipriv { > > + u32 selector; > > + enum scmi_pinctrl_selector_type type; > > + u32 flag; > > + enum scmi_pinctrl_conf_type *config_types; > > + u32 *config_values; > > +}; > > + > > +static void > > +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index, > > + const void *priv) > > +{ > > + struct scmi_msg_settings_get *msg = message; > > + const struct scmi_settings_get_ipriv *p = priv; > > + u32 attributes; > > + > > + attributes = FIELD_PREP(CONFIG_FLAG_MASK, p->flag) | > > + FIELD_PREP(SELECTOR_MASK, p->type); > > + > > + if (p->flag == 1) > > A boolean like .get_all would be more clear..see down below why you dont > need a flag 0|1|2 > > > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index); > > + else if (!p->flag) > > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p- > >config_types[0]); > > + > > + msg->attributes = cpu_to_le32(attributes); > > + msg->identifier = cpu_to_le32(p->selector); } > > + > > +static int > > +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st, > > + const void *response, void *priv) { > > + const struct scmi_resp_settings_get *r = response; > > + struct scmi_settings_get_ipriv *p = priv; > > + > > + if (p->flag == 1) { > > Ditto... see below the explanation > > > + st->num_returned = le32_get_bits(r->num_configs, > GENMASK(7, 0)); > > + st->num_remaining = le32_get_bits(r->num_configs, > > + GENMASK(31, 24)); > > + } else { > > + st->num_returned = 1; > > + st->num_remaining = 0; > > + } > > + > > + return 0; > > +} > > + > > +static int > > +iter_pinctrl_settings_get_process_response(const struct > scmi_protocol_handle *ph, > > + const void *response, > > + struct scmi_iterator_state *st, > > + void *priv) > > +{ > > + const struct scmi_resp_settings_get *r = response; > > + struct scmi_settings_get_ipriv *p = priv; > > + > > + if (!p->flag) { > > + if (p->config_types[0] != > > + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0))) > > + return -EINVAL; > > + } else if (p->flag == 1) { > > + p->config_types[st->desc_index + st->loop_idx] = > > + le32_get_bits(r->configs[st->loop_idx * 2], > > + GENMASK(7, 0)); > > + } else if (p->flag == 2) { > > + return 0; > > + } > > Unneeded...see down below for explanation > > > + > > + p->config_values[st->desc_index + st->loop_idx] = > > + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]); > > + > > + return 0; > > +} > > + > > +static int > > +scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32 > selector, > > + enum scmi_pinctrl_selector_type type, > > + enum scmi_pinctrl_conf_type config_type, > > + u32 *config_value) > > +{ > > + int ret; > > + void *iter; > > + struct scmi_iterator_ops ops = { > > + .prepare_message = > iter_pinctrl_settings_get_prepare_message, > > + .update_state = iter_pinctrl_settings_get_update_state, > > + .process_response = > iter_pinctrl_settings_get_process_response, > > + }; > > + struct scmi_settings_get_ipriv ipriv = { > > + .selector = selector, > > + .type = type, > > + .flag = 0, > > + .config_types = &config_type, > > + .config_values = config_value, > > + }; > > + > > So this function is used to retrieve configs; as of now, just one, then it could > be extended to fetch all the configs, and for this it uses the iterators helpers, > BUT it is not and will not be used to just fetch the selected_function with > flag_2 (even though is always provided), since in that case you wont get back > a multi-part SCMI response and so there is no need to use iterators... > > IOW... no need here to handle flag_2 scenario and as a consequence I would > change the ipriv flag to be be a boolean .get_all, like it was, since it is more > readable (and so you wont need to add any comment..) ok, so your suggestion is drop the iterators, and only support one config, right? Or keep iterators with get_all be passed as a function parameter? > > In the future could make sense to add here also a *selected_function output > param to this function since you will always get it back for free when > retrieving configs ... BUT for now is just not needed really...no users for this > case till now... > > ...when the time will come that we will need a function_selected_get to be > issued without retrieveing also the configs I would add a distinct routine that > crafts properly a SETTINGS_GET with flag_2 without worrying about multi- > part responses (and with no need for iterators support) > > Trying to handle all in here just complicates stuff... > > > + if (!config_value || type == FUNCTION_TYPE) > > + return -EINVAL; > > + > > + ret = scmi_pinctrl_validate_id(ph, selector, type); > > + if (ret) > > + return ret; > > + > > + iter = ph->hops->iter_response_init(ph, &ops, 1, > PINCTRL_SETTINGS_GET, > > + sizeof(struct > scmi_msg_settings_get), > > + &ipriv); > > + > > + if (IS_ERR(iter)) > > + return PTR_ERR(iter); > > + > > + return ph->hops->iter_response_run(iter); > > +} > > + > > +static int > > +scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph, > > + u32 selector, > > + enum scmi_pinctrl_selector_type type, > > + u32 nr_configs, > > + enum scmi_pinctrl_conf_type *config_type, > > + u32 *config_value) > > +{ > > + struct scmi_xfer *t; > > + struct scmi_msg_settings_conf *tx; > > + u32 attributes; > > + int ret, i; > > + u32 configs_in_chunk, conf_num = 0; > > + u32 chunk; > > + int max_msg_size = ph->hops->get_max_msg_size(ph); > > + > > + if (!config_type || !config_value || type == FUNCTION_TYPE) > > + return -EINVAL; > > + > > + ret = scmi_pinctrl_validate_id(ph, selector, type); > > + if (ret) > > + return ret; > > + > > + configs_in_chunk = (max_msg_size - sizeof(*tx)) / (sizeof(__le32) * 2); > > + while (conf_num < nr_configs) { > > + chunk = (nr_configs - conf_num > configs_in_chunk) ? > > + configs_in_chunk : nr_configs - conf_num; > > + > > + ret = ph->xops->xfer_get_init(ph, > PINCTRL_SETTINGS_CONFIGURE, > > + sizeof(*tx) + > > + chunk * 2 * sizeof(__le32), > > + 0, &t); > > + if (ret) > > + return ret; > for consistency I would > break; > > like below and you will exit always from the last return ret; > > > + > > + tx = t->tx.buf; > > + tx->identifier = cpu_to_le32(selector); > > + attributes = FIELD_PREP(GENMASK(1, 0), type) | > > + FIELD_PREP(GENMASK(9, 2), chunk); > > + tx->attributes = cpu_to_le32(attributes); > > + > > + for (i = 0; i < chunk; i++) { > > + tx->configs[i * 2] = > > + cpu_to_le32(config_type[conf_num + i]); > > + tx->configs[i * 2 + 1] = > > + cpu_to_le32(config_value[conf_num + i]); > > + } > > + > > + ret = ph->xops->do_xfer(ph, t); > > + > > + ph->xops->xfer_put(ph, t); > > + > > + if (ret) > > + break; > > + > > + conf_num += chunk; > > + } > > + > > + return ret; > > +} > > + > > +static int scmi_pinctrl_function_select(const struct scmi_protocol_handle > *ph, > > + u32 group, > > + enum scmi_pinctrl_selector_type > type, > > + u32 function_id) > > +{ > > + int ret; > > + struct scmi_xfer *t; > > + struct scmi_msg_settings_conf *tx; > > + u32 attributes; > > + > > + ret = scmi_pinctrl_validate_id(ph, group, type); > > + if (ret) > > + return ret; > > + > > + ret = ph->xops->xfer_get_init(ph, PINCTRL_SETTINGS_CONFIGURE, > > + sizeof(*tx), 0, &t); > > + if (ret) > > + return ret; > > + > > + tx = t->tx.buf; > > + tx->identifier = cpu_to_le32(group); > > + tx->function_id = cpu_to_le32(function_id); > > + attributes = FIELD_PREP(GENMASK(1, 0), type) | BIT(10); > > + tx->attributes = cpu_to_le32(attributes); > > + > > + ret = ph->xops->do_xfer(ph, t); > > + ph->xops->xfer_put(ph, t); > > + > > + return ret; > > +} > > + > > +static int scmi_pinctrl_request(const struct scmi_protocol_handle *ph, > > + u32 identifier, > > + enum scmi_pinctrl_selector_type type) { > > + int ret; > > + struct scmi_xfer *t; > > + struct scmi_msg_request *tx; > > + > > + if (type == FUNCTION_TYPE) > > + return -EINVAL; > > + > > + ret = scmi_pinctrl_validate_id(ph, identifier, type); > > + if (ret) > > + return ret; > > + > > + ret = ph->xops->xfer_get_init(ph, PINCTRL_REQUEST, sizeof(*tx), 0, > &t); > > + if (ret) > > + return ret; > > + > > + tx = t->tx.buf; > > + tx->identifier = cpu_to_le32(identifier); > > + tx->flags = cpu_to_le32(type); > > + > > + ret = ph->xops->do_xfer(ph, t); > > + ph->xops->xfer_put(ph, t); > > + > > + return ret; > > +} > > + > > ..this function ... > > > +static int scmi_pinctrl_pin_request(const struct scmi_protocol_handle *ph, > > + u32 pin) > > +{ > > + return scmi_pinctrl_request(ph, pin, PIN_TYPE); } > > + > > +static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph, > > + u32 identifier, > > + enum scmi_pinctrl_selector_type type) { > > + int ret; > > + struct scmi_xfer *t; > > + struct scmi_msg_request *tx; > > + > > + if (type == FUNCTION_TYPE) > > + return -EINVAL; > > + > > + ret = scmi_pinctrl_validate_id(ph, identifier, type); > > + if (ret) > > + return ret; > > + > > + ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE, sizeof(*tx), 0, &t); > > + if (ret) > > + return ret; > > + > > + tx = t->tx.buf; > > + tx->identifier = cpu_to_le32(identifier); > > + tx->flags = cpu_to_le32(type); > > + > > + ret = ph->xops->do_xfer(ph, t); > > + ph->xops->xfer_put(ph, t); > > + > > + return ret; > > +} > > + > > ...and this are completely identical, beside the used command msg_id...please > make it a common workhorse function by adding a param for the command... > > > +static int scmi_pinctrl_pin_free(const struct scmi_protocol_handle > > +*ph, u32 pin) { > > + return scmi_pinctrl_free(ph, pin, PIN_TYPE); } > > + > > ...and convert these _request/_free functions into a pair odf simple wrapper > invoking the common workhorse... > > > +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle > *ph, > > + u32 selector, > > + struct scmi_group_info *group) { > > + int ret; > > + > > + if (!group) > > + return -EINVAL; > > + > > + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector, > > + group->name, > > + &group->nr_pins); > > + if (ret) > > + return ret; > > + > > + if (!group->nr_pins) { > > + dev_err(ph->dev, "Group %d has 0 elements", selector); > > + return -ENODATA; > > + } > > + > > + group->group_pins = kmalloc_array(group->nr_pins, > > + sizeof(*group->group_pins), > > + GFP_KERNEL); > > + if (!group->group_pins) > > + return -ENOMEM; > > + > > + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE, > > + group->nr_pins, group- > >group_pins); > > + if (ret) { > > + kfree(group->group_pins); > > + return ret; > > + } > > + > > + group->present = true; > > + return 0; > > +} > > + > > +static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle > *ph, > > + u32 selector, const char **name) { > > + struct scmi_pinctrl_info *pi = ph->get_priv(ph); > > + > > + if (!name) > > + return -EINVAL; > > + > > + if (selector >= pi->nr_groups) > > + return -EINVAL; > > + > > + if (!pi->groups[selector].present) { > > + int ret; > > + > > + ret = scmi_pinctrl_get_group_info(ph, selector, > > + &pi->groups[selector]); > > + if (ret) > > + return ret; > > + } > > + > > + *name = pi->groups[selector].name; > > + > > + return 0; > > +} > > + > > +static int scmi_pinctrl_group_pins_get(const struct scmi_protocol_handle > *ph, > > + u32 selector, const u32 **pins, > > + u32 *nr_pins) > > +{ > > + struct scmi_pinctrl_info *pi = ph->get_priv(ph); > > + > > + if (!pins || !nr_pins) > > + return -EINVAL; > > + > > + if (selector >= pi->nr_groups) > > + return -EINVAL; > > + > > + if (!pi->groups[selector].present) { > > + int ret; > > + > > + ret = scmi_pinctrl_get_group_info(ph, selector, > > + &pi->groups[selector]); > > + if (ret) > > + return ret; > > + } > > + > > + *pins = pi->groups[selector].group_pins; > > + *nr_pins = pi->groups[selector].nr_pins; > > + > > + return 0; > > +} > > + > > +static int scmi_pinctrl_get_function_info(const struct > scmi_protocol_handle *ph, > > + u32 selector, > > + struct scmi_function_info *func) { > > + int ret; > > + > > + if (!func) > > + return -EINVAL; > > + > > + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector, > > + func->name, > > + &func->nr_groups); > > + if (ret) > > + return ret; > > + > > + if (!func->nr_groups) { > > + dev_err(ph->dev, "Function %d has 0 elements", selector); > > + return -ENODATA; > > + } > > + > > + func->groups = kmalloc_array(func->nr_groups, sizeof(*func->groups), > > + GFP_KERNEL); > > + if (!func->groups) > > + return -ENOMEM; > > + > > + ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE, > > + func->nr_groups, func->groups); > > + if (ret) { > > + kfree(func->groups); > > + return ret; > > + } > > + > > + func->present = true; > > + return 0; > > +} > > + > > +static int scmi_pinctrl_get_function_name(const struct > scmi_protocol_handle *ph, > > + u32 selector, const char **name) { > > + struct scmi_pinctrl_info *pi = ph->get_priv(ph); > > + > > + if (!name) > > + return -EINVAL; > > + > > + if (selector >= pi->nr_functions) > > + return -EINVAL; > > + > > + if (!pi->functions[selector].present) { > > + int ret; > > + > > + ret = scmi_pinctrl_get_function_info(ph, selector, > > + &pi- > >functions[selector]); > > + if (ret) > > + return ret; > > + } > > + > > + *name = pi->functions[selector].name; > > + return 0; > > +} > > + > > +static int > > +scmi_pinctrl_function_groups_get(const struct scmi_protocol_handle *ph, > > + u32 selector, u32 *nr_groups, > > + const u32 **groups) > > +{ > > + struct scmi_pinctrl_info *pi = ph->get_priv(ph); > > + > > + if (!groups || !nr_groups) > > + return -EINVAL; > > + > > + if (selector >= pi->nr_functions) > > + return -EINVAL; > > + > > + if (!pi->functions[selector].present) { > > + int ret; > > + > > + ret = scmi_pinctrl_get_function_info(ph, selector, > > + &pi- > >functions[selector]); > > + if (ret) > > + return ret; > > + } > > + > > + *groups = pi->functions[selector].groups; > > + *nr_groups = pi->functions[selector].nr_groups; > > + > > + return 0; > > +} > > + > > +static int scmi_pinctrl_mux_set(const struct scmi_protocol_handle *ph, > > + u32 selector, u32 group) > > +{ > > + return scmi_pinctrl_function_select(ph, group, GROUP_TYPE, > > +selector); } > > + > > +static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph, > > + u32 selector, struct scmi_pin_info *pin) { > > + int ret; > > + > > + if (!pin) > > + return -EINVAL; > > + > > + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector, > > + pin->name, NULL); > > + if (ret) > > + return ret; > > + > > + pin->present = true; > > + return 0; > > +} > > + > > +static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle > *ph, > > + u32 selector, const char **name) { > > + struct scmi_pinctrl_info *pi = ph->get_priv(ph); > > + > > + if (!name) > > + return -EINVAL; > > + > > + if (selector >= pi->nr_pins) > > + return -EINVAL; > > + > > + if (!pi->pins[selector].present) { > > + int ret; > > + > > + ret = scmi_pinctrl_get_pin_info(ph, selector, > > + &pi->pins[selector]); > > + if (ret) > > + return ret; > > + } > > + > > + *name = pi->pins[selector].name; > > + > > + return 0; > > +} > > + > > +static int scmi_pinctrl_name_get(const struct scmi_protocol_handle *ph, > > + u32 selector, > > + enum scmi_pinctrl_selector_type type, > > + const char **name) > > +{ > > + switch (type) { > > + case PIN_TYPE: > > + return scmi_pinctrl_get_pin_name(ph, selector, name); > > + case GROUP_TYPE: > > + return scmi_pinctrl_get_group_name(ph, selector, name); > > + case FUNCTION_TYPE: > > + return scmi_pinctrl_get_function_name(ph, selector, name); > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = { > > + .count_get = scmi_pinctrl_count_get, > > + .name_get = scmi_pinctrl_name_get, > > + .group_pins_get = scmi_pinctrl_group_pins_get, > > + .function_groups_get = scmi_pinctrl_function_groups_get, > > + .mux_set = scmi_pinctrl_mux_set, > > + .settings_get = scmi_pinctrl_settings_get, > > + .settings_conf = scmi_pinctrl_settings_conf, > > + .pin_request = scmi_pinctrl_pin_request, > > + .pin_free = scmi_pinctrl_pin_free, > > +}; > > + > > +static int scmi_pinctrl_protocol_init(const struct > > +scmi_protocol_handle *ph) { > > + int ret; > > + u32 version; > > + struct scmi_pinctrl_info *pinfo; > > + > > + ret = ph->xops->version_get(ph, &version); > > + if (ret) > > + return ret; > > + > > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n", > > + PROTOCOL_REV_MAJOR(version), > PROTOCOL_REV_MINOR(version)); > > + > > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL); > > + if (!pinfo) > > + return -ENOMEM; > > + > > + ret = scmi_pinctrl_attributes_get(ph, pinfo); > > + if (ret) > > + return ret; > > ..as a I was saying is nr_pins == 0 the scmi_pinctrl_attributes_get could return > -EINVAL here and bail out....not sure that a running setup with zero pins has > any values (even for testing...) BUT, as said above, I wuld certainly add a > dev_warn in scmi_pinctrl_attributes_get() when nr_pins == 0 Fix it in v7. Thanks, Peng. > > Thanks, > Cristian