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

[Cristian Marussi <cristian.marussi@xxxxxxx>]

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,
Cristian