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