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

[zl020895]

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" <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>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
>