Re:Re: [PATCH v7 12/15] pinctrl: ambarella: add CV75 pin controller

Hi Andy,

Thanks for the review. The small comments will be in v8,
including pinconf set via regmap_assign_bits.

PINCTRL_PINGROUP does not fit here: groups carry
AMBA_PINMUX(pin, alt), not a pin-number list, so I will keep
CV75_GROUP.

Best regards,
Long Zhao

At 2026-09-16 18:49:58, "Andy Shevchenko" [off-list ref] wrote:

On Tue, Sep 15, 2026 at 07:15:42PM +0800, Long Zhao via B4 Relay wrote:

quoted

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

...

quoted

+#define CV75_GROUP(_name)						\
+	{								\
+		.name = #_name,						\
+		.mux = cv75_##_name##_pinmux,				\
+		.nmux = ARRAY_SIZE(cv75_##_name##_pinmux),		\
+	}

Can't we use PICTRL_PINGROUP()? Why not?

...

quoted

+#define CV75_FUNCTION(_name)						\

quoted

+	PINCTRL_PINFUNCTION(#_name, cv75_##_name##_groups,		\
+			    ARRAY_SIZE(cv75_##_name##_groups))

I would dare to make it a single line.

...

quoted

+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>

I don't see the need to use errno.h, err.h provides the basic ones.

quoted

+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>

...

quoted

+static void amb_pinmux_set_altfunc(struct amb_pinctrl *ipc, u32 bank,
+				   u32 offset, u32 altfunc)
+{
+	if (bank >= ipc->data->nr_banks)
+		return;
+
+	for (unsigned int i = 0; i < 3; i++) {
+		u32 data;
+
+		data = readl_relaxed(ipc->iomux_base + IOMUX_REG(bank, i));
+		data &= ~BIT(offset);
+		data |= ((altfunc >> i) & 1U) << offset;

	data |= ((altfunc & BIT(i)) >> i) << offset;

Or even

	unsigned long data;
	...
	__assign_bit(offset, &data, altfunc & BIT(i));

quoted

+		writel_relaxed(data, ipc->iomux_base + IOMUX_REG(bank, i));
+	}
+}

...

quoted

+static int amb_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			   unsigned long *configs, unsigned int num_configs)
+{
+	struct amb_pinctrl *ipc = pinctrl_dev_get_drvdata(pctldev);
+	u32 bank = PINID_TO_BANK(pin);
+	u32 offset = PINID_TO_OFFSET(pin);
+	int ret;
+
+	if (bank >= ipc->data->nr_banks)
+		return -EINVAL;
+
+	for (unsigned int i = 0; i < num_configs; i++) {
+		enum pin_config_param param = pinconf_to_config_param(configs[i]);
+		u32 arg = pinconf_to_config_argument(configs[i]);
+		int ds;
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_DISABLE:
+			ret = regmap_update_bits(ipc->pull_regmap,
+						 ipc->data->pull_en[bank], BIT(offset), 0);
+			if (ret)
+				return ret;
+			break;
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+		case PIN_CONFIG_BIAS_PULL_UP:
+			ret = regmap_update_bits(ipc->pull_regmap,
+						 ipc->data->pull_dir[bank], BIT(offset),
+						 (param == PIN_CONFIG_BIAS_PULL_UP) ?
+						 BIT(offset) : 0);

_assign_bits()?
Ditto for the rest of the similar cases.

quoted

+			if (ret)
+				return ret;
+			ret = regmap_update_bits(ipc->pull_regmap,
+						 ipc->data->pull_en[bank], BIT(offset),
+						 BIT(offset));
+			if (ret)
+				return ret;
+			break;
+		case PIN_CONFIG_DRIVE_STRENGTH:
+			ds = amb_drive_strength_to_reg(ipc, arg);
+			if (ds < 0)
+				return ds;
+			if (ipc->data->have_ds2) {
+				ret = regmap_update_bits(ipc->ds_regmap,
+							 ipc->data->ds0[bank], BIT(offset),
+							 (ds & BIT(0)) ? BIT(offset) : 0);
+				if (ret)
+					return ret;
+				ret = regmap_update_bits(ipc->ds_regmap,
+							 ipc->data->ds1[bank], BIT(offset),
+							 (ds & BIT(1)) ? BIT(offset) : 0);
+				if (ret)
+					return ret;
+				ret = regmap_update_bits(ipc->ds_regmap,
+							 ipc->data->ds2[bank], BIT(offset),
+							 (ds & BIT(2)) ? BIT(offset) : 0);
+				if (ret)
+					return ret;
+			} else {
+				ret = regmap_update_bits(ipc->ds_regmap,
+							 ipc->data->ds0[bank], BIT(offset),
+							 (ds & BIT(1)) ? BIT(offset) : 0);
+				if (ret)
+					return ret;
+				ret = regmap_update_bits(ipc->ds_regmap,
+							 ipc->data->ds1[bank], BIT(offset),
+							 (ds & BIT(0)) ? BIT(offset) : 0);
+				if (ret)
+					return ret;
+			}
+			break;
+		default:
+			return -EOPNOTSUPP;

Is it indeed what we use in pin control? I think the correct one here is
ENOTSUPP (and in that case errno.h is required, yes). Yeah, some drivers
has a mixture and they probably didn't get how this error code is used.

quoted

+		}
+	}
+
+	return 0;
+}

...

quoted

+static int amb_pinconf_get(struct pinctrl_dev *pctldev,
+			   unsigned int pin, unsigned long *config)
+{
+	struct amb_pinctrl *ipc = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	u32 bank = PINID_TO_BANK(pin);
+	u32 offset = PINID_TO_OFFSET(pin);
+	u32 pull_en, pull_dir, ds0, ds1, ds2, ds;
+	int ret, strength;
+
+	if (bank >= ipc->data->nr_banks)
+		return -EINVAL;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+	case PIN_CONFIG_BIAS_PULL_UP:
+		ret = regmap_read(ipc->pull_regmap, ipc->data->pull_en[bank],
+				  &pull_en);
+		if (ret)
+			return ret;
+
+		ret = regmap_read(ipc->pull_regmap, ipc->data->pull_dir[bank],
+				  &pull_dir);
+		if (ret)
+			return ret;

quoted

+		pull_en = (pull_en >> offset) & 1;
+		pull_dir = (pull_dir >> offset) & 1;

Seems to me they can be boolean?
In any case, use ' & BIT(offset)' instead of the above.

quoted

+		if (param == PIN_CONFIG_BIAS_DISABLE) {
+			if (pull_en)
+				return -EINVAL;
+			*config = pinconf_to_config_packed(param, 0);
+			return 0;
+		}
+
+		if (!pull_en)
+			return -EINVAL;
+		if (param == PIN_CONFIG_BIAS_PULL_UP && !pull_dir)
+			return -EINVAL;
+		if (param == PIN_CONFIG_BIAS_PULL_DOWN && pull_dir)
+			return -EINVAL;
+
+		*config = pinconf_to_config_packed(param, 1);
+		return 0;
+
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		ret = regmap_read(ipc->ds_regmap, ipc->data->ds0[bank], &ds0);
+		if (ret)
+			return ret;
+
+		ret = regmap_read(ipc->ds_regmap, ipc->data->ds1[bank], &ds1);
+		if (ret)
+			return ret;
+
+		ds0 = (ds0 >> offset) & 1;
+		ds1 = (ds1 >> offset) & 1;
+		if (ipc->data->have_ds2) {
+			ret = regmap_read(ipc->ds_regmap, ipc->data->ds2[bank],
+					  &ds2);
+			if (ret)
+				return ret;
+
+			ds2 = (ds2 >> offset) & 1;
+			ds = (ds2 << 2) | (ds1 << 1) | ds0;
+		} else {
+			ds = (ds0 << 1) | ds1;
+		}

Same here, use BIT(offset). For example,

		ds2 = !!(ds2 & BIT(offset));

quoted

+		strength = amb_reg_to_drive_strength(ipc, ds);
+		if (strength < 0)
+			return strength;
+
+		*config = pinconf_to_config_packed(param, strength);
+		return 0;
+
+	default:
+		return -EOPNOTSUPP;

Same Q about the error code.

quoted

+	}
+}

