Skip to contentSkip to conversation

pinctrl: s32: driver improvements and generic struct use

PATCH Superseded Chester Lin 2 participants 13 messages Mar 20, 2023 – Mar 21, 2023

Subsystems (4)

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v2 cover pinctrl: s32: driver improvements and generic struct use

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/[email protected]/

Changes in v2:
- Use of_device_get_match_data() to get matched of_device_id data.
- Enhance sizeof() arguments.
- Fix blanks and remove unnecessary parentheses.
- Drop unnecessary marcos and s32_pin_config() implemented in v1 and set/clear mask/config values transparently.
- Put pull-function related cases together in s32_pin_set_pull().
- Simply use generic 'struct pinfunction' rather than having extra 'struct s32_pmx_func'.

Chester Lin (4):
pinctrl: s32: use of_device_get_match_data() to get device data
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   |  26 +--
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 261 +++++++++++++++-------------
 drivers/pinctrl/nxp/pinctrl-s32g2.c |  14 +-
 3 files changed, 152 insertions(+), 149 deletions(-)
Signature · 7 lines
2.37.3


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#1 lore raw

PATCH v2 1/4 pinctrl: s32: use of_device_get_match_data() to get device data

Instead of relying on of_match_device(), using of_device_get_match_data() can simplify implementation and avoid code duplication.

---
(An initial version since v2)

 drivers/pinctrl/nxp/pinctrl-s32g2.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/pinctrl/nxp/pinctrl-s32g2.c +4 -6
index 5028f4adc389..f99f88615ef6 100644
@@ -740,14 +740,12 @@ MODULE_DEVICE_TABLE(of, s32_pinctrl_of_match);
 
 static int s32g_pinctrl_probe(struct platform_device *pdev)
 {
-	const struct of_device_id *of_id =
-		of_match_device(s32_pinctrl_of_match, &pdev->dev);
+	struct s32_pinctrl_soc_info *soc_info;
 
-	if (!of_id)
-		return -ENODEV;
+	soc_info = (struct s32_pinctrl_soc_info *)
+			of_device_get_match_data(&pdev->dev);
 
-	return s32_pinctrl_probe
-			(pdev, (struct s32_pinctrl_soc_info *) of_id->data);
+	return s32_pinctrl_probe(pdev, soc_info);
 }
 
 static const struct dev_pm_ops s32g_pinctrl_pm_ops = {
Signature · 7 lines
2.37.3


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#2 lore raw

Re: [PATCH v2 1/4] pinctrl: s32: use of_device_get_match_data() to get device data

Quoted 3 lines · Chester Lin

Instead of relying on of_match_device(), using of_device_get_match_data() can simplify implementation and avoid code duplication.

Suggested-by?

Quoted 1 line

...

drivers/pinctrl/nxp/pinctrl-s32g2.c MODULE_DEVICE_TABLE(of, s32_pinctrl_of_match); · line 746

Quoted 2 lines
drivers/pinctrl/nxp/pinctrl-s32g2.c +2 -0
+       soc_info = (struct s32_pinctrl_soc_info *)
+                       of_device_get_match_data(&pdev->dev);

Drop the ugly casting, it's not needed.

Signature · 7 lines
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#3 lore raw

Re: [PATCH v2 1/4] pinctrl: s32: use of_device_get_match_data() to get device data

Hi Andy,

Thank you for reviewing this series!

Quoted 7 lines · Andy Shevchenko
Quoted 3 lines · Chester Lin

Instead of relying on of_match_device(), using of_device_get_match_data() can simplify implementation and avoid code duplication.

Suggested-by?

Sorry for the miss. I will fix it in v3.

Quoted 9 lines
Quoted 1 line

...

Review

Quoted 2 lines
(quoted diff) +2 -0
+       soc_info = (struct s32_pinctrl_soc_info *)
+                       of_device_get_match_data(&pdev->dev);

Drop the ugly casting, it's not needed.

Actually it's used for suppressing the compiler warning since some members in this soc_info need to be filled by pinctrl-s32cc.

drivers/pinctrl/nxp/pinctrl-s32g2.c:745:18: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]

I am thinking of allocating & copying a dedicate struct in pinctrl-s32cc.c rather than reusing the .data attached on of_device_id in order to solve this warning properly. Here is an example based on this v2:

