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

[Peng Fan <peng.fan@xxxxxxx>]

Hi Andy,

> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
> 
> 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).

Is there any documentation about this requirement?
Some headers are already included by others.

> 
> ...
> 
> > +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.

Per Cristian, scmi drivers keep 80 max chars.

> 
> > +	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?

The type is enum, not u32.

> 
> > +					    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?

Scmi drivers use 80 max drivers.
> 
> ...
> 
> > +	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.

ok. Fix in v7.
> 
> > +	} 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?

It could be removed.

> 
> > +	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.

Please teach me if this is wrong.

> 
> > +	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.

Yeah.

> 
> > +			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.

Fix in v7

Thanks,
Peng.
> 
> > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
> 
> 
> --
> With Best Regards,
> Andy Shevchenko
>