Received-SPF: pass (google.com: domain of linux-kernel+bounces-161418-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Peng Fan <peng.fan@nxp.com>
To: Dan Carpenter <dan.carpenter@linaro.org>, "Peng Fan (OSS)"
	<peng.fan@oss.nxp.com>
CC: Sudeep Holla <sudeep.holla@arm.com>, Cristian Marussi
	<cristian.marussi@arm.com>, Rob Herring <robh@kernel.org>, Krzysztof
 Kozlowski <krzk+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>, Shawn Guo
	<shawnguo@kernel.org>, Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
 Kernel Team <kernel@pengutronix.de>, Fabio Estevam <festevam@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>, Aisheng Dong
	<aisheng.dong@nxp.com>, Jacky Bai <ping.bai@nxp.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>, "devicetree@vger.kernel.org"
	<devicetree@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "imx@lists.linux.dev" <imx@lists.linux.dev>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>
Subject: RE: [PATCH v3 6/6] pinctrl: imx: support SCMI pinctrl protocol for
 i.MX95
Thread-Topic: [PATCH v3 6/6] pinctrl: imx: support SCMI pinctrl protocol for
 i.MX95
Thread-Index: AQHamSj/KWNiz3mJO0CVZfKqQ8uNkbF9l/eAgAAAZuA=
Date: Sun, 28 Apr 2024 12:21:35 +0000
Message-ID:
 <DU0PR04MB94171C7DE932EB8EFE0CDCA488142@DU0PR04MB9417.eurprd04.prod.outlook.com>
References: <20240428-pinctrl-scmi-oem-v3-v3-0-eda341eb47ed@nxp.com>
 <20240428-pinctrl-scmi-oem-v3-v3-6-eda341eb47ed@nxp.com>
 <1bc4d494-f4c0-4bec-a2de-e9971ef4be49@moroto.mountain>
In-Reply-To: <1bc4d494-f4c0-4bec-a2de-e9971ef4be49@moroto.mountain>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?d/tEm2nWPivtxwKA9lUeX+kpk4VNlyiBVux+oyUfOjm8QgDbQxhOcUFmv2UX?=
 =?us-ascii?Q?/NTLRL562+sMFQEB/4WKbEuCzfL0SkGIfG+OuU2SSS56nxs48XzkQ2hCipzB?=
 =?us-ascii?Q?PEUVA3RSNid+TNBnvuFDvFuPjK5qwrrOnijuMZuRfGgfNSzu1iY5vE23Dglw?=
 =?us-ascii?Q?mTSosrbL51wkeDVhP/HOXZ9qPh02MSEJO3TDJoPuYi3sloAfOZfCZGx2WCje?=
 =?us-ascii?Q?hYv9YkOC8UmKMRDB9Vd2rRaPJH/GnsspLtdG8RopS650wd01YOkfUduODLWG?=
 =?us-ascii?Q?Qnhb3soR4mF96Xbpfy3KqVoXnttS5D64XGBwRRkEcrWLOEuW4hjqhYyMcRjY?=
 =?us-ascii?Q?SIAr3IN5pRzQfdkgMy5YompABh1dBAqbIZOT7kwxQ1xmIHDOI2bMSn7BlWLL?=
 =?us-ascii?Q?NWSFBZ1MS9LMmZDAJgOjNCY81k84rDWbhXooboT9PRI4nzH+gi18JFTFU6qj?=
 =?us-ascii?Q?1pgrEsjMlbqq7p7kNgWgE28TLEi28CFh5bfji4H2fCI1jHZGG9Gvkou4dulR?=
 =?us-ascii?Q?svorCE4cAng3M4H3C89P5pfDoPKy07K+gBMhJKmX1jKBfyQwKjGWzDZY3mVV?=
 =?us-ascii?Q?aMhFMIbZHxN+h0KylH+EkUWUAa7xwMrjvE4FOHqClUKT224EKZu69s7JuX3c?=
 =?us-ascii?Q?TPEpe1WFoLbsyj8ufcy0W5CETrMpGpi4w7h+0pH4EyyIapLhCdufUMpT3cex?=
 =?us-ascii?Q?IHX6aNNFZxinm+lssmZ69DUY7weUy+B36o3rfOv9Ee0smveVKFxzwmLGoIG4?=
 =?us-ascii?Q?fCkm5fQjDQb6arrNPEXib0k2EOKpcTbwEXLtVD32jSwRsPw1u7/mtD6L2/wR?=
 =?us-ascii?Q?xntqb5PK88TPRGPhL6JQiFQKL4tImHfXffylVOeKO0v+5KonklhoagrMVtH4?=
 =?us-ascii?Q?YactwfzQRlK+L237WQSt2P6CE50PIwnldh2H8uGAGrR38bEX3ePvtk6RiVBo?=
 =?us-ascii?Q?FGDRj9TVKoDr8wj17ycxmDp8o/gztYrK9V15JuuPRKNMTWcMcopM36onlzkz?=
 =?us-ascii?Q?IVUXjVJOeQTmPx2ecmOBOXewfNrOZjQNVVFo0DU0OIVYwFzVGPbZvEGY/+5l?=
 =?us-ascii?Q?D1Atxsxw+2ymWBnxSOZBqwp/rU7mK/1USQBTIxMVdAVnXk7LbXrHc9AAHn0E?=
 =?us-ascii?Q?Y8NhDi6DUhjxfzF/b9Lubep7te232E+1wb+J0Lx6VEJdjp4YzO5jaLZTrWp5?=
 =?us-ascii?Q?ZiXBL4irsxusvKDBssXJ34FcdYn7IYfMEwvR1pIdK8AvVmEpHmUhPNB4blfi?=
 =?us-ascii?Q?gv+lpXtskyx9UdRTqfWjO7zU1Y9aHFao/A9NpMNzSkRZSKJZGDqkz6+jOp0Y?=
 =?us-ascii?Q?HXC6PNp98Ne1WvenELrMKHJhEbPmrsjU99l7yHMWdIgzFkEFNP2YGIARYsDx?=
 =?us-ascii?Q?1tKgffvtL0QbJPIu40AH4D/CG7BZ/OxJw3mkjEGd1dbhVHLFBd9sVov9zsit?=
 =?us-ascii?Q?PPG2JhaJsaPOISowOrxcbFtQsWmBTDnUbE5HVLWV8TF0LKyCXcm4YuUIpibv?=
 =?us-ascii?Q?7huQriXmQYR26jFvSTz5NdIFvfqIgjkP7wWQaa7lxpmt+Sgs4bwmG7fHk4mI?=
 =?us-ascii?Q?tGA09UzXs0w+oCetMEs=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DU0PR04MB9417.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 68dd09e5-8f03-495d-a738-08dc677dbf58
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Apr 2024 12:21:35.8446
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 9R5CM3MshYwxt2NZJpzs27bxfvQ4J1JRTIpGRfd+zEd0XMQnF73zhuYBK+tRnxe7gUoS9w0lOszc9csy3zOvcw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB8PR04MB7018

Hi Dan,

> Subject: Re: [PATCH v3 6/6] pinctrl: imx: support SCMI pinctrl protocol f=
or
> i.MX95
>=20
> 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 =3D pctldev->dev;
> > +	unsigned long *cfgs =3D NULL;
> > +	unsigned int n_cfgs, reserve =3D 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 =3D 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;
>=20
> 	return n_pins;

Yeah.  Fix in v4.

>=20
> > +	} else if (!n_pins) {
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret =3D 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 =3D 0;
> > +	for (i =3D 0; i < n_cfgs; i++) {
> > +		param =3D pinconf_to_config_param(cfgs[i]);
> > +		ret =3D pinctrl_scmi_imx_map_pinconf_type(param, &mask,
> &shift);
> > +		if (ret) {
> > +			dev_err(dev, "Error map pinconf_type %d\n", ret);
> > +			return ret;
>=20
> This should be goto free_cfgs.

Good catch. Fix in v4.

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

..
> > +
> > +{
> > +	unsigned int reserved_maps;
> > +	struct device_node *np;
> > +	int ret =3D 0;
> > +
> > +	reserved_maps =3D 0;
> > +	*map =3D NULL;
> > +	*num_maps =3D 0;
> > +
> > +	for_each_available_child_of_node(np_config, np) {
>=20
> 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.

>=20
> > +		ret =3D 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);
>=20
> You could get rid of the calls to of_node_put().  I would move the call t=
o
> pinctrl_utils_free_map() here.

ok. Update in v4.

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

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

Thanks,
Peng.

>=20
> > +}
>=20
> regards,
> dan carpenter