lkml.org 
[lkml]   [2024]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
    Date
    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);
    > > +}
    > > +


    \
     
     \ /
      Last update: 2024-03-31 14:47    [W:2.754 / U:0.464 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site