Hi Andy,

Thanks for reviewing this patch!

On Tue, Mar 07, 2023 at 01:28:09AM +0200, andy.shevchenko@gmail.com wrote:

Mon, Feb 20, 2023 at 10:33:19AM +0800, Chester Lin kirjoitti:

quoted

Add the pinctrl driver for NXP S32 SoC family. This driver is mainly based
on NXP's downstream implementation on nxp-auto-linux repo[1].

Seems Linus already applied this, but still some comments for the future, some
for the followups. However, personally I prefer this to be dropped and redone
because too many things here and there.

quoted

[1] https://github.com/nxp-auto-linux/linux/tree/bsp35.0-5.15.73-rt/drivers/pinctrl/freescale

This can be transformed to Link: tag.

quoted

Signed-off-by: Matthew Nunez <redacted>
Signed-off-by: Phu Luu An <redacted>
Signed-off-by: Stefan-Gabriel Mirea <redacted>
Signed-off-by: Larisa Grigore <redacted>
Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@oss.nxp.com>
Signed-off-by: Andrei Stefanescu <redacted>
Signed-off-by: Radu Pirea <redacted>
Signed-off-by: Chester Lin <redacted>

Is it for real?!
Quite a long chain and none of Co-developed-by.

They are developers who contribute codes and have Signed-off-bys in NXP's downstream
version. Since their implementations can still be seen in this upstream one, I
prefer to list them all. Indeed a part of them who also actively work with me on
upstreaming this driver can be listed as Co-developed-by, but the driver patch
has been merged into the maintainer's for-next so I would not change this part
unless the driver patch needs to be reverted and re-submitted in the end.

Sorry for the patch header that I mess up anyway.

...

quoted

+	depends on ARCH_S32 && OF

Is OF necessary? Can it be

I think it's required since the driver file refers to of_* APIs.

	depends OF || COMPILE_TEST

?

...

quoted

+	depends on ARCH_S32 && OF

Ditto.

...

quoted

+/**
+ * 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
+ */
+struct s32_pin_group {
+	const char *name;
+	unsigned int npins;
+	unsigned int *pin_ids;
+	unsigned int *pin_sss;

Why didn't you embed struct pingroup?

I did think about that but there's an additional 'pin_sss' which could make code
a bit messy. For example:

	s32_regmap_update(pctldev, grp->grp.pins[i],
			  S32_MSCR_SSS_MASK, grp->pin_sss[i]);

quoted

+};
+
+/**
+ * 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 pinfunction.

Thanks for your information. I was not aware of this new struct since it just got
merged recently.

...

quoted

+#ifdef CONFIG_PM_SLEEP
+int __maybe_unused s32_pinctrl_resume(struct device *dev);
+int __maybe_unused s32_pinctrl_suspend(struct device *dev);
+#endif

Please, consider using new PM macros, like pm_ptr().

Maybe pm_sleep_ptr() is more accurate?

...

quoted

+static u32 get_pin_no(u32 pinmux)
+{
+	return (pinmux & S32_PIN_ID_MASK) >> __ffs(S32_PIN_ID_MASK);

Oh, don't you have MASK to be 2^x - 1? Why to drag this with __ffs()?
Just define a complement _SHIFT.

Will fix it.

quoted

+}

...

quoted

+	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);

Use %pOFn instead of %s.

Will change it.

quoted

+	} else if (!n_pins) {
+		return -EINVAL;
+	}

...

quoted

+	for (i = 0; i < grp->npins; ++i) {

Why pre-increment?

Will change that to keep code style consistency.

quoted

+		if (s32_check_pin(pctldev, grp->pin_ids[i]) != 0) {
+			dev_err(info->dev, "invalid pin: %d in group: %d\n",
+				grp->pin_ids[i], group);
+			return -EINVAL;
+		}
+	}
+
+	for (i = 0, ret = 0; i < grp->npins && !ret; ++i) {

Ditto.

quoted

+		ret = s32_regmap_update(pctldev, grp->pin_ids[i],
+					S32_MSCR_SSS_MASK, grp->pin_sss[i]);

Traditional pattern is

		if (ret)
			return ret;

This avoids first assignment of the ret.

quoted

+	}
+
+	return ret;

	return 0;

quoted

+}

...

quoted

+	ret = s32_regmap_read(pctldev, offset, &config);
+	if (ret != 0)
+		return -EINVAL;

Why not

	if (ret)
		return ret;

Will fix this and the following error code shadowings, return handlings and blanks.

Thanks for your reminder.

...

quoted

+	list_add(&(gpio_pin->list), &(ipctl->gpio_configs));

Too many parentheses.

...

quoted

+	list_for_each_safe(pos, tmp, &ipctl->gpio_configs) {
+		gpio_pin = list_entry(pos, struct gpio_pin_config, list);

Why not list_for_each_entry_safe() ?

Will change it.

quoted

+		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);
+			kfree(gpio_pin);
+			break;
+		}
+	}

...

quoted

+static int s32_get_slew_regval(int arg)
+{
+	int i;

	unsigned int i;

+ Blank line.

quoted

+	/* Translate a real slew rate (MHz) to a register value */
+	for (i = 0; i < ARRAY_SIZE(support_slew); i++) {
+		if (arg == support_slew[i])
+			return i;
+	}
+
+	return -EINVAL;
+}

...

quoted

+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (arg)
+			*config |= S32_MSCR_PUS;
+		else
+			*config &= ~S32_MSCR_PUS;

quoted

+		fallthrough;

It's quite easy to miss this and tell us about how is it supposed to work with PU + PD?

I admit that it's ambiguous and should be improved in order to have better readability.

