Received-SPF: pass (google.com: domain of linux-kernel+bounces-126075-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
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 <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 v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
 protocol basic support
Thread-Topic: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
 protocol basic support
Thread-Index: AQHafRqyofu9Mu9OLUiaociI+9q/i7FPHJwAgALIoZA=
Date: Sun, 31 Mar 2024 13:44:28 +0000
Message-ID:
 <DU0PR04MB9417E797F4E0F7BB6154B3BE88382@DU0PR04MB9417.eurprd04.prod.outlook.com>
References: <20240323-pinctrl-scmi-v6-0-a895243257c0@nxp.com>
 <20240323-pinctrl-scmi-v6-3-a895243257c0@nxp.com>
 <ZgcP4IkTQGks9ehH@surfacebook.localdomain>
In-Reply-To: <ZgcP4IkTQGks9ehH@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?3tJmb9uuw/kAwHnATHfVL2uBTujZIn32XOZMdY5/NjjHY5VS9/r6qDEuh9S5?=
 =?us-ascii?Q?T9Je8bVkXUXuZfA4pf/ONLNTgzDczlxyKx3em/rVHJCYp7nQFCwr1zcnspcx?=
 =?us-ascii?Q?utrvIOJpmI1pF+za+MX5oDioGJT9zXZhqYmV3tSzoc8DuQZbR+ynsmxWeOg9?=
 =?us-ascii?Q?egf63+rjmAxEN4jH/YqkhilNCpwsLDwG5gyiNEq4s2Uk8C/7T19uArPBOZei?=
 =?us-ascii?Q?H2tT9SdS5OIyt/d9lV2BOTAx6QU3HTDUEAUq1PPfGgi5tG4Uam4nDhGxWDE9?=
 =?us-ascii?Q?o7xWkXCcci8w3yuJVdBw+pXGKdt3W8ZZDApYmZuboXSxwZWEIGyTwzRCB/rK?=
 =?us-ascii?Q?pTbPPWoxoOA8DaUUp23DSonf85GOJSurJyhVl/0fWmwhyrBNotlDpDzJnG9Z?=
 =?us-ascii?Q?JV1o+Hw334OnYcuGxp2tnZi3/7u3CqyrhBdwitI4Fjs589lkd4XJek/+2irs?=
 =?us-ascii?Q?gxHoCTVBO1x+DoRX1x5dE+BCE+LM4qtprYVCExHA903JGZewyyWmZqBJkGdR?=
 =?us-ascii?Q?r/yce+j8pxhhIXozwUpvSVk+quhFThraCv9xXDR87GKfKIpnancIb4mFdyx3?=
 =?us-ascii?Q?1PwQiKkOL8S6o0xML3AehMSHPR1wh8esyB3M24g4ZZP8AfCZrC4DsTfIsaaZ?=
 =?us-ascii?Q?SQlAo2FBH4II1bSX0NK0tM+LH0PiCk6TYNbGEKwmpmWNANPtDKqFYVnMjcJp?=
 =?us-ascii?Q?AilpwKC9UFoaq3ISJkmBWJFNUAcFEVIB21d6fz9Iimn+l5AbSYHaKFZaxiu3?=
 =?us-ascii?Q?B8YOU0c3qs9hTi/a52oJM+4rMbjNDkj8twPRDufwlI0TMAnMYRFRaCYJ3tNk?=
 =?us-ascii?Q?WujUU0VOdOamFJfG4TBB2YOk8vKrAI5wGpzjucncWlWoRj9EIRV7Xwak+UwQ?=
 =?us-ascii?Q?gxdXt+wTMmUg70qoE0R8OmlVDKYwEYXXuu2i+SybJBuqxirSdlcB6xJTZZrT?=
 =?us-ascii?Q?6jE2YR63bEXPe6L+Fw6oH7djvlRyS1YJVMNYFBIdnmI5+YxG122uQ7kT6Zx4?=
 =?us-ascii?Q?SBHdw3Xtihuk9sRnj+5AHZ8A4VFqKNLSWwiaUVK5gic0b7/szT8+QynxMCe5?=
 =?us-ascii?Q?CbX53PuWwPDFFYD7oNP4BFWb/oj+BGMn4MOC9TywXPxO24xBymhSMLcMzze1?=
 =?us-ascii?Q?6dEtfg8ynLO4hiI+erna9y1TebOtL1VgEeeEUvn9mYeiTjyv9cXpL+YrwAfG?=
 =?us-ascii?Q?bCcOxW09qTYKD7TuWSD2lliXw6eI/aKbfcRCwmv7MTEm6ph1dKeytqF5D+gj?=
 =?us-ascii?Q?57tqd3nCia/KEUu7qfdFm28lHU/XCTo6LCLJuQHUrH1wH8h5uIPnB06h0xEg?=
 =?us-ascii?Q?aD94dzV11oE80TXtdGy5StJWh6Ks3lRVmJRY5cPF6efmNWQlZEuAUwRfs+Y6?=
 =?us-ascii?Q?vIrMl4Z33Jeb/OooejhXYzSdK5ogZ3GFk8TWt3C0nG6hRQ5UTLbOIrjVy8Nx?=
 =?us-ascii?Q?GGnVcyx47NlSSPm1MTvjtTBlre2ebJG1CuZU5em2vp7yIug2zEc8DvHQGLDN?=
 =?us-ascii?Q?MjC7oVDoTbBM50zCq9fm/4HojHj/Bf9GRsdBmtutZOWPgfmct0eBxqyvbqCU?=
 =?us-ascii?Q?ysr+fq1KnEQXTGCqY3I=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: da8e3ba2-5182-4654-4ee5-08dc5188afce
X-MS-Exchange-CrossTenant-originalarrivaltime: 31 Mar 2024 13:44:28.6260
 (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: SWiSltzjuYFb0YxiirFaf0n4m6Wz9O0as9oNQt0X/aH8idUaAlZAzfv0XLB86MNabddtIcn9zda1jQXFioClEg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB8961

Hi Andy,

> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>=20
> Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add basic implementation of the SCMI v3.2 pincontrol protocol.
>=20
> ...
>=20
> >  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) +=3D virtio.o
> >  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) +=3D optee.o
> > scmi-protocols-y =3D base.o clock.o perf.o power.o reset.o sensors.o
> > system.o voltage.o powercap.o
>=20
> Actually you want to have :=3D here.
>=20
> > +scmi-protocols-y +=3D pinctrl.o
>=20
>=20
>=20
> >  scmi-module-objs :=3D $(scmi-driver-y) $(scmi-protocols-y)
> > $(scmi-transport-y)
>=20
> Side note: The -objs has to be -y
>=20
> ...
>=20
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
>=20
> This is semi-random list of headers. Please, follow IWYU principle (inclu=
de
> what you use). There are a lot of inclusions I see missing (just in the c=
ontext of
> this page I see bits.h, types.h, and  asm/byteorder.h).

