| Message ID | 20240323-pinctrl-scmi-v6-0-a895243257c0@nxp.com |
|---|---|
| Headers | show |
| Series | firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support | expand |
Looks really nice. Just a few small comments below. On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote: > + > +struct scmi_msg_func_set { > + __le32 identifier; > + __le32 function_id; > + __le32 flags; > +}; This scmi_msg_func_set struct is unused. Delete. > +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) > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index); > + else if (!p->flag) This is a nit-pick but could you change these !p->flag conditions to p->flag == 0? It's a number zero, not a bool. > + 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) { > + 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->flag == 0) { > + 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; > + } > + > + 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, ->flag should be 0-2. > + .config_types = &config_type, > + .config_values = config_value, > + }; > + > + if (!config_value || type == FUNCTION_TYPE) ^^^^^^^^^^^^ config_value should be optional for flag == 2. regards, dan carpenter > + 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); > +} > +
On Wed, Mar 27, 2024 at 01:46:11PM +0300, Dan Carpenter wrote: > Looks really nice. Just a few small comments below. > Hi, I aa having a look at this today (and try to retest it), just a quick remark down below... > On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote: > > + > > +struct scmi_msg_func_set { > > + __le32 identifier; > > + __le32 function_id; > > + __le32 flags; > > +}; > > This scmi_msg_func_set struct is unused. Delete. > > > +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) > > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index); > > + else if (!p->flag) > > This is a nit-pick but could you change these !p->flag conditions to > p->flag == 0? It's a number zero, not a bool. > > > + 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) { > > + 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->flag == 0) { > > > + 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; > > + } > > + > > + 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, > > ->flag should be 0-2. > The .flag in this priv structure is for 'configuring' the iterators in the SCMI core to parse a multi-part response, so (looking at how the iterators functs are implemented above) setting it here to zero means issuing a SETTINGS_GET with attributes.config_flag[19:18] == 0, that in turn means requesting just a single config_value to be read... ...if you want to support the other 'flavours' of SETTINGS_GET (multiple configs & selected_func) you will have to extend the signature of this function to optionally select to readback multiple configs (and to allow the return of such multiple config_values) and/or optionally request to return the selected function.... ...or maybe add distinct wrapper protocol_ops just for these flavours... Anyway, till now in this series it was avoided to add such 'flavours' support (e.g. for multiple configs) since there are no users for the multi-config and function selected in the pinctrl driver...so no way to test.. Will the Linux GPIO driver need these ? Should we delay anyway the addition of the support of such variants of SETTING_GET for when a real user like GPIO driveer appears ? ...anyway @Peng please add a comment somewhere explaining how p->flag is used to configure the type of SETTINGS_GET Thanks, Cristian
On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > Add basic implementation of the SCMI v3.2 pincontrol protocol. > Hi, a few more comments down below... > Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > 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) 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 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... 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..) 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 Thanks, Cristian
On Sat, Mar 23, 2024 at 08:15:13PM +0800, Peng Fan (OSS) wrote: > This patchset is a rework from Oleksii's RFC v5 patchset > https://lore.kernel.org/all/cover.1698353854.git.oleksii_moisieiev@epam.com/ > Hi, beside a few more remarks on the series, I tested this again on my setup with the latest v3.2 changes and it works fine. Thanks, Cristian
Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti: > From: Peng Fan <peng.fan@nxp.com> > > Add basic implementation of the SCMI v3.2 pincontrol protocol. ... > 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 Actually you want to have := here. > +scmi-protocols-y += pinctrl.o > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y) Side note: The -objs has to be -y ... > +#include <linux/module.h> > +#include <linux/scmi_protocol.h> > +#include <linux/slab.h> This is semi-random list of headers. Please, follow IWYU principle (include what you use). There are a lot of inclusions I see missing (just in the context of this page I see bits.h, types.h, and asm/byteorder.h). ... > +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 Leave trailing comma as it's not a termination. > +}; ... > +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); This looks much better on a single line. > + if (ret) > + return ret; > + > + attr = t->rx.buf; > + > + ret = ph->xops->do_xfer(ph, t); > + if (!ret) { > + pi->nr_functions = GET_FUNCTIONS_NR(attr->attributes_high); > + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low); > + pi->nr_pins = GET_PINS_NR(attr->attributes_low); > + } > + > + ph->xops->xfer_put(ph, t); > + return ret; > +} ... > + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx), > + sizeof(*rx), &t); Possible to have on a single line (if you use relaxed 100 limit). Or (re)split it more logically: ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx), sizeof(*rx), &t); > + if (ret) > + return ret; ... > + /* > + * 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) Please, use standard pattern, i.e. if (ret) return ret; > + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector, > + (u32 *)&type, name, Why is an explicit casting needed? > + SCMI_MAX_STR_SIZE); > + return ret; ... > + iter = ph->hops->iter_response_init(ph, &ops, size, > + PINCTRL_LIST_ASSOCIATIONS, > + sizeof(struct scmi_msg_pinctrl_list_assoc), > + &ipriv); > + Redundant blank line. > + if (IS_ERR(iter)) > + return PTR_ERR(iter); ... > + if (p->flag == 1) > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index); > + else if (!p->flag) Be consistent, i.e. if (p->flag == 0) > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p->config_types[0]); ... > + st->num_remaining = le32_get_bits(r->num_configs, > + GENMASK(31, 24)); One line? ... > + 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)); With a temporary variable for r->configs[st->loop_idx * 2] the above can be written in much better way. > + } else if (p->flag == 2) { > + return 0; > + } > + p->config_values[st->desc_index + st->loop_idx] = > + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]); For the sake of consistency with the above suggestion also temporary for next config value. ... > + iter = ph->hops->iter_response_init(ph, &ops, 1, PINCTRL_SETTINGS_GET, > + sizeof(struct scmi_msg_settings_get), > + &ipriv); > + Redundant blank line. > + if (IS_ERR(iter)) > + return PTR_ERR(iter); ... > +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; When the above is not a dead code? > + 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; > +} ... > + ret = scmi_pinctrl_get_group_info(ph, selector, > + &pi->groups[selector]); One line? > + if (ret) > + return ret; ... > + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector, > + func->name, > + &func->nr_groups); At least last two lines can be joined. > + if (ret) > + return ret; ... > + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector, > + pin->name, NULL); It's pleany of room on the previous line. > + if (ret) > + return ret; ... > + ret = scmi_pinctrl_get_pin_info(ph, selector, > + &pi->pins[selector]); Ditto. > + if (ret) > + return ret; ... > +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); Huh?! Please, get yourself familiar with the scope of devm APIs. > + if (!pinfo) > + return -ENOMEM; > + > + ret = scmi_pinctrl_attributes_get(ph, pinfo); > + if (ret) > + return ret; > + > + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins, > + sizeof(*pinfo->pins), > + GFP_KERNEL); > + if (!pinfo->pins) > + return -ENOMEM; > + > + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups, > + sizeof(*pinfo->groups), > + GFP_KERNEL); > + if (!pinfo->groups) > + return -ENOMEM; > + > + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions, > + sizeof(*pinfo->functions), > + GFP_KERNEL); > + if (!pinfo->functions) > + return -ENOMEM; > + > + pinfo->version = version; > + > + return ph->set_priv(ph, pinfo, version); > +} > + > +static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph) > +{ > + int i; > + struct scmi_pinctrl_info *pi = ph->get_priv(ph); > + > + for (i = 0; i < pi->nr_groups; i++) { > + if (pi->groups[i].present) { > + kfree(pi->groups[i].group_pins); > + pi->groups[i].present = false; > + } > + } > + > + for (i = 0; i < pi->nr_functions; i++) { > + if (pi->functions[i].present) { > + kfree(pi->functions[i].groups); This is wrong in conjunction with the above. > + pi->functions[i].present = false; > + } > + } > + > + return 0; > +} ... > +static const struct scmi_protocol scmi_pinctrl = { > + .id = SCMI_PROTOCOL_PINCTRL, > + .owner = THIS_MODULE, > + .instance_init = &scmi_pinctrl_protocol_init, > + .instance_deinit = &scmi_pinctrl_protocol_deinit, > + .ops = &pinctrl_proto_ops, > + .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, > +}; > + Redundant blank line. > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
Hi Dan, > Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol > protocol basic support > > Looks really nice. Just a few small comments below. > > On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote: > > + > > +struct scmi_msg_func_set { > > + __le32 identifier; > > + __le32 function_id; > > + __le32 flags; > > +}; > > This scmi_msg_func_set struct is unused. Delete. > > > +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) > > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index); > > + else if (!p->flag) > > This is a nit-pick but could you change these !p->flag conditions to > p->flag == 0? It's a number zero, not a bool. > > > + 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) { > > + 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->flag == 0) { > > > + 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; > > + } > > + > > + 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, > > ->flag should be 0-2. > > > + .config_types = &config_type, > > + .config_values = config_value, > > + }; > > + > > + if (!config_value || type == FUNCTION_TYPE) > ^^^^^^^^^^^^ > config_value should be optional for flag == 2. As Cristian replied, I would keep it as is until we have a case in linux that need flag == 2. Thanks, Peng > > regards, > dan carpenter > > > + 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); > > +} > > +
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@nxp.com> > > > > Add basic implementation of the SCMI v3.2 pincontrol protocol. > > > > Hi, > > a few more comments down below... > > > Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > 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
Hi Andy, > Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol > protocol basic support > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti: > > From: Peng Fan <peng.fan@nxp.com> > > > > Add basic implementation of the SCMI v3.2 pincontrol protocol. > > ... > > > 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 > > Actually you want to have := here. > > > +scmi-protocols-y += pinctrl.o > > > > > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) > > $(scmi-transport-y) > > Side note: The -objs has to be -y > > ... > > > +#include <linux/module.h> > > +#include <linux/scmi_protocol.h> > > +#include <linux/slab.h> > > This is semi-random list of headers. Please, follow IWYU principle (include > what you use). There are a lot of inclusions I see missing (just in the context of > this page I see bits.h, types.h, and asm/byteorder.h). Is there any documentation about this requirement? Some headers are already included by others. > > ... > > > +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 > > Leave trailing comma as it's not a termination. > > > +}; > > ... > > > +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); > > This looks much better on a single line. Per Cristian, scmi drivers keep 80 max chars. > > > + if (ret) > > + return ret; > > + > > + attr = t->rx.buf; > > + > > + ret = ph->xops->do_xfer(ph, t); > > + if (!ret) { > > + pi->nr_functions = GET_FUNCTIONS_NR(attr- > >attributes_high); > > + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low); > > + pi->nr_pins = GET_PINS_NR(attr->attributes_low); > > + } > > + > > + ph->xops->xfer_put(ph, t); > > + return ret; > > +} > > ... > > > + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx), > > + sizeof(*rx), &t); > > Possible to have on a single line (if you use relaxed 100 limit). > Or (re)split it more logically: > > ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, > sizeof(*tx), sizeof(*rx), &t); > > > + if (ret) > > + return ret; > > ... > > > + /* > > + * 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) > > Please, use standard pattern, i.e. > > if (ret) > return ret; > > > + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, > selector, > > + (u32 *)&type, name, > > Why is an explicit casting needed? The type is enum, not u32. > > > + SCMI_MAX_STR_SIZE); > > + return ret; > > ... > > > + iter = ph->hops->iter_response_init(ph, &ops, size, > > + PINCTRL_LIST_ASSOCIATIONS, > > + sizeof(struct > scmi_msg_pinctrl_list_assoc), > > + &ipriv); > > > + > > Redundant blank line. > > > + if (IS_ERR(iter)) > > + return PTR_ERR(iter); > > ... > > > + if (p->flag == 1) > > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index); > > + else if (!p->flag) > > Be consistent, i.e. if (p->flag == 0) > > > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p- > >config_types[0]); > > ... > > > + st->num_remaining = le32_get_bits(r->num_configs, > > + GENMASK(31, 24)); > > One line? Scmi drivers use 80 max drivers. > > ... > > > + 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)); > > With a temporary variable for r->configs[st->loop_idx * 2] the above can be > written in much better way. ok. Fix in v7. > > > + } else if (p->flag == 2) { > > + return 0; > > + } > > > + p->config_values[st->desc_index + st->loop_idx] = > > + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]); > > For the sake of consistency with the above suggestion also temporary for next > config value. > > ... > > > + iter = ph->hops->iter_response_init(ph, &ops, 1, > PINCTRL_SETTINGS_GET, > > + sizeof(struct > scmi_msg_settings_get), > > + &ipriv); > > > + > > Redundant blank line. > > > + if (IS_ERR(iter)) > > + return PTR_ERR(iter); > > ... > > > +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; > > When the above is not a dead code? It could be removed. > > > + 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; > > +} > > ... > > > + ret = scmi_pinctrl_get_group_info(ph, selector, > > + &pi->groups[selector]); > > One line? > > > + if (ret) > > + return ret; > > ... > > > + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector, > > + func->name, > > + &func->nr_groups); > > At least last two lines can be joined. > > > + if (ret) > > + return ret; > > ... > > > + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector, > > + pin->name, NULL); > > It's pleany of room on the previous line. > > > + if (ret) > > + return ret; > > ... > > > + ret = scmi_pinctrl_get_pin_info(ph, selector, > > + &pi->pins[selector]); > > Ditto. > > > + if (ret) > > + return ret; > > ... > > > +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); > > Huh?! > > Please, get yourself familiar with the scope of devm APIs. Please teach me if this is wrong. > > > + if (!pinfo) > > + return -ENOMEM; > > + > > + ret = scmi_pinctrl_attributes_get(ph, pinfo); > > + if (ret) > > + return ret; > > + > > + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins, > > + sizeof(*pinfo->pins), > > + GFP_KERNEL); > > + if (!pinfo->pins) > > + return -ENOMEM; > > + > > + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups, > > + sizeof(*pinfo->groups), > > + GFP_KERNEL); > > + if (!pinfo->groups) > > + return -ENOMEM; > > + > > + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions, > > + sizeof(*pinfo->functions), > > + GFP_KERNEL); > > + if (!pinfo->functions) > > + return -ENOMEM; > > + > > + pinfo->version = version; > > + > > + return ph->set_priv(ph, pinfo, version); } > > + > > +static int scmi_pinctrl_protocol_deinit(const struct > > +scmi_protocol_handle *ph) { > > + int i; > > + struct scmi_pinctrl_info *pi = ph->get_priv(ph); > > + > > + for (i = 0; i < pi->nr_groups; i++) { > > + if (pi->groups[i].present) { > > + kfree(pi->groups[i].group_pins); > > + pi->groups[i].present = false; > > + } > > + } > > + > > + for (i = 0; i < pi->nr_functions; i++) { > > + if (pi->functions[i].present) { > > + kfree(pi->functions[i].groups); > > This is wrong in conjunction with the above. Yeah. > > > + pi->functions[i].present = false; > > + } > > + } > > + > > + return 0; > > +} > > ... > > > +static const struct scmi_protocol scmi_pinctrl = { > > + .id = SCMI_PROTOCOL_PINCTRL, > > + .owner = THIS_MODULE, > > + .instance_init = &scmi_pinctrl_protocol_init, > > + .instance_deinit = &scmi_pinctrl_protocol_deinit, > > + .ops = &pinctrl_proto_ops, > > + .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, }; > > > + > > Redundant blank line. Fix in v7 Thanks, Peng. > > > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl) > > > -- > With Best Regards, > Andy Shevchenko >
On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote: > Hi Andy, > Hi Peng, > > Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol > > protocol basic support > > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti: > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > Add basic implementation of the SCMI v3.2 pincontrol protocol. > > > > ... > > > > > 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 > > > > Actually you want to have := here. > > > > > +scmi-protocols-y += pinctrl.o > > > > > > > > > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) > > > $(scmi-transport-y) > > > > Side note: The -objs has to be -y > > > > ... > > > > > +#include <linux/module.h> > > > +#include <linux/scmi_protocol.h> > > > +#include <linux/slab.h> > > > > This is semi-random list of headers. Please, follow IWYU principle (include > > what you use). There are a lot of inclusions I see missing (just in the context of > > this page I see bits.h, types.h, and asm/byteorder.h). > > Is there any documentation about this requirement? > Some headers are already included by others. > Andy made (mostly) the same remarks on this same patch ~1-year ago on this same patch while it was posted by Oleksii. And I told that time that most of the remarks around devm_ usage were wrong due to how the SCMI core handles protocol initialization (using a devres group transparently). This is what I answered that time. https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t I wont repeat myself, but, in a nutshell the memory allocation like it is now is fine: a bit happens via devm_ at protocol initialization, the other is doe via explicit kmalloc at runtime and freed via kfree at remove time (if needed...i.e. checking the present flag of some structs) I'll made further remarks on v7 that you just posted. Thanks, Cristian
On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi <cristian.marussi@arm.com> wrote: > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote: > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti: ... > > > > +#include <linux/module.h> > > > > +#include <linux/scmi_protocol.h> > > > > +#include <linux/slab.h> > > > > > > This is semi-random list of headers. Please, follow IWYU principle (include > > > what you use). There are a lot of inclusions I see missing (just in the context of > > > this page I see bits.h, types.h, and asm/byteorder.h). > > > > Is there any documentation about this requirement? > > Some headers are already included by others. The documentation here is called "a common sense". The C language is built like this and we expect that nobody will invest into the dependency hell that we have already, that's why IWYU principle, please follow it. > Andy made (mostly) the same remarks on this same patch ~1-year ago on > this same patch while it was posted by Oleksii. > > And I told that time that most of the remarks around devm_ usage were > wrong due to how the SCMI core handles protocol initialization (using a > devres group transparently). > > This is what I answered that time. > > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t > > I wont repeat myself, but, in a nutshell the memory allocation like it > is now is fine: a bit happens via devm_ at protocol initialization, the > other is doe via explicit kmalloc at runtime and freed via kfree at > remove time (if needed...i.e. checking the present flag of some structs) This sounds like a mess. devm_ is expected to be used only for the ->probe() stage, otherwise you may consider cleanup.h (__free() macro) to have automatic free at the paths where memory is not needed. And the function naming doesn't suggest that you have a probe-remove pair. Moreover, if the init-deinit part is called in the probe-remove, the devm_ must not be mixed with non-devm ones, as it breaks the order and leads to subtle mistakes. > I'll made further remarks on v7 that you just posted.
> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol > protocol basic support > > On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi > <cristian.marussi@arm.com> wrote: > > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote: > > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti: > > ... > > > > > > +#include <linux/module.h> > > > > > +#include <linux/scmi_protocol.h> #include <linux/slab.h> > > > > > > > > This is semi-random list of headers. Please, follow IWYU principle > > > > (include what you use). There are a lot of inclusions I see > > > > missing (just in the context of this page I see bits.h, types.h, and > asm/byteorder.h). > > > > > > Is there any documentation about this requirement? > > > Some headers are already included by others. > > The documentation here is called "a common sense". > The C language is built like this and we expect that nobody will invest into the > dependency hell that we have already, that's why IWYU principle, please > follow it. > > > Andy made (mostly) the same remarks on this same patch ~1-year ago on > > this same patch while it was posted by Oleksii. > > > > And I told that time that most of the remarks around devm_ usage were > > wrong due to how the SCMI core handles protocol initialization (using > > a devres group transparently). > > > > This is what I answered that time. > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore > > .kernel.org%2Flinux-arm-kernel%2FZJ78hBcjAhiU%2BZBO%40e120937- > lin%2F%2 > > > 3t&data=05%7C02%7Cpeng.fan%40nxp.com%7C3f8c12062db048608e2a08d > c5315bed > > > 0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6384766000583 > 40430%7CUn > > > known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I > k1haW > > > wiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Whn3ehZjXy%2BcKG4irlWjQ6 > K3HF%2FofD > > Yu7j0Lrm8dN5k%3D&reserved=0 > > > > I wont repeat myself, but, in a nutshell the memory allocation like it > > is now is fine: a bit happens via devm_ at protocol initialization, > > the other is doe via explicit kmalloc at runtime and freed via kfree > > at remove time (if needed...i.e. checking the present flag of some > > structs) > > This sounds like a mess. devm_ is expected to be used only for the > ->probe() stage, otherwise you may consider cleanup.h (__free() macro) > to have automatic free at the paths where memory is not needed. > > And the function naming doesn't suggest that you have a probe-remove pair. > Moreover, if the init-deinit part is called in the probe-remove, the devm_ > must not be mixed with non-devm ones, as it breaks the order and leads to > subtle mistakes. I am new to __free() honestly. I'll let Cristian and Sudeep to comment on what should I do for v8. Thanks, Peng. > > > I'll made further remarks on v7 that you just posted. > > -- > With Best Regards, > Andy Shevchenko
Hi Andy, > Subject: RE: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol > protocol basic support > > > Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 > > pincontrol protocol basic support > > > > On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi > > <cristian.marussi@arm.com> wrote: > > > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote: > > > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti: > > > > ... > > > > > > > > +#include <linux/module.h> > > > > > > +#include <linux/scmi_protocol.h> #include <linux/slab.h> > > > > > > > > > > This is semi-random list of headers. Please, follow IWYU > > > > > principle (include what you use). There are a lot of inclusions > > > > > I see missing (just in the context of this page I see bits.h, > > > > > types.h, and > > asm/byteorder.h). > > > > > > > > Is there any documentation about this requirement? > > > > Some headers are already included by others. > > > > The documentation here is called "a common sense". > > The C language is built like this and we expect that nobody will > > invest into the dependency hell that we have already, that's why IWYU > > principle, please follow it. > > > > > Andy made (mostly) the same remarks on this same patch ~1-year ago > > > on this same patch while it was posted by Oleksii. > > > > > > And I told that time that most of the remarks around devm_ usage > > > were wrong due to how the SCMI core handles protocol initialization > > > (using a devres group transparently). > > > > > > This is what I answered that time. > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo > > > re > > > .kernel.org%2Flinux-arm-kernel%2FZJ78hBcjAhiU%2BZBO%40e120937- > > lin%2F%2 > > > > > > 3t&data=05%7C02%7Cpeng.fan%40nxp.com%7C3f8c12062db048608e2a08d > > c5315bed > > > > > > 0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6384766000583 > > 40430%7CUn > > > > > > known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I > > k1haW > > > > > > wiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Whn3ehZjXy%2BcKG4irlWjQ6 > > K3HF%2FofD > > > Yu7j0Lrm8dN5k%3D&reserved=0 > > > > > > I wont repeat myself, but, in a nutshell the memory allocation like > > > it is now is fine: a bit happens via devm_ at protocol > > > initialization, the other is doe via explicit kmalloc at runtime and > > > freed via kfree at remove time (if needed...i.e. checking the > > > present flag of some > > > structs) > > > > This sounds like a mess. devm_ is expected to be used only for the > > ->probe() stage, otherwise you may consider cleanup.h (__free() macro) > > to have automatic free at the paths where memory is not needed. > > > > And the function naming doesn't suggest that you have a probe-remove > pair. > > Moreover, if the init-deinit part is called in the probe-remove, the > > devm_ must not be mixed with non-devm ones, as it breaks the order and > > leads to subtle mistakes. > > I am new to __free() honestly. I'll let Cristian and Sudeep to comment on > what should I do for v8. Just give a look. But since most scmi firmware drivers are using devm_x APIs in protocol init. I would follow the style to use devm_x as of now. And for pinctrl protocol deinit phase, I will add a comment on why use kfree and what it is to free. For the __free macro, people drop all the scmi firmware drivers using devm_x APIs in init phase in a future patch. Is this ok? Thanks, Peng. > > Thanks, > Peng. > > > > > > I'll made further remarks on v7 that you just posted. > > > > -- > > With Best Regards, > > Andy Shevchenko
On Tue, Apr 02, 2024 at 04:06:06PM +0300, Andy Shevchenko wrote: > On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi > <cristian.marussi@arm.com> wrote: > > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote: > > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti: > > ... > > > > > > +#include <linux/module.h> > > > > > +#include <linux/scmi_protocol.h> > > > > > +#include <linux/slab.h> > > > > > > > > This is semi-random list of headers. Please, follow IWYU principle (include > > > > what you use). There are a lot of inclusions I see missing (just in the context of > > > > this page I see bits.h, types.h, and asm/byteorder.h). > > > > > > Is there any documentation about this requirement? > > > Some headers are already included by others. > > The documentation here is called "a common sense". > The C language is built like this and we expect that nobody will > invest into the dependency hell that we have already, that's why IWYU > principle, please follow it. > Yes, but given that we have a growing number of SCMI protocols there is a common local protocols.h header to group all includes needed by any protocols: the idea behind this (and the devm_ saga down below) was to ease development of protocols, since there are lots of them and growing, given the SCMI spec is extensible. > > Andy made (mostly) the same remarks on this same patch ~1-year ago on > > this same patch while it was posted by Oleksii. > > > > And I told that time that most of the remarks around devm_ usage were > > wrong due to how the SCMI core handles protocol initialization (using a > > devres group transparently). > > > > This is what I answered that time. > > > > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t > > > > I wont repeat myself, but, in a nutshell the memory allocation like it > > is now is fine: a bit happens via devm_ at protocol initialization, the > > other is doe via explicit kmalloc at runtime and freed via kfree at > > remove time (if needed...i.e. checking the present flag of some structs) > > This sounds like a mess. devm_ is expected to be used only for the > ->probe() stage, otherwise you may consider cleanup.h (__free() macro) > to have automatic free at the paths where memory is not needed. > Indeed, this protocol_init code is called by the SCMI core once for all when an SCMI driver tries at first to use this specific protocol by 'getting' its protocol_ops, so it is indeed called inside the probe chain of the driver: at this point you *can* decide to use devres to allocate memory and be assured that if the init fails, or when the driver cease to use this protocol (calling its remove()) and no other driver is using it, all the stuff that have been allocated related to this protocol will be released by the core for you. (using an internal devres group) Without this you should handle manually all the deallocation manually on the init error-paths AND also provide all the cleanup explicitly when the protocol is no more used by any driver (multiple users of the same protocol instance are possible)...for all protocols. This is/was handy since, till now, all the SCMI querying and resources allocation happened anyway all at once at init time... ...the mess, as you kindly called it, derives from the fact that this specific protocol is the first and only one that does NOT allocate all that it needs during the initialization (to minimize needless allocs for a lot of possibly unused resources) and this lazy-initialization phase, done after init at runtime, must be handled manually since it cannot be managed by the devres group that is open/clsoed around init by the SCMI core. I dont like particularly this split allocation but it has a reason and any other solution seems more messy to me at the moment. And I dont feel like changing all the SCMI protocol initialziation core code (that address a lot more under the hood) is a desirable solution to address a non-existent problem really. > And the function naming doesn't suggest that you have a probe-remove > pair. Moreover, if the init-deinit part is called in the probe-remove, > the devm_ must not be mixed with non-devm ones, as it breaks the order > and leads to subtle mistakes. > Initialization order is enforced by SCMI core like this: @driver_probe->get_protocol_ops() @core/get_protocol_ops -> devres_group_open() -> protocol_init->devm_*() -> devres_group_close() -> driver_probing @runtime optional explicit_lazy_kmallocs inside the protocol @driver_remove->put_protocol_ops() @core/put_protocol_ops() -> protocol_denit->optional_explicit_kfree_of_the_above -> devres_group_release() -> driver_removing ... dont think there's an ordering problem. ...note that the ph->dev provided in the protocol_init and used by devm_ is NOT the dev of the SCMI driver probe/remove that uses the get_protocol_ops, it is an internal SCMI device associated with the core SCMI stack probing and allocations, within which a devres group for the specific protocol is created when that specific protocol is initialized...protocols are not fully fledged drivers are just bits of the SCMI stack that are initialized when needed (and possibly also loaded when needed for vendor protocols) and de-initialzed when no more SCMI driver users exist for that protocol. Thanks, Cristian
On Tue, Apr 2, 2024 at 6:58 PM Cristian Marussi <cristian.marussi@arm.com> wrote: > On Tue, Apr 02, 2024 at 04:06:06PM +0300, Andy Shevchenko wrote: > > On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi > > <cristian.marussi@arm.com> wrote: > > > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote: > > > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti: ... > > > > > > +#include <linux/module.h> > > > > > > +#include <linux/scmi_protocol.h> > > > > > > +#include <linux/slab.h> > > > > > > > > > > This is semi-random list of headers. Please, follow IWYU principle (include > > > > > what you use). There are a lot of inclusions I see missing (just in the context of > > > > > this page I see bits.h, types.h, and asm/byteorder.h). > > > > > > > > Is there any documentation about this requirement? > > > > Some headers are already included by others. > > > > The documentation here is called "a common sense". > > The C language is built like this and we expect that nobody will > > invest into the dependency hell that we have already, that's why IWYU > > principle, please follow it. > > Yes, but given that we have a growing number of SCMI protocols there is a > common local protocols.h header to group all includes needed by any > protocols: the idea behind this (and the devm_ saga down below) was to ease > development of protocols, since there are lots of them and growing, given > the SCMI spec is extensible. Yes, and what you are effectively suggesting is: "Technical debt? Oh, fine, we do not care!" This is not good. I'm in a long term of cleaning up the dependency hell in the kernel (my main focus is kernel.h for now) and I am talking from my experience. I do not like what people are doing in 95% of the code, that's why I really want to stop the bad practices as soon as possible. Last to add, but not least is that your code may be used as an example for others, hence we really have to do our best in order to avoid bad design, practices, and cargo cults. If this requires more refactoring of the existing code, then do it sooner than later. ... > > > Andy made (mostly) the same remarks on this same patch ~1-year ago on > > > this same patch while it was posted by Oleksii. > > > > > > And I told that time that most of the remarks around devm_ usage were > > > wrong due to how the SCMI core handles protocol initialization (using a > > > devres group transparently). > > > > > > This is what I answered that time. > > > > > > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t > > > > > > I wont repeat myself, but, in a nutshell the memory allocation like it > > > is now is fine: a bit happens via devm_ at protocol initialization, the > > > other is doe via explicit kmalloc at runtime and freed via kfree at > > > remove time (if needed...i.e. checking the present flag of some structs) > > > > This sounds like a mess. devm_ is expected to be used only for the > > ->probe() stage, otherwise you may consider cleanup.h (__free() macro) > > to have automatic free at the paths where memory is not needed. > > Indeed, this protocol_init code is called by the SCMI core once for all when > an SCMI driver tries at first to use this specific protocol by 'getting' its > protocol_ops, so it is indeed called inside the probe chain of the driver: > at this point you *can* decide to use devres to allocate memory and be assured > that if the init fails, or when the driver cease to use this protocol (calling > its remove()) and no other driver is using it, all the stuff that have been > allocated related to this protocol will be released by the core for you. > (using an internal devres group) > > Without this you should handle manually all the deallocation manually on > the init error-paths AND also provide all the cleanup explicitly when > the protocol is no more used by any driver (multiple users of the same > protocol instance are possible)...for all protocols. Yes. Is it a problem? > This is/was handy since, till now, all the SCMI querying and resources > allocation happened anyway all at once at init time... > > ...the mess, as you kindly called it, derives from the fact that this specific > protocol is the first and only one that does NOT allocate all that it needs > during the initialization (to minimize needless allocs for a lot of possibly > unused resources) and this lazy-initialization phase, done after init at runtime, > must be handled manually since it cannot be managed by the devres group that is > open/clsoed around init by the SCMI core. > > I dont like particularly this split allocation but it has a reason and any > other solution seems more messy to me at the moment. > > And I dont feel like changing all the SCMI protocol initialziation core code > (that address a lot more under the hood) is a desirable solution to address a > non-existent problem really. > > > And the function naming doesn't suggest that you have a probe-remove > > pair. Moreover, if the init-deinit part is called in the probe-remove, > > the devm_ must not be mixed with non-devm ones, as it breaks the order > > and leads to subtle mistakes. > > Initialization order is enforced by SCMI core like this: > > @driver_probe->get_protocol_ops() > @core/get_protocol_ops > -> devres_group_open() > -> protocol_init->devm_*() > -> devres_group_close() > -> driver_probing > > @runtime optional explicit_lazy_kmallocs inside the protocol > > @driver_remove->put_protocol_ops() > @core/put_protocol_ops() > -> protocol_denit->optional_explicit_kfree_of_the_above > -> devres_group_release() > -> driver_removing > > ... dont think there's an ordering problem. The mess with devm_ vs. non-devm is quite easy to achieve. You are probably out of the control of what the protocol driver wants to do in the init. Is the usage of devm (which APIs and in which order can be used) WRT SCMI documented somewhere? Misuse of devm is a common issue, I'm not surprised it will hit your subsystem one day with such an approach. > ...note that the ph->dev provided in the protocol_init and used by devm_ > is NOT the dev of the SCMI driver probe/remove that uses the get_protocol_ops, > it is an internal SCMI device associated with the core SCMI stack probing and > allocations, within which a devres group for the specific protocol is created > when that specific protocol is initialized...protocols are not fully > fledged drivers are just bits of the SCMI stack that are initialized when needed > (and possibly also loaded when needed for vendor protocols) and > de-initialzed when no more SCMI driver users exist for that protocol. P.S. I guess from now on it's your call, but this code and in case other drivers use similar, is badly written. I hope some documentation exists to at least justify all this mess and explaining why there no and (what's really important) will never be a problem.
On Wed, Apr 3, 2024 at 11:06 AM Cristian Marussi <cristian.marussi@arm.com> wrote: > On Tue, Apr 02, 2024 at 07:39:44PM +0300, Andy Shevchenko wrote: > > On Tue, Apr 2, 2024 at 6:58 PM Cristian Marussi > > <cristian.marussi@arm.com> wrote: > > > On Tue, Apr 02, 2024 at 04:06:06PM +0300, Andy Shevchenko wrote: > > > > On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi > > > > <cristian.marussi@arm.com> wrote: > > > > > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote: > > > > > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti: ... > > > > > > > > +#include <linux/module.h> > > > > > > > > +#include <linux/scmi_protocol.h> > > > > > > > > +#include <linux/slab.h> > > > > > > > > > > > > > > This is semi-random list of headers. Please, follow IWYU principle (include > > > > > > > what you use). There are a lot of inclusions I see missing (just in the context of > > > > > > > this page I see bits.h, types.h, and asm/byteorder.h). > > > > > > > > > > > > Is there any documentation about this requirement? > > > > > > Some headers are already included by others. > > > > > > > > The documentation here is called "a common sense". > > > > The C language is built like this and we expect that nobody will > > > > invest into the dependency hell that we have already, that's why IWYU > > > > principle, please follow it. > > > > > > Yes, but given that we have a growing number of SCMI protocols there is a > > > common local protocols.h header to group all includes needed by any > > > protocols: the idea behind this (and the devm_ saga down below) was to ease > > > development of protocols, since there are lots of them and growing, given > > > the SCMI spec is extensible. > > > > Yes, and what you are effectively suggesting is: "Technical debt? Oh, > > fine, we do not care!" This is not good. I'm in a long term of > > cleaning up the dependency hell in the kernel (my main focus is > > kernel.h for now) and I am talking from my experience. I do not like > > what people are doing in 95% of the code, that's why I really want to > > stop the bad practices as soon as possible. > > Not at all, the aim was exactly the opposite, avoiding that some protocol > could have been written without all the needed includes: since a basic set > of headers is definitely common to any protocol you may think to write, > grouping all there was meant to avoid this...I thought that by moving the > problem away in one single internal common header was easier to monitor. Which may or may not be okay. It plays too smart, so the end developer won't care about real headers they need as they are the only ones who know the source code in the best possible way. > I certainly maybe wrong, but I dont see how you can deduce I dont care... See above, the protocols.h it's a reincarnation (much less twisted and ugly, though) of something like kernel.h. Do you know why we have a split to headers instead of having everything in one? It speeds up a build, it separates namespace (to the extent of what we call "namespace" in C language), it makes the modularization (of the source code) better (easier to avoid considering what's not needed), and so on. I don't think making a "common" header is a good idea. > ...and maybe, only maybe, what that 95% of people is trying to do in their > horrible code is to deliver the best reasonably possible thing within their > timeline while you are barking at them in chase of never to be released utter > perfection. Yeah. That's why it's good to teach people about many aspects of the C language (which is not popular, in particular due to these nuances, nowadays and we are starving for good developers). > > Last to add, but not least is that your code may be used as an example > > for others, hence we really have to do our best in order to avoid bad > > design, practices, and cargo cults. If this requires more refactoring > > of the existing code, then do it sooner than later. ... > > > > > Andy made (mostly) the same remarks on this same patch ~1-year ago on > > > > > this same patch while it was posted by Oleksii. > > > > > > > > > > And I told that time that most of the remarks around devm_ usage were > > > > > wrong due to how the SCMI core handles protocol initialization (using a > > > > > devres group transparently). > > > > > > > > > > This is what I answered that time. > > > > > > > > > > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t > > > > > > > > > > I wont repeat myself, but, in a nutshell the memory allocation like it > > > > > is now is fine: a bit happens via devm_ at protocol initialization, the > > > > > other is doe via explicit kmalloc at runtime and freed via kfree at > > > > > remove time (if needed...i.e. checking the present flag of some structs) > > > > > > > > This sounds like a mess. devm_ is expected to be used only for the > > > > ->probe() stage, otherwise you may consider cleanup.h (__free() macro) > > > > to have automatic free at the paths where memory is not needed. > > > > > > Indeed, this protocol_init code is called by the SCMI core once for all when > > > an SCMI driver tries at first to use this specific protocol by 'getting' its > > > protocol_ops, so it is indeed called inside the probe chain of the driver: > > > at this point you *can* decide to use devres to allocate memory and be assured > > > that if the init fails, or when the driver cease to use this protocol (calling > > > its remove()) and no other driver is using it, all the stuff that have been > > > allocated related to this protocol will be released by the core for you. > > > (using an internal devres group) > > > > > > Without this you should handle manually all the deallocation manually on > > > the init error-paths AND also provide all the cleanup explicitly when > > > the protocol is no more used by any driver (multiple users of the same > > > protocol instance are possible)...for all protocols. > > > > Yes. Is it a problem? > > Well, no, but is it not a repetitive and error-prone process ? Yes. That's why we now have cleanup.h. Please, consider using it instead. > Is it not the exact reason why devres management exist in first place, to avoid > repetitive manual alloc/free of resources and related pitfalls ? (even though > certainly it is normally used in a more conventional and straightforward way) No. Its scope is to clean the probe-remove parts, one should not spread it otherwise and one definitely should know its limitations and corner cases (I even gave some examples back in ca. 2017 in one of my presentation WRT typical mistakes the developers make > The idea was to give some sort of aid in the SCMI stack for writing protocols, > so regarding mem_mgmt, I just built on top of devres facilities, not invented > anything, to try to avoid repetitions and let the core handle mem allocs/free > during the probe phases as much as possible: in pinctrl case would be > particularly trivial to instead manually allocate stuff at init (due to many > lazy delayed allocations) but other protocols need a lot more to be done at init, > frequently in a loop to allocate multiple resources descriptors, and manually > undoing all of that on each error-path and on cleanup is definitely error-prone > and a pain. I understand the motivation, but again, devm is a beast in the corner cases. I believe Laurent Pinchart gave a presentation about how bad devm can hit you if you don't know what you are doing (OTOH it's hard to know with devm). > Last but not least, this whole thing was designed to address the needs of the > protocols that existed at that time....it is only now with pinctrl lazy-allocations > at runtime that the ugly cohexistence of devm_ and non-devm allocations became a > thing....so clearly the thing needs to be improved/rethinked...even dropped if no > more fitting... > > ... or alternatively since devres allocations are anyway optional, you could just > use regular kmalloc/kfree for this protocol and avoid this dual handling... Probably, just have a look at cleanup.h. > ...this was just to put things in context...and I'll happily let Sudeep decide > what he prefers in the immediate for pinctrl or more in general about all the > scmi devres, that I've got enough of these pleasant interactions for now... As I said, it's your call, I'm not preventing you from applying whatever is going on in the SCMI subsystem. Just tried to point out the problem(s).
This patchset is a rework from Oleksii's RFC v5 patchset https://lore.kernel.org/all/cover.1698353854.git.oleksii_moisieiev@epam.com/ This patchset introduces some changes based on RFC v5: - introduce helper get_max_msg_size - support compatible string - iterate the id_table - Support multiple configs in one command - Added i.MX support - Patch 5 firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support is almost same as RFCv5 expect multiple configs support. - Patch 4 the dt-bindings includes compatible string to support i.MX - Rebased on 2023-12-15 linux-next/master If any comments from RFC v5 are missed, I am sorry in advance. This PINCTRL Protocol is following Version 3.2 SCMI Spec Beta release. On ARM-based systems, a separate Cortex-M based System Control Processor (SCP) provides control on pins, as well as with power, clocks, reset controllers. So implement the driver to support such cases. The i.MX95 Example as below: Configuration: The scmi-pinctrl driver can be configured using DT bindings. For example: / { sram0: sram@445b1000 { compatible = "mmio-sram"; reg = <0x0 0x445b1000 0x0 0x400>; #address-cells = <1>; #size-cells = <1>; ranges = <0x0 0x0 0x445b1000 0x400>; scmi_buf0: scmi-sram-section@0 { compatible = "arm,scmi-shmem"; reg = <0x0 0x80>; }; scmi_buf1: scmi-sram-section@80 { compatible = "arm,scmi-shmem"; reg = <0x80 0x80>; }; }; firmware { scmi { compatible = "arm,scmi"; mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>; shmem = <&scmi_buf0>, <&scmi_buf1>; #address-cells = <1>; #size-cells = <0>; scmi_iomuxc: protocol@19 { compatible = "fsl,imx95-scmi-pinctrl"; reg = <0x19>; }; }; }; }; &scmi_iomuxc { pinctrl_tpm3: tpm3grp { fsl,pins = < IMX95_PAD_GPIO_IO12__TPM3_CH2(0x51e) >; }; }; This patchset has been tested on i.MX95-19x19-EVK board. Signed-off-by: Peng Fan <peng.fan@nxp.com> --- Changes in v6: - Update pinctrl driver following ARM SCMI 3.2 public release - Addressed Dan's comments, and followed Dan's suggestions, thanks. - Dropped R-b/T-b in patch 3/4 and patch 4/4, - Link to v5: https://lore.kernel.org/r/20240314-pinctrl-scmi-v5-0-b19576e557f2@nxp.com Changes in v5: - Rebased to linux-next next-20240313 - Link to v4: https://lore.kernel.org/r/20240223-pinctrl-scmi-v4-0-10eb5a379274@nxp.com Changes in v4: - Rebased to next-20240222 - Drop pinctrl-scmi-imx and compatible patches in V3 - Add T-b and R-b collected from v3 - Link to v3: https://lore.kernel.org/r/20240121-pinctrl-scmi-v3-0-8d94ba79dca8@nxp.com Changes in v3: - Add R-b for dt-binding patch - Use 80 chars per line to align with other scmi drivers - Add pinctrl_scmi_alloc_configs pinctrl_scmi_free_configs to replace driver global config_value and config_type array to avoid in parrell access issue. When num_configs is larger than 4, use alloc, else use stack. - Drop the separate MAITAINERS entry for firmware scmi pinctrl - Use enum type, not u8 when referring the scmi or generic pin conf type - Drop scmi_pinctrl_config_get_all which is not used at all for now. - Update copyright year to 2024 - Move the enum scmi_pinctrl_conf_type above pinctrl_proto_ops for consistency - Link to v2: https://lore.kernel.org/r/20240104-pinctrl-scmi-v2-0-a9bd86ab5a84@nxp.com Changes in v2: Added comments, and added R-b for Patch 1 Moved the compatile string and i.MX patch to the end, marked NOT APPLY Patchset based on lore.kernel.org/all/20231221151129.325749-1-cristian.marussi@arm.com/ Addressed the binding doc issue, dropped i.MX content. For the firmware pinctrl scmi driver, addressed the comments from Cristian For the pinctrl scmi driver, addressed comments from Cristian For the i.MX95 OEM stuff, I not have good idea, expect using compatbile string. Maybe the firmware public an protocol attribute to indicate it is VENDOR stuff or NXP use a new protocol id, not 0x19. But I think current pinctrl-scmi.c not able to support OEM config, should we extend it with some method? Anyway if patch 1-4 is good enough, they could be picked up first. Since I am only able to test the patch on i.MX95 which not support geneirc pinconf, only OEM configs are tested in my side. --- Peng Fan (4): firmware: arm_scmi: introduce helper get_max_msg_size dt-bindings: firmware: arm,scmi: support pinctrl protocol firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support pinctrl: Implementation of the generic scmi-pinctrl driver .../devicetree/bindings/firmware/arm,scmi.yaml | 50 ++ MAINTAINERS | 1 + drivers/firmware/arm_scmi/Makefile | 1 + drivers/firmware/arm_scmi/driver.c | 17 + drivers/firmware/arm_scmi/pinctrl.c | 922 +++++++++++++++++++++ drivers/firmware/arm_scmi/protocols.h | 3 + drivers/pinctrl/Kconfig | 11 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-scmi.c | 564 +++++++++++++ include/linux/scmi_protocol.h | 75 ++ 10 files changed, 1645 insertions(+) --- base-commit: 70ca90deeca172d343bd18bf7fb2f992214c23c0 change-id: 20231215-pinctrl-scmi-4c5b0374f4c6 Best regards,