Received-SPF: pass (google.com: domain of linux-kernel+bounces-128088-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Peng Fan <peng.fan@nxp.com>
To: Andy Shevchenko <andriy.shevchenko@intel.com>, "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 <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley
	<conor+dt@kernel.org>, Linus Walleij <linus.walleij@linaro.org>, Dan
 Carpenter <dan.carpenter@linaro.org>, "linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "devicetree@vger.kernel.org"
	<devicetree@vger.kernel.org>, "linux-gpio@vger.kernel.org"
	<linux-gpio@vger.kernel.org>, Oleksii Moisieiev <oleksii_moisieiev@epam.com>
Subject: RE: [PATCH v7 4/4] pinctrl: Implementation of the generic
 scmi-pinctrl driver
Thread-Topic: [PATCH v7 4/4] pinctrl: Implementation of the generic
 scmi-pinctrl driver
Thread-Index: AQHahKODsF690b6vNUGQv4jxirY7Z7FU+GuAgAADvqA=
Date: Tue, 2 Apr 2024 13:59:19 +0000
Message-ID:
 <DU0PR04MB9417D0D33573E99D440D7BD7883E2@DU0PR04MB9417.eurprd04.prod.outlook.com>
References: <20240402-pinctrl-scmi-v7-0-3ea519d12cf7@nxp.com>
 <20240402-pinctrl-scmi-v7-4-3ea519d12cf7@nxp.com>
 <ZgwGpZ6S13vjk8jh@smile.fi.intel.com>
In-Reply-To: <ZgwGpZ6S13vjk8jh@smile.fi.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?Cden5xeGZS3NHXDPnmHQFMNpw3HAI1Kj+nLcPm2XtALWLYQMDsJfPG8YqVoL?=
 =?us-ascii?Q?n5qAXvmUYTcGJCth/M/Vus/tSO+tgARvw81Ag/Zu5jLbnaMsynIup50Bulx5?=
 =?us-ascii?Q?tK8+AgIjbvp/rmvCvASx7VWeVm/vAzVcv+YSoeY0k+Df6DeD1pCz42pdWyXn?=
 =?us-ascii?Q?0PAfAL6EG/JDtw1uhFIkOulpsd4D7nB33+FysXWdL+KKIYV6AbeUBbiPKcZ4?=
 =?us-ascii?Q?BdgqXTdoQWLYO0BnJdpRsRE39+0SJuKX6/N6vI6R3eiAniMt4KEafReZQSv7?=
 =?us-ascii?Q?wskE1CE6bCSJoIUuSy6fCYj1XIra2xtGsPij1CfKxn7HZtDI1kApwQJyqssF?=
 =?us-ascii?Q?MznoogE+WWmY2fo9dlumGPDtHfTxj8hwmihUxs/qYW6xQtw33EIgJ0MN/kmV?=
 =?us-ascii?Q?sE1u7KyYIGq1N0zUL0tpvkhsg51ClwR2S/S4l2zKF3c2BcQrDXJhRR/T4cJL?=
 =?us-ascii?Q?fh36io0BZubTC7NBrlldP7crJEtdSRbe11G1IAkLWHGiU+VYYwZrqpNfLnrR?=
 =?us-ascii?Q?XK3FY8QUhR19/w7O0EIWpN9ozXIAS3cxHpLfK9Qw939FRt7qF3H74sy0HOma?=
 =?us-ascii?Q?5zt5rJT9GlJnq2LFSnSmnIWOJYfOsl77G78d7X9YAG1N9ne8wY0XJ92U+jvn?=
 =?us-ascii?Q?/jz3YIS2FdaP1bEMmiaJSdp6PQADNIJPirZlhEzCRLhwKt6y/Zc6OANZlbX8?=
 =?us-ascii?Q?pOXzjGqL4zq7asU0P0SZlDigajyF+XEaxQ299Hn+KEeBb2o1e5GGkj/CiQE5?=
 =?us-ascii?Q?SMSogzOr8/e2YPwqMZo+63m4MKNgGKp5/h0/Jj/qlthEyan0noYGVcbMeZOb?=
 =?us-ascii?Q?uiaNUvTriHr542DUA2KtRUYeqHesfuvTA4e4vmkGotCmE37waJ/8vkz9+ckR?=
 =?us-ascii?Q?w2oBEeuCxjRWzH3uhHna4sJFlfNgUC9hFWwjcLIJ85itupdRBdvCtleAghkh?=
 =?us-ascii?Q?IhqTcwUin56Ftl/DEI43747Y9ZAtFChoN8gq1BWsDihwODl1knptFsiGRZsT?=
 =?us-ascii?Q?dwJ2qsi7Aw6GiGFZU/DyXsPxgvuKkf9DZZZ3qgpnq88qhAnsR/hFDjQzfJxm?=
 =?us-ascii?Q?o5v93VTXNRVeT5vAm4xDgUqo/ClUHDai2qrHkY4hXLuLLXTNE8ytzsU286a5?=
 =?us-ascii?Q?Tx8D9MxtvEOZfh+b/8hgnbb/uW1cSX78WgClx9/4B+ybSdq7I114jT+mK1NL?=
 =?us-ascii?Q?X7rdS0Ou605rwXhvZ1ZIDtTRShlVMhjAUl2CkSxO2FShxbTDtB5RYCB/Lou8?=
 =?us-ascii?Q?JxZ5EQhrJGobGP9vOIdzc955zUIvrq53nk2V14iBroXTLT5V0FaTkYFacQSa?=
 =?us-ascii?Q?YT0cDMLixuTMwQ+00VH+HrplEBntqPDukjskj+gRqQvsLOxb6Hj6fgW60TKW?=
 =?us-ascii?Q?CGr6VPQ8fccMVppXDmaYybEkqcwkrGr3+/PrPPi1fOsWOdvecsjHsYx/HqsX?=
 =?us-ascii?Q?egl3YdhYn1diozYJBSdZOHVArJChHHGU+v1gmEeICXSgcER+hzmsbk90xfuM?=
 =?us-ascii?Q?2lA5LGl574SByrZ8TuQ7o0Rut9gMRraLw2jskqxNSQyUdQYck23mqvV+/OBR?=
 =?us-ascii?Q?rMDWbjC8WKy4etBR/Mw=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: 8f9f9579-a1ba-4926-3c76-08dc531d17d9
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Apr 2024 13:59:19.8427
 (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: qNoKrkfvjORxmn7U0JbRSEUG5obA9V+Wdx7qMg49bQ0IIn3zDbWC5klntBYkqrDpGBkwyM0wViaSU3wtqjrm3A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR04MB9145

> Subject: Re: [PATCH v7 4/4] pinctrl: Implementation of the generic scmi-
> pinctrl driver
>=20
> On Tue, Apr 02, 2024 at 10:22:24AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > scmi-pinctrl driver implements pinctrl driver interface and using SCMI
> > protocol to redirect messages from pinctrl subsystem SDK to SCMI
> > platform firmware, which does the changes in HW.
>=20
> ...
>=20
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/module.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
>=20
> Missing headers.

Not sure there is an easy way to filter out what is missed.

>=20
> ...
>=20
> > +	*p_groups =3D (const char * const *)func->groups;
>=20
> Is this casting needed?

This is no needed.

>=20
> ...
>=20
> > +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
> > +				    unsigned int _pin, unsigned long *config)
>=20
> Why underscored parameter name?

Underscore could be dropped.

>=20
> ...
>=20
> > +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
> > +				 struct pinctrl_desc *desc)
> > +{
> > +	struct pinctrl_pin_desc *pins;
> > +	unsigned int npins;
> > +	int ret, i;
> > +
> > +	npins =3D pinctrl_ops->count_get(pmx->ph, PIN_TYPE);
> > +	/*
> > +	 * npins will never be zero, the scmi pinctrl driver has bailed out
> > +	 * if npins is zero.
> > +	 */
>=20
> This is fragile, but at least it is documented.
>=20
> > +	pins =3D devm_kmalloc_array(pmx->dev, npins, sizeof(*pins),
> GFP_KERNEL);
> > +	if (!pins)
> > +		return -ENOMEM;
> > +
> > +	for (i =3D 0; i < npins; i++) {
> > +		pins[i].number =3D i;
> > +		ret =3D pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE,
> &pins[i].name);
> > +		if (ret)
>=20
> How does the cleanup work for the previously assigned pin names? Is it
> needed?

No need. The "name" memory region is allocated in firmware pinctrl
Protocol init phase.

> Maybe a comment?

ok.  As below.
/*
 * The region for name is handled by the scmi firmware driver,=20
 * no need free here
*/

>=20
> > +			return dev_err_probe(pmx->dev, ret,
> > +					     "Can't get name for pin %d", i);
> > +	}
> > +
> > +	desc->npins =3D npins;
> > +	desc->pins =3D pins;
> > +	dev_dbg(pmx->dev, "got pins %u", npins);
> > +
> > +	return 0;
> > +}
>=20
> ...
>=20
> > +static const struct scmi_device_id scmi_id_table[] =3D {
> > +	{ SCMI_PROTOCOL_PINCTRL, "pinctrl" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
>=20
> Move this closer to the user.

ok. Fix in v8.

Thanks,
Peng.
>=20
> --
> With Best Regards,
> Andy Shevchenko
>=20