[PATCH 0/3] pinctrl: s32: driver improvements and generic struct use

[PATCH 0/3] pinctrl: s32: driver improvements and generic struct use

[Chester Lin]

Hello,

This patch series contains some improvements for s32 pinctrl drivers suggested
by upstream[1], such as

  - Fix error shadowings and improve return value handlings.
  - Fix print format.
  - Remove unnecessary blanks.
  - Use proper macros and helpers to simplify codes.
  - Refactor config param parsing and remove config arguments that are never used.
  - Use generic struct pingroup and struct pinfunction to describe pin data.

Regards,
Chester

[1] https://lore.kernel.org/all/20230220023320.3499-1-clin@suse.com/

Chester Lin (3):
  pinctrl: s32: refine error/return/config checks and simplify driver
    codes
  pinctrl: s32cc: refactor pin config parsing
  pinctrl: s32cc: embed generic struct pingroup and pinfunction

 drivers/pinctrl/nxp/pinctrl-s32.h   |  22 +--
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 283 ++++++++++++++++------------
 drivers/pinctrl/nxp/pinctrl-s32g2.c |   8 +-
 3 files changed, 168 insertions(+), 145 deletions(-)

-- 
2.37.3

[PATCH 1/3] pinctrl: s32: refine error/return/config checks and simplify driver codes

[Chester Lin]

Improve error/return code handlings and config checks in order to have
better reliability and simplify driver codes such as removing/changing
improper macros, blanks, print formats and helper calls.

Signed-off-by: Chester Lin <clin@suse.com>
---
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 141 +++++++++++++++-------------
 drivers/pinctrl/nxp/pinctrl-s32g2.c |   8 +-
 2 files changed, 78 insertions(+), 71 deletions(-)

diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
index e1da332433a3..7a38e3216b0c 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
@@ -28,7 +28,8 @@
 #include "../pinctrl-utils.h"
 #include "pinctrl-s32.h"
 
-#define S32_PIN_ID_MASK		GENMASK(31, 4)
+#define S32_PIN_ID_SHIFT	4
+#define S32_PIN_ID_MASK		GENMASK(31, S32_PIN_ID_SHIFT)
 
 #define S32_MSCR_SSS_MASK	GENMASK(2, 0)
 #define S32_MSCR_PUS		BIT(12)