Is there any documentation about this requirement?
Some headers are already included by others.

>=20
> ...
>=20
> > +enum scmi_pinctrl_protocol_cmd {
> > +	PINCTRL_ATTRIBUTES =3D 0x3,
> > +	PINCTRL_LIST_ASSOCIATIONS =3D 0x4,
> > +	PINCTRL_SETTINGS_GET =3D 0x5,
> > +	PINCTRL_SETTINGS_CONFIGURE =3D 0x6,
> > +	PINCTRL_REQUEST =3D 0x7,
> > +	PINCTRL_RELEASE =3D 0x8,
> > +	PINCTRL_NAME_GET =3D 0x9,
> > +	PINCTRL_SET_PERMISSIONS =3D 0xa
>=20
> Leave trailing comma as it's not a termination.
>=20
> > +};
>=20
> ...
>=20
> > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_hand=
le
> *ph,
> > +				       struct scmi_pinctrl_info *pi) {
> > +	int ret;
> > +	struct scmi_xfer *t;
> > +	struct scmi_msg_pinctrl_protocol_attributes *attr;
> > +
> > +	ret =3D ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
> sizeof(*attr),
> > +				      &t);
>=20
> This looks much better on a single line.

Per Cristian, scmi drivers keep 80 max chars.

>=20
> > +	if (ret)
> > +		return ret;
> > +
> > +	attr =3D t->rx.buf;
> > +
> > +	ret =3D ph->xops->do_xfer(ph, t);
> > +	if (!ret) {
> > +		pi->nr_functions =3D GET_FUNCTIONS_NR(attr-
> >attributes_high);
> > +		pi->nr_groups =3D GET_GROUPS_NR(attr->attributes_low);
> > +		pi->nr_pins =3D GET_PINS_NR(attr->attributes_low);
> > +	}
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +	return ret;
> > +}
>=20
> ...
>=20
> > +	ret =3D ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
> > +				      sizeof(*rx), &t);
>=20
> Possible to have on a single line (if you use relaxed 100 limit).
> Or (re)split it more logically:
>=20
> 	ret =3D ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES,
> 				      sizeof(*tx), sizeof(*rx), &t);
>=20
> > +	if (ret)
> > +		return ret;
>=20
> ...
>=20
> > +	/*
> > +	 * 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)
>=20
> Please, use standard pattern, i.e.
>=20
> 	if (ret)
> 		return ret;
>=20
> > +		ph->hops->extended_name_get(ph, PINCTRL_NAME_GET,
> selector,
> > +					    (u32 *)&type, name,
>=20
> Why is an explicit casting needed?

The type is enum, not u32.

>=20
> > +					    SCMI_MAX_STR_SIZE);
> > +	return ret;
>=20
> ...
>=20
> > +	iter =3D ph->hops->iter_response_init(ph, &ops, size,
> > +					    PINCTRL_LIST_ASSOCIATIONS,
> > +					    sizeof(struct
> scmi_msg_pinctrl_list_assoc),
> > +					    &ipriv);
>=20
> > +
>=20
> Redundant blank line.
>=20
> > +	if (IS_ERR(iter))
> > +		return PTR_ERR(iter);
>=20
> ...
>=20
> > +	if (p->flag =3D=3D 1)
> > +		attributes |=3D FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> > +	else if (!p->flag)
>=20
> Be consistent, i.e. if (p->flag =3D=3D 0)
>=20
> > +		attributes |=3D FIELD_PREP(CONFIG_TYPE_MASK, p-
> >config_types[0]);
>=20
> ...
>=20
> > +		st->num_remaining =3D le32_get_bits(r->num_configs,
> > +						  GENMASK(31, 24));
>=20
> One line?

Scmi drivers use 80 max drivers.
>=20
> ...
>=20
> > +	if (!p->flag) {
> > +		if (p->config_types[0] !=3D
> > +		    le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> > +			return -EINVAL;
> > +	} else if (p->flag =3D=3D 1) {
> > +		p->config_types[st->desc_index + st->loop_idx] =3D
> > +			le32_get_bits(r->configs[st->loop_idx * 2],
> > +				      GENMASK(7, 0));
>=20
> With a temporary variable for r->configs[st->loop_idx * 2] the above can =
be
> written in much better way.

ok. Fix in v7.
>=20
> > +	} else if (p->flag =3D=3D 2) {
> > +		return 0;
> > +	}
>=20
> > +	p->config_values[st->desc_index + st->loop_idx] =3D
> > +		le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
>=20
> For the sake of consistency with the above suggestion also temporary for =
next
> config value.
>=20
> ...
>=20
> > +	iter =3D ph->hops->iter_response_init(ph, &ops, 1,
> PINCTRL_SETTINGS_GET,
> > +					    sizeof(struct
> scmi_msg_settings_get),
> > +					    &ipriv);
>=20
> > +
>=20
> Redundant blank line.
>=20
> > +	if (IS_ERR(iter))
> > +		return PTR_ERR(iter);
>=20
> ...
>=20
> > +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_hand=
le
> *ph,
> > +				       u32 selector,
> > +				       struct scmi_group_info *group) {
> > +	int ret;
>=20
> > +	if (!group)
> > +		return -EINVAL;
>=20
> When the above is not a dead code?

It could be removed.

>=20
> > +	ret =3D 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 =3D kmalloc_array(group->nr_pins,
> > +					  sizeof(*group->group_pins),
> > +					  GFP_KERNEL);
> > +	if (!group->group_pins)
> > +		return -ENOMEM;
> > +
> > +	ret =3D scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> > +					     group->nr_pins, group-
> >group_pins);
> > +	if (ret) {
> > +		kfree(group->group_pins);
> > +		return ret;
> > +	}
> > +
> > +	group->present =3D true;
> > +	return 0;
> > +}
>=20
> ...
>=20
> > +		ret =3D scmi_pinctrl_get_group_info(ph, selector,
> > +						  &pi->groups[selector]);
>=20
> One line?
>=20
> > +		if (ret)
> > +			return ret;
>=20
> ...
>=20
> > +	ret =3D scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> > +				      func->name,
> > +				      &func->nr_groups);
>=20
> At least last two lines can be joined.
>=20
> > +	if (ret)
> > +		return ret;
>=20
> ...
>=20
> > +	ret =3D scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> > +				      pin->name, NULL);
>=20
> It's pleany of room on the previous line.
>=20
> > +	if (ret)
> > +		return ret;
>=20
> ...
>=20
> > +		ret =3D scmi_pinctrl_get_pin_info(ph, selector,
> > +						&pi->pins[selector]);
>=20
> Ditto.
>=20
> > +		if (ret)
> > +			return ret;
>=20
> ...
>=20
> > +static int scmi_pinctrl_protocol_init(const struct
> > +scmi_protocol_handle *ph) {
> > +	int ret;
> > +	u32 version;
> > +	struct scmi_pinctrl_info *pinfo;
> > +
> > +	ret =3D ph->xops->version_get(ph, &version);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> > +		PROTOCOL_REV_MAJOR(version),
> PROTOCOL_REV_MINOR(version));
> > +
> > +	pinfo =3D devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
>=20
> Huh?!
>=20
> Please, get yourself familiar with the scope of devm APIs.

Please teach me if this is wrong.

>=20
> > +	if (!pinfo)
> > +		return -ENOMEM;
> > +
> > +	ret =3D scmi_pinctrl_attributes_get(ph, pinfo);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pinfo->pins =3D devm_kcalloc(ph->dev, pinfo->nr_pins,
> > +				   sizeof(*pinfo->pins),
> > +				   GFP_KERNEL);
> > +	if (!pinfo->pins)
> > +		return -ENOMEM;
> > +
> > +	pinfo->groups =3D devm_kcalloc(ph->dev, pinfo->nr_groups,
> > +				     sizeof(*pinfo->groups),
> > +				     GFP_KERNEL);
> > +	if (!pinfo->groups)
> > +		return -ENOMEM;
> > +
> > +	pinfo->functions =3D devm_kcalloc(ph->dev, pinfo->nr_functions,
> > +					sizeof(*pinfo->functions),
> > +					GFP_KERNEL);
> > +	if (!pinfo->functions)
> > +		return -ENOMEM;
> > +
> > +	pinfo->version =3D version;
> > +
> > +	return ph->set_priv(ph, pinfo, version); }
> > +
> > +static int scmi_pinctrl_protocol_deinit(const struct
> > +scmi_protocol_handle *ph) {
> > +	int i;
> > +	struct scmi_pinctrl_info *pi =3D ph->get_priv(ph);
> > +
> > +	for (i =3D 0; i < pi->nr_groups; i++) {
> > +		if (pi->groups[i].present) {
> > +			kfree(pi->groups[i].group_pins);
> > +			pi->groups[i].present =3D false;
> > +		}
> > +	}
> > +
> > +	for (i =3D 0; i < pi->nr_functions; i++) {
> > +		if (pi->functions[i].present) {
> > +			kfree(pi->functions[i].groups);
>=20
> This is wrong in conjunction with the above.

Yeah.

>=20
> > +			pi->functions[i].present =3D false;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
>=20
> ...
>=20
> > +static const struct scmi_protocol scmi_pinctrl =3D {
> > +	.id =3D SCMI_PROTOCOL_PINCTRL,
> > +	.owner =3D THIS_MODULE,
> > +	.instance_init =3D &scmi_pinctrl_protocol_init,
> > +	.instance_deinit =3D &scmi_pinctrl_protocol_deinit,
> > +	.ops =3D &pinctrl_proto_ops,
> > +	.supported_version =3D SCMI_PROTOCOL_SUPPORTED_VERSION, };
>=20
> > +
>=20
> Redundant blank line.

Fix in v7

Thanks,
Peng.
>=20
> > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
>=20
>=20
> --
> With Best Regards,
> Andy Shevchenko
>=20