RE: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[Peng Fan <peng.fan@xxxxxxx>]

> 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