...

quoted

+	for (unsigned int pin = 0; pin < ipc->data->npins; pin++) {
+		pindesc[pin].number = pin;
+		pindesc[pin].name = devm_kasprintf(ipc->dev, GFP_KERNEL,
+						   "io%u", pin);
+		if (!pindesc[pin].name)
+			return -ENOMEM;
+	}

Use devm_kasprintf_strarray().

...

quoted

+static int amb_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct amb_pinctrl *ipc;
+	int ret;
+
+	ipc = devm_kzalloc(dev, sizeof(*ipc), GFP_KERNEL);
+	if (!ipc)
+		return -ENOMEM;
+
+	ipc->dev = dev;
+	ipc->data = device_get_match_data(dev);
+	if (!ipc->data)
+		return dev_err_probe(dev, -EINVAL, "missing SoC data\n");

-ENODATA

quoted

+	if (!ipc->data->nr_banks || ipc->data->nr_banks > AMBA_MAX_BANKS ||
+	    !ipc->data->npins ||
+	    !ipc->data->groups || !ipc->data->ngroups ||
+	    !ipc->data->functions || !ipc->data->nfunctions)
+		return dev_err_probe(dev, -EINVAL, "invalid SoC data\n");
+
+	ipc->iomux_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ipc->iomux_base))
+		return PTR_ERR(ipc->iomux_base);
+
+	ipc->ds_regmap = syscon_regmap_lookup_by_phandle(dev_of_node(dev),
+					"ambarella,drive-strength-syscon");
+	if (IS_ERR(ipc->ds_regmap))
+		return dev_err_probe(dev, PTR_ERR(ipc->ds_regmap),
+				     "missing drive-strength syscon\n");
+
+	ipc->pull_regmap = syscon_regmap_lookup_by_phandle(dev_of_node(dev),
+					"ambarella,pull-syscon");
+	if (IS_ERR(ipc->pull_regmap))
+		return dev_err_probe(dev, PTR_ERR(ipc->pull_regmap),
+				     "missing pull syscon\n");
+
+	spin_lock_init(&ipc->lock);
+
+	ret = amb_pinctrl_register(ipc);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register pinctrl\n");
+
+	platform_set_drvdata(pdev, ipc);
+
+	return 0;
+}

