Received-SPF: pass (google.com: domain of linux-kernel+bounces-128099-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Peng Fan <peng.fan@nxp.com>
To: Cristian Marussi <cristian.marussi@arm.com>, "Peng Fan (OSS)"
	<peng.fan@oss.nxp.com>
CC: Sudeep Holla <sudeep.holla@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 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
 protocol basic support
Thread-Topic: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
 protocol basic support
Thread-Index: AQHahKOBYTQv1KoiVkioSbRA3lNLQLFUx+IAgAA7aVA=
Date: Tue, 2 Apr 2024 14:04:23 +0000
Message-ID:
 <DU0PR04MB941718594983D668A418879C883E2@DU0PR04MB9417.eurprd04.prod.outlook.com>
References: <20240402-pinctrl-scmi-v7-0-3ea519d12cf7@nxp.com>
 <20240402-pinctrl-scmi-v7-3-3ea519d12cf7@nxp.com> <Zgvd7npz1jdJSu-b@pluto>
In-Reply-To: <Zgvd7npz1jdJSu-b@pluto>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?uL40QnGqWlu3WJqw5BwZxf88IdCrp3CsV4LWJ0ysFxaWxDENjSJJpbbliYF2?=
 =?us-ascii?Q?Crq1HMp5SJ9eWRqikyqA0A/9B1qBWvJQduxeGQ4Qj5b/I9GbcvjIE8revKvr?=
 =?us-ascii?Q?Ip/mQBJX4xvv6raGs1+LbGBxWArIEb/r5kzat1wQsv/H89blmgjA9uBGVty4?=
 =?us-ascii?Q?m/6Q6X40E1VNKEWC3TRhOI0tPv4EFBJ0RtlND/SpNvp7+u6T5ghFbvkKB+Mn?=
 =?us-ascii?Q?1/az4dMlfazYSMEcnFk4nqy8bQy+gZfmk+yRAnZSgxR5c9vtW19jus69BZDc?=
 =?us-ascii?Q?OJQmKZRonwa0vqffBVwzbozih2djnrhUW0rNqhXu1ghWN6l2B6WmdZ7AtJOB?=
 =?us-ascii?Q?08l5g8jFkTh9BDWkW7T5DZkm5ZkM9eHLSXOm9suUueQEapudyRAdIN8UkQbF?=
 =?us-ascii?Q?G0RjygVcI4YeBW8OljOzEBzGlZOsD+4pOcCeNFqLqvNHjtqJjucsvs/5UyCy?=
 =?us-ascii?Q?TJ9Bp4OI8LoVrFX0f/PPYVXXZQh3AsZRh+LFiLgDG/7ktK0akQZdkLIbRsNl?=
 =?us-ascii?Q?Hp0SeQ8KmoEyT56/DwJ90Jw7uGnt49l+K7WryaeQXuUUyZ9w5hHnoCDUD9Eo?=
 =?us-ascii?Q?yn4mfnq80jo7q+d7OQvSu1bwHn/TWuScIiOz/4nRx8sTf9vUSvWGGY5z2/tb?=
 =?us-ascii?Q?AuFsG9hf6p8zfE8jc/ekx2MmSPLlSQq4kT+MF5ZpqPdsOnrIaNxhsNZEEz96?=
 =?us-ascii?Q?1oltPAU3M05mg+94qI/Rn/jp47UvTVkGk4gUAx6DqHlH6xTwXHmyhBrcc2be?=
 =?us-ascii?Q?h32TjvxSzE1oslyVJqqeEqllK3xh53vRnuuBgBegPM/fS6wdudUcU1czjDly?=
 =?us-ascii?Q?baaeOYZHRpuBL+6I9XW8C9Ug4/fBN4W8EMKbjHlCjIP4Jy6ir/7zesXKdjTY?=
 =?us-ascii?Q?YOZELrBQKAn/NbIJtFMOSXLDbzycE3qmSSWg4D2zj5aonMHniQYGTKGNBNSz?=
 =?us-ascii?Q?/TXfQEvxTidOPknoZ5dZTBLOV9uzKSEAABATPjJr/YGH1eLmNxnul9cifjpg?=
 =?us-ascii?Q?m8BqAcgtUjGKInv+CuyFRjOAXb5FcRguQ2hzi7yktzDZ0nYvsYu5ay0SEWrJ?=
 =?us-ascii?Q?8SjKWMYBLcAQlXak9PbTXCRCkB1mXqM0W5bRnxVms592Q40fSzrVJOufpOgI?=
 =?us-ascii?Q?G7BQi8txb/c6TvjjSYCVp1Dn8xZackXPHDctQ/c/nvtRwxb6cmwQflQ2D9Xa?=
 =?us-ascii?Q?Ez9lfsKW53t4TxoZ9rqKe+qByrxNGDzl8NiYumAJHEI/ipZ+v++RnwjlxP0b?=
 =?us-ascii?Q?IfFhIH8L2MANs1pY8xA2qGweA3M3A2C7G2KVAADgF7XA4fB2EutPMSRCZpU8?=
 =?us-ascii?Q?huWOdZHqj96I92q28xxdYaaAAb6ZnTUye2CZlKvEzhGNvc8JitR2uEJ55RLB?=
 =?us-ascii?Q?DTTIlejIsCn+x+VkLqSMZZ+ghLiodAt12kKO4nf0wD12UzQssGC2mWdjqyeA?=
 =?us-ascii?Q?z85TkaEzq8vco+AR35HDCUIae7gKP9SZOZGAnOZqAeEoxmCiQErmaxAIVITy?=
 =?us-ascii?Q?p4phSsK+6cDgjB/fty3kZjqYTQonT1tqZLwDcZpZA9xXoh6N6Qk2G01Rpmf3?=
 =?us-ascii?Q?+maIVjEx0J01MYOh7Ng=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: 17fa617d-1e0a-451f-6c49-08dc531dcc8e
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Apr 2024 14:04:23.0661
 (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: BRKUYE6luq8zWsamHjz94bLBmQSrtFOJc6glD3DaVJ8OSvN3cqCBvuylbqB+H7OtwzpDIB1Jp+YxV+9+cnwDCA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR04MB8411

> Subject: Re: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>=20
> On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add basic implementation of the SCMI v3.2 pincontrol protocol.
> >
>=20
> Hi,
>=20
>=20
> > Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
>=20
> [snip]
>=20
>=20
> > +struct scmi_settings_get_ipriv {
> > +	u32 selector;
> > +	enum scmi_pinctrl_selector_type type;
> > +	bool get_all;
> > +	enum scmi_pinctrl_conf_type *config_types;
> > +	u32 *config_values;
> > +};
> > +
> > +static void
> > +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_inde=
x,
> > +					  const void *priv)
> > +{
> > +	struct scmi_msg_settings_get *msg =3D message;
> > +	const struct scmi_settings_get_ipriv *p =3D priv;
> > +	u32 attributes;
> > +
> > +	attributes =3D FIELD_PREP(SELECTOR_MASK, p->type);
> > +
> > +	if (p->get_all) {
> > +		attributes |=3D FIELD_PREP(CONFIG_FLAG_MASK, 1) |
> > +			FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> > +	} else {
> > +		attributes |=3D FIELD_PREP(CONFIG_TYPE_MASK, p-
> >config_types[0]);
> > +	}
> > +
> > +	msg->attributes =3D cpu_to_le32(attributes);
> > +	msg->identifier =3D cpu_to_le32(p->selector); }
> > +
> > +static int
> > +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> > +				       const void *response, void *priv) {
> > +	const struct scmi_resp_settings_get *r =3D response;
> > +	struct scmi_settings_get_ipriv *p =3D priv;
> > +
> > +	if (p->get_all) {
> > +		st->num_returned =3D le32_get_bits(r->num_configs,
> GENMASK(7, 0));
> > +		st->num_remaining =3D le32_get_bits(r->num_configs,
> GENMASK(31, 24));
> > +	} else {
> > +		st->num_returned =3D 1;
> > +		st->num_remaining =3D 0;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +iter_pinctrl_settings_get_process_response(const struct
> scmi_protocol_handle *ph,
> > +					   const void *response,
> > +					   struct scmi_iterator_state *st,
> > +					   void *priv)
> > +{
> > +	const struct scmi_resp_settings_get *r =3D response;
> > +	struct scmi_settings_get_ipriv *p =3D priv;
> > +	u32 type =3D le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7,
> 0));
> > +	u32 val =3D le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> > +
> > +	if (p->get_all) {
> > +		p->config_types[st->desc_index + st->loop_idx] =3D type;
> > +	} else {
> > +		if (p->config_types[0] !=3D type)
> > +			return -EINVAL;
> > +	}
> > +
> > +	p->config_values[st->desc_index + st->loop_idx] =3D val;
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32
> selector,
> > +			  enum scmi_pinctrl_selector_type type,
> > +			  enum scmi_pinctrl_conf_type config_type,
> > +			  u32 *config_value, bool get_all) {
> > +	int ret;
> > +	void *iter;
> > +	struct scmi_iterator_ops ops =3D {
> > +		.prepare_message =3D
> iter_pinctrl_settings_get_prepare_message,
> > +		.update_state =3D iter_pinctrl_settings_get_update_state,
> > +		.process_response =3D
> iter_pinctrl_settings_get_process_response,
> > +	};
> > +	struct scmi_settings_get_ipriv ipriv =3D {
> > +		.selector =3D selector,
> > +		.type =3D type,
> > +		.get_all =3D get_all,
> > +		.config_types =3D &config_type,
> > +		.config_values =3D config_value,
> > +	};
> > +
> > +	if (!config_value || type =3D=3D FUNCTION_TYPE)
> > +		return -EINVAL;
> > +
> > +	ret =3D scmi_pinctrl_validate_id(ph, selector, type);
> > +	if (ret)
> > +		return ret;
> > +
> > +	iter =3D ph->hops->iter_response_init(ph, &ops, SCMI_PIN_OEM_END,
> > +					    PINCTRL_SETTINGS_GET,
> > +					    sizeof(struct
> scmi_msg_settings_get),
> > +					    &ipriv);
> > +	if (IS_ERR(iter))
> > +		return PTR_ERR(iter);
> > +
> > +	return ph->hops->iter_response_run(iter);
> > +}
> > +
> > +static int scmi_pinctrl_settings_get_one(const struct scmi_protocol_ha=
ndle
> *ph,
> > +					 u32 selector,
> > +					 enum scmi_pinctrl_selector_type
> type,
> > +					 enum scmi_pinctrl_conf_type
> config_type,
> > +					 u32 *config_value)
> > +{
> > +	return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> > +					 config_value, false);
> > +}
> > +
> > +static int scmi_pinctrl_settings_get_all(const struct scmi_protocol_ha=
ndle
> *ph,
> > +					 u32 selector,
> > +					 enum scmi_pinctrl_selector_type
> type,
> > +					 enum scmi_pinctrl_conf_type
> config_type,
> > +					 u32 *config_value)
> > +{
> > +	return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> > +					 config_value, true);
> > +}
> > +
>=20
> If you generalize the scmi_pinctrl_settings_get() and reintroduce a
> .settings_get_all() ops (even though unused by pinctrl driver, I am fine =
with
> this..), you should take care to pass as an input parameter NOT only the =
array
> of config_values BUT also an array of config_types since you could get ba=
ck
> up to 256 OEM types: for this reason you will need also to pass to
> scmi_pinctrl_settings_get() an input param that specifies the sizes of th=
e
> 2 array input params (in order to avoid oveflows) AND use that same inout
> param also as an output param to report at the end how many OEM types
> were effectively found and returned....
>=20
> IOW, I did this on top of your V7 to make the settings_get_all work:
>=20
> ---8<---
> diff --git a/drivers/firmware/arm_scmi/pinctrl.c
> b/drivers/firmware/arm_scmi/pinctrl.c
> index b75af1dd75fa..f4937af66c4d 100644
> --- a/drivers/firmware/arm_scmi/pinctrl.c
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -317,6 +317,7 @@ struct scmi_settings_get_ipriv {
>  	u32 selector;
>  	enum scmi_pinctrl_selector_type type;
>  	bool get_all;
> +	unsigned int *nr_configs;
>  	enum scmi_pinctrl_conf_type *config_types;
>  	u32 *config_values;
>  };
> @@ -379,6 +380,7 @@ iter_pinctrl_settings_get_process_response(const
> struct scmi_protocol_handle *ph
>  	}
>=20
>  	p->config_values[st->desc_index + st->loop_idx] =3D val;
> +	++*p->nr_configs;
>=20
>  	return 0;
>  }
> @@ -386,11 +388,13 @@ iter_pinctrl_settings_get_process_response(const
> struct scmi_protocol_handle *ph  static int  scmi_pinctrl_settings_get(co=
nst
> struct scmi_protocol_handle *ph, u32 selector,
>  			  enum scmi_pinctrl_selector_type type,
> -			  enum scmi_pinctrl_conf_type config_type,
> -			  u32 *config_value, bool get_all)
> +			  unsigned int *nr_configs,
> +			  enum scmi_pinctrl_conf_type *config_types,
> +			  u32 *config_values)
>  {
>  	int ret;
>  	void *iter;
> +	unsigned int max_configs =3D *nr_configs;
>  	struct scmi_iterator_ops ops =3D {
>  		.prepare_message =3D
> iter_pinctrl_settings_get_prepare_message,
>  		.update_state =3D iter_pinctrl_settings_get_update_state,
> @@ -399,19 +403,22 @@ scmi_pinctrl_settings_get(const struct
> scmi_protocol_handle *ph, u32 selector,
>  	struct scmi_settings_get_ipriv ipriv =3D {
>  		.selector =3D selector,
>  		.type =3D type,
> -		.get_all =3D get_all,
> -		.config_types =3D &config_type,
> -		.config_values =3D config_value,
> +		.get_all =3D (max_configs > 1),
> +		.nr_configs =3D nr_configs,
> +		.config_types =3D config_types,
> +		.config_values =3D config_values,
>  	};
>=20
> -	if (!config_value || type =3D=3D FUNCTION_TYPE)
> +	if (!config_types || !config_values || type =3D=3D FUNCTION_TYPE)
>  		return -EINVAL;
>=20
>  	ret =3D scmi_pinctrl_validate_id(ph, selector, type);
>  	if (ret)
>  		return ret;
>=20
> -	iter =3D ph->hops->iter_response_init(ph, &ops, SCMI_PIN_OEM_END,
> +	/* Prepare to count returned configs */
> +	*nr_configs =3D 0;
> +	iter =3D ph->hops->iter_response_init(ph, &ops, max_configs,
>  					    PINCTRL_SETTINGS_GET,
>  					    sizeof(struct
> scmi_msg_settings_get),
>  					    &ipriv);
> @@ -427,18 +434,24 @@ static int scmi_pinctrl_settings_get_one(const
> struct scmi_protocol_handle *ph,
>  					 enum scmi_pinctrl_conf_type
> config_type,
>  					 u32 *config_value)
>  {
> -	return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> -					 config_value, false);
> +	unsigned int nr_configs =3D 1;
> +
> +	return scmi_pinctrl_settings_get(ph, selector, type, &nr_configs,
> +					 &config_type, config_value);
>  }
>=20
>  static int scmi_pinctrl_settings_get_all(const struct scmi_protocol_hand=
le
> *ph,
>  					 u32 selector,
>  					 enum scmi_pinctrl_selector_type
> type,
> -					 enum scmi_pinctrl_conf_type
> config_type,
> -					 u32 *config_value)
> +					 unsigned int *nr_configs,
> +					 enum scmi_pinctrl_conf_type
> *config_types,
> +					 u32 *config_values)
>  {
> -	return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> -					 config_value, true);
> +	if (!nr_configs || *nr_configs =3D=3D 0)
> +		return -EINVAL;
> +
> +	return scmi_pinctrl_settings_get(ph, selector, type, nr_configs,
> +					 config_types, config_values);
>  }
>=20
>  static int
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.=
h
> index abaf6122ea37..7915792efd81 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -882,8 +882,9 @@ struct scmi_pinctrl_proto_ops {
>  	int (*settings_get_all)(const struct scmi_protocol_handle *ph,
>  				u32 selector,
>  				enum scmi_pinctrl_selector_type type,
> -				enum scmi_pinctrl_conf_type config_type,
> -				u32 *config_value);
> +				unsigned int *nr_configs,
> +				enum scmi_pinctrl_conf_type *config_types,
> +				u32 *config_values);
>  	int (*settings_conf)(const struct scmi_protocol_handle *ph,
>  			     u32 selector, enum scmi_pinctrl_selector_type
> type,
>  			     unsigned int nr_configs,
> --->8-----
>=20
> Please check if this addition sounds good to you and integrate into v8
> eventually...

Thanks for helping on this, I will included your changes, and your
Co-developed-by tag if you not mind.

Thanks,
Peng.

>=20
> Thanks,
> Cristian