@@ -46,7 +47,7 @@ static struct regmap_config s32_regmap_config = {
 
 static u32 get_pin_no(u32 pinmux)
 {
-	return (pinmux & S32_PIN_ID_MASK) >> __ffs(S32_PIN_ID_MASK);
+	return (pinmux & S32_PIN_ID_MASK) >> S32_PIN_ID_SHIFT;
 }
 
 static u32 get_pin_func(u32 pinmux)
@@ -108,7 +109,7 @@ s32_get_region(struct pinctrl_dev *pctldev, unsigned int pin)
 	unsigned int mem_regions = ipctl->info->mem_regions;
 	unsigned int i;
 
-	for (i = 0; i < mem_regions; ++i) {
+	for (i = 0; i < mem_regions; i++) {
 		pin_range = ipctl->regions[i].pin_range;
 		if (pin >= pin_range->start && pin <= pin_range->end)
 			return &ipctl->regions[i];
@@ -224,8 +225,7 @@ static int s32_dt_group_node_to_map(struct pinctrl_dev *pctldev,
 
 	n_pins = of_property_count_elems_of_size(np, "pinmux", sizeof(u32));
 	if (n_pins < 0) {
-		dev_warn(dev, "Unable to find 'pinmux' property in node %s.\n",
-			np->name);
+		dev_warn(dev, "Can't find 'pinmux' property in node %pOFn\n", np);
 	} else if (!n_pins) {
 		return -EINVAL;
 	}
@@ -317,20 +317,25 @@ static int s32_pmx_set(struct pinctrl_dev *pctldev, unsigned int selector,
 		info->functions[selector].name, grp->name);
 
 	/* Check beforehand so we don't have a partial config. */
-	for (i = 0; i < grp->npins; ++i) {
+	for (i = 0; i < grp->npins; i++) {
 		if (s32_check_pin(pctldev, grp->pin_ids[i]) != 0) {
-			dev_err(info->dev, "invalid pin: %d in group: %d\n",
+			dev_err(info->dev, "invalid pin: %u in group: %u\n",
 				grp->pin_ids[i], group);
 			return -EINVAL;
 		}
 	}
 
-	for (i = 0, ret = 0; i < grp->npins && !ret; ++i) {
+	for (i = 0, ret = 0; i < grp->npins && !ret; i++) {
 		ret = s32_regmap_update(pctldev, grp->pin_ids[i],
 					S32_MSCR_SSS_MASK, grp->pin_sss[i]);
+		if (ret) {
+			dev_err(info->dev, "Failed to set pin %u\n",
+				grp->pin_ids[i]);
+			return ret;
+		}
 	}
 
-	return ret;
+	return 0;
 }
 
 static int s32_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
@@ -375,8 +380,8 @@ static int s32_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
 	int ret;
 
 	ret = s32_regmap_read(pctldev, offset, &config);
-	if (ret != 0)
-		return -EINVAL;
+	if (ret)
+		return ret;
 
 	/* Save current configuration */
 	gpio_pin = kmalloc(sizeof(*gpio_pin), GFP_KERNEL);
@@ -387,7 +392,7 @@ static int s32_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
 	gpio_pin->config = config;
 
 	spin_lock_irqsave(&ipctl->gpio_configs_lock, flags);
-	list_add(&(gpio_pin->list), &(ipctl->gpio_configs));
+	list_add(&gpio_pin->list, &ipctl->gpio_configs);
 	spin_unlock_irqrestore(&ipctl->gpio_configs_lock, flags);
 
 	/* GPIO pin means SSS = 0 */
@@ -401,15 +406,13 @@ static void s32_pmx_gpio_disable_free(struct pinctrl_dev *pctldev,
 				      unsigned int offset)
 {
 	struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
-	struct list_head *pos, *tmp;
-	struct gpio_pin_config *gpio_pin;
+	struct gpio_pin_config *gpio_pin, *tmp;
 	unsigned long flags;
 	int ret;
 
 	spin_lock_irqsave(&ipctl->gpio_configs_lock, flags);
 
-	list_for_each_safe(pos, tmp, &ipctl->gpio_configs) {
-		gpio_pin = list_entry(pos, struct gpio_pin_config, list);
+	list_for_each_entry_safe(gpio_pin, tmp, &ipctl->gpio_configs, list) {
 
 		if (gpio_pin->pin_id == offset) {
 			ret = s32_regmap_write(pctldev, gpio_pin->pin_id,
@@ -417,7 +420,7 @@ static void s32_pmx_gpio_disable_free(struct pinctrl_dev *pctldev,
 			if (ret != 0)
 				goto unlock;
 
-			list_del(pos);
+			list_del(&gpio_pin->list);
 			kfree(gpio_pin);
 			break;
 		}
@@ -461,7 +464,8 @@ static const int support_slew[] = {208, -1, -1, -1, 166, 150, 133, 83};
 
 static int s32_get_slew_regval(int arg)
 {
-	int i;
+	unsigned int i;
+
 	/* Translate a real slew rate (MHz) to a register value */
 	for (i = 0; i < ARRAY_SIZE(support_slew); i++) {
 		if (arg == support_slew[i])
@@ -542,10 +546,11 @@ static int s32_pinconf_mscr_update(struct pinctrl_dev *pctldev,
 	unsigned int config = 0, mask = 0;
 	int i, ret;
 
-	if (s32_check_pin(pctldev, pin_id) != 0)
-		return -EINVAL;
+	ret = s32_check_pin(pctldev, pin_id);
+	if (ret)
+		return ret;
 
-	dev_dbg(ipctl->dev, "pinconf set pin %s with %d configs\n",
+	dev_dbg(ipctl->dev, "pinconf set pin %s with %u configs\n",
 		pin_get_name(pctldev, pin_id), num_configs);
 
 	for (i = 0; i < num_configs; i++) {
@@ -559,11 +564,9 @@ static int s32_pinconf_mscr_update(struct pinctrl_dev *pctldev,
 	if (!config && !mask)
 		return 0;
 
-	ret = s32_regmap_update(pctldev, pin_id, mask, config);
-
-	dev_dbg(ipctl->dev, "update: pin %d cfg 0x%x\n", pin_id, config);
+	dev_dbg(ipctl->dev, "update: pin %u cfg 0x%x\n", pin_id, config);
 
-	return ret;
+	return s32_regmap_update(pctldev, pin_id, mask, config);
 }
 
 static int s32_pinconf_get(struct pinctrl_dev *pctldev,
@@ -604,10 +607,13 @@ static void s32_pinconf_dbg_show(struct pinctrl_dev *pctldev,
 				 struct seq_file *s, unsigned int pin_id)
 {
 	unsigned int config;
-	int ret = s32_regmap_read(pctldev, pin_id, &config);
+	int ret;
+
+	ret = s32_regmap_read(pctldev, pin_id, &config);
+	if (ret)
+		return;
 
-	if (!ret)
-		seq_printf(s, "0x%x", config);
+	seq_printf(s, "0x%x", config);
 }
 
 static void s32_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
@@ -710,7 +716,7 @@ int s32_pinctrl_resume(struct device *dev)
 }
 #endif
 
-static void s32_pinctrl_parse_groups(struct device_node *np,
+static int s32_pinctrl_parse_groups(struct device_node *np,
 				     struct s32_pin_group *grp,
 				     struct s32_pinctrl_soc_info *info)
 {
@@ -722,21 +728,20 @@ static void s32_pinctrl_parse_groups(struct device_node *np,
 
 	dev = info->dev;
 
-	dev_dbg(dev, "group: %s\n", np->name);
+	dev_dbg(dev, "group: %pOFn\n", np);
 
 	/* Initialise group */
 	grp->name = np->name;
 
 	npins = of_property_count_elems_of_size(np, "pinmux", sizeof(u32));
-
 	if (npins < 0) {
 		dev_err(dev, "Failed to read 'pinmux' property in node %s.\n",
-			np->name);
-		return;
+			grp->name);
+		return -EINVAL;
 	}
 	if (!npins) {
-		dev_err(dev, "The group %s has no pins.\n", np->name);
-		return;
+		dev_err(dev, "The group %s has no pins.\n", grp->name);
+		return -EINVAL;
 	}
 
 	grp->npins = npins;
@@ -745,12 +750,8 @@ static void s32_pinctrl_parse_groups(struct device_node *np,
 				    sizeof(unsigned int), GFP_KERNEL);
 	grp->pin_sss = devm_kcalloc(info->dev, grp->npins,
 				    sizeof(unsigned int), GFP_KERNEL);
-
-	if (!grp->pin_ids || !grp->pin_sss) {
-		dev_err(dev, "Failed to allocate memory for the group %s.\n",
-			np->name);
-		return;
-	}
+	if (!grp->pin_ids || !grp->pin_sss)
+		return -ENOMEM;
 
 	i = 0;
 	of_property_for_each_u32(np, "pinmux", prop, p, pinmux) {
@@ -761,9 +762,11 @@ static void s32_pinctrl_parse_groups(struct device_node *np,
 			grp->pin_ids[i], grp->pin_sss[i]);
 		i++;
 	}
+
+	return 0;
 }
 
-static void s32_pinctrl_parse_functions(struct device_node *np,
+static int s32_pinctrl_parse_functions(struct device_node *np,
 					struct s32_pinctrl_soc_info *info,
 					u32 index)
 {
@@ -771,8 +774,9 @@ static void s32_pinctrl_parse_functions(struct device_node *np,
 	struct s32_pmx_func *func;
 	struct s32_pin_group *grp;
 	u32 i = 0;
+	int ret = 0;
 
-	dev_dbg(info->dev, "parse function(%d): %s\n", index, np->name);
+	dev_dbg(info->dev, "parse function(%u): %pOFn\n", index, np);
 
 	func = &info->functions[index];
 
@@ -780,18 +784,24 @@ static void s32_pinctrl_parse_functions(struct device_node *np,
 	func->name = np->name;
 	func->num_groups = of_get_child_count(np);
 	if (func->num_groups == 0) {
-		dev_err(info->dev, "no groups defined in %s\n", np->full_name);
-		return;
+		dev_err(info->dev, "no groups defined in %pOF\n", np);
+		return -EINVAL;
 	}
-	func->groups = devm_kzalloc(info->dev,
-			func->num_groups * sizeof(char *), GFP_KERNEL);
+	func->groups = devm_kcalloc(info->dev, func->num_groups,
+				    sizeof(char *), GFP_KERNEL);
+	if (!func->groups)
+		return -ENOMEM;
 
 	for_each_child_of_node(np, child) {
 		func->groups[i] = child->name;
 		grp = &info->groups[info->grp_index++];
-		s32_pinctrl_parse_groups(child, grp, info);
+		ret = s32_pinctrl_parse_groups(child, grp, info);
+		if (ret)
+			return ret;
 		i++;
 	}
+
+	return 0;
 }
 
 static int s32_pinctrl_probe_dt(struct platform_device *pdev,
@@ -804,6 +814,7 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev,
 	struct regmap *map;
 	void __iomem *base;
 	int mem_regions = info->mem_regions;
+	int ret;
 	u32 nfuncs = 0;
 	u32 i = 0;
 
@@ -815,13 +826,12 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
-	ipctl->regions = devm_kzalloc(&pdev->dev,
-				      mem_regions * sizeof(*(ipctl->regions)),
-				      GFP_KERNEL);
+	ipctl->regions = devm_kcalloc(&pdev->dev, mem_regions,
+				      sizeof(*(ipctl->regions)), GFP_KERNEL);
 	if (!ipctl->regions)
 		return -ENOMEM;
 
-	for (i = 0; i < mem_regions; ++i) {
+	for (i = 0; i < mem_regions; i++) {
 		base = devm_platform_get_and_ioremap_resource(pdev, i, &res);
 		if (IS_ERR(base))
 			return PTR_ERR(base);
@@ -851,24 +861,26 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev,
 	}
 
 	info->nfunctions = nfuncs;
-	info->functions = devm_kzalloc(&pdev->dev,
-				       nfuncs * sizeof(struct s32_pmx_func),
-				       GFP_KERNEL);
+	info->functions = devm_kcalloc(&pdev->dev, nfuncs,
+				       sizeof(struct s32_pmx_func), GFP_KERNEL);
 	if (!info->functions)
 		return -ENOMEM;
 
 	info->ngroups = 0;
 	for_each_child_of_node(np, child)
 		info->ngroups += of_get_child_count(child);
-	info->groups = devm_kzalloc(&pdev->dev,
-				    info->ngroups * sizeof(struct s32_pin_group),
-				    GFP_KERNEL);
+
+	info->groups = devm_kcalloc(&pdev->dev, info->ngroups,
+				    sizeof(struct s32_pin_group), GFP_KERNEL);
 	if (!info->groups)
 		return -ENOMEM;
 
 	i = 0;
-	for_each_child_of_node(np, child)
-		s32_pinctrl_parse_functions(child, info, i++);
+	for_each_child_of_node(np, child) {
+		ret = s32_pinctrl_parse_functions(child, info, i++);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
@@ -923,12 +935,9 @@ int s32_pinctrl_probe(struct platform_device *pdev,
 
 	ipctl->pctl = devm_pinctrl_register(&pdev->dev, s32_pinctrl_desc,
 					    ipctl);
-
-	if (IS_ERR(ipctl->pctl)) {
-		dev_err(&pdev->dev, "could not register s32 pinctrl driver\n");
-		return PTR_ERR(ipctl->pctl);
-	}
-
+	if (IS_ERR(ipctl->pctl))
+		return dev_err_probe(&pdev->dev, PTR_ERR(ipctl->pctl),
+				     "could not register s32 pinctrl driver\n");
 #ifdef CONFIG_PM_SLEEP
 	saved_context = &ipctl->saved_context;
 	saved_context->pads =
diff --git a/drivers/pinctrl/nxp/pinctrl-s32g2.c b/drivers/pinctrl/nxp/pinctrl-s32g2.c
index 5028f4adc389..0b0b06f12b8a 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32g2.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32g2.c
@@ -746,8 +746,8 @@ static int s32g_pinctrl_probe(struct platform_device *pdev)
 	if (!of_id)
 		return -ENODEV;
 
-	return s32_pinctrl_probe
-			(pdev, (struct s32_pinctrl_soc_info *) of_id->data);
+	return s32_pinctrl_probe(pdev,
+				 (struct s32_pinctrl_soc_info *) of_id->data);
 }
 
 static const struct dev_pm_ops s32g_pinctrl_pm_ops = {
@@ -757,14 +757,12 @@ static const struct dev_pm_ops s32g_pinctrl_pm_ops = {
 static struct platform_driver s32g_pinctrl_driver = {
 	.driver = {
 		.name = "s32g-siul2-pinctrl",
-		.owner = THIS_MODULE,
 		.of_match_table = s32_pinctrl_of_match,
-		.pm = &s32g_pinctrl_pm_ops,
+		.pm = pm_sleep_ptr(&s32g_pinctrl_pm_ops),
 		.suppress_bind_attrs = true,
 	},
 	.probe = s32g_pinctrl_probe,
 };
-
 builtin_platform_driver(s32g_pinctrl_driver);
 
 MODULE_AUTHOR("Matthew Nunez <matthew.nunez@nxp.com>");
-- 
2.37.3

Re: [PATCH 1/3] pinctrl: s32: refine error/return/config checks and simplify driver codes

[Andy Shevchenko]

On Tue, Mar 14, 2023 at 3:47 PM Chester Lin <clin@suse.com> wrote:
>
> Improve error/return code handlings and config checks in order to have
> better reliability and simplify driver codes such as removing/changing
> improper macros, blanks, print formats and helper calls.

I dunno if the maintainer wants this to be split to logically unified
changes, but either way it's fine to me.
Also see below some additional minor things you might be interested in fixing.
With these being addressed:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Chester Lin <clin@suse.com>
> ---
>  drivers/pinctrl/nxp/pinctrl-s32cc.c | 141 +++++++++++++++-------------
>  drivers/pinctrl/nxp/pinctrl-s32g2.c |   8 +-
>  2 files changed, 78 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
> index e1da332433a3..7a38e3216b0c 100644
> --- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
> +++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
> @@ -28,7 +28,8 @@
>  #include "../pinctrl-utils.h"
>  #include "pinctrl-s32.h"
>
> -#define S32_PIN_ID_MASK                GENMASK(31, 4)
> +#define S32_PIN_ID_SHIFT       4
> +#define S32_PIN_ID_MASK                GENMASK(31, S32_PIN_ID_SHIFT)
>
>  #define S32_MSCR_SSS_MASK      GENMASK(2, 0)
>  #define S32_MSCR_PUS           BIT(12)
> @@ -46,7 +47,7 @@ static struct regmap_config s32_regmap_config = {
>
>  static u32 get_pin_no(u32 pinmux)
>  {
> -       return (pinmux & S32_PIN_ID_MASK) >> __ffs(S32_PIN_ID_MASK);
> +       return (pinmux & S32_PIN_ID_MASK) >> S32_PIN_ID_SHIFT;
>  }
>
>  static u32 get_pin_func(u32 pinmux)
> @@ -108,7 +109,7 @@ s32_get_region(struct pinctrl_dev *pctldev, unsigned int pin)
>         unsigned int mem_regions = ipctl->info->mem_regions;
>         unsigned int i;
>
> -       for (i = 0; i < mem_regions; ++i) {
> +       for (i = 0; i < mem_regions; i++) {
>                 pin_range = ipctl->regions[i].pin_range;
>                 if (pin >= pin_range->start && pin <= pin_range->end)
>                         return &ipctl->regions[i];
> @@ -224,8 +225,7 @@ static int s32_dt_group_node_to_map(struct pinctrl_dev *pctldev,
>
>         n_pins = of_property_count_elems_of_size(np, "pinmux", sizeof(u32));
>         if (n_pins < 0) {
> -               dev_warn(dev, "Unable to find 'pinmux' property in node %s.\n",
> -                       np->name);
> +               dev_warn(dev, "Can't find 'pinmux' property in node %pOFn\n", np);
>         } else if (!n_pins) {
>                 return -EINVAL;
>         }
> @@ -317,20 +317,25 @@ static int s32_pmx_set(struct pinctrl_dev *pctldev, unsigned int selector,
>                 info->functions[selector].name, grp->name);
>
>         /* Check beforehand so we don't have a partial config. */
> -       for (i = 0; i < grp->npins; ++i) {
> +       for (i = 0; i < grp->npins; i++) {
>                 if (s32_check_pin(pctldev, grp->pin_ids[i]) != 0) {
> -                       dev_err(info->dev, "invalid pin: %d in group: %d\n",
> +                       dev_err(info->dev, "invalid pin: %u in group: %u\n",
>                                 grp->pin_ids[i], group);
>                         return -EINVAL;
>                 }
>         }
>
> -       for (i = 0, ret = 0; i < grp->npins && !ret; ++i) {
> +       for (i = 0, ret = 0; i < grp->npins && !ret; i++) {
>                 ret = s32_regmap_update(pctldev, grp->pin_ids[i],
>                                         S32_MSCR_SSS_MASK, grp->pin_sss[i]);
> +               if (ret) {
> +                       dev_err(info->dev, "Failed to set pin %u\n",
> +                               grp->pin_ids[i]);
> +                       return ret;
> +               }
>         }
>
> -       return ret;
> +       return 0;
>  }
>
>  static int s32_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
> @@ -375,8 +380,8 @@ static int s32_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
>         int ret;
>
>         ret = s32_regmap_read(pctldev, offset, &config);
> -       if (ret != 0)
> -               return -EINVAL;
> +       if (ret)
> +               return ret;
>
>         /* Save current configuration */
>         gpio_pin = kmalloc(sizeof(*gpio_pin), GFP_KERNEL);
> @@ -387,7 +392,7 @@ static int s32_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
>         gpio_pin->config = config;
>
>         spin_lock_irqsave(&ipctl->gpio_configs_lock, flags);
> -       list_add(&(gpio_pin->list), &(ipctl->gpio_configs));
> +       list_add(&gpio_pin->list, &ipctl->gpio_configs);
>         spin_unlock_irqrestore(&ipctl->gpio_configs_lock, flags);
>
>         /* GPIO pin means SSS = 0 */
> @@ -401,15 +406,13 @@ static void s32_pmx_gpio_disable_free(struct pinctrl_dev *pctldev,
>                                       unsigned int offset)
>  {
>         struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> -       struct list_head *pos, *tmp;
> -       struct gpio_pin_config *gpio_pin;
> +       struct gpio_pin_config *gpio_pin, *tmp;
>         unsigned long flags;
>         int ret;
>
>         spin_lock_irqsave(&ipctl->gpio_configs_lock, flags);
>
> -       list_for_each_safe(pos, tmp, &ipctl->gpio_configs) {
> -               gpio_pin = list_entry(pos, struct gpio_pin_config, list);
> +       list_for_each_entry_safe(gpio_pin, tmp, &ipctl->gpio_configs, list) {

>

This blank line now can be removed.

>                 if (gpio_pin->pin_id == offset) {
>                         ret = s32_regmap_write(pctldev, gpio_pin->pin_id,
> @@ -417,7 +420,7 @@ static void s32_pmx_gpio_disable_free(struct pinctrl_dev *pctldev,
>                         if (ret != 0)
>                                 goto unlock;
>
> -                       list_del(pos);
> +                       list_del(&gpio_pin->list);
>                         kfree(gpio_pin);
>                         break;
>                 }
> @@ -461,7 +464,8 @@ static const int support_slew[] = {208, -1, -1, -1, 166, 150, 133, 83};
>
>  static int s32_get_slew_regval(int arg)
>  {
> -       int i;
> +       unsigned int i;
> +
>         /* Translate a real slew rate (MHz) to a register value */
>         for (i = 0; i < ARRAY_SIZE(support_slew); i++) {
>                 if (arg == support_slew[i])
> @@ -542,10 +546,11 @@ static int s32_pinconf_mscr_update(struct pinctrl_dev *pctldev,
>         unsigned int config = 0, mask = 0;
>         int i, ret;
>
> -       if (s32_check_pin(pctldev, pin_id) != 0)
> -               return -EINVAL;
> +       ret = s32_check_pin(pctldev, pin_id);
> +       if (ret)
> +               return ret;
>
> -       dev_dbg(ipctl->dev, "pinconf set pin %s with %d configs\n",
> +       dev_dbg(ipctl->dev, "pinconf set pin %s with %u configs\n",
>                 pin_get_name(pctldev, pin_id), num_configs);
>
>         for (i = 0; i < num_configs; i++) {
> @@ -559,11 +564,9 @@ static int s32_pinconf_mscr_update(struct pinctrl_dev *pctldev,
>         if (!config && !mask)
>                 return 0;
>
> -       ret = s32_regmap_update(pctldev, pin_id, mask, config);
> -
> -       dev_dbg(ipctl->dev, "update: pin %d cfg 0x%x\n", pin_id, config);
> +       dev_dbg(ipctl->dev, "update: pin %u cfg 0x%x\n", pin_id, config);
>
> -       return ret;
> +       return s32_regmap_update(pctldev, pin_id, mask, config);
>  }
>
>  static int s32_pinconf_get(struct pinctrl_dev *pctldev,
> @@ -604,10 +607,13 @@ static void s32_pinconf_dbg_show(struct pinctrl_dev *pctldev,
>                                  struct seq_file *s, unsigned int pin_id)
>  {
>         unsigned int config;
> -       int ret = s32_regmap_read(pctldev, pin_id, &config);
> +       int ret;
> +
> +       ret = s32_regmap_read(pctldev, pin_id, &config);
> +       if (ret)
> +               return;
>
> -       if (!ret)
> -               seq_printf(s, "0x%x", config);
> +       seq_printf(s, "0x%x", config);
>  }
>
>  static void s32_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
> @@ -710,7 +716,7 @@ int s32_pinctrl_resume(struct device *dev)
>  }
>  #endif
>
> -static void s32_pinctrl_parse_groups(struct device_node *np,
> +static int s32_pinctrl_parse_groups(struct device_node *np,
>                                      struct s32_pin_group *grp,
>                                      struct s32_pinctrl_soc_info *info)
>  {
> @@ -722,21 +728,20 @@ static void s32_pinctrl_parse_groups(struct device_node *np,
>
>         dev = info->dev;
>
> -       dev_dbg(dev, "group: %s\n", np->name);
> +       dev_dbg(dev, "group: %pOFn\n", np);
>
>         /* Initialise group */
>         grp->name = np->name;
>
>         npins = of_property_count_elems_of_size(np, "pinmux", sizeof(u32));
> -
>         if (npins < 0) {
>                 dev_err(dev, "Failed to read 'pinmux' property in node %s.\n",
> -                       np->name);
> -               return;
> +                       grp->name);
> +               return -EINVAL;
>         }
>         if (!npins) {
> -               dev_err(dev, "The group %s has no pins.\n", np->name);
> -               return;
> +               dev_err(dev, "The group %s has no pins.\n", grp->name);
> +               return -EINVAL;
>         }
>
>         grp->npins = npins;
> @@ -745,12 +750,8 @@ static void s32_pinctrl_parse_groups(struct device_node *np,
>                                     sizeof(unsigned int), GFP_KERNEL);
>         grp->pin_sss = devm_kcalloc(info->dev, grp->npins,
>                                     sizeof(unsigned int), GFP_KERNEL);
> -
> -       if (!grp->pin_ids || !grp->pin_sss) {
> -               dev_err(dev, "Failed to allocate memory for the group %s.\n",
> -                       np->name);
> -               return;
> -       }
> +       if (!grp->pin_ids || !grp->pin_sss)
> +               return -ENOMEM;
>
>         i = 0;
>         of_property_for_each_u32(np, "pinmux", prop, p, pinmux) {
> @@ -761,9 +762,11 @@ static void s32_pinctrl_parse_groups(struct device_node *np,
>                         grp->pin_ids[i], grp->pin_sss[i]);
>                 i++;
>         }
> +
> +       return 0;
>  }
>
> -static void s32_pinctrl_parse_functions(struct device_node *np,
> +static int s32_pinctrl_parse_functions(struct device_node *np,
>                                         struct s32_pinctrl_soc_info *info,
>                                         u32 index)
>  {
> @@ -771,8 +774,9 @@ static void s32_pinctrl_parse_functions(struct device_node *np,
>         struct s32_pmx_func *func;
>         struct s32_pin_group *grp;
>         u32 i = 0;
> +       int ret = 0;
>
> -       dev_dbg(info->dev, "parse function(%d): %s\n", index, np->name);
> +       dev_dbg(info->dev, "parse function(%u): %pOFn\n", index, np);
>
>         func = &info->functions[index];
>
> @@ -780,18 +784,24 @@ static void s32_pinctrl_parse_functions(struct device_node *np,
>         func->name = np->name;
>         func->num_groups = of_get_child_count(np);
>         if (func->num_groups == 0) {
> -               dev_err(info->dev, "no groups defined in %s\n", np->full_name);
> -               return;
> +               dev_err(info->dev, "no groups defined in %pOF\n", np);
> +               return -EINVAL;
>         }
> -       func->groups = devm_kzalloc(info->dev,
> -                       func->num_groups * sizeof(char *), GFP_KERNEL);
> +       func->groups = devm_kcalloc(info->dev, func->num_groups,

> +                                   sizeof(char *), GFP_KERNEL);

sizeof(*func->groups) ?

> +       if (!func->groups)
> +               return -ENOMEM;
>
>         for_each_child_of_node(np, child) {
>                 func->groups[i] = child->name;
>                 grp = &info->groups[info->grp_index++];
> -               s32_pinctrl_parse_groups(child, grp, info);
> +               ret = s32_pinctrl_parse_groups(child, grp, info);
> +               if (ret)
> +                       return ret;
>                 i++;
>         }
> +
> +       return 0;
>  }
>
>  static int s32_pinctrl_probe_dt(struct platform_device *pdev,
> @@ -804,6 +814,7 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev,
>         struct regmap *map;
>         void __iomem *base;
>         int mem_regions = info->mem_regions;
> +       int ret;
>         u32 nfuncs = 0;
>         u32 i = 0;
>
> @@ -815,13 +826,12 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev,
>                 return -EINVAL;
>         }
>
> -       ipctl->regions = devm_kzalloc(&pdev->dev,
> -                                     mem_regions * sizeof(*(ipctl->regions)),
> -                                     GFP_KERNEL);
> +       ipctl->regions = devm_kcalloc(&pdev->dev, mem_regions,
> +                                     sizeof(*(ipctl->regions)), GFP_KERNEL);

Too many parentheses.

>         if (!ipctl->regions)
>                 return -ENOMEM;
>
> -       for (i = 0; i < mem_regions; ++i) {
> +       for (i = 0; i < mem_regions; i++) {
>                 base = devm_platform_get_and_ioremap_resource(pdev, i, &res);
>                 if (IS_ERR(base))
>                         return PTR_ERR(base);
> @@ -851,24 +861,26 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev,
>         }
>
>         info->nfunctions = nfuncs;
> -       info->functions = devm_kzalloc(&pdev->dev,
> -                                      nfuncs * sizeof(struct s32_pmx_func),
> -                                      GFP_KERNEL);
> +       info->functions = devm_kcalloc(&pdev->dev, nfuncs,
> +                                      sizeof(struct s32_pmx_func), GFP_KERNEL);

sizeof(*info->functions)

>         if (!info->functions)
>                 return -ENOMEM;
>
>         info->ngroups = 0;
>         for_each_child_of_node(np, child)
>                 info->ngroups += of_get_child_count(child);
> -       info->groups = devm_kzalloc(&pdev->dev,
> -                                   info->ngroups * sizeof(struct s32_pin_group),
> -                                   GFP_KERNEL);
> +
> +       info->groups = devm_kcalloc(&pdev->dev, info->ngroups,
> +                                   sizeof(struct s32_pin_group), GFP_KERNEL);

sizeof (*info->groups) ?

>         if (!info->groups)
>                 return -ENOMEM;
>
>         i = 0;
> -       for_each_child_of_node(np, child)
> -               s32_pinctrl_parse_functions(child, info, i++);
> +       for_each_child_of_node(np, child) {
> +               ret = s32_pinctrl_parse_functions(child, info, i++);
> +               if (ret)
> +                       return ret;
> +       }
>
>         return 0;
>  }
> @@ -923,12 +935,9 @@ int s32_pinctrl_probe(struct platform_device *pdev,
>
>         ipctl->pctl = devm_pinctrl_register(&pdev->dev, s32_pinctrl_desc,
>                                             ipctl);
> -
> -       if (IS_ERR(ipctl->pctl)) {
> -               dev_err(&pdev->dev, "could not register s32 pinctrl driver\n");
> -               return PTR_ERR(ipctl->pctl);
> -       }

> -

Don't drop this blank line, see below.

> +       if (IS_ERR(ipctl->pctl))
> +               return dev_err_probe(&pdev->dev, PTR_ERR(ipctl->pctl),
> +                                    "could not register s32 pinctrl driver\n");

It should be here.

>  #ifdef CONFIG_PM_SLEEP
>         saved_context = &ipctl->saved_context;
>         saved_context->pads =
> diff --git a/drivers/pinctrl/nxp/pinctrl-s32g2.c b/drivers/pinctrl/nxp/pinctrl-s32g2.c
> index 5028f4adc389..0b0b06f12b8a 100644
> --- a/drivers/pinctrl/nxp/pinctrl-s32g2.c
> +++ b/drivers/pinctrl/nxp/pinctrl-s32g2.c
> @@ -746,8 +746,8 @@ static int s32g_pinctrl_probe(struct platform_device *pdev)
>         if (!of_id)
>                 return -ENODEV;
>
> -       return s32_pinctrl_probe
> -                       (pdev, (struct s32_pinctrl_soc_info *) of_id->data);
> +       return s32_pinctrl_probe(pdev,
> +                                (struct s32_pinctrl_soc_info *) of_id->data);

No space after casting, but why is it using this interface? Convert to
of_device_get_match_data().
(In a separate patch before this one)

>  }
>
>  static const struct dev_pm_ops s32g_pinctrl_pm_ops = {
> @@ -757,14 +757,12 @@ static const struct dev_pm_ops s32g_pinctrl_pm_ops = {
>  static struct platform_driver s32g_pinctrl_driver = {
>         .driver = {
>                 .name = "s32g-siul2-pinctrl",
> -               .owner = THIS_MODULE,
>                 .of_match_table = s32_pinctrl_of_match,
> -               .pm = &s32g_pinctrl_pm_ops,
> +               .pm = pm_sleep_ptr(&s32g_pinctrl_pm_ops),
>                 .suppress_bind_attrs = true,
>         },
>         .probe = s32g_pinctrl_probe,
>  };
> -
>  builtin_platform_driver(s32g_pinctrl_driver);
>
>  MODULE_AUTHOR("Matthew Nunez <matthew.nunez@nxp.com>");
> --
> 2.37.3
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/3] pinctrl: s32cc: refactor pin config parsing

[Chester Lin]

Move common codes into smaller inline functions and remove some argument
handlings that are not actually used by either S32 MSCR register or generic
config params.

Signed-off-by: Chester Lin <clin@suse.com>
---
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 82 ++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 32 deletions(-)

diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
index 7a38e3216b0c..9508fc1e9a90 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
@@ -39,6 +39,9 @@
 #define S32_MSCR_ODE		BIT(20)
 #define S32_MSCR_OBE		BIT(21)
 
+#define S32_CFG_SET		true
+#define S32_CFG_CLR		false
+
 static struct regmap_config s32_regmap_config = {
 	.reg_bits = 32,
 	.val_bits = 32,
@@ -475,32 +478,57 @@ static int s32_get_slew_regval(int arg)
 	return -EINVAL;
 }
 
-static int s32_get_pin_conf(enum pin_config_param param, u32 arg,
-			    unsigned int *mask, unsigned int *config)
+static inline void s32_pin_config(unsigned int *mask, unsigned int *config,
+				  unsigned int bits, bool set)
+{
+	if (set)
+		*config |= bits;
+	else
+		*config &= ~bits;
+	*mask |= bits;
+}
+
+static inline void s32_pull_enable(enum pin_config_param param,
+				   unsigned int *mask, unsigned int *config)
+{
+
+	if (param == PIN_CONFIG_BIAS_PULL_UP) {
+		s32_pin_config(mask, config, S32_MSCR_PUS | S32_MSCR_PUE,
+			       S32_CFG_SET);
+	} else if (param == PIN_CONFIG_BIAS_PULL_DOWN) {
+		*config &= ~S32_MSCR_PUS;
+		*config |= S32_MSCR_PUE;
+		*mask |= S32_MSCR_PUS | S32_MSCR_PUE;
+	}
+}
+
+static inline void s32_pull_disable(unsigned int *mask, unsigned int *config)
+{
+	s32_pin_config(mask, config, S32_MSCR_PUS | S32_MSCR_PUE, S32_CFG_CLR);
+}
+
+static int s32_parse_pincfg(unsigned long pincfg, unsigned int *mask,
+			    unsigned int *config)
 {
+	enum pin_config_param param;
+	u32 arg;
 	int ret;
 
+	param = pinconf_to_config_param(pincfg);
+	arg = pinconf_to_config_argument(pincfg);
+
 	switch (param) {
 	/* All pins are persistent over suspend */
 	case PIN_CONFIG_PERSIST_STATE:
 		return 0;
 	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
-		*config |= S32_MSCR_ODE;
-		*mask |= S32_MSCR_ODE;
+		s32_pin_config(mask, config, S32_MSCR_ODE, S32_CFG_SET);
 		break;
 	case PIN_CONFIG_OUTPUT_ENABLE:
-		if (arg)
-			*config |= S32_MSCR_OBE;
-		else
-			*config &= ~S32_MSCR_OBE;
-		*mask |= S32_MSCR_OBE;
+		s32_pin_config(mask, config, S32_MSCR_OBE, S32_CFG_SET);
 		break;
 	case PIN_CONFIG_INPUT_ENABLE:
-		if (arg)
-			*config |= S32_MSCR_IBE;
-		else
-			*config &= ~S32_MSCR_IBE;
-		*mask |= S32_MSCR_IBE;
+		s32_pin_config(mask, config, S32_MSCR_IBE, S32_CFG_SET);
 		break;
 	case PIN_CONFIG_SLEW_RATE:
 		ret = s32_get_slew_regval(arg);
@@ -510,25 +538,17 @@ static int s32_get_pin_conf(enum pin_config_param param, u32 arg,
 		*mask |= S32_MSCR_SRE(~0);
 		break;
 	case PIN_CONFIG_BIAS_PULL_UP:
-		if (arg)
-			*config |= S32_MSCR_PUS;
-		else
-			*config &= ~S32_MSCR_PUS;
-		fallthrough;
 	case PIN_CONFIG_BIAS_PULL_DOWN:
-		if (arg)
-			*config |= S32_MSCR_PUE;
-		else
-			*config &= ~S32_MSCR_PUE;
-		*mask |= S32_MSCR_PUE | S32_MSCR_PUS;
+		s32_pull_enable(param, mask, config);
 		break;
 	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
-		*config &= ~(S32_MSCR_ODE | S32_MSCR_OBE | S32_MSCR_IBE);
-		*mask |= S32_MSCR_ODE | S32_MSCR_OBE | S32_MSCR_IBE;
-		fallthrough;
+		s32_pin_config(mask, config,
+			       S32_MSCR_ODE | S32_MSCR_OBE | S32_MSCR_IBE,
+			       S32_CFG_CLR);
+		s32_pull_disable(mask, config);
+		break;
 	case PIN_CONFIG_BIAS_DISABLE:
-		*config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
-		*mask |= S32_MSCR_PUS | S32_MSCR_PUE;
+		s32_pull_disable(mask, config);
 		break;
 	default:
 		return -EOPNOTSUPP;
@@ -554,9 +574,7 @@ static int s32_pinconf_mscr_update(struct pinctrl_dev *pctldev,
 		pin_get_name(pctldev, pin_id), num_configs);
 
 	for (i = 0; i < num_configs; i++) {
-		ret = s32_get_pin_conf(pinconf_to_config_param(configs[i]),
-				       pinconf_to_config_argument(configs[i]),
-				       &mask, &config);
+		ret = s32_parse_pincfg(configs[i], &mask, &config);
 		if (ret)
 			return ret;
 	}
-- 
2.37.3

Re: [PATCH 2/3] pinctrl: s32cc: refactor pin config parsing

[Andy Shevchenko]

On Tue, Mar 14, 2023 at 3:47 PM Chester Lin <clin@suse.com> wrote:
>
> Move common codes into smaller inline functions and remove some argument
> handlings that are not actually used by either S32 MSCR register or generic
> config params.

...

> +#define S32_CFG_SET            true
> +#define S32_CFG_CLR            false

Have no value, use boolean values directly.

...

> +static inline void s32_pin_config(unsigned int *mask, unsigned int *config,
> +                                 unsigned int bits, bool set)
> +{
> +       if (set)
> +               *config |= bits;
> +       else
> +               *config &= ~bits;
> +       *mask |= bits;
> +}
> +
> +static inline void s32_pull_enable(enum pin_config_param param,
> +                                  unsigned int *mask, unsigned int *config)
> +{

> +

Unneeded blank line

> +       if (param == PIN_CONFIG_BIAS_PULL_UP) {
> +               s32_pin_config(mask, config, S32_MSCR_PUS | S32_MSCR_PUE,
> +                              S32_CFG_SET);
> +       } else if (param == PIN_CONFIG_BIAS_PULL_DOWN) {
> +               *config &= ~S32_MSCR_PUS;
> +               *config |= S32_MSCR_PUE;
> +               *mask |= S32_MSCR_PUS | S32_MSCR_PUE;
> +       }

This looks intransparent.

Just use your common sense and write it without that s32_pin_config() helper.

> +}
> +
> +static inline void s32_pull_disable(unsigned int *mask, unsigned int *config)
> +{
> +       s32_pin_config(mask, config, S32_MSCR_PUS | S32_MSCR_PUE, S32_CFG_CLR);
> +}

This should go with above together, just use switch and make the
configuration based on it.
Before the switch-case, clear what has to be cleared for all cases
(some cases may override some bits afterwards), set what has to be set
for all cases (some cases may override some bits afterwards) and
adjust.

Look into pinctrl-intel.c for the rough example.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/3] pinctrl: s32cc: embed generic struct pingroup and pinfunction

[Chester Lin]

Use generic data structure to describe pin control functions and groups in
S32 SoC family and drop duplicated struct members.

Signed-off-by: Chester Lin <clin@suse.com>
---
 drivers/pinctrl/nxp/pinctrl-s32.h   | 22 +++-----
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 78 ++++++++++++++++-------------
 2 files changed, 49 insertions(+), 51 deletions(-)

diff --git a/drivers/pinctrl/nxp/pinctrl-s32.h b/drivers/pinctrl/nxp/pinctrl-s32.h
index 545bf16b988d..1a0aa1995908 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32.h
+++ b/drivers/pinctrl/nxp/pinctrl-s32.h
@@ -15,29 +15,21 @@ struct platform_device;
 
 /**
  * struct s32_pin_group - describes an S32 pin group
- * @name: the name of this specific pin group
- * @npins: the number of pins in this group array, i.e. the number of
- *         elements in pin_ids and pin_sss so we can iterate over that array
- * @pin_ids: an array of pin IDs in this group
- * @pin_sss: an array of source signal select configs paired with pin_ids
+ * @data: generic data describes group name, number of pins, and a pin array in
+	this group.
+ * @pin_sss: an array of source signal select configs paired with pin array.
  */
 struct s32_pin_group {
-	const char *name;
-	unsigned int npins;
-	unsigned int *pin_ids;
+	struct pingroup data;
 	unsigned int *pin_sss;
 };
 
 /**
- * struct s32_pmx_func - describes S32 pinmux functions
- * @name: the name of this specific function
- * @groups: corresponding pin groups
- * @num_groups: the number of groups
+ * struct s32_pmx_func - describes an S32 pinmux function
+ * @data: generic data to describe function name and associated groups.
  */
 struct s32_pmx_func {
-	const char *name;
-	const char **groups;
-	unsigned int num_groups;
+	struct pinfunction data;
 };
 
 /**
diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
index 9508fc1e9a90..76442c1bc7be 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
@@ -191,7 +191,7 @@ static const char *s32_get_group_name(struct pinctrl_dev *pctldev,
 	struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
 	const struct s32_pinctrl_soc_info *info = ipctl->info;
 
-	return info->groups[selector].name;
+	return info->groups[selector].data.name;
 }
 
 static int s32_get_group_pins(struct pinctrl_dev *pctldev,
@@ -201,8 +201,8 @@ static int s32_get_group_pins(struct pinctrl_dev *pctldev,
 	struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
 	const struct s32_pinctrl_soc_info *info = ipctl->info;
 
-	*pins = info->groups[selector].pin_ids;
-	*npins = info->groups[selector].npins;
+	*pins = info->groups[selector].data.pins;
+	*npins = info->groups[selector].data.npins;
 
 	return 0;
 }
@@ -317,23 +317,23 @@ static int s32_pmx_set(struct pinctrl_dev *pctldev, unsigned int selector,
 	grp = &info->groups[group];
 
 	dev_dbg(ipctl->dev, "set mux for function %s group %s\n",
-		info->functions[selector].name, grp->name);
+		info->functions[selector].data.name, grp->data.name);
 
 	/* Check beforehand so we don't have a partial config. */
-	for (i = 0; i < grp->npins; i++) {
-		if (s32_check_pin(pctldev, grp->pin_ids[i]) != 0) {
+	for (i = 0; i < grp->data.npins; i++) {
+		if (s32_check_pin(pctldev, grp->data.pins[i]) != 0) {
 			dev_err(info->dev, "invalid pin: %u in group: %u\n",
-				grp->pin_ids[i], group);
+				grp->data.pins[i], group);
 			return -EINVAL;
 		}
 	}
 
-	for (i = 0, ret = 0; i < grp->npins && !ret; i++) {
-		ret = s32_regmap_update(pctldev, grp->pin_ids[i],
+	for (i = 0, ret = 0; i < grp->data.npins && !ret; i++) {
+		ret = s32_regmap_update(pctldev, grp->data.pins[i],
 					S32_MSCR_SSS_MASK, grp->pin_sss[i]);
 		if (ret) {
 			dev_err(info->dev, "Failed to set pin %u\n",
-				grp->pin_ids[i]);
+				grp->data.pins[i]);
 			return ret;
 		}
 	}
@@ -355,7 +355,7 @@ static const char *s32_pmx_get_func_name(struct pinctrl_dev *pctldev,
 	struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
 	const struct s32_pinctrl_soc_info *info = ipctl->info;
 
-	return info->functions[selector].name;
+	return info->functions[selector].data.name;
 }
 
 static int s32_pmx_get_groups(struct pinctrl_dev *pctldev,
@@ -366,8 +366,8 @@ static int s32_pmx_get_groups(struct pinctrl_dev *pctldev,
 	struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
 	const struct s32_pinctrl_soc_info *info = ipctl->info;
 
-	*groups = info->functions[selector].groups;
-	*num_groups = info->functions[selector].num_groups;
+	*groups = info->functions[selector].data.groups;
+	*num_groups = info->functions[selector].data.ngroups;
 
 	return 0;
 }
@@ -611,8 +611,8 @@ static int s32_pconf_group_set(struct pinctrl_dev *pctldev, unsigned int selecto
 	int i, ret;
 
 	grp = &info->groups[selector];
-	for (i = 0; i < grp->npins; i++) {
-		ret = s32_pinconf_mscr_update(pctldev, grp->pin_ids[i],
+	for (i = 0; i < grp->data.npins; i++) {
+		ret = s32_pinconf_mscr_update(pctldev, grp->data.pins[i],
 					      configs, num_configs);
 		if (ret)
 			return ret;
@@ -646,9 +646,9 @@ static void s32_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
 
 	seq_puts(s, "\n");
 	grp = &info->groups[selector];
-	for (i = 0; i < grp->npins; i++) {
-		name = pin_get_name(pctldev, grp->pin_ids[i]);
-		ret = s32_regmap_read(pctldev, grp->pin_ids[i], &config);
+	for (i = 0; i < grp->data.npins; i++) {
+		name = pin_get_name(pctldev, grp->data.pins[i]);
+		ret = s32_regmap_read(pctldev, grp->data.pins[i], &config);
 		if (ret)
 			return;
 		seq_printf(s, "%s: 0x%x\n", name, config);
@@ -741,6 +741,7 @@ static int s32_pinctrl_parse_groups(struct device_node *np,
 	const __be32 *p;
 	struct device *dev;
 	struct property *prop;
+	unsigned int *pins, *sss;
 	int i, npins;
 	u32 pinmux;
 
@@ -749,38 +750,40 @@ static int s32_pinctrl_parse_groups(struct device_node *np,
 	dev_dbg(dev, "group: %pOFn\n", np);
 
 	/* Initialise group */
-	grp->name = np->name;
+	grp->data.name = np->name;
 
 	npins = of_property_count_elems_of_size(np, "pinmux", sizeof(u32));
 	if (npins < 0) {
 		dev_err(dev, "Failed to read 'pinmux' property in node %s.\n",
-			grp->name);
+			grp->data.name);
 		return -EINVAL;
 	}
 	if (!npins) {
-		dev_err(dev, "The group %s has no pins.\n", grp->name);
+		dev_err(dev, "The group %s has no pins.\n", grp->data.name);
 		return -EINVAL;
 	}
 
-	grp->npins = npins;
+	grp->data.npins = npins;
 
-	grp->pin_ids = devm_kcalloc(info->dev, grp->npins,
+	pins = devm_kcalloc(info->dev, grp->data.npins,
 				    sizeof(unsigned int), GFP_KERNEL);
-	grp->pin_sss = devm_kcalloc(info->dev, grp->npins,
+	sss = devm_kcalloc(info->dev, grp->data.npins,
 				    sizeof(unsigned int), GFP_KERNEL);
-	if (!grp->pin_ids || !grp->pin_sss)
+	if (!pins || !sss)
 		return -ENOMEM;
 
 	i = 0;
 	of_property_for_each_u32(np, "pinmux", prop, p, pinmux) {
-		grp->pin_ids[i] = get_pin_no(pinmux);
-		grp->pin_sss[i] = get_pin_func(pinmux);
+		pins[i] = get_pin_no(pinmux);
+		sss[i] = get_pin_func(pinmux);
 
-		dev_dbg(info->dev, "pin-id: 0x%x, sss: 0x%x",
-			grp->pin_ids[i], grp->pin_sss[i]);
+		dev_dbg(info->dev, "pin: 0x%x, sss: 0x%x", pins[i], sss[i]);
 		i++;
 	}
 
+	grp->data.pins = pins;
+	grp->pin_sss = sss;
+
 	return 0;
 }
 
@@ -791,6 +794,7 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
 	struct device_node *child;
 	struct s32_pmx_func *func;
 	struct s32_pin_group *grp;
+	char **groups;
 	u32 i = 0;
 	int ret = 0;
 
@@ -799,19 +803,19 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
 	func = &info->functions[index];
 
 	/* Initialise function */
-	func->name = np->name;
-	func->num_groups = of_get_child_count(np);
-	if (func->num_groups == 0) {
+	func->data.name = np->name;
+	func->data.ngroups = of_get_child_count(np);
+	if (func->data.ngroups == 0) {
 		dev_err(info->dev, "no groups defined in %pOF\n", np);
 		return -EINVAL;
 	}
-	func->groups = devm_kcalloc(info->dev, func->num_groups,
-				    sizeof(char *), GFP_KERNEL);
-	if (!func->groups)
+	groups = devm_kcalloc(info->dev, func->data.ngroups,
+			      sizeof(char *), GFP_KERNEL);
+	if (!groups)
 		return -ENOMEM;
 
 	for_each_child_of_node(np, child) {
-		func->groups[i] = child->name;
+		groups[i] = (char *)child->name;
 		grp = &info->groups[info->grp_index++];
 		ret = s32_pinctrl_parse_groups(child, grp, info);
 		if (ret)
@@ -819,6 +823,8 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
 		i++;
 	}
 
+	func->data.groups = (const char **)groups;
+
 	return 0;
 }
 
-- 
2.37.3

Re: [PATCH 3/3] pinctrl: s32cc: embed generic struct pingroup and pinfunction

[Andy Shevchenko]

On Tue, Mar 14, 2023 at 3:47 PM Chester Lin <clin@suse.com> wrote:
>
> Use generic data structure to describe pin control functions and groups in
> S32 SoC family and drop duplicated struct members.

...

>  struct s32_pmx_func {
> -       const char *name;
> -       const char **groups;
> -       unsigned int num_groups;
> +       struct pinfunction data;
>  };

Since you have a single driver with this, just kill the entire custom
structure.

The way it's done in the pinctrl-intel.c is due to dozens of drivers
sharing the same data type and hence converting that will provoke
quite a noise for no benefit. Here it's not the case, so just kill it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] pinctrl: s32: driver improvements and generic struct use

[Andy Shevchenko]

On Tue, Mar 14, 2023 at 3:46 PM Chester Lin <clin@suse.com> wrote:
>
> Hello,
>
> This patch series contains some improvements for s32 pinctrl drivers suggested
> by upstream[1], such as
>
>   - Fix error shadowings and improve return value handlings.
>   - Fix print format.
>   - Remove unnecessary blanks.
>   - Use proper macros and helpers to simplify codes.
>   - Refactor config param parsing and remove config arguments that are never used.
>   - Use generic struct pingroup and struct pinfunction to describe pin data.

Overall it looks not bad, thank you for doing this.
Individual patches have been reviewed and commented accordingly.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] pinctrl: s32: driver improvements and generic struct use

[Chester Lin]

Hi Andy,

On Tue, Mar 14, 2023 at 07:21:55PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 14, 2023 at 3:46 PM Chester Lin <clin@suse.com> wrote:
> >
> > Hello,
> >
> > This patch series contains some improvements for s32 pinctrl drivers suggested
> > by upstream[1], such as
> >
> >   - Fix error shadowings and improve return value handlings.
> >   - Fix print format.
> >   - Remove unnecessary blanks.
> >   - Use proper macros and helpers to simplify codes.
> >   - Refactor config param parsing and remove config arguments that are never used.
> >   - Use generic struct pingroup and struct pinfunction to describe pin data.
> 
> Overall it looks not bad, thank you for doing this.
> Individual patches have been reviewed and commented accordingly.

Thank you for reviewing this patch. I will fix the rest in v2 soon.

Regards,
Chester

> 
> -- 
> With Best Regards,
> Andy Shevchenko