...

quoted

+#include <linux/types.h>

quoted

+#include <linux/pinctrl/pinctrl.h>

Not really used. Can be replaced with forward declarations.

quoted

+#define AMBA_MAX_BANKS			8
+
+#define AMBA_PINMUX(pin, alt)		(((alt) << 12) | (pin))
+#define AMBA_PINMUX_TO_PIN(mux)		((mux) & 0xfff)
+#define AMBA_PINMUX_TO_ALT(mux)		(((mux) >> 12) & 0x7)
+
+struct amb_pinmux_group {
+	const char *name;
+	const u32 *mux;
+	unsigned int nmux;
+};
+
+struct amb_pinctrl_data {
+	const struct amb_pinmux_group *groups;
+	const struct pinfunction *functions;
+	unsigned int ngroups;
+	unsigned int nfunctions;
+	unsigned int nr_banks;
+	unsigned int npins;
+	unsigned int ds0[AMBA_MAX_BANKS];
+	unsigned int ds1[AMBA_MAX_BANKS];
+	unsigned int ds2[AMBA_MAX_BANKS];
+	unsigned int pull_en[AMBA_MAX_BANKS];
+	unsigned int pull_dir[AMBA_MAX_BANKS];
+	bool have_ds2;
+};
+
+extern const struct amb_pinctrl_data ambarella_cv75_pinctrl_data;

-- 
With Best Regards,
Andy Shevchenko