Message-ID: <20230706144949.GA830797@EPUAKYIW0A6A>
Date:   Thu, 6 Jul 2023 14:49:51 +0000
From:   Oleksii Moisieiev <Oleksii_Moisieiev@...m.com>
To:     "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>
CC:     "sudeep.holla@....com" <sudeep.holla@....com>,
        Cristian Marussi <cristian.marussi@....com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v3 2/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
 protocol basic support

Hi Andy,

On Wed, Jun 07, 2023 at 10:10:44AM +0300, andy.shevchenko@...il.com wrote:
> 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.
>
Fixed. Thanks,

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

My opinion is that we don't want to make SCMI Pinctrl protocol to be
dependent from Pinctrl subsystem. This will potentially require SCMI
protocol change in case of significant Pinctrl subsystem refactoring.
Another reason is that pincfunction has the following groups
definition:
const char * const *groups;

Which is meant to be constantly allocated.
So I when I try to gather list of groups in
pinctrl_scmi_get_function_groups I will receive compilation error.

Pinctrl subsystem structs are designed to be statically allocated and
can't be used for lazy allocations.

> ...
> 
> > +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.
> 
Thanks, I will go through the code and fix this.
> ...
> 
> > +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.
> 
Ok, thanks.
> > +};
> 
> ...
> 
> > +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?
> 
Ok, thanks. 
> > +}
> 
> ...
> 
> > +	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.
> 

Checked linux kernel code with the following command:
pcregrep -lcrM "for \(.*\)[^{]$\n.*if" .

As I can see - this approach is taking place in kernel code. Although
not as widely as for with outer {}. I will add outer {}.


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

Thanks,
Oleksii.

Powered by blists - more mailing lists