Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
> From: Peng Fan <peng.fan@xxxxxxx>
>
> Add basic implementation of the SCMI v3.2 pincontrol protocol.
...
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
Actually you want to have := here.
> +scmi-protocols-y += pinctrl.o
> scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
Side note: The -objs has to be -y
...
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
This is semi-random list of headers. Please, follow IWYU principle
(include what you use). There are a lot of inclusions I see missing
(just in the context of this page I see bits.h, types.h, and
asm/byteorder.h).
...
> +enum scmi_pinctrl_protocol_cmd {
> + PINCTRL_ATTRIBUTES = 0x3,
> + PINCTRL_LIST_ASSOCIATIONS = 0x4,
> + PINCTRL_SETTINGS_GET = 0x5,
> + PINCTRL_SETTINGS_CONFIGURE = 0x6,
> + PINCTRL_REQUEST = 0x7,
> + PINCTRL_RELEASE = 0x8,
> + PINCTRL_NAME_GET = 0x9,
> + PINCTRL_SET_PERMISSIONS = 0xa
Leave trailing comma as it's not a termination.
> +};
...
> +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> + struct scmi_pinctrl_info *pi)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_pinctrl_protocol_attributes *attr;
> +
> + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr),
> + &t);
This looks much better on a single line.
> + if (ret)
> + return ret;
> +
> + attr = t->rx.buf;
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
> + pi->nr_functions = GET_FUNCTIONS_NR(attr->attributes_high);
> + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low);
> + pi->nr_pins = GET_PINS_NR(attr->attributes_low);
> + }
> +
> + ph->xops->xfer_put(ph, t);
> + return ret;
> +}
...
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
> + sizeof(*rx), &t);
Possible to have on a single line (if you use relaxed 100 limit).
Or (re)split it more logically:
ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES,
sizeof(*tx), sizeof(*rx), &t);
> + if (ret)
> + return ret;
...
> + /*
> + * If supported overwrite short name with the extended one;
> + * on error just carry on and use already provided short name.
> + */
> + if (!ret && ext_name_flag)
Please, use standard pattern, i.e.
if (ret)
return ret;
> + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
> + (u32 *)&type, name,
Why is an explicit casting needed?
> + SCMI_MAX_STR_SIZE);
> + return ret;
...
> + iter = ph->hops->iter_response_init(ph, &ops, size,
> + PINCTRL_LIST_ASSOCIATIONS,
> + sizeof(struct scmi_msg_pinctrl_list_assoc),
> + &ipriv);
> +
Redundant blank line.
> + if (IS_ERR(iter))
> + return PTR_ERR(iter);
...
> + if (p->flag == 1)
> + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> + else if (!p->flag)
Be consistent, i.e. if (p->flag == 0)
> + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p->config_types[0]);
...
> + st->num_remaining = le32_get_bits(r->num_configs,
> + GENMASK(31, 24));
One line?
...
> + if (!p->flag) {
> + 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));
With a temporary variable for r->configs[st->loop_idx * 2] the above can be
written in much better way.
> + } 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]);
For the sake of consistency with the above suggestion also temporary for next
config value.
...
> + iter = ph->hops->iter_response_init(ph, &ops, 1, PINCTRL_SETTINGS_GET,
> + sizeof(struct scmi_msg_settings_get),
> + &ipriv);
> +
Redundant blank line.
> + if (IS_ERR(iter))
> + return PTR_ERR(iter);
...
> +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + struct scmi_group_info *group)
> +{
> + int ret;
> + if (!group)
> + return -EINVAL;
When the above is not a dead code?
> + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
> + group->name,
> + &group->nr_pins);
> + if (ret)
> + return ret;
> +
> + if (!group->nr_pins) {
> + dev_err(ph->dev, "Group %d has 0 elements", selector);
> + return -ENODATA;
> + }
> +
> + group->group_pins = kmalloc_array(group->nr_pins,
> + sizeof(*group->group_pins),
> + GFP_KERNEL);
> + if (!group->group_pins)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> + group->nr_pins, group->group_pins);
> + if (ret) {
> + kfree(group->group_pins);
> + return ret;
> + }
> +
> + group->present = true;
> + return 0;
> +}
...
> + ret = scmi_pinctrl_get_group_info(ph, selector,
> + &pi->groups[selector]);
One line?
> + if (ret)
> + return ret;
...
> + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> + func->name,
> + &func->nr_groups);
At least last two lines can be joined.
> + if (ret)
> + return ret;
...
> + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> + pin->name, NULL);
It's pleany of room on the previous line.
> + if (ret)
> + return ret;
...
> + ret = scmi_pinctrl_get_pin_info(ph, selector,
> + &pi->pins[selector]);
Ditto.
> + if (ret)
> + return ret;
...
> +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + int ret;
> + u32 version;
> + struct scmi_pinctrl_info *pinfo;
> +
> + ret = ph->xops->version_get(ph, &version);
> + if (ret)
> + return ret;
> +
> + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
Huh?!
Please, get yourself familiar with the scope of devm APIs.
> + if (!pinfo)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> + if (ret)
> + return ret;
> +
> + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
> + sizeof(*pinfo->pins),
> + GFP_KERNEL);
> + if (!pinfo->pins)
> + return -ENOMEM;
> +
> + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
> + sizeof(*pinfo->groups),
> + GFP_KERNEL);
> + if (!pinfo->groups)
> + return -ENOMEM;
> +
> + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
> + sizeof(*pinfo->functions),
> + GFP_KERNEL);
> + if (!pinfo->functions)
> + return -ENOMEM;
> +
> + pinfo->version = version;
> +
> + return ph->set_priv(ph, pinfo, version);
> +}
> +
> +static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
> +{
> + int i;
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + for (i = 0; i < pi->nr_groups; i++) {
> + if (pi->groups[i].present) {
> + kfree(pi->groups[i].group_pins);
> + pi->groups[i].present = false;
> + }
> + }
> +
> + for (i = 0; i < pi->nr_functions; i++) {
> + if (pi->functions[i].present) {
> + kfree(pi->functions[i].groups);
This is wrong in conjunction with the above.
> + pi->functions[i].present = false;
> + }
> + }
> +
> + return 0;
> +}
...
> +static const struct scmi_protocol scmi_pinctrl = {
> + .id = SCMI_PROTOCOL_PINCTRL,
> + .owner = THIS_MODULE,
> + .instance_init = &scmi_pinctrl_protocol_init,
> + .instance_deinit = &scmi_pinctrl_protocol_deinit,
> + .ops = &pinctrl_proto_ops,
> + .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
> +};
> +
Redundant blank line.
> +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
--
With Best Regards,
Andy Shevchenko