drivers/pinctrl/nxp/pinctrl-s32.h +1 -1
index 2f7aecd462e4..f3a0b579757c 100644
@@ -51,7 +51,7 @@ struct s32_pinctrl_soc_info {
 #define S32_PIN_RANGE(_start, _end) { .start = _start, .end = _end }
 
 int s32_pinctrl_probe(struct platform_device *pdev,
-			struct s32_pinctrl_soc_info *info);
+		      const struct s32_pinctrl_soc_info *soc_data);
 int s32_pinctrl_resume(struct device *dev);
 int s32_pinctrl_suspend(struct device *dev);
 #endif /* __DRIVERS_PINCTRL_S32_H */
drivers/pinctrl/nxp/pinctrl-s32cc.c +10 -2
index 4ed0cc905232..4c70ab753d15 100644
@@ -899,20 +899,28 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev,
 }
 
 int s32_pinctrl_probe(struct platform_device *pdev,
-		      struct s32_pinctrl_soc_info *info)
+		      const struct s32_pinctrl_soc_info *soc_data)
 {
 	struct s32_pinctrl *ipctl;
 	int ret;
 	struct pinctrl_desc *s32_pinctrl_desc;
+	struct s32_pinctrl_soc_info *info;
 #ifdef CONFIG_PM_SLEEP
 	struct s32_pinctrl_context *saved_context;
 #endif
 
-	if (!info || !info->pins || !info->npins) {
+	if (!soc_data || !soc_data->pins || !soc_data->npins) {
 		dev_err(&pdev->dev, "wrong pinctrl info\n");
 		return -EINVAL;
 	}
 
+
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	memcpy(info, soc_data, sizeof(*info));
+
 	info->dev = &pdev->dev;
 
 	/* Create state holders etc for this driver */
drivers/pinctrl/nxp/pinctrl-s32g2.c +2 -3
index 9f521312f768..0a49205414eb 100644
@@ -740,10 +740,9 @@ MODULE_DEVICE_TABLE(of, s32_pinctrl_of_match);
 
 static int s32g_pinctrl_probe(struct platform_device *pdev)
 {
-	struct s32_pinctrl_soc_info *soc_info;
+	const struct s32_pinctrl_soc_info *soc_info;
 
-	soc_info = (struct s32_pinctrl_soc_info *)
-			of_device_get_match_data(&pdev->dev);
+	soc_info = of_device_get_match_data(&pdev->dev);
 
 	return s32_pinctrl_probe(pdev, soc_info);
 }
Quoted 3 lines
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#4 lore raw

Re: [PATCH v2 1/4] pinctrl: s32: use of_device_get_match_data() to get device data

Quoted 2 lines · Chester Lin
Quoted 1 line · Andy Shevchenko

On Mon, Mar 20, 2023 at 6:39 PM Chester Lin <[email protected]> wrote:

...

Quoted 7 lines
Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+       soc_info = (struct s32_pinctrl_soc_info *)
+                       of_device_get_match_data(&pdev->dev);

Drop the ugly casting, it's not needed.

Actually it's used for suppressing the compiler warning since some members in this soc_info need to be filled by pinctrl-s32cc.

Yes, that's one way to solve this.

...

drivers/pinctrl/nxp/pinctrl-s32cc.c static int s32_pinctrl_probe_dt(struct platform_device *pdev, · line 922

Quoted 5 lines
drivers/pinctrl/nxp/pinctrl-s32cc.c +5 -0
+       info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+       if (!info)
+               return -ENOMEM;
+
+       memcpy(info, soc_data, sizeof(*info));

Right, but use devm_kmemdup() instead.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#5 lore raw

PATCH v2 2/4 pinctrl: s32: refine error/return/config checks and simplify driver codes

In reply to 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.

---
Changes in v2:
- Enhance sizeof() arguments.
- Fix blanks and remove unnecessary parentheses.
- Move s32g_pinctrl_probe() changes to the patch [1/4].

 drivers/pinctrl/nxp/pinctrl-s32cc.c | 141 +++++++++++++++-------------
 drivers/pinctrl/nxp/pinctrl-s32g2.c |   4 +-
 2 files changed, 76 insertions(+), 69 deletions(-)
drivers/pinctrl/nxp/pinctrl-s32cc.c +75 -66
index e1da332433a3..f698e1a240ef 100644
@@ -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,23 +406,20 @@ 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,
 						 gpio_pin->config);
 			if (ret != 0)
 				goto unlock;
 
-			list_del(pos);
+			list_del(&gpio_pin->list);
 			kfree(gpio_pin);
 			break;
 		}
@@ -461,7 +463,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 +545,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 +563,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 %u cfg 0x%x\n", pin_id, config);
 
-	dev_dbg(ipctl->dev, "update: pin %d 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 +606,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;
 
-	if (!ret)
-		seq_printf(s, "0x%x", config);
+	ret = s32_regmap_read(pctldev, pin_id, &config);
+	if (ret)
+		return;
+
+	seq_printf(s, "0x%x", config);
 }
 
 static void s32_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
@@ -710,7 +715,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 +727,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 +749,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 +761,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 +773,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 +783,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(*func->groups), 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 +813,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 +825,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 +860,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(*info->functions), 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(*info->groups), 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,11 +934,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;
drivers/pinctrl/nxp/pinctrl-s32g2.c +1 -3
index f99f88615ef6..9f521312f768 100644
@@ -755,14 +755,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 <[email protected]>");
Signature · 1 line
2.37.3
#6 lore raw

PATCH v2 3/4 pinctrl: s32cc: refactor pin config parsing

In reply to 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.

---
Changes in v2:
- Drop unnecessary marcos and s32_pin_config() implemented in v1 and set/clear mask/config values transparently.
- Put pull-function related cases together in s32_pin_set_pull().

 drivers/pinctrl/nxp/pinctrl-s32cc.c | 62 +++++++++++++++++------------
 1 file changed, 36 insertions(+), 26 deletions(-)
drivers/pinctrl/nxp/pinctrl-s32cc.c +36 -26
index f698e1a240ef..cb8a0844c0fa 100644
@@ -474,11 +474,38 @@ 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_set_pull(enum pin_config_param param,
+				   unsigned int *mask, unsigned int *config)
 {
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		*config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		*config |= S32_MSCR_PUS | S32_MSCR_PUE;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		*config &= ~S32_MSCR_PUS;
+		*config |= S32_MSCR_PUE;
+		break;
+	default:
+		return;
+	}
+
+	*mask |= S32_MSCR_PUS | S32_MSCR_PUE;
+}
+
+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:
@@ -488,17 +515,11 @@ static int s32_get_pin_conf(enum pin_config_param param, u32 arg,
 		*mask |= S32_MSCR_ODE;
 		break;
 	case PIN_CONFIG_OUTPUT_ENABLE:
-		if (arg)
-			*config |= S32_MSCR_OBE;
-		else
-			*config &= ~S32_MSCR_OBE;
+		*config |= S32_MSCR_OBE;
 		*mask |= S32_MSCR_OBE;
 		break;
 	case PIN_CONFIG_INPUT_ENABLE:
-		if (arg)
-			*config |= S32_MSCR_IBE;
-		else
-			*config &= ~S32_MSCR_IBE;
+		*config |= S32_MSCR_IBE;
 		*mask |= S32_MSCR_IBE;
 		break;
 	case PIN_CONFIG_SLEW_RATE:
@@ -509,25 +530,16 @@ 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_pin_set_pull(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_set_pull(param, mask, config);
+		break;
 	case PIN_CONFIG_BIAS_DISABLE:
-		*config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
-		*mask |= S32_MSCR_PUS | S32_MSCR_PUE;
+		s32_pin_set_pull(param, mask, config);
 		break;
 	default:
 		return -EOPNOTSUPP;
@@ -553,9 +565,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;
 	}
Signature · 7 lines
2.37.3


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#7 lore raw

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

Quoted 4 lines · 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.

...

Quoted 6 lines

case PIN_CONFIG_OUTPUT_ENABLE:

drivers/pinctrl/nxp/pinctrl-s32cc.c +1 -4
-               if (arg)
-                       *config |= S32_MSCR_OBE;
-               else
-                       *config &= ~S32_MSCR_OBE;
+               *config |= S32_MSCR_OBE;

drivers/pinctrl/nxp/pinctrl-s32cc.c static int s32_get_pin_conf(enum pin_config_param param, u32 arg, · line 524

Quoted 10 lines
drivers/pinctrl/nxp/pinctrl-s32cc.c +1 -4
                *mask |= S32_MSCR_OBE;
                break;
        case PIN_CONFIG_INPUT_ENABLE:
-               if (arg)
-                       *config |= S32_MSCR_IBE;
-               else
-                       *config &= ~S32_MSCR_IBE;
+               *config |= S32_MSCR_IBE;
                *mask |= S32_MSCR_IBE;
                break;

Isn't it a regression here?
Otherwise needs an explicit explanation in the commit message on what's going on here and why it's not a regression.

...

drivers/pinctrl/nxp/pinctrl-s32cc.c static int s32_get_pin_conf(enum pin_config_param param, u32 arg, · line 543

Quoted 5 lines

case PIN_CONFIG_BIAS_DISABLE:

drivers/pinctrl/nxp/pinctrl-s32cc.c +1 -2
-               *config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
-               *mask |= S32_MSCR_PUS | S32_MSCR_PUE;
+               s32_pin_set_pull(param, mask, config);
                break;

This now can be unified with PU and PD cases above.

Signature · 7 lines
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#8 lore raw

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

Quoted 25 lines · Andy Shevchenko
Quoted 4 lines · 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.

...

Quoted 6 lines

case PIN_CONFIG_OUTPUT_ENABLE:

drivers/pinctrl/nxp/pinctrl-s32cc.c +1 -4
-               if (arg)
-                       *config |= S32_MSCR_OBE;
-               else
-                       *config &= ~S32_MSCR_OBE;
+               *config |= S32_MSCR_OBE;

drivers/pinctrl/nxp/pinctrl-s32cc.c static int s32_get_pin_conf(enum pin_config_param param, u32 arg, · line 524

Quoted 10 lines
drivers/pinctrl/nxp/pinctrl-s32cc.c +1 -4
                *mask |= S32_MSCR_OBE;
                break;
        case PIN_CONFIG_INPUT_ENABLE:
-               if (arg)
-                       *config |= S32_MSCR_IBE;
-               else
-                       *config &= ~S32_MSCR_IBE;
+               *config |= S32_MSCR_IBE;
                *mask |= S32_MSCR_IBE;
                break;
Quoted 3 lines

Isn't it a regression here?
Otherwise needs an explicit explanation in the commit message on what's going on here and why it's not a regression.

Oops, it's wrong implementation. The argument checks of OUTPUT_EN and INPUT_EN shouldn't be dropped. Thanks for the reminder and I will fix it.

Regards,
Chester

Quoted 14 lines

...

drivers/pinctrl/nxp/pinctrl-s32cc.c static int s32_get_pin_conf(enum pin_config_param param, u32 arg, · line 543

Quoted 5 lines

case PIN_CONFIG_BIAS_DISABLE:

drivers/pinctrl/nxp/pinctrl-s32cc.c +1 -2
-               *config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
-               *mask |= S32_MSCR_PUS | S32_MSCR_PUE;
+               s32_pin_set_pull(param, mask, config);
                break;

This now can be unified with PU and PD cases above.

Signature · 2 lines
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#9 lore raw

PATCH v2 4/4 pinctrl: s32cc: embed generic struct pingroup and pinfunction

In reply to Chester Lin

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

---
Changes in v2:
- Simply use generic 'struct pinfunction' rather than having extra 'struct s32_pmx_func'.

 drivers/pinctrl/nxp/pinctrl-s32.h   | 26 ++--------
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 76 +++++++++++++++--------------
 2 files changed, 45 insertions(+), 57 deletions(-)
drivers/pinctrl/nxp/pinctrl-s32.h +5 -21
index 545bf16b988d..2f7aecd462e4 100644
@@ -15,31 +15,15 @@ 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 {
-	const char *name;
-	const char **groups;
-	unsigned int num_groups;
-};
-
 /**
  * struct s32_pin_range - pin ID range for each memory region.
  * @start: start pin ID
@@ -56,7 +40,7 @@ struct s32_pinctrl_soc_info {
 	unsigned int npins;
 	struct s32_pin_group *groups;
 	unsigned int ngroups;
-	struct s32_pmx_func *functions;
+	struct pinfunction *functions;
 	unsigned int nfunctions;
 	unsigned int grp_index;
 	const struct s32_pin_range *mem_pin_ranges;
drivers/pinctrl/nxp/pinctrl-s32cc.c +40 -36

178 lines not on this page · raw message

Signature · 7 lines
2.37.3


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Reviewed-by on v2 4/4
#10 lore raw

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

Quoted 3 lines · Chester Lin

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

Not sure about the need of the casting, see below, otherwise LGTM.

Quoted 244 lines

---
Changes in v2:
- Simply use generic 'struct pinfunction' rather than having extra 'struct s32_pmx_func'.

 drivers/pinctrl/nxp/pinctrl-s32.h   | 26 ++--------
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 76 +++++++++++++++--------------
 2 files changed, 45 insertions(+), 57 deletions(-)
drivers/pinctrl/nxp/pinctrl-s32.h +5 -21
index 545bf16b988d..2f7aecd462e4 100644
@@ -15,31 +15,15 @@ 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 {
-       const char *name;
-       const char **groups;
-       unsigned int num_groups;
-};
-
 /**
  * struct s32_pin_range - pin ID range for each memory region.
  * @start: start pin ID
@@ -56,7 +40,7 @@ struct s32_pinctrl_soc_info {
        unsigned int npins;
        struct s32_pin_group *groups;
        unsigned int ngroups;
-       struct s32_pmx_func *functions;
+       struct pinfunction *functions;
        unsigned int nfunctions;
        unsigned int grp_index;
        const struct s32_pin_range *mem_pin_ranges;
drivers/pinctrl/nxp/pinctrl-s32cc.c +38 -36
index cb8a0844c0fa..4ed0cc905232 100644
@@ -188,7 +188,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,
@@ -198,8 +198,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;
 }
@@ -314,23 +314,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].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;
                }
        }
@@ -364,7 +364,7 @@ static int s32_pmx_get_groups(struct pinctrl_dev *pctldev,
        const struct s32_pinctrl_soc_info *info = ipctl->info;
 
        *groups = info->functions[selector].groups;
-       *num_groups = info->functions[selector].num_groups;
+       *num_groups = info->functions[selector].ngroups;
 
        return 0;
 }
@@ -602,8 +602,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;
@@ -637,9 +637,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);
@@ -732,6 +732,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;
 
@@ -740,38 +741,38 @@ 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,
-                                   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)
+       pins = devm_kcalloc(info->dev, npins, sizeof(*pins), GFP_KERNEL);
+       sss = devm_kcalloc(info->dev, npins, sizeof(*sss), GFP_KERNEL);
+       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;
 }
 
@@ -780,8 +781,9 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
                                        u32 index)
 {
        struct device_node *child;
-       struct s32_pmx_func *func;
+       struct pinfunction *func;
        struct s32_pin_group *grp;
+       char **groups;
        u32 i = 0;
        int ret = 0;
 
@@ -791,18 +793,18 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
 
        /* Initialise function */
        func->name = np->name;
-       func->num_groups = of_get_child_count(np);
-       if (func->num_groups == 0) {
+       func->ngroups = of_get_child_count(np);
+       if (func->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(*func->groups), GFP_KERNEL);
-       if (!func->groups)
+       groups = devm_kcalloc(info->dev, func->ngroups,
+                             sizeof(*func->groups), 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)

drivers/pinctrl/nxp/pinctrl-s32cc.c static int s32_pinctrl_parse_functions(struct device_node *np, · line 815

Quoted 4 lines
drivers/pinctrl/nxp/pinctrl-s32cc.c +1 -0
@@ -810,6 +812,8 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
                i++;
        }
 
+       func->groups = (const char **)groups;

Hmm... Why is casting needed?

Quoted 2 lines
drivers/pinctrl/nxp/pinctrl-s32cc.c +0 -0
        return 0;
 }
Signature · 7 lines
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#11 lore raw

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

Quoted 255 lines · Andy Shevchenko
Quoted 3 lines · Chester Lin

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

Not sure about the need of the casting, see below, otherwise LGTM.

Quoted 247 lines

---
Changes in v2:
- Simply use generic 'struct pinfunction' rather than having extra 'struct s32_pmx_func'.

 drivers/pinctrl/nxp/pinctrl-s32.h   | 26 ++--------
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 76 +++++++++++++++--------------
 2 files changed, 45 insertions(+), 57 deletions(-)
drivers/pinctrl/nxp/pinctrl-s32.h +5 -21
index 545bf16b988d..2f7aecd462e4 100644
@@ -15,31 +15,15 @@ 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 {
-       const char *name;
-       const char **groups;
-       unsigned int num_groups;
-};
-
 /**
  * struct s32_pin_range - pin ID range for each memory region.
  * @start: start pin ID
@@ -56,7 +40,7 @@ struct s32_pinctrl_soc_info {
        unsigned int npins;
        struct s32_pin_group *groups;
        unsigned int ngroups;
-       struct s32_pmx_func *functions;
+       struct pinfunction *functions;
        unsigned int nfunctions;
        unsigned int grp_index;
        const struct s32_pin_range *mem_pin_ranges;
drivers/pinctrl/nxp/pinctrl-s32cc.c +38 -36
index cb8a0844c0fa..4ed0cc905232 100644
@@ -188,7 +188,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,
@@ -198,8 +198,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;
 }
@@ -314,23 +314,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].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;
                }
        }
@@ -364,7 +364,7 @@ static int s32_pmx_get_groups(struct pinctrl_dev *pctldev,
        const struct s32_pinctrl_soc_info *info = ipctl->info;
 
        *groups = info->functions[selector].groups;
-       *num_groups = info->functions[selector].num_groups;
+       *num_groups = info->functions[selector].ngroups;
 
        return 0;
 }
@@ -602,8 +602,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;
@@ -637,9 +637,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);
@@ -732,6 +732,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;
 
@@ -740,38 +741,38 @@ 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,
-                                   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)
+       pins = devm_kcalloc(info->dev, npins, sizeof(*pins), GFP_KERNEL);
+       sss = devm_kcalloc(info->dev, npins, sizeof(*sss), GFP_KERNEL);
+       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;
 }
 
@@ -780,8 +781,9 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
                                        u32 index)
 {
        struct device_node *child;
-       struct s32_pmx_func *func;
+       struct pinfunction *func;
        struct s32_pin_group *grp;
+       char **groups;
        u32 i = 0;
        int ret = 0;
 
@@ -791,18 +793,18 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
 
        /* Initialise function */
        func->name = np->name;
-       func->num_groups = of_get_child_count(np);
-       if (func->num_groups == 0) {
+       func->ngroups = of_get_child_count(np);
+       if (func->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(*func->groups), GFP_KERNEL);
-       if (!func->groups)
+       groups = devm_kcalloc(info->dev, func->ngroups,
+                             sizeof(*func->groups), 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)
@@ -810,6 +812,8 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
                i++;
        }
 
Quoted 3 lines

drivers/pinctrl/nxp/pinctrl-s32cc.c static int s32_pinctrl_parse_functions(struct device_node *np, · line 815

Quoted 1 line
drivers/pinctrl/nxp/pinctrl-s32cc.c +1 -0
+       func->groups = (const char **)groups;

Hmm... Why is casting needed?

It's used for fulfilling the type checking done by kbuild otherwise an error will occur:

drivers/pinctrl/nxp/pinctrl-s32cc.c:815:22: error: assignment to 'const char * const*' from incompatible pointer type 'char **' [-Werror=incompatible-pointer-types]

In 'struct pinfunction', the member 'groups' is declared as (const char * const *).

Regards,
Chester

Quoted 7 lines
Quoted 3 lines
drivers/pinctrl/nxp/pinctrl-s32cc.c +0 -0
        return 0;
 }
Signature · 2 lines
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#12 lore raw

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

Quoted 2 lines · Chester Lin
Quoted 1 line · Andy Shevchenko

On Mon, Mar 20, 2023 at 6:39 PM Chester Lin <[email protected]> wrote:

...

Quoted 3 lines
Quoted 3 lines

drivers/pinctrl/nxp/pinctrl-s32cc.c static int s32_pinctrl_parse_functions(struct device_node *np, · line 807

Quoted 3 lines
        for_each_child_of_node(np, child) {
drivers/pinctrl/nxp/pinctrl-s32cc.c +1 -1
-               func->groups[i] = child->name;
+               groups[i] = (char *)child->name;

Here is also questionable casting.

...

Quoted 9 lines
Quoted 3 lines
Quoted 1 line

+ func->groups = (const char **)groups;

Hmm... Why is casting needed?

It's used for fulfilling the type checking done by kbuild otherwise an error will occur:

drivers/pinctrl/nxp/pinctrl-s32cc.c:815:22: error: assignment to 'const char * const*' from incompatible pointer type 'char **' [-Werror=incompatible-pointer-types]

In 'struct pinfunction', the member 'groups' is declared as (const char * const *).

So, please decouple struct pingroup change to a separate patch and hence struct pinfunction on its own.

After, consider changing types elsewhere that are following the types in that data structures.

Signature · 2 lines
With Best Regards,
Andy Shevchenko