[PATCH v7 12/15] pinctrl: ambarella: add CV75 pin controller
zl020895
zl020895 at 163.com
Thu Sep 17 02:06:45 PDT 2026
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 at linux.intel.com> 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
>
More information about the linux-arm-kernel
mailing list