> Subject: Re: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol > protocol basic support > > On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@xxxxxxx> > > > > Add basic implementation of the SCMI v3.2 pincontrol protocol. > > > > Hi, > > > > Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx> > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx> > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx> > > --- > > [snip] > > > > +struct scmi_settings_get_ipriv { > > + u32 selector; > > + enum scmi_pinctrl_selector_type type; > > + bool get_all; > > + 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(SELECTOR_MASK, p->type); > > + > > + if (p->get_all) { > > + attributes |= FIELD_PREP(CONFIG_FLAG_MASK, 1) | > > + FIELD_PREP(SKIP_CONFIGS_MASK, desc_index); > > + } else { > > + 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->get_all) { > > + 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; > > + u32 type = le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, > 0)); > > + u32 val = le32_to_cpu(r->configs[st->loop_idx * 2 + 1]); > > + > > + if (p->get_all) { > > + p->config_types[st->desc_index + st->loop_idx] = type; > > + } else { > > + if (p->config_types[0] != type) > > + return -EINVAL; > > + } > > + > > + p->config_values[st->desc_index + st->loop_idx] = val; > > + > > + 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, bool get_all) { > > + 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, > > + .get_all = get_all, > > + .config_types = &config_type, > > + .config_values = config_value, > > + }; > > + > > + 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, SCMI_PIN_OEM_END, > > + 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_get_one(const struct scmi_protocol_handle > *ph, > > + u32 selector, > > + enum scmi_pinctrl_selector_type > type, > > + enum scmi_pinctrl_conf_type > config_type, > > + u32 *config_value) > > +{ > > + return scmi_pinctrl_settings_get(ph, selector, type, config_type, > > + config_value, false); > > +} > > + > > +static int scmi_pinctrl_settings_get_all(const struct scmi_protocol_handle > *ph, > > + u32 selector, > > + enum scmi_pinctrl_selector_type > type, > > + enum scmi_pinctrl_conf_type > config_type, > > + u32 *config_value) > > +{ > > + return scmi_pinctrl_settings_get(ph, selector, type, config_type, > > + config_value, true); > > +} > > + > > If you generalize the scmi_pinctrl_settings_get() and reintroduce a > .settings_get_all() ops (even though unused by pinctrl driver, I am fine with > this..), you should take care to pass as an input parameter NOT only the array > of config_values BUT also an array of config_types since you could get back > up to 256 OEM types: for this reason you will need also to pass to > scmi_pinctrl_settings_get() an input param that specifies the sizes of the > 2 array input params (in order to avoid oveflows) AND use that same inout > param also as an output param to report at the end how many OEM types > were effectively found and returned.... > > IOW, I did this on top of your V7 to make the settings_get_all work: > > ---8<--- > diff --git a/drivers/firmware/arm_scmi/pinctrl.c > b/drivers/firmware/arm_scmi/pinctrl.c > index b75af1dd75fa..f4937af66c4d 100644 > --- a/drivers/firmware/arm_scmi/pinctrl.c > +++ b/drivers/firmware/arm_scmi/pinctrl.c > @@ -317,6 +317,7 @@ struct scmi_settings_get_ipriv { > u32 selector; > enum scmi_pinctrl_selector_type type; > bool get_all; > + unsigned int *nr_configs; > enum scmi_pinctrl_conf_type *config_types; > u32 *config_values; > }; > @@ -379,6 +380,7 @@ iter_pinctrl_settings_get_process_response(const > struct scmi_protocol_handle *ph > } > > p->config_values[st->desc_index + st->loop_idx] = val; > + ++*p->nr_configs; > > return 0; > } > @@ -386,11 +388,13 @@ iter_pinctrl_settings_get_process_response(const > struct scmi_protocol_handle *ph 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, bool get_all) > + unsigned int *nr_configs, > + enum scmi_pinctrl_conf_type *config_types, > + u32 *config_values) > { > int ret; > void *iter; > + unsigned int max_configs = *nr_configs; > struct scmi_iterator_ops ops = { > .prepare_message = > iter_pinctrl_settings_get_prepare_message, > .update_state = iter_pinctrl_settings_get_update_state, > @@ -399,19 +403,22 @@ scmi_pinctrl_settings_get(const struct > scmi_protocol_handle *ph, u32 selector, > struct scmi_settings_get_ipriv ipriv = { > .selector = selector, > .type = type, > - .get_all = get_all, > - .config_types = &config_type, > - .config_values = config_value, > + .get_all = (max_configs > 1), > + .nr_configs = nr_configs, > + .config_types = config_types, > + .config_values = config_values, > }; > > - if (!config_value || type == FUNCTION_TYPE) > + if (!config_types || !config_values || 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, SCMI_PIN_OEM_END, > + /* Prepare to count returned configs */ > + *nr_configs = 0; > + iter = ph->hops->iter_response_init(ph, &ops, max_configs, > PINCTRL_SETTINGS_GET, > sizeof(struct > scmi_msg_settings_get), > &ipriv); > @@ -427,18 +434,24 @@ static int scmi_pinctrl_settings_get_one(const > struct scmi_protocol_handle *ph, > enum scmi_pinctrl_conf_type > config_type, > u32 *config_value) > { > - return scmi_pinctrl_settings_get(ph, selector, type, config_type, > - config_value, false); > + unsigned int nr_configs = 1; > + > + return scmi_pinctrl_settings_get(ph, selector, type, &nr_configs, > + &config_type, config_value); > } > > static int scmi_pinctrl_settings_get_all(const struct scmi_protocol_handle > *ph, > u32 selector, > enum scmi_pinctrl_selector_type > type, > - enum scmi_pinctrl_conf_type > config_type, > - u32 *config_value) > + unsigned int *nr_configs, > + enum scmi_pinctrl_conf_type > *config_types, > + u32 *config_values) > { > - return scmi_pinctrl_settings_get(ph, selector, type, config_type, > - config_value, true); > + if (!nr_configs || *nr_configs == 0) > + return -EINVAL; > + > + return scmi_pinctrl_settings_get(ph, selector, type, nr_configs, > + config_types, config_values); > } > > static int > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h > index abaf6122ea37..7915792efd81 100644 > --- a/include/linux/scmi_protocol.h > +++ b/include/linux/scmi_protocol.h > @@ -882,8 +882,9 @@ struct scmi_pinctrl_proto_ops { > int (*settings_get_all)(const struct scmi_protocol_handle *ph, > u32 selector, > enum scmi_pinctrl_selector_type type, > - enum scmi_pinctrl_conf_type config_type, > - u32 *config_value); > + unsigned int *nr_configs, > + enum scmi_pinctrl_conf_type *config_types, > + u32 *config_values); > int (*settings_conf)(const struct scmi_protocol_handle *ph, > u32 selector, enum scmi_pinctrl_selector_type > type, > unsigned int nr_configs, > --->8----- > > Please check if this addition sounds good to you and integrate into v8 > eventually... Thanks for helping on this, I will included your changes, and your Co-developed-by tag if you not mind. Thanks, Peng. > > Thanks, > Cristian