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

[Peng Fan <peng.fan@xxxxxxx>]

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);
> > +}
> > +