Re: [PATCH 5/5] pinctrl: pinctrl-scmi: Log number of pins, groups, functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Unhandled error code from pinctrl_scmi_get_groups_count() masks firmware failure.
- [Low] Unnecessary persistent state added to struct scmi_pinctrl.
- [Low] Format string type mismatch in dev_info() logging.
--

commit d7d0dd8e474d3b2b3d8c68dd2d8713e62d0299a0
Author: Alex Tran <alex.tran@oss.qualcomm.com>

pinctrl: pinctrl-scmi: Log number of pins, groups, functions

The SCMI pinctrl driver does not currently log the number of pins,
groups, and functions discovered from firmware.

> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
> index f22be6b7b82a4..abc90a3a7eaf4 100644
> --- a/drivers/pinctrl/pinctrl-scmi.c
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -40,6 +40,7 @@ struct scmi_pinctrl {
>  	struct pinctrl_desc pctl_desc;
>  	struct pinfunction *functions;
>  	unsigned int nr_functions;
> +	unsigned int nr_groups;
>  };

Does nr_groups need to be stored in the scmi_pinctrl struct? It looks like
it is only used once in scmi_pinctrl_probe() for logging, so could it be
declared as a local variable instead?

[ ... ]

> @@ -578,7 +579,15 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev)
>  	if (!pmx->functions)
>  		return -ENOMEM;
>  
> -	return pinctrl_enable(pmx->pctldev);
> +	pmx->nr_groups = pinctrl_scmi_get_groups_count(pmx->pctldev);

Can pinctrl_scmi_get_groups_count() return a negative error code here if the
firmware fails? Since pmx->nr_groups is an unsigned int, would a negative
error code be implicitly cast to a large positive value and mask the failure?

> +
> +	ret = pinctrl_enable(pmx->pctldev);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(dev, "Initialized %d pins, %d groups, %d functions\n",
> +		 pmx->pctl_desc.npins, pmx->nr_groups, pmx->nr_functions);

This isn't a bug, but the variables npins, nr_groups, and nr_functions are
all unsigned integers. Should these format specifiers be %u instead of %d?

> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-scmi-client-probe-log-v1-0-00b47b1be009@oss.qualcomm.com?part=5