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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Sep 15, 2026 at 07:15:42PM +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.

...

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

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

...

> +#define CV75_FUNCTION(_name)						\

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

I would dare to make it a single line.

...

> +#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.

> +#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>

...

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

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

...

> +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.

> +			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.

> +		}
> +	}
> +
> +	return 0;
> +}

...

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

> +		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.

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

> +		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.

> +	}
> +}

...

> +	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().

...

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

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

...

> +#include <linux/types.h>

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

Not really used. Can be replaced with forward declarations.

> +#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