Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev kirjoitti:
> scmi: Introduce pinctrl SCMI protocol driver
Seems like you forgot to remove previous line(s) in the commit message and
the above has to be the Subject here.
> Add basic implementation of the SCMI v3.2 pincontrol protocol
> excluding GPIO support. All pinctrl related callbacks and operations
> are exposed in the include/linux/scmi_protocol.h
drop include/ part, everybody will understand that. Also mind the grammar
period.
...
> -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
Why not splitting it and make it ordered?
...
Missing headers:
bitfield.h
bits.h
byteorder/
types.h
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
Missing
asm/unaligned.h
...
> +struct scmi_group_info {
> + bool present;
> + char name[SCMI_MAX_STR_SIZE];
> + unsigned int *group_pins;
> + unsigned int nr_pins;
> +};
So, why struct pingroup can't be embeded here?
> +struct scmi_function_info {
> + bool present;
> + char name[SCMI_MAX_STR_SIZE];
> + unsigned int *groups;
> + unsigned int nr_groups;
> +};
So, why and struct pinfunction can't be embedded here (yes, you would need a
duplication of groups as here they are integers)?
As far as I understand these data structures are not part of any ABI (otherwise
the wrong type(s) / padding might be in use) and hence don't see the impediments
to use them, but would be nice to have a comment on top of each.
...
> +struct scmi_pin_info {
> + bool present;
> + char name[SCMI_MAX_STR_SIZE];
Swapping order might help compiler to generate a better code.
Also this applies to the _group_info and _function_info.
> +};
...
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
Can you rather follow the usual pattern, i.e. checking for the errors?
if (ret)
goto out_put_xfer;
> + 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);
> + }
out_put_xfer:
> + ph->xops->xfer_put(ph, t);
> + return ret;
...
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
Ditto.
> + if (n_elems)
> + *n_elems = NUM_ELEMS(rx->attributes);
> +
> + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
> + }
> +
> + ph->xops->xfer_put(ph, t);
> +
> + /*
> + * 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(rx->attributes))
if (ret)
return ret;
> + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
> + (u32 *)&type, name,
> + SCMI_MAX_STR_SIZE);
> + return ret;
return 0;
> +}
...
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret)
if (ret)
goto out_put_xfer;
(but in this and similar, aka one line, cases the current wouldn't be bad, just
matter of the consistency with the rest of the code)
> + *config_value = get_unaligned_le32(t->rx.buf);
> +
> + ph->xops->xfer_put(ph, t);
> + return ret;
...
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE,
> + sizeof(*tx), 0, &t);
This is perfectly one line. Please also check entire code for such an unneeded
wrap.
...
> +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;
> +
> + 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 = devm_kmalloc_array(ph->dev, 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) {
> + devm_kfree(ph->dev, group->group_pins);
This is a red flag. Is this function is solely used at the ->probe() stage?
I do not think so. Why then the devm_*() is being used to begin with?
Also what are the object lifetimes for device here and the _group_info
instances?
> + return ret;
> + }
> +
> + group->present = true;
> + return 0;
> +}
...
> +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + struct scmi_function_info *func)
> +{
As per above.
> +}
...
> +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> + .get_count = scmi_pinctrl_get_count,
> + .get_name = scmi_pinctrl_get_name,
> + .get_group_pins = scmi_pinctrl_get_group_pins,
> + .get_function_groups = scmi_pinctrl_get_function_groups,
> + .set_mux = scmi_pinctrl_set_mux,
> + .get_config = scmi_pinctrl_get_config,
> + .set_config = scmi_pinctrl_set_config,
> + .request_pin = scmi_pinctrl_request_pin,
> + .free_pin = scmi_pinctrl_free_pin
It's not a terminator item, so leave trailing comma. It will reduce the burden
in case of update of this.
> +};
...
> +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> + if (!pinfo)
> + return -ENOMEM;
All the same, why devm_*() is in use and what are the object lifetimes?
> +}
...
> + for (i = 0; i < pi->nr_groups; i++)
> + if (pi->groups[i].present) {
> + devm_kfree(ph->dev, pi->groups[i].group_pins);
> + pi->groups[i].present = false;
> + }
> +
> + for (i = 0; i < pi->nr_functions; i++)
> + if (pi->functions[i].present) {
> + devm_kfree(ph->dev, pi->functions[i].groups);
> + pi->functions[i].present = false;
> + }
Missing outer {}, but see above as well.
...
> +static const struct scmi_protocol scmi_pinctrl = {
> + .id = SCMI_PROTOCOL_PINCTRL,
> + .owner = THIS_MODULE,
This is not needed if you use a trick from ~15 years back...
> + .instance_init = &scmi_pinctrl_protocol_init,
> + .instance_deinit = &scmi_pinctrl_protocol_deinit,
> + .ops = &pinctrl_proto_ops,
> +};
> +
Redundant blank line.
> +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
...i.e. initializing the owner by this macro.
It might require some update to the above macro and its users.
...
> +enum scmi_pinctrl_selector_type {
> + PIN_TYPE = 0,
> + GROUP_TYPE,
> + FUNCTION_TYPE
Leave trailing comma.
> +};
--
With Best Regards,
Andy Shevchenko
