> 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
