Re: [PATCH v10 1/6] pinctrl: s32cc: add/fix some comments

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Typo in the newly added documentation for `struct s32_pinctrl_context`.
- [Low] Invalid kernel-doc comment block syntax.
- [Low] Inconsistent error message capitalization.
--

commit a7822dc8d9c7d5010c39b80aee179b5ec0735e03
Author: Khristine Andreea Barbulescu <khristineandreea.barbulescu@oss.nxp.com>

pinctrl: s32cc: add/fix some comments

Add/fix some comments and print statements.

> diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
> index fe7cd641fddd0..4f88c24e62a21 100644
> --- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
> +++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
> @@ -60,6 +60,12 @@ static u32 get_pin_func(u32 pinmux)
>  	return pinmux & GENMASK(3, 0);
>  }
>  
> +/*

[Severity: Low]
This isn't a bug, but should this start with /** instead of /*?

The newly added comment for struct s32_pinctrl_mem_region follows the
kernel-doc format, but because it begins with /* rather than /**, the
kernel-doc scripts will likely ignore it.

> + * struct s32_pinctrl_mem_region - memory region for a set of SIUL2 registers
> + * @map: regmap used for this range
> + * @pin_range: the pins controlled by these registers
> + * @name: name of the current range
> + */
>  struct s32_pinctrl_mem_region {

[ ... ]

> @@ -67,7 +73,7 @@ struct s32_pinctrl_mem_region {
>  };
>  
>  /*
> - * Holds pin configuration for GPIO's.
> + * struct gpio_pin_config - holds pin configuration for GPIO's

[Severity: Low]
This isn't a bug, but this comment also follows the kernel-doc format
without starting with /**. Should it be updated to /** so it can be picked up
by the documentation tools?

>   * @pin_id: Pin ID for this GPIO
>   * @config: Pin settings
>   * @list: Linked list entry for each gpio pin

[ ... ]

> @@ -79,20 +85,22 @@ struct gpio_pin_config {
>  };
>  
>  /*
> - * Pad config save/restore for power suspend/resume.
> + * struct s32_pinctrl_context - pad config save/restore for suspend/resume
> + * @pads: saved values for the pards

[Severity: Low]
This isn't a bug, but there's a typo here (pards instead of pads).

Additionally, this comment block also lacks the /** prefix required for
kernel-doc.

>   */
>  struct s32_pinctrl_context {
>  	unsigned int *pads;
>  };
>  
>  /*
> + * struct s32_pinctrl - private driver data
>   * @dev: a pointer back to containing device

[Severity: Low]
This isn't a bug, but this comment block is also missing the /** prefix.

>   * @pctl: a pointer to the pinctrl device structure
>   * @regions: reserved memory regions with start/end pin

[ ... ]

> @@ -974,7 +982,7 @@ int s32_pinctrl_probe(struct platform_device *pdev,
>  					    ipctl);
>  	if (IS_ERR(ipctl->pctl))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(ipctl->pctl),
> -				     "could not register s32 pinctrl driver\n");
> +				     "Could not register s32 pinctrl driver\n");

[Severity: Low]
This isn't a bug, but does changing "could" to "Could" break consistency
with the rest of this file?

Other error messages in s32_pinctrl_probe() start with lowercase letters
(e.g., "wrong pinctrl info", "fail to probe dt properties"). Should this be
kept lowercase to match the rest of the file and standard kernel practice?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602080132.3256239-1-khristineandreea.barbulescu@oss.nxp.com?part=1