Re: [PATCH v6 10/13] pinctrl: ambarella: add CV75 pin controller

On Fri, Sep 04, 2026 at 02:38:17PM +0800, Long Zhao via B4 Relay wrote:

Add an Ambarella pinmux-only pinctrl driver with CV75 function/group
tables. GPIO is handled by the PL061 driver.

...

+#include <linux/kernel.h>

Absolutely no. Follow IWYU principle in all your patches.

...

+#define CV75_GROUP(_name)						\
+	{								\
+		.name = #_name,						\
+		.pinmux = cv75_##_name##_pinmux,				\
+		.num_pins = ARRAY_SIZE(cv75_##_name##_pinmux),		\
+	}
+
+#define CV75_FUNCTION(_name)						\
+	{								\
+		.name = #_name,						\
+		.groups = cv75_##_name##_groups,				\
+		.num_groups = ARRAY_SIZE(cv75_##_name##_groups),		\
+	}

Don't we have respective macros in linux/pinctrl/pinctrl.h

...

+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/bitmap.h>
+#include <linux/io.h>

+#include <linux/of.h>
+#include <linux/of_device.h>

+#include <linux/slab.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/seq_file.h>

Make it ordered, group linux/pinctrl/ separately after generic linux/*.h

+#include "pinconf.h"
+#include "pinctrl-ambarella.h"

...

+struct amb_pinctrl_soc_data {
+	struct device			*dev;
+	const struct amb_pinctrl_data	*data;
+	void __iomem			*iomux_base;
+	struct regmap			*ds_regmap;
+	struct regmap			*pull_regmap;
+	unsigned int			npins;
+	unsigned long			used[BITS_TO_LONGS(AMBA_MAX_PINS)];
+	raw_spinlock_t lock;
+
+	struct pinctrl_dev		*pctl;

+	const struct ambpin_function	*functions;
+	unsigned int			nr_functions;
+	struct ambpin_group		*groups;
+	unsigned int			nr_groups;

Check what linux/pinctrl/pinctrl.h and drivers/pinctrl/core.h provide.

+};

...

+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static void amb_pin_dbg_show(struct pinctrl_dev *pctldev,
+			     struct seq_file *s, unsigned int pin)
+{
+	seq_printf(s, " %s", pinctrl_dev_get_devname(pctldev));
+}
+#endif

Does this bring any new information?

...

+/* check if the selector is a valid pin function selector */
+static int amb_pinmux_request(struct pinctrl_dev *pctldev, unsigned int pin)
+{
+	struct amb_pinctrl_soc_data *soc = pinctrl_dev_get_drvdata(pctldev);
+
+	if (test_and_set_bit(pin, soc->used))
+		return -EBUSY;
+
+	return 0;
+}
+
+/* check if the selector is a valid pin function selector */
+static int amb_pinmux_free(struct pinctrl_dev *pctldev, unsigned int pin)
+{
+	struct amb_pinctrl_soc_data *soc = pinctrl_dev_get_drvdata(pctldev);
+
+	clear_bit(pin, soc->used);
+
+	return 0;
+}

Why? Isn't internal pinctrl tracking is not enough?

...

+static void amb_pinmux_set_altfunc(struct amb_pinctrl_soc_data *soc,
+				   u32 bank, u32 offset, u32 altfunc)
+{
+	u32 i, data;
+
+	if (!amb_iomux_accessible(soc))
+		return;
+
+	for (i = 0; i < 3; i++) {

	for (unsigned int i...

+		data = readl_relaxed(soc->iomux_base + IOMUX_OFFSET(bank, i));
+		data &= ~BIT(offset);
+		data |= ((altfunc >> i) & 1U) << offset;
+		writel_relaxed(data, soc->iomux_base + IOMUX_OFFSET(bank, i));
+	}
+}

...

+static int amb_drive_strength_to_reg(struct amb_pinctrl_soc_data *soc,
+				     u32 strength)
+{
+	if (soc->data->have_ds2) {
+		switch (strength) {
+		case 3:
+			return 0;
+		case 4:
+		case 5:
+			return 1;
+		case 6:
+			return 2;
+		case 7:
+		case 8:
+			return 3;
+		case 9:
+			return 4;
+		case 12:
+			return 5;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	switch (strength) {
+	case 2:
+		return 0;
+	case 4:
+		return 1;
+	case 8:
+		return 2;
+	case 12:
+		return 3;
+	default:
+		return -EINVAL;
+	}
+}

I would look better in three functions

static int amb_drive_strength_to_reg(struct amb_pinctrl_soc_data *soc,
				     u32 strength)
{
	if (soc->data->have_ds2)
		return amb_drive_strength_to_reg_ds2(soc, strength);

	return amb_drive_strength_to_reg_ds1(soc, strength);

}

+static int amb_reg_to_drive_strength(struct amb_pinctrl_soc_data *soc, u32 ds)
+{
+	static const int ds2_ma[] = { 3, 4, 6, 8, 9, 12 };
+	static const int ds_ma[] = { 2, 4, 8, 12 };

You have switch cases that use the same arrays, use them for both.

+	if (soc->data->have_ds2) {
+		if (ds >= ARRAY_SIZE(ds2_ma))
+			return -EINVAL;
+
+		return ds2_ma[ds];
+	}
+
+	if (ds >= ARRAY_SIZE(ds_ma))
+		return -EINVAL;
+
+	return ds_ma[ds];
+}

...

I stopped here, this driver is quite far from the modern pin control standards,
please look around, you find a lot of new drivers there, that are written in
more-or-less compact ways, using better APIs and approaches.

When creating a driver, use also helper tools, like `pahole`,
scripts/bloat-o-meter, et cetera. Compile with `make W=1 C=1 ...`
using both compilers (GCC and clang).

...

+static const struct of_device_id amb_pinctrl_dt_match[] = {
+	{
+		.compatible = "ambarella,cv75-pinctrl",
+		.data = &ambarella_cv75_pinctrl_data,
+	},
+	{},

No trailing comma for the terminator entry.

+};

...

+static struct platform_driver amb_pinctrl_driver = {
+	.probe	= amb_pinctrl_probe,
+	.driver	= {
+		.name	= "ambarella-pinctrl",
+		.of_match_table = of_match_ptr(amb_pinctrl_dt_match),

Why of_match_ptr()?!

+	},
+};

-- 
With Best Regards,
Andy Shevchenko