Re: [PATCH 6/7] pinctrl: Implementation of the generic scmi-pinctrl driver

[Cristian Marussi <cristian.marussi@xxxxxxx>]

On Fri, Dec 15, 2023 at 07:56:34PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@xxxxxxx>
> 
> 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.
> 

[CC: Akashi which was interested in this series]

This generic driver still works fine for me as of now in my emulated
setup using the generic SCMI bindings as in the initial Oleksii example
and a dummy driver consumer for this pins, BUT there is still an open
issue as reported by Akashi previously ..see below.

> Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> ---
>  MAINTAINERS                    |   1 +
>  drivers/pinctrl/Kconfig        |  11 ++
>  drivers/pinctrl/Makefile       |   1 +
>  drivers/pinctrl/pinctrl-scmi.c | 403 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 416 insertions(+)
 
 [snip]

> +static const struct pinmux_ops pinctrl_scmi_pinmux_ops = {
> +	.request = pinctrl_scmi_request,
> +	.free = pinctrl_scmi_free,
> +	.get_functions_count = pinctrl_scmi_get_functions_count,
> +	.get_function_name = pinctrl_scmi_get_function_name,
> +	.get_function_groups = pinctrl_scmi_get_function_groups,
> +	.set_mux = pinctrl_scmi_func_set_mux,
> +};
> +
> +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev, unsigned int _pin,
> +				    unsigned long *config)
> +{
> +	int ret;
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +	enum pin_config_param config_type;
> +	unsigned long config_value;
> +
> +	if (!config)
> +		return -EINVAL;
> +
> +	config_type = pinconf_to_config_param(*config);

This config types that are packed/unpacked from the generic Pinctrl
subsystem have also to be mapped/umapped, here and below, to the SCMI
ones since they are slightly different/.

IOW SCMI V3.2 Table_24 in the spec, which defines Pins Config Types as
understood by an SCMI platform FW is NOT exactly mapping to the enum
pin_config_param used by Pinctrl in its pinconf_to_config so you risk
passing a config_type to the SCMI fw that does NOT mean at all what
intended...if the FW is compliant with SCMI.

> +
> +	ret = pinctrl_ops->config_get(pmx->ph, _pin, PIN_TYPE, config_type, &config_value);
> +	if (ret)
> +		return ret;
> +
> +	*config = pinconf_to_config_packed(config_type, config_value);
> +
> +	return 0;
> +}
> +
> +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
> +				    unsigned int _pin,
> +				    unsigned long *configs,
> +				    unsigned int num_configs)
> +{
> +	int ret;
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!configs || !num_configs)
> +		return -EINVAL;
> +

Same here, as said in the previous patch about pinctrl protocol, you
should pack/unpack and map/unmap from Pinctrl packed configs and types
to SCMI types and unpacked config/values, to fix the mismatch between
Pinctrl and SCMI types and also to avoid the usage of Pinctrl helpers to
extract the types from the SCMI protocol layer so that we can keep it
agnostic in these regards.

Thanks,
Cristian