In a S32G2 MSCR register, there are two register bits related to pull up/down functions:

PUE (Pull Enable, MSCR[13]): 0'b: Disabled,  1'b: Enabled
PUS (Pull Select, MSCR[12]): 0'b: Pull Down, 1'b: Pull Up

The dt properties could be like these:

1) 'bias-pull-up' or 'bias-pull-up: true'  --> arg = 1
   In this case both PUE and PUS are set.

2) 'bias-pull-up: false'  --> arg = 0
   In this case both PUE and PUS are cleared since the pull-up function must be disabled.

quoted

+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (arg)
+			*config |= S32_MSCR_PUE;
+		else
+			*config &= ~S32_MSCR_PUE;
+		*mask |= S32_MSCR_PUE | S32_MSCR_PUS;
+		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;

Ditto.

It's similar to the case 'PIN_CONFIG_BIAS_PULL_UP' although the PUS bit is assumed
as 0 via the config variable so only the PUE bit needs to be configured, for example:

1) 'bias-pull-down' or 'bias-pull-down: true'  --> arg = 1
   PUE is set and PUS is cleared.

2) 'bias-pull-down: false'  --> arg = 0
   In this case both PUE and PUS are cleared since the pull-down function must be disabled.

quoted

+	case PIN_CONFIG_BIAS_DISABLE:
+		*config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
+		*mask |= S32_MSCR_PUS | S32_MSCR_PUE;
+		break;

...

quoted

+	if (s32_check_pin(pctldev, pin_id) != 0)

Shadowing an error?

quoted

+		return -EINVAL;

...

quoted

+	ret = s32_regmap_update(pctldev, pin_id, mask, config);
+
+	dev_dbg(ipctl->dev, "update: pin %d cfg 0x%x\n", pin_id, config);
+
+	return ret;

You are not using ret in between, hence

	return _regmap_update(...);

...

quoted

+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);
+
+	if (!ret)
+		seq_printf(s, "0x%x", config);

	int ret;

	ret = s32_regmap_read(pctldev, pin_id, &config);
	if (ret)
		return;

	seq_printf(s, "0x%x", config);

quoted

+}

...

quoted

+	npins = of_property_count_elems_of_size(np, "pinmux", sizeof(u32));

quoted

+

Unneccessary blank line.

quoted

+	if (npins < 0) {
+		dev_err(dev, "Failed to read 'pinmux' property in node %s.\n",
+			np->name);
+		return;
+	}
+	if (!npins) {
+		dev_err(dev, "The group %s has no pins.\n", np->name);
+		return;
+	}

...

quoted

+	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);

quoted

+

Ditto.

quoted

+	if (!grp->pin_ids || !grp->pin_sss) {

quoted

+		dev_err(dev, "Failed to allocate memory for the group %s.\n",
+			np->name);

We do not print ENOMEM error messages.

Will drop it.

quoted

+		return;
+	}

...

quoted

+	func->groups = devm_kzalloc(info->dev,
+			func->num_groups * sizeof(char *), GFP_KERNEL);

First of all, this is devm_kcalloc().
Second, where is the error check?

Will fix it.

quoted

+	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);
+		i++;
+	}

...

quoted

+	ipctl->regions = devm_kzalloc(&pdev->dev,
+				      mem_regions * sizeof(*(ipctl->regions)),
+				      GFP_KERNEL);

devm_kcalloc()

quoted

+	if (!ipctl->regions)
+		return -ENOMEM;

...

quoted

+	info->functions = devm_kzalloc(&pdev->dev,
+				       nfuncs * sizeof(struct s32_pmx_func),
+				       GFP_KERNEL);

Ditto.

quoted

+	if (!info->functions)
+		return -ENOMEM;

...

quoted

+	info->groups = devm_kzalloc(&pdev->dev,
+				    info->ngroups * sizeof(struct s32_pin_group),
+				    GFP_KERNEL);

Ditto.

quoted

+	if (!info->groups)
+		return -ENOMEM;

...

quoted

+	ipctl->pctl = devm_pinctrl_register(&pdev->dev, s32_pinctrl_desc,
+					    ipctl);

quoted

+

Unneeded blank line.

quoted

+	if (IS_ERR(ipctl->pctl)) {

quoted

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

	return dev_err_probe(...);

Will change it, thanks!

quoted

+	}

...

quoted

diff --git a/drivers/pinctrl/nxp/pinctrl-s32g2.c b/drivers/pinctrl/nxp/pinctrl-s32g2.c

Similar issues has to be addressed, if any.

...

quoted

+	return s32_pinctrl_probe
+			(pdev, (struct s32_pinctrl_soc_info *) of_id->data);

Broken indentation.

...

The checkpatch.pl seems not to warn this but I will definitely improve it.

quoted

+static const struct dev_pm_ops s32g_pinctrl_pm_ops = {
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(s32_pinctrl_suspend,
+				     s32_pinctrl_resume)
+};

Consider using DEFINE_* PM macros.

You probably mean DEFINE_SIMPLE_DEV_PM_OPS. Will change it.

...

quoted

+static struct platform_driver s32g_pinctrl_driver = {
+	.driver = {
+		.name = "s32g-siul2-pinctrl",

quoted

+		.owner = THIS_MODULE,

Duplicate assignment.

Will drop it.

quoted

+		.of_match_table = s32_pinctrl_of_match,
+		.pm = &s32g_pinctrl_pm_ops,
+		.suppress_bind_attrs = true,
+	},
+	.probe = s32g_pinctrl_probe,
+};

quoted

+

Unnecessary blank line.

quoted

+builtin_platform_driver(s32g_pinctrl_driver);

-- 
With Best Regards,
Andy Shevchenko