linux-kernel - RE: [PATCH v3 6/6] pinctrl: imx: support SCMI pinctrl protocol for i.MX95

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <DU0PR04MB94171C7DE932EB8EFE0CDCA488142@DU0PR04MB9417.eurprd04.prod.outlook.com>
Date: Sun, 28 Apr 2024 12:21:35 +0000
From: Peng Fan <peng.fan@....com>
To: Dan Carpenter <dan.carpenter@...aro.org>, "Peng Fan (OSS)"
	<peng.fan@....nxp.com>
CC: Sudeep Holla <sudeep.holla@....com>, Cristian Marussi
	<cristian.marussi@....com>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Shawn Guo
	<shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix
 Kernel Team <kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>,
	Linus Walleij <linus.walleij@...aro.org>, Aisheng Dong
	<aisheng.dong@....com>, Jacky Bai <ping.bai@....com>,
	"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>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>
Subject: RE: [PATCH v3 6/6] pinctrl: imx: support SCMI pinctrl protocol for
 i.MX95

Hi Dan,

> Subject: Re: [PATCH v3 6/6] pinctrl: imx: support SCMI pinctrl protocol for
> i.MX95
> 
> On Sun, Apr 28, 2024 at 01:07:52PM +0800, Peng Fan (OSS) wrote:
> > +static int pinctrl_scmi_imx_dt_group_node_to_map(struct pinctrl_dev
> *pctldev,
> > +						 struct device_node *np,
> > +						 struct pinctrl_map **map,
> > +						 unsigned int
> *reserved_maps,
> > +						 unsigned int *num_maps,
> > +						 const char *func_name)
> > +{
> > +	struct device *dev = pctldev->dev;
> > +	unsigned long *cfgs = NULL;
> > +	unsigned int n_cfgs, reserve = 1;
> > +	int i, n_pins, ret;
> > +	u32 ncfg, val, mask, shift, pin_conf, pinmux_group;
> > +	unsigned long cfg[IMX_SCMI_NUM_CFG];
> > +	enum pin_config_param param;
> > +	struct property *prop;
> > +	const __be32 *p;
> > +
> > +	n_pins = of_property_count_u32_elems(np, "pinmux");
> > +	if (n_pins < 0) {
> > +		dev_warn(dev, "Can't find 'pinmux' property in
> node %pOFn\n", np);
> > +		return -EINVAL;
> 
> 	return n_pins;

Yeah.  Fix in v4.

> 
> > +	} else if (!n_pins) {
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = pinconf_generic_parse_dt_config(np, pctldev, &cfgs, &n_cfgs);
> > +	if (ret) {
> > +		dev_err(dev, "%pOF: could not parse node property\n", np);
> > +		return ret;
> > +	}
> > +
> > +	pin_conf = 0;
> > +	for (i = 0; i < n_cfgs; i++) {
> > +		param = pinconf_to_config_param(cfgs[i]);
> > +		ret = pinctrl_scmi_imx_map_pinconf_type(param, &mask,
> &shift);
> > +		if (ret) {
> > +			dev_err(dev, "Error map pinconf_type %d\n", ret);
> > +			return ret;
> 
> This should be goto free_cfgs.

Good catch. Fix in v4.

> 
> > +		}
> > +
> > +		val = pinconf_to_config_argument(cfgs[i]);
> > +

..
> > +
> > +{
> > +	unsigned int reserved_maps;
> > +	struct device_node *np;
> > +	int ret = 0;
> > +
> > +	reserved_maps = 0;
> > +	*map = NULL;
> > +	*num_maps = 0;
> > +
> > +	for_each_available_child_of_node(np_config, np) {
> 
> Btw, if you used the scoped version of these loops such as
> for_each_available_child_of_node_scoped(), then

Yeah, it will simply code.

> 
> > +		ret = pinctrl_scmi_imx_dt_group_node_to_map(pctldev, np,
> map,
> > +
> &reserved_maps,
> > +							    num_maps,
> > +							    np_config-
> >name);
> > +		if (ret < 0) {
> > +			of_node_put(np);
> 
> You could get rid of the calls to of_node_put().  I would move the call to
> pinctrl_utils_free_map() here.

ok. Update in v4.

> 
> 		if (ret) {
> 			pinctrl_utils_free_map(pctldev, *map, *num_maps);
> 			return ret;
> 		}
> 
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (ret)
> > +		pinctrl_utils_free_map(pctldev, *map, *num_maps);
> > +
> > +	return ret;
> 
> 	return 0;

Thanks for your quick response, I will wait for more reviews, then post v4.

Thanks,
Peng.

> 
> > +}
> 
> regards,
> dan carpenter