Received-SPF: pass (google.com: domain of linux-kernel+bounces-177386-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Peng Fan <peng.fan@nxp.com>
To: Andy Shevchenko <andy.shevchenko@gmail.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 <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 v4 3/3] pinctrl: imx: support SCMI pinctrl protocol for
 i.MX95
Thread-Topic: [PATCH v4 3/3] pinctrl: imx: support SCMI pinctrl protocol for
 i.MX95
Thread-Index: AQHanp3bntBZVxVwuE+ylwKb1N1Nl7GSddeAgAJ1tqA=
Date: Mon, 13 May 2024 09:09:04 +0000
Message-ID:
 <DU0PR04MB94179C67B46BD5F644FC2DF288E22@DU0PR04MB9417.eurprd04.prod.outlook.com>
References: <20240505-pinctrl-scmi-oem-v3-v4-0-7c99f989e9ba@nxp.com>
 <20240505-pinctrl-scmi-oem-v3-v4-3-7c99f989e9ba@nxp.com>
 <Zj_HFxHMV57EXfYm@surfacebook.localdomain>
In-Reply-To: <Zj_HFxHMV57EXfYm@surfacebook.localdomain>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?tNlkMVamggzyqeJAzn6bMotHuwRVwIEP6umzlfOjDqcKPwbQgCHKMm2VTHQH?=
 =?us-ascii?Q?8hEdjCrdxbqjLxFQy6BO8RPjFSaLLRnEiGxiScgWjF/nDDcT4Gx2g+IG97wL?=
 =?us-ascii?Q?Ej6zPPKKBRLs4V/wYQ7nrV+xwL6yUWDx+Aq8hLhjm7FP7MYQcnW1qkj6kvSW?=
 =?us-ascii?Q?LDH1E+meHuyAADOCH9eOjBa5ShqPBPL+qRZB0I4Mz8YWCTIMAKeeT8Gpc3xo?=
 =?us-ascii?Q?5f3t7igeGeR+t4NZ/sFManZy4b8B/r0NhttYWSFFP1b/hnOrM6cWEZqPQ+eo?=
 =?us-ascii?Q?iGa9IXqWYE3fPVzTT4CRQ/3KwCfJKBUi065on+fyXftwxM054IIx4boJ9vSQ?=
 =?us-ascii?Q?ShXptkYtRj3PQS7ev7TI3AYjTAFhX1iZAnP9ESusib4ZS7MvdZBa8jfrgXZx?=
 =?us-ascii?Q?w/sX9DRJvsLZZiZr4JSLBlzhgP3cTttQnBooNkklWn5D22NSN4Bbq6bhw0GS?=
 =?us-ascii?Q?gKPUDfUWMCVbu9a8+4xLB+HbShbBERUFF4esyJs3EhlaC7xBdGwGLQcS+rL1?=
 =?us-ascii?Q?2ZjlCBdR2wTZW7wZAKCRuV9gXdg1ZhnZkBKDx9pzkCOdAAMhoH/OppE6CHUS?=
 =?us-ascii?Q?zqvX+fjXPNNrNdTjaU3EFAqqJMFfkodgtqCrsqIQq2uGLOA6miseeAivQfCE?=
 =?us-ascii?Q?jb+LmSmAXbikZXHT1QtewyeoP7GxGfdUypPzKDpBlGZ1r4AUqxpUiDuIvxHU?=
 =?us-ascii?Q?ysf2rlqCHIZiOzVCQoRJWI5qdXA10DdV3ijEDFNz/5WMUd6Hm96fN2+4WOAy?=
 =?us-ascii?Q?jCL/O8VFnUKvuiHJzeP0sKWAVcs9GtG20nxQcZpSEIldC+dP5lnbg0lh4Fbr?=
 =?us-ascii?Q?8UuDy3khmU62FVnQfmln1r3X6dKdZvqAaIkQgid/77HfwH8cvcZB1gQFeIfJ?=
 =?us-ascii?Q?qbq/uuV/MszQn1zjAupxrZHI6lqD0feocv+OlJaQfK4pf/vJ+L7DUahyt3H6?=
 =?us-ascii?Q?LVIySZJG+k8K0BwXdR85cJ7pVRTzfE23YrjFbMct5esQxYot+cKbnUCybbs2?=
 =?us-ascii?Q?BcAMe0LZ85ruDQNAum7WzAeD51VTPe1SsuAFtanl9tX+D50z8PTeCveiqpCc?=
 =?us-ascii?Q?UeQQ67Y3M9lDQ4D8uMfcE2GHRcnr04X1vfRjYiAUDdD2nMwDVHP0x/w1xtIz?=
 =?us-ascii?Q?wZFkjxqyQJvKnAEtRtraTPk1oVZsk1Ghl/bR7Rdm2WzdYItTl/USEWBgi4la?=
 =?us-ascii?Q?GHUFpU5vLDnZyz5Tuk0Mm9HaKrd4zv1Hf/snAbK7cmLAL9OwmF5/RCJ1yDo0?=
 =?us-ascii?Q?woJNuzGxloGwuPoD/w28gRMDkoCWOHbH3IL3eSWxgIOuQgCeb/NMo9MNnTVE?=
 =?us-ascii?Q?/fi2wa89FmuTqPJAWBznIdwc7JXuVGcZFzPxUvwY3yYJd0AB9pdyD7+Qf/Ec?=
 =?us-ascii?Q?9E1dGBgCHQdQY2ZIOrHQ+jmxQfSdJhy2k2veYIBYkEiXJMZTZVBVBwjeqCD5?=
 =?us-ascii?Q?/wF2x4zLseJ7Wycs+5xC6Cp/oXXkrugb2CcHefc71lzHJSrOQfL1Oqc1XLy2?=
 =?us-ascii?Q?PaS9dmElt062y2rFErkBHlvf19lm8HqX9ySZ/ocTBFWfYRPnJghtUSNMm1DS?=
 =?us-ascii?Q?NKIZQ3OLCIPW9vVJ+/M=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: 9ab95370-99d8-4d43-e4ac-08dc732c5638
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 May 2024 09:09:04.1755
 (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: 4SUnrGXD0OvIm+RcpXawHjeDgf1LXaIlXCHHNNWP3tKjFiMSY+DPLErrzXUzjt99S7KyEZef2TWCSx0mOB767A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PA1PR04MB10206

> Subject: Re: [PATCH v4 3/3] pinctrl: imx: support SCMI pinctrl protocol f=
or
> i.MX95
>=20
> Sun, May 05, 2024 at 11:47:19AM +0800, Peng Fan (OSS) kirjoitti:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The generic pinctrl-scmi.c driver could not be used for i.MX95 because
> > i.MX95 SCMI firmware not supports functions, groups or generic 'Pin
> > Configuration Type and Enumerations' listed in SCMI Specification.
> >
> > i.MX95 System Control Management Interface(SCMI) firmware only
> > supports below pin configuration types which are OEM specific types:
> >     192: PIN MUX
> >     193: PIN CONF
> >     194: DAISY ID
> >     195: DAISY VAL
> >
> > To support Linux generic pinctrl properties(pinmux, bias-pull-[up,
> > down], and etc), need extract the value from the property and map them
> > to the format that i.MX95 SCMI pinctrl protocol understands, so add
> > this driver.
>=20

Thanks for refining this.

> ...
>=20
> > +struct imx_pin_group {
> > +	struct pingroup data;
> > +};
>=20
> I don't see the necessity of having this wrapper structure. Can't you sim=
ply
> use struct pingroup directly?

Yeah. Let me drop the wrapper in v6.

>=20
> ...
>=20
> > +static int scmi_pinctrl_imx_probe(struct scmi_device *sdev) {
> > +	int ret;
> > +	struct device *dev =3D &sdev->dev;
> > +	struct scmi_pinctrl_imx *pmx;
> > +	const struct scmi_handle *handle;
> > +	struct scmi_protocol_handle *ph;
> > +	struct device_node *np __free(device_node) =3D
> of_find_node_by_path("/");
> > +	const struct scmi_pinctrl_proto_ops *pinctrl_ops;
>=20
> > +	if (!sdev->handle)
> > +		return -EINVAL;
>=20
> When this conditional can be true?

Just follow what other SCMI drivers do.
scmi_set_handle will get handles, per the code, there
might be NULL.

>=20
> > +	if (!of_match_node(scmi_pinctrl_imx_allowlist, np))
> > +		return -ENODEV;
>=20
> > +	handle =3D sdev->handle;
>=20
> It's even better to assign first and then check if the above check is nee=
ded at
> all.

Yeah. Update in v6.

>=20
> > +	pinctrl_ops =3D handle->devm_protocol_get(sdev,
> SCMI_PROTOCOL_PINCTRL, &ph);
> > +	if (IS_ERR(pinctrl_ops))
> > +		return PTR_ERR(pinctrl_ops);
> > +
> > +	pmx =3D devm_kzalloc(dev, sizeof(*pmx), GFP_KERNEL);
> > +	if (!pmx)
> > +		return -ENOMEM;
> > +
> > +	pmx->ph =3D ph;
> > +	pmx->ops =3D pinctrl_ops;
> > +
> > +	pmx->dev =3D dev;
> > +	pmx->pctl_desc.name =3D DRV_NAME;
> > +	pmx->pctl_desc.owner =3D THIS_MODULE;
> > +	pmx->pctl_desc.pctlops =3D &pinctrl_scmi_imx_pinctrl_ops;
> > +	pmx->pctl_desc.pmxops =3D &pinctrl_scmi_imx_pinmux_ops;
> > +	pmx->pctl_desc.confops =3D &pinctrl_scmi_imx_pinconf_ops;
> > +
> > +	ret =3D scmi_pinctrl_imx_get_pins(pmx, &pmx->pctl_desc);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret =3D scmi_pinctrl_imx_probe_dt(sdev, pmx);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret =3D devm_pinctrl_register_and_init(dev, &pmx->pctl_desc, pmx,
> > +					     &pmx->pctldev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to register pinctrl\n");
> > +
> > +	return pinctrl_enable(pmx->pctldev); }

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